From 84314573f0d733b3d5d38c94514f83de3bd89fba Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 20 Aug 2026 01:40:51 +0800 Subject: [PATCH 1/4] fix(ci): heal a symlinked workspace instead of wedging the runner on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hardened wipe guard refuses any workspace that canonicalizes outside the runner workspace. That refusal is correct, and it created a permanent failure: when a previous job leaves the workspace replaced by a symlink pointing outside — or by any non-directory — the guard resolves it to the target, refuses, and exits 1 having removed nothing. Nothing else clears that state, so every later job on the runner dies at the same line, forever. The pre-guard code wiped through the link and self-healed by accident. Reproduced against main's own step text before this change. Heal it: the link itself lives inside the runner workspace and is safe to unlink, and only once it is gone can a legitimate wipe proceed. The layer has to sit before canonicalization — afterwards the path has already resolved to the target and the allowlist refuses before any repair can happen — which means it judges a raw path, and that is where the first attempt at this (closed with #9369) went wrong. A raw `"$RWS"/*` match accepts `$RWS/link/sub` as a string while the kernel resolves it through an intermediate symlink to a file outside the runner workspace, so the unlink and the mkdir landed outside and only then did the allowlist refuse the wipe. Here the containment is judged on the canonicalized PARENT — never on $WS, which would resolve through the very link being removed — and the unlink then acts on the raw path, so it takes the link and never follows it. Four more constraints the same review surfaced: the raw trailing-slash strip moves ahead of the predicates (both `[ -L "$WS/" ]` and `[ ! -d "$WS/" ]` resolve through a link and report its target, so one slash hides the corruption); the allowlist root is prepared before the heal, since it bounds it, and an empty $RUNNER_WORKSPACE would degenerate the containment pattern to the match-all `/*`; both the unlink and the mkdir fail closed, because under `-e` a failure that is not the last command of an && list is swallowed and would leave the wipe running on a corrupt path; and the heal logs what it found and where the link pointed, since this incident otherwise leaves no trace at all. All three copies get it — the two triage wipes and the A/B wipe — with per-suite fixtures: the wedge healed (link gone, directory recreated, target's contents intact), the intermediate-symlink attack refused with the outside file unmutated and zero rm calls, the non-directory half, the trailing-slash spelling, the fail-closed unlink, and the ordinary workspace where the heal must not fire at all. Mutation-checked layer by layer; each has a fixture that fails when it is removed. One pre-existing test changes meaning: the canonicalization pin used a symlinked workspace and asserted refusal, which is now the healed path. It moves to a vector the heal does not touch — an intermediate symlink whose far end is a directory — and keeps its mutation strength: with the canonicalization deleted, find resolves the link and hands the outside directory's entries to the rm recorder. Closes #9480 --- .github/workflows/qwen-triage.yml | 134 ++++++++--- .github/workflows/serve-ab.yml | 61 ++++- scripts/tests/qwen-triage-workflow.test.js | 262 +++++++++++++++++++- scripts/tests/serve-ab-workflow.test.js | 267 +++++++++++++++++++-- 4 files changed, 650 insertions(+), 74 deletions(-) diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 68dc9b6a236..cce0d3f2cb8 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2599,21 +2599,58 @@ jobs: run: |- set -uo pipefail 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 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). + # Strip trailing slashes on the RAW path, before anything reads it: + # `[ -L "$WS/" ]` and `[ ! -d "$WS/" ]` both resolve THROUGH a link + # and report its target, so one trailing slash hides the corruption + # the heal below exists to clear. + while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done + # The allowlist root is prepared BEFORE the heal, because it bounds + # what the heal may touch: canonical, slash-free and non-degenerate. + # An empty $RUNNER_WORKSPACE would turn every containment pattern + # below into the match-all "/*". + 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 + # Heal a workspace a previous job replaced with a symlink (or any + # non-directory) BEFORE canonicalizing it. Afterwards the path + # resolves to the link's target, the allowlist refuses that, and the + # refusal removes nothing — so every later job on this runner dies + # here, permanently, on corruption that is itself inside the runner + # workspace and safe to unlink. + if [ -L "$WS" ] || [ ! -d "$WS" ]; then + # Judge the PARENT, canonicalized. The heal necessarily acts on a + # raw path, and a raw containment match is not enough: the kernel + # resolves intermediate components too, so `$RWS/link/sub` matches + # "$RWS"/* as a string while naming a file outside it. Resolving + # the parent — never $WS itself, which would resolve through the + # very link being removed — is what makes the unlink containable. + HEAL_PARENT="$(realpath -m -- "$(dirname -- "$WS")" 2>/dev/null)" || { echo "::error::refusing to heal: realpath unavailable, cannot canonicalize the parent of ${WS}"; exit 1; } + case "$HEAL_PARENT" in + "$RWS"|"$RWS"/*) ;; + *) echo "::error::refusing to heal workspace outside the runner workspace: ${WS} (parent: ${HEAL_PARENT}, runner workspace: ${RWS})"; exit 1 ;; + esac + # The incident this heal exists for leaves no other trace: say what + # was found, and where it pointed, before it is gone. + if [ -L "$WS" ]; then + echo "::warning::healing workspace ${WS}: it was a symlink to $(readlink -- "$WS" 2>/dev/null || echo "")" + else + echo "::warning::healing workspace ${WS}: it was not a directory" + fi + # `rm -f` on the RAW path removes the link itself and never + # follows it. Both legs fail closed: under `-e` a failure that is + # not the last command of an && list is swallowed, and a swallowed + # one here would leave the wipe running against a corrupt path. + rm -f -- "$WS" || { echo "::error::refusing to continue: could not remove ${WS}"; exit 1; } + mkdir -- "$WS" || { echo "::error::refusing to continue: could not recreate ${WS}"; exit 1; } + fi # 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 ;; @@ -2623,19 +2660,7 @@ jobs: 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 + # runner workspace may be wiped. case "$WS" in "$RWS"/*) ;; *) echo "::error::refusing to wipe workspace outside the runner workspace: ${WS} (runner workspace: ${RWS})"; exit 1 ;; @@ -4817,14 +4842,15 @@ 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 ${GITHUB_WORKSPACE}"; exit 1; } + # Strip trailing slashes on the RAW path, before anything reads it: + # `[ -L "$WS/" ]` and `[ ! -d "$WS/" ]` both resolve THROUGH a link + # and report its target, so one trailing slash hides the corruption + # the heal below exists to clear. 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 + # The allowlist root is prepared BEFORE the heal, because it bounds + # what the heal may touch: canonical, slash-free and non-degenerate. + # An empty $RUNNER_WORKSPACE would turn every containment pattern + # below into the match-all "/*". 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 @@ -4832,6 +4858,52 @@ jobs: case "$RWS" in ..|../*|*/..|*/../*) echo "::error::refusing runner workspace path containing '..': ${RWS}"; exit 1 ;; esac + # Heal a workspace a previous job replaced with a symlink (or any + # non-directory) BEFORE canonicalizing it. Afterwards the path + # resolves to the link's target, the allowlist refuses that, and the + # refusal removes nothing — so every later job on this runner dies + # here, permanently, on corruption that is itself inside the runner + # workspace and safe to unlink. + if [ -L "$WS" ] || [ ! -d "$WS" ]; then + # Judge the PARENT, canonicalized. The heal necessarily acts on a + # raw path, and a raw containment match is not enough: the kernel + # resolves intermediate components too, so `$RWS/link/sub` matches + # "$RWS"/* as a string while naming a file outside it. Resolving + # the parent — never $WS itself, which would resolve through the + # very link being removed — is what makes the unlink containable. + HEAL_PARENT="$(realpath -m -- "$(dirname -- "$WS")" 2>/dev/null)" || { echo "::error::refusing to heal: realpath unavailable, cannot canonicalize the parent of ${WS}"; exit 1; } + case "$HEAL_PARENT" in + "$RWS"|"$RWS"/*) ;; + *) echo "::error::refusing to heal workspace outside the runner workspace: ${WS} (parent: ${HEAL_PARENT}, runner workspace: ${RWS})"; exit 1 ;; + esac + # The incident this heal exists for leaves no other trace: say what + # was found, and where it pointed, before it is gone. + if [ -L "$WS" ]; then + echo "::warning::healing workspace ${WS}: it was a symlink to $(readlink -- "$WS" 2>/dev/null || echo "")" + else + echo "::warning::healing workspace ${WS}: it was not a directory" + fi + # `rm -f` on the RAW path removes the link itself and never + # follows it. Both legs fail closed: under `-e` a failure that is + # not the last command of an && list is swallowed, and a swallowed + # one here would leave the wipe running against a corrupt path. + rm -f -- "$WS" || { echo "::error::refusing to continue: could not remove ${WS}"; exit 1; } + mkdir -- "$WS" || { echo "::error::refusing to continue: could not recreate ${WS}"; exit 1; } + fi + # 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; } + 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. case "$WS" in "$RWS"/*) ;; *) echo "::error::refusing to wipe workspace outside the runner workspace: ${WS} (runner workspace: ${RWS})"; exit 1 ;; diff --git a/.github/workflows/serve-ab.yml b/.github/workflows/serve-ab.yml index 74a5d5c7c61..e54ccf3f0a9 100644 --- a/.github/workflows/serve-ab.yml +++ b/.github/workflows/serve-ab.yml @@ -88,14 +88,58 @@ jobs: # strip trailing slashes, denylist the known roots, and require # the target to sit inside the runner workspace before any rm. WS="${GITHUB_WORKSPACE:?}" + # Strip trailing slashes on the RAW path, before anything reads it: + # `[ -L "$WS/" ]` and `[ ! -d "$WS/" ]` both resolve THROUGH a link + # and report its target, so one trailing slash hides the corruption + # the heal below exists to clear. + while [ "${WS%/}" != "$WS" ]; do WS="${WS%/}"; done + # The allowlist root is prepared BEFORE the heal, because it bounds + # what the heal may touch: canonical, slash-free and non-degenerate. + # An empty $RUNNER_WORKSPACE would turn every containment pattern + # below into the match-all "/*". + 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 + # Heal a workspace a previous job replaced with a symlink (or any + # non-directory) BEFORE canonicalizing it. Afterwards the path + # resolves to the link's target, the allowlist refuses that, and the + # refusal removes nothing — so every later job on this runner dies + # here, permanently, on corruption that is itself inside the runner + # workspace and safe to unlink. + if [ -L "$WS" ] || [ ! -d "$WS" ]; then + # Judge the PARENT, canonicalized. The heal necessarily acts on a + # raw path, and a raw containment match is not enough: the kernel + # resolves intermediate components too, so `$RWS/link/sub` matches + # "$RWS"/* as a string while naming a file outside it. Resolving + # the parent — never $WS itself, which would resolve through the + # very link being removed — is what makes the unlink containable. + HEAL_PARENT="$(realpath -m -- "$(dirname -- "$WS")" 2>/dev/null)" || { echo "::error::refusing to heal: realpath unavailable, cannot canonicalize the parent of ${WS}"; exit 1; } + case "$HEAL_PARENT" in + "$RWS"|"$RWS"/*) ;; + *) echo "::error::refusing to heal workspace outside the runner workspace: ${WS} (parent: ${HEAL_PARENT}, runner workspace: ${RWS})"; exit 1 ;; + esac + # The incident this heal exists for leaves no other trace: say what + # was found, and where it pointed, before it is gone. + if [ -L "$WS" ]; then + echo "::warning::healing workspace ${WS}: it was a symlink to $(readlink -- "$WS" 2>/dev/null || echo "")" + else + echo "::warning::healing workspace ${WS}: it was not a directory" + fi + # `rm -f` on the RAW path removes the link itself and never + # follows it. Both legs fail closed: under `-e` a failure that is + # not the last command of an && list is swallowed, and a swallowed + # one here would leave the wipe running against a corrupt path. + rm -f -- "$WS" || { echo "::error::refusing to continue: could not remove ${WS}"; exit 1; } + mkdir -- "$WS" || { echo "::error::refusing to continue: could not recreate ${WS}"; exit 1; } + fi # 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 ;; @@ -106,15 +150,6 @@ jobs: # 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 ;; diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 41d43282a3e..7e01da30bfd 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -8,6 +8,7 @@ import { execFileSync, spawn, spawnSync } from 'node:child_process'; import { chmodSync, existsSync, + lstatSync, mkdirSync, mkdtempSync, readdirSync, @@ -1632,6 +1633,38 @@ describe('qwen-triage verify hardening', () => { expect(wipe).toContain('runner workspace resolved to /'); }); + it('carries the symlink heal in both wipes, ordered and bounded (#9480)', () => { + // The guard's own wedge: a workspace replaced by a symlink resolves to + // its target, the allowlist refuses, and the refusal removes nothing — + // so the runner dies here on every later job. The heal must therefore + // run BEFORE canonicalization, AFTER the allowlist root that bounds it, + // and after the raw trailing-slash strip, since both of its predicates + // resolve through a link when the path ends in '/'. + for (const stepName of [ + 'Wipe workspace before external code', + 'Wipe workspace after external code', + ]) { + const run = stepIn('verify', stepName); + const healAt = run.indexOf('[ -L "$WS" ] || [ ! -d "$WS" ]'); + expect(healAt, `${stepName} has no heal`).toBeGreaterThan(-1); + expect(healAt).toBeLessThan(run.indexOf('realpath -m -- "$WS"')); + expect(run.indexOf('RWS="${RUNNER_WORKSPACE:?}"')).toBeLessThan(healAt); + expect(run.indexOf('while [ "${WS%/}" != "$WS" ]')).toBeLessThan(healAt); + // Containment on the canonical PARENT — resolving $WS would follow + // the very link being removed, and a raw match cannot see + // intermediate symlink components. + expect(run).toContain( + 'HEAL_PARENT="$(realpath -m -- "$(dirname -- "$WS")" 2>/dev/null)"', + ); + expect(run).toContain('"$RWS"|"$RWS"/*)'); + expect(run).toContain('refusing to heal workspace outside'); + // Both legs fail closed, and the incident leaves a trace. + expect(run).toContain('rm -f -- "$WS" || {'); + expect(run).toContain('mkdir -- "$WS" || {'); + expect(run).toContain('::warning::healing workspace'); + } + }); + // 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. @@ -1783,12 +1816,13 @@ describe('qwen-triage verify hardening', () => { // 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. + // What does pin it is a path whose INTERMEDIATE component leaves the + // runner workspace: it matches "$RWS"/* as a string and names a + // directory outside it, so with the line deleted find resolves the link + // through the kernel and hands that directory's entries to the rm + // recorder. It is deliberately a directory at the far end rather than + // the link itself — a workspace that IS a link is now healed rather + // than refused (#9480), and this test exists for the refusal. const extractRun = (stepName) => { const run = stepIn('verify', stepName) .match(/run: \|-\n([\s\S]*)$/)?.[1] @@ -1798,15 +1832,15 @@ describe('qwen-triage verify hardening', () => { }; it.skipIf(!hasGnuRealpath)( - 'refuses an allowlist-escaping symlink via canonicalization', + 'refuses an allowlist-escaping path reached through an intermediate symlink', () => { const dir = mkdtempSync(join(tmpdir(), 'verify-wipe-escape-')); const outside = mkdtempSync(join(tmpdir(), 'verify-wipe-outside-')); - writeFileSync(join(outside, 'canary'), 'x'); + mkdirSync(join(outside, 'sub')); + writeFileSync(join(outside, 'sub', '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`, @@ -1826,20 +1860,21 @@ describe('qwen-triage verify hardening', () => { 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'), + GITHUB_WORKSPACE: join(dir, 'link', 'sub'), RUNNER_WORKSPACE: dir, GITHUB_STEP_SUMMARY: join(dir, 'summary'), }, }, ); expect(res.status, `${stepName} did not refuse`).not.toBe(0); + expect(res.stdout + res.stderr).toContain( + 'outside the runner workspace', + ); expect( readFileSync(calls, 'utf8'), `${stepName} invoked rm on the escaping path`, ).toBe(''); + expect(readdirSync(join(outside, 'sub'))).toEqual(['canary']); } } finally { rmSync(dir, { recursive: true, force: true }); @@ -1848,6 +1883,207 @@ describe('qwen-triage verify hardening', () => { }, ); + // The wedge the guard itself created, and the hole the first attempt at + // healing it shipped. Both wipes carry the same layer, so both are driven. + const WIPE_STEPS = [ + 'Wipe workspace before external code', + 'Wipe workspace after external code', + ]; + + it.skipIf(!hasGnuRealpath)( + 'heals a workspace a previous job replaced with a symlink', + () => { + // Without the heal this is permanent: canonicalization resolves the + // link to its target, the allowlist refuses, the step exits 1 having + // removed nothing, and every later job on the runner dies at the same + // line. The unlink must take the LINK and leave the target alone — + // which is also what pins the heal judging the canonical PARENT + // rather than $WS itself, since resolving $WS follows the very link + // being removed and would refuse a repair that must succeed. + for (const stepName of WIPE_STEPS) { + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); + const ws = join(parent, 'workspace'); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, ws); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status, `${stepName}: ${res.stdout}${res.stderr}`).toBe(0); + expect(res.stdout + res.stderr).toContain('healing workspace'); + expect(res.stdout + res.stderr).toContain(outside); + expect(lstatSync(ws).isSymbolicLink()).toBe(false); + expect(readdirSync(ws)).toEqual([]); + expect(readdirSync(outside)).toEqual(['canary']); + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + } + }, + ); + + it.skipIf(!hasGnuRealpath)( + 'refuses to heal through an intermediate symlink, before touching anything', + () => { + // The defect the first attempt kept through three rounds: it matched + // the RAW path, and `$RWS/link/sub` matches "$RWS"/* as a string while + // naming a file outside the runner workspace — so the unlink and the + // mkdir landed OUTSIDE, and only then did the allowlist refuse. + for (const stepName of WIPE_STEPS) { + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-inter-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); + writeFileSync(join(outside, 'sub'), 'canary'); + symlinkSync(outside, join(parent, 'link')); + try { + const calls = join(parent, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(parent, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${parent}:${process.env.PATH}`, + GITHUB_WORKSPACE: join(parent, 'link', 'sub'), + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status, stepName).not.toBe(0); + expect(res.stdout + res.stderr).toContain( + 'refusing to heal workspace outside the runner workspace', + ); + expect(readFileSync(calls, 'utf8')).toBe(''); + // The mutation the old shape performed before refusing: the file + // at the resolved target is still a file, with its contents. + expect(lstatSync(join(outside, 'sub')).isFile()).toBe(true); + expect(readFileSync(join(outside, 'sub'), 'utf8')).toBe('canary'); + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + } + }, + ); + + it.skipIf(!hasGnuRealpath)( + 'heals a non-directory workspace, and sees it through a trailing slash', + () => { + // The `|| [ ! -d "$WS" ]` half, and the raw strip that has to precede + // both predicates: `[ -L "$WS/" ]` is false and `[ ! -d "$WS/" ]` + // resolves through the link, so one trailing slash hides the + // corruption entirely. + const stepName = WIPE_STEPS[0]; + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-file-')); + const ws = join(parent, 'workspace'); + writeFileSync(ws, 'not a directory'); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status, res.stdout + res.stderr).toBe(0); + expect(res.stdout + res.stderr).toContain('it was not a directory'); + expect(lstatSync(ws).isDirectory()).toBe(true); + } finally { + rmSync(parent, { recursive: true, force: true }); + } + + const linkParent = mkdtempSync(join(tmpdir(), 'verify-heal-slash-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); + const linkWs = join(linkParent, 'workspace'); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, linkWs); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: `${linkWs}/`, + RUNNER_WORKSPACE: linkParent, + GITHUB_STEP_SUMMARY: join(linkParent, 'summary'), + }, + }, + ); + expect(res.status, res.stdout + res.stderr).toBe(0); + expect(lstatSync(linkWs).isSymbolicLink()).toBe(false); + expect(readdirSync(outside)).toEqual(['canary']); + } finally { + rmSync(linkParent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!hasGnuRealpath || process.getuid?.() === 0)( + 'fails closed when the corrupt workspace cannot be unlinked', + () => { + // A swallowed `rm -f` failure would let the mkdir and the wipe run on + // a path that is still a symlink. Root bypasses the mode bits, so the + // fixture cannot produce the refusal there. + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-perm-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); + const ws = join(parent, 'workspace'); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, ws); + chmodSync(parent, 0o555); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(WIPE_STEPS[0])], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status).not.toBe(0); + expect(res.stdout + res.stderr).toContain('could not remove'); + expect(lstatSync(ws).isSymbolicLink()).toBe(true); + expect(readdirSync(outside)).toEqual(['canary']); + } finally { + chmodSync(parent, 0o755); + rmSync(parent, { 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 = () => { diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js index 997cf70c81a..71fcf3bec94 100644 --- a/scripts/tests/serve-ab-workflow.test.js +++ b/scripts/tests/serve-ab-workflow.test.js @@ -7,6 +7,7 @@ import { execFileSync, spawnSync } from 'node:child_process'; import { chmodSync, + lstatSync, mkdirSync, mkdtempSync, readdirSync, @@ -88,6 +89,42 @@ describe('serve-ab pre-checkout workspace wipe', () => { expect(wipe.run).not.toContain('|| true'); }); + it('carries the symlink heal, ordered and bounded (#9480)', () => { + // The heal has to sit BEFORE the canonicalization: afterwards the path + // has already resolved to the link's target, the allowlist refuses it, + // and that refusal removes nothing — the wedge. Order is the property, + // so it is asserted as one, not as the presence of two strings. + const healAt = wipe.run.indexOf('[ -L "$WS" ] || [ ! -d "$WS" ]'); + const canonAt = wipe.run.indexOf('realpath -m -- "$WS"'); + const rwsAt = wipe.run.indexOf('RWS="${RUNNER_WORKSPACE:?}"'); + expect(healAt).toBeGreaterThan(-1); + expect(healAt).toBeLessThan(canonAt); + // …and AFTER the allowlist root is prepared, since that root is what + // bounds the heal. A raw, empty $RUNNER_WORKSPACE would degenerate the + // containment pattern to the match-all `/*`. + expect(rwsAt).toBeLessThan(healAt); + // The raw strip has to precede the predicates: `[ -L "$WS/" ]` and + // `[ ! -d "$WS/" ]` both resolve THROUGH the link and report its target. + expect(wipe.run.indexOf('while [ "${WS%/}" != "$WS" ]')).toBeLessThan( + healAt, + ); + // Containment is judged on the canonical PARENT, never on $WS itself — + // resolving $WS would follow the very link being removed, and a raw + // match cannot see intermediate symlink components. + expect(wipe.run).toContain( + 'HEAL_PARENT="$(realpath -m -- "$(dirname -- "$WS")" 2>/dev/null)"', + ); + expect(wipe.run).toContain('"$RWS"|"$RWS"/*)'); + expect(wipe.run).toContain('refusing to heal workspace outside'); + // Both legs fail closed: under `-e` a failure that is not the last + // command of an && list is swallowed, and a swallowed one here leaves + // the wipe running against a corrupt path. + expect(wipe.run).toContain('rm -f -- "$WS" || {'); + expect(wipe.run).toContain('mkdir -- "$WS" || {'); + // The incident leaves no other trace. + expect(wipe.run).toContain('::warning::healing workspace'); + }); + it.skipIf(!hasGnuRealpath)( 'wipes a legitimate workspace inside the runner workspace', () => { @@ -180,22 +217,26 @@ describe('serve-ab pre-checkout workspace wipe', () => { }); it.skipIf(!hasGnuRealpath)( - 'refuses an allowlist-escaping symlink via canonicalization', + 'refuses an allowlist-escaping path reached through an intermediate symlink', () => { - // 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. + // Pins the canonicalization line. The bad paths above all sit outside + // the recorder dir, so the allowlist refuses them identically whether + // canonicalization runs or not, and a raw '..' spelling is refused by + // the '..' arm first — neither can pin it. The vector that can is a + // path whose INTERMEDIATE component is a link out of the runner + // workspace: it matches "$RWS"/* as a string and names a directory + // outside it. + // + // It is deliberately a directory at the far end, not the link itself: + // a workspace that IS a link is now healed rather than refused + // (#9480), and this test exists for the refusal, not the heal. + // Executed mutant (canonicalization line deleted): the raw path + // passes the allowlist and find, resolving the link through the + // kernel, hands the outside directory's entries to 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'); + mkdirSync(join(outside, 'sub')); + writeFileSync(join(outside, 'sub', 'canary'), 'x'); symlinkSync(outside, join(dir, 'link')); try { const calls = join(dir, 'rm-calls'); @@ -213,16 +254,17 @@ describe('serve-ab pre-checkout workspace wipe', () => { 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'), + GITHUB_WORKSPACE: join(dir, 'link', 'sub'), RUNNER_WORKSPACE: dir, }, }, ); expect(res.status).not.toBe(0); + expect(res.stdout + res.stderr).toContain( + 'outside the runner workspace', + ); expect(readFileSync(calls, 'utf8')).toBe(''); + expect(readdirSync(join(outside, 'sub'))).toEqual(['canary']); } finally { rmSync(dir, { recursive: true, force: true }); rmSync(outside, { recursive: true, force: true }); @@ -379,4 +421,195 @@ describe('serve-ab pre-checkout workspace wipe', () => { } }, ); + + // The wedge this heal exists for, and the hole the first attempt at it + // shipped. Both are exec fixtures: the guard runs for real, and where a + // regression would delete something, `rm` is a PATH-fronted recorder so + // the assertion is on the decision and nothing on the machine can be lost. + const rmRecorder = (dir) => { + const calls = join(dir, 'rm-calls'); + writeFileSync(calls, ''); + writeFileSync( + join(dir, 'rm'), + `#!/bin/sh\nprintf '%s\\n' "$*" >> '${calls}'\nexit 0\n`, + { mode: 0o755 }, + ); + return calls; + }; + + it.skipIf(!hasGnuRealpath)( + 'heals a workspace a previous job replaced with a symlink', + () => { + // Without the heal this is a permanent wedge: canonicalization + // resolves the link to its target, the allowlist refuses, the step + // exits 1 having removed nothing, and every later job on the runner + // dies at the same line. The unlink must take the LINK and leave the + // target alone. + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-heal-')); + const outside = mkdtempSync(join(tmpdir(), 'serve-ab-heal-outside-')); + const ws = join(parent, 'repo'); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, ws); + try { + const out = runWipe({ GITHUB_WORKSPACE: ws, RUNNER_WORKSPACE: parent }); + expect(out).toContain('healing workspace'); + expect(out).toContain(outside); + expect(lstatSync(ws).isSymbolicLink()).toBe(false); + expect(lstatSync(ws).isDirectory()).toBe(true); + expect(readdirSync(ws)).toEqual([]); + // The link was removed, not followed: the target keeps its contents. + expect(readdirSync(outside)).toEqual(['canary']); + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!hasGnuRealpath)( + 'refuses to heal through an intermediate symlink, before touching anything', + () => { + // The defect the first attempt at this layer kept through three + // rounds: it matched the RAW path, and a raw `"$RWS"/*` accepts + // `$RWS/link/sub` as a string while the kernel resolves it to a file + // outside the runner workspace — so the unlink and the mkdir landed + // OUTSIDE, and only then did canonicalization and the allowlist + // refuse the wipe. + // + // What this fixture pins is the containment arm: deleting it lets the + // heal act here. It does NOT discriminate parent-from-self — resolving + // $WS itself also lands outside and also refuses. The fixture that + // separates the two is the legitimate heal above, where judging $WS + // resolves through the link being removed and refuses a repair that + // must succeed. + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-heal-inter-')); + const outside = mkdtempSync(join(tmpdir(), 'serve-ab-heal-outside-')); + writeFileSync(join(outside, 'sub'), 'canary'); + symlinkSync(outside, join(parent, 'link')); + try { + const calls = rmRecorder(parent); + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', wipe.run], + { + encoding: 'utf8', + env: { + ...process.env, + PATH: `${parent}:${process.env.PATH}`, + GITHUB_WORKSPACE: join(parent, 'link', 'sub'), + RUNNER_WORKSPACE: parent, + }, + }, + ); + expect(res.status).not.toBe(0); + expect(res.stdout + res.stderr).toContain( + 'refusing to heal workspace outside the runner workspace', + ); + // Nothing was deleted, and the file at the resolved target is still + // a file — the mutation the old shape performed before refusing. + expect(readFileSync(calls, 'utf8')).toBe(''); + expect(lstatSync(join(outside, 'sub')).isFile()).toBe(true); + expect(readFileSync(join(outside, 'sub'), 'utf8')).toBe('canary'); + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!hasGnuRealpath)( + 'heals a workspace that is not a directory', + () => { + // The other half of the predicate: a leftover regular file where the + // workspace should be wedges the step exactly the same way, and it is + // the half the first attempt left untested. + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-heal-file-')); + const ws = join(parent, 'repo'); + writeFileSync(ws, 'not a directory'); + try { + const out = runWipe({ GITHUB_WORKSPACE: ws, RUNNER_WORKSPACE: parent }); + expect(out).toContain('it was not a directory'); + expect(lstatSync(ws).isDirectory()).toBe(true); + } finally { + rmSync(parent, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!hasGnuRealpath)( + 'sees the corruption through a trailing-slash spelling', + () => { + // `[ -L "$WS/" ]` is false and `[ ! -d "$WS/" ]` resolves through the + // link, so without the raw strip ahead of the predicates the heal + // never fires and the wedge survives one keystroke. + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-heal-slash-')); + const outside = mkdtempSync(join(tmpdir(), 'serve-ab-heal-outside-')); + const ws = join(parent, 'repo'); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, ws); + try { + runWipe({ GITHUB_WORKSPACE: `${ws}/`, RUNNER_WORKSPACE: parent }); + expect(lstatSync(ws).isSymbolicLink()).toBe(false); + expect(readdirSync(outside)).toEqual(['canary']); + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!hasGnuRealpath)( + 'leaves an ordinary workspace untouched by the heal', + () => { + // The heal must cost nothing on the path every real run takes. + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-heal-noop-')); + const ws = join(parent, 'repo'); + mkdirSync(ws); + writeFileSync(join(ws, 'leftover'), 'x'); + try { + const out = runWipe({ GITHUB_WORKSPACE: ws, RUNNER_WORKSPACE: parent }); + expect(out).not.toContain('healing workspace'); + expect(readdirSync(ws)).toEqual([]); + } finally { + rmSync(parent, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!hasGnuRealpath || process.getuid?.() === 0)( + 'fails closed when the corrupt workspace cannot be unlinked', + () => { + // A swallowed `rm -f` failure would let the mkdir and the wipe run on + // a path that is still a symlink. Root bypasses the mode bits, so the + // fixture cannot produce the refusal there. + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-heal-perm-')); + const outside = mkdtempSync(join(tmpdir(), 'serve-ab-heal-outside-')); + const ws = join(parent, 'repo'); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, ws); + chmodSync(parent, 0o555); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', wipe.run], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + }, + }, + ); + expect(res.status).not.toBe(0); + expect(res.stdout + res.stderr).toContain('could not remove'); + expect(lstatSync(ws).isSymbolicLink()).toBe(true); + expect(readdirSync(outside)).toEqual(['canary']); + } finally { + chmodSync(parent, 0o755); + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); }); From c4a9ac921f1d58517b058e3235d7281fcaae5f99 Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 20 Aug 2026 07:45:40 +0800 Subject: [PATCH 2/4] fix(ci): keep the heal's log out of the workflow-command channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the first review round on this layer. The heal logged the symlink's target inside a `::warning::` line. The target is bytes a PREVIOUS job chose — on the verify lane that job may have run a contributor's code — and the runner parses `::` at the start of any stdout line as a workflow command, so a target of $'…\n::error::forged' let the step reporting the corruption forge an annotation. The annotation now carries no untrusted bytes: the target is stripped of line breaks, capped, and printed on its own prefixed line, where a leading `::` cannot begin a command. Verified against the real step text — the forged line lands as data, and no output line starts with `::error::`. The mkdir leg's refusal had no executed fixture while its `rm -f` sibling had one. It does not need a permission trick: `rm -f` returns 0 for a path whose parent is not a directory (it reads as "already absent"), and the mkdir that follows cannot succeed — so the branch is reachable, and a swallowed failure there would run the wipe against a path that does not exist. Fixtures in both suites, and it runs as root too. And the post-run triage copy's header still said this copy "predates the checkout-heal hardening and never received it" while carrying the whole guard plus the heal directly underneath. That header is the in-code inventory the eventual convergence of these copies will read; understating it is how a sync strips layers in the wrong direction. --- .github/workflows/qwen-triage.yml | 37 ++++++++--- .github/workflows/serve-ab.yml | 12 +++- scripts/tests/qwen-triage-workflow.test.js | 72 ++++++++++++++++++++++ scripts/tests/serve-ab-workflow.test.js | 59 ++++++++++++++++++ 4 files changed, 172 insertions(+), 8 deletions(-) diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index cce0d3f2cb8..0c24246d1e6 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -2636,7 +2636,17 @@ jobs: # The incident this heal exists for leaves no other trace: say what # was found, and where it pointed, before it is gone. if [ -L "$WS" ]; then - echo "::warning::healing workspace ${WS}: it was a symlink to $(readlink -- "$WS" 2>/dev/null || echo "")" + # The target is bytes a PREVIOUS job chose — on this pool that + # job may have run a contributor's code — and the runner parses + # `::` at the start of any stdout line as a workflow command. A + # target of $'x\n::error::forged' would therefore forge an + # annotation. Keep untrusted bytes off the command line itself, + # strip the line breaks that could start a new one, and cap the + # length. + heal_target="$(readlink -- "$WS" 2>/dev/null || printf '%s' '')" + heal_target="$(printf '%s' "$heal_target" | tr -d '\r\n' | cut -c1-200)" + echo "::warning::healing workspace ${WS}: it was a symlink" + printf 'heal: %s pointed at %s\n' "$WS" "$heal_target" else echo "::warning::healing workspace ${WS}: it was not a directory" fi @@ -4836,11 +4846,14 @@ 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. + # Same guard as the pre-run wipe above, layer for layer: raw + # trailing-slash strip, RUNNER_WORKSPACE allowlist root, symlink + # heal, canonicalize, strip, denylist, allowlist. Both copies now + # carry the checkout-heal hardening (#9277) and its heal (#9480); + # this header is the in-code inventory a future convergence of the + # wipe copies will read, so it must not understate what is here. + # See that step's comments for what each layer catches; the suite + # pins this copy's behavior on its own. WS="${GITHUB_WORKSPACE:?}" # Strip trailing slashes on the RAW path, before anything reads it: # `[ -L "$WS/" ]` and `[ ! -d "$WS/" ]` both resolve THROUGH a link @@ -4879,7 +4892,17 @@ jobs: # The incident this heal exists for leaves no other trace: say what # was found, and where it pointed, before it is gone. if [ -L "$WS" ]; then - echo "::warning::healing workspace ${WS}: it was a symlink to $(readlink -- "$WS" 2>/dev/null || echo "")" + # The target is bytes a PREVIOUS job chose — on this pool that + # job may have run a contributor's code — and the runner parses + # `::` at the start of any stdout line as a workflow command. A + # target of $'x\n::error::forged' would therefore forge an + # annotation. Keep untrusted bytes off the command line itself, + # strip the line breaks that could start a new one, and cap the + # length. + heal_target="$(readlink -- "$WS" 2>/dev/null || printf '%s' '')" + heal_target="$(printf '%s' "$heal_target" | tr -d '\r\n' | cut -c1-200)" + echo "::warning::healing workspace ${WS}: it was a symlink" + printf 'heal: %s pointed at %s\n' "$WS" "$heal_target" else echo "::warning::healing workspace ${WS}: it was not a directory" fi diff --git a/.github/workflows/serve-ab.yml b/.github/workflows/serve-ab.yml index e54ccf3f0a9..fad2571d8d0 100644 --- a/.github/workflows/serve-ab.yml +++ b/.github/workflows/serve-ab.yml @@ -125,7 +125,17 @@ jobs: # The incident this heal exists for leaves no other trace: say what # was found, and where it pointed, before it is gone. if [ -L "$WS" ]; then - echo "::warning::healing workspace ${WS}: it was a symlink to $(readlink -- "$WS" 2>/dev/null || echo "")" + # The target is bytes a PREVIOUS job chose — on this pool that + # job may have run a contributor's code — and the runner parses + # `::` at the start of any stdout line as a workflow command. A + # target of $'x\n::error::forged' would therefore forge an + # annotation. Keep untrusted bytes off the command line itself, + # strip the line breaks that could start a new one, and cap the + # length. + heal_target="$(readlink -- "$WS" 2>/dev/null || printf '%s' '')" + heal_target="$(printf '%s' "$heal_target" | tr -d '\r\n' | cut -c1-200)" + echo "::warning::healing workspace ${WS}: it was a symlink" + printf 'heal: %s pointed at %s\n' "$WS" "$heal_target" else echo "::warning::healing workspace ${WS}: it was not a directory" fi diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 7e01da30bfd..4f40699124d 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -2046,6 +2046,78 @@ describe('qwen-triage verify hardening', () => { }, ); + it.skipIf(!hasGnuRealpath)( + 'keeps a forged workflow command in the symlink target out of the log', + () => { + // On this lane the previous job may have run a contributor's code, and + // the runner parses `::` at the start of ANY stdout line as a workflow + // command — so a target of $'…\n::error::forged' would forge an + // annotation from the very step reporting the corruption. + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-inject-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); + const ws = join(parent, 'workspace'); + symlinkSync(`${outside}\n::error::forged-annotation`, ws); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(WIPE_STEPS[0])], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status, res.stdout + res.stderr).toBe(0); + const out = res.stdout + res.stderr; + expect(out).toContain('healing workspace'); + expect(out).toContain('pointed at'); + for (const line of out.split('\n')) { + expect(line.startsWith('::error::')).toBe(false); + } + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!hasGnuRealpath)( + 'fails closed when the healed workspace cannot be recreated', + () => { + // The mkdir leg's own refusal, reachable without a permission trick: + // `rm -f` returns 0 for a path whose parent is not a directory, and + // the mkdir that follows cannot succeed. Swallowed, the wipe would run + // against a path that does not exist. + for (const stepName of WIPE_STEPS) { + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-mkdir-')); + writeFileSync(join(parent, 'file'), 'not a directory'); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: join(parent, 'file', 'sub'), + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, + }, + ); + expect(res.status, stepName).not.toBe(0); + expect(res.stdout + res.stderr).toContain('could not recreate'); + } finally { + rmSync(parent, { recursive: true, force: true }); + } + } + }, + ); + it.skipIf(!hasGnuRealpath || process.getuid?.() === 0)( 'fails closed when the corrupt workspace cannot be unlinked', () => { diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js index 71fcf3bec94..51488334846 100644 --- a/scripts/tests/serve-ab-workflow.test.js +++ b/scripts/tests/serve-ab-workflow.test.js @@ -612,4 +612,63 @@ describe('serve-ab pre-checkout workspace wipe', () => { } }, ); + + it.skipIf(!hasGnuRealpath)( + 'keeps a forged workflow command in the symlink target out of the log', + () => { + // The target is bytes a previous job chose, and the runner parses `::` + // at the start of ANY stdout line as a workflow command — so a target + // of $'…\n::error::forged' would forge an annotation from a step that + // is reporting corruption. The annotation carries no untrusted bytes + // and the target is printed on a prefixed line with its newlines gone. + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-heal-inject-')); + const outside = mkdtempSync(join(tmpdir(), 'serve-ab-heal-outside-')); + const ws = join(parent, 'repo'); + symlinkSync(`${outside}\n::error::forged-annotation`, ws); + try { + const out = runWipe({ GITHUB_WORKSPACE: ws, RUNNER_WORKSPACE: parent }); + expect(out).toContain('healing workspace'); + // The target is still reported — just never as a command. + expect(out).toContain('pointed at'); + expect(out).toContain(basename(outside)); + for (const line of out.split('\n')) { + expect(line.startsWith('::error::')).toBe(false); + } + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + it.skipIf(!hasGnuRealpath)( + 'fails closed when the healed workspace cannot be recreated', + () => { + // The mkdir leg's own refusal, reachable without a permission trick: + // `rm -f` returns 0 for a path whose parent is not a directory (it + // reads as "already absent"), and the mkdir that follows cannot + // succeed. Swallowed, the wipe would then run against a path that + // does not exist. + const parent = mkdtempSync(join(tmpdir(), 'serve-ab-heal-mkdir-')); + writeFileSync(join(parent, 'file'), 'not a directory'); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', wipe.run], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: join(parent, 'file', 'sub'), + RUNNER_WORKSPACE: parent, + }, + }, + ); + expect(res.status).not.toBe(0); + expect(res.stdout + res.stderr).toContain('could not recreate'); + } finally { + rmSync(parent, { recursive: true, force: true }); + } + }, + ); }); From b19bead2f558da954976346d53b4d7dca5d15b46 Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 20 Aug 2026 05:21:55 +0000 Subject: [PATCH 3/4] test(ci): drive both wipe copies in the remaining single-step heal fixtures --- scripts/tests/qwen-triage-workflow.test.js | 205 +++++++++++---------- 1 file changed, 105 insertions(+), 100 deletions(-) diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 4f40699124d..5671b20339c 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -1992,56 +1992,57 @@ describe('qwen-triage verify hardening', () => { // both predicates: `[ -L "$WS/" ]` is false and `[ ! -d "$WS/" ]` // resolves through the link, so one trailing slash hides the // corruption entirely. - const stepName = WIPE_STEPS[0]; - const parent = mkdtempSync(join(tmpdir(), 'verify-heal-file-')); - const ws = join(parent, 'workspace'); - writeFileSync(ws, 'not a directory'); - try { - const res = spawnSync( - 'bash', - ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], - { - encoding: 'utf8', - env: { - ...process.env, - GITHUB_WORKSPACE: ws, - RUNNER_WORKSPACE: parent, - GITHUB_STEP_SUMMARY: join(parent, 'summary'), + for (const stepName of WIPE_STEPS) { + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-file-')); + const ws = join(parent, 'workspace'); + writeFileSync(ws, 'not a directory'); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, }, - }, - ); - expect(res.status, res.stdout + res.stderr).toBe(0); - expect(res.stdout + res.stderr).toContain('it was not a directory'); - expect(lstatSync(ws).isDirectory()).toBe(true); - } finally { - rmSync(parent, { recursive: true, force: true }); - } + ); + expect(res.status, `${stepName}: ${res.stdout}${res.stderr}`).toBe(0); + expect(res.stdout + res.stderr).toContain('it was not a directory'); + expect(lstatSync(ws).isDirectory()).toBe(true); + } finally { + rmSync(parent, { recursive: true, force: true }); + } - const linkParent = mkdtempSync(join(tmpdir(), 'verify-heal-slash-')); - const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); - const linkWs = join(linkParent, 'workspace'); - writeFileSync(join(outside, 'canary'), 'x'); - symlinkSync(outside, linkWs); - try { - const res = spawnSync( - 'bash', - ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], - { - encoding: 'utf8', - env: { - ...process.env, - GITHUB_WORKSPACE: `${linkWs}/`, - RUNNER_WORKSPACE: linkParent, - GITHUB_STEP_SUMMARY: join(linkParent, 'summary'), + const linkParent = mkdtempSync(join(tmpdir(), 'verify-heal-slash-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); + const linkWs = join(linkParent, 'workspace'); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, linkWs); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: `${linkWs}/`, + RUNNER_WORKSPACE: linkParent, + GITHUB_STEP_SUMMARY: join(linkParent, 'summary'), + }, }, - }, - ); - expect(res.status, res.stdout + res.stderr).toBe(0); - expect(lstatSync(linkWs).isSymbolicLink()).toBe(false); - expect(readdirSync(outside)).toEqual(['canary']); - } finally { - rmSync(linkParent, { recursive: true, force: true }); - rmSync(outside, { recursive: true, force: true }); + ); + expect(res.status, `${stepName}: ${res.stdout}${res.stderr}`).toBe(0); + expect(lstatSync(linkWs).isSymbolicLink()).toBe(false); + expect(readdirSync(outside)).toEqual(['canary']); + } finally { + rmSync(linkParent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } } }, ); @@ -2053,34 +2054,36 @@ describe('qwen-triage verify hardening', () => { // the runner parses `::` at the start of ANY stdout line as a workflow // command — so a target of $'…\n::error::forged' would forge an // annotation from the very step reporting the corruption. - const parent = mkdtempSync(join(tmpdir(), 'verify-heal-inject-')); - const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); - const ws = join(parent, 'workspace'); - symlinkSync(`${outside}\n::error::forged-annotation`, ws); - try { - const res = spawnSync( - 'bash', - ['-e', '-o', 'pipefail', '-c', extractRun(WIPE_STEPS[0])], - { - encoding: 'utf8', - env: { - ...process.env, - GITHUB_WORKSPACE: ws, - RUNNER_WORKSPACE: parent, - GITHUB_STEP_SUMMARY: join(parent, 'summary'), + for (const stepName of WIPE_STEPS) { + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-inject-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); + const ws = join(parent, 'workspace'); + symlinkSync(`${outside}\n::error::forged-annotation`, ws); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, }, - }, - ); - expect(res.status, res.stdout + res.stderr).toBe(0); - const out = res.stdout + res.stderr; - expect(out).toContain('healing workspace'); - expect(out).toContain('pointed at'); - for (const line of out.split('\n')) { - expect(line.startsWith('::error::')).toBe(false); + ); + expect(res.status, `${stepName}: ${res.stdout}${res.stderr}`).toBe(0); + const out = res.stdout + res.stderr; + expect(out).toContain('healing workspace'); + expect(out).toContain('pointed at'); + for (const line of out.split('\n')) { + expect(line.startsWith('::error::')).toBe(false); + } + } finally { + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); } - } finally { - rmSync(parent, { recursive: true, force: true }); - rmSync(outside, { recursive: true, force: true }); } }, ); @@ -2124,34 +2127,36 @@ describe('qwen-triage verify hardening', () => { // A swallowed `rm -f` failure would let the mkdir and the wipe run on // a path that is still a symlink. Root bypasses the mode bits, so the // fixture cannot produce the refusal there. - const parent = mkdtempSync(join(tmpdir(), 'verify-heal-perm-')); - const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); - const ws = join(parent, 'workspace'); - writeFileSync(join(outside, 'canary'), 'x'); - symlinkSync(outside, ws); - chmodSync(parent, 0o555); - try { - const res = spawnSync( - 'bash', - ['-e', '-o', 'pipefail', '-c', extractRun(WIPE_STEPS[0])], - { - encoding: 'utf8', - env: { - ...process.env, - GITHUB_WORKSPACE: ws, - RUNNER_WORKSPACE: parent, - GITHUB_STEP_SUMMARY: join(parent, 'summary'), + for (const stepName of WIPE_STEPS) { + const parent = mkdtempSync(join(tmpdir(), 'verify-heal-perm-')); + const outside = mkdtempSync(join(tmpdir(), 'verify-heal-outside-')); + const ws = join(parent, 'workspace'); + writeFileSync(join(outside, 'canary'), 'x'); + symlinkSync(outside, ws); + chmodSync(parent, 0o555); + try { + const res = spawnSync( + 'bash', + ['-e', '-o', 'pipefail', '-c', extractRun(stepName)], + { + encoding: 'utf8', + env: { + ...process.env, + GITHUB_WORKSPACE: ws, + RUNNER_WORKSPACE: parent, + GITHUB_STEP_SUMMARY: join(parent, 'summary'), + }, }, - }, - ); - expect(res.status).not.toBe(0); - expect(res.stdout + res.stderr).toContain('could not remove'); - expect(lstatSync(ws).isSymbolicLink()).toBe(true); - expect(readdirSync(outside)).toEqual(['canary']); - } finally { - chmodSync(parent, 0o755); - rmSync(parent, { recursive: true, force: true }); - rmSync(outside, { recursive: true, force: true }); + ); + expect(res.status, stepName).not.toBe(0); + expect(res.stdout + res.stderr).toContain('could not remove'); + expect(lstatSync(ws).isSymbolicLink()).toBe(true); + expect(readdirSync(outside)).toEqual(['canary']); + } finally { + chmodSync(parent, 0o755); + rmSync(parent, { recursive: true, force: true }); + rmSync(outside, { recursive: true, force: true }); + } } }, ); From ee8fd40dee385cc4b1a97be9206fb69d3d375757 Mon Sep 17 00:00:00 2001 From: Qwen Autofix Date: Thu, 20 Aug 2026 09:13:40 +0000 Subject: [PATCH 4/4] fix(ci): keep the Serve A/B job from timing out on slow runners --- .github/workflows/serve-ab.yml | 5 ++++- scripts/tests/serve-ab-workflow.test.js | 11 ++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/.github/workflows/serve-ab.yml b/.github/workflows/serve-ab.yml index fad2571d8d0..3000a1372fe 100644 --- a/.github/workflows/serve-ab.yml +++ b/.github/workflows/serve-ab.yml @@ -58,7 +58,10 @@ jobs: # other fork PRs stay on ephemeral hosted runners. Keep in sync with # ci.yml's classify_pr routing. Kill-switch: MAINTAINER_ECS_RUNNER_DISABLED. runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'' && (github.event.pull_request.head.repo.full_name == github.repository || contains(fromJSON(''["OWNER","MEMBER","COLLABORATOR"]''), github.event.pull_request.author_association))) && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' - timeout-minutes: 30 + # Two full checkouts, each npm-ci + build + drive: a healthy run lands + # near twenty minutes, and a slow runner pushed a run past the old + # 30-minute bound, cancelling it. + timeout-minutes: 45 steps: - name: 'Restore workspace ownership' if: "${{ runner.environment == 'self-hosted' }}" diff --git a/scripts/tests/serve-ab-workflow.test.js b/scripts/tests/serve-ab-workflow.test.js index 51488334846..aa7bbd5c365 100644 --- a/scripts/tests/serve-ab-workflow.test.js +++ b/scripts/tests/serve-ab-workflow.test.js @@ -23,7 +23,8 @@ import { parse } from 'yaml'; const workflow = readFileSync('.github/workflows/serve-ab.yml', 'utf8'); -const steps = parse(workflow).jobs['ab'].steps; +const job = parse(workflow).jobs['ab']; +const steps = job.steps; const WIPE = 'Wipe stale workspace before checkout'; const wipe = steps.find((s) => s.name === WIPE); @@ -64,6 +65,14 @@ describe('serve-ab pre-checkout workspace wipe', () => { expect(wipe.if).toBe("${{ runner.environment == 'self-hosted' }}"); }); + // The job drives BOTH checkouts end-to-end (npm ci, full monorepo build, + // daemon drive, each); a healthy run lands near twenty minutes, so the + // old 30-minute bound left a slow runner no headroom and the run timed + // out as CANCELLED. Pin the floor — dropping it back re-cancels the run. + it('keeps a job timeout with headroom for two full build cycles', () => { + expect(job['timeout-minutes']).toBeGreaterThanOrEqual(45); + }); + 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 {} +`.