diff --git a/docs/desktop-application-export.md b/docs/desktop-application-export.md index 173bb93b0c..5125b8d47f 100644 --- a/docs/desktop-application-export.md +++ b/docs/desktop-application-export.md @@ -599,10 +599,11 @@ when a DOCTYPE names a DTD. Known differences remain: - dconf databases are reported as binary, since REA reads them with `dconf`; inside an archive, one is `dconf` with an error, as in REA. -- Zip entry names: a name in an Info-ZIP Unicode Path extra field is used only - where Python's `zipfile` reads it (3.12 and later), and an entry whose local - header names it differently from the central directory is damaged rather - than read. +- Zip entries whose local header names them differently from the central + directory are damaged rather than read. Entry names follow REA's Unicode + Path selection: the final extra field replaces earlier claims, a bad version + or CRC falls back to the stored name, and the UTF-8 flag takes precedence. + Invalid UTF-8 in the selected name makes the archive view unreadable. - SQLite databases are read in place, read-only, rather than from a staged copy; a write lock the running app holds is waited out for a quarter of a second and then gives the file an `error`, and so does a table or column name that diff --git a/src/domain/desktop/appExportPythonZip.ts b/src/domain/desktop/appExportPythonZip.ts index 3197fdd8b1..3f6dab1edc 100644 --- a/src/domain/desktop/appExportPythonZip.ts +++ b/src/domain/desktop/appExportPythonZip.ts @@ -60,11 +60,8 @@ def _entry_bytes(archive, info): return data if len(data) == info.file_size else None -def _zip_entry(archive, info, refusal): +def _zip_entry(archive, info, refusal, name): """One entry of a zip view: its name, size and digest, and its own format and view, or why it was not read.""" - # zipfile can override a UTF-8-flagged name with a Unicode Path extra field. - # REA's ZIP reader gives the explicitly UTF-8 stored name precedence. - name = info.orig_filename if info.flag_bits & 0x800 else info.filename entry = {"name": name, "size": info.file_size, "sha256": None, "format": None, "view": None, "view_error": refusal} if refusal is not None: @@ -109,35 +106,48 @@ def _zip_end(file, size: int): def _zip_unicode_path(raw: bytes, flags: int, extra: bytes): - """An Info-ZIP Unicode Path when zip.js uses it: version 1, no UTF-8 flag, and the CRC-32 of the stored name.""" - at = 0 + """The last Info-ZIP Unicode Path, validated only after it replaces earlier claims, as in zip.js.""" + at, data, length = 0, None, 0 + while at + 4 <= len(extra): + kind, size = struct.unpack_from("= 5 and len(data) == length and data[0] == 1 and not flags & 0x800 + and int.from_bytes(data[1:5], "little") == zlib.crc32(raw)) + return data[5:] if usable else None + + +def _zip_name(raw: bytes, flags: int, unicode_path): + """The exact REA name, shared by directory classification and the retained entry view.""" + if flags & 0x800: + return raw.decode("utf-8") + return raw.decode("cp437") if unicode_path is None else unicode_path.decode("utf-8") + + +def _zip_kept_record(fixed: bytes, rest: bytes, name_length: int, extra_length: int): + """Remove already-interpreted Unicode claims from the bounded decoder copy. zipfile otherwise interprets each + occurrence, retaining an earlier valid name or rejecting invalid UTF-8 that zip.js already superseded. + All other fields, comments and the original artifact bytes remain unchanged.""" + raw, extra = rest[:name_length], rest[name_length:name_length + extra_length] + kept, at = bytearray(), 0 while at + 4 <= len(extra): kind, length = struct.unpack_from("= 5 and len(data) == length and data[0] == 1 and not flags & 0x800 - and int.from_bytes(data[1:5], "little") == zlib.crc32(raw)) - return data[5:] if usable else None - at += 4 + length - return None - - -def _zip_ends_in_slash(raw: bytes, flags: int, extra: bytes): - """Whether an entry's name, as REA decodes it, ends in a slash; None when it must be UTF-8 and is not.""" - utf8 = _zip_unicode_path(raw, flags, extra) - if utf8 is None and flags & 0x800: - utf8 = raw - if utf8 is None: - return raw.endswith(b"/") - try: - return utf8.decode("utf-8").endswith("/") - except UnicodeDecodeError: - return None + end = at + 4 + length + if end > len(extra): + raise zipfile.BadZipFile("Corrupt extra field") + if kind != 0x7075: + kept += extra[at:end] + at = end + kept += extra[at:] + header = bytearray(fixed) + struct.pack_into(" end or len(fixed) < 46 or fixed[:4] != ZIP_CENTRAL: @@ -159,14 +169,14 @@ def _zip_directory(file, size: int, keep: int): at += 46 + name_length + extra_length + comment_length if len(rest) < name_length + extra_length + comment_length or at > end: return None - slash = _zip_ends_in_slash(rest[:name_length], flags, rest[name_length:name_length + extra_length]) - if slash is None: - return None - if not (slash or (external >> 16) & 0o170000 == 0o040000 or (made_by >> 8 == 0 and external & 0x10)): + unicode_path = _zip_unicode_path(rest[:name_length], flags, rest[name_length:name_length + extra_length]) + name = _zip_name(rest[:name_length], flags, unicode_path) + if not (name.endswith("/") or (external >> 16) & 0o170000 == 0o040000 or (made_by >> 8 == 0 and external & 0x10)): count += 1 if len(records) < keep: - records.append(fixed + rest) - return start, declared, records, count + records.append(_zip_kept_record(fixed, rest, name_length, extra_length)) + names.append(name) + return start, declared, records, count, names class _BoundedArchive(io.RawIOBase): @@ -211,7 +221,7 @@ def _zip(file, size: int) -> dict: directory = _zip_directory(file, size, MAX_ZIP_ENTRIES) if directory is None: raise zipfile.BadZipFile("no readable central directory") - start, declared, records, count = directory + start, declared, records, count, names = directory kept = b"".join(records) offset = declared if declared <= 0xFFFFFFFF else start # zipfile follows a Zip64 locator signature wherever one sits before the end record, and the kept records @@ -221,10 +231,10 @@ def _zip(file, size: int) -> dict: + struct.pack("<4sIQI", ZIP64_LOCATOR, 0, offset + len(kept), 1) + struct.pack("<4s4H2IH", ZIP_END, 0, 0, len(records), len(records), len(kept), offset, 0)) with zipfile.ZipFile(_BoundedArchive(file, start, tail)) as archive: - for info in archive.infolist(): + for info, name in zip(archive.infolist(), names, strict=True): refusal = _zip_refusal(info, spent) spent += info.file_size if refusal is None else 0 - entries.append(_zip_entry(archive, info, refusal)) + entries.append(_zip_entry(archive, info, refusal, name)) except (zipfile.BadZipFile, zlib.error, EOFError, NotImplementedError, OSError, ValueError, struct.error): return {"format": "zip", "view": None, "error": "not a readable zip archive"} view = {"entries": entries, "entry_count": count, "truncated": count > len(entries)} diff --git a/src/domain/desktop/zipArchive.fixture.ts b/src/domain/desktop/zipArchive.fixture.ts index c35200a51e..01554e45c9 100644 --- a/src/domain/desktop/zipArchive.fixture.ts +++ b/src/domain/desktop/zipArchive.fixture.ts @@ -19,6 +19,21 @@ export interface ZipMember { const UTF8_NAME = 0x800; +/** A version-1 Info-ZIP Unicode Path claim bound to the stored filename bytes. */ +export const unicodePathExtra = ( + name: string, + path: string | Uint8Array, +): Buffer => { + const text = Buffer.from(path); + const field = Buffer.alloc(9 + text.length); + field.writeUInt16LE(0x7075); + field.writeUInt16LE(field.length - 4, 2); + field[4] = 1; + field.writeUInt32LE(crc32(Buffer.from(name)), 5); + text.copy(field, 9); + return field; +}; + /** A zip archive of the members in order, framed from the PKWARE APPNOTE records. */ export const zipArchive = (members: readonly ZipMember[]): Buffer => { const locals: Buffer[] = []; diff --git a/src/domain/desktop/zipDirectory.ts b/src/domain/desktop/zipDirectory.ts index 0e5a959e49..05e1474ae7 100644 --- a/src/domain/desktop/zipDirectory.ts +++ b/src/domain/desktop/zipDirectory.ts @@ -84,19 +84,23 @@ const unicodePath = ( view: DataView, { name, extra, comment, flags }: RecordName, ): Uint8Array | undefined => { + // zip.js indexes extra fields by ID with Map.set: the last occurrence + // replaces earlier ones, even when that last claim fails validation. + let selected: { at: number; length: number } | undefined; for (let at = extra; at + 4 <= comment;) { const length = view.getUint16(at + 2, true); - if (view.getUint16(at, true) === UNICODE_PATH) - return length >= 5 && - at + 4 + length <= comment && - bytes[at + 4] === 1 && - (flags & UTF8_NAME) === 0 && - view.getUint32(at + 5, true) === crc32(bytes.subarray(name, extra)) - ? bytes.subarray(at + 9, at + 4 + length) - : undefined; + if (view.getUint16(at, true) === UNICODE_PATH) selected = { at, length }; at += 4 + length; } - return undefined; + if (selected === undefined) return undefined; + const { at, length } = selected; + return length >= 5 && + at + 4 + length <= comment && + bytes[at + 4] === 1 && + (flags & UTF8_NAME) === 0 && + view.getUint32(at + 5, true) === crc32(bytes.subarray(name, extra)) + ? bytes.subarray(at + 9, at + 4 + length) + : undefined; }; /** diff --git a/tests/boundary/cli/desktopZipUnicodeCli.test.ts b/tests/boundary/cli/desktopZipUnicodeCli.test.ts new file mode 100644 index 0000000000..edd94ba816 --- /dev/null +++ b/tests/boundary/cli/desktopZipUnicodeCli.test.ts @@ -0,0 +1,205 @@ +import { createHash } from "node:crypto"; +import { readFile, writeFile } from "node:fs/promises"; +import { execFileSync } from "node:child_process"; +import { join } from "node:path"; +import { expect } from "vitest"; +import { + HOME, + capture, + change, + retained, + writeCaptureFolder, +} from "../../../src/application/desktop/desktopCapture.fixture.js"; +import { desktopApplicationStateSchema } from "../../../src/domain/desktop/desktopState.js"; +import { parseEvidence } from "../../../src/domain/evidence.js"; +import { + unicodePathExtra, + zipArchive, +} from "../../../src/domain/desktop/zipArchive.fixture.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { cliTest } from "../../support/cli/cliFixture.js"; + +const retainedZipCapture = async () => { + const member = "settings.json"; + const file = "Documents/state.zip"; + const path = `${HOME}/${file}`; + const archives = [1, 2].map((zoom) => + zipArchive([ + { + name: member, + data: JSON.stringify({ zoom }), + ...(zoom === 1 + ? {} + : { + centralExtra: Buffer.concat([ + unicodePathExtra(member, "hidden/"), + unicodePathExtra(member, member), + ]), + }), + }, + ]), + ); + const [before, after] = archives; + const base = capture(); + const [seed, end] = base.state.checkpoints; + if ( + before === undefined || + after === undefined || + seed === undefined || + end === undefined + ) + throw new Error("missing fixture checkpoint"); + const folder = await writeCaptureFolder( + { + ...base, + state: { + ...base.state, + checkpoints: [ + { ...seed, retained: [...seed.retained, retained(path, before)] }, + { + ...end, + changes: [...end.changes, change(path, { before, after })], + retained: [...end.retained, retained(path, after)], + }, + ], + }, + }, + undefined, + archives, + ); + return { member, file, path, archives, folder }; +}; + +cliTest( + "retains a ZIP file through compiled state inspection and task export despite an earlier Unicode directory claim", + async ({ cli }) => { + const { member, file, path, archives, folder } = await retainedZipCapture(); + const root = await createTestTempDirectory("desktop-zip-unicode-"); + const absentTask = join(root, "absent.json"); + await writeFile( + absentTask, + JSON.stringify({ + name: "removed", + prompt: "Remove settings.json from the archive.", + checks: [ + { + id: "removed", + kind: "value_absent", + file, + pointer: "/entries/[name=settings.json]", + }, + ], + }), + ); + const absent = await cli.run({ + arguments: [ + "export-desktop-application", + folder, + join(root, "absent-bundle"), + "--task", + absentTask, + "--json", + ], + }); + // A complete-looking empty view once made this false removal task pass + // against the captured end, despite the file still being present. + expect(absent.exitCode, absent.stdout).toBe(1); + expect(absent.json).toMatchObject({ code: "invalid_request" }); + const inspected = await cli.run({ + arguments: [ + "inspect-desktop-application-state", + folder, + "--state-path", + path, + "--json", + ], + }); + expect(inspected.exitCode, inspected.stdout).toBe(0); + const state = desktopApplicationStateSchema.parse( + parseEvidence(inspected.json).normalized_result, + ); + expect(state.paths[0]?.versions).toMatchObject( + [1, 2].map((zoom, i) => ({ + sha256: createHash("sha256").update(archives[i]!).digest("hex"), + view: { + entries: [{ name: member, view: { zoom }, view_error: null }], + entry_count: 1, + truncated: false, + }, + })), + ); + const task = join(root, "task.json"); + await writeFile( + task, + JSON.stringify({ + name: "zoom", + prompt: "Set zoom to 2 and save the document.", + checks: [ + { + id: "zoom", + kind: "value_equals", + file, + pointer: "/entries/[name=settings.json]/view/zoom", + value: 2, + }, + ], + }), + ); + const bundle = join(root, "bundle"); + const exported = await cli.run({ + arguments: [ + "export-desktop-application", + folder, + bundle, + "--task", + task, + "--json", + ], + }); + expect(exported.exitCode, exported.stdout).toBe(0); + expect(parseEvidence(exported.json).normalized_result).toMatchObject({ + tasks: [{ name: "zoom", failing_on_seed: ["zoom"] }], + }); + // Run the exact emitted reader, without the unavailable AgentEnv framework. + const module = await readFile( + join(bundle, "envs/notes/notes_app.py"), + "utf8", + ); + const helpers = module + .split("@computer_app(")[0]! + .split("\n") + .filter((line) => !line.includes("from agentenv_protocol")) + .join("\n"); + const verifier = ( + await readFile(join(bundle, "artifacts/zoom-check/verify.py"), "utf8") + ) + .split("\n") + .filter((line) => !line.includes("from agentenv_protocol")) + .join("\n"); + const copies = [join(root, "seed.zip"), join(root, "end.zip")]; + await Promise.all(copies.map((copy, i) => writeFile(copy, archives[i]!))); + const script = join(root, "reader.py"); + await writeFile( + script, + `${helpers}\n${verifier}\nseen = [_view(Path(p)) for p in json.load(sys.stdin)]\nprint(json.dumps({"views": [v["view"] for v in seen], "grades": [[r["result"] for r in evaluate(CHECKS, {${JSON.stringify(file)}: v})] for v in seen]}))\n`, + ); + const views: unknown = JSON.parse( + execFileSync( + process.platform === "win32" ? "python" : "python3", + ["-I", "-B", script], + { input: JSON.stringify(copies), encoding: "utf8" }, + ), + ); + expect(views).toMatchObject({ + views: [1, 2].map((zoom) => ({ + entries: [{ name: member, view: { zoom } }], + entry_count: 1, + truncated: false, + })), + grades: [[false], [true]], + }); + expect(await Promise.all(copies.map((copy) => readFile(copy)))).toEqual( + archives, + ); + }, +); diff --git a/tests/boundary/providers/desktopZipPython.test.ts b/tests/boundary/providers/desktopZipPython.test.ts index fb9342c28d..e62ffa76ba 100644 --- a/tests/boundary/providers/desktopZipPython.test.ts +++ b/tests/boundary/providers/desktopZipPython.test.ts @@ -1,16 +1,23 @@ import { readFile } from "node:fs/promises"; import { join } from "node:path"; import { crc32 } from "node:zlib"; +import { Uint8ArrayReader, ZipReader } from "@zip.js/zip.js"; import { describe, expect, it } from "vitest"; import { readZipView } from "../../../src/application/desktop/DesktopZipView.js"; +import { + decodeZipEntryText, + ZIP_READER_OPTIONS, + zipEntryPath, +} from "../../../src/artifacts/ZipArtifactReader.js"; import type { DesktopTaskCheck } from "../../../src/domain/desktop/appTask.js"; import type { JsonValue } from "../../../src/domain/jsonValue.js"; import { detectStateFormat } from "../../../src/domain/desktop/stateFormats.js"; import { emptyEntriesArchive, odtArchive, + unicodePathExtra, withZip64End, zipArchive, type ZipMember, @@ -66,13 +73,7 @@ const sqliteHeader = Buffer.concat([ it("gives the UTF-8 flag precedence over a Unicode Path for names and formats", async () => { const storedName = "legacy.txt"; - const unicodeName = Buffer.from("café.json"); - const extra = Buffer.alloc(9 + unicodeName.length); - extra.writeUInt16LE(0x7075); - extra.writeUInt16LE(extra.length - 4, 2); - extra[4] = 1; - extra.writeUInt32LE(crc32(Buffer.from(storedName)), 5); - unicodeName.copy(extra, 9); + const extra = unicodePathExtra(storedName, "café.json"); const archive = zipArchive([ { name: storedName, data: "42", flags: 0x800, centralExtra: extra }, { name: "other.txt", data: "42", centralExtra: extra }, @@ -86,6 +87,123 @@ it("gives the UTF-8 flag precedence over a Unicode Path for names and formats", }); }); +it.each([ + "file", + "crc", + "version", + "short", + "superseded-utf8", + "flagged", + "empty", + "nul", +])( + "keeps the ZIP decoder's final Unicode Path choice through bounded views (%s)", + async (kind) => { + const name = "settings.json"; + const displayed = + kind === "empty" ? "" : kind === "nul" ? "kept\0.json" : name; + const first = unicodePathExtra( + name, + kind === "superseded-utf8" ? Buffer.from([0xff]) : "hidden/", + ); + let last = unicodePathExtra(name, displayed); + if (kind === "crc") last.writeUInt32LE(0, 5); + if (kind === "version") last[4] = 2; + if (kind === "short") last = Buffer.from([0x75, 0x70, 1, 0, 1]); + const archive = zipArchive([ + { + name, + data: '{"zoom":2}', + flags: kind === "flagged" ? 0x800 : 0, + centralExtra: Buffer.concat([first, last]), + }, + { name: "control.txt", data: "unaffected" }, + ]); + // The complete production decoder is the semantic control for the bounded + // preflight, including invalid final claims falling back to the raw name. + const reader = new ZipReader( + new Uint8ArrayReader(archive), + ZIP_READER_OPTIONS, + ); + try { + expect( + (await reader.getEntries({ decodeText: decodeZipEntryText })).map( + (entry) => ({ + name: zipEntryPath(entry), + directory: entry.directory, + }), + ), + ).toEqual([ + { name: displayed, directory: false }, + { name: "control.txt", directory: false }, + ]); + } finally { + await reader.close(); + } + expect(await sameView(archive)).toMatchObject({ + entries: [ + { + name: displayed, + format: "json", + view: { zoom: 2 }, + view_error: null, + }, + { name: "control.txt", view_error: null }, + ], + entry_count: 2, + truncated: false, + }); + }, +); + +it("refuses invalid UTF-8 in the selected Unicode claim while preserving an unrelated extra field", async () => { + const name = "settings.json"; + const opaque = Buffer.from([0xfe, 0xca, 3, 0, 1, 2, 3]); + const archive = (last: Buffer) => + zipArchive([ + { + name, + data: '{"zoom":2}', + centralExtra: Buffer.concat([ + unicodePathExtra(name, "hidden/"), + opaque, + last, + ]), + }, + ]); + expect(await sameView(archive(unicodePathExtra(name, name)))).toMatchObject({ + entry_count: 1, + entries: [{ name, view: { zoom: 2 } }], + truncated: false, + }); + const invalid = archive(unicodePathExtra(name, Buffer.from([0xff]))); + await expectSameViews([invalid]); + expect(await readZipView(invalid)).toEqual({ + ok: false, + error: "not a readable zip archive", + }); +}); + +it("excludes a final Unicode directory without consuming a retained file slot", async () => { + const name = "settings.json"; + const archive = zipArchive([ + { + name, + data: "not a file view", + centralExtra: Buffer.concat([ + unicodePathExtra(name, name), + unicodePathExtra(name, "hidden/"), + ]), + }, + { name: "control.json", data: '{"kept":true}' }, + ]); + expect(await sameView(archive)).toMatchObject({ + entries: [{ name: "control.json", view: { kept: true } }], + entry_count: 1, + truncated: false, + }); +}); + describe("zip views in the exported module", () => { it("lists entries in archive order with the formats and views REA reads", async () => { const members: ZipMember[] = [