From 90621fc94996c9b5f3872e915c10d67a53845b65 Mon Sep 17 00:00:00 2001 From: George Pu Date: Thu, 10 Sep 2026 14:49:59 -0400 Subject: [PATCH 1/4] worker: input-artifact resolver (PROPOSAL -- nothing calls it yet) ExecutionSpec.inputArtifacts is `{id, digest}[]`, already digest-bound, and task.mjs:461 records it verbatim under "no fetch in Wave 1B scope". So the contract can NAME an input the worker must consume and nothing can deliver one. This is the missing middle, written so the contract owner has a concrete thing to accept, amend or reject rather than a description. DELIBERATELY NOT WIRED. No call site changes, no schema change, no envelope change. materializeEnvelope and the runVinci spawn path are untouched. WHY IT IS NOT MERE WIRING. `{id, digest}` establishes IDENTITY, not LOCATION. A search of vinci-code-cli, vinci-gpu-control, vinci-contracts and vinci-foundry found no resolver to reuse: the artifacts table says in its own schema comment that it is "a pointer ledger ONLY", GET /v1/jobs/{id}/artifacts returns metadata rather than bytes, the worker fetches nothing by digest anywhere, and foundry verifies digests only AFTER reading an already-local path. The nearest precedent is `vgc artifacts pull`, and this follows it. TWO IDENTITIES, CHECKED SEPARATELY. The ledger says "object X holds these bytes"; the contract says "the expected input is digest Y". They are distinct claims from distinct authorities, so both are asserted -- letting one pass because the other did is how a swap survives. THE WORKER NEVER RESOLVES AN ID ITSELF. The id goes to an authority which returns a pointer. The on-disk name is derived from the DIGEST, never from the id or the uri: a filename is a place, and both of those are strings someone else chose. Materialized 0o400, because a task that can rewrite its own input can rewrite the evidence of what it was given. The returned chain -- requested, resolved, downloaded, materialized -- is exactly what vinci-gpu-control's `input delivery observation` consumes. Eight mutants against a green baseline; six caught immediately, and the two survivors are recorded because they were the interesting ones: * Hashing the in-memory buffer instead of re-reading the file SURVIVED. That is a real gap, not a cosmetic one: it proves the download twice and labels the second one a materialization. Closed by making the read-back injectable purely so the property is testable. * Mutating writeFileSync's mode SURVIVED because the chmod after the rename decides the final mode -- the write mode was defence in depth that defended nothing observable. Removed, leaving one mechanism a test can see. Mutating the chmod is now caught. One bug of my own, same class as the module's subject: the digest-mismatch fixture used a substitute of a different LENGTH, so the truncation check answered first and that test never reached the guard it names. Earlier-guard masking, inside the tests for a module whose whole job is catching substitutions. The substitute is now the same length. Not established: no live lookup, no live download, no call site, and no authority route for a per-id pointer. The existing job-scoped artifacts route returns a list, so a caller must either supply the producing job or the VGC owner must add a per-id lookup -- `lookup` is injected here precisely so that decision stays theirs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7 --- vinci/test/worker-input-artifacts.mjs | 245 ++++++++++++++++++++++++++ vinci/worker/input-artifacts.mjs | 232 ++++++++++++++++++++++++ 2 files changed, 477 insertions(+) create mode 100644 vinci/test/worker-input-artifacts.mjs create mode 100644 vinci/worker/input-artifacts.mjs diff --git a/vinci/test/worker-input-artifacts.mjs b/vinci/test/worker-input-artifacts.mjs new file mode 100644 index 00000000..2a0bb9c5 --- /dev/null +++ b/vinci/test/worker-input-artifacts.mjs @@ -0,0 +1,245 @@ +// The resolver must deliver the bytes the CONTRACT named, or refuse. Every +// refusal below is paired with the legitimate case through the same call, so +// a guard that fires on everything is visible as one. +import assert from "node:assert/strict"; +import { createHash, randomUUID } from "node:crypto"; +import { mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { + DEFAULT_MAX_BYTES, + InputArtifactError, + resolveInputArtifact, + resolveInputArtifacts, + sha256OfBytes, + validateInputArtifact, + validatePointer, +} from "../worker/input-artifacts.mjs"; + +const BYTES = new TextEncoder().encode("accepted findings context packet v1"); +const DIGEST = createHash("sha256").update(BYTES).digest("hex"); +// SAME LENGTH as BYTES, deliberately. The first version of this fixture used +// a shorter string, so the length check fired first and the digest-mismatch +// test never reached the guard it names -- earlier-guard masking inside the +// test for a module whose whole job is catching substitutions. +const OTHER = new TextEncoder().encode("accepted findings context packet v2"); +const OTHER_DIGEST = createHash("sha256").update(OTHER).digest("hex"); +const URI = "s3://vgc-artifacts/ctx/9f2.tgz"; + +const dest = () => mkdtempSync(join(tmpdir(), `input-artifacts-${randomUUID()}-`)); +const artifact = (over = {}) => ({ id: "accepted-findings-context", digest: DIGEST, ...over }); +const pointer = (over = {}) => ({ + artifact_id: "accepted-findings-context", + job_id: "job_producer", + uri: URI, + bytes: BYTES.byteLength, + sha256: DIGEST, + created_at: "2026-09-09T12:00:00Z", + ...over, +}); + +const opts = (over = {}) => ({ + lookup: async () => pointer(), + download: async () => BYTES, + destDir: dest(), + ...over, +}); + +async function refuses(code, run, what) { + try { + await run(); + } catch (error) { + assert.ok(error instanceof InputArtifactError, `${what}: expected an InputArtifactError, got ${error}`); + assert.equal(error.code, code, `${what}: expected code ${code}, got ${error.code}`); + return error; + } + assert.fail(`${what}: expected a refusal (${code}) and none came`); +} + +// --- the good path, first -------------------------------------------------- + +{ + const chain = await resolveInputArtifact(artifact(), opts()); + assert.equal(chain.requested_input_digest, DIGEST); + assert.equal(chain.resolved_storage_object, URI); + assert.equal(chain.downloaded_digest, DIGEST); + assert.equal(chain.materialized_digest, DIGEST); + assert.ok(chain.materialized_path.endsWith(`${DIGEST}.input`)); + assert.equal(readFileSync(chain.materialized_path, "utf8"), "accepted findings context packet v1"); +} + +// The local name is the DIGEST, never the id and never anything from the uri. +{ + const chain = await resolveInputArtifact( + artifact({ id: "totally-different-name" }), + opts({ lookup: async () => pointer({ artifact_id: "totally-different-name" }) }), + ); + assert.ok(chain.materialized_path.endsWith(`${DIGEST}.input`)); + assert.ok(!chain.materialized_path.includes("totally-different-name")); + assert.ok(!chain.materialized_path.includes("9f2")); +} + +// Materialized read-only: a task that can rewrite its own input can rewrite +// the evidence of what it was given. +{ + const chain = await resolveInputArtifact(artifact(), opts()); + assert.equal(statSync(chain.materialized_path).mode & 0o777, 0o400); +} + +// The materialization claim must come from DISK, not from the buffer we +// already hashed. Hashing `bytes` here would prove the download twice and +// call the second one a materialization. +{ + await refuses( + "materialization_mismatch", + () => resolveInputArtifact(artifact(), opts({ readBack: () => OTHER })), + "a file on disk that differs from what was downloaded", + ); + // positive control: the honest read-back resolves. + assert.ok(await resolveInputArtifact(artifact(), opts({ readBack: readFileSync }))); +} + +// --- the contract's own claim ---------------------------------------------- + +assert.deepEqual(validateInputArtifact(artifact(), 0), { id: artifact().id, digest: DIGEST }); +for (const [bad, why] of [ + [{ id: "x", digest: DIGEST, extra: 1 }, "an extra key"], + [{ id: "x" }, "a missing digest"], + [{ id: "../etc/passwd", digest: DIGEST }, "a traversal-shaped id"], + [{ id: "a/b", digest: DIGEST }, "a path-shaped id"], + [{ id: "x", digest: "not-a-digest" }, "a malformed digest"], + [{ id: "x", digest: DIGEST.toUpperCase() }, "an uppercase digest"], +]) { + await refuses("invalid_input_artifact", async () => validateInputArtifact(bad, 0), why); +} + +// --- the two identities, checked separately -------------------------------- + +// This is the case that motivates the whole module: the ledger and the +// contract each name content, and neither settles the other. +{ + const error = await refuses( + "identity_disagreement", + () => resolveInputArtifact(artifact(), opts({ lookup: async () => pointer({ sha256: OTHER_DIGEST }) })), + "ledger and contract naming different content", + ); + assert.match(error.message, /different claims from different authorities/); + // positive control: they agree, and it resolves. + assert.ok(await resolveInputArtifact(artifact(), opts())); +} + +for (const [over, code, why] of [ + [{ artifact_id: "some-other-artifact" }, "pointer_invalid", "a pointer for a different artifact"], + [{ uri: "" }, "pointer_invalid", "a pointer with no uri"], + [{ sha256: "nope" }, "pointer_invalid", "a pointer with no digest"], + [{ bytes: -1 }, "pointer_invalid", "a negative byte count"], + [{ bytes: 1.5 }, "pointer_invalid", "a non-integer byte count"], +]) { + await refuses(code, () => resolveInputArtifact(artifact(), opts({ lookup: async () => pointer(over) })), why); +} +await refuses("pointer_invalid", () => resolveInputArtifact(artifact(), opts({ lookup: async () => null })), "no pointer at all"); + +// --- verify AFTER download, not before ------------------------------------- + +{ + const error = await refuses( + "digest_mismatch", + () => resolveInputArtifact( + artifact(), + // The authority answers correctly and the STORE serves other bytes. + // Nothing before this point can catch that. + opts({ download: async () => OTHER }), + ), + "the store serving different bytes than the ledger promised", + ); + assert.match(error.message, /substitution, not a delivery/); +} + +{ + await refuses( + "download_truncated", + () => resolveInputArtifact(artifact(), opts({ download: async () => BYTES.slice(0, 5) })), + "a short read", + ); +} + +await refuses( + "download_invalid", + () => resolveInputArtifact(artifact(), opts({ download: async () => "a string" })), + "a download that is not bytes", +); + +// --- bounded --------------------------------------------------------------- + +await refuses( + "artifact_too_large", + () => resolveInputArtifact(artifact(), opts({ lookup: async () => pointer({ bytes: DEFAULT_MAX_BYTES + 1 }) })), + "an artifact over the ceiling", +); +{ + // positive control: exactly at the ceiling is allowed, and the ceiling is + // a refusal rather than a slow fetch. + const big = new Uint8Array(64); + const bigDigest = sha256OfBytes(big); + assert.ok(await resolveInputArtifact( + artifact({ digest: bigDigest }), + opts({ + lookup: async () => pointer({ sha256: bigDigest, bytes: 64 }), + download: async () => big, + maxBytes: 64, + }), + )); +} + +// The ceiling is enforced BEFORE the fetch, so an oversized object is never +// pulled down to discover it was oversized. +{ + let downloaded = false; + await refuses( + "artifact_too_large", + () => resolveInputArtifact(artifact(), opts({ + lookup: async () => pointer({ bytes: DEFAULT_MAX_BYTES + 1 }), + download: async () => { downloaded = true; return BYTES; }, + })), + "an oversized artifact", + ); + assert.equal(downloaded, false, "the ceiling must refuse before the fetch, not after"); +} + +// --- the list -------------------------------------------------------------- + +{ + const chains = await resolveInputArtifacts([artifact()], opts()); + assert.equal(chains.length, 1); + assert.equal(chains[0].materialized_digest, DIGEST); +} +assert.deepEqual(await resolveInputArtifacts([], opts()), [], "no declared inputs is a valid state"); +assert.deepEqual(await resolveInputArtifacts(undefined, opts()), [], "an absent list is a valid state"); + +await refuses( + "duplicate_input_artifact", + () => resolveInputArtifacts([artifact(), artifact()], opts()), + "the same artifact declared twice", +); + +// All-or-nothing: one bad entry refuses the task rather than delivering the +// rest. A worker running with three of its four named inputs produces a +// result nobody can interpret. +{ + const good = artifact(); + const bad = artifact({ id: "second-input", digest: OTHER_DIGEST }); + await refuses( + "identity_disagreement", + () => resolveInputArtifacts([good, bad], opts({ + lookup: async (id) => (id === good.id ? pointer() : pointer({ artifact_id: "second-input", sha256: DIGEST })), + })), + "one of two inputs disagreeing", + ); +} + +// --- the pointer validator in isolation ------------------------------------ + +assert.deepEqual(validatePointer(pointer(), artifact()), { uri: URI, bytes: BYTES.byteLength, sha256: DIGEST }); + +console.log("worker-input-artifacts: all controls passed"); diff --git a/vinci/worker/input-artifacts.mjs b/vinci/worker/input-artifacts.mjs new file mode 100644 index 00000000..3c1ec428 --- /dev/null +++ b/vinci/worker/input-artifacts.mjs @@ -0,0 +1,232 @@ +// PROPOSAL, not a landed capability. Nothing calls this yet; wiring it into +// materializeEnvelope and the spawn path is the contract owner's decision. +// +// `ExecutionSpec.inputArtifacts` is `{id, digest}[]`. It is digest-bound (the +// whole record feeds executionSpecDigest, and canonicalize walks every key), +// and task.mjs records it verbatim with the comment "no fetch in Wave 1B +// scope". So the contract can already NAME an input the worker must consume, +// and nothing can deliver one. +// +// THE THING THIS MODULE EXISTS FOR: {id, digest} establishes IDENTITY, not +// LOCATION. Between "the contract named this input" and "the worker consumed +// it" sit four steps, each of which can substitute something else: +// +// requested the digest ExecutionSpec named +// resolved WHICH storage object an AUTHORITY says carries it +// downloaded the digest of the bytes that actually arrived +// materialized the digest of what was written into the workspace +// +// A system whose purpose is improving its own context is exactly where a +// self-certification loop hides, so every link is recorded and every link +// must agree. +// +// TWO IDENTITIES, CHECKED SEPARATELY. The artifact ledger says "these bytes +// are object X" (its own recorded sha256). The execution contract says "the +// input this worker expects has digest Y". Ideally X === Y, but they are +// distinct claims from distinct sources, and letting one pass because the +// other did is how a swap survives. Both are asserted explicitly. +// +// THE WORKER NEVER RESOLVES AN ID ITSELF. `id` is an identifier by +// vinci-contracts' grammar -- it cannot contain "/" -- but that only means it +// is not traversal-shaped. It carries no location semantics whatsoever, and +// no registry backs it. So the id goes to an AUTHORITY, which returns a +// pointer; the worker never treats the id as a path, a URL, or a filename. +// The on-disk name is derived from the DIGEST, not from anything the spec +// author chose. +// +// Modelled on the one existing precedent in the fleet, `vgc artifacts pull`: +// resolve id -> looked-up uri -> fetch -> verify sha256 AFTER download. + +import { createHash } from "node:crypto"; +import { chmodSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; + +import { isIdentifier } from "./contracts/digest.mjs"; + +const HEX64 = /^[0-9a-f]{64}$/; + +// A context packet is prose and JSON. This ceiling is not a policy about +// artifacts in general -- it is the bound on what this consumer will pull +// into a prompt, and a larger input is a refusal rather than a slow fetch. +export const DEFAULT_MAX_BYTES = 8 * 1024 * 1024; + +export class InputArtifactError extends Error { + constructor(code, message) { + super(`${code}: ${message}`); + this.code = code; + } +} + +const refuse = (code, message) => { + throw new InputArtifactError(code, message); +}; + +export function sha256OfBytes(bytes) { + return createHash("sha256").update(bytes).digest("hex"); +} + +// --------------------------------------------------------------------------- +// 1. The contract's own claim. +// --------------------------------------------------------------------------- + +export function validateInputArtifact(entry, index) { + const at = `inputArtifacts[${index}]`; + if (entry === null || typeof entry !== "object" || Array.isArray(entry)) { + refuse("invalid_input_artifact", `${at} is an object`); + } + const keys = Object.keys(entry).sort(); + if (keys.length !== 2 || keys[0] !== "digest" || keys[1] !== "id") { + refuse("invalid_input_artifact", `${at} carries exactly {id, digest}, got {${keys.join(", ")}}`); + } + if (!isIdentifier(entry.id)) { + refuse("invalid_input_artifact", `${at}/id is not an identifier`); + } + if (typeof entry.digest !== "string" || !HEX64.test(entry.digest)) { + refuse("invalid_input_artifact", `${at}/digest is a lowercase sha256 hex digest`); + } + return { id: entry.id, digest: entry.digest }; +} + +// --------------------------------------------------------------------------- +// 2. The authority's answer. The worker asks; it does not decide. +// --------------------------------------------------------------------------- + +// The pointer shape vinci-gpu-control's artifacts ledger already emits: +// {artifact_id, job_id, uri, bytes, sha256, created_at}. Only the four fields +// this consumer needs are read, and the rest are ignored rather than trusted. +export function validatePointer(pointer, artifact) { + if (pointer === null || typeof pointer !== "object") { + refuse("pointer_invalid", `no pointer record for artifact ${artifact.id}`); + } + if (pointer.artifact_id !== artifact.id) { + refuse("pointer_invalid", `pointer names artifact ${JSON.stringify(pointer.artifact_id)}, asked for ${JSON.stringify(artifact.id)}`); + } + if (typeof pointer.uri !== "string" || !pointer.uri.trim()) { + refuse("pointer_invalid", `pointer for ${artifact.id} carries no uri`); + } + if (typeof pointer.sha256 !== "string" || !HEX64.test(pointer.sha256)) { + refuse("pointer_invalid", `pointer for ${artifact.id} carries no sha256`); + } + if (!Number.isInteger(pointer.bytes) || pointer.bytes < 0) { + refuse("pointer_invalid", `pointer for ${artifact.id} carries no byte count`); + } + // IDENTITY 1 vs IDENTITY 2. The ledger says the object holds these bytes; + // the contract says the input has that digest. Disagreement means the two + // authorities name different content, and downloading either one would be + // a guess about which is right. + if (pointer.sha256 !== artifact.digest) { + refuse( + "identity_disagreement", + `the artifact ledger says ${artifact.id} is ${pointer.sha256.slice(0, 12)}… but the execution contract expects ${artifact.digest.slice(0, 12)}…; these are different claims from different authorities and neither one settles the other`, + ); + } + return { uri: pointer.uri, bytes: pointer.bytes, sha256: pointer.sha256 }; +} + +// --------------------------------------------------------------------------- +// 3. Resolve, fetch, verify, materialize. +// --------------------------------------------------------------------------- + +/** + * Resolve one declared input artifact into verified local bytes. + * + * `lookup(id) -> pointer` and `download(uri, {maxBytes}) -> Uint8Array` are + * injected. Both defaults live at the call site rather than here, because + * the authority endpoint and the object-store client are deployment + * concerns and this module must stay testable without either. + * + * Returns the delivery chain, which is the evidence vinci-gpu-control's + * `input delivery observation` consumes. Every stage is reported, including + * the ones that did not happen. + */ +export async function resolveInputArtifact(artifact, { lookup, download, destDir, maxBytes = DEFAULT_MAX_BYTES, readBack = readFileSync }) { + const chain = { + artifact_id: artifact.id, + requested_input_digest: artifact.digest, + resolved_storage_object: null, + downloaded_digest: null, + materialized_digest: null, + materialized_path: null, + }; + + const pointer = validatePointer(await lookup(artifact.id), artifact); + chain.resolved_storage_object = pointer.uri; + + if (pointer.bytes > maxBytes) { + refuse("artifact_too_large", `${artifact.id} is ${pointer.bytes} bytes, over the ${maxBytes}-byte ceiling for a worker input`); + } + + const bytes = await download(pointer.uri, { maxBytes }); + if (!(bytes instanceof Uint8Array)) { + refuse("download_invalid", `download of ${artifact.id} returned ${typeof bytes}, not bytes`); + } + // A short read is not a small file. The ledger recorded a length; bytes + // that stop early hash to something else and would be caught below, but + // naming the actual failure beats reporting a digest mismatch for it. + if (bytes.byteLength !== pointer.bytes) { + refuse("download_truncated", `${artifact.id} downloaded ${bytes.byteLength} bytes, the ledger recorded ${pointer.bytes}`); + } + chain.downloaded_digest = sha256OfBytes(bytes); + if (chain.downloaded_digest !== artifact.digest) { + refuse( + "digest_mismatch", + `${artifact.id} downloaded as ${chain.downloaded_digest.slice(0, 12)}… but the execution contract named ${artifact.digest.slice(0, 12)}…; this is a substitution, not a delivery`, + ); + } + + // CONTENT-ADDRESSED LOCAL NAME. Not the id, not anything from the uri: + // both are strings someone else chose, and a filename is a place. The + // digest is the only name here that the bytes themselves prove. + const finalPath = join(destDir, `${artifact.digest}.input`); + const tempPath = `${finalPath}.partial`; + mkdirSync(destDir, { recursive: true }); + // Written 0o600 and narrowed to 0o400 after the rename. Setting 0o400 at + // write time as well looked like defence in depth and was not: the chmod + // below decides the final mode, so a mutation of the write mode changed + // nothing observable. One mechanism that a test can see beats two where + // only one is load-bearing. + writeFileSync(tempPath, bytes, { mode: 0o600 }); + // Rename after the bytes are down, so a crash mid-write cannot leave a + // half-file sitting at the name a later run would treat as cached. + renameSync(tempPath, finalPath); + chmodSync(finalPath, 0o400); + + // Re-read from disk rather than re-hashing the buffer we already have. + // Hashing the in-memory copy would prove the download and call it the + // materialization -- the two are only the same claim if nothing went + // wrong between them, which is the thing being checked. + // `readBack` is injectable ONLY so this property is testable. Hashing the + // buffer we already have would prove the download and label it the + // materialization; they are the same claim only if nothing went wrong in + // between, which is the thing being checked. A mutation that hashes + // `bytes` here survived every test until this seam existed. + chain.materialized_digest = sha256OfBytes(readBack(finalPath)); + if (chain.materialized_digest !== artifact.digest) { + refuse("materialization_mismatch", `${artifact.id} materialized as ${chain.materialized_digest.slice(0, 12)}…, not ${artifact.digest.slice(0, 12)}…`); + } + chain.materialized_path = finalPath; + return chain; +} + +/** + * Resolve every declared input artifact, or refuse the task. + * + * All-or-nothing on purpose: a worker that ran with three of its four named + * inputs would produce a result nobody could interpret, and the contract + * digest covers the whole list. + */ +export async function resolveInputArtifacts(inputArtifacts, options) { + const declared = (inputArtifacts ?? []).map(validateInputArtifact); + const seen = new Set(); + for (const artifact of declared) { + if (seen.has(artifact.id)) { + refuse("duplicate_input_artifact", `inputArtifacts names ${artifact.id} twice`); + } + seen.add(artifact.id); + } + const chains = []; + for (const artifact of declared) { + chains.push(await resolveInputArtifact(artifact, options)); + } + return chains; +} From a9df5840f05a086d32e0b2beea54e6f347c1ef73 Mon Sep 17 00:00:00 2001 From: George Pu Date: Thu, 10 Sep 2026 15:01:13 -0400 Subject: [PATCH 2/4] worker: permission before publication, id+digest lookup, closed test seam Four corrections from review, all of which reproduced. 1. PERMISSION BEFORE PUBLICATION. I had concluded the write mode was useless because a chmod set the final mode. That was the wrong lesson: write -> rename -> chmod leaves an interval where the FINAL pathname exists and is still writable, and no test observed that interleaving, which is exactly why the mutant survived. The ordering was the load-bearing part and nothing was checking it. Now every narrowing happens on the TEMP path, identity is verified while the file is still unpublished, and the rename publishes something already read-only. A staged file that does not hash correctly never acquires the final name. 2. LOOKUP TAKES id AND digest. Sending only the id would require artifact ids to be globally unique, and the search behind this module found no registry semantics establishing that. More than one matching pointer is now `ambiguous_pointer` -- unresolvable, never pick-the-newest, because choosing by recency is how the wrong artifact arrives with every digest check passing. 3. THE READ-BACK SEAM IS NOT CALLER-CONTROLLED. It exists so the buffer-versus-materialization property is testable, and a caller must not be able to supply the function that decides what the worker believes it materialized. The production entry point now enumerates what it forwards instead of passing an options object through, and a test proves an injected readBack does not reach the resolver that way. 4. The destination directory is created 0o700 before anything is written into it. MUTATION NOTES, because three instruments were wrong before the code was. * My mutation harness reported all six mutants SURVIVING. It grepped `head -1` and caught an earlier console.log while hiding the assertion below it. Re-run on exit code: four caught, two survived. A mutation harness that cannot see a failure is worse than none, because it certifies. * The directory-privacy mutant then survived because the FIXTURE pre-created the destination with mkdtempSync, which already makes 0o700 directories -- the fixture was doing the thing under test. The dest now hands the resolver a directory it must create. * The write-mode and chmod-on-temp mutants each survived individually because the other still set 0o400. That is genuine defence in depth rather than dead code -- unlike the earlier chmod-after-rename case -- and the combined mutation IS caught, with "the temp file must be read-only before it is published". Still not established: no live lookup, no live download, no call site, and no authority route for a per-id pointer. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7 --- vinci/test/worker-input-artifacts.mjs | 97 ++++++++++++++++++++++++++- vinci/worker/input-artifacts.mjs | 88 +++++++++++++++++++----- 2 files changed, 165 insertions(+), 20 deletions(-) diff --git a/vinci/test/worker-input-artifacts.mjs b/vinci/test/worker-input-artifacts.mjs index 2a0bb9c5..fa2dfd37 100644 --- a/vinci/test/worker-input-artifacts.mjs +++ b/vinci/test/worker-input-artifacts.mjs @@ -5,7 +5,7 @@ import assert from "node:assert/strict"; import { createHash, randomUUID } from "node:crypto"; import { mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { DEFAULT_MAX_BYTES, @@ -27,7 +27,11 @@ const OTHER = new TextEncoder().encode("accepted findings context packet v2"); const OTHER_DIGEST = createHash("sha256").update(OTHER).digest("hex"); const URI = "s3://vgc-artifacts/ctx/9f2.tgz"; -const dest = () => mkdtempSync(join(tmpdir(), `input-artifacts-${randomUUID()}-`)); +// A destDir the resolver must CREATE. mkdtempSync already makes 0o700 +// directories, so handing one straight to the resolver meant the +// directory-privacy assertion could never discriminate -- the fixture was +// doing the thing under test. +const dest = () => join(mkdtempSync(join(tmpdir(), `input-artifacts-${randomUUID()}-`)), "inputs"); const artifact = (over = {}) => ({ id: "accepted-findings-context", digest: DIGEST, ...over }); const pointer = (over = {}) => ({ artifact_id: "accepted-findings-context", @@ -243,3 +247,92 @@ await refuses( assert.deepEqual(validatePointer(pointer(), artifact()), { uri: URI, bytes: BYTES.byteLength, sha256: DIGEST }); console.log("worker-input-artifacts: all controls passed"); + +// --- corrections from review ------------------------------------------------ + +// PERMISSION BEFORE PUBLICATION. Concluding that the write mode was useless +// because the final mode was chmod-ed was the wrong lesson: the ordering was +// load-bearing and nothing tested it. The final pathname must never exist +// writable, even briefly, so every narrowing happens on the temp path and the +// rename publishes something already read-only. +{ + const seen = []; + const chain = await resolveInputArtifact(artifact(), opts({ + // Observe the mode of the file at the moment it is read back, i.e. after + // staging and before/at publication. + readBack: (path) => { + seen.push({ path, mode: statSync(path).mode & 0o777 }); + return BYTES; + }, + })); + // The staged file is already 0o400 when its identity is verified, BEFORE + // it ever acquires the final name. + const staged = seen.find((s) => s.path.endsWith(".partial")); + assert.ok(staged, "identity must be verified while the file is still unpublished"); + assert.equal(staged.mode, 0o400, "the temp file must be read-only before it is published"); + // And the published file is read-only too. + assert.equal(statSync(chain.materialized_path).mode & 0o777, 0o400); + // The containing directory is private BEFORE anything is written into it, + // so the partial file is never reachable by another user even briefly. + assert.equal(statSync(dirname(chain.materialized_path)).mode & 0o777, 0o700); +} + +// A staged file whose bytes do not hash correctly never acquires the final +// name at all -- it is refused while still unpublished. +{ + const destDir = dest(); + let readCount = 0; + await refuses( + "materialization_mismatch", + () => resolveInputArtifact(artifact(), opts({ + destDir, + readBack: (path) => { readCount += 1; return path.endsWith(".partial") ? OTHER : BYTES; }, + })), + "a staged file that does not hash correctly", + ); + assert.equal(readCount, 1, "the refusal must happen at staging, before publication"); + assert.throws(() => statSync(join(destDir, `${DIGEST}.input`)), "the final name must not exist"); +} + +// --- id + digest, and ambiguity fails closed -------------------------------- + +// The resolver is asked for BOTH. Sending only the id would require artifact +// ids to be globally unique, which nothing establishes. +{ + const asked = []; + await resolveInputArtifact(artifact(), opts({ + lookup: async (id, digest) => { asked.push([id, digest]); return pointer(); }, + })); + assert.deepEqual(asked, [["accepted-findings-context", DIGEST]]); +} + +// More than one match is unresolvable, not a choice. Picking the newest is +// how the wrong artifact arrives with every digest check passing. +for (const [answer, why] of [ + [[pointer(), pointer({ uri: "s3://other/obj.tgz" })], "two matching pointers"], + [[], "an empty pointer list"], +]) { + await refuses("ambiguous_pointer", () => resolveInputArtifact(artifact(), opts({ lookup: async () => answer })), why); +} +// positive control: a single-element list resolves exactly like a bare object. +{ + const chain = await resolveInputArtifact(artifact(), opts({ lookup: async () => [pointer()] })); + assert.equal(chain.materialized_digest, DIGEST); +} + +// --- the read-back seam is not caller-controlled in production -------------- + +// resolveInputArtifacts is the production entry point. A caller must not be +// able to supply the function that decides what the worker believes it +// materialized, so the seam does not pass through it. +{ + let injected = false; + const chains = await resolveInputArtifacts([artifact()], { + ...opts(), + readBack: () => { injected = true; return OTHER; }, + }); + assert.equal(injected, false, "readBack must not be forwardable through the production entry point"); + assert.equal(chains[0].materialized_digest, DIGEST); +} + +console.log("worker-input-artifacts: review corrections passed"); diff --git a/vinci/worker/input-artifacts.mjs b/vinci/worker/input-artifacts.mjs index 3c1ec428..1d3406a5 100644 --- a/vinci/worker/input-artifacts.mjs +++ b/vinci/worker/input-artifacts.mjs @@ -38,7 +38,7 @@ // resolve id -> looked-up uri -> fetch -> verify sha256 AFTER download. import { createHash } from "node:crypto"; -import { chmodSync, mkdirSync, readFileSync, renameSync, writeFileSync } from "node:fs"; +import { chmodSync, closeSync, fsyncSync, mkdirSync, openSync, readFileSync, renameSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { isIdentifier } from "./contracts/digest.mjs"; @@ -95,6 +95,20 @@ export function validateInputArtifact(entry, index) { // {artifact_id, job_id, uri, bytes, sha256, created_at}. Only the four fields // this consumer needs are read, and the rest are ignored rather than trusted. export function validatePointer(pointer, artifact) { + // An authority may answer with a list. Nothing establishes that artifact + // ids form a globally unique namespace -- the investigation behind this + // module found no registry semantics at all -- so more than one match is + // a refusal, never a pick-the-newest. Choosing by recency is how the + // wrong artifact arrives with every digest check passing. + if (Array.isArray(pointer)) { + if (pointer.length !== 1) { + refuse( + "ambiguous_pointer", + `the authority returned ${pointer.length} pointer records for ${artifact.id}; artifact ids are not known to be globally unique, so this is unresolvable rather than a choice`, + ); + } + pointer = pointer[0]; + } if (pointer === null || typeof pointer !== "object") { refuse("pointer_invalid", `no pointer record for artifact ${artifact.id}`); } @@ -149,7 +163,11 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir materialized_path: null, }; - const pointer = validatePointer(await lookup(artifact.id), artifact); + // The resolver is asked for id AND expected digest. Sending only the id + // would require artifact ids to be globally unique, which nothing + // establishes; the digest lets the authority disambiguate rather than + // guess, and lets it refuse rather than return the wrong subject. + const pointer = validatePointer(await lookup(artifact.id, artifact.digest), artifact); chain.resolved_storage_object = pointer.uri; if (pointer.bytes > maxBytes) { @@ -179,27 +197,53 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir // digest is the only name here that the bytes themselves prove. const finalPath = join(destDir, `${artifact.digest}.input`); const tempPath = `${finalPath}.partial`; - mkdirSync(destDir, { recursive: true }); - // Written 0o600 and narrowed to 0o400 after the rename. Setting 0o400 at - // write time as well looked like defence in depth and was not: the chmod - // below decides the final mode, so a mutation of the write mode changed - // nothing observable. One mechanism that a test can see beats two where - // only one is load-bearing. - writeFileSync(tempPath, bytes, { mode: 0o600 }); - // Rename after the bytes are down, so a crash mid-write cannot leave a - // half-file sitting at the name a later run would treat as cached. + // The destination is private BEFORE anything is written into it, so the + // partial file is never reachable by another user even briefly. + mkdirSync(destDir, { recursive: true, mode: 0o700 }); + chmodSync(destDir, 0o700); + + // PERMISSION BEFORE PUBLICATION. An earlier version wrote, renamed, then + // chmod-ed -- which leaves an interval where the FINAL pathname exists + // and is still writable. Tests do not normally observe that interleaving, + // which is exactly why a mutation of the write mode survived: the ordering + // was the load-bearing part and nothing was checking it. Every narrowing + // now happens on the TEMP path, and the rename publishes something that is + // already read-only. + writeFileSync(tempPath, bytes, { mode: 0o400 }); + const handle = openSync(tempPath, "r"); + try { + fsyncSync(handle); + } finally { + closeSync(handle); + } + chmodSync(tempPath, 0o400); + // Verify identity while it is still unpublished: a temp file that does not + // hash correctly must never acquire the final name at all. + const stagedDigest = sha256OfBytes(readBack(tempPath)); + if (stagedDigest !== artifact.digest) { + refuse("materialization_mismatch", `${artifact.id} staged as ${stagedDigest.slice(0, 12)}…, not ${artifact.digest.slice(0, 12)}…`); + } renameSync(tempPath, finalPath); - chmodSync(finalPath, 0o400); + const dirHandle = openSync(destDir, "r"); + try { + fsyncSync(dirHandle); + } catch { + // Directory fsync is not portable everywhere; the rename is still + // atomic, so this is durability hardening rather than a correctness + // step, and failing it must not fail the delivery. + } finally { + closeSync(dirHandle); + } // Re-read from disk rather than re-hashing the buffer we already have. // Hashing the in-memory copy would prove the download and call it the // materialization -- the two are only the same claim if nothing went // wrong between them, which is the thing being checked. - // `readBack` is injectable ONLY so this property is testable. Hashing the - // buffer we already have would prove the download and label it the - // materialization; they are the same claim only if nothing went wrong in - // between, which is the thing being checked. A mutation that hashes - // `bytes` here survived every test until this seam existed. + // Re-read the PUBLISHED path. Hashing the buffer we already have would + // prove the download and label it the materialization; they are the same + // claim only if nothing went wrong in between, which is the thing being + // checked. A mutation that hashes `bytes` here survived every test until + // this seam existed. chain.materialized_digest = sha256OfBytes(readBack(finalPath)); if (chain.materialized_digest !== artifact.digest) { refuse("materialization_mismatch", `${artifact.id} materialized as ${chain.materialized_digest.slice(0, 12)}…, not ${artifact.digest.slice(0, 12)}…`); @@ -215,7 +259,15 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir * inputs would produce a result nobody could interpret, and the contract * digest covers the whole list. */ -export async function resolveInputArtifacts(inputArtifacts, options) { +export async function resolveInputArtifacts(inputArtifacts, { lookup, download, destDir, maxBytes = DEFAULT_MAX_BYTES }) { + // THE PRODUCTION ENTRY POINT ENUMERATES WHAT IT FORWARDS. `readBack` is a + // test seam and must stay one: forwarding an options object wholesale + // would let a caller supply the very function that decides what the + // worker believes it materialized. Trusted code picks that + // implementation, so it is not in this signature and cannot pass through + // it. Callers only reach it by calling the lower-level function directly, + // which production does not do. + const options = { lookup, download, destDir, maxBytes }; const declared = (inputArtifacts ?? []).map(validateInputArtifact); const seen = new Set(); for (const artifact of declared) { From 8e7d4f6ab98bfab47eb6560ef01757e505452f2e Mon Sep 17 00:00:00 2001 From: George Pu Date: Thu, 10 Sep 2026 15:08:41 -0400 Subject: [PATCH 3/4] worker: narrow the atomicity claim, bound the fetch namespace Two corrections from review, plus a mutant that closed a real gap. 1. THE ALL-OR-NOTHING COMMENT OVERCLAIMED. The loop publishes each artifact as it goes, so a refusal on artifact N leaves 1..N-1 materialized. The property that actually holds is EXECUTION-ATOMIC: no spawn unless every declared input resolved, downloaded, verified and materialized. The residue is verified, immutable and unreferenced -- cleanup debris, not partial execution -- but it is residue, and calling this a transaction was a claim the code does not implement. Narrowed rather than fixed with staging machinery: staging the whole set buys nothing while the execution-atomic property is enforced at the call site, which is where the wiring must test it. A test now asserts the residue exists rather than leaving the reader to assume rollback. 2. AN AUTHORITY THAT CAN SELECT AN OBJECT DOES NOT THEREBY GAIN ARBITRARY NETWORK-FETCH AUTHORITY. The pointer decides where this worker goes, so the caller must declare the storage namespace its downloader is qualified for, and a pointer outside it is refused however well-formed. There is NO permissive default: an omitted allowlist is `no_uri_allowlist`, because a default would make the trust boundary invisible at the call site, which is the one place it has to be visible. Refused cases include another bucket, an http endpoint, a file:// url, the link-local metadata address, and a prefix-adjacent bucket name. MUTATION, with the harness qualified first. Following the doctrine this session earned: the run began with a KNOWN-KILL CONTROL -- make `refuse` a no-op, which must be caught -- so a harness that cannot see failures is detected before it certifies anything. It was caught, then four real mutants ran. Three were caught. The fourth, `startsWith` -> `includes`, SURVIVED: every out-of-namespace case I had written also fails a substring test, so none could tell a prefix check from a substring check. Closed with the namespace appearing as a query parameter and buried in a key -- the two shapes where the difference is the whole attack. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7 --- vinci/test/worker-input-artifacts.mjs | 78 ++++++++++++++++++++++++++- vinci/worker/input-artifacts.mjs | 51 ++++++++++++++---- 2 files changed, 119 insertions(+), 10 deletions(-) diff --git a/vinci/test/worker-input-artifacts.mjs b/vinci/test/worker-input-artifacts.mjs index fa2dfd37..2ba00ce2 100644 --- a/vinci/test/worker-input-artifacts.mjs +++ b/vinci/test/worker-input-artifacts.mjs @@ -43,10 +43,13 @@ const pointer = (over = {}) => ({ ...over, }); +const NAMESPACE = ["s3://vgc-artifacts/"]; + const opts = (over = {}) => ({ lookup: async () => pointer(), download: async () => BYTES, destDir: dest(), + allowedUriPrefixes: NAMESPACE, ...over, }); @@ -244,7 +247,7 @@ await refuses( // --- the pointer validator in isolation ------------------------------------ -assert.deepEqual(validatePointer(pointer(), artifact()), { uri: URI, bytes: BYTES.byteLength, sha256: DIGEST }); +assert.deepEqual(validatePointer(pointer(), artifact(), NAMESPACE), { uri: URI, bytes: BYTES.byteLength, sha256: DIGEST }); console.log("worker-input-artifacts: all controls passed"); @@ -336,3 +339,76 @@ for (const [answer, why] of [ } console.log("worker-input-artifacts: review corrections passed"); + +// --- the download trust boundary ------------------------------------------- + +// An authority that can SELECT an object does not thereby gain arbitrary +// network-fetch authority. The pointer decides where this worker goes. +{ + // positive control: inside the qualified namespace, it resolves. + assert.ok(await resolveInputArtifact(artifact(), opts())); + + for (const [uri, why] of [ + ["s3://someone-elses-bucket/obj.tgz", "another bucket"], + ["https://evil.example/obj.tgz", "an http endpoint"], + ["file:///etc/passwd", "a local file url"], + ["http://169.254.169.254/latest/meta-data/", "a link-local metadata address"], + ["s3://vgc-artifacts-evil/obj.tgz", "a prefix-adjacent bucket name"], + // The namespace must be a PREFIX, not a substring. Every case above + // fails a substring test too, so none of them could tell `startsWith` + // from `includes` -- a mutation to `includes` survived until these. + ["https://evil.example/redirect?to=s3://vgc-artifacts/obj.tgz", "the namespace as a query parameter"], + ["s3://attacker-bucket/s3://vgc-artifacts/obj.tgz", "the namespace buried in a key"], + ]) { + await refuses( + "uri_outside_namespace", + () => resolveInputArtifact(artifact(), opts({ lookup: async () => pointer({ uri }) })), + why, + ); + } +} + +// No allowlist is a refusal, not a pass. A permissive default would make the +// trust boundary invisible at the call site, which is the one place it has +// to be visible. +for (const missing of [undefined, [], null, "s3://vgc-artifacts/"]) { + await refuses( + "no_uri_allowlist", + () => resolveInputArtifact(artifact(), { ...opts(), allowedUriPrefixes: missing }), + `an allowlist of ${JSON.stringify(missing)}`, + ); +} + +// The namespace reaches the primitive through the PRODUCTION entry point too, +// so wiring cannot accidentally drop it and get a permissive fetch. +await refuses( + "uri_outside_namespace", + () => resolveInputArtifacts([artifact()], { ...opts(), lookup: async () => pointer({ uri: "s3://elsewhere/x" }) }), + "an out-of-namespace pointer through the production entry point", +); + +// --- execution-atomic, NOT materialization-atomic --------------------------- + +// The honest property, asserted rather than described: a refusal on the +// second artifact leaves the first one materialized. That residue is +// verified and immutable, but it exists -- calling this a transaction would +// be a claim the code does not implement. +{ + const destDir = dest(); + const first = artifact(); + const second = artifact({ id: "second-input", digest: OTHER_DIGEST }); + await refuses( + "identity_disagreement", + () => resolveInputArtifacts([first, second], { + ...opts(), + destDir, + lookup: async (id) => (id === first.id ? pointer() : pointer({ artifact_id: "second-input", sha256: DIGEST })), + }), + "the second of two inputs disagreeing", + ); + // The FIRST artifact is on disk. This is residue, not rollback. + assert.equal(statSync(join(destDir, `${DIGEST}.input`)).mode & 0o777, 0o400, + "the earlier artifact stays materialized: execution-atomic, not materialization-atomic"); +} + +console.log("worker-input-artifacts: trust-boundary controls passed"); diff --git a/vinci/worker/input-artifacts.mjs b/vinci/worker/input-artifacts.mjs index 1d3406a5..fed0d7c1 100644 --- a/vinci/worker/input-artifacts.mjs +++ b/vinci/worker/input-artifacts.mjs @@ -94,7 +94,7 @@ export function validateInputArtifact(entry, index) { // The pointer shape vinci-gpu-control's artifacts ledger already emits: // {artifact_id, job_id, uri, bytes, sha256, created_at}. Only the four fields // this consumer needs are read, and the rest are ignored rather than trusted. -export function validatePointer(pointer, artifact) { +export function validatePointer(pointer, artifact, allowedUriPrefixes) { // An authority may answer with a list. Nothing establishes that artifact // ids form a globally unique namespace -- the investigation behind this // module found no registry semantics at all -- so more than one match is @@ -134,6 +134,25 @@ export function validatePointer(pointer, artifact) { `the artifact ledger says ${artifact.id} is ${pointer.sha256.slice(0, 12)}… but the execution contract expects ${artifact.digest.slice(0, 12)}…; these are different claims from different authorities and neither one settles the other`, ); } + // AN AUTHORITY THAT CAN SELECT AN OBJECT DOES NOT THEREBY GAIN ARBITRARY + // NETWORK-FETCH AUTHORITY. The pointer decides where this worker will go, + // so the caller must declare the storage namespace its downloader is + // qualified for, and a pointer outside it is refused however well-formed. + // No default: an omitted allowlist is a refusal, because a permissive + // default would make the trust boundary invisible at the call site -- + // which is the one place it has to be visible. + if (!Array.isArray(allowedUriPrefixes) || allowedUriPrefixes.length === 0) { + refuse( + "no_uri_allowlist", + "the caller must declare which storage namespace its downloader is qualified to fetch from; there is no permissive default", + ); + } + if (!allowedUriPrefixes.some((prefix) => typeof prefix === "string" && prefix && pointer.uri.startsWith(prefix))) { + refuse( + "uri_outside_namespace", + `the pointer for ${artifact.id} names ${pointer.uri}, which is outside the qualified storage namespace [${allowedUriPrefixes.join(", ")}]`, + ); + } return { uri: pointer.uri, bytes: pointer.bytes, sha256: pointer.sha256 }; } @@ -153,7 +172,7 @@ export function validatePointer(pointer, artifact) { * `input delivery observation` consumes. Every stage is reported, including * the ones that did not happen. */ -export async function resolveInputArtifact(artifact, { lookup, download, destDir, maxBytes = DEFAULT_MAX_BYTES, readBack = readFileSync }) { +export async function resolveInputArtifact(artifact, { lookup, download, destDir, allowedUriPrefixes, maxBytes = DEFAULT_MAX_BYTES, readBack = readFileSync }) { const chain = { artifact_id: artifact.id, requested_input_digest: artifact.digest, @@ -167,7 +186,7 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir // would require artifact ids to be globally unique, which nothing // establishes; the digest lets the authority disambiguate rather than // guess, and lets it refuse rather than return the wrong subject. - const pointer = validatePointer(await lookup(artifact.id, artifact.digest), artifact); + const pointer = validatePointer(await lookup(artifact.id, artifact.digest), artifact, allowedUriPrefixes); chain.resolved_storage_object = pointer.uri; if (pointer.bytes > maxBytes) { @@ -253,13 +272,27 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir } /** - * Resolve every declared input artifact, or refuse the task. + * Resolve every declared input artifact, or refuse. + * + * EXECUTION-ATOMIC, NOT MATERIALIZATION-ATOMIC. The distinction matters and + * the first version of this comment got it wrong by calling the whole thing + * "all-or-nothing". + * + * What holds: no worker spawn occurs unless EVERY declared input resolved, + * downloaded, verified and materialized. A refusal here propagates, and a + * task that ran with three of its four named inputs would produce a result + * nobody could interpret. * - * All-or-nothing on purpose: a worker that ran with three of its four named - * inputs would produce a result nobody could interpret, and the contract - * digest covers the whole list. + * What does NOT hold: this loop publishes each artifact as it goes, so a + * refusal on artifact N leaves artifacts 1..N-1 already materialized. That + * residue is verified, immutable and unreferenced -- it is cleanup debris, + * not partial execution -- but it is residue, and calling this a transaction + * would be a claim the code does not implement. Staging the whole set before + * publishing any of it is possible and is deliberately not done: it buys + * nothing while the execution-atomic property is enforced at the call site, + * which is where the eventual wiring must test it. */ -export async function resolveInputArtifacts(inputArtifacts, { lookup, download, destDir, maxBytes = DEFAULT_MAX_BYTES }) { +export async function resolveInputArtifacts(inputArtifacts, { lookup, download, destDir, allowedUriPrefixes, maxBytes = DEFAULT_MAX_BYTES }) { // THE PRODUCTION ENTRY POINT ENUMERATES WHAT IT FORWARDS. `readBack` is a // test seam and must stay one: forwarding an options object wholesale // would let a caller supply the very function that decides what the @@ -267,7 +300,7 @@ export async function resolveInputArtifacts(inputArtifacts, { lookup, download, // implementation, so it is not in this signature and cannot pass through // it. Callers only reach it by calling the lower-level function directly, // which production does not do. - const options = { lookup, download, destDir, maxBytes }; + const options = { lookup, download, destDir, allowedUriPrefixes, maxBytes }; const declared = (inputArtifacts ?? []).map(validateInputArtifact); const seen = new Set(); for (const artifact of declared) { From 229bb4c68dac50078cdc62ad68396dfc5de545e0 Mon Sep 17 00:00:00 2001 From: George Pu Date: Thu, 10 Sep 2026 15:21:05 -0400 Subject: [PATCH 4/4] worker: close a symlink write-through in the staging path (CRITICAL) Adversarial review of ambient capability found a real defect, and it is the one that matters: the resolver followed a symlink at its staging path. THE ATTACK, reproduced before the fix. Pre-place a symlink at the deterministic staging name `${digest}.input.partial` in destDir, pointing at any file the worker can write. writeFileSync and chmodSync BOTH follow it, so the victim outside destDir is overwritten with the artifact bytes and forced to 0o400. renameSync does NOT follow, so it publishes the LINK as the materialized file. The final digest re-read follows the link and matches -- so the chain reports success, with a correct materialized_digest, while an arbitrary file has been clobbered and permission-locked and `materialized_path` points out of the sandbox. Worse for the module's own claim: the attacker still owns that location, so the "materialized, verified, immutable" artifact can be rewritten after resolution returns. Measured: victim content became the artifact bytes, mode 400, and lstat(materialized_path).isSymbolicLink() was true. Three independent defences, because one is a single point of failure on a path that had none: * the staging name is now unpredictable (randomUUID), so it cannot be pre-created; * the write is `flag: "wx"` -- exclusive create fails EEXIST on anything already there, symlink included, rather than writing through it; * the published path is lstat-ed and must be a regular file. MUTATION, harness qualified first with a known-kill control. Y2 (exclusive create) and Y3 (lstat) each SURVIVE individual removal, because the unpredictable name alone already blocks the attack. That is the redundant-guards-mask-each-other pattern, so they were removed PAIRWISE: with only lstat left the attack dies as `published_path_not_a_regular_file`; with only exclusive-create left it dies as `staging_path_occupied`; with all three gone the symlink test goes red with "a file outside destDir must not be written through a symlink". Each backstop demonstrably contributes rather than being decoration. Also from the same review: * Unanchored allowlist prefixes are refused. `s3://vgc-artifacts` without the delimiter admits `s3://vgc-artifacts@evil.example/x`, which passes startsWith while a WHATWG parser reads host evil.example and userinfo vgc-artifacts. A structural rule on configuration, not a URI parser -- the module previously placed no requirement on these entries at all. * Wrong-typed inputArtifacts raised a bare TypeError, escaping callers that catch the documented InputArtifactError contract. Now refuses. * The size ceiling is documented as HONESTY-DEPENDENT: it reads the pointer's declared size, so a lying authority can under-declare and the adapter still buffers the real object before this module sees a byte. maxBytes reaches download as ADVISORY and this module cannot verify the adapter honoured it. What is enforced is delivered-length equals declared-length, which bounds materialization, not transfer. * `download_truncated` renamed `download_length_mismatch`: an oversized payload is not a truncated one. The reviewer confirmed the sponsor's URI question directly: `..` and `%2e%2e` both pass the string check and collapse to the same path under a real parser. That divergence stands as a known limit of a prefix test; the structured replacement belongs with the real downloader integration and is deliberately not built here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013aLpjQi8CaDqo6WoGFRAU7 --- vinci/test/worker-input-artifacts.mjs | 93 ++++++++++++++++++++++++++- vinci/worker/input-artifacts.mjs | 65 +++++++++++++++++-- 2 files changed, 150 insertions(+), 8 deletions(-) diff --git a/vinci/test/worker-input-artifacts.mjs b/vinci/test/worker-input-artifacts.mjs index 2ba00ce2..7c25370f 100644 --- a/vinci/test/worker-input-artifacts.mjs +++ b/vinci/test/worker-input-artifacts.mjs @@ -3,7 +3,7 @@ // a guard that fires on everything is visible as one. import assert from "node:assert/strict"; import { createHash, randomUUID } from "node:crypto"; -import { mkdtempSync, readFileSync, statSync, writeFileSync } from "node:fs"; +import { lstatSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, statSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; @@ -165,7 +165,7 @@ await refuses("pointer_invalid", () => resolveInputArtifact(artifact(), opts({ l { await refuses( - "download_truncated", + "download_length_mismatch", () => resolveInputArtifact(artifact(), opts({ download: async () => BYTES.slice(0, 5) })), "a short read", ); @@ -412,3 +412,92 @@ await refuses( } console.log("worker-input-artifacts: trust-boundary controls passed"); + +// --- symlink attack on the staging path (CRITICAL, found in review) -------- + +// Pre-place a symlink where the resolver will stage, pointing at a file +// OUTSIDE destDir. Before the fix: writeFileSync and chmodSync follow the +// link, so the victim was overwritten with the artifact bytes and forced to +// 0o400; renameSync does NOT follow, so the published "materialized" file +// became a symlink out of the sandbox; and the digest re-read followed it +// and matched, so the whole chain reported success. +{ + const root = mkdtempSync(join(tmpdir(), `symlink-${randomUUID()}-`)); + const destDir = join(root, "inputs"); + mkdirSync(destDir, { recursive: true, mode: 0o700 }); + const victim = join(root, "victim.txt"); + writeFileSync(victim, "ORIGINAL", { mode: 0o644 }); + // The deterministic name the resolver used before the fix. + symlinkSync(victim, join(destDir, `${DIGEST}.input.partial`)); + + const chain = await resolveInputArtifact(artifact(), opts({ destDir })); + + assert.equal(readFileSync(victim, "utf8"), "ORIGINAL", "a file outside destDir must not be written through a symlink"); + assert.equal(statSync(victim).mode & 0o777, 0o644, "a file outside destDir must not be permission-clobbered"); + assert.equal(lstatSync(chain.materialized_path).isSymbolicLink(), false, "the published artifact must be a real file, not a link out of the sandbox"); + assert.equal(lstatSync(chain.materialized_path).isFile(), true); + assert.equal(readFileSync(chain.materialized_path, "utf8"), "accepted findings context packet v1"); +} + +// The staging name is unpredictable, so it cannot be pre-created at all. +{ + const destDir = dest(); + await resolveInputArtifact(artifact(), opts({ destDir })); + const names = readdirSync(destDir); + assert.deepEqual(names, [`${DIGEST}.input`], "no residue, and the published name is digest-derived"); +} +{ + // Two resolutions of the same artifact into one directory do not collide + // on the staging path -- which a fixed `.partial` name would. + const destDir = dest(); + await resolveInputArtifact(artifact(), opts({ destDir })); + await resolveInputArtifact(artifact(), opts({ destDir })); + assert.deepEqual(readdirSync(destDir), [`${DIGEST}.input`]); +} + +// Exclusive create: anything already sitting at the staging path is a +// refusal, never something written through. The staging name is random, so +// this is exercised by driving the write at a path that already exists -- +// the property is `flag: "wx"`, not the name. +{ + const destDir = dest(); + mkdirSync(destDir, { recursive: true, mode: 0o700 }); + const squatted = join(destDir, "squatted.partial"); + writeFileSync(squatted, "squatter"); + assert.throws( + () => writeFileSync(squatted, BYTES, { mode: 0o400, flag: "wx" }), + (error) => error.code === "EEXIST", + "exclusive create must refuse an occupied path rather than write through it", + ); + // ...and the squatter is untouched, which is the point. + assert.equal(readFileSync(squatted, "utf8"), "squatter"); +} + +// --- wrong-typed input is the module's own refusal, not a TypeError -------- + +for (const bad of [{}, "hello", 42, true]) { + await refuses("invalid_input_artifact", () => resolveInputArtifacts(bad, opts()), `inputArtifacts of ${JSON.stringify(bad)}`); +} +assert.deepEqual(await resolveInputArtifacts(null, opts()), [], "null is still 'no declared inputs'"); + +// --- unanchored allowlist prefixes are refused ----------------------------- + +// `s3://vgc-artifacts` without the delimiter admits +// `s3://vgc-artifacts@evil.example/x`, which passes startsWith while a +// WHATWG parser reads host evil.example. Demonstrated in review. +await refuses( + "unanchored_uri_prefix", + () => resolveInputArtifact(artifact(), { ...opts(), allowedUriPrefixes: ["s3://vgc-artifacts"] }), + "an unanchored prefix", +); +await refuses( + "unanchored_uri_prefix", + () => resolveInputArtifact(artifact(), { + ...opts(), + allowedUriPrefixes: ["s3://vgc-artifacts"], + lookup: async () => pointer({ uri: "s3://vgc-artifacts@evil.example/x" }), + }), + "the userinfo-confusion URI its absence would have admitted", +); + +console.log("worker-input-artifacts: ambient-capability controls passed"); diff --git a/vinci/worker/input-artifacts.mjs b/vinci/worker/input-artifacts.mjs index fed0d7c1..2af4d2ef 100644 --- a/vinci/worker/input-artifacts.mjs +++ b/vinci/worker/input-artifacts.mjs @@ -37,8 +37,8 @@ // Modelled on the one existing precedent in the fleet, `vgc artifacts pull`: // resolve id -> looked-up uri -> fetch -> verify sha256 AFTER download. -import { createHash } from "node:crypto"; -import { chmodSync, closeSync, fsyncSync, mkdirSync, openSync, readFileSync, renameSync, writeFileSync } from "node:fs"; +import { createHash, randomUUID } from "node:crypto"; +import { chmodSync, closeSync, fsyncSync, lstatSync, mkdirSync, openSync, readFileSync, renameSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { isIdentifier } from "./contracts/digest.mjs"; @@ -48,6 +48,14 @@ const HEX64 = /^[0-9a-f]{64}$/; // A context packet is prose and JSON. This ceiling is not a policy about // artifacts in general -- it is the bound on what this consumer will pull // into a prompt, and a larger input is a refusal rather than a slow fetch. +// +// HONESTY-DEPENDENT. The pre-fetch check reads the pointer's DECLARED size, +// so a lying or compromised authority can under-declare and the adapter will +// still buffer whatever the object really is before this module sees a byte. +// `maxBytes` reaches `download` as ADVISORY: enforcing it during transfer is +// the adapter's job and this module cannot verify that it did. What is +// enforced here is that the delivered length equals the declared one, which +// bounds what can be MATERIALIZED, not what can be transferred. export const DEFAULT_MAX_BYTES = 8 * 1024 * 1024; export class InputArtifactError extends Error { @@ -147,7 +155,21 @@ export function validatePointer(pointer, artifact, allowedUriPrefixes) { "the caller must declare which storage namespace its downloader is qualified to fetch from; there is no permissive default", ); } - if (!allowedUriPrefixes.some((prefix) => typeof prefix === "string" && prefix && pointer.uri.startsWith(prefix))) { + // ANCHORED PREFIXES ONLY. `s3://vgc-artifacts` without the trailing + // delimiter admits `s3://vgc-artifacts@evil.example/x`, which passes + // startsWith while a WHATWG parser reads host `evil.example` and userinfo + // `vgc-artifacts`. Demonstrated in review. Requiring the delimiter is a + // structural rule on configuration, not a URI parser, and the module + // previously placed no requirement on these entries at all. + for (const prefix of allowedUriPrefixes) { + if (typeof prefix !== "string" || !prefix.endsWith("/")) { + refuse( + "unanchored_uri_prefix", + `allowedUriPrefixes entry ${JSON.stringify(prefix)} must end with "/"; an unanchored prefix admits userinfo and host confusion`, + ); + } + } + if (!allowedUriPrefixes.some((prefix) => pointer.uri.startsWith(prefix))) { refuse( "uri_outside_namespace", `the pointer for ${artifact.id} names ${pointer.uri}, which is outside the qualified storage namespace [${allowedUriPrefixes.join(", ")}]`, @@ -201,7 +223,7 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir // that stop early hash to something else and would be caught below, but // naming the actual failure beats reporting a digest mismatch for it. if (bytes.byteLength !== pointer.bytes) { - refuse("download_truncated", `${artifact.id} downloaded ${bytes.byteLength} bytes, the ledger recorded ${pointer.bytes}`); + refuse("download_length_mismatch", `${artifact.id} downloaded ${bytes.byteLength} bytes, the ledger recorded ${pointer.bytes}`); } chain.downloaded_digest = sha256OfBytes(bytes); if (chain.downloaded_digest !== artifact.digest) { @@ -215,7 +237,16 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir // both are strings someone else chose, and a filename is a place. The // digest is the only name here that the bytes themselves prove. const finalPath = join(destDir, `${artifact.digest}.input`); - const tempPath = `${finalPath}.partial`; + // UNPREDICTABLE temp name. A deterministic one (`${digest}.input.partial`) + // is a name an attacker can pre-create -- and a symlink there is followed + // by both writeFileSync and chmodSync, so the artifact bytes land on the + // link's TARGET and force it to 0o400, while renameSync (which does not + // follow) then publishes the link itself as the "materialized" file. The + // digest re-read follows the link and matches, so the whole chain reports + // success while an arbitrary file outside destDir has been overwritten and + // permission-locked, and `materialized_path` points out of the sandbox. + // Reproduced before this fix; see the symlink controls in the test file. + const tempPath = `${finalPath}.${randomUUID()}.partial`; // The destination is private BEFORE anything is written into it, so the // partial file is never reachable by another user even briefly. mkdirSync(destDir, { recursive: true, mode: 0o700 }); @@ -228,7 +259,17 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir // was the load-bearing part and nothing was checking it. Every narrowing // now happens on the TEMP path, and the rename publishes something that is // already read-only. - writeFileSync(tempPath, bytes, { mode: 0o400 }); + // `wx` is exclusive-create: it fails with EEXIST on anything already at + // this path, symlink included, instead of following it. Unpredictable name + // AND exclusive create -- either alone is weaker than it looks. + try { + writeFileSync(tempPath, bytes, { mode: 0o400, flag: "wx" }); + } catch (error) { + if (error?.code === "EEXIST") { + refuse("staging_path_occupied", `${tempPath} already exists; refusing to write through whatever is there`); + } + throw error; + } const handle = openSync(tempPath, "r"); try { fsyncSync(handle); @@ -267,6 +308,13 @@ export async function resolveInputArtifact(artifact, { lookup, download, destDir if (chain.materialized_digest !== artifact.digest) { refuse("materialization_mismatch", `${artifact.id} materialized as ${chain.materialized_digest.slice(0, 12)}…, not ${artifact.digest.slice(0, 12)}…`); } + // The published path must be a REGULAR FILE we created, not a link to + // somewhere else. Belt and braces behind the two guards above, and the + // one check that would have caught the symlink defect on its own. + const published = lstatSync(finalPath); + if (!published.isFile()) { + refuse("published_path_not_a_regular_file", `${finalPath} is not a regular file after publication`); + } chain.materialized_path = finalPath; return chain; } @@ -301,6 +349,11 @@ export async function resolveInputArtifacts(inputArtifacts, { lookup, download, // it. Callers only reach it by calling the lower-level function directly, // which production does not do. const options = { lookup, download, destDir, allowedUriPrefixes, maxBytes }; + if (inputArtifacts !== undefined && inputArtifacts !== null && !Array.isArray(inputArtifacts)) { + // Everything else in this module refuses through InputArtifactError; a + // bare TypeError escapes a caller that catches the documented contract. + refuse("invalid_input_artifact", `inputArtifacts is a list, got ${typeof inputArtifacts}`); + } const declared = (inputArtifacts ?? []).map(validateInputArtifact); const seen = new Set(); for (const artifact of declared) {