From 40f7f7f8e7bc094416127632e65c2a2163f10c4b Mon Sep 17 00:00:00 2001 From: StepenkoAnatoli Date: Wed, 14 Oct 2026 01:29:39 +0300 Subject: [PATCH] fix(artifact): refuse file and directory path collisions what changed: ZIP inspection now detects a regular file used as another entry's parent directory, independent of entry order, before any inflation or materialization. Explicit file/directory name collisions also receive ZIP-PATH-COLLISION. Added two validator fixtures with stable FAIL results and diagnostics that contain no private temp path or raw filesystem error. why: A readable, self-contradictory archive should be FAIL; it previously threw EEXIST during materialization and became BLOCKED with a temporary path and a misleading kit-bug remedy (#362). This applies the existing structural ZIP refusal boundary. No alternative design was set aside. what it touched: artifact-zip inspection, artifact-validator fixtures, the artifact-zip architecture row, changelog and README test inventory. No schema, public option or exit-code mapping changed. what you verified: verified - red-first 45 passed and two expected collision failures; then 47 artifact-validator tests passed. The final strengthened diagnostics assertion passed in 35.2 seconds. Architecture-map 21 passed; handoff OK (33 ledger entries, 27 evidence rows), preflight PASS (28 passing, one warning, no blockers), doctor READY collector/firecrawl-cli. The issue's artifact-zip test group has no file in this tree; its ZIP regressions are in artifact-validator. The required full offline suite runs in the commit gate before acceptance. what you got wrong and fixed: mistake - the initial temp-path assertion searched for research-kit-artifact- instead of the actual rk-artifact- prefix. where - artifact-validator.test.mjs collision diagnostic assertion. impact - the assertion did not independently detect the real private extraction path; the FAIL/status and ZIP-PATH-COLLISION checks still caught the baseline defect, and no delivered PR was affected. cause - the prefix was written without checking materializeProject. fix - check the actual scratch directory and rk-artifact- prefix, plus EEXIST/ENOTDIR, in all diagnostic messages and remedies. verified - all 47 artifact-validator tests passed in 35.2 seconds. --- CHANGELOG.md | 1 + docs/ARCHITECTURE.md | 2 +- research-kit/README.md | 2 +- research-kit/lib/artifact-zip.mjs | 34 ++++++++++++++++++- research-kit/test/artifact-validator.test.mjs | 23 +++++++++++++ 5 files changed, 59 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7affd40..885763fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ Each release of Research-Kit, newest first. The version is `KIT_VERSION` in `research-kit/lib/core.mjs`; a release is tagged `v` (ADR-0116). ## Unreleased +- `artifact validate` rejects ZIP entries where a file is also an ancestor directory of another entry, before extraction, with a stable `ZIP-PATH-COLLISION` failure (#362). - Fixed `AGENTS.md` corpus description to use evergreen relative references rather than stale hardcoded entry counts (#299). - The suite's "every CLI refuses a flag it does not know" table now covers `measure`, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index ca0813fa..b1f8ff3f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -177,7 +177,7 @@ is an open decision (2026-09-30): | `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 | -| `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, 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-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` | **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). 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. | ADR-0032 | | `artifact.mjs` | **Packaging a project without changing it, and deriving every authorization field.** `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`. | ADR-0032, ADR-0031, ADR-0052, ADR-0143 | | `mcp.mjs` | **The collector as a Model Context Protocol server**, and the first module in this repository built from an APPROVED BRIEF rather than from a plan - every design choice cites an E-row in `docs/decisions/2026-09-21-agent-interface/`. `handle(message, deps)` is a pure function of a JSON-RPC message plus injected transport, so the whole surface is tested offline. **stdio, not Streamable HTTP**: semantics are identical on both, so the difference is operational, and remote costs the entire OAuth 2.1 chain. **Two tools, not one** - `collect` returns a run id immediately, `fetch_corpus` returns a link - because a server cannot initiate a request, so the alternative is one call held open for minutes and an agent that loses a paid run when its process dies. **A `resource_link`, never embedded bytes**: the spec defines embedding but states no maximum size, so a link is the defensible option. **No tool takes a token** and a test asserts no parameter is credential-shaped. `PROTOCOL_VERSION` is pinned and a mismatch answers `-32022` with `data.supported`, the same discipline as pinning `X-GitHub-Api-Version`. `createStdioLoop` serialises chunk processing through one promise chain - overlapping `data` events were mutating the buffer concurrently, which a test caught. **DUAL-ERA**: `SUPPORTED_VERSIONS` is `['2026-07-28', '2025-11-25', '2025-06-18']` (2025-06-18 since 2026-09-27, ADR-0049: a client on the official SDK up to 1.24.0 asks for it and refuses 2025-11-25, and nothing 2025-11-25 changed touches this server; 2025-03-26 is not served, because the results carry `structuredContent` and `resource_link`) and `initialize` is implemented beside `server/discover`, because the specification is ahead of every shipped client - the official SDK at v1.30.0 declares `2025-11-25` as its latest and answered the modern-only first version with `-32601: unknown method initialize`. `initialize` **echoes** a serveable requested version and falls back to **legacy** rather than to this server's preference, because a client throws on a version it does not support. **Arguments that fail a tool's schema are a tool result** (`isError: true`, the reason as text) since 2026-09-27 - SEP-1303 in 2025-11-25 says input validation errors should be Tool Execution Errors, so the model can correct itself; they were JSON-RPC -32602, which a client raises as an exception. An unknown tool stays -32602. `collect`'s `topic` carries `minLength: 1`, and `validateArgs` enforces `minLength` after trimming (2026-09-27): an empty topic had passed and reached GitHub, which answered HTTP 422. `fetch_corpus` checks `out_dir` with `usableOutDir` before it reads the run (2026-09-27), so an unusable folder is an `OUT_DIR` tool error, not `UNKNOWN`. `fetch_corpus` returns `reviewedBy`, and its text says review is for an agent or a person (ADR-0074). `createStdioLoop` bounds a line at `MAX_LINE` (4 MiB, 2026-09-28): a line past it is refused with -32600 as soon as it passes the cap and dropped through its newline, and the next message is served - the buffer had grown without limit until a newline came, so a client that sent none could grow the server until it died. **No request is lost at end of input** (break-test PR #145, 2026-09-29): `exitWhenSettled` waits for every call already read before exiting (bounded by `END_GRACE_MS`), and a final request without a trailing newline is served on `end` - both had been dropped with exit 0. `SERVER_INFO.version` is `KIT_VERSION` (ADR-0116); it had announced a fixed 1.0.0. **The approved fetch response directs the builder to README-FIRST and the brief for warnings and known unknowns** (ADR-0143). It forwards no new machine warning field and derives no new authorization. `validateArgs` checks allowed keys by own membership and enforces the declared arrays of strings: malformed query/URL values and inherited Object property names become schema tool errors before credential lookup or dispatch. Omitted or empty arrays, and valid string arrays, keep their existing normalization. | ADR-0034, ADR-0049, ADR-0143 | diff --git a/research-kit/README.md b/research-kit/README.md index 4bc12369..5ac159b0 100644 --- a/research-kit/README.md +++ b/research-kit/README.md @@ -563,7 +563,7 @@ node research-kit/bin/selftest.mjs # all of it node research-kit/bin/selftest.mjs gate hook # just these files ``` -1789 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means +1791 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/artifact-zip.mjs b/research-kit/lib/artifact-zip.mjs index 01d2a0ff..652ff60a 100644 --- a/research-kit/lib/artifact-zip.mjs +++ b/research-kit/lib/artifact-zip.mjs @@ -212,6 +212,8 @@ export function inspectEntries(entries, { limits = ZIP_LIMITS } = {}) { const problems = []; const seen = new Map(); // exact name -> count const folded = new Map(); // lowercased name -> first exact name that produced it + const files = new Set(); + const directories = new Set(); let totalBytes = 0; for (const entry of entries) { @@ -242,7 +244,12 @@ export function inspectEntries(entries, { limits = ZIP_LIMITS } = {}) { continue; } - if (entry.isDirectory) continue; // carries no payload and declares no size worth counting + if (entry.isDirectory) { + directories.add(entry.name.replace(/\/$/, '')); + continue; // carries no payload and declares no size worth counting + } + + files.add(entry.name); const count = (seen.get(entry.name) ?? 0) + 1; seen.set(entry.name, count); @@ -281,6 +288,31 @@ export function inspectEntries(entries, { limits = ZIP_LIMITS } = {}) { } } + // An archive may not use one path as both a regular file and the directory + // containing another entry. Otherwise materialization order decides whether mkdir + // or writeFileSync throws, turning a readable contradictory package into BLOCKED. + for (const file of files) { + if (directories.has(file)) { + problems.push({ + code: 'ZIP-PATH-COLLISION', + detail: `file "${file}" is also declared as a directory`, + path: file, + }); + } + let slash = file.indexOf('/'); + while (slash !== -1) { + const parent = file.slice(0, slash); + if (files.has(parent)) { + problems.push({ + code: 'ZIP-PATH-COLLISION', + detail: `file "${parent}" is also a directory prefix of "${file}"`, + path: file, + }); + } + slash = file.indexOf('/', slash + 1); + } + } + if (totalBytes > limits.maxTotalBytes) { problems.push({ code: 'ZIP-SIZE-LIMIT', detail: `the package declares ${totalBytes} uncompressed bytes, over the ${limits.maxTotalBytes} limit` }); } diff --git a/research-kit/test/artifact-validator.test.mjs b/research-kit/test/artifact-validator.test.mjs index 077fdc67..9ceca6e1 100644 --- a/research-kit/test/artifact-validator.test.mjs +++ b/research-kit/test/artifact-validator.test.mjs @@ -152,6 +152,24 @@ const FIXTURES = { }, expect: { status: 'FAIL', code: 'ZIP-CASE-COLLISION' }, }, + '30-file-directory-collision-file-first': { + build: () => { + const payload = copyPayload(collected.payload); + payload.push({ name: 'project/docs/a', data: text('file') }); + payload.push({ name: 'project/docs/a/b.md', data: text('child') }); + return seal(payload, clone(collected.manifest), { recomputeFiles: false }); + }, + expect: { status: 'FAIL', code: 'ZIP-PATH-COLLISION', noTempPath: true }, + }, + '31-file-directory-collision-child-first': { + build: () => { + const payload = copyPayload(collected.payload); + payload.push({ name: 'project/docs/a/b.md', data: text('child') }); + payload.push({ name: 'project/docs/a', data: text('file') }); + return seal(payload, clone(collected.manifest), { recomputeFiles: false }); + }, + expect: { status: 'FAIL', code: 'ZIP-PATH-COLLISION', noTempPath: true }, + }, '18-unsupported-major-version': { build: () => { const manifest = clone(collected.manifest); @@ -300,6 +318,11 @@ for (const [name, fixture] of Object.entries(FIXTURES)) { assert.ok(codes.includes(fixture.expect.code), `${name}: expected error ${fixture.expect.code}, got ${codes.join(', ') || '(none)'}`); } + if (fixture.expect.noTempPath) { + const diagnostics = result.errors.map((error) => `${error.message} ${error.remedy ?? ''}`).join('\n'); + assert.ok(!diagnostics.includes(scratch) && !/rk-artifact-|EEXIST|ENOTDIR/.test(diagnostics), + `${name}: structural ZIP error must not expose a materialization temp path`); + } if ('buildAuthorized' in fixture.expect) { assert.equal(result.buildAuthorized, fixture.expect.buildAuthorized, `${name}: buildAuthorized should be ${fixture.expect.buildAuthorized}`);