Skip to content

fix(markdown): keep embedded images when sanitizing content - #4235

Draft
john-traas wants to merge 3 commits into
mainfrom
investigate-base64-inline-images
Draft

fix(markdown): keep embedded images when sanitizing content#4235
john-traas wants to merge 3 commits into
mainfrom
investigate-base64-inline-images

Conversation

@john-traas

Copy link
Copy Markdown
Contributor

@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.ts contained this, on purpose:

describe('data URLs', () => {
    it('should strip data URLs for security', async () => {
        // Data URLs are stripped by the sanitizer to prevent
        // potential XSS via data: protocol
        expect(await sanitizeHTML('<img src="data:image/png;base64,abc123">'))
            .toEqualHtml('<img>');
    });
});

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 other data: 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-sanitize with a schema built on its defaultSchema (markdown-parser.ts), whose protocol list allows only http/https for src:

protocols: { src: ['http', 'https'], … }

So every base64 image src is deleted while loading saved content:

input output before this PR
![pic.png](data:image/png;base64,…) <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:

  1. Paste → the node holds the full data URI → serialized to <img src="data:…" />saved correctly (which is why it still renders in Lime Portal)
  2. Reopen → sanitizer deletes src
  3. img parseDOM (plugins/image/node.ts) takes src: dom.getAttribute('src') || '', so a src-less <img> becomes an image node with src: '' and state: 'success' — the editor shows only the alt text
  4. Next edit → getImageHTML writes src="${attrs.src}" → the consumer is handed <img src="" …>the base64 payload is overwritten in the database

Step 4, reproduced in Chromium before the fix:

Received: '<p><img src="" alt="picture.png" style="max-width: 100%;">caption</p>'

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 and data: URL parsing, lifted out of email-viewer/sanitize-email-html.ts, which had already solved exactly this problem (raster formats only; image/svg+xml excluded, 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 — allow data for src in the schema, then narrow it back to allow-listed image MIME types in a post-sanitize pass. The pass is shared by markdownToHTML and sanitizeHTML so the two paths cannot drift apart.

limel-markdown shares 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

  1. Thread an opt-in flag through the text-editor's converters only, leaving limel-markdown unable to display base64 images.
  2. Keep the blanket ban and have the editor refuse to persist images it cannot round-trip, so users get an explicit failure instead of silent loss.

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 — stripped
  • data:image/svg+xml — stripped
  • data: on <a href> — stripped
  • data: without a MIME type, and a malformed header with no comma — rejected
  • javascript: on src and href — unchanged, still stripped

The 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

  • A data: image we legitimately reject (an SVG, say) still becomes src="" 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.
  • The email composer (limepkg-email) feeds prefilledBody + emailSignature + forwardData.html straight into the editor with contentType="html". Its own pasted images are uploaded and use http(s) srcs, so they were never affected, but a data: 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.ts validates URLs inside custom-element JSON attributes against the same default protocol list, so data: images there are still stripped. Out of scope.

Verification

  • npm run test:spec — 74 files, 1138 tests, green
  • npx stencil-test --project e2e — 50 files, 1022 passed / 8 skipped, green
  • npm run lint — clean
  • The two round-trip e2e tests fail on main (src="") and pass here

Review:

  • Commits are atomic
  • Commits have the correct type for the changes made
  • Commits with breaking changes are marked as such

Browsers tested:

(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)

Windows:

  • Chrome
  • Edge
  • Firefox

Linux:

  • Chrome
  • Firefox

macOS:

  • Chrome
  • Firefox
  • Safari

Mobile:

  • Chrome on Android
  • iOS

🤖 Generated with Claude Code

@john-traas
john-traas requested a lite review from Copilot August 17, 2026 12:59
@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4cc68a7e-08a5-4583-8e73-5345d3e93dff

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4235/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Limel-text-editor base64 - Parsing Error

2 participants