fix(markdown): keep embedded images when sanitizing content - #4235
fix(markdown): keep embedded images when sanitizing content#4235john-traas wants to merge 3 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4235/ |
@coderabbitai summary
Closes #3662
🔍 Needs a decision before merging
This PR deliberately reverses a documented security choice, and that call should be reviewed rather than merged as-is. Input wanted from whoever implemented the base64 inline-image support on whether this is the shape we want.
markdown-parser.spec.tscontained this, on purpose:That blanket ban is what breaks #3662, so this PR replaces it with a narrower rule: image
data:URLs are kept and MIME-validated; every otherdata:URL is still stripped. If we would rather keep the blanket ban, the fix has to move elsewhere (see alternatives below) — that is the question to settle.Root cause
Not a backend parsing problem, as suspected in the issue — the value is stored correctly. The loss happens on the way back into the editor.
Both content converters parse incoming values through
rehype-sanitizewith a schema built on itsdefaultSchema(markdown-parser.ts), whose protocol list allows onlyhttp/httpsforsrc:So every base64 image src is deleted while loading saved content:
<p><img alt="pic.png"></p><img alt="pic.png" src="data:image/png;base64,…"><p><img alt="pic.png"></p>Then the loss becomes permanent:
<img src="data:…" />→ saved correctly (which is why it still renders in Lime Portal)srcimgparseDOM (plugins/image/node.ts) takessrc: dom.getAttribute('src') || '', so a src-less<img>becomes an image node withsrc: ''andstate: 'success'— the editor shows only the alt textgetImageHTMLwritessrc="${attrs.src}"→ the consumer is handed<img src="" …>→ the base64 payload is overwritten in the databaseStep 4, reproduced in Chromium before the fix:
Within a single session it looks fine, because the value echoed back into the editor is suppressed — the loss only surfaces after a reload or remount.
This also affects our own documented example,
limel-example-text-editor-with-inline-images-base64, and any consumer letting users paste images without an upload backend.The change
safe-image-data-urls.ts— the MIME allowlist anddata:URL parsing, lifted out ofemail-viewer/sanitize-email-html.ts, which had already solved exactly this problem (raster formats only;image/svg+xmlexcluded, since an SVG can carry script). That module had no callers other than its own tests, so this puts the logic to work instead of duplicating it.markdown-parser.ts— allowdataforsrcin the schema, then narrow it back to allow-listed image MIME types in a post-sanitize pass. The pass is shared bymarkdownToHTMLandsanitizeHTMLso the two paths cannot drift apart.limel-markdownshares this sanitizer, so it can now also render the base64 images the editor produces. That is intentional — the editor writing content its own renderer cannot display is a bug in its own right — but it does mean the change reaches every markdown consumer in the library, which is the main reason to review the scope.Alternatives, if the shared change is too wide
limel-markdownunable to display base64 images.Guardrails
Kept as tests, so a future widening of the protocol list cannot quietly re-open them:
data:text/html,data:application/javascript,data:text/plain— strippeddata:image/svg+xml— strippeddata:on<a href>— strippeddata:without a MIME type, and a malformed header with no comma — rejectedjavascript:onsrcandhref— unchanged, still strippedThe genuinely dangerous sinks (
iframe,embed,object,a href) are not affected: they remain protocol-gated, and<img src="data:text/html">does not execute.Not addressed here
data:image we legitimately reject (an SVG, say) still becomessrc=""and can overwrite stored data on the next save. Narrower than Limel-text-editor base64 - Parsing Error #3662 but the same failure mode — worth its own issue.limepkg-email) feedsprefilledBody + emailSignature + forwardData.htmlstraight into the editor withcontentType="html". Its own pasted images are uploaded and usehttp(s)srcs, so they were never affected, but adata:image in a forwarded body or an admin-configured signature would have been stripped before sending. This PR fixes that path too. Traced through the code, not reproduced in a running composer — worth confirming.hydrate-custom-elements.tsvalidates URLs inside custom-element JSON attributes against the same default protocol list, sodata:images there are still stripped. Out of scope.Verification
npm run test:spec— 74 files, 1138 tests, greennpx stencil-test --project e2e— 50 files, 1022 passed / 8 skipped, greennpm run lint— cleanmain(src="") and pass hereReview:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile:
🤖 Generated with Claude Code