Skip to content

fix: add DomPurify to sanitize html - #819

Closed
aerodeval wants to merge 2 commits into
mainfrom
md-to-html-fix
Closed

fix: add DomPurify to sanitize html#819
aerodeval wants to merge 2 commits into
mainfrom
md-to-html-fix

Conversation

@aerodeval

@aerodeval aerodeval commented Jul 3, 2026

Copy link
Copy Markdown
Member

marked output was rendered without sanitization, letting javascript and event handlers execute in other users' browsers.

fix: Pass it through DOMPurify to successfully render md to HTML with any issues

Docs preview: https://ui.frappe.io/pr-preview/pr-819/

Coverage: 69.10% (-0.03% vs main)

@aerodeval
aerodeval marked this pull request as draft July 3, 2026 14:40
@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

Safe to merge for browser-only use; introduces a silent empty-string failure path if the function is ever called outside a browser environment.

The change correctly adds XSS sanitization. The only concern is that DOMPurify will silently return empty string when there is no DOM, which current callers avoid but future ones may not.

src/utils/markdown.ts — specifically the DOMPurify import and its behavior in non-browser environments.

Reviews (1): Last reviewed commit: "fix: add DomPurify to sanitize html" | Re-trigger Greptile

Comment thread src/utils/markdown.ts
@@ -1,12 +1,14 @@
import { marked } from 'marked'
import DOMPurify from 'dompurify'

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.

P2 DOMPurify silently returns empty string in SSR/Node.jsdompurify requires a browser DOM; without window/document it falls back to returning "", silently swallowing all markdown output. Current callers (paste extensions) are browser-only, but as a public utility this is a latent data-loss trap. Consider guarding with typeof window !== 'undefined' or using isomorphic-dompurify.

@barista-for-frappe

Copy link
Copy Markdown

Looks good — wraps markdownToHTML output in DOMPurify.sanitize to strip XSS from pasted markdown. Contained and safe.

  • dompurify@^3.4.0 is already in dependencies, so no new install (package.json:175).
  • No public API change. Both callers (content-paste-extension.ts:62, paste-markdown-utils.ts:18) are browser-only paste handlers, so DOMPurify's DOM requirement is fine.

Nit (optional): no test for the sanitization path — a one-liner asserting a <script> in markdown gets stripped would lock this in.

@aerodeval aerodeval closed this Jul 28, 2026
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.

1 participant