Cover the SVG vectors that defeat regex sanitizing - #163
Merged
Conversation
sanitizeSvg was rewritten from five regex replaces into a real parser, but the tests were never extended -- the existing three cases pass against both implementations, so nothing stops someone "simplifying" it back and silently reopening the holes. Adds the vectors the regex version let through, each verified to bypass it: unquoted javascript: hrefs, javascript: behind leading whitespace or a tab, iframe/embed/object/video/audio/canvas elements, self-closing foreignObject, inline style attributes, and external xlink:href. Plus two cases in the other direction -- same-document #fragment refs and ordinary drawing markup must survive untouched.
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
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.
Summary
sanitizeSvghere is a real parser — blocked-element stack, attribute allowlisting,safeUrlAttributeValuethat strips control characters before validating. But its tests were never extended past the original three, and those three pass against the old five-regex implementation too. Nothing in CI distinguishes the hardened version from the naive one, so a well-meaning simplification would reopen every hole silently.This adds the vectors the regex version let through. I verified each one bypasses the old implementation before writing the assertion:
All pass against the current parser. Two cases go the other way — same-document
#fragmentrefs and ordinaryviewBox/path/fillmarkup must survive untouched — so the tests pin behavior, not just rejection.Tests only. No production code changes.
Why now
The cloud fork is still running the old regex version of this function, and cloud serves both workspace logos and user signatures through it. The port is PageDen/pageden-cloud#281. These tests go in first so both repos land on the same guarantees, and so public can't drift back.
To be clear about severity: the serve routes send
default-src 'none'; sandboxplusnosniff, which is the actual barrier — the code comment says as much and it's correct. This is defense-in-depth, not a live exploit.Numbers
logo.test.ts, up from 5)Test plan
pnpm -r typecheckpnpm -r lintpnpm --filter @pageden/server test