Skip to content

Fix staging post-migration ZAP findings#4115

Closed
mrkeshav-05 wants to merge 2 commits intoOWASP:mainfrom
mrkeshav-05:fix/post-migration-ZAP
Closed

Fix staging post-migration ZAP findings#4115
mrkeshav-05 wants to merge 2 commits intoOWASP:mainfrom
mrkeshav-05:fix/post-migration-ZAP

Conversation

@mrkeshav-05
Copy link
Contributor

@mrkeshav-05 mrkeshav-05 commented Feb 28, 2026

Proposed change

After the Zappa → Terraform/ECS migration, the ZAP baseline scan at https://nest.owasp.dev fails with multiple WARN-NEW findings. The root cause is that the security headers previously served by the nginx reverse proxy (proxy/headers.conf) were not migrated to the new infrastructure.

This PR restores those security headers by adding them at the Next.js application layer via next.config.ts, and fixes a Terraform syntax error in the state management configuration.

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

Summary by CodeRabbit

  • New Features

    • Added comprehensive HTTP security headers, including Content Security Policy, automatically applied across all application routes in production environments
  • Chores

    • Updated infrastructure configuration to improve deployment consistency

Walkthrough

Added route-level HTTP security headers (CSP and related) via Next.js config; adjusted Terraform S3 object lock dependency to reference a global bucket versioning resource instead of per-item references.

Changes

Cohort / File(s) Summary
Frontend Security Headers
frontend/next.config.ts
Introduced a securityHeaders constant with comprehensive HTTP security headers and implemented an async headers() method that applies these headers to all routes (disabled in local environment).
Infrastructure Dependencies
infrastructure/state/main.tf
Changed aws_s3_bucket_object_lock_configuration depends_on from per-item aws_s3_bucket_versioning.state[each.key] to global aws_s3_bucket_versioning.state, altering dependency resolution timing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

backend

Suggested reviewers

  • kasya
  • arkid15r
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing ZAP security findings after the post-migration phase.
Description check ✅ Passed The description is directly related to the changeset, explaining the root cause (missing security headers from nginx migration) and the implemented solutions.
Linked Issues check ✅ Passed The PR successfully addresses issue #4090 by restoring security headers in the Next.js layer rather than suppressing findings, and fixes the associated Terraform syntax error.
Out of Scope Changes check ✅ Passed All changes are directly related to resolving the ZAP findings issue: security headers migration and Terraform dependency fix are both necessary for the stated objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

@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.

1 issue found across 2 files

Confidence score: 4/5

  • Only issue is a dev-environment CSP restriction, so overall risk is low and this PR should be safe to merge
  • frontend/next.config.ts applies a strict CSP without unsafe-eval or WS allowances, which can break local next dev tooling and HMR connections
  • Pay close attention to frontend/next.config.ts - CSP may block eval-based tooling and HMR in development.
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:68">
P2: Global CSP headers are applied in all environments, but the CSP omits 'unsafe-eval' and WS allowances required for Next.js dev/HMR. This can break local `next dev` by blocking eval-based tooling and HMR connections. Consider skipping these headers in local/dev or adding dev-only allowances.</violation>
</file>

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

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)
frontend/next.config.ts (1)

12-12: Consider migrating away from 'unsafe-inline' for scripts in the future.

While 'unsafe-inline' is commonly needed for Next.js applications and is acceptable here for restoring the previous security posture, using CSP nonces via Next.js middleware would provide stronger protection against XSS. This can be deferred since the current configuration matches the pre-migration baseline.

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

In `@frontend/next.config.ts` at line 12, The CSP currently includes "script-src
'self' 'unsafe-inline' ..." in frontend/next.config.ts; remove reliance on
'unsafe-inline' by implementing per-request CSP nonces via Next.js middleware:
generate a secure random nonce in the middleware, add it to the response CSP
header (replace 'unsafe-inline' with 'nonce-{nonce}' in the "script-src"
directive), and expose the same nonce to pages (via a response header or request
context) so scripts and <Script> tags can include nonce attributes; update any
inline scripts to use the generated nonce and ensure the middleware logic that
sets the CSP header and supplies the nonce is invoked for all relevant routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@frontend/next.config.ts`:
- Line 12: The CSP currently includes "script-src 'self' 'unsafe-inline' ..." in
frontend/next.config.ts; remove reliance on 'unsafe-inline' by implementing
per-request CSP nonces via Next.js middleware: generate a secure random nonce in
the middleware, add it to the response CSP header (replace 'unsafe-inline' with
'nonce-{nonce}' in the "script-src" directive), and expose the same nonce to
pages (via a response header or request context) so scripts and <Script> tags
can include nonce attributes; update any inline scripts to use the generated
nonce and ensure the middleware logic that sets the CSP header and supplies the
nonce is invoked for all relevant routes.

ℹ️ 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 5881877.

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

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 28, 2026
@sonarqubecloud
Copy link

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)
frontend/next.config.ts (1)

12-13: Harden CSP by phasing out unsafe-inline for scripts/styles.

Line 12 and Line 13 currently allow inline execution, which reduces CSP’s XSS protection. Prefer nonce/hash-based CSP for inline code/styles where feasible.

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

In `@frontend/next.config.ts` around lines 12 - 13, The CSP currently allows
unsafe inline for scripts and styles in the Next.js config entries "script-src"
and "style-src"; remove 'unsafe-inline' from both directives in next.config.ts
and implement nonce- or hash-based CSP instead—generate a per-request nonce in
your server (e.g., middleware or getServerSideProps), inject that nonce into the
CSP header and add the same nonce attribute to any legitimate inline <script> or
<style> usages (or convert inline code to external files and include their
hashes in the CSP), and update places that relied on inline execution to use the
nonce/hash so the CSP no longer requires 'unsafe-inline'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@frontend/next.config.ts`:
- Around line 12-13: The CSP currently allows unsafe inline for scripts and
styles in the Next.js config entries "script-src" and "style-src"; remove
'unsafe-inline' from both directives in next.config.ts and implement nonce- or
hash-based CSP instead—generate a per-request nonce in your server (e.g.,
middleware or getServerSideProps), inject that nonce into the CSP header and add
the same nonce attribute to any legitimate inline <script> or <style> usages (or
convert inline code to external files and include their hashes in the CSP), and
update places that relied on inline execution to use the nonce/hash so the CSP
no longer requires 'unsafe-inline'.

ℹ️ 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 5881877 and 89fed30.

📒 Files selected for processing (1)
  • frontend/next.config.ts

@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