Skip to content

Enforce env-driven CORS origin allow-list on /graphql, /graphql/stream, and /export - #25

Merged
charles2ke merged 7 commits into
mainfrom
copilot/fix-cors-configuration
Sep 16, 2026
Merged

charles2ke merged 7 commits into
mainfrom
copilot/fix-cors-configuration

Conversation

Copilot AI commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

All three HTTP mounts used bare cors(), emitting Access-Control-Allow-Origin: *. Combined with GET /graphql/stream taking query from the query string — a CORS "simple" request with no preflight — any page could read GraphQL results from a reachable instance. The allow-list was only documented in the README, never enforced.

Changes

  • src/config/cors.js (new) — loadCorsOptions(env) builds the cors() options from CORS_ALLOWED_ORIGINS (comma-separated). An allowed Origin is echoed back verbatim; anything else gets no CORS headers, so the browser blocks the response. The wildcard is never emitted.
    • Deny-by-default: unset variable means no cross-origin reads. Deployments with a browser front-end on another origin must set it.
    • Requests without an Origin header (curl, server-to-server) pass through untouched.
  • src/index.js — builds the options once and applies them to /export, /graphql/stream, and /graphql.
  • README.md — CORS section now documents the actual mechanism (env var + wiring) instead of stating an expectation; CORS_ALLOWED_ORIGINS added to the config table.
  • test/cors.test.js (new) — allow-list parsing, allowed vs. denied origin headers, missing Origin, and the deny-all default.
CORS_ALLOWED_ORIGINS='https://app.example.com,https://admin.example.com' npm start

curl -si -H 'Origin: https://app.example.com' http://localhost:4000/export
# Access-Control-Allow-Origin: https://app.example.com

curl -si -H 'Origin: https://evil.example' http://localhost:4000/export
# (no Access-Control-Allow-Origin header)

Note the deny-by-default is a behavior change for any existing browser client relying on the previous wildcard; those origins need to be listed explicitly.

Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix CORS middleware configuration for security Enforce env-driven CORS origin allow-list on /graphql, /graphql/stream, and /export Sep 16, 2026
Copilot AI requested a review from charles2ke September 16, 2026 05:06
@charles2ke
charles2ke marked this pull request as ready for review September 16, 2026 05:34
@charles2ke
charles2ke requested a lite review from Copilot September 16, 2026 05:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Production-route integration coverage and wildcard rejection remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Enforces an environment-driven, deny-by-default CORS allow-list across the public HTTP mounts.

Changes:

  • Adds reusable CORS option parsing.
  • Applies CORS to /export, /graphql/stream, and /graphql.
  • Updates documentation and adds CORS tests.
File summaries
File Summary Review findings
test/cors.test.js Tests parsing and origin behavior. Moderate: add integration coverage for allowed and denied origins on all three production mounts (2 votes).
src/index.js Applies shared CORS configuration to all three mounts. Moderate: test the production app or an extracted app factory to prevent middleware regressions (1 vote).
src/config/cors.js Builds deny-by-default CORS options. Nit: reject wildcard entries and add a regression test (1 vote).
README.md Documents configuration and defaults. No findings.
Review details

Suppressed comments (2)

src/config/cors.js:13

  • Filtering only blanks leaves * as a permitted entry. With CORS_ALLOWED_ORIGINS='*' and an Origin: * request, the callback returns ['*'], which the cors middleware treats as a matching origin and can emit Access-Control-Allow-Origin: *, contradicting the documented invariant that the wildcard is never emitted. Reject wildcard entries and add a regression test for this configuration.
    .filter(Boolean);

src/index.js:49

  • The new test exercises loadCorsOptions only on a throwaway /probe app; it never starts the application wired in src/index.js. Reverting any of these three mounts to bare cors() would therefore leave the tests green while reintroducing the vulnerability. Please add an integration test around the production app (or an extracted app factory) that checks allowed and denied origins on /graphql, /graphql/stream, and /export.
    cors(corsOptions),
    createExportRouter({ store, finance: financeService, logger })
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/cors.test.js
Copilot AI and others added 5 commits September 16, 2026 05:42
Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
…sert status

Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
…igin header

Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
@charles2ke
charles2ke merged commit f0ce719 into main Sep 16, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment