fix(cors): reject wildcard origin with credentials#366
Conversation
|
@nice-bills Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughA CORS security hardening that validates and enforces explicit origin allowlists, rejecting wildcard origins and requiring URL normalization. Environment configuration, parsing utilities, middleware refactoring, and comprehensive test coverage implement credential-compatible CORS. ChangesCORS Wildcard Rejection and Explicit Allowlist
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens CORS configuration by replacing the permissive origin handling with a validated allowlist that rejects wildcards (incompatible with credentials) and requires explicit origins in production.
Changes:
- Adds
parseCorsOriginsto validate, normalize, and deduplicateCORS_ORIGIN, with environment-specific defaults and wildcard rejection. - Refactors CORS middleware into
createCorsMiddleware+resolveCorsOriginhelpers that reflect exact allowed origins. - Adds unit/integration tests and updates
.env.exampledocumentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config/corsOrigins.ts | New parser with validation, normalization, and production safeguards. |
| src/config/env.ts | Wires parseCorsOrigins into config and removes unsafe split. |
| src/middleware/cors.ts | Splits middleware into testable helpers; reflects allowed origin. |
| tests/cors.test.ts | New tests for parser, resolver, and middleware behavior. |
| tests/env.test.ts | Adds wildcard rejection test for CORS_ORIGIN. |
| .env.example | Documents new CORS_ORIGIN format and defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `Invalid CORS origin "${origin}". Only http:// and https:// origins are supported.`, | ||
| ); | ||
| } | ||
| if (url.username || url.password || url.pathname !== "/" || url.search || url.hash) { |
| await request(buildApp([allowedOrigin])) | ||
| .get("/ping") | ||
| .set("Origin", "https://evil.example.com") | ||
| .expect(500); |
| // CORS | ||
| corsOrigin: process.env.CORS_ORIGIN?.split(",") || [], | ||
| // CORS — explicit origins only; wildcard * is rejected (incompatible with credentials) | ||
| corsOrigin: parseCorsOrigins(process.env.CORS_ORIGIN, env.NODE_ENV), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/config/corsOrigins.ts`:
- Around line 11-18: The NODE_ENV check is case-sensitive and can misclassify
production; update the logic around nodeEnv (used with trimmed and
DEFAULT_DEV_CORS_ORIGIN) to normalize nodeEnv (e.g., nodeEnv?.toLowerCase()) or
perform a case-insensitive comparison before deciding to throw the production
Error — this ensures values like "Production" are treated as production and will
not fall back to DEFAULT_DEV_CORS_ORIGIN.
In `@tests/env.test.ts`:
- Around line 50-53: The test currently calls require("../src/config/env")
inside it (the require call in tests/env.test.ts) which violates
`@typescript-eslint/no-require-imports`; add a localized eslint suppression
directly above that require (use an inline // eslint-disable-next-line
`@typescript-eslint/no-require-imports`) so only that line is exempt, keeping the
rest of the file lint-clean and preserving the dynamic require behavior used in
the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51435035-78ba-4fd0-b587-fe4574f574dc
📒 Files selected for processing (6)
.env.examplesrc/config/corsOrigins.tssrc/config/env.tssrc/middleware/cors.tstests/cors.test.tstests/env.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/cors.test.ts`:
- Around line 90-97: The test "rejects disallowed cross-origin requests"
currently only checks that CORS headers are missing but doesn't assert the
response status; update the assertion so the request to
buildApp([allowedOrigin]).get("/ping").set("Origin", "https://evil.example.com")
also verifies a 200 status (e.g., add .expect(200) to the request chain or
assert res.status === 200) while keeping the existing checks for missing
access-control headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d314c8f2-1900-4cc4-b848-8d9ac27397b6
📒 Files selected for processing (5)
src/config/corsOrigins.tssrc/config/env.tssrc/middleware/cors.tstests/cors.test.tstests/env.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/env.test.ts
- src/middleware/cors.ts
- src/config/corsOrigins.ts
- src/config/env.ts
| it("rejects disallowed cross-origin requests", async () => { | ||
| const res = await request(buildApp([allowedOrigin])) | ||
| .get("/ping") | ||
| .set("Origin", "https://evil.example.com"); | ||
|
|
||
| expect(res.headers["access-control-allow-origin"]).toBeUndefined(); | ||
| expect(res.headers["access-control-allow-credentials"]).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check cors library behavior when origin callback returns an error
# Search for error handling in the cors middleware and related Express error patterns
rg -nP -C5 'callback\(new Error.*CORS' --type=ts
# Look for any existing documentation or tests about expected status codes
rg -nP 'expect\(\d{3}\)|toHaveStatus|status.*\d{3}' tests/cors.test.tsRepository: Pi-Defi-world/acbu-backend
Length of output: 641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the whole relevant test file section around the disallowed-origin test
nl -ba tests/cors.test.ts | sed -n '1,220p'
# 2) Inspect CORS middleware implementation
nl -ba src/middleware/cors.ts | sed -n '1,220p'
# 3) Find where buildApp registers error handlers (to see what status errors map to)
# (Look for common Express error-handler signatures or `app.use((err, req, res, next) => ...)`)
rg -n --hidden --glob='!**/node_modules/**' "buildApp|function buildApp|app\.use\(\s*\(err" src tests || true
rg -n --hidden --glob='!**/node_modules/**' "res\.status\(|status\(" src | head -n 50
# 4) Check if there's a centralized error handler file
fd -t f -a 'error|errors|middleware.*error|http.*error' . 2>/dev/null | head -n 50
rg -n --hidden --glob='!**/node_modules/**' "Not allowed by CORS|cors" src | head -n 50Repository: Pi-Defi-world/acbu-backend
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show relevant parts of tests/cors.test.ts (first 220 lines; adjust if needed)
echo "=== tests/cors.test.ts (1-220) ==="
cat -n tests/cors.test.ts | sed -n '1,220p'
# 2) Inspect CORS middleware implementation
echo "=== src/middleware/cors.ts (1-220) ==="
cat -n src/middleware/cors.ts | sed -n '1,220p'
# 3) Locate Express error handler(s) in the app setup
echo "=== searching for error-handler signatures ==="
rg -n --hidden --glob='!**/node_modules/**' "(err\s*,\s*req\s*,\s*res\s*,\s*next)|(function\s*\(\s*err\s*,\s*req)|(app\.use\(\s*\(\s*err)" src tests || true
# 4) Find buildApp registration of middleware/error handling
echo "=== searching for buildApp ==="
rg -n --hidden --glob='!**/node_modules/**' "buildApp" src tests || true
# 5) If there's an error-handling module, inspect it
echo "=== searching for error handling modules ==="
fd -t f -a 'error|errors|http.*error|middleware.*error' . 2>/dev/null | head -n 50
# 6) If we find a candidate error handler file, print it (top portion)
# (Try common filename matches first)
for f in $(fd -t f -a 'error|errors' src 2>/dev/null | head -n 10); do
echo "=== candidate: $f (first 200 lines) ==="
cat -n "$f" | sed -n '1,200p'
done
# 7) Also inspect where routes/middleware are wired (if there is an app file)
for f in $(fd -t f -a 'app\.ts$|server\.ts$|index\.ts$|main\.ts$' src 2>/dev/null | head -n 10); do
echo "=== candidate: $f (first 200 lines) ==="
cat -n "$f" | sed -n '1,200p'
doneRepository: Pi-Defi-world/acbu-backend
Length of output: 34068
🌐 Web query:
How does the cors npm package handle errors passed to the origin callback (callback(new Error(...))) in Express? What HTTP status is returned?
💡 Result:
cors does not translate callback(new Error(...)) into an Express error response with a specific HTTP status. Instead, the middleware treats the dynamic origin callback error as an origin rejection: it will not set Access-Control-Allow-Origin, and the request continues to the next middleware/route handler (so the final HTTP status is whatever your Express app ultimately sends). Authoritative behavior: the origin callback is documented as callback(error, origin) where error can be an error object to indicate failure [1][2][3]. For rejection/denial cases, the project specifically discusses handling when the origin callback returns false / rejects and focuses on CORS header behavior rather than sending an HTTP error response; e.g., a PR notes the middleware must set Vary: Origin even when the origin callback rejects, implying the middleware is controlling headers (not returning an app-level status) [4]. So, there isn’t a single fixed HTTP status returned by cors for callback(new Error(...)) in Express; your app’s normal response status (e.g., 200, 404, 500, etc.) will be used, and the client will typically see a CORS error because the required CORS headers are missing.
Citations:
- 1: https://github.com/expressjs/cors?tab=readme-ov-file
- 2: https://registry.npmjs.org/cors
- 3: https://www.npmjs.com/package/cors
- 4: fix: set Vary: Origin header when origin callback rejects expressjs/cors#403
Update the disallowed-origin CORS test to assert the correct HTTP status (200, not 500).
In tests/cors.test.ts the request should still reach the /ping handler; disallowed origins are handled by omitting Access-Control-* headers, not by producing an Express error response. Add a status assertion (.expect(200)) alongside the existing checks for missing CORS headers.
🤖 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 `@tests/cors.test.ts` around lines 90 - 97, The test "rejects disallowed
cross-origin requests" currently only checks that CORS headers are missing but
doesn't assert the response status; update the assertion so the request to
buildApp([allowedOrigin]).get("/ping").set("Origin", "https://evil.example.com")
also verifies a 200 status (e.g., add .expect(200) to the request chain or
assert res.status === 200) while keeping the existing checks for missing
access-control headers.
Closes #315
Browsers block Access-Control-Allow-Origin: * when credentials: true. This change rejects * in CORS_ORIGIN at startup, uses an explicit allowlist, and reflects
the request origin on allowed cross-origin calls. Adds tests for parsing, validation, and response headers.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests