fix(security): strip DOCTYPEs whose external identifier contains a bracket

scripts/lib/svg-active-content.mjs

@@ -77,13 +77,29 @@ const ATTRIBUTE_PATTERN =
  * `<!DOCTYPE\s[^>]*>` cannot express this: the first `>` inside an internal
  * subset closes an `<!ENTITY ...>` 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
+ * `'<!DOCTYPE' S Name (S ExternalID)? S? ('[' intSubset ']' S?)? '>'`, and an
+ * ExternalID carries quoted literals that may themselves contain `[`, `]` or
+ * `>`. `<!DOCTYPE svg SYSTEM "https://host/x[.dtd">` 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 `<![CDATA[...]]>` 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 = /<!DOCTYPE\s[^[>]*(?:\[[\s\S]*?\]\s*>|>)\s*/gi;
+const DOCTYPE_PATTERN =
+  /<!DOCTYPE\s(?:[^[>"']|"[^"]*"|'[^']*')*(?:\[(?:[^\]"']|"[^"]*"|'[^']*')*\]\s*)?>\s*/gi;
 
 /** An entity declaration, which only appears inside an internal DTD subset. */
 const ENTITY_DECLARATION = /<!ENTITY\s/i;

scripts/validate-architecture-assets.mjs

@@ -232,7 +232,20 @@ function validateSvg(path, quality) {
   // Critical: DOCTYPE is unnecessary in SVG images and can break XML parsers.
   if (hasDoctype(source)) {
     if (shouldFix) {
-      source = stripDoctype(source);
+      const withoutDoctype = stripDoctype(source);
+      // Report the fix only once it has demonstrably happened. A DOCTYPE the
+      // pattern cannot express (one opening an internal subset it never
+      // closes) comes back unchanged, and recording "removed DOCTYPE" for it
+      // would hand back a declaration this run claims to have deleted.
+      if (hasDoctype(withoutDoctype)) {
+        record(
+          path,
+          'error',
+          'contains a DOCTYPE declaration that could not be removed automatically',
+        );
+        return;
+      }
+      source = withoutDoctype;
       fixed.push(`${rel}: removed DOCTYPE`);
     } else {
       record(path, 'error', 'contains a DOCTYPE declaration');

tests/svg-active-content.test.mjs

@@ -684,6 +684,61 @@ test('stripDoctype removes a plain DOCTYPE and every repeat of one', () => {
   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 = '<!DOCTYPE svg SYSTEM "https://host.example/x[.dtd">\n' + INERT;
+  assert.equal(stripDoctype(svg), INERT);
+  assert.equal(hasDoctype(stripDoctype(svg)), false);
+});
+
+test('stripDoctype removes an external identifier containing "]>" or ">"', () => {
+  assert.equal(
+    stripDoctype(
+      '<!DOCTYPE svg SYSTEM "https://host.example/x]>y.dtd">\n' + INERT,
+    ),
+    INERT,
+  );
+  assert.equal(
+    stripDoctype(
+      '<!DOCTYPE svg SYSTEM "https://host.example/x>y.dtd">\n' + INERT,
+    ),
+    INERT,
+  );
+});
+
+// The deletion that an unterminated subset reading produces is unbounded: it
+// runs to the next `]`-then-`>`, and every `<![CDATA[...]]>` 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 =
+    '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 10 10">' +
+    '<style><![CDATA[ .a{fill:red} ]]></style><rect width="10" height="10"/></svg>';
+  const svg = '<!DOCTYPE svg SYSTEM "https://host.example/x[.dtd">\n' + body;
+  assert.equal(stripDoctype(svg), body);
+});
+
+test('stripDoctype leaves an unclosed subset in place for the entity check', () => {
+  const svg = '<!DOCTYPE svg [<!ENTITY x "javascript:alert(1)">\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('<!DOCTYPE svg SYSTEM "' + 'a'.repeat(200000));
+  assert.ok(Date.now() - started < 1000);
+});
+
 // A processing instruction is not a start tag, so no tag or attribute scan
 // ever sees it. `<?xml-stylesheet href?>` makes the browser fetch the target
 // when the SVG is opened directly, and `type="text/xsl"` applies an XSLT

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 = `<!DOCTYPE svg SYSTEM "https://host.example/x[.dtd">\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 = `<!DOCTYPE svg [<!ENTITY x "y">\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 =
     '<svg xmlns="http://www.w3.org/2000/svg" width="640" height="480"><rect/></svg>';