fix:zap baseline security headers#4137
Conversation
Summary by CodeRabbit
WalkthroughAdds HTTP security headers across frontend (Next.js) and backend (ALB) and introduces tests: Content-Security-Policy, Strict-Transport-Security (HSTS), X-Frame-Options, X-Content-Type-Options, and several cross-origin and referrer headers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
infrastructure/modules/alb/main.tf (1)
35-36: CSP alignment with Next.js is correct.The Content-Security-Policy here matches the production CSP in
next.config.ts(excluding the local-only'unsafe-eval'directive). This dual-layer approach ensures headers are set even if one layer fails.Consider extracting this policy to a shared location or adding a comment referencing
frontend/next.config.tsto help maintainers keep them in sync during future updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/modules/alb/main.tf` around lines 35 - 36, The Content-Security-Policy string assigned to content_security_policy (near backend_path_chunks/local.backend_paths usage) should be made maintainable: either extract the CSP into a shared variable or module so both Terraform and Next.js read the same source, or at minimum add an inline comment above the content_security_policy assignment referencing the canonical policy in frontend/next.config.ts so future changes stay in sync; update references (content_security_policy) accordingly and ensure any local-only directives like 'unsafe-eval' remain excluded here.infrastructure/modules/alb/tests/alb.tftest.hcl (1)
245-252: CSP test provides strong validation but requires synchronization.The hardcoded CSP string in the assertion (line 249) must stay in sync with
local.content_security_policyinmain.tf. While this creates maintenance overhead, it also ensures any CSP modifications are intentional and reviewed. Consider adding a comment noting this dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/modules/alb/tests/alb.tftest.hcl` around lines 245 - 252, The CSP assertion in the test run "test_https_listener_sets_csp_header" hardcodes a value that must remain synchronized with local.content_security_policy in main.tf; update the test to add a clear comment above the assert referencing local.content_security_policy and instructing maintainers to update this hardcoded string whenever local.content_security_policy changes (or to derive the expected value from the same source in future), and mention aws_lb_listener.https as the resource validated so reviewers know where to check and keep in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@infrastructure/modules/alb/main.tf`:
- Around line 35-36: The Content-Security-Policy string assigned to
content_security_policy (near backend_path_chunks/local.backend_paths usage)
should be made maintainable: either extract the CSP into a shared variable or
module so both Terraform and Next.js read the same source, or at minimum add an
inline comment above the content_security_policy assignment referencing the
canonical policy in frontend/next.config.ts so future changes stay in sync;
update references (content_security_policy) accordingly and ensure any
local-only directives like 'unsafe-eval' remain excluded here.
In `@infrastructure/modules/alb/tests/alb.tftest.hcl`:
- Around line 245-252: The CSP assertion in the test run
"test_https_listener_sets_csp_header" hardcodes a value that must remain
synchronized with local.content_security_policy in main.tf; update the test to
add a clear comment above the assert referencing local.content_security_policy
and instructing maintainers to update this hardcoded string whenever
local.content_security_policy changes (or to derive the expected value from the
same source in future), and mention aws_lb_listener.https as the resource
validated so reviewers know where to check and keep in sync.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/next.config.tsinfrastructure/modules/alb/main.tfinfrastructure/modules/alb/tests/alb.tftest.hcl
|
@arkid15r, please review it and let me know if any changes are required. |
|
Strong security improvement — adding CSP, HSTS, X-Frame-Options, and the other hardening headers at both the Next.js and ALB layers is good defense-in-depth. A few things I'd flag: The CSP policy is defined in two places — The CSP includes The On the Terraform side, the ALB listener now sets headers directly on the listener resource. This is clean, but note that these headers will apply to all responses including error pages and redirects. The The test coverage for the ALB headers is solid — assertions for each header value are a nice touch. |
arkid15r
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
8826054
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
infrastructure/modules/alb/tests/alb.tftest.hcl (2)
284-289: Directive count assertion is brittle for future-safe CSP additions.Using an exact
== 11can fail on benign hardening changes. Prefer a minimum threshold plus normalized splitting.♻️ Proposed robustness tweak
run "test_csp_directive_count" { command = plan assert { - condition = length(regexall("[^;]+", trimspace(local.content_security_policy))) == 11 - error_message = "Content-Security-Policy must contain 11 directives." + condition = length([ + for directive in split(";", local.content_security_policy) : + trimspace(directive) if trimspace(directive) != "" + ]) >= 11 + error_message = "Content-Security-Policy must contain at least 11 directives." } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/modules/alb/tests/alb.tftest.hcl` around lines 284 - 289, The CSP directive count check in run "test_csp_directive_count" is brittle because it asserts equality to 11; change the assertion to enforce a minimum (e.g., >= 11) and normalize splitting by trimming and filtering out empty entries before counting so harmless future directives don't break the test — update the condition that uses regexall/trimspace/local.content_security_policy to trim each matched segment and count only non-empty directives, asserting length(...) >= 11 instead of == 11.
268-281: Add a negative guard for insecurescript-srctokens.The current checks ensure directive presence, but they won’t catch accidental weakening of CSP via
unsafe-inline/unsafe-evalinscript-src.♻️ Proposed test hardening
run "test_csp_contains_required_directives" { command = plan assert { condition = alltrue([ strcontains(local.content_security_policy, "default-src 'self'"), strcontains(local.content_security_policy, "object-src 'none'"), strcontains(local.content_security_policy, "frame-ancestors 'none'"), strcontains(local.content_security_policy, "script-src"), strcontains(local.content_security_policy, "style-src"), - strcontains(local.content_security_policy, "base-uri 'self'") + strcontains(local.content_security_policy, "base-uri 'self'"), + !can(regex("script-src[^;]*'unsafe-inline'", local.content_security_policy)), + !can(regex("script-src[^;]*'unsafe-eval'", local.content_security_policy)) ]) error_message = "Content-Security-Policy must contain required security directives." } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/modules/alb/tests/alb.tftest.hcl` around lines 268 - 281, The test run "test_csp_contains_required_directives" currently checks presence of directives in local.content_security_policy but lacks negative guards for insecure script-src tokens; update the assert conditions to also ensure local.content_security_policy does NOT contain "unsafe-inline" and "unsafe-eval" (e.g., add not(strcontains(local.content_security_policy, "unsafe-inline")) and not(strcontains(local.content_security_policy, "unsafe-eval"))), so the test fails if the CSP has those weakening tokens in script-src.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@infrastructure/modules/alb/tests/alb.tftest.hcl`:
- Around line 284-289: The CSP directive count check in run
"test_csp_directive_count" is brittle because it asserts equality to 11; change
the assertion to enforce a minimum (e.g., >= 11) and normalize splitting by
trimming and filtering out empty entries before counting so harmless future
directives don't break the test — update the condition that uses
regexall/trimspace/local.content_security_policy to trim each matched segment
and count only non-empty directives, asserting length(...) >= 11 instead of ==
11.
- Around line 268-281: The test run "test_csp_contains_required_directives"
currently checks presence of directives in local.content_security_policy but
lacks negative guards for insecure script-src tokens; update the assert
conditions to also ensure local.content_security_policy does NOT contain
"unsafe-inline" and "unsafe-eval" (e.g., add
not(strcontains(local.content_security_policy, "unsafe-inline")) and
not(strcontains(local.content_security_policy, "unsafe-eval"))), so the test
fails if the CSP has those weakening tokens in script-src.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/next.config.tsinfrastructure/modules/alb/main.tfinfrastructure/modules/alb/tests/alb.tftest.hcl
🚧 Files skipped from review as they are similar to previous changes (2)
- infrastructure/modules/alb/main.tf
- frontend/next.config.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4137 +/- ##
=======================================
Coverage 99.80% 99.80%
=======================================
Files 519 519
Lines 16031 16031
Branches 2186 2143 -43
=======================================
Hits 16000 16000
Misses 26 26
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
3 participants
Proposed change
Resolves #4090
Migrate security headers from Nginx proxy into Next.js config and ALB listener to fix ZAP baseline scan failures on staging.
Checklist
make check-testlocally: all warnings addressed, tests passed