From ebf69a1250231187004caa1e92a9270c27a35dbf Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sun, 16 Aug 2026 16:50:54 +0000 Subject: [PATCH 1/7] fix(ci): back-port the checkout-heal wipe guard to the triage and serve-ab wipes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "empty the workspace, keep the directory" idiom exists in three copies; only the review workflow's copy received the #9220 hardening (canonicalization, trailing-slash strip, RUNNER_WORKSPACE allowlist). Measured on main for #9265, the two triage guards let non-canonical spellings of the guarded roots through (/home/, /home/., //usr, /root/, /var/ all reached the rm), and serve-ab's wipe had no guard at all — even `/home` or an empty string arrived at `find … -exec rm -rf`. Port the reference guard to all three sites, keeping each site's exit contract: triage fails loud both before and after external code, serve-ab stays bare under the job's `-eo pipefail` so an unclearable workspace fails before either checkout builds on top of the leftovers. Pin each ported copy with its own tests: bad-path batteries under an rm recorder (the destructive primitive cannot fire under any edit), an allowlist-escaping `..` case gated on a GNU-realpath host probe (the lesson from 90fa6bb4), a realpath-absent trailing-slash RUNNER_WORKSPACE case, and text pins on the ported layers. Every pin was mutation-verified red against a deletion of the layer it guards. --- .github/workflows/qwen-triage.yml | 51 +++- .github/workflows/serve-ab.yml | 43 +++- scripts/tests/qwen-triage-workflow.test.js | 269 ++++++++++++++++++++- scripts/tests/serve-ab-workflow.test.js | 242 ++++++++++++++++++ scripts/tests/vitest.config.ts | 5 +- 5 files changed, 602 insertions(+), 8 deletions(-) create mode 100644 scripts/tests/serve-ab-workflow.test.js diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index b867587bcba..73d4c724997 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2574,10 +2574,44 @@ 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. + # `-m` is GNU-only — a BSD realpath exits 1 on it and the fallback + # silently keeps the raw spelling. Safe here (this pool is + # Linux-only), and off-GNU the strip loop and the allowlist below + # are what still hold; the suite gates its GNU-only assertion on a + # host probe rather than assuming this line ran. + WS="$(realpath -m -- "$WS" 2>/dev/null || printf '%s' "$WS")" + # Trailing slashes slip past the exact-match case arms below + # (`/home/` would pass the guard and reach the rm); realpath strips + # them too, but this keeps the guard whole when realpath is absent. + while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done 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 || printf '%s' "$RWS")" + # Mirror the WS strip: without realpath, a trailing slash would + # turn the allowlist pattern into "$RWS"//* and refuse the real + # workspace; "/" 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 "$WS" in + "$RWS"/*) ;; + *) echo "::error::refusing to wipe workspace outside the runner workspace: ${WS}"; 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 +3538,25 @@ 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 || printf '%s' "$WS")" + while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done 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 || printf '%s' "$RWS")" + while [ "${RWS%/}" != "$RWS" ]; do RWS="${RWS%/}"; done + if [ -z "$RWS" ]; then echo "::error::refusing to wipe: runner workspace resolved to /"; exit 1; fi + case "$WS" in + "$RWS"/*) ;; + *) echo "::error::refusing to wipe workspace outside the runner workspace: ${WS}"; 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..3083ca851ef 100644 --- a/.github/workflows/serve-ab.yml +++ b/.github/workflows/serve-ab.yml @@ -80,7 +80,48 @@ 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. + # `-m` is GNU-only — a BSD realpath exits 1 on it and the fallback + # silently keeps the raw spelling. Safe here (this pool is + # Linux-only), and off-GNU the strip loop and the allowlist below + # are what still hold; the suite gates its GNU-only assertion on a + # host probe rather than assuming this line ran. + WS="$(realpath -m -- "$WS" 2>/dev/null || printf '%s' "$WS")" + # Trailing slashes slip past the exact-match case arms below + # (`/home/` would pass the guard and reach the rm); realpath strips + # them too, but this keeps the guard whole when realpath is absent. + while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done + 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 || printf '%s' "$RWS")" + # Mirror the WS strip: without realpath, a trailing slash would + # turn the allowlist pattern into "$RWS"//* and refuse the real + # workspace; "/" 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 "$WS" in + "$RWS"/*) ;; + *) echo "::error::refusing to wipe workspace outside the runner workspace: ${WS}"; 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..e367e1dd2c4 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -6,6 +6,7 @@ import { execFileSync, spawn, spawnSync } from 'node:child_process'; import { + chmodSync, existsSync, mkdirSync, mkdtempSync, @@ -15,7 +16,7 @@ import { 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'; @@ -1612,6 +1613,11 @@ 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('RUNNER_WORKSPACE:?'); + expect(wipe).toContain('"$RWS"/*'); }); // The wipe is the deny-by-default control, so run the real step text @@ -1644,6 +1650,9 @@ describe('qwen-triage verify hardening', () => { 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'), }, }); @@ -1687,7 +1696,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 +1729,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 +1756,123 @@ 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. This one string-matches "$RWS"/* + // (it sits inside the runner workspace) and canonicalizes OUT of it, + // so only the realpath line can refuse it: with the line deleted the + // raw path passes the allowlist and reaches rm. + const hasGnuRealpath = + spawnSync('realpath', ['-m', '--', '/'], { stdio: 'ignore' }).status === 0; + + 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 .. path via canonicalization', + () => { + const dir = mkdtempSync(join(tmpdir(), 'verify-wipe-escape-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-wipe-outside-')); + mkdirSync(join(dir, 'sub')); + writeFileSync(join(outside, 'canary'), '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 }, + ); + + 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}`, + // Keep the '..' spelling raw — resolving it here would + // canonicalize the fixture away before the script sees + // it. + 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 }); + } + }, + ); + + // Fronts PATH with a failing realpath so the script's `|| printf` + // fallback engages — the realpath-absent case the strip loops' comments + // justify themselves by. + 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('wipes a legitimate workspace despite a trailing-slash RUNNER_WORKSPACE when realpath is absent', () => { + // Without the RWS strip loop the allowlist pattern becomes "$RWS//*" + // and refuses the real workspace — the pre-run wipe would then fail + // every external verify on exactly the runners it exists for. + const parent = mkdtempSync(join(tmpdir(), 'verify-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', + extractRun('Wipe workspace before external code'), + ], + { + 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).toBe(0); + expect(readdirSync(ws)).toEqual([]); + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(bin, { 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,6 +1891,109 @@ 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('RUNNER_WORKSPACE:?'); + expect(postWipe).toContain('"$RWS"/*'); + }); + + // 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('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 diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js new file mode 100644 index 00000000000..d05a65e10e5 --- /dev/null +++ b/scripts/tests/serve-ab-workflow.test.js @@ -0,0 +1,242 @@ +/** + * @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, + 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: a BSD userland — macOS ships FreeBSD's `realpath [-q] [path +// ...]` — exits 1 on it, so the script's `|| printf` fallback keeps the +// path raw and no canonicalization happens at all. The serve-ab wipe only +// runs on the Linux ECS pool, so the production script is unaffected; this +// suite is not — it also runs on the merge-queue macOS lane, where an +// assertion about GNU behavior is red for a defect that cannot exist +// there. Probe the host, not the platform (same discipline as #9220's +// 90fa6bb4, applied to this port per #9265). +const hasGnuRealpath = + spawnSync('realpath', ['-m', '--', '/'], { stdio: 'ignore' }).status === 0; + +describe('serve-ab pre-checkout workspace wipe', () => { + 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('refusing to wipe suspicious workspace path'); + expect(wipe.run).toContain('RUNNER_WORKSPACE:?'); + expect(wipe.run).toContain('"$RWS"/*'); + // 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('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 .. path 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. This one + // string-matches "$RWS"/* but canonicalizes out of it, so only + // `realpath -m` can refuse it: with the line deleted, the raw path + // passes the allowlist and reaches rm (executed mutant: exit 0, + // canary in the call log). + const dir = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-escape-')); + const outside = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-outside-')); + mkdirSync(join(dir, 'sub')); + writeFileSync(join(outside, 'canary'), '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}`, + // Keep the '..' spelling raw — resolving it here would + // canonicalize the fixture away before the script sees it. + 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 }); + } + }, + ); + + // Fronts PATH with a failing realpath so the script's `|| printf` + // fallback engages — the realpath-absent case the strip loops' comments + // justify themselves by. + 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('wipes a legitimate workspace despite a trailing-slash RUNNER_WORKSPACE when realpath is absent', () => { + // Without the RWS strip loop the allowlist pattern becomes "$RWS//*" + // and refuses the real workspace — the wipe would then fail every + // serve-ab run on exactly the self-hosted runners this step exists + // for, and the job would die before the first checkout. + 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 { + runWipe({ + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: `${parent}/`, + PATH: `${bin}:${process.env.PATH}`, + }); + expect(readdirSync(ws)).toEqual([]); + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(bin, { recursive: true, force: true }); + } + }); +}); diff --git a/scripts/tests/vitest.config.ts b/scripts/tests/vitest.config.ts index 78f25870fa6..0f0f5ecc18e 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', - 'scripts/tests/qwen-*-workflow.test.js', + // Every *-workflow.test.js drives a workflow's real `run:` + // blocks under bash; none is named `qwen-*` uniformly (e.g. + // serve-ab-workflow.test.js), so exclude by the shared suffix. + 'scripts/tests/*-workflow.test.js', ] : [...configDefaults.exclude], setupFiles: ['scripts/tests/test-setup.ts'], From b6b3d87872c1289adb08aa420ebb5138594d1849 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 17 Aug 2026 02:36:28 +0800 Subject: [PATCH 2/7] test(ci): pin guarded serve wipe --- .github/scripts/ci-runner-routing.test.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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/); }); }); From d417ed4f394c8a1005f6a11f0d97d7a3bcb4c03b Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 17 Aug 2026 04:36:17 +0800 Subject: [PATCH 3/7] fix(ci): close wipe guard fallback gaps --- .github/workflows/qwen-triage.yml | 22 ++++-- .github/workflows/serve-ab.yml | 14 +++- scripts/tests/qwen-triage-workflow.test.js | 86 ++++++++++++++++++++++ scripts/tests/serve-ab-workflow.test.js | 59 +++++++++++++++ scripts/tests/vitest.config.ts | 8 +- 5 files changed, 176 insertions(+), 13 deletions(-) diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 73d4c724997..07204672c24 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2584,14 +2584,17 @@ jobs: # /usr), so a raw string match lets them slip past the case arms. # `-m` is GNU-only — a BSD realpath exits 1 on it and the fallback # silently keeps the raw spelling. Safe here (this pool is - # Linux-only), and off-GNU the strip loop and the allowlist below - # are what still hold; the suite gates its GNU-only assertion on a - # host probe rather than assuming this line ran. + # Linux-only), and off-GNU the strip loop, `..` refusal, and + # allowlist below are what still hold; the suite gates its GNU-only + # assertion on a host probe rather than assuming this line ran. WS="$(realpath -m -- "$WS" 2>/dev/null || printf '%s' "$WS")" # Trailing slashes slip past the exact-match case arms below # (`/home/` would pass the guard and reach the rm); realpath strips # them too, but this keeps the guard whole when realpath is absent. 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 @@ -2608,9 +2611,12 @@ jobs: # workspace; "/" 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}"; exit 1 ;; + *) 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 @@ -3546,6 +3552,9 @@ jobs: WS="${GITHUB_WORKSPACE:?}" WS="$(realpath -m -- "$WS" 2>/dev/null || printf '%s' "$WS")" 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 @@ -3553,9 +3562,12 @@ jobs: RWS="$(realpath -m -- "$RWS" 2>/dev/null || printf '%s' "$RWS")" 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}"; exit 1 ;; + *) 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 3083ca851ef..fe7f62797eb 100644 --- a/.github/workflows/serve-ab.yml +++ b/.github/workflows/serve-ab.yml @@ -93,14 +93,17 @@ jobs: # /usr), so a raw string match lets them slip past the case arms. # `-m` is GNU-only — a BSD realpath exits 1 on it and the fallback # silently keeps the raw spelling. Safe here (this pool is - # Linux-only), and off-GNU the strip loop and the allowlist below - # are what still hold; the suite gates its GNU-only assertion on a - # host probe rather than assuming this line ran. + # Linux-only), and off-GNU the strip loop, `..` refusal, and + # allowlist below are what still hold; the suite gates its GNU-only + # assertion on a host probe rather than assuming this line ran. WS="$(realpath -m -- "$WS" 2>/dev/null || printf '%s' "$WS")" # Trailing slashes slip past the exact-match case arms below # (`/home/` would pass the guard and reach the rm); realpath strips # them too, but this keeps the guard whole when realpath is absent. 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 @@ -114,9 +117,12 @@ jobs: # workspace; "/" 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}"; exit 1 ;; + *) 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 diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index e367e1dd2c4..3e8df641a0e 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -1873,6 +1873,92 @@ describe('qwen-triage verify hardening', () => { } }); + 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 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', () => { diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js index d05a65e10e5..feb895b31e0 100644 --- a/scripts/tests/serve-ab-workflow.test.js +++ b/scripts/tests/serve-ab-workflow.test.js @@ -239,4 +239,63 @@ describe('serve-ab pre-checkout workspace wipe', () => { 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 }); + } + }); }); diff --git a/scripts/tests/vitest.config.ts b/scripts/tests/vitest.config.ts index 0f0f5ecc18e..31c7de775ee 100644 --- a/scripts/tests/vitest.config.ts +++ b/scripts/tests/vitest.config.ts @@ -19,10 +19,10 @@ export default defineConfig({ ? [ ...configDefaults.exclude, 'scripts/tests/pr-self-report-label.test.js', - // Every *-workflow.test.js drives a workflow's real `run:` - // blocks under bash; none is named `qwen-*` uniformly (e.g. - // serve-ab-workflow.test.js), so exclude by the shared suffix. - 'scripts/tests/*-workflow.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'], From 13fca2c39528447e7ae03948751999bdd66ad36d Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 17 Aug 2026 10:40:01 +0800 Subject: [PATCH 4/7] fix(ci): fail closed without realpath --- .github/workflows/qwen-triage.yml | 21 +++++-------- .github/workflows/serve-ab.yml | 17 ++++------- scripts/tests/qwen-triage-workflow.test.js | 14 ++++----- scripts/tests/serve-ab-workflow.test.js | 35 +++++++++------------- 4 files changed, 33 insertions(+), 54 deletions(-) diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 07204672c24..9365f95c093 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2582,15 +2582,11 @@ jobs: # 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. - # `-m` is GNU-only — a BSD realpath exits 1 on it and the fallback - # silently keeps the raw spelling. Safe here (this pool is - # Linux-only), and off-GNU the strip loop, `..` refusal, and - # allowlist below are what still hold; the suite gates its GNU-only - # assertion on a host probe rather than assuming this line ran. - WS="$(realpath -m -- "$WS" 2>/dev/null || printf '%s' "$WS")" + WS="$(realpath -m -- "$WS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${WS}"; 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, but this keeps the guard whole when realpath is absent. + # 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 ;; @@ -2605,10 +2601,9 @@ jobs: # 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 || printf '%s' "$RWS")" - # Mirror the WS strip: without realpath, a trailing slash would - # turn the allowlist pattern into "$RWS"//* and refuse the real - # workspace; "/" stripped empty would match every path instead. + RWS="$(realpath -m -- "$RWS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${RWS}"; 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 @@ -3550,7 +3545,7 @@ jobs: # (#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 || printf '%s' "$WS")" + WS="$(realpath -m -- "$WS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${WS}"; exit 1; } while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done case "$WS" in ..|../*|*/..|*/../*) echo "::error::refusing to wipe path containing '..': ${WS}"; exit 1 ;; @@ -3559,7 +3554,7 @@ jobs: /|/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 || printf '%s' "$RWS")" + RWS="$(realpath -m -- "$RWS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${RWS}"; 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 diff --git a/.github/workflows/serve-ab.yml b/.github/workflows/serve-ab.yml index fe7f62797eb..811a62b7da7 100644 --- a/.github/workflows/serve-ab.yml +++ b/.github/workflows/serve-ab.yml @@ -91,15 +91,11 @@ jobs: # 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. - # `-m` is GNU-only — a BSD realpath exits 1 on it and the fallback - # silently keeps the raw spelling. Safe here (this pool is - # Linux-only), and off-GNU the strip loop, `..` refusal, and - # allowlist below are what still hold; the suite gates its GNU-only - # assertion on a host probe rather than assuming this line ran. - WS="$(realpath -m -- "$WS" 2>/dev/null || printf '%s' "$WS")" + WS="$(realpath -m -- "$WS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${WS}"; 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, but this keeps the guard whole when realpath is absent. + # 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 ;; @@ -111,10 +107,9 @@ jobs: # 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 || printf '%s' "$RWS")" - # Mirror the WS strip: without realpath, a trailing slash would - # turn the allowlist pattern into "$RWS"//* and refuse the real - # workspace; "/" stripped empty would match every path instead. + RWS="$(realpath -m -- "$RWS" 2>/dev/null)" || { echo "::error::refusing to wipe: realpath unavailable, cannot canonicalize ${RWS}"; 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 diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 3e8df641a0e..98e719fce6c 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -1825,9 +1825,8 @@ describe('qwen-triage verify hardening', () => { }, ); - // Fronts PATH with a failing realpath so the script's `|| printf` - // fallback engages — the realpath-absent case the strip loops' comments - // justify themselves by. + // 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'); @@ -1835,10 +1834,7 @@ describe('qwen-triage verify hardening', () => { return bin; }; - it('wipes a legitimate workspace despite a trailing-slash RUNNER_WORKSPACE when realpath is absent', () => { - // Without the RWS strip loop the allowlist pattern becomes "$RWS//*" - // and refuses the real workspace — the pre-run wipe would then fail - // every external verify on exactly the runners it exists for. + it('refuses to wipe when realpath is absent', () => { const parent = mkdtempSync(join(tmpdir(), 'verify-wipe-rws-')); const ws = join(parent, 'repo'); mkdirSync(ws); @@ -1865,8 +1861,8 @@ describe('qwen-triage verify hardening', () => { }, }, ); - expect(res.status).toBe(0); - expect(readdirSync(ws)).toEqual([]); + expect(res.status).not.toBe(0); + expect(readdirSync(ws)).toEqual(['leftover']); } finally { rmSync(parent, { recursive: true, force: true }); rmSync(bin, { recursive: true, force: true }); diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js index feb895b31e0..065c7ac3d19 100644 --- a/scripts/tests/serve-ab-workflow.test.js +++ b/scripts/tests/serve-ab-workflow.test.js @@ -37,14 +37,7 @@ const runWipe = (env, options = {}) => }); // `realpath -m` (the script's canonicalization line) is a GNU coreutils -// extension: a BSD userland — macOS ships FreeBSD's `realpath [-q] [path -// ...]` — exits 1 on it, so the script's `|| printf` fallback keeps the -// path raw and no canonicalization happens at all. The serve-ab wipe only -// runs on the Linux ECS pool, so the production script is unaffected; this -// suite is not — it also runs on the merge-queue macOS lane, where an -// assertion about GNU behavior is red for a defect that cannot exist -// there. Probe the host, not the platform (same discipline as #9220's -// 90fa6bb4, applied to this port per #9265). +// extension. Probe the host before asserting GNU-specific path behavior. const hasGnuRealpath = spawnSync('realpath', ['-m', '--', '/'], { stdio: 'ignore' }).status === 0; @@ -207,9 +200,8 @@ describe('serve-ab pre-checkout workspace wipe', () => { }, ); - // Fronts PATH with a failing realpath so the script's `|| printf` - // fallback engages — the realpath-absent case the strip loops' comments - // justify themselves by. + // 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'); @@ -217,23 +209,24 @@ describe('serve-ab pre-checkout workspace wipe', () => { return bin; }; - it('wipes a legitimate workspace despite a trailing-slash RUNNER_WORKSPACE when realpath is absent', () => { - // Without the RWS strip loop the allowlist pattern becomes "$RWS//*" - // and refuses the real workspace — the wipe would then fail every - // serve-ab run on exactly the self-hosted runners this step exists - // for, and the job would die before the first checkout. + 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 { - runWipe({ - GITHUB_WORKSPACE: ws, - RUNNER_WORKSPACE: `${parent}/`, - PATH: `${bin}:${process.env.PATH}`, + 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(readdirSync(ws)).toEqual([]); + expect(res.status).not.toBe(0); + expect(readdirSync(ws)).toEqual(['leftover']); } finally { rmSync(parent, { recursive: true, force: true }); rmSync(bin, { recursive: true, force: true }); From d6fbef0c03a883d521496b181a9edb2c02d55173 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 17 Aug 2026 14:42:24 +0800 Subject: [PATCH 5/7] fix(ci): keep wipe guards portable --- .github/workflows/qwen-triage.yml | 8 +- .github/workflows/serve-ab.yml | 4 +- scripts/tests/qwen-triage-workflow.test.js | 146 +++++++++++---------- scripts/tests/serve-ab-workflow.test.js | 39 +++--- 4 files changed, 104 insertions(+), 93 deletions(-) diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 9365f95c093..6fb17ebf646 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2582,7 +2582,7 @@ jobs: # 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 ${WS}"; exit 1; } + 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 @@ -2601,7 +2601,7 @@ jobs: # 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 ${RWS}"; exit 1; } + 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 @@ -3545,7 +3545,7 @@ jobs: # (#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 ${WS}"; exit 1; } + 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 ;; @@ -3554,7 +3554,7 @@ jobs: /|/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 ${RWS}"; exit 1; } + 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 diff --git a/.github/workflows/serve-ab.yml b/.github/workflows/serve-ab.yml index 811a62b7da7..74a5d5c7c61 100644 --- a/.github/workflows/serve-ab.yml +++ b/.github/workflows/serve-ab.yml @@ -91,7 +91,7 @@ jobs: # 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 ${WS}"; exit 1; } + 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 @@ -107,7 +107,7 @@ jobs: # 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 ${RWS}"; exit 1; } + 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 diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 98e719fce6c..afec0a6632c 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -30,6 +30,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, '\\$&'); @@ -1623,45 +1625,51 @@ describe('qwen-triage verify hardening', () => { // 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(); + 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 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 }); + 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 }); - const res = spawnSync('bash', ['-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 }); - } - }); + const res = spawnSync('bash', ['-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` @@ -1763,9 +1771,6 @@ describe('qwen-triage verify hardening', () => { // (it sits inside the runner workspace) and canonicalizes OUT of it, // so only the realpath line can refuse it: with the line deleted the // raw path passes the allowlist and reaches rm. - const hasGnuRealpath = - spawnSync('realpath', ['-m', '--', '/'], { stdio: 'ignore' }).status === 0; - const extractRun = (stepName) => { const run = stepIn('verify', stepName) .match(/run: \|-\n([\s\S]*)$/)?.[1] @@ -2046,37 +2051,40 @@ describe('qwen-triage verify hardening', () => { // 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('wipes a legitimate workspace in the post-run wipe and writes the summary', () => { - const wipe = extractRun('Wipe workspace after external code'); + 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', + 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', ); - } finally { - rmSync(parent, { recursive: true, force: true }); - } - }); + 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 diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js index 065c7ac3d19..ebefc28249e 100644 --- a/scripts/tests/serve-ab-workflow.test.js +++ b/scripts/tests/serve-ab-workflow.test.js @@ -66,24 +66,27 @@ describe('serve-ab pre-checkout workspace wipe', () => { expect(wipe.run).not.toContain('|| true'); }); - it('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 }); - } - }); + 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 From a241a3d9b3b0a588dd2a1ae1acb68dc6d49587bf Mon Sep 17 00:00:00 2001 From: yiliang114 <1204183885@qq.com> Date: Mon, 17 Aug 2026 10:22:30 +0000 Subject: [PATCH 6/7] test(ci): pin wipe-guard RWS layers and unmask the pre-run battery - run the rewritten pre-run sweep battery under -e -o pipefail so a failing sweep can no longer report success (bare bash -c masked it) - pin the RWS '..' refusal and degenerate-root refusal text in all copies, and add RUNNER_WORKSPACE='/' exec cases to both copy suites - exercise both pre-run and post-run copies in the realpath-absent refusal test - replace the '..' escape vector with a symlink escape that only the realpath line can refuse, and correct the mutant-outcome comments - add the serve-ab wipe-before-checkouts ordering pin from the sister suite and a happy-path RWS canonicalization pin --- scripts/tests/qwen-triage-workflow.test.js | 171 +++++++++++++++++---- scripts/tests/serve-ab-workflow.test.js | 103 +++++++++++-- 2 files changed, 232 insertions(+), 42 deletions(-) diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index afec0a6632c..bb1fbffe034 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -13,6 +13,7 @@ import { readdirSync, readFileSync, rmSync, + symlinkSync, writeFileSync, } from 'node:fs'; import { tmpdir } from 'node:os'; @@ -1618,8 +1619,14 @@ describe('qwen-triage verify hardening', () => { // 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 @@ -1652,7 +1659,11 @@ describe('qwen-triage verify hardening', () => { writeFileSync(join(ws, 'package.json'), '{}'); mkdirSync(join(ws, 'node_modules'), { recursive: true }); - const res = spawnSync('bash', ['-c', wipe], { + // 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, @@ -1767,10 +1778,13 @@ 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. This one string-matches "$RWS"/* - // (it sits inside the runner workspace) and canonicalizes OUT of it, - // so only the realpath line can refuse it: with the line deleted the - // raw path passes the allowlist and reaches rm. + // 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"/* and find reaches rm through the link + // target. const extractRun = (stepName) => { const run = stepIn('verify', stepName) .match(/run: \|-\n([\s\S]*)$/)?.[1] @@ -1780,12 +1794,12 @@ describe('qwen-triage verify hardening', () => { }; it.skipIf(!hasGnuRealpath)( - 'refuses an allowlist-escaping .. path via canonicalization', + 'refuses an allowlist-escaping symlink via canonicalization', () => { const dir = mkdtempSync(join(tmpdir(), 'verify-wipe-escape-')); const outside = mkdtempSync(join(tmpdir(), 'verify-wipe-outside-')); - mkdirSync(join(dir, 'sub')); writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, join(dir, 'link')); try { const calls = join(dir, 'rm-calls'); writeFileSync(calls, ''); @@ -1808,10 +1822,10 @@ describe('qwen-triage verify hardening', () => { env: { ...process.env, PATH: `${dir}:${process.env.PATH}`, - // Keep the '..' spelling raw — resolving it here would - // canonicalize the fixture away before the script sees - // it. - GITHUB_WORKSPACE: `${dir}/sub/../../${basename(outside)}`, + // 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'), }, @@ -1846,28 +1860,31 @@ describe('qwen-triage verify hardening', () => { writeFileSync(join(ws, 'leftover'), 'x'); const bin = stubRealpath(); try { - const res = spawnSync( - 'bash', - [ - '-e', - '-o', - 'pipefail', - '-c', - extractRun('Wipe workspace before external code'), - ], - { - encoding: 'utf8', - env: { - ...process.env, - PATH: `${bin}:${process.env.PATH}`, - GITHUB_WORKSPACE: ws, - RUNNER_WORKSPACE: `${parent}/`, - GITHUB_STEP_SUMMARY: join(parent, 'summary'), + // 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).not.toBe(0); - expect(readdirSync(ws)).toEqual(['leftover']); + ); + 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 }); @@ -1960,6 +1977,91 @@ describe('qwen-triage verify hardening', () => { } }); + // 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', () => { @@ -1983,8 +2085,13 @@ describe('qwen-triage verify hardening', () => { // 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 diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js index ebefc28249e..25e5e0cda76 100644 --- a/scripts/tests/serve-ab-workflow.test.js +++ b/scripts/tests/serve-ab-workflow.test.js @@ -12,6 +12,7 @@ import { readdirSync, readFileSync, rmSync, + symlinkSync, writeFileSync, } from 'node:fs'; import { tmpdir } from 'node:os'; @@ -42,6 +43,19 @@ 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 @@ -56,9 +70,17 @@ describe('serve-ab pre-checkout workspace wipe', () => { // 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. @@ -158,19 +180,21 @@ describe('serve-ab pre-checkout workspace wipe', () => { }); it.skipIf(!hasGnuRealpath)( - 'refuses an allowlist-escaping .. path via canonicalization', + '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. This one - // string-matches "$RWS"/* but canonicalizes out of it, so only - // `realpath -m` can refuse it: with the line deleted, the raw path - // passes the allowlist and reaches rm (executed mutant: exit 0, - // canary in the call log). + // 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"/* and find reaches rm through the link target. const dir = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-escape-')); const outside = mkdtempSync(join(tmpdir(), 'serve-ab-wipe-outside-')); - mkdirSync(join(dir, 'sub')); writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, join(dir, 'link')); try { const calls = join(dir, 'rm-calls'); writeFileSync(calls, ''); @@ -187,9 +211,10 @@ describe('serve-ab pre-checkout workspace wipe', () => { env: { ...process.env, PATH: `${dir}:${process.env.PATH}`, - // Keep the '..' spelling raw — resolving it here would - // canonicalize the fixture away before the script sees it. - GITHUB_WORKSPACE: `${dir}/sub/../../${basename(outside)}`, + // 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, }, }, @@ -294,4 +319,62 @@ describe('serve-ab pre-checkout workspace wipe', () => { 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 }); + } + }, + ); }); From d2cee7fce493ca4aae0ee7033602b46cce38ef74 Mon Sep 17 00:00:00 2001 From: yiliang114 <1204183885@qq.com> Date: Mon, 17 Aug 2026 12:28:59 +0000 Subject: [PATCH 7/7] test(ci): correct wipe-guard mutant-outcome comments for find -P MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The symlink-escape comments claimed that with the WS realpath line deleted, find reaches rm through the link target. GNU find's default -P mode does not descend symlink operands: the mutant passes every guard, wipes nothing, and exits 0, so only the non-zero-status assertion catches it — the rm-log assertion passes vacuously. Reword both twin comments (R5-1). --- scripts/tests/qwen-triage-workflow.test.js | 5 +++-- scripts/tests/serve-ab-workflow.test.js | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index bb1fbffe034..65909f73af7 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -1783,8 +1783,9 @@ describe('qwen-triage verify hardening', () => { // 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"/* and find reaches rm through the link - // target. + // 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] diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js index 25e5e0cda76..997cf70c81a 100644 --- a/scripts/tests/serve-ab-workflow.test.js +++ b/scripts/tests/serve-ab-workflow.test.js @@ -190,7 +190,9 @@ describe('serve-ab pre-checkout workspace wipe', () => { // 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"/* and find reaches rm through the link target. + // "$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');