fix(reader): enforce CSP in rendered foliate documents#794
fix(reader): enforce CSP in rendered foliate documents#794
Conversation
📝 WalkthroughWalkthroughAdds helpers that ensure parsed EPUB/MOBI documents have Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/assets/foliate/mobi.js (1)
1183-1202:⚠️ Potential issue | 🔴 CriticalKF8/AZW3
loadSectiondoes not inject the script-blocking CSP — script execution re-enabled for modern Kindle books.
MOBI6.loadSection(line 877) callsprependScriptBlockingMetaCsp(doc)before serialization, butKF8.loadSectionhere serializes and creates the blob URL without ever injecting the CSP meta. Because the paginator and fixed-layout iframes now carryallow-scripts, any<script>(inline or external) present in a KF8/AZW3 document will now execute in the rendered iframe. This is the same threat model the EPUB/MOBI6 paths address and undoes the PR's security goal for this format.🔒 Proposed fix
async loadSection(section) { if (this.#cache.has(section)) return this.#cache.get(section) const str = await this.loadText(section) const replaced = await this.replaceResources(str) // by default, type is XHTML; change to HTML if it's not valid XHTML let doc = this.parser.parseFromString(replaced, this.#type) if (doc.querySelector('parsererror') || !doc.documentElement?.namespaceURI) { this.#type = MIME.HTML doc = this.parser.parseFromString(replaced, this.#type) } for (const [url, node] of this.#inlineMap) { for (const el of doc.querySelectorAll(`img[src="${url}"]`)) el.replaceWith(node) } + // Keep rendered book documents non-scriptable even when Safari requires + // allow-scripts on the iframe for parent-side interaction. + prependScriptBlockingMetaCsp(doc) const url = URL.createObjectURL( new Blob([this.serializer.serializeToString(doc)], { type: this.#type })) this.#cache.set(section, url) return url }Note: since KF8 documents are often XHTML (
this.#type === MIME.XHTML), the helper here that uses a plaincreateElementmay produce a non-namespaced<meta>in an XHTML tree. Consider mirroring thecreateElementNS(documentElement.namespaceURI, ...)variant used inepub.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/assets/foliate/mobi.js` around lines 1183 - 1202, loadSection is missing the call that injects the script-blocking CSP meta so KF8/AZW3 documents can run scripts; call the same helper used elsewhere (prependScriptBlockingMetaCsp) on the parsed document before serialization and blob creation, taking care to create the meta in the document namespace when this.#type === MIME.XHTML (i.e., use createElementNS(document.documentElement.namespaceURI, 'meta') or mirror the epub.js namespaced variant) so the meta is valid in XHTML trees, then serialize/Blob as before.
♻️ Duplicate comments (1)
frontend/src/assets/foliate/epub.js (1)
111-120:⚠️ Potential issue | 🟠 MajorConsider removing the existing-CSP short-circuit (same concern as
mobi.js).See the detailed discussion on
frontend/src/assets/foliate/mobi.js:16-24. A book-supplied permissive<meta http-equiv="Content-Security-Policy">will currently cause your restrictive CSP to be skipped; since multiple CSP metas intersect per spec, unconditionally prepending is safer. ThecreateElementNSfallback here is a nice touch for XHTML — worth mirroring to the mobi.js helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/assets/foliate/epub.js` around lines 111 - 120, The function prependScriptBlockingMetaCsp currently short-circuits if a meta CSP already exists; remove that short-circuit so the function always prepends a restrictive meta, i.e., delete or disable the check using head.querySelector('meta[http-equiv="Content-Security-Policy"]') in prependScriptBlockingMetaCsp so the new meta (created with createElementNS fallback) is always prepended to head; mirror the same behavior change you applied/will apply in mobi.js if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/assets/foliate/mobi.js`:
- Around line 16-24: The current early-return in prependScriptBlockingMetaCsp
prevents injecting a script-blocking CSP when a permissive CSP meta already
exists; change prependScriptBlockingMetaCsp to always create and prepend a meta
http-equiv="Content-Security-Policy" content="script-src 'none'" into the
document head (if head exists) instead of returning when any
meta[http-equiv="Content-Security-Policy"] is present, and apply the same change
to the near-duplicate helper in frontend/src/assets/foliate/epub.js so both
functions unconditionally prepend the restrictive meta (redundant strict CSPs
are acceptable and safer than skipping injection).
---
Outside diff comments:
In `@frontend/src/assets/foliate/mobi.js`:
- Around line 1183-1202: loadSection is missing the call that injects the
script-blocking CSP meta so KF8/AZW3 documents can run scripts; call the same
helper used elsewhere (prependScriptBlockingMetaCsp) on the parsed document
before serialization and blob creation, taking care to create the meta in the
document namespace when this.#type === MIME.XHTML (i.e., use
createElementNS(document.documentElement.namespaceURI, 'meta') or mirror the
epub.js namespaced variant) so the meta is valid in XHTML trees, then
serialize/Blob as before.
---
Duplicate comments:
In `@frontend/src/assets/foliate/epub.js`:
- Around line 111-120: The function prependScriptBlockingMetaCsp currently
short-circuits if a meta CSP already exists; remove that short-circuit so the
function always prepends a restrictive meta, i.e., delete or disable the check
using head.querySelector('meta[http-equiv="Content-Security-Policy"]') in
prependScriptBlockingMetaCsp so the new meta (created with createElementNS
fallback) is always prepended to head; mirror the same behavior change you
applied/will apply in mobi.js if present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 537f301f-430f-4463-9c09-9ae558b3a6a9
📒 Files selected for processing (4)
frontend/src/assets/foliate/epub.jsfrontend/src/assets/foliate/fixed-layout.jsfrontend/src/assets/foliate/mobi.jsfrontend/src/assets/foliate/paginator.js
📜 Review details
🔇 Additional comments (3)
frontend/src/assets/foliate/paginator.js (1)
242-246: Sandbox relaxation is consistent with CSP-based enforcement.Adding
allow-scriptshere is fine because rendered documents reaching this iframe have already been processed byepub.js/mobi.jswhich inject<meta http-equiv="Content-Security-Policy" content="script-src 'none'">prior to blob URL creation. The linked WebKit bug reference is helpful. Note the KF8 path inmobi.js(see other comment) — that flow currently skips the CSP injection and would undermine this iframe relaxation for AZW3/KF8 books.frontend/src/assets/foliate/epub.js (1)
937-942: Injection point is correct for the HTML/XHTML/SVG replacement branch.Placing
prependScriptBlockingMetaCsp(doc)after style/resource rewrites and before serialization ensures the CSP meta is present on the blob-served document. Note SVG root documents won't have a<head>so the helper no-ops for pure SVG — acceptable since SVG loaded as a standalone document is less common in spines, but worth being aware of.frontend/src/assets/foliate/fixed-layout.js (1)
84-87: Sandbox change OK for EPUB fixed-layout; KF8 fixed-layout still at risk.For EPUB, section
load()routes throughLoader.loadReplacedinepub.js, so the CSP meta is present before this iframe loads the blob. However, fixed-layout KF8/AZW3 books route throughKF8.loadSectioninmobi.js, which does not currently inject the CSP meta (see comment onfrontend/src/assets/foliate/mobi.js:1183-1202). Once that gap is fixed, this sandbox change is safe across formats.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/assets/foliate/epub.js`:
- Around line 111-118: The SVG branch currently bypasses CSP because
prependScriptBlockingMetaCsp looks for a <head> and returns for image/svg+xml;
update loadReplaced / the SVG handling path (where MIME.SVG and
DOMParser('image/svg+xml') are used) to explicitly sanitize SVG documents before
serialization: detect SVG by MIME.SVG or doc.documentElement.namespaceURI ===
'http://www.w3.org/2000/svg', remove all <script> elements and any element
attributes starting with "on" (e.g., onclick, onload), strip or neutralize
<iframe>/<object> that can execute scripts, and/or ensure the iframe sandbox
used for spine/resources does not include allow-scripts for SVG resources
(functions referenced: prependScriptBlockingMetaCsp, loadReplaced, MIME.SVG);
implement these removals prior to serialization so scripted SVGs cannot execute.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e1a5c2e-53e9-4240-b112-eb54cec3481d
📒 Files selected for processing (2)
frontend/src/assets/foliate/epub.jsfrontend/src/assets/foliate/mobi.js
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/assets/foliate/mobi.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/assets/foliate/mobi.js (1)
1203-1222:⚠️ Potential issue | 🔴 CriticalCSP injection is missing from the KF8 rendering path — scripts will execute for KF8/AZW3 books.
MOBI6.loadSectionnow injects the script-blocking CSP (line 897), butKF8.loadSectionserializes the parsed document straight to ablob:URL at line 1219 without callingprependScriptBlockingMetaCsp. Sincepaginator.js/fixed-layout.jsiframe sandboxes were relaxed to includeallow-scripts, any scripts inside a KF8/AZW3 book will execute in the rendered iframe — exactly the regression this PR is meant to prevent. KF8 is the dominant modern MOBI/Kindle format, so the fix must cover it as well as MOBI6.Also note:
KF8.#typedefaults toMIME.XHTML(line 986). When injecting into an XHTML document, the<meta>must be created in the document's namespace, matching whatepub.jsdoes withcreateElementNS(doc.documentElement?.namespaceURI, 'meta'). The currentprependScriptBlockingMetaCsphelper (line 40) only usesdoc.createElement, which yields a no-namespace element in XHTML and will either fail CSP parsing in strict XML or simply be ignored. Suggest aligning this helper with theepub.jsversion.🛡️ Proposed fix
const prependScriptBlockingMetaCsp = doc => { const head = ensureHeadElement(doc) - const meta = doc.createElement('meta') + const nsURI = doc.documentElement?.namespaceURI + const meta = nsURI ? doc.createElementNS(nsURI, 'meta') : doc.createElement('meta') meta.setAttribute('http-equiv', 'Content-Security-Policy') meta.setAttribute('content', "script-src 'none'") head.prepend(meta) }async loadSection(section) { if (this.#cache.has(section)) return this.#cache.get(section) const str = await this.loadText(section) const replaced = await this.replaceResources(str) // by default, type is XHTML; change to HTML if it's not valid XHTML let doc = this.parser.parseFromString(replaced, this.#type) if (doc.querySelector('parsererror') || !doc.documentElement?.namespaceURI) { this.#type = MIME.HTML doc = this.parser.parseFromString(replaced, this.#type) } for (const [url, node] of this.#inlineMap) { for (const el of doc.querySelectorAll(`img[src="${url}"]`)) el.replaceWith(node) } + // Keep rendered book documents non-scriptable even when Safari requires + // allow-scripts on the iframe for parent-side interaction. + prependScriptBlockingMetaCsp(doc) const url = URL.createObjectURL( new Blob([this.serializer.serializeToString(doc)], { type: this.#type })) this.#cache.set(section, url) return url }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/assets/foliate/mobi.js` around lines 1203 - 1222, KF8.loadSection is missing the CSP injection step that MOBI6.loadSection has, so update KF8.loadSection to call prependScriptBlockingMetaCsp(doc) after parsing/replacing resources and before serializing to a Blob (the code around the method named loadSection in the KF8 class); also modify the prependScriptBlockingMetaCsp helper to create the <meta> element in the document namespace when the parsed doc is XHTML (use doc.createElementNS(doc.documentElement?.namespaceURI, 'meta') when namespaceURI exists, otherwise fall back to doc.createElement) so the injected CSP meta is valid for XML/XHTML documents and will be honored by the browser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/assets/foliate/mobi.js`:
- Around line 1203-1222: KF8.loadSection is missing the CSP injection step that
MOBI6.loadSection has, so update KF8.loadSection to call
prependScriptBlockingMetaCsp(doc) after parsing/replacing resources and before
serializing to a Blob (the code around the method named loadSection in the KF8
class); also modify the prependScriptBlockingMetaCsp helper to create the <meta>
element in the document namespace when the parsed doc is XHTML (use
doc.createElementNS(doc.documentElement?.namespaceURI, 'meta') when namespaceURI
exists, otherwise fall back to doc.createElement) so the injected CSP meta is
valid for XML/XHTML documents and will be honored by the browser.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbf05057-59cf-4b3e-91f2-b992f3913ecd
📒 Files selected for processing (2)
frontend/src/assets/foliate/epub.jsfrontend/src/assets/foliate/mobi.js
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/assets/foliate/epub.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (java-kotlin)
|
I confirmed this correctly avoids the exploit in XHTML pages - both when using inline scripts, as script tags, or when referencing external script sources. This still allows SVG or other renderable documents that allow scripting. |
2 participants
Description
Restores EPUB reader usability on iOS Safari while keeping script execution blocked for rendered book content.
This changes the Foliate reader back to using
allow-scriptsin the iframe sandbox so Safari can properly handle reader tap and touch interactions again, and moves the script-blocking control into the actual rendered Foliate documents by injecting aContent-Security-Policymeta tag before they are serialized toblob:URLsLinked Issue: Fixes #793
Changes
allow-scriptsin Foliate paginator andfixed-layoutiframe sandboxesContent-Security-Policy: script-src 'none'into rendered EPUB/MOBI documents inepub.js+mobi.jsTesting
Summary by CodeRabbit
Security
Compatibility