diff --git a/packages/agent-bff/README.md b/packages/agent-bff/README.md index ab545eea70..1f033da571 100644 --- a/packages/agent-bff/README.md +++ b/packages/agent-bff/README.md @@ -142,8 +142,11 @@ normalized away, no trailing slash, no wildcard, no subdomain matching): blocks). Applies to `POST /oauth/token` too. Preflight `OPTIONS` from an allow-listed origin gets the allowed methods + headers; credentials are never enabled. - **Layer 2 (per-key authorization, Mode 2 only)** — when the resolved key has a non-empty - `allowedOrigins`, the request `Origin` must also be in that list (a missing `Origin` is rejected), - else `403 origin_not_allowed`. An empty per-key list is a no-op. + `allowedOrigins`, an `Origin` sent by the client must be in that list, else + `403 origin_not_allowed`. A request with no `Origin` at all — a server-side client such as curl, + a Node backend or CI, an empty header counting as none — passes: there the API key is the + boundary, not the origin. An opaque `Origin: null` (sandboxed iframe, cross-origin redirect) is a + present origin and is rejected. An empty per-key list is a no-op. **Local development:** browsers still enforce CORS against `localhost`, so add your dev origin(s) to `BFF_ALLOWED_ORIGINS` (e.g. `BFF_ALLOWED_ORIGINS=http://localhost:4200`) — there is no dev bypass. diff --git a/packages/agent-bff/src/cors/origin.ts b/packages/agent-bff/src/cors/origin.ts index 967c62cf10..784040208c 100644 --- a/packages/agent-bff/src/cors/origin.ts +++ b/packages/agent-bff/src/cors/origin.ts @@ -1,8 +1,12 @@ +export function hasOrigin(raw: string | undefined | null): raw is string { + return raw !== undefined && raw !== null && raw.trim() !== ''; +} + export function normalizeOrigin(raw: string | undefined | null): string | null { - if (raw === undefined || raw === null) return null; + if (!hasOrigin(raw)) return null; const trimmed = raw.trim(); - if (trimmed === '' || trimmed === 'null') return null; + if (trimmed === 'null') return null; let url: URL; diff --git a/packages/agent-bff/src/cors/per-key-origin.ts b/packages/agent-bff/src/cors/per-key-origin.ts index 7f6fcfc7ee..a3f1656d68 100644 --- a/packages/agent-bff/src/cors/per-key-origin.ts +++ b/packages/agent-bff/src/cors/per-key-origin.ts @@ -1,14 +1,15 @@ import type { Middleware } from 'koa'; -import { originAllowed } from './origin'; +import { hasOrigin, originAllowed } from './origin'; import { originNotAllowed } from '../http/bff-http-error'; export default function createPerKeyOriginMiddleware(): Middleware { return async function perKeyOriginMiddleware(ctx, next) { const identity = ctx.state.apiKeyIdentity as { allowedOrigins?: string[] } | undefined; const allowedOrigins = identity?.allowedOrigins ?? []; + const origin = ctx.get('Origin'); - if (allowedOrigins.length > 0 && !originAllowed(ctx.get('Origin'), allowedOrigins)) { + if (allowedOrigins.length > 0 && hasOrigin(origin) && !originAllowed(origin, allowedOrigins)) { throw originNotAllowed(); } diff --git a/packages/agent-bff/src/openapi/openapi-document.ts b/packages/agent-bff/src/openapi/openapi-document.ts index 8103504403..3f86a5a491 100644 --- a/packages/agent-bff/src/openapi/openapi-document.ts +++ b/packages/agent-bff/src/openapi/openapi-document.ts @@ -401,7 +401,13 @@ export function generateOpenApiDocument( type: 'apiKey', in: 'header', name: 'X-Forest-Bff-Key', - description: 'Mode 2: a BFF API key. Never send both this and an Authorization header.', + description: + 'Mode 2: a BFF API key. Never send both this and an Authorization header. A key created ' + + 'with allowedOrigins restricts browser callers only: an Origin the client sends must be in ' + + 'that list, else 403 origin_not_allowed, while a request with no Origin at all — curl, a ' + + 'server-side client, CI, an empty Origin header counting as none — passes, because there ' + + 'the key is the boundary rather than the origin. The opaque Origin null is a present origin ' + + 'and is rejected.', }); registry.registerPath({ diff --git a/packages/agent-bff/test/cors/origin.test.ts b/packages/agent-bff/test/cors/origin.test.ts index 94d912888b..6f3827f3b8 100644 --- a/packages/agent-bff/test/cors/origin.test.ts +++ b/packages/agent-bff/test/cors/origin.test.ts @@ -1,4 +1,26 @@ -import { normalizeOrigin, originAllowed, parseAllowedOrigins } from '../../src/cors/origin'; +import { + hasOrigin, + normalizeOrigin, + originAllowed, + parseAllowedOrigins, +} from '../../src/cors/origin'; + +describe('hasOrigin', () => { + it('is false for the empty string koa returns when the header is absent', () => { + expect(hasOrigin('')).toBe(false); + }); + + it('is true for any present value, even an opaque or malformed one', () => { + expect(hasOrigin('null')).toBe(true); + expect(hasOrigin('not a url')).toBe(true); + }); + + it('is false for blank or absent input, the same contract normalizeOrigin takes', () => { + expect(hasOrigin(' ')).toBe(false); + expect(hasOrigin(undefined)).toBe(false); + expect(hasOrigin(null)).toBe(false); + }); +}); describe('normalizeOrigin', () => { it.each([ diff --git a/packages/agent-bff/test/cors/per-key-origin.test.ts b/packages/agent-bff/test/cors/per-key-origin.test.ts index f056ddb8ec..fb22ebe843 100644 --- a/packages/agent-bff/test/cors/per-key-origin.test.ts +++ b/packages/agent-bff/test/cors/per-key-origin.test.ts @@ -21,6 +21,17 @@ function buildApp(allowedOrigins?: string[]) { return app; } +function expectOriginForbidden(response: request.Response) { + expect(response.status).toBe(403); + expect(response.body).toEqual({ + error: { + type: 'origin_not_allowed', + status: 403, + message: 'Origin is not allowed for this key', + }, + }); +} + describe('per-key origin middleware (layer 2)', () => { it('proceeds when the origin is in the per-key list', async () => { const response = await request(buildApp(['https://a.com']).callback()) @@ -36,10 +47,7 @@ describe('per-key origin middleware (layer 2)', () => { .get('/agent/x') .set('Origin', 'https://b.com'); - expect(response.status).toBe(403); - expect(response.body).toEqual({ - error: { type: 'origin_not_allowed', status: 403, message: expect.any(String) }, - }); + expectOriginForbidden(response); }); it('matches a per-key origin returned by SaaS in a non-normalized form', async () => { @@ -58,11 +66,36 @@ describe('per-key origin middleware (layer 2)', () => { expect(response.status).toBe(200); }); - it('returns 403 when the per-key list is non-empty and the request has no Origin', async () => { + it('proceeds when the per-key list is non-empty and the request has no Origin', async () => { const response = await request(buildApp(['https://a.com']).callback()).get('/agent/x'); - expect(response.status).toBe(403); - expect(response.body.error.type).toBe('origin_not_allowed'); + expect(response.status).toBe(200); + expect(response.body).toEqual({ reached: true }); + }); + + it('returns 403 when the per-key list is non-empty and the Origin is the opaque null', async () => { + const response = await request(buildApp(['https://a.com']).callback()) + .get('/agent/x') + .set('Origin', 'null'); + + expectOriginForbidden(response); + }); + + it('returns 403 when the per-key list is non-empty and the Origin is present but not a parseable origin', async () => { + const response = await request(buildApp(['https://a.com']).callback()) + .get('/agent/x') + .set('Origin', 'garbage'); + + expectOriginForbidden(response); + }); + + it('treats an empty Origin header like an absent one and proceeds', async () => { + const response = await request(buildApp(['https://a.com']).callback()) + .get('/agent/x') + .set('Origin', ''); + + expect(response.status).toBe(200); + expect(response.body).toEqual({ reached: true }); }); it('does not restrict an oauth request, which carries no per-key identity', async () => { diff --git a/packages/agent-bff/test/openapi/openapi-document.test.ts b/packages/agent-bff/test/openapi/openapi-document.test.ts index eb62e23b72..341419c34d 100644 --- a/packages/agent-bff/test/openapi/openapi-document.test.ts +++ b/packages/agent-bff/test/openapi/openapi-document.test.ts @@ -197,6 +197,16 @@ describe('generateOpenApiDocument', () => { expect(session.description).toContain('every data and action route'); }); + it('should say in the api key scheme that allowedOrigins gates browsers only', () => { + const apiKey = (document.components?.securitySchemes as Record) + .bffApiKey; + + expect(apiKey.description).toContain('restricts browser callers only'); + expect(apiKey.description).toContain('a request with no Origin at all'); + expect(apiKey.description).toContain('an empty Origin header counting as none'); + expect(apiKey.description).toContain('opaque Origin null is a present origin and is rejected'); + }); + it('should require a body where parentId or recordIds is mandatory', () => { const requiredByPath = Object.fromEntries( Object.entries(document.paths ?? {})