Skip to content

Resolve staging scan issues after migration#4106

Closed
anurag2787 wants to merge 3 commits intoOWASP:mainfrom
anurag2787:fix/staging-post-migration
Closed

Resolve staging scan issues after migration#4106
anurag2787 wants to merge 3 commits intoOWASP:mainfrom
anurag2787:fix/staging-post-migration

Conversation

@anurag2787
Copy link
Contributor

@anurag2787 anurag2787 commented Feb 27, 2026

Proposed change

This PR addresses the security header issues causing the staging ZAP baseline scan to fail after the recent infrastructure migration.

Resolves #4090

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 27, 2026

Walkthrough

Adds security HTTP headers in two places: Next.js config now returns route-wide CSP, Permissions-Policy, and X-Content-Type-Options headers; Terraform ALB module adds HSTS and X-Frame-Options header values to the HTTPS listener.

Changes

Cohort / File(s) Summary
Frontend Security Headers
frontend/next.config.ts
Adds an async headers() export in the Next.js config that returns global route header entries for Content-Security-Policy (built from directives), Permissions-Policy, and X-Content-Type-Options.
Infrastructure ALB Headers
infrastructure/modules/alb/main.tf
Adds routing_http_response_strict_transport_security_header_value (max-age=63072000; includeSubDomains; preload) and routing_http_response_x_frame_options_header_value (DENY) to the HTTPS ALB listener configuration.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

ci

Suggested reviewers

  • kasya
  • arkid15r
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately summarizes the main objective of fixing security header issues after infrastructure migration.
Linked Issues check ✅ Passed Changes implement required security headers (CSP, HSTS, X-Frame-Options) to fix ZAP scan failures mentioned in issue #4090.
Out of Scope Changes check ✅ Passed All changes are directly related to resolving security header issues from the ZAP scan as specified in the linked issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description clearly describes the changeset: adding security headers to address ZAP baseline scan failures after infrastructure migration.

✏️ 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.

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@frontend/next.config.ts`:
- Around line 18-19: The CSP in next.config.ts currently uses "script-src
'unsafe-inline' ..." which weakens XSS protection; remove 'unsafe-inline' from
the "script-src" directive and replace it with a per-request nonce approach:
generate a random nonce in middleware, attach it to the request/response and
inject it into the Content-Security-Policy header as "script-src 'self'
'nonce-<nonce>' https://owasp-nest.s3.amazonaws.com ..." (preserve other hosts),
and update any inline scripts to use the nonce attribute (or convert them to
external scripts/hashes); implement the nonce generation and header wiring in
your Next.js middleware and ensure server-side rendering/templates output the
same nonce value into script tags (refer to the "script-src" string in
next.config.ts and your middleware function that sets headers).

ℹ️ 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 e5d3d6a and f97fa7e.

📒 Files selected for processing (2)
  • frontend/next.config.ts
  • infrastructure/modules/alb/main.tf

Comment on lines +18 to +19
"script-src 'self' 'unsafe-inline' https://owasp-nest.s3.amazonaws.com https://owasp-nest-production.s3.amazonaws.com https://www.googletagmanager.com https://*.i.posthog.com https://*.tile.openstreetmap.org",
"style-src 'self' 'unsafe-inline' https://cdn.jsdelivr.net https://owasp-nest.s3.amazonaws.com https://owasp-nest-production.s3.amazonaws.com",
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

script-src 'unsafe-inline' materially weakens the CSP.

On Line 18, allowing inline scripts means injected inline JS can still execute, which undercuts the main XSS protection CSP is meant to provide. Please move to nonce/hash-based script control instead of 'unsafe-inline'.

If useful, I can draft a nonce-based CSP approach for Next.js (middleware + header wiring) in a follow-up.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@frontend/next.config.ts` around lines 18 - 19, The CSP in next.config.ts
currently uses "script-src 'unsafe-inline' ..." which weakens XSS protection;
remove 'unsafe-inline' from the "script-src" directive and replace it with a
per-request nonce approach: generate a random nonce in middleware, attach it to
the request/response and inject it into the Content-Security-Policy header as
"script-src 'self' 'nonce-<nonce>' https://owasp-nest.s3.amazonaws.com ..."
(preserve other hosts), and update any inline scripts to use the nonce attribute
(or convert them to external scripts/hashes); implement the nonce generation and
header wiring in your Next.js middleware and ensure server-side
rendering/templates output the same nonce value into script tags (refer to the
"script-src" string in next.config.ts and your middleware function that sets
headers).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that nonce-based CSP is better long term but this PR is mainly about restoring what we already had before migration so staging scan stops failing changing to strict nonce setup would need bigger changes around GTM, analytics and maybe caching behavior, so I think it’s better to handle that separately instead of mixing it in this infra fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anurag2787, that's a fair point—keeping the infra fix scoped separately makes sense, especially given the integration points with GTM and analytics that would need careful testing.

Would you like me to open a follow-up issue to track the nonce-based CSP improvement for later? That way it's documented and can be addressed when the team has bandwidth to handle the broader changes needed.


🧠 Learnings used
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 3398
File: frontend/graphql-codegen.ts:0-0
Timestamp: 2026-01-19T09:18:37.206Z
Learning: When reviewing TypeScript files, watch for async immediately-invoked function expressions (IIFEs) and refactor to use top-level await where the runtime/module system supports it. If you encounter an async IIFE, replace it with top-level await or equivalent async code at module scope to improve readability and startup behavior. Ensure the environment (Node/ bundler) and module settings support top-level await before applying.

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.

2 issues found across 2 files

Confidence score: 3/5

  • CSP in frontend/next.config.ts allows script-src 'unsafe-inline', weakening XSS protection; this is a user-facing security concern even if otherwise functional.
  • Global CSP headers in frontend/next.config.ts omit unsafe-eval, which can break next dev/HMR locally; some risk of developer workflow disruption.
  • Given a medium-severity security weakening and a likely dev-tooling impact, this is some risk to merge as-is.
  • Pay close attention to frontend/next.config.ts - tighten CSP while ensuring dev tooling compatibility.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="frontend/next.config.ts">

<violation number="1" location="frontend/next.config.ts:18">
P2: Global CSP headers are applied in all environments, but the script-src policy omits the 'unsafe-eval' allowance required by Next.js dev tooling. This can break `next dev`/HMR in local development. Consider relaxing CSP in local/dev (e.g., add 'unsafe-eval' or skip headers when isLocal).</violation>

<violation number="2" location="frontend/next.config.ts:18">
P2: CSP allows script-src 'unsafe-inline', which permits any inline script execution and weakens XSS protection. Prefer nonces/hashes or move inline scripts to external files to keep CSP effective.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@sonarqubecloud
Copy link

@anurag2787 anurag2787 marked this pull request as ready for review February 27, 2026 17:59
@anurag2787
Copy link
Contributor Author

Screenshot from 2026-02-27 19-51-21 image Hi @arkid15r, the PR is ready for review. I have tested it locally. Please let me know if any changes are required.

@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