diff --git a/docs/apktool-resource-analysis.md b/docs/apktool-resource-analysis.md index 8c98f762f..3632cce22 100644 --- a/docs/apktool-resource-analysis.md +++ b/docs/apktool-resource-analysis.md @@ -85,8 +85,11 @@ select Android's `values-pt-rBR`; Android qualifiers such as `pt-rBR` and truncated when larger. - **Strings are projected verbatim.** Escaped entities stay escaped, and entries containing nested markup (``, ``) are counted as - unparsed rather than flattened — the limitation names the count. The - default table is bounded to 2000 entries with partial coverage when more + unparsed rather than flattened — the limitation names the examined count. + Malformed XML and a wrong document root are whole-document projection + failures: the result is partial, the failure reason is disclosed, and entry + counts remain unknown. They are not counted as one unsupported string. + The default table is bounded to 2000 entries with partial coverage when more exist. `locales` lists observed regular `strings.xml` tables whose exact `values-` directory is selectable by the request's locale grammar. Locale suffixes allow up to 23 ASCII characters, derived from Apktool's diff --git a/src/apktool/ApktoolDecodeOutput.test.ts b/src/apktool/ApktoolDecodeOutput.test.ts index 94ff2ae81..ed6140ff4 100644 --- a/src/apktool/ApktoolDecodeOutput.test.ts +++ b/src/apktool/ApktoolDecodeOutput.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { ok } from "../domain/result.js"; import { parseApktoolYml, @@ -161,30 +162,37 @@ describe("parseDecodedStrings", () => { "", "", ].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 = 'Hello worldok'; - 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 = 'a & b <c>'; - expect(parseDecodedStrings(stringsXml).entries[0]?.value).toBe( - "a & b <c>", + expect(parseDecodedStrings(stringsXml)).toEqual( + ok({ + entries: [{ name: "escaped", value: "a & b <c>" }], + unparsedCount: 0, + }), ); }); @@ -193,22 +201,28 @@ describe("parseDecodedStrings", () => { parseDecodedStrings( 'Hello\uFFFD', ), - ).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([ "\uFFFD", '\uFFFD', ])("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( '', ), - ).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( 'beforeafter', ), - ).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( 'one', ), - ).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( ' a & b ', - ).entries, - ).toEqual([{ name: "raw", value: " a & b " }]); + ), + ).toEqual( + ok({ + entries: [{ name: "raw", value: " a & b " }], + unparsedCount: 0, + }), + ); }); it("keeps source slices aligned across CRLF and CR line endings", () => { @@ -262,27 +287,48 @@ describe("parseDecodedStrings", () => { parseDecodedStrings( `${newline} x & y${newline}`, ), - ).toEqual({ - entries: [{ name: "raw", value: "x & y" }], - unparsedCount: 0, - }); + ).toEqual( + ok({ + entries: [{ name: "raw", value: "x & 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("value"), - ).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("no"), - ).toEqual({ entries: [], unparsedCount: 1 }); + ).toMatchObject({ + ok: false, + error: { _tag: "ApktoolStringsDocumentError", reason: "malformed" }, + }); expect( parseDecodedStrings( 'Hello world', ), - ).toEqual({ entries: [], unparsedCount: 1 }); + ).toEqual(ok({ entries: [], unparsedCount: 1 })); }); }); + +it.each(["", 'one'])( + "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"); + }, +); diff --git a/src/apktool/ApktoolDecodeOutput.ts b/src/apktool/ApktoolDecodeOutput.ts index 4638aca29..949678146 100644 --- a/src/apktool/ApktoolDecodeOutput.ts +++ b/src/apktool/ApktoolDecodeOutput.ts @@ -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 "); 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 `` 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 ", + ), + ); 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) diff --git a/src/apktool/ApktoolProvider.test.ts b/src/apktool/ApktoolProvider.test.ts index 5c02c3c8e..dd6a39dbd 100644 --- a/src/apktool/ApktoolProvider.test.ts +++ b/src/apktool/ApktoolProvider.test.ts @@ -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", + "bad", + ], + [ + true, + "3.0.1", + "CVE-2026-39973", + "bad", + ], ] 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", + 'onetwo', + [], + ], + ["wrong_root", "", []], + [ + "unsupported", + 'Aunnamedok', + [{ 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 " + : "XML is not well-formed", + ); + } + } finally { + await provider.close(); + } + }, +); diff --git a/src/apktool/ApktoolProvider.ts b/src/apktool/ApktoolProvider.ts index 7681774ac..7599bb6e8 100644 --- a/src/apktool/ApktoolProvider.ts +++ b/src/apktool/ApktoolProvider.ts @@ -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`, ]