Skip to content

Fix staging ZAP baseline scan by migrating security headers#4114

Closed
xDipzz wants to merge 2 commits intoOWASP:mainfrom
xDipzz:fix/4090-zap-headers-migrate
Closed

Fix staging ZAP baseline scan by migrating security headers#4114
xDipzz wants to merge 2 commits intoOWASP:mainfrom
xDipzz:fix/4090-zap-headers-migrate

Conversation

@xDipzz
Copy link

@xDipzz xDipzz commented Feb 28, 2026

Resolves #4090

Staging ZAP baseline scan started failing post-migration with new warnings for missing response headers:

  • Missing Anti-clickjacking Header (X-Frame-Options) [10020]
  • X-Content-Type-Options Header Missing [10021]
  • Content Security Policy (CSP) Header Not Set [10038]
  • Permissions Policy Header Not Set [10063]

This PR fixes the findings without adding ignores by migrating the existing header policy (from proxy/headers.conf) into the new stack:

  • Frontend (Next.js): add Content-Security-Policy, Permissions-Policy, X-Content-Type-Options: nosniff, X-Frame-Options: DENY for all routes via headers().
    • Local dev only: relax CSP to include 'unsafe-eval' when NEXT_PUBLIC_ENVIRONMENT=local.
  • Infra (Terraform ALB): set HTTPS listener response headers for HSTS, CSP, X-Content-Type-Options, and X-Frame-Options.
  • Infra tests: add terraform test assertions to prevent regressions.

Notes

  • Permissions-Policy is kept at the app layer because AWS provider aws_lb_listener does not expose a Permissions-Policy response header attribute.

Verification

  • Terraform (ALB module):

    • cd infrastructure/modules/alb
    • docker run --rm -v "$PWD":/work -w /work hashicorp/terraform:1.14.0 init -backend=false
    • docker run --rm -v "$PWD":/work -w /work hashicorp/terraform:1.14.0 validate
    • docker run --rm -v "$PWD":/work -w /work hashicorp/terraform:1.14.0 test
  • Headers (local dev):

    • curl -sSI http://localhost:3000/ | grep -iE '^(content-security-policy|permissions-policy|x-content-type-options|x-frame-options):'
    • curl -sSI http://localhost:3000/chapters | grep -iE '^(content-security-policy|permissions-policy|x-content-type-options|x-frame-options):'
  • Terraform init / validate
    Terraform init and validate output

  • Terraform test (44 passed)
    Terraform test results - 44 passed

  • Response headers (curl)
    Curl response headers output

Checklist

  • Required: I followed the contributing workflow
  • Required: I verified that my code works as intended and resolves the issue as described
  • Required: I ran make check-test locally: all warnings addressed, tests passed
  • I used AI for code, documentation, tests, or communication related to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 28, 2026

Walkthrough

This PR introduces Content Security Policy and related HTTP security headers (HSTS, X-Content-Type-Options, X-Frame-Options) across the frontend and infrastructure layers. Headers are configured at the Next.js application level and AWS ALB level, with corresponding test coverage added.

Changes

Cohort / File(s) Summary
Frontend Security Headers
frontend/next.config.ts
Added CSP constants and async headers() function to configure and return security headers for all routes. Differentiates CSP for local environments by including unsafe-eval in script-src directive.
Infrastructure Security Headers
infrastructure/modules/alb/main.tf
Introduced local content_security_policy string and attached four new routing_http_response_header_value settings to the HTTPS listener: Content-Security-Policy, Strict-Transport-Security, X-Content-Type-Options, and X-Frame-Options.
Infrastructure Security Tests
infrastructure/modules/alb/tests/alb.tftest.hcl
Added new test blocks to validate HTTPS listener security headers with assertions for HSTS max-age, X-Frame-Options, X-Content-Type-Options, and Content-Security-Policy values.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

ci

Suggested reviewers

  • arkid15r
  • kasya
  • noland-crane
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: migrating security headers to fix ZAP scan findings, which is the primary objective of the PR.
Linked Issues check ✅ Passed The PR fully addresses issue #4090 by migrating security headers into the new infrastructure instead of ignoring findings, implementing CSP, X-Frame-Options, X-Content-Type-Options, and Permissions-Policy across frontend and ALB layers.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #4090: security headers are added to Next.js config, Terraform ALB listener, and infrastructure tests; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The pull request description clearly relates to the changeset, explaining the rationale for adding security headers to both frontend and infrastructure layers to resolve ZAP baseline scan findings.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 3 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
infrastructure/modules/alb/tests/alb.tftest.hcl (1)

218-248: Good test coverage for security headers.

The new test blocks comprehensively verify all four security headers on the HTTPS listener. This will prevent regressions.

Minor maintenance concern: The CSP string on line 245 is duplicated from main.tf. If the CSP policy changes, both locations need to be updated. Consider extracting the CSP to a shared variable or using a Terraform data source in tests to reference the actual value, though this may not be straightforward in test files.

🤖 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 218 - 248, The
CSP string is duplicated in the test "test_https_listener_sets_csp_header"
(aws_lb_listener.https.routing_http_response_content_security_policy_header_value)
and should be referenced from a single source; update the tests to pull the
expected CSP from the same variable used in main (e.g., create or reuse a shared
variable like var.content_security_policy or a Terraform data source and
reference it in the assert) so that changes to the CSP only need to be made in
one place rather than editing the hard-coded string in the test block.
🤖 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 218-248: The CSP string is duplicated in the test
"test_https_listener_sets_csp_header"
(aws_lb_listener.https.routing_http_response_content_security_policy_header_value)
and should be referenced from a single source; update the tests to pull the
expected CSP from the same variable used in main (e.g., create or reuse a shared
variable like var.content_security_policy or a Terraform data source and
reference it in the assert) so that changes to the CSP only need to be made in
one place rather than editing the hard-coded string in the test block.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6d1c458 and abdc5e8.

📒 Files selected for processing (3)
  • frontend/next.config.ts
  • infrastructure/modules/alb/main.tf
  • infrastructure/modules/alb/tests/alb.tftest.hcl

@xDipzz
Copy link
Author

xDipzz commented Feb 28, 2026

@arkid15r #4090 fix is ready for review: migrated required security headers
from proxy policy into Next.js and ALB config, with local validation completed.
Please review and share any required changes.

@arkid15r
Copy link
Collaborator

arkid15r commented Mar 3, 2026

Closing in favor of #4137

@arkid15r arkid15r closed this Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix staging post-migration ZAP findings

2 participants