feat(api): CORS restrito a origens confiáveis (ENG-1666) - #427
Conversation
…G-1666) - Remove Access-Control-Allow-Origin: * (router e endpoint SSE) - Novo módulo core/cors.ts: allowlist via ALLOWED_ORIGINS (comma-separated) - Echo do Origin apenas para origens confiáveis + Vary: Origin - Wildcard só com opt-in explícito; secure by default sem config - Aplicado a todos os endpoints (preflight, respostas, erros, SSE) - 9 testes de conformidade em core/cors.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6051c8c0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Access-Control-Allow-Origin é emitido — secure by default. | ||
|
|
||
| function getAllowedOrigins(): string[] { | ||
| return (process.env.ALLOWED_ORIGINS ?? "") |
There was a problem hiding this comment.
Configure frontend origins before default-denying CORS
When deploying with the checked-in Fly workflow, neither fly.toml nor packages/api/.env.example configures the new ALLOWED_ORIGINS variable, while the separately deployed dashboard calls https://multiplai.fly.dev (packages/web/.env.production:1). This empty default therefore omits Access-Control-Allow-Origin from every dashboard API and SSE response, causing browsers to block the application unless an untracked Fly secret is manually provisioned; the documented Vite-on-5173 development setup is similarly affected by pages that call port 3000 directly. Add the trusted production/development origins to the deployment and sample configuration, or fail startup when the required setting is absent.
Useful? React with 👍 / 👎.
limaronaldo
left a comment
There was a problem hiding this comment.
Light Review — PR #427 (ENG-1666): CORS restricted to trusted origins
Verdict: ok-to-merge (one MEDIUM hardening gap noted, not blocking)
Summary
corsHeadersFor/addCorsHeaders in packages/api/src/core/cors.ts replace the old always-wildcard CORS_HEADERS object. Default behavior is now secure-by-default (no Access-Control-Allow-Origin emitted unless the request's Origin matches ALLOWED_ORIGINS, with an explicit * opt-in still supported). All four call sites in router.ts (the /api/logs/stream route, the OPTIONS preflight 204, the rate-limit-rejected path, the route-handler success/catch path, and the 404 fallback) were migrated consistently. Access-Control-Allow-Credentials is never set anywhere in this diff or on the pre-existing router.ts, so the classic "reflected origin + credentials" CORS vulnerability class does not apply here.
Findings
-
[MEDIUM]
packages/api/src/core/cors.ts:38-45(addCorsHeaders) — stale/pre-existingAccess-Control-Allow-Originon aResponseis not stripped, only conditionally overwritten.
addCorsHeadersbuildsnewHeadersfrom the existing response headers, then only sets keys thatcorsHeadersForreturns. For a disallowed/absent origin,corsHeadersFornever includesAccess-Control-Allow-Originin its return value, so if aResponseobject already carriedAccess-Control-Allow-Origin: *(e.g., set by a future handler, a library, or a copy-pasted route), that stale value would survive untouched and bypass the new allowlist policy. Confirmed against currentrouter.ts(fetched frommain) that no route handler in this diff's scope currently sets its ownAccess-Control-Allow-Origin— so this is not exploitable via any code path present today — but it's a real defense-in-depth gap since the function's contract ("apply the CORS policy to this response") is not actually enforced; it's merged. Recommend explicitly deletingAccess-Control-Allow-OriginfromnewHeadersbefore applyingcorsHeadersFor's result, and adding a regression test that pre-seeds aResponsewithAccess-Control-Allow-Origin: *to prove the override actually clears it. (Flagged independently by cross-model review viacodex exec.) -
[LOW]
packages/api/src/core/cors.ts:24— comma-split allowlist has no validation of malformed entries.
getAllowedOrigins()blindly splits/trimsALLOWED_ORIGINS; a misconfigured env var (e.g., trailing comma producing an empty string after.filter(Boolean), or a typo'd scheme) fails safely (no match → no header), so this is low risk, but there's no startup-time validation/logging if the env var is misconfigured, which could mask an ops mistake silently. Not a blocker.
Not found
- No wildcard-by-default regression, no null-origin bypass, no credentials/wildcard combination, no missing
Vary: Origin— all handled correctly and covered by the newcors.test.ts(9 tests).
Cross-model review
codex exec -m gpt-5.6-terra reviewed the diff independently and returned a single MEDIUM finding matching the one above (the stale-header bypass), corroborating this analysis. Codex noted its own scope limitation (no local packages/api checkout available), so its review was diff-only static analysis, same constraint as this review.
…ddCorsHeaders (#431) Follow-up to #427. addCorsHeaders re-applied corsHeadersFor via Headers.set, but when the evaluated origin is not allowed no ACAO is computed, so a stale/upstream Access-Control-Allow-Origin (and Allow-Credentials) survived on the response and could leak. Delete both CORS headers before re-applying, guaranteeing at most one correct value and no ACAO for disallowed origins.
Resumo
Access-Control-Allow-Origin: *do router e do endpoint SSE (/api/logs/stream)packages/api/src/core/cors.ts: allowlist via envALLOWED_ORIGINS(comma-separated)Originapenas quando presente na allowlist +Vary: OriginALLOWED_ORIGINS=*); sem config, nenhumAllow-Originé emitido (secure by default)Testes
bun test src/core/cors.test.ts: 9 pass / 0 failbunx tsc --noEmit: limpoCloses ENG-1666