fix(security): drop 'unsafe-inline' from the CSP script-src - #213
Open
fjaeckel wants to merge 1 commit into
Open
fix(security): drop 'unsafe-inline' from the CSP script-src#213fjaeckel wants to merge 1 commit into
fjaeckel wants to merge 1 commit into
Conversation
script-src carried 'unsafe-inline', which means any attacker-controlled string that reaches the DOM as markup executes. That is precisely the class of bug the policy exists to stop, and it matters more here than usual: auth tokens live in localStorage, so a single successful injection reads them directly. The CSP was the compensating control and it was opted out of. 'unsafe-inline' was there for two hand-written inline <script> blocks in index.html — the APP_NAME title override and the pre-paint theme bootstrap. Both move to public/app-init.js, loaded as a classic render-blocking script from <head> immediately after env-config.js. It must stay classic and non-deferred: a deferred script paints the wrong theme first and corrects it afterwards, which is the flash the block was written to prevent. Nothing else in the built output is inline. vite-plugin-pwa references registerSW.js by src and Vite emits the entry as a module with src, so script-src 'self' is satisfiable as-is. app-init.js has no content hash in its filename, so it needs an explicit no-cache rule or it falls through to the immutable 1-year static-asset rule and a deploy cannot dislodge the old copy. Added in nginx.conf and in the TLS server block the entrypoint generates. The TLS block was missing that rule for env-config.js too, which meant runtime environment config was being served immutable for a year under HTTPS; added there as well. style-src keeps 'unsafe-inline'. The UI styles components through React's style prop, so removing it needs a different styling approach rather than a config change. Verified against a static server emitting the exact production CSP header: the app boots with zero violations, React mounts, the stored theme applies (dark set, light and default clear), and the APP_NAME override retitles the document. As a control, re-adding an inline script to the same page is refused by the browser with "Refused to execute inline script" and does not run — confirming the policy is enforced, not merely present.
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.
Problem
The Content-Security-Policy included
'unsafe-inline'inscript-src:That opts out of the single most useful thing a CSP does. Any attacker-controlled string that reaches the DOM as markup executes — exactly the class of bug the policy exists to stop. It matters more here than usual, because auth tokens live in
localStorage: one successful injection reads them directly. The CSP was the compensating control for that storage choice, and it was disabled.The directive was there for two hand-written inline
<script>blocks inindex.html— theAPP_NAMEtitle override and the pre-paint theme bootstrap.Change
Both blocks move to
public/app-init.js, loaded as a classic render-blocking script from<head>immediately afterenv-config.js.It must stay classic and non-deferred. A deferred or module script paints the wrong theme first and corrects it afterwards, which is the flash the original block was written to prevent. There is a comment on the file saying so.
Nothing else in the built output is inline — verified against a real
npm run build:vite-plugin-pwa references
registerSW.jsbysrcand Vite emits the entry as a module withsrc, soscript-src 'self'is satisfiable with no hashes or nonces.Caching
app-init.jshas no content hash in its filename, so without an explicit rule it falls through to the immutable 1-year static-asset block and a deploy cannot dislodge the old copy from browser caches. Added a no-cache location innginx.confand in the TLS server block the entrypoint generates.While adding it there I found the TLS block was missing that rule for
env-config.jstoo — meaning runtime environment config was being servedpublic, immutable, expires 1yunder HTTPS, so a redeploy with changed env values would not reach returning browsers. Added there as well.Verification
Served the production
dist/from a static server emitting the exact production CSP header and loaded it in headless Chromium:dark<html class="dark">light/ unsetVITE_APP_NAMEoverride"SkyBook - Pilot Logbook"As a control, re-adding an inline script to that same page under the same header:
so the policy is enforced, not merely present.
Also:
npx vitest run— 40 files / 352 tests pass;npx tsc --noEmitclean;sh -n docker-entrypoint.shclean;eslint .0 errors.Deliberately not in this PR
style-srckeeps'unsafe-inline'. The UI styles components through React'sstyleprop, so dropping it is a styling-architecture change, not a config change.Tokens remain in
localStorage. The original finding paired the CSP gap with token storage. Moving toHttpOnlycookies is a cross-repo design change — the API has to set and rotate the cookies, CSRF protection has to be added alongside, the refresh flow changes shape, and the PWA/service-worker path needs re-testing on iOS (there is already a comment invite.config.tsabout session resume being fragile there). That deserves its own decision and its own PR rather than being folded in behind a CSP fix. This change is still the right first move regardless of how that lands: XSS is the delivery mechanism for stealing those tokens, andscript-src 'self'is what makes injected script hard to run.Generated by Claude Code