security: client-side sanitize snapshot HTML (#110)#131
Merged
Conversation
Snapshot HTML stored in .pad files is injected into the viewer and embed via innerHTML. It is already sanitized server-side by SnapshotHtmlSanitizer, but that is the sole XSS gate and .pad bodies are attacker-writable (WebDAV / malicious share). Add a client-side defense-in-depth pass so a regression in the server gate can't become stored XSS. - New src/lib/sanitize-html.js: sanitizeSnapshotHtml() runs DOMPurify with the same allowlist the server enforces (formatting tags only, no attributes), mirroring SnapshotHtmlSanitizer::ALLOWED_TAGS. - Apply it at both innerHTML sinks: embed-main.js snapshot preview and viewer-main.js renderSnapshotView. - Tests: tests/js/lib/sanitize-html.test.js (pinned to jsdom — happy-dom mis-sanitizes with DOMPurify) covers script/onclick/img/class stripping, disallowed-tag unwrapping, and allowed-tag passthrough. - Deps: dompurify (runtime, shared chunk ~11 kB gzip across viewer+embed), jsdom (dev, for the sanitizer test env). vitest 123 green; full Playwright 23/23 on NC 33 (snapshot rendering unaffected). Closes #110.
Review follow-up on #110: embed-main computed hasSnapshotHtml from the raw snapshotHtml, so if DOMPurify emptied it (e.g. all-dangerous markup) the embed would still render an empty HTML preview block instead of falling back to the snapshot text / empty message. Sanitize first, then decide the path from the sanitized result — mirrors viewer-main's renderSnapshotView. vitest 123 green; full Playwright 23/23 on NC 33.
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
1 participant
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.
What
Defense-in-depth for the snapshot HTML that the viewer and embed inject via
innerHTML. It is already sanitized server-side bySnapshotHtmlSanitizer, but that is the sole XSS gate and.padbodies are attacker-writable (WebDAV / a malicious share). A client-side pass adds resilience if the server gate ever regresses.Changes
src/lib/sanitize-html.js—sanitizeSnapshotHtml()runs DOMPurify with the same allowlist the server enforces: formatting tags only (p, br, ul, ol, li, h1–h6, strong, b, em, i, u, s, del, blockquote, pre, code),ALLOWED_ATTR: []. MirrorsSnapshotHtmlSanitizer::ALLOWED_TAGS.innerHTMLsinks:embed-main.jssnapshot preview andviewer-main.jsrenderSnapshotView.tests/js/lib/sanitize-html.test.js): script/onclick/img/class stripping, disallowed-tag unwrapping, allowed-tag passthrough, nullish input. Pinned tojsdom(@vitest-environment jsdom) because happy-dom mis-sanitizes with DOMPurify.dompurify(runtime) +jsdom(dev, test env only).Acceptance
Verification
npm run build: DOMPurify lands in a shared chunk (~11 kB gzip) used by viewer + embed; pad-sync is bundled into the same shared chunk (Vite merged the two shared modules — functionality intact, confirmed by the literals + e2e).Note on bundle cost
DOMPurify adds ~11 kB gzip to the shared Files/viewer/embed chunk. Worth it for an XSS defense layer on attacker-writable content; the issue explicitly proposed DOMPurify.
Closes #110.