diff --git a/.github/scripts/ci-runner-routing.test.mjs b/.github/scripts/ci-runner-routing.test.mjs index ac7db5b35e5..2cdcc2453ec 100644 --- a/.github/scripts/ci-runner-routing.test.mjs +++ b/.github/scripts/ci-runner-routing.test.mjs @@ -194,6 +194,6 @@ describe('serve-ab.yml runner routing', () => { ); assert.ok(wipe, 'self-hosted reuse must not bleed one PR into the next'); assert.equal(wipe.if, "${{ runner.environment == 'self-hosted' }}"); - assert.match(wipe.run, /find "\$GITHUB_WORKSPACE" -mindepth 1 -maxdepth 1 -exec rm -rf/); + assert.match(wipe.run, /find "\$WS" -mindepth 1 -maxdepth 1 -exec rm -rf/); }); }); diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index b867587bcba..6fb17ebf646 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2574,10 +2574,45 @@ jobs: WS="${GITHUB_WORKSPACE:?}" # Refuse to run anywhere unexpected: a wipe pointed at the wrong # path by a mangled env is far worse than a skipped wipe, and - # this job cannot proceed safely without it either way. + # this job cannot proceed safely without it either way. This is + # the guard from qwen-code-pr-review.yml's checkout heal (#9220), + # backported per #9265: measured on main, the bare denylist let + # non-canonical spellings of the guarded roots through (/home/, + # /home/., //usr, /root/, /var/ all reached the rm). + # Canonicalize before matching: the kernel resolves non-canonical + # spellings to the guarded roots (`/home/.` -> /home, `//usr` -> + # /usr), so a raw string match lets them slip past the case arms. + WS="$(realpath -m -- "$WS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${GITHUB_WORKSPACE}"; exit 1; } + # Trailing slashes slip past the exact-match case arms below + # (`/home/` would pass the guard and reach the rm); realpath strips + # them too; keep the guard whole if the path reaches this point with + # trailing slashes. + while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done + case "$WS" in + ..|../*|*/..|*/../*) echo "::error::refusing to wipe path containing '..': ${WS}"; exit 1 ;; + esac case "$WS" in /|/home|/root|/usr*|/etc*|/var|"") echo "::error::refusing to wipe suspicious workspace path: ${WS}"; exit 1 ;; esac + # A denylist can only enumerate known roots — the allowlist closes + # every other one (/tmp, /opt, ...): only a directory inside the + # runner workspace may be wiped. RUNNER_WORKSPACE is set in every + # step env, and for container steps the runner translates it — + # together with GITHUB_WORKSPACE — to the container path, so the + # allowlist holds inside this job's container as well. + RWS="${RUNNER_WORKSPACE:?}" + RWS="$(realpath -m -- "$RWS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${RUNNER_WORKSPACE}"; exit 1; } + # Mirror the WS strip before building the allowlist pattern; "/" + # stripped empty would match every path instead. + while [ "${RWS%/}" != "$RWS" ]; do RWS="${RWS%/}"; done + if [ -z "$RWS" ]; then echo "::error::refusing to wipe: runner workspace resolved to /"; exit 1; fi + case "$RWS" in + ..|../*|*/..|*/../*) echo "::error::refusing runner workspace path containing '..': ${RWS}"; exit 1 ;; + esac + case "$WS" in + "$RWS"/*) ;; + *) echo "::error::refusing to wipe workspace outside the runner workspace: ${WS} (runner workspace: ${RWS})"; exit 1 ;; + esac # Contents, not the directory itself: the runner owns the mount # point. Dotfiles included — a planted .npmrc or .git is exactly # what this removes. `find -exec rm -rf` rather than a glob so the @@ -3504,10 +3539,31 @@ jobs: if: "always() && needs.authorize.outputs.verify_trust == 'external'" run: |- set -uo pipefail + # Same guard as the pre-run wipe above (canonicalize, strip + # trailing slashes, denylist, RUNNER_WORKSPACE allowlist) — this + # copy predates the checkout-heal hardening and never received it + # (#9265). See that step's comments for what each layer catches; + # the suite pins this copy's behavior on its own. WS="${GITHUB_WORKSPACE:?}" + WS="$(realpath -m -- "$WS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${GITHUB_WORKSPACE}"; exit 1; } + while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done + case "$WS" in + ..|../*|*/..|*/../*) echo "::error::refusing to wipe path containing '..': ${WS}"; exit 1 ;; + esac case "$WS" in /|/home|/root|/usr*|/etc*|/var|"") echo "::error::refusing to wipe suspicious workspace path: ${WS}"; exit 1 ;; esac + RWS="${RUNNER_WORKSPACE:?}" + RWS="$(realpath -m -- "$RWS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${RUNNER_WORKSPACE}"; exit 1; } + while [ "${RWS%/}" != "$RWS" ]; do RWS="${RWS%/}"; done + if [ -z "$RWS" ]; then echo "::error::refusing to wipe: runner workspace resolved to /"; exit 1; fi + case "$RWS" in + ..|../*|*/..|*/../*) echo "::error::refusing runner workspace path containing '..': ${RWS}"; exit 1 ;; + esac + case "$WS" in + "$RWS"/*) ;; + *) echo "::error::refusing to wipe workspace outside the runner workspace: ${WS} (runner workspace: ${RWS})"; exit 1 ;; + esac find "$WS" -mindepth 1 -maxdepth 1 -exec rm -rf {} + 2>/dev/null || true echo "Workspace wiped after external code (deny-by-default cleanup for the next pool job)." >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/serve-ab.yml b/.github/workflows/serve-ab.yml index 591b2d89538..74a5d5c7c61 100644 --- a/.github/workflows/serve-ab.yml +++ b/.github/workflows/serve-ab.yml @@ -80,7 +80,49 @@ jobs: # could bleed into the builds and silently change the posted A/B # diff. Hosted runners are ephemeral and never see this. After the # ownership-restore step everything is user-owned, so no sudo. - find "$GITHUB_WORKSPACE" -mindepth 1 -maxdepth 1 -exec rm -rf {} + + # + # Guard, ported from qwen-code-pr-review.yml's checkout heal + # (#9220, #9265): this step had none — under a mangled env even + # `/home` or an empty string reached the rm. A wipe pointed at the + # wrong path is far worse than a skipped wipe, so canonicalize, + # strip trailing slashes, denylist the known roots, and require + # the target to sit inside the runner workspace before any rm. + WS="${GITHUB_WORKSPACE:?}" + # Canonicalize before matching: the kernel resolves non-canonical + # spellings to the guarded roots (`/home/.` -> /home, `//usr` -> + # /usr), so a raw string match lets them slip past the case arms. + WS="$(realpath -m -- "$WS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${GITHUB_WORKSPACE}"; exit 1; } + # Trailing slashes slip past the exact-match case arms below + # (`/home/` would pass the guard and reach the rm); realpath strips + # them too; keep the guard whole if the path reaches this point with + # trailing slashes. + while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done + case "$WS" in + ..|../*|*/..|*/../*) echo "::error::refusing to wipe path containing '..': ${WS}"; exit 1 ;; + esac + case "$WS" in + /|/home|/root|/usr*|/etc*|/var|"") echo "::error::refusing to wipe suspicious workspace path: ${WS}"; exit 1 ;; + esac + # A denylist can only enumerate known roots — the allowlist closes + # every other one (/tmp, /opt, ...): only a directory inside the + # runner workspace may be wiped. + RWS="${RUNNER_WORKSPACE:?}" + RWS="$(realpath -m -- "$RWS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${RUNNER_WORKSPACE}"; exit 1; } + # Mirror the WS strip before building the allowlist pattern; "/" + # stripped empty would match every path instead. + while [ "${RWS%/}" != "$RWS" ]; do RWS="${RWS%/}"; done + if [ -z "$RWS" ]; then echo "::error::refusing to wipe: runner workspace resolved to /"; exit 1; fi + case "$RWS" in + ..|../*|*/..|*/../*) echo "::error::refusing runner workspace path containing '..': ${RWS}"; exit 1 ;; + esac + case "$WS" in + "$RWS"/*) ;; + *) echo "::error::refusing to wipe workspace outside the runner workspace: ${WS} (runner workspace: ${RWS})"; exit 1 ;; + esac + # Bare on purpose: this job runs under `bash -eo pipefail`, so a + # wipe that cannot clear the workspace fails the job here instead + # of building both checkouts on top of the leftovers. + find "$WS" -mindepth 1 -maxdepth 1 -exec rm -rf {} + - name: 'Checkout PR head' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 9f121ad0191..65909f73af7 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6,16 +6,18 @@ import { execFileSync, spawn, spawnSync } from 'node:child_process'; import { + chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, + symlinkSync, writeFileSync, } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { basename, join } from 'node:path'; import { describe, expect, it } from 'vitest'; import { parse } from 'yaml'; @@ -29,6 +31,8 @@ const prSkill = readFileSync( 'utf8', ); const verifySkill = readFileSync('.qwen/skills/verify-pr/SKILL.md', 'utf8'); +const hasGnuRealpath = + spawnSync('realpath', ['-m', '--', '/'], { stdio: 'ignore' }).status === 0; function escapeRegExp(value) { return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); @@ -1612,47 +1616,71 @@ describe('qwen-triage verify hardening', () => { // Fails the job when the workspace is not actually empty afterwards. expect(wipe).toContain('refusing to execute external code on it'); expect(wipe).toContain('exit 1'); + // The ported guard layers (#9265) — see the post-run wipe test for + // what each pin catches. + expect(wipe).toContain('realpath -m'); + expect(wipe).toContain('realpath -m -- "$RWS"'); + expect(wipe).toContain('RUNNER_WORKSPACE:?'); + expect(wipe).toContain('"$RWS"/*'); + // RWS-side layers: the '..' arm and the degenerate-root refusal that + // keeps a stripped-empty runner workspace from degenerating the + // allowlist pattern to `/*`. + expect(wipe).toContain("refusing runner workspace path containing '..'"); + expect(wipe).toContain('runner workspace resolved to /'); }); // The wipe is the deny-by-default control, so run the real step text // against a workspace carrying the vectors the allowlist sweep is built // to enumerate — plus one it is not — and require every one to be gone. - it('removes planted persistence vectors, known and unknown', () => { - const wipe = stepIn('verify', 'Wipe workspace before external code') - .match(/run: \|-\n([\s\S]*)$/)?.[1] - .replace(/^ {10}/gm, ''); - expect(wipe).toBeTruthy(); - - const dir = mkdtempSync(join(tmpdir(), 'verify-wipe-')); - try { - const ws = join(dir, 'workspace'); - mkdirSync(join(ws, '.git', 'hooks'), { recursive: true }); - // Vectors the sweep enumerates... - writeFileSync(join(ws, '.git', 'hooks', 'pre-commit'), '#!/bin/sh\nid\n'); - writeFileSync( - join(ws, '.git', 'config.worktree'), - '[core]\n\thooksPath = /\n', - ); - // ...and ones it does not: a dotfile the next npm run would read, - // and an ordinary file. Deny-by-default has to take all of them. - writeFileSync(join(ws, '.npmrc'), 'script-shell=/tmp/evil\n'); - writeFileSync(join(ws, 'package.json'), '{}'); - mkdirSync(join(ws, 'node_modules'), { recursive: true }); + it.skipIf(!hasGnuRealpath)( + 'removes planted persistence vectors, known and unknown', + () => { + const wipe = stepIn('verify', 'Wipe workspace before external code') + .match(/run: \|-\n([\s\S]*)$/)?.[1] + .replace(/^ {10}/gm, ''); + expect(wipe).toBeTruthy(); - const res = spawnSync('bash', ['-c', wipe], { - encoding: 'utf8', - env: { - ...process.env, - GITHUB_WORKSPACE: ws, - GITHUB_STEP_SUMMARY: join(dir, 'summary'), - }, - }); - expect(res.status).toBe(0); - expect(readdirSync(ws)).toEqual([]); - } finally { - rmSync(dir, { recursive: true, force: true }); - } - }); + const dir = mkdtempSync(join(tmpdir(), 'verify-wipe-')); + try { + const ws = join(dir, 'workspace'); + mkdirSync(join(ws, '.git', 'hooks'), { recursive: true }); + // Vectors the sweep enumerates... + writeFileSync( + join(ws, '.git', 'hooks', 'pre-commit'), + '#!/bin/sh\nid\n', + ); + writeFileSync( + join(ws, '.git', 'config.worktree'), + '[core]\n\thooksPath = /\n', + ); + // ...and ones it does not: a dotfile the next npm run would read, + // and an ordinary file. Deny-by-default has to take all of them. + writeFileSync(join(ws, '.npmrc'), 'script-shell=/tmp/evil\n'); + writeFileSync(join(ws, 'package.json'), '{}'); + mkdirSync(join(ws, 'node_modules'), { recursive: true }); + + // GitHub Actions runs `shell: bash` steps with `-eo pipefail`, + // so the battery must too: bare `bash -c` masks a failing + // sweep command into a green test (probed with a failing `find` + // stub on PATH: exit 0 here, exit 1 under the real step flags). + const res = spawnSync('bash', ['-e', '-o', 'pipefail', '-c', wipe], { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + // The ported guard allowlists against the runner workspace + // (#9265): the test workspace must sit inside it. + RUNNER_WORKSPACE: dir, + GITHUB_STEP_SUMMARY: join(dir, 'summary'), + }, + }); + expect(res.status).toBe(0); + expect(readdirSync(ws)).toEqual([]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }, + ); // The guard has to be exercised with the REAL dangerous paths — that is // the whole point of it — but a test that passes `/` to a live `rm -rf` @@ -1687,7 +1715,31 @@ describe('qwen-triage verify hardening', () => { { mode: 0o755 }, ); - for (const bad of ['/', '/usr', '/etc', '/var', '/root', '/home', '']) { + // The second half are the spellings the ported guard layers exist + // for (#9265): the kernel resolves each of them to a guarded root, + // yet the bare denylist they faced before let every one through + // (measured on main in the issue). `/tmp` and `/opt` are refused by + // the allowlist alone — the denylist has no arm for them. + for (const bad of [ + '/', + '/usr', + '/etc', + '/var', + '/root', + '/home', + '', + '/home/', + '/root/', + '/var/', + '//', + '/home//', + '/home/.', + '/home/..', + '//usr', + '//home', + '/tmp', + '/opt', + ]) { writeFileSync(calls, ''); const guard = spawnSync('bash', ['-c', wipe], { encoding: 'utf8', @@ -1696,12 +1748,18 @@ describe('qwen-triage verify hardening', () => { PATH: `${dir}:${process.env.PATH}`, RM_CALLS: calls, GITHUB_WORKSPACE: bad, + // The recorder dir doubles as the allowlist root: every bad + // path sits outside it, so the refusal is the guard's, not a + // side effect of the fixture layout. + RUNNER_WORKSPACE: dir, GITHUB_STEP_SUMMARY: join(dir, 'summary'), }, }); - // Non-zero, not exactly 1: two mechanisms refuse here — the `case` - // exits 1 for a named path, while an empty one never reaches it - // because `${GITHUB_WORKSPACE:?}` aborts the shell first (127). + // Non-zero, not exactly 1: several mechanisms refuse here — the + // `case` exits 1 for a named root, an empty path never reaches it + // because `${GITHUB_WORKSPACE:?}` aborts the shell first (127), + // and the allowlist exits 1 for everything outside the runner + // workspace. Which one fires depends on the host's realpath. expect( guard.status, `path ${bad || ''} was not refused`, @@ -1717,6 +1775,294 @@ describe('qwen-triage verify hardening', () => { } }); + // The canonicalization layer needs its own pin: every bad path above + // sits OUTSIDE the recorder dir, so the allowlist refuses them + // identically whether `realpath -m` ran or not — deleting that line + // ships green against the battery. A raw '..' spelling does not pin it + // either: the '..' case arm refuses that vector first, mutant or not. + // A symlink INSIDE the runner workspace pointing outside is the + // spelling only the realpath line can catch: canonicalized, it lands + // outside and the allowlist refuses it; with the line deleted the raw + // link path matches "$RWS"/*, but find's default -P mode does not + // descend symlink operands, so the mutant exits 0 having wiped nothing + // — caught by the non-zero-status assertion below, not the rm recorder. + const extractRun = (stepName) => { + const run = stepIn('verify', stepName) + .match(/run: \|-\n([\s\S]*)$/)?.[1] + .replace(/^ {10}/gm, ''); + expect(run, `run block for ${stepName}`).toBeTruthy(); + return run; + }; + + it.skipIf(!hasGnuRealpath)( + 'refuses an allowlist-escaping symlink via canonicalization', + () => { + const dir = mkdtempSync(join(tmpdir(), 'verify-wipe-escape-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-wipe-outside-')); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, join(dir, 'link')); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + + for (const stepName of [ + 'Wipe workspace before external code', + 'Wipe workspace after external code', + ]) { + writeFileSync(calls, ''); + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + // A link inside the recorder dir whose target sits + // outside it — no '..' component, so only canonicalization + // can resolve the escape. + GITHUB_WORKSPACE: join(dir, 'link'), + RUNNER_WORKSPACE: dir, + GITHUB_STEP_SUMMARY: join(dir, 'summary'), + }, + }, + ); + expect(res.status, `${stepName} did not refuse`).not.toBe(0); + expect( + readFileSync(calls, 'utf8'), + `${stepName} invoked rm on the escaping path`, + ).toBe(''); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + // Fronts PATH with a failing realpath so the script must fail closed instead + // of matching and wiping a raw, potentially misleading spelling. + const stubRealpath = () => { + const bin = mkdtempSync(join(tmpdir(), 'verify-wipe-bin-')); + writeFileSync(join(bin, 'realpath'), '#!/bin/sh\nexit 1\n'); + chmodSync(join(bin, 'realpath'), 0o755); + return bin; + }; + + it('refuses to wipe when realpath is absent', () => { + const parent = mkdtempSync(join(tmpdir(), 'verify-wipe-rws-')); + const ws = join(parent, 'repo'); + mkdirSync(ws); + writeFileSync(join(ws, 'leftover'), 'x'); + const bin = stubRealpath(); + try { + // Both copies carry the fail-closed realpath leg; exercise each so + // a fail-open regression of either copy is caught. + for (const stepName of [ + 'Wipe workspace before external code', + 'Wipe workspace after external code', + ]) { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: `${parent}/`, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status, `${stepName} did not fail closed`).not.toBe(0); + expect(readdirSync(ws), `${stepName} wiped without realpath`).toEqual([ + 'leftover', + ]); + } + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(bin, { recursive: true, force: true }); + } + }); + + it('refuses a trailing-slash GITHUB_WORKSPACE when realpath is absent', () => { + const dir = mkdtempSync(join(tmpdir(), 'verify-wipe-ws-')); + const bin = stubRealpath(); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + + for (const stepName of [ + 'Wipe workspace before external code', + 'Wipe workspace after external code', + ]) { + writeFileSync(calls, ''); + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${bin}:${process.env.PATH}`, + GITHUB_WORKSPACE: '/home/', + RUNNER_WORKSPACE: '/home', + GITHUB_STEP_SUMMARY: join(dir, 'summary'), + }, + }, + ); + expect(res.status, `${stepName} did not refuse`).not.toBe(0); + expect(readFileSync(calls, 'utf8'), `${stepName} invoked rm`).toBe(''); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + rmSync(bin, { recursive: true, force: true }); + } + }); + + it('refuses an allowlist-escaping .. path when realpath is absent', () => { + const dir = mkdtempSync(join(tmpdir(), 'verify-wipe-fallback-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-wipe-outside-')); + const bin = stubRealpath(); + mkdirSync(join(dir, 'sub')); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + + for (const stepName of [ + 'Wipe workspace before external code', + 'Wipe workspace after external code', + ]) { + writeFileSync(calls, ''); + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${bin}:${process.env.PATH}`, + GITHUB_WORKSPACE: `${dir}/sub/../../${basename(outside)}`, + RUNNER_WORKSPACE: dir, + GITHUB_STEP_SUMMARY: join(dir, 'summary'), + }, + }, + ); + expect(res.status, `${stepName} did not refuse`).not.toBe(0); + expect( + readFileSync(calls, 'utf8'), + `${stepName} invoked rm on the escaping path`, + ).toBe(''); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + rmSync(bin, { recursive: true, force: true }); + } + }); + + // The degenerate-root arm keeps a stripped-empty RUNNER_WORKSPACE from + // turning the allowlist pattern into `/*` (which admits every absolute + // path). The reference suite covers the review workflow; each backported + // copy needs its own case — deleting the arm ships green otherwise. + it('refuses a runner workspace that resolves to / without invoking rm', () => { + const parent = mkdtempSync(join(tmpdir(), 'verify-wipe-root-')); + const ws = join(parent, 'repo'); + mkdirSync(ws); + writeFileSync(join(ws, 'leftover'), 'x'); + try { + const calls = join(parent, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(parent, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + + for (const stepName of [ + 'Wipe workspace before external code', + 'Wipe workspace after external code', + ]) { + writeFileSync(calls, ''); + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${parent}:${process.env.PATH}`, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: '/', + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status, `${stepName} did not refuse`).not.toBe(0); + expect(readFileSync(calls, 'utf8'), `${stepName} invoked rm`).toBe(''); + expect(readdirSync(ws), `${stepName} wiped`).toEqual(['leftover']); + } + } finally { + rmSync(parent, { recursive: true, force: true }); + } + }); + + // The RWS realpath line has no refusal of its own to observe, so pin it + // from the happy side: a RUNNER_WORKSPACE spelled with '..' that + // canonicalizes back to the real parent must still be allowed to wipe. + // Deleting the RWS realpath line leaves the raw spelling to the '..' + // arm, which refuses — and this test fails on that mutant. + it.skipIf(!hasGnuRealpath)( + 'canonicalizes a ..-spelled runner workspace instead of refusing it', + () => { + const parent = mkdtempSync(join(tmpdir(), 'verify-wipe-rwsdot-')); + const ws = join(parent, 'repo'); + mkdirSync(ws); + try { + for (const stepName of [ + 'Wipe workspace before external code', + 'Wipe workspace after external code', + ]) { + writeFileSync(join(ws, 'leftover'), 'x'); + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: join(ws, '..'), + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status, `${stepName} refused a canonical RWS`).toBe(0); + expect(readdirSync(ws), `${stepName} did not wipe`).toEqual([]); + } + } finally { + rmSync(parent, { recursive: true, force: true }); + } + }, + ); + // The pre-run wipe answers what an external run inherits; the // post-run wipe answers what it leaves behind for the next pool job. it('wipes the workspace after external code, on every outcome', () => { @@ -1735,8 +2081,119 @@ describe('qwen-triage verify hardening', () => { ); // Same path guard as the pre-run wipe. expect(postWipe).toContain('refusing to wipe suspicious workspace path'); + // The ported guard layers (#9265), pinned on THIS copy: canonicalize + // before matching, and allowlist the target inside the runner + // workspace. The exec tests below prove behavior; these text pins + // catch a mutation that deletes a layer from this copy alone. + expect(postWipe).toContain('realpath -m'); + expect(postWipe).toContain('realpath -m -- "$RWS"'); + expect(postWipe).toContain('RUNNER_WORKSPACE:?'); + expect(postWipe).toContain('"$RWS"/*'); + expect(postWipe).toContain( + "refusing runner workspace path containing '..'", + ); + expect(postWipe).toContain('runner workspace resolved to /'); }); + // The pre-run guard battery runs the post-run wipe too: it is a + // separate copy of the script, and the pre-run battery passing says + // nothing about mutations to this one. + it('refuses a suspicious workspace path in the post-run wipe without invoking rm', () => { + const wipe = extractRun('Wipe workspace after external code'); + + const dir = mkdtempSync(join(tmpdir(), 'verify-postwipe-guard-')); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + + for (const bad of [ + '/', + '/usr', + '/etc', + '/var', + '/root', + '/home', + '', + '/home/', + '/root/', + '/var/', + '//', + '/home//', + '/home/.', + '/home/..', + '//usr', + '//home', + '/tmp', + '/opt', + ]) { + writeFileSync(calls, ''); + const guard = spawnSync('bash', ['-e', '-o', 'pipefail', '-c', wipe], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GITHUB_WORKSPACE: bad, + RUNNER_WORKSPACE: dir, + GITHUB_STEP_SUMMARY: join(dir, 'summary'), + }, + }); + expect( + guard.status, + `path ${bad || ''} was not refused`, + ).not.toBe(0); + expect( + readFileSync(calls, 'utf8'), + `rm was invoked for ${bad || ''}`, + ).toBe(''); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + // The post-run wipe still has to wipe: its `if: always()` means it is + // the ONLY cleanup a cancelled or timed-out external run gets, so a + // guard regression that refuses the legitimate workspace would leak + // every aborted run's tree into the next pool job. + it.skipIf(!hasGnuRealpath)( + 'wipes a legitimate workspace in the post-run wipe and writes the summary', + () => { + const wipe = extractRun('Wipe workspace after external code'); + + const parent = mkdtempSync(join(tmpdir(), 'verify-postwipe-ok-')); + const ws = join(parent, 'workspace'); + mkdirSync(join(ws, '.git', 'hooks'), { recursive: true }); + writeFileSync( + join(ws, '.git', 'hooks', 'post-checkout'), + '#!/bin/sh\nid\n', + ); + writeFileSync(join(ws, 'leftover.o'), 'x'); + const summary = join(parent, 'summary'); + try { + const res = spawnSync('bash', ['-e', '-o', 'pipefail', '-c', wipe], { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: summary, + }, + }); + expect(res.status).toBe(0); + expect(readdirSync(ws)).toEqual([]); + expect(readFileSync(summary, 'utf8')).toContain( + 'Workspace wiped after external code', + ); + } finally { + rmSync(parent, { recursive: true, force: true }); + } + }, + ); + // The sponsored lane's pre-execution risk screen, driven for real: the // actual resolve-step text runs against a stubbed gh and a live local // HTTP server standing in for the model endpoint, so the heredoc's diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js new file mode 100644 index 00000000000..997cf70c81a --- /dev/null +++ b/scripts/tests/serve-ab-workflow.test.js @@ -0,0 +1,382 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { execFileSync, spawnSync } from 'node:child_process'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { basename, join } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { parse } from 'yaml'; + +const workflow = readFileSync('.github/workflows/serve-ab.yml', 'utf8'); + +const steps = parse(workflow).jobs['ab'].steps; +const WIPE = 'Wipe stale workspace before checkout'; +const wipe = steps.find((s) => s.name === WIPE); + +// Runs the real wipe script under the runner's shell flags: this job sets +// `defaults.run.shell: bash`, which GitHub Actions executes with +// `-eo pipefail`, so the exec tests must reproduce that instead of hiding +// it behind bare `bash -c`. +const runWipe = (env, options = {}) => + execFileSync('bash', ['-e', '-o', 'pipefail', '-c', wipe.run], { + encoding: 'utf8', + env: { ...process.env, ...env }, + ...options, + }); + +// `realpath -m` (the script's canonicalization line) is a GNU coreutils +// extension. Probe the host before asserting GNU-specific path behavior. +const hasGnuRealpath = + spawnSync('realpath', ['-m', '--', '/'], { stdio: 'ignore' }).status === 0; + +describe('serve-ab pre-checkout workspace wipe', () => { + it('runs the wipe before both checkouts', () => { + // Both checkouts clone into the wiped workspace; a wipe ordered after + // either one deletes what was just cloned, and whichever build runs + // first runs on the previous run's leftovers — the exact cross-PR + // bleed the step exists to prevent. The sister qwen-triage suite pins + // the same property. + const names = steps.map((stepItem) => stepItem.name); + const wipeAt = names.indexOf(WIPE); + expect(wipeAt).toBeGreaterThanOrEqual(0); + expect(wipeAt).toBeLessThan(names.indexOf('Checkout PR head')); + expect(wipeAt).toBeLessThan(names.indexOf('Checkout the merge-base')); + }); + + it('runs only on self-hosted runners, where workspace state persists', () => { + expect(wipe).toBeTruthy(); + // Hosted runners are ephemeral; the wipe (and its guard) exist for the + // reusable ECS pool only. + expect(wipe.if).toBe("${{ runner.environment == 'self-hosted' }}"); + }); + + it('carries the full checkout-heal guard (#9220, #9265)', () => { + // Before the port this step had NO guard: under a mangled env even + // `/home` or an empty string reached `find … -exec rm -rf {} +`. + // Pin each ported layer textually, mirroring the reference guard in + // qwen-code-pr-review.yml; the exec tests below prove the behavior. + expect(wipe.run).toContain('GITHUB_WORKSPACE:?'); + expect(wipe.run).toContain('realpath -m'); + expect(wipe.run).toContain('realpath -m -- "$RWS"'); + expect(wipe.run).toContain('refusing to wipe suspicious workspace path'); + expect(wipe.run).toContain('RUNNER_WORKSPACE:?'); + expect(wipe.run).toContain('"$RWS"/*'); + // RWS-side layers: the '..' arm and the degenerate-root refusal that + // keeps a stripped-empty runner workspace from degenerating the + // allowlist pattern to `/*`. + expect(wipe.run).toContain( + "refusing runner workspace path containing '..'", + ); + expect(wipe.run).toContain('runner workspace resolved to /'); + // Exit contract: the wipe stays bare on purpose — under the job's + // `-eo pipefail` a wipe that cannot clear the workspace fails the job + // here instead of building both checkouts on top of the leftovers. + // `|| true` would silently void that. + expect(wipe.run).not.toContain('|| true'); + }); + + it.skipIf(!hasGnuRealpath)( + 'wipes a legitimate workspace inside the runner workspace', + () => { + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-ok-')); + const ws = join(parent, 'repo'); + // Leftovers shaped like the real ones: the two checkout subtrees plus + // a stale build artifact. + mkdirSync(join(ws, 'head'), { recursive: true }); + mkdirSync(join(ws, 'base'), { recursive: true }); + writeFileSync(join(ws, 'head', 'package.json'), '{}'); + writeFileSync(join(ws, 'bundle.tgz'), 'x'); + try { + runWipe({ GITHUB_WORKSPACE: ws, RUNNER_WORKSPACE: parent }); + expect(readdirSync(ws)).toEqual([]); + // The directory itself survives: the checkouts clone into it next. + expect(wipe.run).toContain('-mindepth 1 -maxdepth 1'); + } finally { + rmSync(parent, { recursive: true, force: true }); + } + }, + ); + + // The guard must be exercised with the REAL dangerous paths, so `rm` is + // stubbed to a recorder on PATH: the destructive primitive cannot fire + // here under ANY edit, and the assertion is on the decision rather than + // on filesystem effects — with the guard gone the recorder shows an + // attempted delete and the test fails, having deleted nothing. + it('refuses suspicious workspace paths without invoking rm', () => { + const dir = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-guard-')); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + + // Canonical roots, the non-canonical spellings the canonicalize and + // strip layers exist for, and /tmp + /opt which only the allowlist + // refuses (the denylist has no arm for them). + for (const bad of [ + '/', + '/usr', + '/etc', + '/var', + '/root', + '/home', + '', + '/home/', + '/root/', + '/var/', + '//', + '/home//', + '/home/.', + '/home/..', + '//usr', + '//home', + '/tmp', + '/opt', + ]) { + writeFileSync(calls, ''); + const guard = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', wipe.run], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GITHUB_WORKSPACE: bad, + // The recorder dir doubles as the allowlist root: every bad + // path sits outside it, so the refusal is the guard's, not + // a side effect of the fixture layout. + RUNNER_WORKSPACE: dir, + }, + }, + ); + expect( + guard.status, + `path ${bad || ''} was not refused`, + ).not.toBe(0); + expect( + readFileSync(calls, 'utf8'), + `rm was invoked for ${bad || ''}`, + ).toBe(''); + } + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it.skipIf(!hasGnuRealpath)( + 'refuses an allowlist-escaping symlink via canonicalization', + () => { + // The bad paths above all sit outside the recorder dir, so the + // allowlist refuses them identically whether canonicalization runs + // or not — they cannot pin the realpath line. A raw '..' spelling + // does not pin it either: the '..' case arm refuses that vector + // first, mutant or not (executed mutant: exit 1 via 'refusing to + // wipe path containing ..', rm log empty). A symlink INSIDE the + // recorder dir pointing outside is the spelling only `realpath -m` + // can catch: with the line deleted the raw link path matches + // "$RWS"/*, but find's default -P mode does not descend symlink + // operands, so the mutant exits 0 having wiped nothing — caught by + // the non-zero-status assertion, not the rm recorder. + const dir = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-escape-')); + const outside = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-outside-')); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, join(dir, 'link')); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', wipe.run], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + // A link inside the recorder dir whose target sits outside + // it — no '..' component, so only canonicalization can + // resolve the escape. + GITHUB_WORKSPACE: join(dir, 'link'), + RUNNER_WORKSPACE: dir, + }, + }, + ); + expect(res.status).not.toBe(0); + expect(readFileSync(calls, 'utf8')).toBe(''); + } finally { + rmSync(dir, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + // Fronts PATH with a failing realpath so the script must fail closed instead + // of matching and wiping a raw, potentially misleading spelling. + const stubRealpath = () => { + const bin = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-bin-')); + writeFileSync(join(bin, 'realpath'), '#!/bin/sh\nexit 1\n'); + chmodSync(join(bin, 'realpath'), 0o755); + return bin; + }; + + it('refuses to wipe when realpath is absent', () => { + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-rws-')); + const ws = join(parent, 'repo'); + mkdirSync(ws); + writeFileSync(join(ws, 'leftover'), 'x'); + const bin = stubRealpath(); + try { + const res = spawnSync('bash', ['-e', '-o', 'pipefail', '-c', wipe.run], { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: `${parent}/`, + PATH: `${bin}:${process.env.PATH}`, + }, + }); + expect(res.status).not.toBe(0); + expect(readdirSync(ws)).toEqual(['leftover']); + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(bin, { recursive: true, force: true }); + } + }); + + it('refuses a trailing-slash GITHUB_WORKSPACE when realpath is absent', () => { + const dir = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-ws-')); + const bin = stubRealpath(); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + const res = spawnSync('bash', ['-e', '-o', 'pipefail', '-c', wipe.run], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${bin}:${process.env.PATH}`, + GITHUB_WORKSPACE: '/home/', + RUNNER_WORKSPACE: '/home', + }, + }); + expect(res.status).not.toBe(0); + expect(readFileSync(calls, 'utf8')).toBe(''); + } finally { + rmSync(dir, { recursive: true, force: true }); + rmSync(bin, { recursive: true, force: true }); + } + }); + + it('refuses an allowlist-escaping .. path when realpath is absent', () => { + const dir = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-fallback-')); + const outside = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-outside-')); + const bin = stubRealpath(); + mkdirSync(join(dir, 'sub')); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + const res = spawnSync('bash', ['-e', '-o', 'pipefail', '-c', wipe.run], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${bin}:${process.env.PATH}`, + GITHUB_WORKSPACE: `${dir}/sub/../../${basename(outside)}`, + RUNNER_WORKSPACE: dir, + }, + }); + expect(res.status).not.toBe(0); + expect(readFileSync(calls, 'utf8')).toBe(''); + } finally { + rmSync(dir, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + rmSync(bin, { recursive: true, force: true }); + } + }); + + // The degenerate-root arm keeps a stripped-empty RUNNER_WORKSPACE from + // turning the allowlist pattern into `/*` (which admits every absolute + // path). The reference suite covers the review workflow; this copy needs + // its own case — deleting the arm ships green otherwise. + it('refuses a runner workspace that resolves to / without invoking rm', () => { + const dir = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-root-')); + const ws = join(dir, 'repo'); + mkdirSync(ws); + writeFileSync(join(ws, 'leftover'), 'x'); + try { + const calls = join(dir, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + const res = spawnSync('bash', ['-e', '-o', 'pipefail', '-c', wipe.run], { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${dir}:${process.env.PATH}`, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: '/', + }, + }); + expect(res.status).not.toBe(0); + expect(readFileSync(calls, 'utf8')).toBe(''); + expect(readdirSync(ws)).toEqual(['leftover']); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + // The RWS realpath line has no refusal of its own to observe, so pin it + // from the happy side: a RUNNER_WORKSPACE spelled with '..' that + // canonicalizes back to the real parent must still be allowed to wipe. + // Deleting the RWS realpath line leaves the raw spelling to the '..' + // arm, which refuses — and this test fails on that mutant. + it.skipIf(!hasGnuRealpath)( + 'canonicalizes a ..-spelled runner workspace instead of refusing it', + () => { + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-rwsdot-')); + const ws = join(parent, 'repo'); + mkdirSync(ws); + writeFileSync(join(ws, 'leftover'), 'x'); + try { + runWipe({ + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: join(ws, '..'), + }); + expect(readdirSync(ws)).toEqual([]); + } finally { + rmSync(parent, { recursive: true, force: true }); + } + }, + ); +}); diff --git a/scripts/tests/vitest.config.ts b/scripts/tests/vitest.config.ts index 78f25870fa6..31c7de775ee 100644 --- a/scripts/tests/vitest.config.ts +++ b/scripts/tests/vitest.config.ts @@ -19,7 +19,10 @@ export default defineConfig({ ? [ ...configDefaults.exclude, 'scripts/tests/pr-self-report-label.test.js', + // Bash-driven workflow suites cannot run on Windows; pure + // YAML-parse workflow suites still do. 'scripts/tests/qwen-*-workflow.test.js', + 'scripts/tests/serve-ab-workflow.test.js', ] : [...configDefaults.exclude], setupFiles: ['scripts/tests/test-setup.ts'],