fix(server): restrict CORS to same host:port as the server (P1) - #1119
fix(server): restrict CORS to same host:port as the server (P1)#1119wjc2821296948 wants to merge 2 commits into
Conversation
`app.use(cors({ exposedHeaders: [...] }))` invoked the `cors` package with no
`origin` option, so the package reflected the request's `Origin` header back
unchanged in `Access-Control-Allow-Origin` for every cross-origin request.
Combined with the fact that most `/api` routes are only protected by a
bearer JWT that the client keeps in localStorage, any malicious site a
victim visits in the same browser could read responses from the server on
the victim's behalf by issuing requests with the victim's token.
Replace the default reflector with a callback that only allows the origin
through when its host:port matches the server's own host:port. Same-origin
requests (no Origin header) continue to be allowed through. Wildcard binds
(0.0.0.0/::) accept any host on the configured port, which preserves the
LAN-hosted use case while still refusing unrelated public origins.
Move `SERVER_PORT` / `HOST` / `DISPLAY_HOST` / `VITE_PORT` declarations above
the CORS middleware so the reflector can read them at module load time.
Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
Severity summaryP1 — CORS reflects any This PR is split out from the previous umbrella PR (#1106) per @blackmammoth's request that each finding be reviewed in isolation. 🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe server now defines host and port constants near startup. CORS validates origins against the configured host and port, handles loopback and wildcard bindings, and rejects malformed or mismatched origins. Duplicate declarations were removed. ChangesCORS configuration
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/index.ts (1)
133-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an origin-policy test matrix.
Cover no
Origin, approved host and port, unrelated host, malformed origin, wildcard binds, loopback aliases, Vite development origins, default ports, scheme differences, and credential response headers. Assert both callback decisions and actual middleware response headers.This change controls access to authenticated response data.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/index.ts` around lines 133 - 157, Add a focused test matrix for corsOriginReflector and the configured CORS middleware, covering missing Origin, matching and mismatching hosts/ports, malformed values, wildcard binds, loopback aliases, Vite origins, implicit default ports, scheme differences, and credential-related response headers. Assert both the reflector callback result and actual middleware response headers, including authenticated responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/index.ts`:
- Around line 145-148: Replace the request matching logic around
serverHost/serverPort with an explicit trusted-origin allowlist for advertised
browser-facing URLs; do not treat HOST values 0.0.0.0 or :: as wildcard origins.
Canonicalize loopback and LAN aliases in that allowlist, including localhost
versus 127.0.0.1, and have corsOriginReflector approve origins only when they
match the configured trusted entries on SERVER_PORT.
- Around line 159-162: Update the CORS options passed to app.use(cors(...)) in
the server setup to enable credentials by setting the credentials option to
true, while preserving the existing origin reflector and exposed headers.
---
Nitpick comments:
In `@server/index.ts`:
- Around line 133-157: Add a focused test matrix for corsOriginReflector and the
configured CORS middleware, covering missing Origin, matching and mismatching
hosts/ports, malformed values, wildcard binds, loopback aliases, Vite origins,
implicit default ports, scheme differences, and credential-related response
headers. Assert both the reflector callback result and actual middleware
response headers, including authenticated responses.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Two CodeRabbit follow-ups on the CORS hardening: 1. The previous reflector treated `HOST === '0.0.0.0'` or `HOST === '::'` as "accept any host on `SERVER_PORT`". That conflates the listen address (what the kernel binds to) with the trust boundary (what origins we are willing to serve). Under the default `0.0.0.0` listen a request from `http://evil.example.com:3001` was accepted, which the `cors` package then reflected as `Access-Control-Allow-Origin` (combined with credentials: true from the follow-up below, that would have completed the cross-origin session-theft chain). Replace the HOST-derived comparison with an explicit allowlist of trusted origins built from `getConnectableHost(HOST)`, canonicalizing loopback aliases (`127.0.0.1` ↔ `localhost` ↔ `::1`) so a deployment that binds on `127.0.0.1` still accepts `http://localhost:3001` and vice versa. The listen address never appears in the trust set on its own; wildcard binds expand only into the loopback aliases. 2. The previous `cors({ origin, exposedHeaders })` configuration did not set `credentials: true`, so `Access-Control-Allow-Credentials` was never emitted and the browser refused to send cookies on cross-origin requests — defeating the auth-flow integration that the cookie middleware is otherwise designed to support. Add `credentials: true` so credentialed cross-origin requests from trusted origins are correctly answered. Co-authored-by: cgsdn <chaogeshuodiannao@users.noreply.github.com>
|
P1 — CORS reflects any
OriginheaderVulnerability description
app.use(cors({ exposedHeaders: [...] }))inserver/index.tswas invoked with nooriginoption. Thecorspackage's default behaviour whenoriginis unset is to reflect whateverOriginheader the request carries back asAccess-Control-Allow-Origin, and to setAccess-Control-Allow-Credentials: truebecause the route layer also sendsSet-Cookie. Combined, this lets any third-party site that the victim visits read every response from this server (including the/api/auth/*JSON bodies and any cookie-bearing endpoints) under the victim's authenticated session.Fix
Replace the unset
originoption with a custom origin callback that only allows the configured server host and port (http://<SERVER_HOST>:<SERVER_PORT>). Wildcard binds (e.g.0.0.0.0,::) accept any host on the configured port so a LAN peer that resolves the server's hostname can still connect; explicit binds accept only the bound host. Requests without anOriginheader (e.g. same-origin navigations, server-to-server calls) are still allowed.Files
server/index.tsCommits
b497c37—fix(server): restrict CORS to same host:port as the serverCode snippet
🤖 Generated with Claude Code
Summary by CodeRabbit