Skip to content

fix(dashboard): add CORS origin validation in production#976

Merged
aaight merged 1 commit intodevfrom
fix/cors-origin-validation
Mar 22, 2026
Merged

fix(dashboard): add CORS origin validation in production#976
aaight merged 1 commit intodevfrom
fix/cors-origin-validation

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 22, 2026

Summary

  • Fix open CORS policy — The previous fallback cors() allowed all origins when CORS_ORIGIN was unset. This replaces it with a safe default.
  • Production guard — When CORS_ORIGIN is unset and NODE_ENV=production, the dashboard now logs a warning at startup and uses an empty origin list (blocking all cross-origin requests).
  • Dev default — When CORS_ORIGIN is unset outside production, CORS defaults to http://localhost:5173 (with credentials: true) for local npm run dev:web usage.
  • Refactor — CORS logic extracted to src/utils/corsConfig.ts (buildCorsMiddleware) for testability and clarity.

Closes: https://trello.com/c/msVjZYft/497-as-a-developer-i-want-cors-origin-validation-in-production-so-that-the-dashboard-is-not-vulnerable-to-cross-origin-attacks

Changes

  • src/utils/corsConfig.ts — New helper buildCorsMiddleware({ corsOriginEnv, isProduction, warn }) with all three-scenario logic
  • src/dashboard.ts — Uses buildCorsMiddleware instead of inline cors() call; removes unused cors import
  • tests/unit/utils/corsConfig.test.ts — 11 unit tests covering all three scenarios

Test plan

  • When CORS_ORIGIN is set, only those origins are allowed (comma-separated, whitespace-trimmed)
  • When CORS_ORIGIN is set, non-listed origins are blocked
  • When CORS_ORIGIN is unset in production: warning is logged at startup
  • When CORS_ORIGIN is unset in production: all cross-origin requests blocked (including localhost:5173)
  • When CORS_ORIGIN is unset outside production: defaults to http://localhost:5173 with credentials
  • When CORS_ORIGIN is unset outside production: other origins are still blocked
  • No warning logged in development when CORS_ORIGIN is unset
  • All 331 unit test files pass

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 22, 2026

Codecov Report

❌ Patch coverage is 72.41379% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/dashboard.ts 0.00% 8 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

Summary

LGTM — Clean security fix that closes an open CORS policy. The implementation is correct, well-tested, and the extraction to a dedicated utility improves testability.

Notes

  • Unused export: CorsConfig interface (lines 18–23 in corsConfig.ts) is exported but never imported anywhere. Minor dead code — not blocking.
  • Credential behavior change: The old fallback cors() did not set credentials: true; the new code always does. This is the correct behavior for the dashboard (session cookies require credentials: true), but worth noting as an intentional semantic change beyond what the PR description calls out.
  • Test coverage: 11 tests cover all three environment scenarios thoroughly. The tests correctly validate actual CORS header behavior via Hono's request helper rather than just testing config object shapes. The warn callback injection makes the production warning testable without console mocks.
  • Empty-string edge case: CORS_ORIGIN="" correctly falls through to the production/dev default (.filter(Boolean) strips the empty string) — good.

🕵️ claude-code · claude-opus-4-6 · run details

@aaight aaight merged commit d6d2170 into dev Mar 22, 2026
8 of 9 checks passed
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.

2 participants