Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
216 changes: 196 additions & 20 deletions .github/scripts/resolve-sandbox-image.mjs
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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 <tag>` 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 `<repo>@<expectedDigest>` 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), `<no value>` 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`);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Make the pinned output the value the gates actually consume

None of the six Resolve sandbox image steps has an id, and there is no steps.<id>.outputs.image consumer anywhere in this commit, so this protected output is not addressable downstream. The agents and direct Docker verification still inherit QWEN_SANDBOX_IMAGE from appendable GITHUB_ENV; in a process-level reproduction, appending a second mutable value after resolution replaced the effective image while this pinned output remained unused. Give every resolver step an ID and bind every agent/gate image input directly to its image output; otherwise the pull-to-digest binding does not prevent post-resolution steering.

中文说明

六个 Resolve sandbox image 步骤均未设置 id,当前提交中也没有任何 steps.<id>.outputs.image 消费者,因此这个受保护的输出无法被下游引用。Agent 和直接 Docker 验证仍从可追加的 GITHUB_ENV 继承 QWEN_SANDBOX_IMAGE。进程级复现表明,在解析完成后追加第二个可变标签会覆盖实际生效镜像,而固定 digest 的 output 完全未被使用。应为所有解析步骤设置 ID,并让所有 agent/gate 直接绑定其 image 输出,否则该修复无法阻止解析后的镜像劫持。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split by footprint. In scope — resolved in da1d9ad44b: both Resolve sandbox image steps in qwen-autofix.yml and qwen-autofix-recovery.yml now carry id: 'sandbox_image', and every security-sensitive consumer in those jobs (Assess candidates, Develop fix, Triage and address, Repair deterministic rejection) binds QWEN_SANDBOX_IMAGE: '${{ steps.sandbox_image.outputs.image }}' at step level, which outranks the appendable GITHUB_ENV value; the workflow contract test pins all of it. Out of scope — deferred and escalated: the identical defect in repo-hygiene.yml (two resolver steps without ids, three env-inheriting consumers) is real, but that file is outside this PR's footprint and the deterministic gate rejected the previous commit precisely because it touched it — review feedback alone cannot authorize changes to CI machinery the PR never touched. The repo-hygiene change has been reverted byte-for-byte (01452e0358) and the binding is recorded in the deferred review findings queue so it survives merge. Open question for the maintainer: should the same id + step-level binding land in repo-hygiene.yml as a maintainer-owned change (direct commit or a small separate PR)? The contract test only needs repo-hygiene.yml added back to its workflow list once that lands.

中文说明

按足迹拆分处理。范围内 —— 已在 da1d9ad44b 中解决: qwen-autofix.yml 与 qwen-autofix-recovery.yml 中的两个 Resolve sandbox image 步骤均已带 id: 'sandbox_image',这些 job 中所有安全敏感的消费者(Assess candidates、Develop fix、Triage and address、Repair deterministic rejection)都在 step 级别绑定 QWEN_SANDBOX_IMAGE: '${{ steps.sandbox_image.outputs.image }}',step 级 env 优先于可追加的 GITHUB_ENV 值;工作流契约测试已将上述全部接线固化。范围外 —— 延后并升级: repo-hygiene.yml 中的同类缺陷(两个 resolver 步骤没有 id、三个消费者继承环境变量)确实存在,但该文件在本 PR 足迹之外,确定性门禁正是因为上一提交触碰了它而拒绝——仅凭审查反馈不能授权修改 PR 从未触碰的 CI 机制。repo-hygiene 的改动已逐字节回退(01452e0358),该绑定已记入延迟审查发现队列,确保合并后不丢失。给维护者的开放问题:同样的 id + step 级绑定是否应以维护者主导的改动(直接提交或单独的小 PR)落入 repo-hygiene.yml?届时契约测试只需把 repo-hygiene.yml 加回其工作流列表即可。

}
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;
}

Expand All @@ -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 (
Expand Down
Loading
Loading