From e504a825217a4b6b66bb8961c46ab271cbf12932 Mon Sep 17 00:00:00 2001 From: sec-check Date: Sat, 3 Oct 2026 19:20:07 -0400 Subject: [PATCH] fix(security): strip DOCTYPEs whose external identifier contains a bracket stripDoctype() read '[' as the unambiguous start of a DOCTYPE's internal subset, but an ExternalID carries quoted literals that may contain '[', ']' or '>'. A declaration such as has no internal subset, yet the old pattern saw one that never closes. With no later ']'-then-'>' the declaration was returned unchanged, so the strip silently did nothing and an external DTD reference survived into published content; with one -- every section supplies it -- the lazy subset match deleted every byte in between, destroying live markup. Both outcomes are inert, so both passed the callers' post-strip rescans in mirrorLandscapeLogo() and sanitizeArchitectureAssets(), neither of which has a DOCTYPE gate of its own. Match each part with a quote-aware alternation so a quoted literal always consumes through its closing mate. This also fixes '>' and ']>' inside a quoted system identifier, and preserves the documented behaviour for a subset that genuinely never closes: it still matches nothing and is left for the check to flag. The branches are disjoint on their first character, so matching stays linear. Make the auto-fix in validateSvg() fail closed as well, recording an error rather than reporting 'removed DOCTYPE' for a declaration still present. That also covers the early-return write, which persisted the result with no rescan. Signed-off-by: sec-check --- scripts/lib/svg-active-content.mjs | 28 ++++++++--- scripts/validate-architecture-assets.mjs | 15 +++++- tests/svg-active-content.test.mjs | 55 +++++++++++++++++++++ tests/validate-architecture-assets.test.mjs | 24 +++++++++ 4 files changed, 115 insertions(+), 7 deletions(-) diff --git a/scripts/lib/svg-active-content.mjs b/scripts/lib/svg-active-content.mjs index 8a6512d3..41c55204 100644 --- a/scripts/lib/svg-active-content.mjs +++ b/scripts/lib/svg-active-content.mjs @@ -77,13 +77,29 @@ const ATTRIBUTE_PATTERN = * `]*>` cannot express this: the first `>` inside an internal * subset closes an `` declaration, not the DOCTYPE, so that * pattern deletes the declaration and leaves a bare `]>` behind -- in a file - * the caller then publishes as well-formed XML. The subset alternative carries - * its own closing `>` so that a `]` inside a quoted entity value cannot end - * the subset early. A DOCTYPE that opens a subset and never closes it matches - * neither alternative and is left in place, where hasDoctype() and the entity - * check below still see it. + * the caller then publishes as well-formed XML. + * + * Neither can a pattern that treats `[` as the unambiguous start of the + * subset. Per the XML grammar a DOCTYPE is + * `''`, and an + * ExternalID carries quoted literals that may themselves contain `[`, `]` or + * `>`. `` is an external + * identifier with no subset at all, but an unquoted reading sees its `[` as a + * subset that never closes: the declaration is then left in place (so the + * strip silently does nothing), or -- where the document later holds any + * `]`-then-`>`, which every `` section provides -- everything + * between is deleted, destroying live markup that was never part of the + * declaration. Both outcomes survive the callers' post-strip rescans, because + * an unstripped external identifier and a truncated document are each inert. + * + * So each part is matched with the quote-aware alternation below, in which a + * `"` or `'` always consumes through its closing mate. A `]` inside a quoted + * entity value therefore cannot end the subset early either. A DOCTYPE that + * opens a subset and never closes it still matches nothing and is left in + * place, where hasDoctype() and the entity check below see it. */ -const DOCTYPE_PATTERN = /]*(?:\[[\s\S]*?\]\s*>|>)\s*/gi; +const DOCTYPE_PATTERN = + /"']|"[^"]*"|'[^']*')*(?:\[(?:[^\]"']|"[^"]*"|'[^']*')*\]\s*)?>\s*/gi; /** An entity declaration, which only appears inside an internal DTD subset. */ const ENTITY_DECLARATION = / { assert.equal(stripDoctype(INERT), INERT); }); +// An ExternalID carries quoted literals, and a quoted literal may hold any of +// the characters that otherwise delimit the declaration. Reading `[` as the +// unambiguous start of an internal subset therefore misreads a subset-less +// declaration as one that never closes, and the strip silently does nothing. +test('stripDoctype removes an external identifier containing a bracket', () => { + const svg = '\n' + INERT; + assert.equal(stripDoctype(svg), INERT); + assert.equal(hasDoctype(stripDoctype(svg)), false); +}); + +test('stripDoctype removes an external identifier containing "]>" or ">"', () => { + assert.equal( + stripDoctype( + 'y.dtd">\n' + INERT, + ), + INERT, + ); + assert.equal( + stripDoctype( + 'y.dtd">\n' + INERT, + ), + INERT, + ); +}); + +// The deletion that an unterminated subset reading produces is unbounded: it +// runs to the next `]`-then-`>`, and every `` section supplies +// one. The wreckage is inert, so it survives the callers' post-strip rescans +// and is published as a well-formed-looking asset. +test('stripDoctype does not swallow markup up to a later CDATA terminator', () => { + const body = + '' + + ''; + const svg = '\n' + body; + assert.equal(stripDoctype(svg), body); +}); + +test('stripDoctype leaves an unclosed subset in place for the entity check', () => { + const svg = '\n' + INERT; + assert.equal(stripDoctype(svg), svg); + assert.equal(hasDoctype(svg), true); + assert.deepEqual(findActiveContent(svg), [ + 'contains an entity declaration in an internal DTD subset ' + + '(the XML parser expands it, so its payload is not visible here)', + ]); +}); + +test('stripDoctype matches an unterminated literal in linear time', () => { + // The alternation branches are disjoint on their first character, so no + // input can make the engine explore them combinatorially. + const started = Date.now(); + stripDoctype('` makes the browser fetch the target // when the SVG is opened directly, and `type="text/xsl"` applies an XSLT diff --git a/tests/validate-architecture-assets.test.mjs b/tests/validate-architecture-assets.test.mjs index 27cafc7b..01ec4d8e 100644 --- a/tests/validate-architecture-assets.test.mjs +++ b/tests/validate-architecture-assets.test.mjs @@ -203,6 +203,30 @@ test('--fix removes a DOCTYPE declaration instead of reporting it', () => { assert.match(result.stdout, /removed DOCTYPE/); }); +test('--fix removes a DOCTYPE whose external identifier contains a bracket', () => { + const svg = `\n${VALID_SVG}`; + const result = runFix(svg); + assert.equal(result.status, 0, result.stderr); + assert.doesNotMatch(result.files[ASSET], /DOCTYPE/); + assert.equal(result.files[ASSET], VALID_SVG); + assert.match(result.stdout, /removed DOCTYPE/); +}); + +test('--fix reports a DOCTYPE it could not remove instead of claiming success', () => { + // An internal subset that never closes matches no DOCTYPE pattern, so the + // strip returns the source unchanged. Recording "removed DOCTYPE" for it + // would hand back a declaration the run claims to have deleted. + const svg = `\n${VALID_SVG}`; + const result = runFix(svg); + assert.notEqual(result.status, 0); + assert.equal(result.files[ASSET], svg); + assert.doesNotMatch(result.stdout, /removed DOCTYPE/); + assert.match( + result.stdout + result.stderr, + /DOCTYPE declaration that could not be removed automatically/, + ); +}); + test('--fix adds a viewBox derived from width and height', () => { const svg = '';