From 6adef63d77645281f64d199e0873dc7a12b1c623 Mon Sep 17 00:00:00 2001 From: StepenkoAnatoli Date: Wed, 14 Oct 2026 15:29:31 +0300 Subject: [PATCH] fix(archive): declare UTF-8 entry names in ZIP headers Fixes #345 what changed: Set language encoding bit 11 in the local and central ZIP headers. Added direct header assertions and a Python extraction regression proving a Unicode document lands under its manifest-declared path. why: Names were UTF-8 bytes with an encoding flag declaring the legacy ZIP character set, so standard extractors produced mojibake names. Declare the existing encoding as APPNOTE requires. No alternative design was set aside. what it touched: archive.mjs, audit and artifact-producer tests, architecture, changelog and research-kit README inventory. The existing reader and manifest format are unchanged. what you verified: verified - red-first 65 passes, two expected failures and two unsupported local symlink tests; then 67 passes, zero failures and the same two unsupported capabilities. Python extraction preserved the exact Unicode path and content. Handoff OK, preflight PASS, doctor READY collector/firecrawl-cli in this checkout. After refresh onto merged main 82a02cb, focused audit/artifact-producer checks passed 69 tests with zero failures and two unsupported symlink cases in 37.3 seconds. The documented local allowance produced exit 0 while retaining those unsupported results; CI blocks unsupported cases. Advanced the patch to a6d6328b, the locally accepted #364 fix in PR415 (not yet merged), to verify both remaining fixes together. All three staged code/test blobs match the focused-tested patch; the shared docs preserve that dependency and the 1,845-test inventory. The normal full commit gate runs before acceptance; final publication uses current main and GitHub CI before merge. what you got wrong and fixed: nothing to report. --- CHANGELOG.md | 2 ++ docs/ARCHITECTURE.md | 2 +- research-kit/README.md | 2 +- research-kit/lib/archive.mjs | 4 ++-- research-kit/test/artifact-producer.test.mjs | 19 ++++++++++++++++++- research-kit/test/audit.test.mjs | 12 +++++++++++- 6 files changed, 35 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0674799f..00e0a471 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ Each release of Research-Kit, newest first. The version is `KIT_VERSION` in ## Unreleased +- ZIP archives declare UTF-8 names in both headers, so ordinary extractors preserve non-ASCII paths declared by the package manifest (#345). + - Machine config saves preserve unpatched raw role and posture values, including typos that diagnostics must still report. An unreadable config without a usable last-good snapshot refuses an unrelated save rather than persisting the restrictive fallback; diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8cd149db..a35be350 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -176,7 +176,7 @@ is an open decision (2026-09-30): | `property-vector-conformance.mjs` | Exports the property suite's canonical-hash and graph findings as a static vector packet, and re-verifies them — in Node and in Python, against the same file. Read-only in all three files. Its value is that it turns property tests, which generate their own inputs and could drift with the generator, into **fixed vectors a second language can check**: canonical hashing invariant under object insertion order and sensitive to authored array order, predecessor closure across generated chains, and descendant invalidation that is transitive, sorted, duplicate-free and permutation-invariant. Two of its five tests guard the fixture rather than the code — that the exported vectors stay synthetic, and that the exported graph and hash values stay bound to the property seeds that produced them. | ADR-0029 | | `r29-workbook-linkage-validator.mjs` | Cross-reference validator for the R29 reviewer-workbook linkage register: 24 task ids, six expected reference kinds (task/gold/source/calibration/warm/lock), and a status reduction that takes the worst of `PASS < INCOMPLETE < FAIL < REOPEN`. **Zero imports** — it is 58 lines of pure functions over `{ register, catalog, pointer }` passed in as arguments, so it touches no filesystem, no network and no clock, and the caller owns every input. That is also why it needed no sealed-record fixture: its tests synthesise a register into a temp directory rather than shipping one. A revoked lock pointer reduces to `REOPEN` rather than `FAIL`, which is the distinction the rank order exists for — a withdrawn approval is not the same as a broken link. | ADR-0029 | | `config.mjs` | A **compatibility shim**, nine lines, re-exporting `machine.mjs`'s config surface so an older import path keeps working. It adds nothing and decides nothing. Listed because it was the one module in `lib/` this map did not mention — found by counting files against rows during the 2026-09-20 reconciliation, which is the only way that kind of omission surfaces. | ADR-0002 | -| `archive.mjs` | **One container format**: `buildZip(entries)` → bytes, `writeZip(file, entries)` → `{ file, bytes, entries }` (written whole or not at all, through `writeBytes`, since 2026-09-28), `crc32`, `entryName`. ZIP with deflate (store when deflating would only grow the payload), written because the kit has no dependencies and cannot shell out to a `zip` binary that may not exist. Knows the container and nothing else — what goes inside a bundle is `lib/audit.mjs`'s. Refuses entry names that are not plain relative paths (an archive must not unpack outside its folder) and an archive with no entries. See ADR-0019. **Changes no name it is handed** (2026-09-28): `entryName` refuses a backslash - it translated one into a separator, which stored `a\b.md` as `a/b.md` while the package manifest still declared the original. | ADR-0019 | +| `archive.mjs` | **One container format**: `buildZip(entries)` → bytes, `writeZip(file, entries)` → `{ file, bytes, entries }` (written whole or not at all, through `writeBytes`, since 2026-09-28), `crc32`, `entryName`. ZIP with deflate (store when deflating would only grow the payload), with UTF-8 name encoding declared by bit 11 in both local and central headers, preserving Unicode manifest paths when ordinary ZIP tools extract them (#345); written because the kit has no dependencies and cannot shell out to a `zip` binary that may not exist. Knows the container and nothing else — what goes inside a bundle is `lib/audit.mjs`'s. Refuses entry names that are not plain relative paths (an archive must not unpack outside its folder) and an archive with no entries. See ADR-0019. **Changes no name it is handed** (2026-09-28): `entryName` refuses a backslash - it translated one into a separator, which stored `a\b.md` as `a/b.md` while the package manifest still declared the original. | ADR-0019 | | `artifact-zip.mjs` | **Reading a ZIP somebody else wrote** — the opposite job from `archive.mjs`, and a separate file on purpose: a writer that also parses hostile input grows a trust boundary in the middle of itself. `openZip(bytes)` → `{ entries, names, problems, has, read }`, plus `checkEntryName`, `readCentralDirectory`, `inspectEntries`, `readEntry` and the exported `ZIP_LIMITS` (entry count, total and per-entry uncompressed bytes, ratio + floor, name length) so tests drive the refusal paths with small fixtures instead of real bombs. **The order of operations is the security property**: every structural refusal — traversal, absolute, drive letter, backslash, NUL, duplicate, case collision, file/directory-prefix collision, symlink/special entry, encryption, unknown method, counts, sizes, ratio — is decided from the **central directory before one byte is inflated**. The directory is attacker-controlled too, so it is never the only defence: `inflateRawSync` runs with `maxOutputLength`, making the ceiling the runtime's to enforce rather than a number compared against a field the attacker wrote. Names are judged as raw text, never `path.normalize`'d first — normalising would let `a/../../b` become `../b` and then be judged, when the honest answer is that `..` has no business in a research artifact. Touches no filesystem: entries come back as buffers. ZIP64 is refused rather than half-read. | ADR-0032 | | `artifact-validator.mjs` | Ledger verification follows actual archive capture entries and parsed evidence rows, so relabelling roles or declaring zero captures cannot disable provenance checking; an empty failed-collection scaffold still passes (#358). **Is this package valid, and does it authorize a build** — two questions, kept apart. `validateArtifact({ file, expectedClientRef })` → `{ status, buildAuthorized, packageId, clientRef, workflowRunId, state, errors, warnings }`, where `status: 'PASS'` means internally consistent and says *nothing* about permission. Offline and read-only by construction: no transport imported, `process.env` never read, nothing from the package executed, no shell, and the only write is a `fs.mkdtemp` copy of `project/` removed in a `finally` — needed because the chain and handoff checks are the kit's existing code (`readCorpus`, `verifyHandoff`) and reuse beats a second implementation of provenance verification nobody tested. Stable error codes (`ZIP-*`, `MANIFEST-*`, `FILE-*`, `LEDGER-*`, `CAPTURE-*`, `AUTHORIZATION-INCONSISTENT`, `SECRET-DETECTED`, `FORMAT-UNSUPPORTED`, `CLIENT-REF-MISMATCH`) and **never a secret value in an error** — the report names the pattern and the line. `authorizationProblems()` is the one implementation of the build rule; the schema encodes it a second time and when the schema catches it first the rule is still reported in its own words. An unsupported format **major** is `INCOMPLETE`, not invalid. A crash is `BLOCKED`, never `PASS`. Its report carries `reviewedBy` (`review.by`, or `undeclared` for a 1.0.0 package) and judges nothing by it (ADR-0074). Report metadata is surfaced only after the manifest passes schema validation; malformed state and identity fields are null, and the CLI escapes control characters in every text diagnostic so hostile strings cannot forge lines. Reads format majors **1 and 2** (2026-09-30, ADR-0107): a 1.x `HUMAN_REVIEW_REQUIRED` is reported as `REVIEW_REQUIRED`, and `eraProblems` refuses a value from the other major by name - a 2.x package with the old state or `review.by: "human"`, or a 1.x one with the new state. A binary body (NUL in its first 8000 bytes) is not matched against the credential patterns, as a body over 512 KiB is not (ADR-0128, 2026-10-02): `docs/` travels whole, and one vendored DLL carrying the PEM marker it parses would have failed a package with no secret in it. Missing, ill-typed, malformed and below-oldest format versions are MANIFEST-SCHEMA/FAIL with regeneration as the remedy; only a numeric format major newer than this reader is FORMAT-UNSUPPORTED/INCOMPLETE (#361). Claimed approvals are re-derived from checked bytes with the portable unpromoted pluralist, 180-day baseline; warnings name that baseline and format-1 legacy compatibility. | ADR-0032, ADR-0160 | | `artifact.mjs` | Raw-cell review reads and explicit ledger reads use projectFile under ADR-0109; Raw reads stay inside the capture folder, and a non-file ledger is refused before reading (#351). **Packaging a project without changing it; shared authorization derivation lives in `artifact-state.mjs`.** `createArtifact` → `{ bytes, name, manifest, entries, derived }`; `writeArtifact` writes then **re-validates with the consumer's own validator** before reporting success; its bytes go through `writeBytes`, so re-creating a package at a name that already holds one cannot trade a good package for a truncated one (2026-09-28). There is no `--build-authorized` and no option that reaches an authorization field: `deriveState(root)` runs the real gate with `env` defaulting to `{}` so a packaging shell's override cannot become a property of a package that outlives it. `mapClassified` and `briefReviewed` have honest machine answers; `findingsReviewState` answers the third **negatively** — a `Finding` byte-identical to what `firstFinding` re-extracts from the capture was demonstrably never rewritten, which detects the unreviewed case without grading the reviewed one (ADR-0013's line, uncrossed). It repeats the collector's own call exactly - the page body after the front-matter, falling back to the page's `title` or `url` from that front-matter - since 2026-09-27: run over the whole file, the extractor read the front-matter as the page, and a URL-only corpus nobody had touched (one JSON capture, run 36287211468) was packaged `findingsReviewed: true`. `packageName` never carries the topic (an artifact listing is world-readable on a public repository) and falls back to `run-`; `checkClientRef` refuses rather than sanitises. Inventory sorted by ZIP path, manifest and digest appended **last**, digest over the exact manifest bytes. README-FIRST in a package that is not yet approved prints the preflight and brief commands and **one `create` line carrying the package's own identity** (`repackageArgs`, 2026-09-27): the manifest is written once, and the package gave its reviewer no way to get one that says `APPROVED_BRIEF`. Commands are spelled by `homeCommand` (ADR-0050). `createArtifact` checks the identity against the manifest schema's own `source` rules before deriving or writing anything (`sourceProblem`, 2026-09-27): `create --commit abc` wrote a package, then reported it did not validate, and left it on disk. **`review.by`** (format 1.1.0, ADR-0074) is the reviewer's declaration read from `BRIEF.md` by `reviewedBy`: `agent` or `undeclared` (`human` was retired in format 2.0.0, ADR-0107). Not an approval condition. README-FIRST no longer says "a human reviewed it"; it names the declared reviewer, or says none was declared. `collectProjectFiles` and the explicit ledger add refuse (`OUTSIDE_PROJECT`) any path whose real location is outside the project: it followed links and packed outside files, a private key included (ADR-0076). The same walk asks the consumer's own `checkEntryName` about every name it is about to store and refuses the run before a byte is written (`UNPACKAGEABLE_NAME`, 2026-09-28): `entryName` translates a backslash into a path separator, so a file named `a\b.md` travelled as `a/b.md` - a directory invented out of a name - while the manifest kept the original, and only the read-back caught the contradiction, after the package had been written. The same refusal covers two files differing only in case (2026-09-28), with the reader's lowercase comparison: the reader's ZIP-CASE-COLLISION fired only after the write. Format **2.0.0** (2026-09-30, ADR-0107): a collected package waiting for review says `REVIEW_REQUIRED`, and README-FIRST gives the review to the agent alone. collectProjectFiles refuses (`UNPACKAGEABLE_FILE`) what packaging cannot read to an end - a FIFO, socket or device, and a folder link back into its own ancestry (judged by real path, so a link to a sibling folder inside the project still packages) - before a byte is read (2026-10-01). **A reviewed brief is complete and current** (2026-10-03, output-reliability audit G1): `deriveState` approves only when every judged section is present and answered (`judgedSection`) and a stamped brief's inputs hash matches the corpus (`draftStamp`, `briefInputsHash`); `briefState` alone read a missing section as answered, so a drafted brief with no review sections derived APPROVED_BRIEF on the repository's own corpus, and a brief whose evidence changed after the draft stayed authorized beside hygiene's brief-stale warning. A brief with no stamp predates ADR-0055 - the root's own and ten decision briefs - and keeps its approval: its currency cannot be checked, `hygiene/brief-unstamped` says so (ADR-0138), a redraft with --force stamps it. **README-FIRST in every state and the collection summary preserve the producer evaluation warnings** through `renderGateWarnings` (ADR-0143). Authorization fields and machine package formats are unchanged. `bin/artifact.mjs` refuses a value flag given no value by name, exit 3, before anything is read or written (2026-10-11, #280): `create` had asked only whether a required flag was present, so a bare `--ref` went into the manifest as "true", a bare `--run-id` as 1 and a bare `--output` wrote a package named `true` that re-validated PASS; a bare `validate --file` tried to open a file called `true`. Caller references use checkClientRef before token lookup, output-folder creation or dispatch (#375, 2026-10-13); CR/LF are refused by the shared checker, and diagnostics are redacted. The artifact CLI awaits exitAfterFlush for its report, help and refusal endings, preserving queued stdout/stderr and the existing exit status on asynchronous POSIX pipes (#359). | ADR-0032, ADR-0031, ADR-0052, ADR-0143 | diff --git a/research-kit/README.md b/research-kit/README.md index b85ab518..c06356d9 100644 --- a/research-kit/README.md +++ b/research-kit/README.md @@ -568,7 +568,7 @@ node research-kit/bin/selftest.mjs # all of it node research-kit/bin/selftest.mjs gate hook # just these files ``` -1843 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means +1845 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means the assertions settled (ADR-0021), and each test is raced against a watchdog (`RESEARCH_KIT_TEST_TIMEOUT`, default 60s). diff --git a/research-kit/lib/archive.mjs b/research-kit/lib/archive.mjs index 4244d1c7..d418453d 100644 --- a/research-kit/lib/archive.mjs +++ b/research-kit/lib/archive.mjs @@ -73,7 +73,7 @@ export function buildZip(entries, { date = new Date(1980, 0, 1, 0, 0, 0) } = {}) const local = Buffer.alloc(30); local.writeUInt32LE(0x04034b50, 0); local.writeUInt16LE(20, 4); - local.writeUInt16LE(0, 6); + local.writeUInt16LE(0x0800, 6); // Names are UTF-8 (APPNOTE 4.4.4, bit 11). local.writeUInt16LE(method, 8); local.writeUInt16LE(time, 10); local.writeUInt16LE(day, 12); @@ -88,7 +88,7 @@ export function buildZip(entries, { date = new Date(1980, 0, 1, 0, 0, 0) } = {}) dir.writeUInt32LE(0x02014b50, 0); dir.writeUInt16LE(20, 4); dir.writeUInt16LE(20, 6); - dir.writeUInt16LE(0, 8); + dir.writeUInt16LE(0x0800, 8); dir.writeUInt16LE(method, 10); dir.writeUInt16LE(time, 12); dir.writeUInt16LE(day, 14); diff --git a/research-kit/test/artifact-producer.test.mjs b/research-kit/test/artifact-producer.test.mjs index 017d5c9d..ac267631 100644 --- a/research-kit/test/artifact-producer.test.mjs +++ b/research-kit/test/artifact-producer.test.mjs @@ -6,7 +6,7 @@ // because the way this gets lost is somebody adding a convenient option later. import { execFileSync } from 'node:child_process'; -import { test, describe, assert, fs, path, os, cleanup, tempDir, requireSymlink } from './harness.mjs'; +import { test, describe, assert, fs, path, os, cleanup, tempDir, requireSymlink, requirePython } from './harness.mjs'; import { sha256, canonicalJson, resolve, readText, writeText, today, PATHS, HEADERS } from '../lib/core.mjs'; import { createArtifact, deriveState, collectProjectFiles, packageName, checkClientRef, @@ -25,6 +25,23 @@ describe('artifact-producer'); const scratch = tempDir('rk-producer-'); +test('Python extracts a Unicode document under the path declared by the manifest', () => { + const root = collectedProject(); + const name = 'docs/über-日本語.md'; + writeText(path.join(root, name), 'Unicode document\n'); + const built = build(root); + const declared = `project/${name}`; + assert.ok(built.manifest.files.some((entry) => entry.path === declared)); + const dir = tempDir('rk-unicode-zip-'); + const zipFile = path.join(dir, 'package.zip'); + const extractDir = path.join(dir, 'extracted'); + fs.writeFileSync(zipFile, built.bytes); + const script = 'import json,sys,zipfile\nwith zipfile.ZipFile(sys.argv[1]) as z:\n print(json.dumps(z.namelist()))\n z.extractall(sys.argv[2])\n'; + const names = JSON.parse(execFileSync(requirePython('Unicode ZIP extraction'), ['-I', '-c', script, zipFile, extractDir], { encoding: 'utf8' })); + assert.ok(names.includes(declared), JSON.stringify(names)); + assert.equal(fs.readFileSync(path.join(extractDir, declared), 'utf8'), 'Unicode document\n'); +}); + function build(root, overrides = {}) { return createArtifact({ root, ...IDENTITY, ...overrides }); } diff --git a/research-kit/test/audit.test.mjs b/research-kit/test/audit.test.mjs index 070516f9..4b73bb73 100644 --- a/research-kit/test/audit.test.mjs +++ b/research-kit/test/audit.test.mjs @@ -129,7 +129,7 @@ function readZip(bytes) { assert.equal(data.length, uncompressed, `${name}: size disagrees with the directory`); assert.equal(crc32(data), sum, `${name}: CRC disagrees with the directory`); - out.push({ name, text: data.toString('utf8'), method }); + out.push({ name, text: data.toString('utf8'), method, flags: bytes.readUInt16LE(at + 8), localFlags: bytes.readUInt16LE(offset + 6) }); at += 46 + nameLen + bytes.readUInt16LE(at + 30) + bytes.readUInt16LE(at + 32); } return out; @@ -153,6 +153,16 @@ test('two runs produce byte-identical archives, so re-zipping is free', () => { assert.ok(buildZip(entries).equals(buildZip(entries))); }); +test('UTF-8 names declare their encoding in both ZIP headers', () => { + const names = ['ascii.md', 'docs/über-日本語.md']; + const entries = readZip(buildZip(names.map((name) => ({ name, data: 'contents' })))); + assert.deepEqual(entries.map((entry) => entry.name), names); + for (const entry of entries) { + assert.equal(entry.flags & 0x0800, 0x0800, `${entry.name}: central UTF-8 flag`); + assert.equal(entry.localFlags & 0x0800, 0x0800, `${entry.name}: local UTF-8 flag`); + } +}); + // --- the audit --------------------------------------------------------------------- test('an audit renders only when the gate passes', () => {