diff --git a/.github/scripts/resolve-sandbox-image.mjs b/.github/scripts/resolve-sandbox-image.mjs index 4d02ad1e8bb..0e4be042a56 100644 --- a/.github/scripts/resolve-sandbox-image.mjs +++ b/.github/scripts/resolve-sandbox-image.mjs @@ -1,5 +1,5 @@ #!/usr/bin/env node -import { appendFileSync } from 'node:fs'; +import { closeSync, constants, fstatSync, openSync, writeSync } from 'node:fs'; import { spawn } from 'node:child_process'; import { pathToFileURL } from 'node:url'; @@ -73,55 +73,230 @@ async function fetchLatestGhcrSemver() { return latest; } -function pullImage(command, image) { +// Pin the daemon endpoint for every spawn below. The docker CLI resolves its +// endpoint in precedence order DOCKER_HOST, --context, DOCKER_CONTEXT, and +// finally `currentContext` in $DOCKER_CONFIG/config.json — a file shared by +// every runner registration on the self-hosted pool. Clearing DOCKER_CONTEXT +// is not enough: an empty value falls through to currentContext, so a +// co-resident process that rewrites config.json (or sets DOCKER_HOST through +// $GITHUB_ENV) can point the pull and the inspect at a daemon it controls and +// hand back whatever digest it likes. Name the context explicitly and drop +// DOCKER_HOST, so nothing above `default` in that order is reachable. There is +// deliberately no env override: a variable that selects the endpoint would be +// settable through the same channel this closes. DOCKER_CONFIG is left alone — +// it carries the registry credentials the pull needs. +export function sandboxSpawnEnv(env = process.env) { + const childEnv = { ...env, DOCKER_CONTEXT: 'default' }; + delete childEnv.DOCKER_HOST; + return childEnv; +} + +// $GITHUB_ENV and $GITHUB_OUTPUT are runner-managed files under $RUNNER_TEMP, +// which on the shared pool is writable by anything running at the runner's +// uid. A plain append opens whatever sits at that path: a planted FIFO with no +// reader blocks open(2) until the step timeout, turning one line of writable +// state into a per-round hang. O_NONBLOCK makes that case an immediate ENXIO, +// and the post-open fstat refuses every non-regular file — the type check, not +// the path, is what holds. O_NOFOLLOW is deliberately not set: the runner may +// legitimately place these files behind a symlinked temp directory. +export function appendStepFile(file, line) { + let fd; + try { + fd = openSync( + file, + constants.O_WRONLY | + constants.O_CREAT | + constants.O_APPEND | + constants.O_NONBLOCK, + 0o600, + ); + if (!fstatSync(fd).isFile()) { + throw new Error( + `${file} is not a regular file; refusing to write step state to it.`, + ); + } + // writeSync can come up short on a signal; appendFileSync loops for us + // and this does not, so drain the buffer explicitly rather than shipping + // a truncated `image=` line that a consumer would read as a valid one. + const payload = Buffer.from(line, 'utf8'); + for (let written = 0; written < payload.length; ) { + written += writeSync(fd, payload, written, payload.length - written); + } + } finally { + if (fd !== undefined) { + closeSync(fd); + } + } +} + +// The `Digest: sha256:…` line docker prints for the tag it just resolved is +// the only pull-time content identity: the post-pull inspect can race a +// `docker tag` swap (see repoDigestOf), so the exported reference must be +// bound to what the pull itself reported, never to inspect alone. +export function parsePullDigest(pullOutput) { + return pullOutput.match(/^Digest: (sha256:[0-9a-f]{64})\s*$/m)?.[1] ?? ''; +} + +// The spawn guard lives in ONE place: the endpoint pin, the settle-once +// finish, the SIGKILL timer, the stdout accumulation, and the error/close +// wiring. The pull and the inspect used to carry near-verbatim copies that +// had already drifted (#9527 review) — a fix to any of those paths must +// reach every docker invocation, and no future caller can drop the pin. +function spawnDockerCapture(command, args, { timeoutMs, label, onChunk }) { return new Promise((resolve) => { - const child = spawn(command, ['pull', image], { stdio: 'inherit' }); + const child = spawn(command, args, { + stdio: ['ignore', 'pipe', 'inherit'], + env: sandboxSpawnEnv(), + }); + let stdout = ''; let settled = false; let timer; - const finish = (ok) => { + const finish = (exitCode) => { if (settled) return; settled = true; clearTimeout(timer); - resolve(ok); + resolve({ exitCode, stdout }); }; timer = setTimeout(() => { - console.error( - `::error::Timed out pulling ${image} after ${PULL_TIMEOUT_MS / 1000}s.`, - ); + console.error(`::error::Timed out ${label} after ${timeoutMs / 1000}s.`); child.kill('SIGKILL'); - finish(false); - }, PULL_TIMEOUT_MS); + finish(null); + }, timeoutMs); + child.stdout.on('data', (chunk) => { + stdout += chunk; + onChunk?.(chunk); + }); child.on('error', (error) => { console.error( - `::error::Failed to start '${command} pull ${image}': ${error.message}`, + `::error::Failed to start '${command} ${args.join(' ')}': ${error.message}`, ); - finish(false); + finish(null); }); child.on('close', (code) => { if (code !== 0) { console.error( - `::error::'${command} pull ${image}' exited with code ${code}.`, + `::error::'${command} ${args.join(' ')}' exited with code ${code}.`, ); } - finish(code === 0); + finish(code); }); }); } -function exportImage(image) { +export function pullImage(command, image, timeoutMs = PULL_TIMEOUT_MS) { + return spawnDockerCapture(command, ['pull', image], { + timeoutMs, + label: `pulling ${image}`, + onChunk: (chunk) => process.stdout.write(chunk), + }).then(({ exitCode, stdout }) => + exitCode === 0 + ? { ok: true, digest: parsePullDigest(stdout) } + : { ok: false, digest: '' }, + ); +} + +// The repository part of an image reference: everything before the :tag / +// @digest. A registry port keeps its colon — the tag only ever follows the +// LAST '/'. +export function repoOfImage(image) { + const withoutDigest = image.split('@')[0]; + const lastColon = withoutDigest.lastIndexOf(':'); + const lastSlash = withoutDigest.lastIndexOf('/'); + return lastColon > lastSlash + ? withoutDigest.slice(0, lastColon) + : withoutDigest; +} + +// Resolve a PULLED image to its content digest (repo@sha256:…). The tag +// alone is a mutable local handle: `docker run ` resolves against the +// local store without re-pull, and a co-resident process with daemon access +// can `docker tag` different content under the same name between resolve +// and gate. A digest reference cannot be moved by `docker tag`/`docker build`. +// The export must be the EXACT `@` RepoDigests entry: +// RepoDigests is shared by every tag of the same content, so `docker tag` +// of the pulled image adds an alphabetically-sorted entry for the new name +// and index 0 can move OFF the pulled repo (a suffix-only digest check +// still passes) — and retagged attacker content keeps ITS original repo, so +// only the pulled repo + the pull's own `Digest:` line together bind the +// export to the content the pull fetched (#9214 review). +export function repoDigestOf( + command, + image, + expectedDigest, + timeoutMs = FETCH_TIMEOUT_MS, +) { + return spawnDockerCapture( + command, + ['image', 'inspect', '--format', '{{json .RepoDigests}}', image], + { timeoutMs, label: `inspecting ${image}` }, + ) + .then(({ exitCode, stdout }) => (exitCode === 0 ? stdout.trim() : '')) + .then((raw) => { + // `null`/`[]` (no RepoDigests, a locally built image), `` and + // empty (the inspect failed) all mean there is no repository digest — + // the mutable tag is exactly what must not be exported. + let digests = []; + try { + const parsed = JSON.parse(raw.trim()); + if (Array.isArray(parsed)) { + digests = parsed.filter((entry) => typeof entry === 'string'); + } + } catch { + // Non-JSON output carries no digests. + } + // Docker records Hub repos in canonical short form in RepoDigests — + // `docker.io/library/busybox` is stored as `busybox@sha256:…` — so fold + // those prefixes before matching, or a fully-qualified Hub reference + // fails closed on its own correct digest. + const repo = repoOfImage(image).replace(/^docker\.io\/(library\/)?/, ''); + const digest = + digests.find((entry) => entry === `${repo}@${expectedDigest}`) ?? ''; + if (digest.includes('@sha256:')) { + return digest; + } + if (digests.length > 0) { + throw new Error( + `Pulled image ${image} resolved to digests none of which is '${repo}@${expectedDigest}' (${digests.join(', ')}); refusing to export a foreign or mutable reference.`, + ); + } + throw new Error( + `Pulled image ${image} resolved to no repository digest ('${raw.trim()}'); refusing to export a mutable tag.`, + ); + }); +} + +export function exportImage(image) { if (process.env.GITHUB_ENV) { - appendFileSync(process.env.GITHUB_ENV, `QWEN_SANDBOX_IMAGE=${image}\n`); + appendStepFile(process.env.GITHUB_ENV, `QWEN_SANDBOX_IMAGE=${image}\n`); + } + // Also as a step OUTPUT: $GITHUB_ENV is a file later steps can append to, + // so a consumer that must not be steered by branch code (the verification + // gate's container image) reads the expression-context value instead. + if (process.env.GITHUB_OUTPUT) { + appendStepFile(process.env.GITHUB_OUTPUT, `image=${image}\n`); } console.log(`QWEN_SANDBOX_IMAGE=${image}`); } +// The refusal policy is the same for both export paths: no `Digest:` line +// from the pull means nothing binds the export to the pulled content. +async function exportDigestBoundImage(command, image, pull) { + if (!pull.digest) { + throw new Error( + `'${command} pull ${image}' reported no Digest line; refusing to export an unbound image reference.`, + ); + } + exportImage(await repoDigestOf(command, image, pull.digest)); +} + async function main() { const requestedImage = validateRequestedImage(process.argv[2]); const command = process.env.SANDBOX_COMMAND || 'docker'; - if (await pullImage(command, requestedImage)) { - exportImage(requestedImage); + const requestedPull = await pullImage(command, requestedImage); + if (requestedPull.ok) { + await exportDigestBoundImage(command, requestedImage, requestedPull); return; } @@ -136,10 +311,11 @@ async function main() { console.warn( `::warning::Falling back from ${requestedImage} to latest GHCR semver ${fallbackImage}; sandbox image version may differ from package version.`, ); - if (!(await pullImage(command, fallbackImage))) { + const fallbackPull = await pullImage(command, fallbackImage); + if (!fallbackPull.ok) { throw new Error(`Fallback sandbox image failed to pull: ${fallbackImage}`); } - exportImage(fallbackImage); + await exportDigestBoundImage(command, fallbackImage, fallbackPull); } if ( diff --git a/.github/scripts/resolve-sandbox-image.test.mjs b/.github/scripts/resolve-sandbox-image.test.mjs index fc583155b28..d172f615e32 100644 --- a/.github/scripts/resolve-sandbox-image.test.mjs +++ b/.github/scripts/resolve-sandbox-image.test.mjs @@ -1,9 +1,33 @@ import assert from 'node:assert/strict'; import test from 'node:test'; +import { + chmodSync, + closeSync, + constants, + existsSync, + mkdtempSync, + openSync, + readFileSync, + readdirSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { execFileSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { parse } from 'yaml'; import { latestSemverTag, validateRequestedImage, + exportImage, + repoDigestOf, + repoOfImage, + parsePullDigest, + pullImage, + sandboxSpawnEnv, + appendStepFile, } from './resolve-sandbox-image.mjs'; test('latestSemverTag returns the highest stable semver tag', () => { @@ -40,3 +64,690 @@ test('validateRequestedImage rejects missing package config output', () => { ); } }); + +test('exportImage publishes the resolved image as a step output', () => { + // The autofix gate reads this output (GATE_IMAGE) to choose the container + // it runs the branch's build/test in — deliberately NOT $GITHUB_ENV, which + // an earlier step can append to. An empty output makes the gate wrapper + // refuse and every round take the gate-crashed retry path, so the write is + // load-bearing enough to pin. + const dir = mkdtempSync(join(tmpdir(), 'sandbox-image-')); + const outFile = join(dir, 'out'); + const envFile = join(dir, 'env'); + const saved = { out: process.env.GITHUB_OUTPUT, env: process.env.GITHUB_ENV }; + try { + process.env.GITHUB_OUTPUT = outFile; + process.env.GITHUB_ENV = envFile; + exportImage('ghcr.io/qwenlm/qwen-code:1.2.3'); + assert.equal( + readFileSync(outFile, 'utf8'), + 'image=ghcr.io/qwenlm/qwen-code:1.2.3\n', + ); + assert.equal( + readFileSync(envFile, 'utf8'), + 'QWEN_SANDBOX_IMAGE=ghcr.io/qwenlm/qwen-code:1.2.3\n', + ); + } finally { + if (saved.out === undefined) delete process.env.GITHUB_OUTPUT; + else process.env.GITHUB_OUTPUT = saved.out; + if (saved.env === undefined) delete process.env.GITHUB_ENV; + else process.env.GITHUB_ENV = saved.env; + rmSync(dir, { recursive: true, force: true }); + } +}); + +// async + `return await`: a bare `return fn(stub)` would run the `finally` +// unlink BEFORE the async body's promise settles, racing the spawned child's +// script-open — the parent wins often enough to flake the success path with +// a misleading 'no repository digest' error (probe: 23/30 loops failed). +async function withDockerStub(scriptBody, fn) { + const dir = mkdtempSync(join(tmpdir(), 'sandbox-image-stub-')); + const stub = join(dir, 'docker-stub'); + try { + writeFileSync(stub, `#!/bin/sh\n${scriptBody}\n`, { mode: 0o755 }); + chmodSync(stub, 0o755); + return await fn(stub); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +// The resolver's invocation contract shared by the e2e pins below — arg +// passing plus the three env names the export depends on. One copy, so a +// contract change cannot update one test and leave the other green (#9527 +// review). +function runResolver(stub) { + const envFile = join(dirname(stub), 'env'); + const outFile = join(dirname(stub), 'out'); + const scriptPath = fileURLToPath( + new URL('./resolve-sandbox-image.mjs', import.meta.url), + ); + execFileSync( + process.execPath, + [scriptPath, 'ghcr.io/qwenlm/qwen-code:1.2.3'], + { + env: { + ...process.env, + SANDBOX_COMMAND: stub, + GITHUB_ENV: envFile, + GITHUB_OUTPUT: outFile, + }, + timeout: 15_000, + stdio: 'pipe', + }, + ); + return { envFile, outFile }; +} + +test('repoDigestOf resolves a pulled image to its content digest', async () => { + await withDockerStub( + "printf '%s\\n' '[\"ghcr.io/qwenlm/qwen-code@sha256:0123456789abcdef\"]'", + async (stub) => { + // The exported reference must be pinned by CONTENT: `docker tag` and + // `docker build` cannot move a digest reference, while the tag the + // image was pulled under can be retagged by any co-resident process + // with daemon access before the gate runs. + assert.equal( + await repoDigestOf( + stub, + 'ghcr.io/qwenlm/qwen-code:1.2.3', + 'sha256:0123456789abcdef', + ), + 'ghcr.io/qwenlm/qwen-code@sha256:0123456789abcdef', + ); + }, + ); +}); + +test('withDockerStub keeps the stub alive until the async body settles', async () => { + // One success-path call per process hides the unlink race above, so drive + // the spawn→open window in a loop. + for (let i = 0; i < 30; i++) { + await withDockerStub( + "printf '%s\\n' '[\"ghcr.io/qwenlm/qwen-code@sha256:0123456789abcdef\"]'", + async (stub) => { + assert.equal( + await repoDigestOf( + stub, + 'ghcr.io/qwenlm/qwen-code:1.2.3', + 'sha256:0123456789abcdef', + ), + 'ghcr.io/qwenlm/qwen-code@sha256:0123456789abcdef', + ); + }, + ); + } +}); + +test('repoDigestOf refuses an image without a repository digest', async () => { + // A locally built image has no RepoDigests — `{{json .RepoDigests}}` + // renders `null`, older daemons print ``; exporting the mutable + // tag in either state is exactly what the pin exists to block. + for (const shape of ['null', '', '[]']) { + await withDockerStub(`printf "%s\\n" "${shape}"`, async (stub) => { + await assert.rejects( + repoDigestOf(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3', GENUINE), + /no repository digest/, + ); + }); + } +}); + +test('repoDigestOf fails closed when the inspect fails', async () => { + await withDockerStub('exit 1', async (stub) => { + await assert.rejects( + repoDigestOf(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3', GENUINE), + /no repository digest/, + ); + }); +}); + +// The exported reference is bound to the digest the PULL itself reported: +// `docker tag` never rewrites digests, so retagged attacker content keeps +// its original repo in RepoDigests (measured live: a tag moved to other +// content resolves to `busybox@sha256:…` and passes the `@sha256:` presence +// check). Only the pulled repo + the pull's own Digest line together tie +// the export to the fetched content (#9214 review). +const GENUINE = + 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef'; + +test('repoDigestOf refuses content whose repo is not the pulled image', async () => { + await withDockerStub( + "printf '%s\\n' '[\"aaa.example/backdoor@sha256:dc2d74b2dc2d74b2dc2d74b2dc2d74b2dc2d74b2dc2d74b2dc2d74b2dc2d74b2\"]'", + async (stub) => { + await assert.rejects( + repoDigestOf(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3', GENUINE), + /none of which is/, + ); + }, + ); +}); + +test('repoDigestOf accepts the digest the pull reported', async () => { + await withDockerStub( + `printf '%s\\n' '["ghcr.io/qwenlm/qwen-code@${GENUINE}"]'`, + async (stub) => { + assert.equal( + await repoDigestOf(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3', GENUINE), + `ghcr.io/qwenlm/qwen-code@${GENUINE}`, + ); + }, + ); +}); + +test('repoDigestOf keeps the pulled repo when a same-content tag sorts first', async () => { + // `docker tag` of the SAME content adds an alphabetically-sorted + // RepoDigests entry for the new name: index 0 moves off the pulled repo + // while a suffix-only digest check still passes (docker 29.1.3 probe: + // after `docker tag a/a:1`, RepoDigests[0] is `a/a@sha256:…`). + // The resolver must export the `@` entry, not index 0 — + // every gate consumer's shape regex refuses a foreign repo, so exporting + // index 0 gate-crashes the autofix loop until a manual `docker rmi`. + await withDockerStub( + `printf '%s\\n' '["a/a@${GENUINE}","ghcr.io/qwenlm/qwen-code@${GENUINE}"]'`, + async (stub) => { + assert.equal( + await repoDigestOf(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3', GENUINE), + `ghcr.io/qwenlm/qwen-code@${GENUINE}`, + ); + }, + ); +}); + +test('repoDigestOf matches Docker Hub canonical short-form RepoDigests', async () => { + // docker records Hub repos in canonical short form — a pull of + // docker.io/library/busybox:stable stores RepoDigests as busybox@sha256:… + // (measured live, docker 24.0.9) — so the match must fold the same + // prefixes or a fully-qualified Hub reference fails closed on its own + // correct digest. + for (const requested of [ + 'docker.io/library/busybox:stable', + 'docker.io/busybox:stable', + ]) { + await withDockerStub( + `printf '%s\\n' '["busybox@${GENUINE}"]'`, + async (stub) => { + assert.equal( + await repoDigestOf(stub, requested, GENUINE), + `busybox@${GENUINE}`, + ); + }, + ); + } +}); + +test('repoOfImage strips tag and digest but keeps a registry port', () => { + assert.equal( + repoOfImage('ghcr.io/qwenlm/qwen-code:1.2.3'), + 'ghcr.io/qwenlm/qwen-code', + ); + assert.equal( + repoOfImage('ghcr.io/qwenlm/qwen-code@sha256:ab'), + 'ghcr.io/qwenlm/qwen-code', + ); + assert.equal(repoOfImage('registry:5000/img:tag'), 'registry:5000/img'); +}); + +test('parsePullDigest extracts the Digest line from pull output', () => { + const pullLog = [ + '1.2.3: Pulling from qwenlm/qwen-code', + `Digest: ${GENUINE}`, + 'Status: Image is up to date for ghcr.io/qwenlm/qwen-code:1.2.3', + 'ghcr.io/qwenlm/qwen-code:1.2.3', + ].join('\n'); + assert.equal(parsePullDigest(pullLog), GENUINE); + assert.equal(parsePullDigest('Status: Image is up to date'), ''); + assert.equal(parsePullDigest('Digest: sha256:tooshort'), ''); +}); + +test('pullImage captures the pull-reported digest on success', async () => { + await withDockerStub( + `printf "%s\\n" "pulling..." "Digest: ${GENUINE}" "Status: Downloaded"`, + async (stub) => { + const result = await pullImage(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3'); + assert.deepEqual(result, { ok: true, digest: GENUINE }); + }, + ); +}); + +test('pullImage reports failure without a digest', async () => { + await withDockerStub('exit 1', async (stub) => { + const result = await pullImage(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3'); + assert.deepEqual(result, { ok: false, digest: '' }); + }); +}); + +test('sandboxSpawnEnv pins the daemon endpoint against both override routes', () => { + // DOCKER_HOST outranks everything and DOCKER_CONTEXT='' falls through to + // `currentContext` in the pool-shared config.json, so clearing is not + // enough: the context must be NAMED and DOCKER_HOST must be gone. + const env = sandboxSpawnEnv({ + PATH: '/usr/bin', + DOCKER_HOST: 'tcp://attacker.invalid:2375', + DOCKER_CONTEXT: '', + }); + assert.equal(env.DOCKER_CONTEXT, 'default'); + assert.ok(!('DOCKER_HOST' in env), 'DOCKER_HOST must not reach the child'); + assert.equal(env.PATH, '/usr/bin'); + // A hostile context NAME must not survive either. + assert.equal( + sandboxSpawnEnv({ DOCKER_CONTEXT: 'rogue' }).DOCKER_CONTEXT, + 'default', + ); + // process.env is the default source and must not be mutated. + process.env.DOCKER_HOST = 'tcp://attacker.invalid:2375'; + try { + assert.ok(!('DOCKER_HOST' in sandboxSpawnEnv())); + assert.equal(process.env.DOCKER_HOST, 'tcp://attacker.invalid:2375'); + } finally { + delete process.env.DOCKER_HOST; + } +}); + +test('the inspect runs against the pinned endpoint too', async () => { + // The env pin has to be on BOTH spawns: an inspect answered by a daemon + // someone else controls hands back any digest it likes, and the pull's own + // Digest line is then compared against attacker-chosen RepoDigests. + process.env.DOCKER_HOST = 'tcp://attacker.invalid:2375'; + process.env.DOCKER_CONTEXT = 'rogue'; + const dir = mkdtempSync(join(tmpdir(), 'sandbox-image-inspect-env-')); + const sink = join(dir, 'env.txt'); + try { + await withDockerStub( + `printf "%s\\n" "HOST=[\${DOCKER_HOST-unset}]" "CTX=[\${DOCKER_CONTEXT-unset}]" > ${sink}\n` + + `printf '%s\\n' '["ghcr.io/qwenlm/qwen-code@sha256:0123456789abcdef"]'`, + async (stub) => { + assert.equal( + await repoDigestOf( + stub, + 'ghcr.io/qwenlm/qwen-code:1.2.3', + 'sha256:0123456789abcdef', + ), + 'ghcr.io/qwenlm/qwen-code@sha256:0123456789abcdef', + ); + }, + ); + assert.equal(readFileSync(sink, 'utf8'), 'HOST=[unset]\nCTX=[default]\n'); + } finally { + rmSync(dir, { recursive: true, force: true }); + delete process.env.DOCKER_HOST; + delete process.env.DOCKER_CONTEXT; + } +}); + +test('the pull runs against the pinned endpoint, not the parent values', async () => { + process.env.DOCKER_HOST = 'tcp://attacker.invalid:2375'; + process.env.DOCKER_CONTEXT = 'rogue'; + const dir = mkdtempSync(join(tmpdir(), 'sandbox-image-env-')); + const sink = join(dir, 'env.txt'); + try { + await withDockerStub( + `printf "%s\\n" "HOST=[\${DOCKER_HOST-unset}]" "CTX=[\${DOCKER_CONTEXT-unset}]" > ${sink}`, + async (stub) => { + await pullImage(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3'); + }, + ); + assert.equal(readFileSync(sink, 'utf8'), 'HOST=[unset]\nCTX=[default]\n'); + } finally { + rmSync(dir, { recursive: true, force: true }); + delete process.env.DOCKER_HOST; + delete process.env.DOCKER_CONTEXT; + } +}); + +test('repoDigestOf gives up on an inspect that never returns', async () => { + // The timeout is the only thing standing between a wedged daemon and the + // step timeout; without an injectable bound a timer mutant ships green. + const started = Date.now(); + await withDockerStub('exec sleep 30', async (stub) => { + await assert.rejects( + repoDigestOf(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3', GENUINE, 200), + /no repository digest/, + ); + }); + assert.ok( + Date.now() - started < 10_000, + 'the inspect timeout must fire well before the step timeout', + ); +}); + +test('pullImage accumulates stdout across chunk boundaries', async () => { + // The Digest line is deliberately split across two writes: a reader that + // keeps only the latest chunk parses no digest here, while the real pull + // output (progress lines, then the digest) hides that mutant entirely. + await withDockerStub( + `printf "%s" "1.2.3: Pulling from qwenlm/qwen-code\nDigest: "; sleep 0.2; printf "%s\\n" "${'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef'}" "Status: Downloaded"`, + async (stub) => { + const result = await pullImage(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3'); + assert.deepEqual(result, { + ok: true, + digest: + 'sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef', + }); + }, + ); +}); + +test('pullImage gives up on a pull that never returns', async () => { + // Mirrors the repoDigestOf timeout pin: without an injectable bound a + // wedged registry holds the step until the job timeout instead of + // failing closed and taking the fallback path. + const started = Date.now(); + await withDockerStub('exec sleep 30', async (stub) => { + assert.deepEqual( + await pullImage(stub, 'ghcr.io/qwenlm/qwen-code:1.2.3', 200), + { ok: false, digest: '' }, + ); + }); + assert.ok( + Date.now() - started < 10_000, + 'the pull timeout must fire well before the step timeout', + ); +}); + +test('appendStepFile appends to a regular file', () => { + const dir = mkdtempSync(join(tmpdir(), 'sandbox-image-append-')); + const file = join(dir, 'out.txt'); + try { + appendStepFile(file, 'a=1\n'); + appendStepFile(file, 'b=2\n'); + assert.equal(readFileSync(file, 'utf8'), 'a=1\nb=2\n'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('appendStepFile refuses a planted FIFO instead of blocking on it', () => { + // $GITHUB_OUTPUT lives under the runner-writable temp tree. A FIFO planted + // at that path with no reader blocks a plain append until the step timeout; + // the non-blocking open turns that into an immediate refusal. + const dir = mkdtempSync(join(tmpdir(), 'sandbox-image-fifo-')); + const fifo = join(dir, 'github_output'); + try { + execFileSync('mkfifo', [fifo]); + } catch { + rmSync(dir, { recursive: true, force: true }); + return; // no mkfifo on this platform — nothing to assert + } + // Drive the append from a child process: a regression to a blocking open + // would wedge this suite exactly as it wedges the step, and a hung test + // reports far later and far less clearly than a failed one. `killed` is + // what separates the two outcomes — the non-blocking open fails on its own + // with ENXIO, while a blocking open only ever dies from the timeout here. + const moduleUrl = new URL('./resolve-sandbox-image.mjs', import.meta.url) + .href; + const source = `const m = await import(${JSON.stringify(moduleUrl)}); m.appendStepFile(${JSON.stringify(fifo)}, 'image=x\\n');`; + let error; + try { + execFileSync(process.execPath, ['--input-type=module', '-e', source], { + timeout: 5_000, + stdio: 'pipe', + }); + } catch (thrown) { + error = thrown; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + assert.ok(error, 'appending to a FIFO must fail, not succeed'); + assert.notEqual( + error.killed, + true, + 'the refusal must be immediate, not a block until the timeout', + ); + // A blocking open dies with no output at all, so the refusal has to be + // visible in the child's own words for this to mean anything. + assert.match(String(error.stderr), /ENXIO|not a regular file/); +}); + +test('appendStepFile refuses a FIFO whose reader is held open', () => { + // With a reader already holding the FIFO open, the write-side open + // SUCCEEDS and only the post-open type check refuses the write — the + // exact guard the no-reader test never reaches (that one dies in + // openSync with ENXIO first). Without the check the `image=` line is + // swallowed by the attacker's reader and the gate sees an empty image. + const dir = mkdtempSync(join(tmpdir(), 'sandbox-image-fifo-reader-')); + const fifo = join(dir, 'github_output'); + let reader; + try { + try { + execFileSync('mkfifo', [fifo]); + } catch { + return; // no mkfifo on this platform — nothing to assert + } + reader = openSync(fifo, constants.O_RDONLY | constants.O_NONBLOCK); + assert.throws( + () => appendStepFile(fifo, 'image=x\n'), + /not a regular file/, + ); + } finally { + if (reader !== undefined) closeSync(reader); + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('appendStepFile refuses a directory at the step-file path', () => { + const dir = mkdtempSync(join(tmpdir(), 'sandbox-image-dir-')); + try { + assert.throws(() => appendStepFile(dir, 'image=x\n')); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('the resolver refuses to export when the pull reports no Digest line', async () => { + // End-to-end pin of main()'s headline refusal: pull exits 0 without a + // Digest line while inspect happily reports a same-repo digest — the shape + // an attacker-retagged image presents. The resolver must exit non-zero and + // leave BOTH step files untouched; deleting the refusal exports the + // unbound content (mutant probe in the #9527 review). + await withDockerStub( + [ + 'if [ "$1" = "pull" ]; then', + " printf '%s\\n' 'Status: Downloaded newer image'", + ' exit 0', + 'fi', + `printf '%s\\n' '["ghcr.io/qwenlm/qwen-code@${GENUINE}"]'`, + ].join('\n'), + (stub) => { + const envFile = join(dirname(stub), 'env'); + const outFile = join(dirname(stub), 'out'); + let error; + try { + runResolver(stub); + } catch (thrown) { + error = thrown; + } + assert.ok(error, 'the resolver must exit non-zero'); + assert.match(String(error.stderr), /reported no Digest line/); + // The untouched-file checks must run BEFORE the helper's cleanup + // deletes the dir — after it they can never see what the resolver + // wrote (#9527 review). + assert.equal( + existsSync(envFile), + false, + 'GITHUB_ENV must stay untouched', + ); + assert.equal( + existsSync(outFile), + false, + 'GITHUB_OUTPUT must stay untouched', + ); + }, + ); +}); + +test('the resolver exports the digest-bound reference on the success path', async () => { + // Success-path companion to the refusal pin above: with a genuine Digest + // line and matching RepoDigests, BOTH step files must carry the + // `@` reference, never the mutable tag. A regression to + // exporting the requested tag ships the exact vulnerability class this + // PR closes and must fail here — the suite stayed green for that mutant + // until this test existed (#9527 review). + await withDockerStub( + [ + 'if [ "$1" = "pull" ]; then', + ` printf '%s\\n' 'Status: Downloaded newer image' 'Digest: ${GENUINE}'`, + ' exit 0', + 'fi', + `printf '%s\\n' '["ghcr.io/qwenlm/qwen-code@${GENUINE}"]'`, + ].join('\n'), + (stub) => { + const expected = `ghcr.io/qwenlm/qwen-code@${GENUINE}`; + const { envFile, outFile } = runResolver(stub); + assert.equal( + readFileSync(envFile, 'utf8'), + `QWEN_SANDBOX_IMAGE=${expected}\n`, + ); + assert.equal(readFileSync(outFile, 'utf8'), `image=${expected}\n`); + }, + ); +}); + +function checkWorkflowSandboxBindings(name, doc) { + let totalConsumers = 0; + for (const [jobName, job] of Object.entries(doc.jobs ?? {})) { + const steps = job.steps ?? []; + const consumers = steps.filter( + (step) => + (typeof step.run === 'string' && + step.run.includes('QWEN_SANDBOX_IMAGE')) || + (typeof step.env?.SETTINGS_JSON === 'string' && + step.env.SETTINGS_JSON.includes('"sandbox": "docker"')), + ); + if (consumers.length === 0) continue; + totalConsumers += consumers.length; + const resolvers = steps.filter( + (step) => + typeof step.run === 'string' && + step.run.includes('resolve-sandbox-image.mjs'), + ); + assert.ok( + resolvers.length > 0, + `${name} job '${jobName}': consumes the sandbox image but has no resolver step`, + ); + for (const resolver of resolvers) { + assert.ok( + resolver.id, + `${name} job '${jobName}': the resolver step needs an id so its image output is addressable`, + ); + // The export is derived from this binary's stdout: an unpinned + // SANDBOX_COMMAND (or the bare `docker` default) is steerable + // through the same $GITHUB_ENV-append channel the DOCKER_HOST pin + // closes, and a PATH shadow in the runner-writable qwen-bin dir + // defeats a bare-name pin — so step env must bind an absolute + // path (#9527 review). + assert.equal( + resolver.env?.SANDBOX_COMMAND, + '/usr/bin/docker', + `${name} job '${jobName}': pin SANDBOX_COMMAND to an absolute docker path so neither an appended $GITHUB_ENV value nor a $GITHUB_PATH shadow can steer the exported digest`, + ); + } + const bindings = resolvers.map( + (resolver) => `\${{ steps.${resolver.id}.outputs.image }}`, + ); + for (const step of consumers) { + assert.ok( + bindings.includes(step.env?.QWEN_SANDBOX_IMAGE), + `${name} job '${jobName}' step '${step.name}': bind QWEN_SANDBOX_IMAGE to the resolver step output, not the appendable $GITHUB_ENV value`, + ); + // The digest-bound reference is only as strong as the daemon that + // resolves it: a DOCKER_HOST appended to $GITHUB_ENV (or a rewritten + // currentContext in the pool-shared $DOCKER_CONFIG/config.json) + // steers the sandbox this step spawns unless the step env pins the + // endpoint the same way the resolver's own spawns do (#9527 review). + assert.equal( + step.env?.DOCKER_HOST, + '', + `${name} job '${jobName}' step '${step.name}': set DOCKER_HOST to '' so step env outranks an appended value (the docker CLI skips an empty value)`, + ); + assert.equal( + step.env?.DOCKER_CONTEXT, + 'default', + `${name} job '${jobName}' step '${step.name}': pin DOCKER_CONTEXT so the pool-shared currentContext cannot steer the docker endpoint`, + ); + // always() and failure() can both run a consumer when the resolver + // FAILED, and its image binding is then empty — an empty + // QWEN_SANDBOX_IMAGE relaunches the CLI without any sandbox, so such + // a consumer must also gate on the resolver's outcome to fail closed + // (#9527 review). + if (/always\(\)|failure\(\)/.test(String(step.if ?? ''))) { + assert.ok( + resolvers.some((resolver) => + String(step.if).includes( + `steps.${resolver.id}.outcome == 'success'`, + ), + ), + `${name} job '${jobName}' step '${step.name}': a consumer that can run after a resolver failure must also gate on the resolver step outcome`, + ); + } + } + } + assert.ok( + totalConsumers > 0, + `${name}: no sandbox consumers detected — the contract test would pass vacuously`, + ); +} + +// Workflow contract: the resolver's step OUTPUT is the value every agent and +// gate must consume — $GITHUB_ENV is appendable by later steps, so a consumer +// that inherits QWEN_SANDBOX_IMAGE from it can be steered after resolution. +// Every 'Resolve sandbox image' step must therefore carry an id, and every +// sandbox-consuming step in the job must bind that step's image output. +// The protected set is DERIVED from the tree — every workflow with a step +// that runs the resolver — so a new resolver step cannot land untested +// (#9527 review). +test('every sandbox-image consumer binds the resolver step output', () => { + const workflowsDir = join( + dirname(fileURLToPath(import.meta.url)), + '..', + 'workflows', + ); + const workflows = readdirSync(workflowsDir) + .filter((name) => name.endsWith('.yml') || name.endsWith('.yaml')) + .map((name) => ({ + name, + doc: parse(readFileSync(join(workflowsDir, name), 'utf8')), + })) + .filter(({ doc }) => + Object.values(doc?.jobs ?? {}).some((job) => + (job.steps ?? []).some( + (step) => + typeof step.run === 'string' && + step.run.includes('resolve-sandbox-image.mjs'), + ), + ), + ); + // repo-hygiene.yml runs the same resolver/consumer shape and needs the + // same binding, but it is outside this PR's footprint and the gate rejects + // the change here; it is tracked in the deferred review findings queue + // (#9527). Remove it from this set once that binding lands. + const UNBOUND_WORKFLOWS = ['repo-hygiene.yml']; + for (const name of UNBOUND_WORKFLOWS) { + assert.ok( + workflows.some((workflow) => workflow.name === name), + `${name} no longer runs the resolver — drop it from UNBOUND_WORKFLOWS`, + ); + } + + for (const { name, doc } of workflows) { + const exempt = UNBOUND_WORKFLOWS.includes(name); + let checkError; + try { + checkWorkflowSandboxBindings(name, doc); + } catch (thrown) { + checkError = thrown; + } + if (exempt) { + // Inverted tripwire: the exemption exists ONLY while the binding + // is absent, so the same check must fail once the workflow binds + // the resolver output — a stale entry would silently re-open the + // hole the derived set closes (#9527 review). + assert.ok( + checkError, + `${name} now binds the resolver output — drop it from UNBOUND_WORKFLOWS`, + ); + } else if (checkError) { + throw checkError; + } + } +}); diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index 2a4aeb8e0c1..18ea66cd1d2 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -1096,8 +1096,16 @@ jobs: echo "has_candidates=$([[ "${COUNT}" -gt 0 ]] && echo true || echo false)" >> "${GITHUB_OUTPUT}" - name: 'Resolve sandbox image' + id: 'sandbox_image' if: |- ${{ steps.scan.outputs.has_candidates == 'true' }} + env: + # The export is derived from this binary's stdout, so step env + # pins it the way the DOCKER_HOST pins do: step env outranks an + # appended $GITHUB_ENV value, and the absolute path cannot be + # shadowed by a '${RUNNER_TEMP}/qwen-bin' or $GITHUB_PATH plant + # (#9527 review). + SANDBOX_COMMAND: '/usr/bin/docker' run: |- node .github/scripts/resolve-sandbox-image.mjs \ "$(node -p "require('./package.json').config.sandboxImageUri")" @@ -1134,6 +1142,15 @@ jobs: if: |- ${{ steps.scan.outputs.has_candidates == 'true' && steps.fasttrack.outputs.fast_tracked != 'true' }} env: + # Step-level env outranks $GITHUB_ENV: bind the resolver's pinned + # digest output, not the appendable env value it also writes. + QWEN_SANDBOX_IMAGE: '${{ steps.sandbox_image.outputs.image }}' + # Pin the daemon endpoint for the sandbox this step spawns, the + # same way the resolver pins its own spawns: an empty DOCKER_HOST + # is skipped by the docker CLI, and step env outranks both the + # appendable $GITHUB_ENV and the pool-shared currentContext. + DOCKER_HOST: '' + DOCKER_CONTEXT: 'default' GITHUB_TOKEN: '${{ secrets.CI_DEV_BOT_PAT }}' OPENAI_API_KEY: '${{ secrets.AUTOFIX_OPENAI_API_KEY }}' OPENAI_BASE_URL: '${{ secrets.AUTOFIX_OPENAI_BASE_URL || secrets.OPENAI_BASE_URL }}' @@ -1293,6 +1310,9 @@ jobs: if: |- ${{ steps.decision.outputs.go_issue != '' }} env: + QWEN_SANDBOX_IMAGE: '${{ steps.sandbox_image.outputs.image }}' + DOCKER_HOST: '' + DOCKER_CONTEXT: 'default' ISSUE: '${{ steps.decision.outputs.go_issue }}' OPENAI_API_KEY: '${{ secrets.AUTOFIX_OPENAI_API_KEY }}' OPENAI_BASE_URL: '${{ secrets.AUTOFIX_OPENAI_BASE_URL || secrets.OPENAI_BASE_URL }}' @@ -3912,6 +3932,14 @@ jobs: qwen --version - name: 'Resolve sandbox image' + id: 'sandbox_image' + env: + # The export is derived from this binary's stdout, so step env + # pins it the way the DOCKER_HOST pins do: step env outranks an + # appended $GITHUB_ENV value, and the absolute path cannot be + # shadowed by a '${RUNNER_TEMP}/qwen-bin' or $GITHUB_PATH plant + # (#9527 review). + SANDBOX_COMMAND: '/usr/bin/docker' run: |- node .github/scripts/resolve-sandbox-image.mjs \ "$(node -p "require('./package.json').config.sandboxImageUri")" @@ -4853,6 +4881,9 @@ jobs: # Full rationale → qwen-autofix.md#af-050 timeout-minutes: 130 env: + QWEN_SANDBOX_IMAGE: '${{ steps.sandbox_image.outputs.image }}' + DOCKER_HOST: '' + DOCKER_CONTEXT: 'default' PR: '${{ env.PR }}' ISSUE: '${{ env.ISSUE }}' OPENAI_API_KEY: '${{ secrets.AUTOFIX_OPENAI_API_KEY }}' @@ -4979,10 +5010,16 @@ jobs: - name: 'Repair deterministic rejection' id: 'repair' + # The outcome check fails closed: a FAILED resolver leaves the image + # binding below empty, and an empty QWEN_SANDBOX_IMAGE relaunches + # the CLI without any sandbox (#9527 review). if: |- - ${{ always() && steps.verify.outputs.retryable == 'true' }} + ${{ always() && steps.verify.outputs.retryable == 'true' && steps.sandbox_image.outcome == 'success' }} timeout-minutes: 20 env: + QWEN_SANDBOX_IMAGE: '${{ steps.sandbox_image.outputs.image }}' + DOCKER_HOST: '' + DOCKER_CONTEXT: 'default' PR: '${{ env.PR }}' ISSUE: '${{ env.ISSUE }}' OPENAI_API_KEY: '${{ secrets.AUTOFIX_OPENAI_API_KEY }}' diff --git a/packages/cli/src/utils/sandbox.test.ts b/packages/cli/src/utils/sandbox.test.ts index d0139d5029e..4116b8e9906 100644 --- a/packages/cli/src/utils/sandbox.test.ts +++ b/packages/cli/src/utils/sandbox.test.ts @@ -97,6 +97,51 @@ describe('start_sandbox', () => { child.emit('close', 0); await expect(result).resolves.toBe(0); }); + + it('checks image presence with an offline inspect that sees digest references', async () => { + vi.stubEnv('SANDBOX_SET_UID_GID', 'false'); + vi.spyOn(fs, 'existsSync').mockReturnValue(true); + vi.spyOn(fs, 'realpathSync').mockImplementation((filePath) => + String(filePath), + ); + execSyncMock.mockReturnValue(Buffer.from('')); + + const digestImage = + 'ghcr.io/qwenlm/qwen-code@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef'; + + const imageCheck = Object.assign(new EventEmitter(), { + stdout: new EventEmitter(), + }); + const child = new EventEmitter(); + spawnMock + .mockImplementationOnce(() => { + queueMicrotask(() => { + imageCheck.stdout.emit('data', Buffer.from('sha256:local')); + imageCheck.emit('close', 0); + }); + return imageCheck; + }) + .mockReturnValueOnce(child); + + const result = start_sandbox({ command: 'docker', image: digestImage }, []); + + await vi.waitFor(() => expect(spawnMock).toHaveBeenCalledTimes(2)); + // Digest references never appear in a `docker images -q ` listing + // even when the content is local, which forced a needless network pull + // (and a FatalSandboxError whenever the registry was unreachable) at + // every consumer startup; the presence check must be the offline + // `image inspect` (#9527 review). + expect(spawnMock.mock.calls[0]).toEqual([ + 'docker', + ['image', 'inspect', '--format', '{{.Id}}', digestImage], + ]); + // Content found locally: no pull is attempted — the next spawn is the + // sandbox run itself. + expect((spawnMock.mock.calls[1]?.[1] as string[])[0]).toBe('run'); + + child.emit('close', 0); + await expect(result).resolves.toBe(0); + }); }); describe('resolveSeatbeltProfileFile', () => { diff --git a/packages/cli/src/utils/sandbox.ts b/packages/cli/src/utils/sandbox.ts index 706ef1dec42..af91d8e2553 100644 --- a/packages/cli/src/utils/sandbox.ts +++ b/packages/cli/src/utils/sandbox.ts @@ -947,7 +947,12 @@ export async function start_sandbox( // Helper functions to ensure sandbox image is present async function imageExists(sandbox: string, image: string): Promise { return new Promise((resolve) => { - const args = ['images', '-q', image]; + // `images -q` lists repository:tag entries only, so a digest reference + // (`repo@sha256:…`) lists empty even when its content is local — forcing + // a needless registry round-trip for content already present. `image + // inspect` resolves digest references against local content offline + // (#9527). + const args = ['image', 'inspect', '--format', '{{.Id}}', image]; const checkProcess = spawn(sandbox, args); let stdoutData = ''; diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 44d883b9671..76f44b1e4b3 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -11302,18 +11302,22 @@ exit 1 'GHCR returned at least 1000 tags', ); expect(sandboxImageResolverScript).toContain('latestSemverTag(tags)'); + // The pull and the inspect share ONE spawn guard (#9527 review): these + // pin the shared helper's wiring — endpoint pin, timeout, start-failure + // and exit reporting — plus the pull's label, not per-callsite copies. expect(sandboxImageResolverScript).toContain( - "spawn(command, ['pull', image]", + "spawnDockerCapture(command, ['pull', image]", ); - expect(sandboxImageResolverScript).toContain('Timed out pulling ${image}'); + expect(sandboxImageResolverScript).toContain('env: sandboxSpawnEnv(),'); + expect(sandboxImageResolverScript).toContain('pulling ${image}'); expect(sandboxImageResolverScript).toContain( - '::error::Timed out pulling ${image}', + '::error::Timed out ${label} after ${timeoutMs / 1000}s.', ); expect(sandboxImageResolverScript).toContain( - "Failed to start '${command} pull ${image}'", + "::error::Failed to start '${command} ${args.join(' ')}': ${error.message}", ); expect(sandboxImageResolverScript).toContain( - "::error::'${command} pull ${image}' exited with code ${code}", + "::error::'${command} ${args.join(' ')}' exited with code ${code}.", ); expect(sandboxImageResolverScript).toContain( '::warning::Falling back from ${requestedImage} to latest GHCR semver ${fallbackImage}',