Skip to content

fix: enable strict baseline Content-Security-Policy in Helmet - #380

Merged
birme merged 2 commits into
mainfrom
security-audit/fix-237-enable-helmet-csp
Sep 29, 2026
Merged

birme merged 2 commits into
mainfrom
security-audit/fix-237-enable-helmet-csp

Conversation

@birme

@birme birme commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Helmet was registered with contentSecurityPolicy: false, disabling CSP entirely (src/api.ts). Any XSS reaching the browser had no second line of defence.
  • Enable a strict baseline CSP locking default-src, base-uri, form-action and frame-ancestors to 'none'. The manager is a JSON API and serves no application HTML, so the policy can be fully restrictive.
  • The one HTML surface — Swagger UI at /api/docs — now emits its own compatible CSP via swagger-ui's staticCSP: true, which overrides the strict global policy on that route so the docs page keeps rendering.

Test plan

  • Tests pass (npm test — 363 passed)
  • TypeScript compiles (npm run typecheck)
  • Lint clean (npm run lint on changed file)
  • Manual: confirm /api/docs (Swagger UI) still renders in a browser under the new CSP, and that API responses carry the strict Content-Security-Policy header

Closes #237

🤖 Generated with Claude Code

Helmet was registered with contentSecurityPolicy: false, disabling CSP
entirely and leaving any XSS reaching the browser without a second line
of defence. Enable a strict CSP locking every directive to 'none' — the
manager is a JSON API and serves no application HTML — and let the one
HTML surface, the Swagger UI at /api/docs, emit its own compatible CSP
via swagger-ui's staticCSP option so the docs page keeps working.

Closes #237

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@birme

birme commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Automated code review (daily-backlog-pr, self-authored PR) — Needs Changes

The change itself is correct: the strict global Helmet CSP genuinely hardens the JSON API, and swagger-ui's staticCSP: true registers an encapsulated onSend hook scoped to /api/docs that overrides the global policy on that route only, so the Swagger UI page keeps working. The implementation matches the remediation #237 recommends and adds base-uri/form-action beyond the minimum.

Blocking:

  • src/api.ts — this security fix ships with no regression test. Correctness rests on a subtle, non-obvious interaction (global helmet CSP via onRequest overridden by swagger-ui's staticCSP onSend, encapsulated to /api/docs). Add a server.inject() test in api.test.ts asserting: (a) a normal API route returns content-security-policy: default-src 'none';base-uri 'none';form-action 'none';frame-ancestors 'none'; and (b) the docs index returns swagger's own CSP (default-src 'self' …), i.e. the strict policy did not leak onto the docs page.

Suggestions (non-blocking): optionally add explicit object-src 'none'; add a one-line note about the onRequest/onSend override mechanism so a future maintainer doesn't move the helmet registration and silently break the docs page.

Moving the board item back to Ready; the regression test will be added on the next implementation pass.

Locks in both halves of the onRequest/onSend CSP interaction from #237:
API routes carry the strict default-src 'none' baseline, while /api/docs
keeps Swagger UI's own default-src 'self' policy so the docs page works.

Addresses the self-review Needs-Changes feedback on PR #380.
@birme

birme commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Automated code review (daily-backlog-pr, self-authored PR) — LGTM

Reviewed by a separate code-reviewer invocation against the project criteria. The strict Helmet CSP baseline (default-src/base-uri/form-action/frame-ancestors all 'none') is correct for a JSON API, and swagger-ui's staticCSP: true correctly preserves the /api/docs page under its own default-src 'self' policy. The two regression tests added in 81c38cd lock in both halves of that interaction — strict policy on API routes, and no leak of the strict policy onto the docs page — resolving the prior Needs-Changes item (missing regression test).

Blocking: None. Warnings: None. Suggestions were optional hygiene only (e.g. adding the standard ./log mock to the file — a pre-existing gap, not introduced here). CI green: lint / pretty / ts / unittests all pass. Proceeding to admin-merge.

@birme
birme merged commit 9a9ac53 into main Sep 29, 2026
4 checks passed
@birme
birme deleted the security-audit/fix-237-enable-helmet-csp branch September 29, 2026 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: Content-Security-Policy is explicitly disabled in Helmet config

2 participants