fix(security): stop dropping security headers on static assets and the service worker - #209
Open
fjaeckel wants to merge 1 commit into
Open
fix(security): stop dropping security headers on static assets and the service worker#209fjaeckel wants to merge 1 commit into
fjaeckel wants to merge 1 commit into
Conversation
…e SW
nginx inherits add_header from an outer block ONLY when the current level
defines no add_header of its own -- and the 'always' flag does not change that.
Several locations set their own Cache-Control and therefore silently discarded
the entire server-level security header set:
location = /sw.js the service worker, served with NO CSP
location = /registerSW.js
location = /env-config.js
location ~* \.(js|css|png|jpg|jpeg|gif|ico|svg|woff|woff2|ttf|eot)$
every JS, CSS, SVG and font asset, served with
no nosniff and no CSP
location /health
The same defect was present in the TLS server block generated by
docker-entrypoint.sh, where it additionally dropped HSTS.
The headers now live in security-headers.conf, included at the server level and
re-included in every location that sets an add_header of its own, so the set
survives. HSTS moved into the shared include as well -- user agents ignore it
over plain HTTP (RFC 6797), so emitting it unconditionally is safe and avoids
a standalone add_header in the TLS block that would retrigger the same problem.
Header values are unchanged; this commit is only about where they apply.
Verified by running docker-entrypoint.sh with TLS_DOMAIN set and inspecting the
generated tls.conf.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GukWfyJMY28qv2CJjxFvKF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a Medium severity misconfiguration: most responses were served without any security headers.
nginx inherits
add_headerfrom an outer block only when the current level defines noadd_headerof its own — and thealwaysflag does not change this. Several locations set their ownCache-Controland therefore silently discarded the entire server-level set (CSP,X-Content-Type-Options,X-Frame-Options,Referrer-Policy, and on TLS, HSTS):location = /sw.jslocation = /registerSW.jslocation = /env-config.jslocation ~* \.(js|css|png|jpg|jpeg|gif|ico|svg|woff|woff2|ttf|eot)$nosniff, no CSPlocation /healthThe same defect was present in the TLS server block generated by
docker-entrypoint.sh, where it additionally dropped HSTS.Changes
security-headers.conf, included at the server level and re-included in every location that defines anadd_headerof its own.add_headerin the TLS block that would re-trigger the same inheritance problem.Header values are unchanged. This PR is only about where they apply.
Verification
No nginx binary is available in this environment for
nginx -t, so instead I randocker-entrypoint.shwithTLS_DOMAINset and fake certs in place, and inspected the generatedtls.conf— confirming the include lands at the server level and inside/sw.js,/registerSW.js, the static-asset regex and/health.Worth a quick
nginx -tin CI or on a built image before merge, since config syntax can't be fully exercised here.Related
The
Referrer-Policyvalue (no-referrer-when-downgrade) and the Google Fonts CSP hosts are deliberately left as-is here — those are a separate finding (tokens leaking to third parties viaReferer) handled insecurity/referrer-policy-token-leak. That PR touches the same file, so expect a small conflict depending on merge order.🤖 Generated with Claude Code
Generated by Claude Code