fix(plist): preserve source semantics in XML data decoding

Adapt the completed-quartet decoder from PR #4981 at 4cf6cac14. Bind raw source spans to validated DOM nodes before entity and line-ending interpretation can change data bytes. Preserve depth preflight, reject invalid data forms and allocate exact decoded output through one shared quartet grammar.

Fixes #4978

Co-authored-by: Avichal Pandey <avichal2018lko@gmail.com>

docs/native-investigation.md

@@ -337,6 +337,12 @@ complete observation.
   Object `kind` describes the decoded archive value: data, dates and numbers
   remain `scalar` when their JSON value uses a `$plist_type` wrapper. An actual
   dictionary with the same fields remains `dictionary`.
+  XML must be well-formed. Data literals use
+  [CoreFoundation's raw ASCII quartet rules](https://github.com/swiftlang/swift-corelibs-foundation/blob/384ff680be7c7445fee6ce0c55e9ae11a46e07c5/Sources/CoreFoundation/CFPropertyList.c#L1588):
+  non-alphabet ASCII characters are ignored and an incomplete final quartet
+  emits no bytes. Character references retain their source spelling for data
+  decoding. Non-ASCII content, child markup and self-closing `<data/>` are format
+  failures; explicit `<data></data>` represents empty data.
   UID 0 cannot distinguish conditional nil from ordinary nil. Limits are
   64 MiB of input, 200,000 objects/references and 128 nesting levels.
   XML dictionary entries decoded as `__proto__` are omitted with an explicit

src/domain/propertyListKeys.ts

@@ -1,4 +1,3 @@
-import { normalizeLineEndings, type Document, type Node } from "@xmldom/xmldom";
 import { parseOpenStep } from "plist";
 
 import { decodeXmlPropertyList } from "./propertyListXml.js";
@@ -9,7 +8,6 @@ import {
 } from "./apple/plistNumbers.js";
 
 import { MAX_PLIST_DEPTH } from "./apple/plistValue.js";
-import { parseXmlDocument } from "./xmlDocument.js";
 
 /** The dictionary key REA results cannot carry: validation strips it. */
 const PROTOTYPE_KEY = "__proto__";
@@ -108,20 +106,7 @@ export const parseXmlPropertyList = (text: string): ParsedPropertyList => {
       omittedPrototypeKeys: prototypeKeys,
     };
   }
-  // Normalize only the literal line endings the pinned decoder normalizes.
-  // Decode this DOM directly so character references are never reserialized
-  // into literal line endings and normalized a second time.
-  const normalized = normalizeLineEndings(text);
-  // Strict syntax owns well-formedness before value interpretation. Reuse the
-  // same DOM for depth, keys, values and number facts.
-  const parsed = parseXmlDocument(normalized, {
-    doctype: "allow",
-    lineEndings: "preserve",
-  });
-  if (!parsed.ok) throw parsed.error;
-  const document = parsed.value;
-  assertXmlPlistDepthWithinLimit(document);
-  const decoded = decodeXmlPropertyList(document);
+  const decoded = decodeXmlPropertyList(text);
   return {
     ...omitPrototypeKeys(decoded.value),
     numberLiterals: decoded.numberLiterals,
@@ -288,43 +273,3 @@ const skipOpenStepScalar = (text: string, start: number): number => {
   }
   return cursor;
 };
-
-/** Elements whose children the plist value decoder visits recursively. */
-const PLIST_CONTAINER_ELEMENTS = new Set(["plist", "dict", "array"]);
-
-/**
- * Reject XML plists nested deeper than the projection supports before any
- * recursive value pass runs. Value decoding and prototype omission recurse once
- * per nesting level, so a deeply nested document exhausts the call stack
- * before the projection limit can report the resource bound. The failure
- * matches `projectPlistValue` so callers classify it as a limit failure.
- *
- * Depth counts values the way the projection does: the root value is level 0
- * and every container member is one level deeper. A dictionary key names a
- * member instead of nesting one, and a self-closing element is the value at
- * its own level. The outer plist envelope is unwrapped by the decoder;
- * nested plist elements decode as arrays and must count toward the bound.
- */
-const assertXmlPlistDepthWithinLimit = (document: Document): void => {
-  const root = document.documentElement;
-  if (root === null) return;
-  const pending: Array<{ node: Node; depth: number }> = [
-    { node: root, depth: -1 },
-  ];
-  while (pending.length > 0) {
-    const entry = pending.pop();
-    if (entry === undefined) break;
-    const { node, depth } = entry;
-    // Schedule only the next sibling, keeping traversal storage proportional
-    // to depth rather than the number of members in a wide container.
-    if (node.nextSibling !== null)
-      pending.push({ node: node.nextSibling, depth });
-    if (node.nodeType !== node.ELEMENT_NODE) continue;
-    if (node.nodeName !== "key" && depth > MAX_PLIST_DEPTH)
-      throw new RangeError(
-        `Plist exceeds ${String(MAX_PLIST_DEPTH)} nesting levels`,
-      );
-    if (PLIST_CONTAINER_ELEMENTS.has(node.nodeName) && node.firstChild !== null)
-      pending.push({ node: node.firstChild, depth: depth + 1 });
-  }
-};

src/domain/propertyListXml.test.ts

@@ -92,6 +92,20 @@ it.each([
   },
 );
 
+it.each([
+  ["AQID", [1, 2, 3]],
+  [" A Q I D\n", [1, 2, 3]],
+  ["AQI", []],
+  ["AQ!ID", [1, 2, 3]],
+  ["AQ==", [1]],
+  ["AQI=", [1, 2]],
+  ["AQ= =\n", [1]],
+  ["", []],
+])("decodes data text %j as CFPropertyList does", (text, expected) => {
+  const parsed = parseXmlPropertyList(`<plist><data>${text}</data></plist>`);
+  expect(parsed.value).toEqual(Uint8Array.from(expected));
+});
+
 it("collects source integers and CDATA reals before rounding and duplicate assignment", () => {
   const parsed = parseXmlPropertyList(
     "<plist><dict><key>a</key><integer>0x20000000000001</integer><key>a</key><integer>9007199254740992</integer><key>b</key><real>9007199254<![CDATA[740992]]></real></dict></plist>",

src/domain/propertyListXml.ts

@@ -1,4 +1,6 @@
-import type { Document, Node } from "@xmldom/xmldom";
+import type { Node } from "@xmldom/xmldom";
+
+import { readXmlPlistSource } from "./propertyListXmlSource.js";
 
 import {
   UnknownPlistReal,
@@ -7,8 +9,10 @@ import {
   parsePlistNonFiniteRealLiteral,
 } from "./apple/plistNumbers.js";
 
-/** Interpret an already validated, depth-bounded DOM, owning values and number facts together. */
-export const decodeXmlPropertyList = (document: Document) => {
+/** Own source interpretation, depth, values and numeric facts together. */
+export const decodeXmlPropertyList = (text: string) => {
+  const source = readXmlPlistSource(text);
+  const document = source.document;
   const root = document.documentElement;
   if (root?.nodeName !== "plist")
     throw new TypeError("malformed document. First element should be <plist>");
@@ -134,8 +138,9 @@ export const decodeXmlPropertyList = (document: Document) => {
         return value;
       }
       case "data": {
-        const binary = atob(content(node, textNode).text.replace(/\s+/gu, ""));
-        return Uint8Array.from(binary, (character) => character.charCodeAt(0));
+        const value = source.dataValue(node);
+        if (!value.ok) throw value.error;
+        return value.value;
       }
       case "date": {
         const value = requireContent(node).nodeValue;

src/domain/propertyListXmlData.test.ts

@@ -0,0 +1,78 @@
+import { expect, it } from "vitest";
+import { parseXmlPropertyList } from "./propertyListKeys.js";
+
+// Expected bytes were independently read from plutil's binary conversion.
+it.each([
+  ["AQID", "eb9408"],
+  ["&AQID", "6a6a40"],
+  ["AQ
ID", "010d74"],
+  ["éAQID", "c44f40"],
+  ["====", "00"],
+  ["=AQI", "000408"],
+  ["AQI=Z", "0102"],
+  ["AQ=!==", "0100"],
+])(
+  "retains CFPropertyList data bytes from source spelling %j",
+  (literal, hex) => {
+    const parsed = parseXmlPropertyList(
+      `<plist><data>${literal}</data></plist>`,
+    );
+    expect(parsed.value).toEqual(Uint8Array.from(Buffer.from(hex, "hex")));
+  },
+);
+
+it.each(["é", "😀", "\u00a0", "\u0085", "\u2028", "\u2029"])(
+  "rejects non-ASCII data before source normalization can hide %j",
+  (character) => {
+    expect(() =>
+      parseXmlPropertyList(`<plist><data>AQ${character}ID</data></plist>`),
+    ).toThrow(
+      expect.objectContaining({
+        _tag: "XmlPlistDataError",
+        reason: "non-ascii",
+        characterOffset: 2,
+      }),
+    );
+  },
+);
+
+it.each([
+  "A<!-- omitted -->QID",
+  "<![CDATA[AQID]]>",
+  "A<?rea ignored?>QID",
+  "<string>AQID</string>",
+])(
+  "rejects data markup instead of silently decoding a different source: %j",
+  (literal) => {
+    expect(() =>
+      parseXmlPropertyList(`<plist><data>${literal}</data></plist>`),
+    ).toThrow(expect.objectContaining({ reason: "markup" }));
+  },
+);
+
+it.each(["", ' note=">"', " note='>/'"])(
+  "distinguishes explicit empty data from a self-closing element with attributes %j",
+  (attributes) => {
+    expect(
+      parseXmlPropertyList(`<plist><data${attributes}></data></plist>`).value,
+    ).toEqual(new Uint8Array());
+    expect(() =>
+      parseXmlPropertyList(`<plist><data${attributes}/></plist>`),
+    ).toThrow(expect.objectContaining({ reason: "empty-element" }));
+  },
+);
+
+it.each(["\n", "\r\n", "\r", "\u0085", "\u2028", "\u2029"])(
+  "binds raw data after astral text, entities and line ending %j to the right node",
+  (ending) => {
+    const parsed = parseXmlPropertyList(
+      `<plist><array><string note=">">π😀&#10;${ending}before</string>${ending}` +
+        "<data>&#65;QID</data><data>AQID</data></array></plist>",
+    );
+    expect(parsed.value).toEqual([
+      expect.any(String),
+      Uint8Array.from([235, 148, 8]),
+      Uint8Array.from([1, 2, 3]),
+    ]);
+  },
+);

src/domain/propertyListXmlData.ts

@@ -0,0 +1,65 @@
+import { err, ok, type Result } from "./result.js";
+
+export class XmlPlistDataError extends TypeError {
+  readonly _tag = "XmlPlistDataError";
+  constructor(
+    readonly reason:
+      | "non-ascii"
+      | "markup"
+      | "empty-element"
+      | "source-location",
+    /** UTF-16 offset within the raw data literal, when known. */
+    readonly characterOffset: number | null = null,
+  ) {
+    super(`Cannot decode XML plist <data>: ${reason}`);
+    this.name = "XmlPlistDataError";
+  }
+}
+
+const BASE64_DIGITS =
+  "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
+const C_WHITESPACE = new Set([" ", "\t", "\n", "\v", "\f", "\r"]);
+
+/** Interpret each completed quartet once per pass, including CF's padding resets. */
+const visitDataQuartets = (
+  text: string,
+  visit: (value: number, byteCount: 1 | 2 | 3) => void,
+): Result<void, XmlPlistDataError> => {
+  let quartet = 0;
+  let digits = 0;
+  let padding = 0;
+  for (let index = 0; index < text.length; index += 1) {
+    if (text.charCodeAt(index) >= 128)
+      return err(new XmlPlistDataError("non-ascii", index));
+    const character = text[index]!;
+    if (character === "=") padding += 1;
+    else if (!C_WHITESPACE.has(character)) padding = 0;
+    const digit = character === "=" ? 0 : BASE64_DIGITS.indexOf(character);
+    if (digit < 0) continue;
+    quartet = ((quartet << 6) | digit) & 0xffffff;
+    digits += 1;
+    if (digits % 4 === 0)
+      visit(quartet, padding >= 2 ? 1 : padding === 1 ? 2 : 3);
+  }
+  return ok(undefined);
+};
+
+/** CFPropertyList reads raw ASCII, skips nonalphabet bytes and emits complete quartets. */
+export const decodeXmlPlistData = (
+  text: string,
+): Result<Uint8Array, XmlPlistDataError> => {
+  let length = 0;
+  const counted = visitDataQuartets(text, (_value, byteCount) => {
+    length += byteCount;
+  });
+  if (!counted.ok) return counted;
+  // Count actual output before allocation, including padding in every quartet.
+  const bytes = new Uint8Array(length);
+  let offset = 0;
+  const decoded = visitDataQuartets(text, (value, byteCount) => {
+    bytes[offset++] = value >> 16;
+    if (byteCount >= 2) bytes[offset++] = value >> 8;
+    if (byteCount === 3) bytes[offset++] = value;
+  });
+  return decoded.ok ? ok(bytes) : decoded;
+};

src/domain/propertyListXmlSource.ts

@@ -0,0 +1,128 @@
+import type { Document, Node } from "@xmldom/xmldom";
+import { MAX_PLIST_DEPTH } from "./apple/plistValue.js";
+import { err, ok, type Result } from "./result.js";
+import { parseXmlDocument } from "./xmlDocument.js";
+import {
+  decodeXmlPlistData,
+  XmlPlistDataError,
+} from "./propertyListXmlData.js";
+
+// Preserve the established XML decoder normalization; keep raw data separately.
+const LINE_ENDING = /\r[\n\u0085]|[\r\n\u0085\u2028\u2029]/gu;
+
+interface DataSpan {
+  readonly start: number;
+  readonly end: number;
+}
+
+/** One source owner binds lexical data values to their validated DOM nodes. */
+export const readXmlPlistSource = (text: string) => {
+  const normalized = text.replace(LINE_ENDING, "\n");
+  const parsed = parseXmlDocument(normalized, {
+    doctype: "allow",
+    lineEndings: "preserve",
+  });
+  if (!parsed.ok) throw parsed.error;
+  const document = parsed.value;
+  const lineEndings = new RegExp(LINE_ENDING);
+  let line = 1;
+  let lineStart = 0;
+  // The depth walk visits elements in source order, so location lookup is
+  // linear without an index entry for every line in a potentially large value.
+  const readData = (
+    node: Node,
+  ): Result<DataSpan, XmlPlistDataError["reason"]> => {
+    if (node.lineNumber === undefined || node.columnNumber === undefined)
+      return err("source-location");
+    while (line < node.lineNumber) {
+      const ending = lineEndings.exec(text);
+      if (ending === null) return err("source-location");
+      lineStart = ending.index + ending[0].length;
+      line += 1;
+    }
+    if (line !== node.lineNumber) return err("source-location");
+    for (let child = node.firstChild; child !== null; child = child.nextSibling)
+      if (child.nodeType !== child.TEXT_NODE) return err("markup");
+    const start = lineStart + node.columnNumber - 1;
+    const openingEnd = openingTagEnd(text, start);
+    if (openingEnd < 0) return err("source-location");
+    if (text[openingEnd - 1] === "/") return err("empty-element");
+    const end = text.indexOf("<", openingEnd + 1);
+    if (end < 0) return err("source-location");
+    return ok({ start: openingEnd + 1, end });
+  };
+  const data = collectXmlPlistDataSpans(document, readData);
+  return {
+    document,
+    dataValue(node: Node): Result<Uint8Array, XmlPlistDataError> {
+      const span = data.get(node);
+      if (span === undefined)
+        return err(new XmlPlistDataError("source-location"));
+      return span.ok
+        ? decodeXmlPlistData(text.slice(span.value.start, span.value.end))
+        : err(new XmlPlistDataError(span.error));
+    },
+  };
+};
+
+/** The validated DOM omits the distinction between <data/> and <data></data>. */
+const openingTagEnd = (source: string, start: number): number => {
+  let quote: string | undefined;
+  for (let cursor = start; cursor < source.length; cursor += 1) {
+    const character = source[cursor];
+    if (quote !== undefined) {
+      if (character === quote) quote = undefined;
+    } else if (character === '"' || character === "'") quote = character;
+    else if (character === ">") return cursor;
+  }
+  return -1;
+};
+
+/** Elements whose children the plist value decoder visits recursively. */
+const PLIST_CONTAINER_ELEMENTS = new Set(["plist", "dict", "array"]);
+
+/**
+ * Reject XML plists nested deeper than the projection supports before any
+ * recursive value pass runs. Value decoding and prototype omission recurse once
+ * per nesting level, so a deeply nested document exhausts the call stack
+ * before the projection limit can report the resource bound. The failure
+ * matches `projectPlistValue` so callers classify it as a limit failure.
+ *
+ * Depth counts values the way the projection does: the root value is level 0
+ * and every container member is one level deeper. A dictionary key names a
+ * member instead of nesting one, and a self-closing element is the value at
+ * its own level. The outer plist envelope is unwrapped by the decoder;
+ * nested plist elements decode as arrays and must count toward the bound.
+ */
+const collectXmlPlistDataSpans = (
+  document: Document,
+  readData: (node: Node) => Result<DataSpan, XmlPlistDataError["reason"]>,
+): WeakMap<Node, Result<DataSpan, XmlPlistDataError["reason"]>> => {
+  const data = new WeakMap<
+    Node,
+    Result<DataSpan, XmlPlistDataError["reason"]>
+  >();
+  const root = document.documentElement;
+  if (root === null) return data;
+  const pending: Array<{ node: Node; depth: number }> = [
+    { node: root, depth: -1 },
+  ];
+  while (pending.length > 0) {
+    const entry = pending.pop();
+    if (entry === undefined) break;
+    const { node, depth } = entry;
+    // Schedule only the next sibling, keeping traversal storage proportional
+    // to depth rather than the number of members in a wide container.
+    if (node.nextSibling !== null)
+      pending.push({ node: node.nextSibling, depth });
+    if (node.nodeType !== node.ELEMENT_NODE) continue;
+    if (node.nodeName !== "key" && depth > MAX_PLIST_DEPTH)
+      throw new RangeError(
+        `Plist exceeds ${String(MAX_PLIST_DEPTH)} nesting levels`,
+      );
+    if (node.nodeName === "data") data.set(node, readData(node));
+    if (PLIST_CONTAINER_ELEMENTS.has(node.nodeName) && node.firstChild !== null)
+      pending.push({ node: node.firstChild, depth: depth + 1 });
+  }
+  return data;
+};

tests/acceptance/applications/keyedArchiveXmlNumbers.test.ts

@@ -39,8 +39,28 @@ cliTest.for([
     element: `<integer>1${"0".repeat(400)}</integer>`,
     expected: { $plist_type: "integer", decimal: `1${"0".repeat(400)}` },
   },
+  {
+    name: "incomplete data quartet",
+    element: "<data>AQI</data>",
+    expected: { $plist_type: "data", base64: "" },
+  },
+  {
+    name: "data with a non-alphabet character",
+    element: "<data>AQ!ID</data>",
+    expected: { $plist_type: "data", base64: "AQID" },
+  },
+  {
+    name: "data with a numeric entity spelling",
+    element: "<data>&#65;QID</data>",
+    expected: { $plist_type: "data", base64: "65QI" },
+  },
+  {
+    name: "data with a predefined entity spelling",
+    element: "<data>&amp;AQID</data>",
+    expected: { $plist_type: "data", base64: "ampA" },
+  },
 ])(
-  "preserves $name XML numbers through CLI and MCP",
+  "preserves $name XML scalars through CLI and MCP",
   async (scenario, { cli }) => {
     const root = await createTestTempDirectory("rea-keyed-xml-numbers-");
     const bytes = Buffer.from(
@@ -116,3 +136,54 @@ cliTest.for([
     expect(await readFile(path)).toEqual(bytes);
   },
 );
+
+cliTest(
+  "refuses invalid data without publishing observed bytes through CLI and MCP",
+  async ({ cli }) => {
+    const root = await createTestTempDirectory("rea-keyed-data-refusal-");
+    const { call } = await connectLocalToolsMcp({
+      HOME: root,
+      USERPROFILE: root,
+    });
+    for (const [name, element, reason] of [
+      ["non-ascii", "<data>AQ\u0085ID</data>", "non-ascii"],
+      ["markup", "<data>A<!-- ignored -->QID</data>", "markup"],
+      ["self-closing", '<data note=">"/>', "empty-element"],
+    ] as const) {
+      const path = join(root, `${name}.plist`);
+      const bytes = Buffer.from(
+        build({
+          $archiver: "NSKeyedArchiver",
+          $version: 100000,
+          $objects: ["$null", "__VALUE__"],
+          $top: { root: { CF$UID: 1 } },
+        }).replace("<string>__VALUE__</string>", element),
+      );
+      await writeFile(path, bytes);
+      const output = await cli.run({
+        arguments: ["inspect-keyed-archive", path, "--json"],
+        environment: { HOME: root, USERPROFILE: root, REA_LOG_LEVEL: "silent" },
+      });
+      expect(output.exitCode).toBe(1);
+      expect(output.json).toMatchObject({
+        code: "artifact_operation_failed",
+        details: { reason: "format", detail: expect.stringContaining(reason) },
+      });
+      expect((await call("open_binary", { path })).isError).not.toBe(true);
+      const reply = await call("inspect_keyed_archive", {});
+      expect(reply.isError).toBe(true);
+      const block = reply.content.find((item) => item.type === "text");
+      if (block?.type !== "text") throw new Error("Missing MCP error text");
+      expect(JSON.parse(block.text)).toMatchObject({
+        error: {
+          code: "artifact_operation_failed",
+          details: {
+            reason: "format",
+            detail: expect.stringContaining(reason),
+          },
+        },
+      });
+      expect(await readFile(path)).toEqual(bytes);
+    }
+  },
+);