fix(apktool): distinguish document failures from entry omissions

Return tagged string-document parsing failures instead of fabricating one unparsed entry. Preserve XML failure reasons and publish unknown counts with partial coverage; count only examined unsupported entries in validated documents. Migrate every consumer, remove the superseded throwing XML wrapper, and retain completed failure Evidence through cancellation and failed cleanup. Adapt the document-versus-entry distinction from PR #4804 using the shared Result and XML owner. Multi-table projection and its aggregate bounds remain under review in #4777. Validation before incoming main rebase: 229 focused tests in 15 files, four baseline controls fail, static/docs checks and two source reviews. Twelve tiny actual MCP workflows and four CLI controls; authored launchers only, no real Apktool/JVM/Windows qualification. Fixes #4820 Co-authored-by: Abhay Tiwari <abhaytiwari0821@gmail.com>
2026-10-26 22:28:27 -04:00 · 2026-10-17 23:27:22 +08:00
parent bec88b9d77
commit dc2e8117b6
5 changed files with 251 additions and 96 deletions
@@ -1,4 +1,5 @@
 import { describe, expect, it } from "vitest";
+import { ok } from "../domain/result.js";

 import {
  parseApktoolYml,
@@ -161,30 +162,37 @@ describe("parseDecodedStrings", () => {
      "</resources>",
      "",
    ].join("\n");
-    expect(parseDecodedStrings(stringsXml)).toEqual({
-      entries: [
-        { name: "api_endpoint", value: "https://api.example.com/v1" },
-        { name: "app_name", value: "ApktoolProbe" },
-        { name: "debug_flag", value: "true" },
-      ],
-      unparsedCount: 0,
-    });
+    expect(parseDecodedStrings(stringsXml)).toEqual(
+      ok({
+        entries: [
+          { name: "api_endpoint", value: "https://api.example.com/v1" },
+          { name: "app_name", value: "ApktoolProbe" },
+          { name: "debug_flag", value: "true" },
+        ],
+        unparsedCount: 0,
+      }),
+    );
  });

  it("counts entries with nested markup instead of flattening them", () => {
    const stringsXml =
      '<resources><string name="styled">Hello <b>world</b></string><string name="plain">ok</string></resources>';
-    expect(parseDecodedStrings(stringsXml)).toEqual({
-      entries: [{ name: "plain", value: "ok" }],
-      unparsedCount: 1,
-    });
+    expect(parseDecodedStrings(stringsXml)).toEqual(
+      ok({
+        entries: [{ name: "plain", value: "ok" }],
+        unparsedCount: 1,
+      }),
+    );
  });

  it("preserves escaped entities verbatim", () => {
    const stringsXml =
      '<resources><string name="escaped">a &amp; b &lt;c&gt;</string></resources>';
-    expect(parseDecodedStrings(stringsXml).entries[0]?.value).toBe(
-      "a &amp; b &lt;c&gt;",
+    expect(parseDecodedStrings(stringsXml)).toEqual(
+      ok({
+        entries: [{ name: "escaped", value: "a &amp; b &lt;c&gt;" }],
+        unparsedCount: 0,
+      }),
    );
  });

@@ -193,22 +201,28 @@ describe("parseDecodedStrings", () => {
      parseDecodedStrings(
        '<resources><string name="good">Hello</string><string name="edge">\uFFFD</string></resources>',
      ),
-    ).toEqual({
-      entries: [
-        { name: "good", value: "Hello" },
-        { name: "edge", value: "\uFFFD" },
-      ],
-      unparsedCount: 0,
-    });
+    ).toEqual(
+      ok({
+        entries: [
+          { name: "good", value: "Hello" },
+          { name: "edge", value: "\uFFFD" },
+        ],
+        unparsedCount: 0,
+      }),
+    );
  });

  it.each([
    "<resources><string name=unquoted>\uFFFD</string></resources>",
    '<resources><string name="broken">\uFFFD</resources>',
  ])("still rejects malformed XML containing U+FFFD: %s", (xml) => {
-    expect(parseDecodedStrings(xml)).toEqual({
-      entries: [],
-      unparsedCount: 1,
+    expect(parseDecodedStrings(xml)).toMatchObject({
+      ok: false,
+      error: {
+        _tag: "ApktoolStringsDocumentError",
+        reason: "malformed",
+        cause: { _tag: "XmlDocumentParseError" },
+      },
    });
  });

@@ -217,13 +231,15 @@ describe("parseDecodedStrings", () => {
      parseDecodedStrings(
        '<resources><string translatable="false" name="empty"/><string name="paired"></string></resources>',
      ),
-    ).toEqual({
-      entries: [
-        { name: "empty", value: "" },
-        { name: "paired", value: "" },
-      ],
-      unparsedCount: 0,
-    });
+    ).toEqual(
+      ok({
+        entries: [
+          { name: "empty", value: "" },
+          { name: "paired", value: "" },
+        ],
+        unparsedCount: 0,
+      }),
+    );
  });

  it("ignores comments without mistaking them for nested markup", () => {
@@ -231,10 +247,12 @@ describe("parseDecodedStrings", () => {
      parseDecodedStrings(
        '<resources><!-- <string name="fake">bad</string> --><string name="real">before<!-- note -->after</string></resources>',
      ),
-    ).toEqual({
-      entries: [{ name: "real", value: "beforeafter" }],
-      unparsedCount: 0,
-    });
+    ).toEqual(
+      ok({
+        entries: [{ name: "real", value: "beforeafter" }],
+        unparsedCount: 0,
+      }),
+    );
  });

  it("does not include a trailing document comment in the last string", () => {
@@ -242,18 +260,25 @@ describe("parseDecodedStrings", () => {
      parseDecodedStrings(
        '<resources><string name="a">one</string></resources><!-- </string> -->',
      ),
-    ).toEqual({
-      entries: [{ name: "a", value: "one" }],
-      unparsedCount: 0,
-    });
+    ).toEqual(
+      ok({
+        entries: [{ name: "a", value: "one" }],
+        unparsedCount: 0,
+      }),
+    );
  });

  it("preserves source whitespace and entity spelling", () => {
    expect(
      parseDecodedStrings(
        '<resources><string name="raw">  a &amp; b  </string></resources>',
-      ).entries,
-    ).toEqual([{ name: "raw", value: "  a &amp; b  " }]);
+      ),
+    ).toEqual(
+      ok({
+        entries: [{ name: "raw", value: "  a &amp; b  " }],
+        unparsedCount: 0,
+      }),
+    );
  });

  it("keeps source slices aligned across CRLF and CR line endings", () => {
@@ -262,27 +287,48 @@ describe("parseDecodedStrings", () => {
        parseDecodedStrings(
          `<resources>${newline}  <string name="raw">x &amp; y</string>${newline}</resources>`,
        ),
-      ).toEqual({
-        entries: [{ name: "raw", value: "x &amp; y" }],
-        unparsedCount: 0,
-      });
+      ).toEqual(
+        ok({
+          entries: [{ name: "raw", value: "x &amp; y" }],
+          unparsedCount: 0,
+        }),
+      );
    }
  });
+});

+describe("string document validity and omission counts", () => {
  it("counts a string with no name as unsupported instead of emitting an empty name", () => {
    expect(
      parseDecodedStrings("<resources><string>value</string></resources>"),
-    ).toEqual({ entries: [], unparsedCount: 1 });
+    ).toEqual(ok({ entries: [], unparsedCount: 1 }));
  });

-  it("reports malformed XML and unsupported string markup as partial", () => {
+  it("distinguishes malformed XML from counted unsupported string markup", () => {
    expect(
      parseDecodedStrings("<resources><string name='broken'>no</resources>"),
-    ).toEqual({ entries: [], unparsedCount: 1 });
+    ).toMatchObject({
+      ok: false,
+      error: { _tag: "ApktoolStringsDocumentError", reason: "malformed" },
+    });
    expect(
      parseDecodedStrings(
        '<resources><string name="styled">Hello <b>world</b></string></resources>',
      ),
-    ).toEqual({ entries: [], unparsedCount: 1 });
+    ).toEqual(ok({ entries: [], unparsedCount: 1 }));
  });
 });
+
+it.each(["<manifest/>", '<other><string name="a">one</string></other>'])(
+  "keeps a wrong document root separate from an entry count: %s",
+  (xml) => {
+    const parsed = parseDecodedStrings(xml);
+    expect(parsed).toMatchObject({
+      ok: false,
+      error: { _tag: "ApktoolStringsDocumentError", reason: "wrong_root" },
+    });
+    expect(parsed).not.toHaveProperty("value");
+    if (parsed.ok) throw new Error("Expected document failure");
+    expect(parsed.error).not.toHaveProperty("unparsedCount");
+  },
+);
@@ -1,5 +1,6 @@
-import type { Document, Element, Node } from "@xmldom/xmldom";
+import type { Element, Node } from "@xmldom/xmldom";
 import { parseXmlDocument } from "../domain/xmlDocument.js";
+import { ok, err, type Result } from "../domain/result.js";
 import {
  isMap,
  isScalar,
@@ -53,8 +54,16 @@ export const parseApktoolYml = (yml: string): ApktoolMetadata => {

 /** Read the package attribute from the decoded Android manifest root. */
 export const parseManifestPackage = (manifest: string): string | null => {
-  const document = parseXml(manifest, "AndroidManifest.xml");
-  const root = document.documentElement;
+  const parsed = parseXmlDocument(manifest, {
+    doctype: "allow",
+    lineEndings: "preserve",
+  });
+  if (!parsed.ok)
+    throw new TypeError(
+      `AndroidManifest.xml is not well-formed XML: ${parsed.error.message}`,
+      { cause: parsed.error },
+    );
+  const root = parsed.value.documentElement;
  if (root === null || root.nodeName !== "manifest")
    throw new TypeError("AndroidManifest.xml root must be <manifest>");
  return root.getAttribute("package") || null;
@@ -65,6 +74,18 @@ export interface DecodedStringEntry {
  readonly value: string;
 }

+/** A whole-document failure does not establish an omitted-entry count. */
+export class ApktoolStringsDocumentError extends Error {
+  readonly _tag = "ApktoolStringsDocumentError";
+  constructor(
+    readonly reason: "malformed" | "wrong_root",
+    message: string,
+    options?: ErrorOptions,
+  ) {
+    super(message, options);
+  }
+}
+
 /**
 * Project direct `<string>` children from decoded resources XML. DOM parsing
 * identifies elements and attributes; text values are sliced from the input
@@ -73,19 +94,31 @@ export interface DecodedStringEntry {
 */
 export const parseDecodedStrings = (
  stringsXml: string,
-): {
-  entries: readonly DecodedStringEntry[];
-  unparsedCount: number;
-} => {
-  let document: Document;
-  try {
-    document = parseXml(stringsXml, "strings.xml");
-  } catch {
-    return { entries: [], unparsedCount: 1 };
-  }
-  const root = document.documentElement;
+): Result<
+  {
+    readonly entries: readonly DecodedStringEntry[];
+    readonly unparsedCount: number;
+  },
+  ApktoolStringsDocumentError
+> => {
+  const parsed = parseXmlDocument(stringsXml, {
+    doctype: "allow",
+    lineEndings: "preserve",
+  });
+  if (!parsed.ok)
+    return err(
+      new ApktoolStringsDocumentError("malformed", parsed.error.message, {
+        cause: parsed.error,
+      }),
+    );
+  const root = parsed.value.documentElement;
  if (root === null || root.nodeName !== "resources")
-    return { entries: [], unparsedCount: 1 };
+    return err(
+      new ApktoolStringsDocumentError(
+        "wrong_root",
+        "String resource document root must be <resources>",
+      ),
+    );

  const entries: DecodedStringEntry[] = [];
  let unparsedCount = 0;
@@ -125,7 +158,7 @@ export const parseDecodedStrings = (
    }
    entries.push({ name, value: value.join("") });
  }
-  return { entries, unparsedCount };
+  return ok({ entries, unparsedCount });
 };

 const yamlScalar = (
@@ -142,19 +175,6 @@ const yamlScalar = (
    : source.slice(range[0], range[1]);
 };

-const parseXml = (xml: string, label: string) => {
-  const parsed = parseXmlDocument(xml, {
-    doctype: "allow",
-    lineEndings: "preserve",
-  });
-  if (!parsed.ok)
-    throw new TypeError(
-      `${label} is not well-formed XML: ${parsed.error.message}`,
-      { cause: parsed.error },
-    );
-  return parsed.value;
-};
-
 const childNodes = (node: Node): Node[] => {
  const children: Node[] = [];
  for (let child = node.firstChild; child !== null; child = child.nextSibling)
@@ -688,15 +688,30 @@ it("retains completed Evidence and live cleanup until a denied workspace removal
 });

 it.each([
-  [false, "2.7.0-dirty", "CVE-2024-21633"],
-  [true, "2.7.0-dirty", "CVE-2024-21633"],
-  [false, "3.0.1", "CVE-2026-39973"],
-  [true, "3.0.1", "CVE-2026-39973"],
+  [false, "2.7.0-dirty", "CVE-2024-21633", undefined],
+  [true, "2.7.0-dirty", "CVE-2024-21633", undefined],
+  [false, "3.0.1", "CVE-2026-39973", undefined],
+  [true, "3.0.1", "CVE-2026-39973", undefined],
+  [
+    false,
+    "3.0.1",
+    "CVE-2026-39973",
+    "<resources><string name=unquoted>bad</string></resources>",
+  ],
+  [
+    true,
+    "3.0.1",
+    "CVE-2026-39973",
+    "<resources><string name=unquoted>bad</string></resources>",
+  ],
 ] as const)(
-  "preserves completed Evidence and cancellation during workspace cleanup (denied=%s, version=%s)",
-  async (deniedInitially, version, advisory) => {
+  "preserves completed Evidence and cancellation during workspace cleanup (denied=%s, version=%s, advisory=%s, XML=%s)",
+  async (deniedInitially, version, advisory, stringsXml) => {
    const { command, root } = await writeApktoolStub((root) =>
-      decodeStub()(root).replace(VERSION_BLOCK, `printf '%s\\n' '${version}'`),
+      decodeStub(stringsXml === undefined ? {} : { stringsXml })(root).replace(
+        VERSION_BLOCK,
+        `printf '%s\\n' '${version}'`,
+      ),
    );
    const apk = await apkFixture(root);
    const controller = new AbortController();
@@ -730,6 +745,12 @@ it.each([
          expect.stringContaining(advisory),
        ]),
      });
+      if (stringsXml !== undefined)
+        expect(result.error.partialObservation).toMatchObject({
+          limitations: expect.arrayContaining([
+            expect.stringContaining("entry counts are unknown"),
+          ]),
+        });
      denied = false;
      await provider.close();
      expect(result.error.cleanupIncomplete).toBe(false);
@@ -1084,3 +1105,58 @@ it.each([
    }
  },
 );
+
+it.each([
+  [
+    "malformed",
+    '<resources><string name="a">one</string><string name="b">two</resources>',
+    [],
+  ],
+  ["wrong_root", "<manifest/>", []],
+  [
+    "unsupported",
+    '<resources><string name="a"><b>A</b></string><string>unnamed</string><string name="good">ok</string></resources>',
+    [{ name: "good", value: "ok" }],
+  ],
+] as const)(
+  "reports document projection semantics for %s (#4820)",
+  async (kind, stringsXml, strings) => {
+    const { command, root } = await writeApktoolStub(
+      decodeStub({ stringsXml }),
+    );
+    const apk = await apkFixture(root);
+    const provider = providerOver(command);
+    try {
+      const result = await provider.execute({
+        operation: "decode_android_resources",
+        input: { path: apk.path, include_strings: true },
+      });
+      if (!result.ok) throw result.error;
+      expect(result.value.result).toMatchObject({
+        strings,
+        target: { sha256: apk.sha256 },
+        coverage: "partial",
+      });
+      const limitations = result.value.limitations.join("\n");
+      if (kind === "unsupported") {
+        expect(limitations).toContain(
+          "String entries omitted due to unsupported markup or missing names: 2",
+        );
+        expect(limitations).not.toContain("entry counts are unknown");
+      } else {
+        expect(limitations).toContain(
+          'String table "res/values/strings.xml" could not be projected:',
+        );
+        expect(limitations).toContain("entry counts are unknown");
+        expect(limitations).not.toContain("String entries omitted");
+        expect(limitations).toContain(
+          kind === "wrong_root"
+            ? "root must be <resources>"
+            : "XML is not well-formed",
+        );
+      }
+    } finally {
+      await provider.close();
+    }
+  },
+);
@@ -598,11 +598,13 @@ export class ApktoolProvider {
        : { text: null, truncated: false };
      // Do not parse an incomplete XML document as though its projection were complete.
      const stringsXml = stringsRead.truncated ? null : stringsRead.text;
+      const stringsProjection =
+        stringsXml === null ? undefined : parseDecodedStrings(stringsXml);
      const parsedStrings =
-        stringsXml === null
-          ? { entries: [], unparsedCount: 0 }
-          : parseDecodedStrings(stringsXml);
-      const bounded = parsedStrings.entries.slice(0, MAX_STRINGS);
+        stringsProjection?.ok === true ? stringsProjection.value : undefined;
+      const documentFailure =
+        stringsProjection?.ok === false ? stringsProjection.error : undefined;
+      const bounded = parsedStrings?.entries.slice(0, MAX_STRINGS) ?? [];
      const files = await countDecodedFiles(
        resources,
        workspace,
@@ -611,10 +613,12 @@ export class ApktoolProvider {
      );
      throwIfCancelled(operation, signal);
      const coverage =
-        (stringsXml !== null && parsedStrings.entries.length > MAX_STRINGS) ||
+        (parsedStrings !== undefined &&
+          parsedStrings.entries.length > MAX_STRINGS) ||
        manifestRead.truncated ||
        stringsRead.truncated ||
-        parsedStrings.unparsedCount > 0 ||
+        (parsedStrings !== undefined && parsedStrings.unparsedCount > 0) ||
+        documentFailure !== undefined ||
        files.capped ||
        files.unreadableStringsTables.length > 0
          ? "partial"
@@ -656,12 +660,18 @@ export class ApktoolProvider {
                    "No default res/values/strings.xml was found in the decoded workspace",
                  ]
                : []),
-              ...(parsedStrings.unparsedCount > 0
+              ...(documentFailure === undefined
+                ? []
+                : [
+                    `String table ${JSON.stringify(`res/${stringsDirectory}/strings.xml`)} could not be projected: ${documentFailure.message}; entry counts are unknown`,
+                  ]),
+              ...(parsedStrings !== undefined && parsedStrings.unparsedCount > 0
                ? [
-                    `String entries omitted due to malformed XML or unsupported markup: ${String(parsedStrings.unparsedCount)}`,
+                    `String entries omitted due to unsupported markup or missing names: ${String(parsedStrings.unparsedCount)}`,
                  ]
                : []),
-              ...(parsedStrings.entries.length > MAX_STRINGS
+              ...(parsedStrings !== undefined &&
+              parsedStrings.entries.length > MAX_STRINGS
                ? [
                    `strings reports the first ${String(MAX_STRINGS)} of ${String(parsedStrings.entries.length)} entries`,
                  ]