feat(agent-bff): log origin guard rejections - #1868
Conversation
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (2)
🛟 Help
|
Tonours
left a comment
Tonours
left a comment
There was a problem hiding this comment.
Spec (PRD-1114): conforms. Both guards emit one Warn per rejection carrying identifiers only (origin, path, renderingId where the identity exists), wired in cli-core.ts, pinned by tests including the zero-log pass paths.
Applies to: the PR's scope. Preferential. Layer-1 telemetry fires for preflights only, as the ticket scopes it, so three rejection shapes still leave no server-side trace: OAuth requests with a disallowed origin (no per-key identity), API keys with an empty allowedOrigins list, and origins allowed per-key but rejected by BFF_ALLOWED_ORIGINS. For a non-preflight request the browser then blocks the response after the server has run it, with nothing in the logs. Pre-existing and out of this ticket's scope; a follow-up could log layer-1 disallowed origins on simple requests too, distinguishing preflight from actual request.
| ctx.set('Access-Control-Allow-Headers', ALLOWED_HEADERS); | ||
| ctx.set('Access-Control-Max-Age', String(PREFLIGHT_MAX_AGE_SECONDS)); | ||
| } else if (origin) { | ||
| logger('Warn', 'BFF preflight origin rejected', { origin, path: ctx.path }); |
There was a problem hiding this comment.
Preferential. The preflight classification keys on OPTIONS + Origin only. A spec-strict preflight also carries Access-Control-Request-Method (fetch spec), so a plain OPTIONS probe with a forwarded Origin (curl, monitoring) is logged as a preflight rejection. An operator grepping logs then hunts a browser preflight problem that never happened. Consider keying the classification on Access-Control-Request-Method as well. The existing preflight tests in this suite use OPTIONS + Origin without it, so this matches the established test contract; flagged as hardening, not a defect.
There was a problem hiding this comment.
Fixed in faeda53: the rejection log now requires Access-Control-Request-Method alongside OPTIONS + Origin, so a plain OPTIONS probe with only an Origin stays silent. A test pins that shape.
| return async function perKeyOriginMiddleware(ctx, next) { | ||
| const identity = ctx.state.apiKeyIdentity as { allowedOrigins?: string[] } | undefined; | ||
| const origin = ctx.get('Origin'); | ||
| const identity = ctx.state.apiKeyIdentity as |
There was a problem hiding this comment.
permissions-routes-middleware.ts:38 already casts this same state slot to ResolvedApiKeyIdentity — reusing it keeps one shape, and lets identity.renderingId drop the ?.
| logger('Warn', 'BFF per-key origin rejected', { | ||
| origin, | ||
| path: ctx.path, | ||
| renderingId: identity?.renderingId, |
There was a problem hiding this comment.
renderingId is not key-unique and the console logger emits no request id, so this can't be joined back to the Resolved BFF API key line. What about keyHash: fingerprintApiKey(ctx.get(BFF_KEY_HEADER)) alongside, like api-key-middleware.ts does?
2 participants
fixes PRD-1114
What
Both CORS guards in agent-bff now log one
Warnper rejection. Layer 2 (per-key origin) logs{ origin, path, renderingId }right before the 403. Layer 1 logs{ origin, path }when apreflight carries an origin that is not allow-listed. Identifiers only, never the key or the
payload, per the logging convention. Both middleware factories take a required
loggeroption, wired from
cli-core.ts.Why
Until now every
403 origin_not_allowedand every rejected preflight left zero server-sideevidence. The browser reports a generic "blocked by CORS policy" and the BFF logs nothing, so
an operator troubleshooting a broken
allowedOriginssetting had nothing to start from. Thisis the most common operator-facing failure of this stack.
Scope and safety
No behavior change on pass paths, no change to response bodies or status codes. The signature
change stays internal: neither middleware is exported from the package barrel, and the three
test call sites move in the same commit. There is no dedup or rate limit on the log; a
misconfigured deployment emits one Warn per rejected request, which is the telemetry this
ticket asks for.
How to test
yarn workspace @forestadmin/agent-bff test: 79 suites, 1353 tests, green.yarn buildat the repo root: tsc over 24 projects, green.The rejection and pass behaviors are pinned by the supertest suites in
test/cors/. I did not drive a live server by hand; the suites mount the real Koa middlewarestack, which covers the same path.
Definition of Done
General
Security