Skip to content

fix(security): remove localhost CORS origin, consolidate CORS in proxy#4658

Merged
waleedlatif1 merged 6 commits into
stagingfrom
waleedlatif1/csp-localhost-review
May 19, 2026
Merged

fix(security): remove localhost CORS origin, consolidate CORS in proxy#4658
waleedlatif1 merged 6 commits into
stagingfrom
waleedlatif1/csp-localhost-review

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Move all /api/* CORS handling from next.config.ts to proxy.ts so allowed origin is resolved per-request at runtime instead of baked at build time (which produced Access-Control-Allow-Origin: http://localhost:3000 with credentials: true in production)
  • Per-route policy table in proxy covers auth, MCP, form, and workflow execute endpoints; OPTIONS preflight short-circuited uniformly
  • Drop NEXT_PUBLIC_APP_URL build placeholder from Dockerfile (Zod has skipValidation: true; build path doesn't read it)
  • Remove 8 dead route OPTIONS handlers and their preflight tests now that the proxy handles preflight

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented May 19, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 19, 2026 6:42pm

Request Review

@gitguardian
Copy link
Copy Markdown

gitguardian Bot commented May 19, 2026

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@cursor
Copy link
Copy Markdown

cursor Bot commented May 19, 2026

PR Summary

High Risk
High risk because it changes CORS behavior for all API endpoints and centralizes preflight handling in middleware, which can break cross-origin consumers if any policy rule is wrong. Also impacts embedded chat/form auth flows by changing how origins/credentials are set.

Overview
CORS handling is centralized at runtime in proxy.ts for all /api/* routes. A new rule-based resolveApiCorsPolicy applies per-path policies (OAuth2/JWKS/well-known, MCP Copilot, workflow execute, and embedded chat/form identifier routes) and uniformly short-circuits OPTIONS preflights, avoiding Next’s auto-OPTIONS path that would miss middleware headers.

Embedded chat/form API handlers (/api/chat/[identifier]/*, /api/form/[identifier]/*) and related OTP/SSO routes stop calling addCorsHeaders and rely on the proxy for CORS. Multiple explicit OPTIONS exports and their tests are removed from file, MCP, templates, tools image, and form routes; createOptionsResponse is deleted from api/files/utils.

next.config.ts drops build-time /api/* CORS header configuration (including credentialed origins), and the Docker build no longer injects a dummy NEXT_PUBLIC_APP_URL; a new proxy.test.ts covers the CORS policy matrix and the invariant that wildcard origin never pairs with credentials.

Reviewed by Cursor Bugbot for commit 5968f3b. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 19, 2026

Greptile Summary

This PR consolidates all /api/* CORS handling from build-time next.config.ts headers into a runtime middleware policy table in proxy.ts, fixing a production bug where NEXT_PUBLIC_APP_URL was baked at image build time and could produce Access-Control-Allow-Origin: http://localhost:3000 with credentials: true.

  • proxy.ts: Adds resolveApiCorsPolicy with per-route rules (OAuth2/JWKS/MCP → wildcard; embed chat/form paths → reflected origin; workflow execute → wildcard; workspace routes → APP_URL + credentials), a buildPreflightResponse helper that short-circuits OPTIONS uniformly, and applyCorsHeaders that sets Vary: Origin for all non-wildcard responses.
  • Route handlers (chat, form, files, MCP, templates, tools): Removes 8 dead OPTIONS exports and all addCorsHeaders call-sites now that the middleware is the single source of truth.
  • docker/app.Dockerfile: Drops the ARG/ENV NEXT_PUBLIC_APP_URL build placeholder; the proxy reads the value at runtime via getEnv() (dynamic process.env[key] access, bypassing Next.js build-time literal substitution).

Confidence Score: 5/5

Safe to merge. The change removes a production misconfiguration and replaces it with a well-tested runtime policy table with no regressions.

All addCorsHeaders call-sites and dead OPTIONS handlers are cleanly removed. The new resolveApiCorsPolicy covers every previously configured route, the embed path logic correctly guards reserved segments, and getEnv() dynamic lookup correctly reads NEXT_PUBLIC_APP_URL at runtime rather than build time.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/proxy.ts Core of the change: adds CORS policy table, resolveApiCorsPolicy, applyCorsHeaders, and buildPreflightResponse; API early-return correctly short-circuits before security filtering.
apps/sim/proxy.test.ts New test suite with comprehensive coverage: wildcard routes, embed origin reflection, PUT in embed policy, reserved-segment exclusions, workflow execute rule, default credentialed policy, and the wildcard+credentials invariant.
apps/sim/lib/core/security/deployment.ts Removes addCorsHeaders export; CORS now lives in proxy.ts. Clean removal with no residual references.
apps/sim/next.config.ts Strips all /api/* CORS header blocks that baked NEXT_PUBLIC_APP_URL at build time; retains COEP/COOP overrides and security-header blocks for non-API routes.
docker/app.Dockerfile Drops NEXT_PUBLIC_APP_URL build ARG/ENV; safe because proxy.ts uses getEnv() dynamic process.env access which reads the runtime value.

Reviews (4): Last reviewed commit: "fix(cors): allow PUT in embed CORS polic..." | Re-trigger Greptile

Comment thread apps/sim/proxy.ts Outdated
Comment thread apps/sim/proxy.ts
Comment thread apps/sim/proxy.ts
@waleedlatif1 waleedlatif1 changed the base branch from staging to main May 19, 2026 02:32
@waleedlatif1 waleedlatif1 changed the base branch from main to staging May 19, 2026 02:37
Move all /api/* CORS handling from next.config.ts to proxy.ts so the
runtime can resolve allowed origin per-request instead of baking it at
build time (which produced "Access-Control-Allow-Origin: http://localhost:3000"
with credentials:true in production).

- proxy.ts: per-route CORS policy table covering auth, MCP, form, and
  workflow execute endpoints; OPTIONS preflight short-circuit; Vary:
  Origin when origin is not '*'; form routes defer to route handler's
  addCorsHeaders to avoid double-setting
- next.config.ts: drop all /api/* Access-Control-Allow-* headers; keep
  COEP/COOP/CSP
- deployment.ts: addCorsHeaders sets Vary: Origin alongside reflected
  Allow-Origin
- Dockerfile: drop NEXT_PUBLIC_APP_URL build placeholder (Zod has
  skipValidation:true; build path doesn't read it)
- Remove 8 dead OPTIONS handlers and their preflight tests now that the
  proxy handles preflight uniformly
…ruth

Move CORS for /api/chat/* and /api/form/* into the proxy policy table with
reflected-origin + credentials:false, and delete the per-route addCorsHeaders
helper. Routes no longer set CORS headers — the proxy is the only writer.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… tests

Replace the if/else chain in resolveApiCorsPolicy with a CORS_RULES table
so each route's policy lives in one place and is trivially scannable.
Add proxy.test.ts covering each rule and the wildcard-with-credentials
invariant.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/proxy.ts
The embed policy (reflected origin, credentials:false) was matching
workspace-internal session-authed routes — /api/chat, /api/chat/manage/*,
/api/chat/validate, and the form equivalents — which need the default
credentialed policy. Tighten the matcher to the embed paths only and add
tests covering the exclusion.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The regex form `^/api/(chat|form)/(?!manage|validate)[^/]+(/(otp|sso))?$`
was opaque on review and would silently exclude any future identifier
subroute outside the hard-coded (otp|sso) group from the embed policy.
Replace it with an imperative segment check and a named
EMBED_RESERVED_SEGMENTS Set, so the policy boundary is visible at the
top of the function and adding a reserved subpath is a one-line diff.
Add a test asserting that future identifier subroutes also get the
embed policy.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/proxy.ts
Comment thread apps/sim/proxy.ts
Both /api/chat/[identifier]/otp and /api/form/[identifier]/otp export
PUT for OTP code verification. The embed policy advertised only
GET/POST/OPTIONS, so cross-origin embed clients failed preflight on
verify. Add PUT and assert it in the embed policy test.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 5968f3b. Configure here.

@waleedlatif1 waleedlatif1 merged commit ef14b2b into staging May 19, 2026
14 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/csp-localhost-review branch May 19, 2026 19:04
waleedlatif1 added a commit that referenced this pull request May 19, 2026
#4665)

* fix(docker): restore NEXT_PUBLIC_APP_URL build arg with dummy fallback

getBaseUrl() in lib/core/utils/urls is evaluated at module load during
next build's page-data collection and throws if NEXT_PUBLIC_APP_URL is
unset. PR #4658 removed the build arg, breaking the Docker build at the
"/_not-found" page-data collection step.

Restore the dummy localhost fallback (mirroring DATABASE_URL). The CORS
fix from #4658 is preserved: next.config.ts no longer reads
NEXT_PUBLIC_APP_URL at build time, and no module-level expression
captures getBaseUrl() — every caller invokes it at request time, where
getEnv() reads the deployed container env. The dummy localhost value
cannot leak into runtime CORS response headers.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* chore(docker): trim verbose comment on build-time env args

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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.

1 participant