Skip to content

fix(bff): let a key with allowedOrigins serve non-browser clients - #1861

Open
Tonours wants to merge 5 commits into
mainfrom
fix/prd-1102-absent-origin
Open

fix(bff): let a key with allowedOrigins serve non-browser clients#1861
Tonours wants to merge 5 commits into
mainfrom
fix/prd-1102-absent-origin

Conversation

@Tonours

@Tonours Tonours commented Aug 31, 2026

Copy link
Copy Markdown
Member

What

Layer 2 (per-key allowedOrigins) now only rejects an Origin the client actually sent. A request with no Origin header — curl, a Node backend, CI — passes.

One condition in packages/agent-bff/src/cors/per-key-origin.ts: read the header once, skip the allow-list check when the client sent nothing.

fixes PRD-1102

Why

A key created with allowedOrigins: ["http://localhost:4567"] was usable from a browser only. Koa returns '' for an absent header, normalizeOrigin('') returns null, originAllowed then returns false — so every server-side client got 403 origin_not_allowed, with nothing in the error or the docs to explain it. The workaround was provisioning a second, origins-less key.

The Origin is not the boundary for a server client; the API key is. The API key middleware runs before this guard (cli-core.ts) and is the only writer of ctx.state.apiKeyIdentity, so no unauthenticated request reaches the relaxed path.

Scope and safety

  • A present, unlisted Origin still gets 403. Unchanged, still tested.
  • Origin: null (sandboxed iframe, cross-origin redirect) is a present origin and still gets 403. The ticket suggested normalizeOrigin(...) !== null as the skip condition; that would have let the opaque origin through, and a browser can produce it. Asking whether the client sent anything at all keeps every browser-producible origin gated.
  • A present but unparseable Origin still gets 403, as before.
  • Layer 1 (BFF_ALLOWED_ORIGINS, cors-middleware.ts) is untouched — byte-identical to main. The two layers need opposite absent-Origin semantics, which is why the fix is at the caller and not in originAllowed.
  • An empty per-key list stays a no-op; OAuth requests carry no per-key identity and stay unrestricted.

A blank Origin: header counts as absent and passes. That is not a new decision so much as a fact about HTTP: Node strips leading and trailing whitespace from header values, so Origin: and Origin: both reach koa as the empty string, indistinguishable from no header at all. No browser sends either — an opaque origin serializes to the literal string null, which stays rejected.

Documented, not just fixed

bffApiKey in the OpenAPI document said only "a BFF API key". It now carries the rule, so a consumer reading the document alone learns that a key with allowedOrigins gates browsers and not server-side callers, and that Origin: null is rejected. A test pins it against the README.

How to test

yarn workspace @forestadmin/agent-bff test

The no-Origin case in test/cors/per-key-origin.test.ts was flipped from 403 to 200 as the ticket asked, and two route cases were added: Origin: null gets 403, an empty header gets 200. The blank-input contract of hasOrigin is pinned in test/cors/origin.test.ts rather than through the route, since HTTP cannot deliver whitespace to it.

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

@linear-code

linear-code Bot commented Aug 31, 2026

Copy link
Copy Markdown

PRD-1102

@qltysh

qltysh Bot commented Aug 31, 2026

Copy link
Copy Markdown

Qlty


Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (2)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
packages/agent-bff/src/cors/origin.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent-bff/src/cors/per-key-origin.ts100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

Tonours

This comment was marked as off-topic.

@nbouliol nbouliol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core fix looks right, and the deviation from the ticket's suggested normalizeOrigin(...) !== null is the better call — that snippet would have let Origin: null and garbage through, against the ticket's own Expected. Three non-blocking notes below.

Comment thread packages/agent-bff/src/cors/cors-middleware.ts Outdated
Comment thread packages/agent-bff/src/cors/origin.ts Outdated
Comment thread packages/agent-bff/README.md Outdated
Comment thread packages/agent-bff/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants