Skip to content

[Security 3/9] Production CORS allowlist with fail-safe denial - #245

Merged
willchen96 merged 1 commit into
Open-Legal-Products:mainfrom
amal66:olp-pr/sec-cors
Aug 2, 2026
Merged

[Security 3/9] Production CORS allowlist with fail-safe denial#245
willchen96 merged 1 commit into
Open-Legal-Products:mainfrom
amal66:olp-pr/sec-cors

Conversation

@amal66

@amal66 amal66 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

[Security 3/9] Production CORS allowlist with fail-safe denial

Part of the split of #227 into single-topic PRs. Index: tracking comment on #227.

TL;DR

Replace the single-origin CORS reflection with an explicit allowlist and fail-safe defaults: unknown origins get no Access-Control-Allow-Origin header (the browser blocks the response) instead of an error, preflight stays a clean 2xx/4xx, and approved request headers/methods are enumerated.

Risk to user data

Severity: medium–high. The browser's same-origin policy is what stops evil.example from reading our API's responses using a logged-in visitor's cookies/credentials. CORS is a controlled relaxation of that policy. A server that reflects any Origin back — a common dev shortcut — has relaxed it for every site on the internet, so a malicious page a user visits could read their documents, chats, and account data out of our API.

Flows affected

  • Every browser-originated API request (app.ts CORS middleware).
  • Preflight (OPTIONS) handling for all routes.
  • No change for the legitimate frontend origin.

Attack precedent

Reflected-origin and overly-permissive CORS (Access-Control-Allow-Origin: * with credentials, or naive origin echoing) is a staple finding. See PortSwigger's CORS labs for the credential-theft chains this enables.

Possible fixes, and what we chose

Option Verdict
Reflect Origin back The insecure default we're removing — allows every site.
origin: FRONTEND_URL (single string) The prior state. Works but rigid, and doesn't set headers/methods explicitly.
Throw an Error for disallowed origins Rejected — the cors package propagates the throw to Express's default handler, turning every disallowed cross-origin request (including preflight) into an HTTP 500. Noisy, and a 500 on preflight is a worse signal than a clean deny.
Allowlist Set, callback resolves false for unknown origins Chosen. Disallowed origin → header simply omitted → browser blocks the read, no 5xx. Server-to-server calls (no Origin) still allowed.
sequenceDiagram
    participant B as Browser on evil.example
    participant S as Mike backend
    B->>S: request with Origin: https://evil.example
    S->>S: allowedOrigins.has(origin)? → false
    S-->>B: response WITHOUT Access-Control-Allow-Origin
    Note over B: Same-origin policy kicks in →<br/>browser refuses to expose the body to JS
Loading

Fail-safe detail (in-code comment): the callback returns callback(null, false) rather than callback(new Error()), precisely so a disallowed origin is a silent deny, not a 500. Approved headers are limited to Authorization / Content-Type, and methods are enumerated.

What's in this PR

  • backend/src/app.ts — allowlist Set + fail-safe origin callback + explicit headers/methods.
  • Tests: cors.test.ts (4 integration tests: allowed origin reflected with credentials, disallowed origin omitted, disallowed origin is not a 5xx, header allowlist enforced).

Ported from #227's index.ts change onto the post-refactor app.ts (the Express app moved during the integration-test extraction on main). The 4 tests are new, written against main's integration harness.

Reading

MDN: CORS · PortSwigger: CORS misconfigurations

@CLAassistant

CLAassistant commented Jul 23, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Ported from the security pack's index.ts change to the post-refactor
app.ts (the Express app moved in the integration-test extraction).
Adapted-from: Open-Legal-Products#227 (4c44c15, CORS half)
@amal66
amal66 force-pushed the olp-pr/sec-cors branch from b1c82aa to 3068002 Compare July 25, 2026 21:31
@amal66
amal66 requested a review from willchen96 July 26, 2026 14:09

@willchen96 willchen96 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

@willchen96
willchen96 merged commit 961457c into Open-Legal-Products:main Aug 2, 2026
5 checks passed
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.

3 participants