-
Notifications
You must be signed in to change notification settings - Fork 1
Restrict CORS to allowed origins #87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Completed Working on "Code Review v2.0"✅ Code review comments from chunk processed and review submitted with REQUEST_CHANGES. Posted 4 comments across 3 files highlighting major security and robustness issues and provided remediation suggestions. Next steps communicated to author. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security tightening is welcome, but several blockers remain:
- Example environment file may leak open localhost origins in production—comment out defaults.
main.ts: CORS allow-list parsing fails whenCORS_ALLOWED_ORIGINSis set but empty; filter out blank values and retain sane defaults.- No automated tests cover the new CORS logic; add integration tests to ensure disallowed origins are rejected and wildcard
*cannot be re-enabled. lint:corsgrep pattern is brittle; variants like{cors: true}or double-quoted keys bypass the check—use a more flexible regex and broader path scope.
Please address these issues before merging.
| # Copy this file to `.env` (or another env file) and adjust values as needed. | ||
|
|
||
| # Comma-separated list of allowed origins for CORS | ||
| CORS_ALLOWED_ORIGINS=http://localhost:3000,http://localhost:4200 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including active localhost origins in the example file may lead teams to copy the value unmodified into production, unintentionally exposing the API. Consider commenting this line out and providing it as guidance instead, e.g. # CORS_ALLOWED_ORIGINS=http://example.com, so production environments start locked-down by default.
| const app = await NestFactory.create(AppModule); | ||
|
|
||
| const allowedOrigins = | ||
| process.env.CORS_ALLOWED_ORIGINS?.split(',').map(o => o.trim()) ?? [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If CORS_ALLOWED_ORIGINS is defined but empty (e.g. CORS_ALLOWED_ORIGINS=""), .split(',') returns ["" ], so the fallback list is skipped and Nest will reply with Access-Control-Allow-Origin: null, effectively blocking all browsers. Please trim & filter falsy entries and fall back to the defaults when the resulting list is empty.
| 'http://localhost:4200', | ||
| ]; | ||
|
|
||
| app.enableCors({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security-critical behaviour changed, but no automated test verifies that disallowed origins are rejected or that wildcards are never re-enabled. Please add an integration test that spins up the app with/without CORS_ALLOWED_ORIGINS and asserts the Access-Control-Allow-Origin header for allowed vs disallowed origins.
| "name": "awesome-overcut-root", | ||
| "private": true, | ||
| "scripts": { | ||
| "lint:cors": "! grep -R --line-number -- '{ cors: true }' apps/*/src/main.ts || (echo 'Error: Disallowed permissive CORS usage found.' && exit 1)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The grep guard is fragile: it matches only the exact substring with spaces/braces, so variants like {cors: true}, double quotes, or usages in other files could bypass CI. Use a more flexible regex such as grep -R --line-number -E '\{\s*cors\s*:\s*true' apps/**/*.ts to prevent false negatives.
Completed Working on "Auto-Changelog Reminder"✅ Analyzed PR #87: detected user-facing CORS behaviour change with no changelog update. Suggested appropriate entry and commented on PR requesting addition, offering to commit upon "approve changelog" reply. |
|
It looks like this PR introduces user-facing behaviour changes (CORS restrictions) but no Here’s a suggested entry you can copy-paste (a root-level ## [Unreleased]
### Changed
- Restrict CORS: the Hotel-Management-Service server now allows only the origins listed in the `CORS_ALLOWED_ORIGINS` environment variable, replacing the previous permissive `*` policy.Reply with "approve changelog" and I will create a commit on this PR with the suggested entry. |
Overall goal
Restrict Cross-Origin Resource Sharing (CORS) so that only explicitly allowed origins can access the Hotel-Management-Service API, closing security gap identified in #83.
Checklist
main.ts– removed{ cors: true }, addedapp.enableCors()with env-driven allow-list..env.example– introducesCORS_ALLOWED_ORIGINSvariable template.lint:corsscript guard – fails CI if permissive CORS snippet re-appears.Follow-up notes for reviewers
http://localhost:3000andhttp://localhost:4200for local development.CORS_ALLOWED_ORIGINSin each deployment environment to the production origins expected by clients.Closes #83.