From 623339a7abfbfa7c5fe8dc8e01e6fcfd50f4b539 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sat, 17 Oct 2026 15:16:24 +0800 Subject: [PATCH] refactor(xml): centralize strict document parsing Own syntax validation and source-located DOM projection together. Preserve explicit consumer policies and remove duplicated warning-message filters from Apktool and SAZ. Validation: 92 focused checks, 3 selected fake Apktool boundary checks, check:fast and docs:check. No real Apktool or Ghidra execution. Refs #4694 --- src/apktool/ApktoolDecodeOutput.ts | 33 ++++--------- src/browser/history/SazMetadata.ts | 74 ++++++----------------------- src/domain/xmlDocument.test.ts | 75 ++++++++++++++++++++++++++++++ src/domain/xmlDocument.ts | 62 ++++++++++++++++++++++++ 4 files changed, 161 insertions(+), 83 deletions(-) create mode 100644 src/domain/xmlDocument.test.ts create mode 100644 src/domain/xmlDocument.ts diff --git a/src/apktool/ApktoolDecodeOutput.ts b/src/apktool/ApktoolDecodeOutput.ts index f545935a26..4638aca29b 100644 --- a/src/apktool/ApktoolDecodeOutput.ts +++ b/src/apktool/ApktoolDecodeOutput.ts @@ -1,9 +1,5 @@ -import { - DOMParser, - type Document, - type Element, - type Node, -} from "@xmldom/xmldom"; +import type { Document, Element, Node } from "@xmldom/xmldom"; +import { parseXmlDocument } from "../domain/xmlDocument.js"; import { isMap, isScalar, @@ -147,25 +143,16 @@ const yamlScalar = ( }; const parseXml = (xml: string, label: string) => { - const errors: string[] = []; - const document = new DOMParser({ - normalizeLineEndings: (source) => source, - onError: (level, message) => { - // U+FFFD is a legal XML character. The provider's fatal UTF-8 decoder - // already rejects invalid bytes; this warning alone is not malformed XML. - if ( - level === "warning" && - message.startsWith("Unicode replacement character detected") - ) - return; - errors.push(message); - }, - }).parseFromString(xml, "application/xml"); - if (errors.length > 0 || document.documentElement === null) + const parsed = parseXmlDocument(xml, { + doctype: "allow", + lineEndings: "preserve", + }); + if (!parsed.ok) throw new TypeError( - `${label} is not well-formed XML${errors[0] === undefined ? "" : `: ${errors[0]}`}`, + `${label} is not well-formed XML: ${parsed.error.message}`, + { cause: parsed.error }, ); - return document; + return parsed.value; }; const childNodes = (node: Node): Node[] => { diff --git a/src/browser/history/SazMetadata.ts b/src/browser/history/SazMetadata.ts index 5ae8d950b7..589760a4bc 100644 --- a/src/browser/history/SazMetadata.ts +++ b/src/browser/history/SazMetadata.ts @@ -1,5 +1,5 @@ -import { DOMParser, ParseError, type Node } from "@xmldom/xmldom"; -import { SaxesParser } from "saxes"; +import type { Node } from "@xmldom/xmldom"; +import { parseXmlDocument } from "../../domain/xmlDocument.js"; import type { JsonValue } from "../../domain/jsonValue.js"; import { WEB_NETWORK_CAPTURE_LIMITS } from "../../domain/webNetworkCapture.js"; import { @@ -41,36 +41,20 @@ export const decodeSazMetadata = (bytes: Buffer): JsonValue => { cause, ); } - validateSazXml(xml); - let parseFailed = false; - let document: ReturnType; - try { - document = new DOMParser({ - normalizeLineEndings: (source) => source, - onError: (level, message) => { - // The parser warns whenever the source contains U+FFFD. It is a legal - // XML Char, and the fatal decoder above already rejected invalid - // source bytes, so only that warning is not a well-formedness failure. - if ( - level === "warning" && - message.startsWith("Unicode replacement character detected") - ) - return; - parseFailed = true; - }, - }).parseFromString(xml, "application/xml"); - } catch (cause: unknown) { - if (!(cause instanceof ParseError)) throw cause; + const parsed = parseXmlDocument(xml, { + doctype: "reject", + lineEndings: "preserve", + }); + if (!parsed.ok) throw new CaptureFormatError( - "format", - "SAZ metadata is not well-formed XML.", - ); - } - if (parseFailed) - throw new CaptureFormatError( - "format", - "SAZ metadata is not well-formed XML.", + parsed.error.reason === "doctype" ? "unsupported" : "format", + parsed.error.reason === "doctype" + ? "SAZ metadata DTDs are unsupported." + : "SAZ metadata is not well-formed XML.", + "", + parsed.error, ); + const document = parsed.value; if (document.documentElement?.tagName !== "Session") throw new CaptureFormatError( "format", @@ -104,33 +88,3 @@ export const decodeSazMetadata = (bytes: Buffer): JsonValue => { }; return visit(document, 0); }; - -/** Validate well-formedness before the DOM parser projects retained evidence. */ -const validateSazXml = (xml: string): void => { - const parser = new SaxesParser({ - defaultXMLVersion: "1.0", - forceXMLVersion: true, - }); - let hasDoctype = false; - parser.on("doctype", () => { - hasDoctype = true; - }); - try { - parser.write(xml).close(); - } catch { - if (hasDoctype) - throw new CaptureFormatError( - "unsupported", - "SAZ metadata DTDs are unsupported.", - ); - throw new CaptureFormatError( - "format", - "SAZ metadata is not well-formed XML.", - ); - } - if (hasDoctype) - throw new CaptureFormatError( - "unsupported", - "SAZ metadata DTDs are unsupported.", - ); -}; diff --git a/src/domain/xmlDocument.test.ts b/src/domain/xmlDocument.test.ts new file mode 100644 index 0000000000..c8ad068ab6 --- /dev/null +++ b/src/domain/xmlDocument.test.ts @@ -0,0 +1,75 @@ +import { expect, it } from "vitest"; +import { parseXmlDocument } from "./xmlDocument.js"; + +it.each([ + '😀 � & �', + "", + '\r\n �\rtext', + "�", + ']>�', +])("preserves legal Unicode and source-located DOM nodes: %j", (source) => { + const parsed = parseXmlDocument(source, { + doctype: "allow", + lineEndings: "preserve", + }); + if (!parsed.ok) throw parsed.error; + expect(parsed.value.documentElement?.tagName).toBe("root"); + expect(parsed.value.documentElement?.lineNumber).toBeDefined(); + expect(parsed.value.documentElement?.textContent).toContain("�"); +}); + +it.each([ + "�", + '', + '', + "�", + "� & unknown", + "� &missing;", + "�\u0000", + "��", + "�\uFFFF", + "�]]>", + "", + "�", + "�", + "]>�", + "�", +])("rejects malformed XML without warning-message matching: %j", (source) => { + const parsed = parseXmlDocument(source, { + doctype: "allow", + lineEndings: "preserve", + }); + expect(parsed.ok).toBe(false); + if (parsed.ok) throw new Error("Expected refusal"); + expect(parsed.error.reason).toBe("malformed"); + expect(parsed.error.cause).toBeDefined(); +}); + +it("keeps literal line endings and character-reference values distinct", () => { + const parsed = parseXmlDocument("\r\n \r�", { + doctype: "allow", + lineEndings: "preserve", + }); + if (!parsed.ok) throw parsed.error; + expect(parsed.value.documentElement?.textContent).toBe("\r\n\r\r�"); +}); + +it("applies the consumer's explicit DTD policy", () => { + const parsed = parseXmlDocument( + ']>', + { doctype: "reject", lineEndings: "preserve" }, + ); + expect(parsed.ok).toBe(false); + if (parsed.ok) throw new Error("Expected unsupported DTD"); + expect(parsed.error.reason).toBe("doctype"); +}); + +it("applies XML line-ending normalization only when the consumer selects it", () => { + const parsed = parseXmlDocument('\r\nx', { + doctype: "allow", + lineEndings: "normalize", + }); + if (!parsed.ok) throw parsed.error; + expect(parsed.value.documentElement?.getAttribute("attr")).toBe("a b"); + expect(parsed.value.documentElement?.textContent).toBe("\nx"); +}); diff --git a/src/domain/xmlDocument.ts b/src/domain/xmlDocument.ts new file mode 100644 index 0000000000..af4f636cc1 --- /dev/null +++ b/src/domain/xmlDocument.ts @@ -0,0 +1,62 @@ +import { + DOMParser, + normalizeLineEndings, + onErrorStopParsing, + type Document, +} from "@xmldom/xmldom"; +import { SaxesParser } from "saxes"; +import { err, ok, type Result } from "./result.js"; + +export class XmlDocumentParseError extends Error { + readonly _tag = "XmlDocumentParseError"; + constructor( + readonly reason: "malformed" | "doctype", + cause?: unknown, + ) { + super( + reason === "doctype" + ? "XML document types are unsupported" + : `XML is not well-formed${cause instanceof Error ? `: ${cause.message}` : ""}`, + { cause }, + ); + } +} + +/** One boundary owns strict XML syntax and the source-located DOM projection. */ +export const parseXmlDocument = ( + source: string, + policy: { + readonly doctype: "allow" | "reject"; + readonly lineEndings: "preserve" | "normalize"; + }, +): Result => { + try { + const syntax = new SaxesParser({ + defaultXMLVersion: "1.0", + forceXMLVersion: true, + }); + syntax.on("doctype", () => { + if (policy.doctype === "reject") + throw new XmlDocumentParseError("doctype"); + }); + syntax.write(source).close(); + // Saxes owns well-formedness outside DTDs. The DOM parser additionally + // checks DTD syntax and builds source-located nodes; its advisory warnings + // (including literal U+FFFD) are not byte-decoding or syntax failures. + return ok( + new DOMParser({ + normalizeLineEndings: + policy.lineEndings === "preserve" + ? (text) => text + : normalizeLineEndings, + onError: onErrorStopParsing, + }).parseFromString(source, "application/xml"), + ); + } catch (cause: unknown) { + return err( + cause instanceof XmlDocumentParseError + ? cause + : new XmlDocumentParseError("malformed", cause), + ); + } +};