Repository navigation
Enforce a script Content-Security-Policy - #499
Merged
Merged
Conversation
Uploaded files come from this origin, so one served with a JavaScript type could run as one of the app's own scripts. Browsers accept sixteen spellings of that type, and Disk URLs sign the type they were minted with, so refusing a list of types at upload can't cover links already out there. Instead, a middleware rewrites the type of anything Active Storage reads out that isn't plainly media, a PDF or plain text to application/octet-stream, whatever the URL says.
script-src 'self', object-src 'none' and base-uri 'self', enforced. The two inline scripts the importmap helper emits are allowed by their SHA-256, and the origins its URLs point to (a configured asset host, in any form Rails accepts) are allowed by name. There is no per-request nonce, so cached pages stay valid. The enforcing frame-src for configured embed providers is unchanged. DISABLE_CONTENT_SECURITY_POLICY=true leaves this policy off, for an install whose proxy or customization injects scripts; frame-src still applies then, as it does today.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🔵 Needs a closer look
Browser script enforcement and upload content-type handling change security boundaries that warrant final human verification.
0 open findings
What changed in this PR
Adds an enforcing script Content Security Policy to Writebook while preserving existing embed restrictions and cacheable pages.
Changes:
- Allows application scripts through same-origin access, importmap hashes, and resolved asset-host origins.
- Rewrites unsafe upload content types to binary.
- Adds policy, asset-host, escape-hatch, and upload middleware tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| test/middleware/uploaded_script_guard_test.rb | Tests upload type filtering and unaffected responses. |
| test/integration/content_security_policy_test.rb | Tests policy enforcement, script allowances, and opt-out. |
| config/initializers/uploaded_script_guard.rb | Filters upload response content types. |
| config/initializers/content_security_policy.rb | Enables the policy with an environment opt-out. |
| app/controllers/concerns/inline_script_hashes.rb | Adds inline script hashes and asset origins. |
| app/controllers/application_controller.rb | Includes script policy support. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A request for several byte ranges gets a 206 multipart/byteranges whose boundary the client needs to split the body. Rewriting it to octet-stream lost the boundary. No browser runs that type as a script, so it passes through.
With DISABLE_CONTENT_SECURITY_POLICY=true there's no application policy, and the hash callback started an empty one, so pages sent an empty Content-Security-Policy header. Skip the callback then.
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
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.
Turns on an enforcing Content-Security-Policy that limits scripts to the ones Writebook ships. Replaces #464, which was report-only. The enforcing
frame-srcfor configured embed providers stays as it is.What changes
For attackers: if markup gets past the markdown renderer and sanitizer, it can no longer run script. Inline
<script>and inline event handlers are refused. An uploaded file can't be loaded as a script either, since uploads are now served as downloads unless they're plainly media.For self-hosters: nothing, for most installs. The policy adds
script-src,object-src 'none'andbase-uri 'self'next to the existingframe-src. Images, media, fonts, styles (custom styles included) and connections behave as before.script-src 'self', plus the SHA-256 of the two inline scripts the importmap helper emits. There's no per-request nonce or nonce cookie, so pages served withCache-Control: publicstay valid in shared caches: the hashes only change when the importmap does, at a deploy.asset_hostis allowed by origin. It's read from the importmap URLs Rails' asset helpers build, so a host with or without a scheme, protocol-relative, a%dset or a proc all resolve the same way the page's own tags do.UploadedScriptGuard(a middleware) serves anything Active Storage reads out that isn'timage/*,video/*,audio/*, PDF, octet-stream, plain text or a multi-rangemultipart/byterangesresponse asapplication/octet-stream. It does this when the file is served, not when it's uploaded, so Disk URLs already out there are covered too. That includes published books' non-expiring ones.Release notes
#465 serves custom styles from their own route, which removes the custom-styles trick altogether.
Evidence
ActiveStorageAuthorizationTest#test_unpublishing_a_book_revokes_the_disk_URL_behind_a_page_upload(undefined method 'request_uri' for URI::Generic), and main errors the same way on that box. System tests 3/3. RuboCop clean, importmap audit clean. Brakeman reports no warnings, butbin/brakeman's--ensure-latestgate fails the same way on main: 8.1.0 is out and the lockfile pins 8.0.6.frame-srcwith the default embed providers.jsuploaded through the page editor's upload endpoint, loaded from its Disk URL as a<script>text/javascript)</style><script>asset_hostforms (scheme, bare host, protocol-relative,%d, one- and two-argument procs) allow every script origin the page loads; the escape hatch boots with no global policy, and a page served without one sends onlyframe-src, or no header when no embeds are configured; all sixteen JavaScript MIME spellings plus parameter and casing variants leave uploads as octet-stream; media keeps its type; a multi-range response keeps itsmultipart/byterangesboundary; the existingframe-srctests still pass.Risk and rollback
DISABLE_CONTENT_SECURITY_POLICY=truerestores today's behavior without a deploy,frame-srcincluded.config/initializers/active_storage.rbsetsexpires_in 1.year). Revalidation doesn't change it, because a 304 carries no content type.Content-Typeon files that were already downloads.What ships
config/initializers/content_security_policy.rb: the enforcing policy, skipped whenDISABLE_CONTENT_SECURITY_POLICY=true.app/controllers/concerns/inline_script_hashes.rb: importmap hashes and asset-host origins.config/initializers/uploaded_script_guard.rb: the upload type guard.test/integration/content_security_policy_test.rbandtest/middleware/uploaded_script_guard_test.rb.