From 68c9fa27fb9c240d18b35d9bff8cacde74112744 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 3 Nov 2026 14:13:59 +0800 Subject: [PATCH] feat(verify-pr): add local Docker recipe with a host-side model proxy (#16077) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(verify-pr): add local Docker recipe with a host-side model proxy A local /verify-pr round needs the same isolation the CI lane provides, but the skill only documented the boundary check, not how to run the model credential through it. Add scripts/verify-pr-model-proxy.mjs, a host-side relay that holds the real key and lets the container call the model with a per-run bearer token, mirroring the CI lane's inline proxy (POST /chat/completions only, nonce health check, per-chunk idle watchdog). The listen address is a flag so the proxy can bind the Docker bridge, and readiness is printed as one JSON line. The SKILL recipe pairs it with a docker run template: the token travels as QWEN_VERIFY_PROXY_TOKEN (not OPENAI_API_KEY, which the isolation gate refuses), and the container's provider entry reads it via envKey. The CI lane keeps its inline copy — the proxy holds the real key, so it must not execute a script from the PR-head checkout. Verified: 11 new pin tests plus all existing verify-pr suites (120 passed), and a real docker smoke — the gate passes inside node:22-bookworm with the proxy token, refuses OPENAI_API_KEY with exit 4, and a container POST reaches a mock upstream with the real key injected and the /v1 prefix preserved. * fix(verify-pr): harden the model proxy after adversarial audit Audit round 1 found the relay forwarded client hop-by-hop headers (connection, transfer-encoding, expect, proxy-authorization, ...) to the upstream, which RFC 7230 forbids and which fights undici's own framing for the relayed stream. Strip them, with a chunked-body test that pins the behaviour. Round 1 also found the CLI entry had no coverage: add tests for the missing-config exit code and the printed JSON coordinates (token override, base-path prefix, no real-key leak, health nonce), and document that the printed baseUrl uses the listen address, so binding 0.0.0.0 yields a URL clients cannot dial. Rounds 2 and 3 (open-ended re-read plus adversarial passes over the updated diff) found nothing further. All verify-pr suites: 123 passed. * fix(verify-pr): address review findings in the model proxy and checker Round-1 review (12 inline findings at 70aed891bf). Proxy: abort the upstream when the client disconnects before headers instead of paying for a completion nobody reads (R1-1); make the idle watchdog pause-aware, re-arm on drain, and match the client's own 240 s stream tolerance (R1-3); bracket IPv6 listen hosts via an exported formatter and normalize base paths once (R1-12). Checker: add --context so a host-held snapshot pins the expected head instead of the writable artifact copy, and refuse an unusable snapshot rather than silently falling back (R1-9). Each guard is pinned by a test that was verified to go red when the guard is removed (six mutations: pre-header abort, pause guard, per-chunk re-arm, body error handler, IPv6 bracket, --context). * docs(verify-pr): harden the local container recipe and record the design Recipe: run the key-holding proxy and the checker from a trusted checkout, never the PR tree (R1-6, R1-9); background the proxy with a pid file and stop it at the end (R1-4); read the key instead of inlining it (R1-5); use a named detached container driven by docker exec (R1-7); require a standalone clone, the host uid and a fresh HOME (R1-8); mount the full host-held metadata snapshot rather than the four-key artifact copy (R1-10); and attest only inside, after rejecting credential-shaped ignored files (R1-11). Add the bilingual design record (R1-2), cross-link it from the local-round design, and pin the recipe's trust lines in tests so reverting them turns the suite red. * fix(verify-pr): address round-2 verification findings Round 2 at 3509c2d1aa reported one Low and four Suggestions. - Relay the exact /chat/completions path. A suffix match also forwarded /admin/chat/completions to the upstream. - Pin the response content-encoding strip with a gzip upstream: undici hands the proxy decoded bytes, so forwarding the header makes the client gunzip plain text. - Reject credential-shaped ignored entries at any depth (a nested secrets/.env included) instead of only a root .env/.env.*. - Name the round's engine as the released CLI and offer the tree build for a round that must exercise the PR's own CLI. The CI inline copies keep their existing two-header strip: that lane is untouched by design and a change there needs its own workflow review. Tests: 141 passed. Three mutations verified red (exact path, gzip header, nested credential pathspec). The review-pr check at this PR is red because the bot lane exhausted ten retries on "429 rpm exceeded", not because of this diff. * fix(verify-pr): close round-3 trust-boundary findings Round 3 at 3ccf38697d reported two Criticals against the recipe, one fix-induced Critical, and four Suggestions. - prepare, the credential scan, the isolation gate and the checker now all come from ; prepare runs with the PR clone as its cwd so the HEAD check still applies (R2-1). - The base commit's .qwen is extracted and mounted read-only over the tree, so the round's instructions and project settings cannot come from the PR (R2-2). - The attestation is passed to the shell that starts the round, because every docker exec is a fresh shell (R1-11, fix-induced). - The proxy token is exported from proxy.json and passed with a bare -e; the file is chmod 600 and removed at cleanup (R2-3). - Connection-nominated hop-by-hop headers are stripped (R2-6). - Recipe pins now enumerate every checker call site, require a non-empty docker-run slice, and seed the writable checker snapshot so a fallback regression is caught (R2-4, R2-5). - New verify-pr-credential-scan.mjs lists ignored files one by one, refuses credential-shaped names at any depth, and leaves node_modules and dist alone — replacing the pathspec check that refused every installed or built tree (round-3 Low). Tests: 148 passed; ten mutations verified red. * fix(verify-pr): address round-4 review findings Four P2 findings at 441a523927: - The credential scan now limits git enumeration with credential pathspecs and depth-aware node_modules/dist excludes, so an installed tree's multi-megabyte ignored listing never overflows the output buffer. - Those excludes apply inside workspace packages too, so packages/*/dist and packages/*/node_modules are not mistaken for project credentials. - The recipe waits for proxy.json to carry a token and validates the exported value, so a truncated background write cannot reach Docker. - The pre-header disconnect test waits until the mock upstream has recorded the request before destroying the downstream socket, instead of racing the proxy's own (correct) early abort. Tests: 150 passed; three mutations verified red (enumeration pathspecs, excludes, readiness loop). * fix(verify-pr): refuse nested repositories in the credential scan Round-4 verification at 19b14b1a8f left three Suggestions. - A nested git repository is now refused instead of silently skipped: git does not descend into it, so a .env under tmp/pr-clone/ would pass. A vendored .git under node_modules/dist stays excluded like the files there. - isExcluded is removed: the pathspecs already exclude those directories, and the surviving JS filter was unpinnable. - The spaced Connection-nomination list is pinned; dropping trim made the proxy answer 502. - The node_modules/dist credential exclusions are documented as deliberate. Tests: 152 passed; two mutations verified red (nested refusal, trim). * fix(verify-pr): address round-5 review findings Four Criticals: - The key entry works without a TTY: an interactive shell prompts, an agent's shell reads REVIEW_OPENAI_KEY_FILE and fails with a named error. - prepare disables fsmonitor, hooks and credential helpers on the PR-controlled clone, and the scan refuses credential-bearing remote URLs. - The proxy token is re-derived from proxy.json in the docker run step, because each step is its own shell. - The credential scan covers .env~, *.p12, id_rsa*, gha-creds-*, and the other credential families the repo's own .gitignore names. Suggestions: - Guard call site moved to , defined for watch mode. - The newest verify comment is snapshotted as previous-report.md. - The real key is unset once the proxy owns it; the token is unset at cleanup. - `{1..100}` replaces seq for macOS; --user and the proxy token's non-credential status are pinned; the scan abort is structural (`|| exit 1`). - Step 6 wording matches the base-pin; the docs narrow the base-pin scope for root AGENTS.md/QWEN.md, list the real CI divergence, explain content-encoding in English, fix the zh token wording, and record the checker --context contract and the ownership attribution. - The readiness gate compares the health nonce; --port is removed. Test gaps closed: gzip header assertion, mid-body disconnect, reset event, PUT, bare-base exact path, positive header passthrough, 502 transport failure, full paused-stream equality, token shape, publish --context pin, checker second positional, scan path fixtures, headers-only idle window. Tests: 159 passed; twelve mutations verified red. * fix(verify-pr): close round-5 verification findings Two Lows reproduced at b5c717a5d5: - prepare's hardening named clean filters but only disabled fsmonitor, hooks and credential helpers. It now enumerates every local filter.*.clean/smudge/process and empties it with a -c override before any git read; a planted-filter fixture proves the override with a positive control that fires without it. - The nested-repository refusal over-refused installed trees whose node_modules holds a real vendored git repo (git reports the directory entry, which the exclude pathspec cannot match). The enumeration is now segment-filtered in JS, so vendored dependencies stay excluded while a nested project repository is still refused. Suggestions and nits: - The remote-URL guard reads url.*.insteadOf as well as remote.*.url and refuses any http(s) userinfo, including a token used as the username; ssh://git@ stays benign. - Public keys (.pub) are no longer treated as credentials, and a backup.env~ fixture pins the trailing-tilde family. Tests: 163 passed; six mutations verified red (filter overrides, vendored segment filter, userinfo breadth, insteadOf enumeration, public-key exclusion, backup.env~ pathspec). * fix(verify-pr): close round-6 verification findings Co-authored-by: Qwen-Coder * fix(verify-pr): close round-7 verification findings Co-authored-by: Qwen-Coder * fix(verify-pr): address round-8 review findings Co-authored-by: Qwen-Coder * fix(verify-pr): close round-9 review findings Co-authored-by: Qwen-Coder * fix(verify-pr): close round-10 review findings Co-authored-by: Qwen-Coder --------- Co-authored-by: Qwen-Coder --- .qwen/skills/verify-pr/SKILL.md | 301 +++++- docs/design/verify-pr-local-model-proxy.md | 209 ++++ .../verify-pr-local-model-proxy.zh-CN.md | 44 + docs/design/verify-pr-local-round.md | 10 +- docs/design/verify-pr-local-round.zh-CN.md | 7 +- scripts/tests/verify-pr-check.test.js | 74 ++ .../tests/verify-pr-credential-scan.test.js | 702 ++++++++++++++ scripts/tests/verify-pr-model-proxy.test.js | 906 ++++++++++++++++++ scripts/tests/verify-pr-prepare.test.js | 314 ++++++ scripts/verify-pr-check.mjs | 81 +- scripts/verify-pr-credential-scan.mjs | 329 +++++++ scripts/verify-pr-model-proxy.mjs | 345 +++++++ scripts/verify-pr-prepare.mjs | 158 ++- 13 files changed, 3432 insertions(+), 48 deletions(-) create mode 100644 docs/design/verify-pr-local-model-proxy.md create mode 100644 docs/design/verify-pr-local-model-proxy.zh-CN.md create mode 100644 scripts/tests/verify-pr-credential-scan.test.js create mode 100644 scripts/tests/verify-pr-model-proxy.test.js create mode 100644 scripts/verify-pr-credential-scan.mjs create mode 100644 scripts/verify-pr-model-proxy.mjs diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index 5b07abd2ba7..4c3256099f3 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -20,8 +20,10 @@ Two entry paths share the contract below. supplies `$QWEN_VERIFY_CONTEXT`, an already-built tree and the time budget, and publishes the report. There is nothing to prepare. - **Maintainer round on a workstation** — `/verify-pr `, starting with - `node scripts/verify-pr-prepare.mjs --repo /` (the - preflight requires the repository explicitly; `origin` is not a fallback). + `cd && node /scripts/verify-pr-prepare.mjs --repo /` + (the trusted copy, with the PR clone only as its + cwd; see the container recipe below). The preflight requires the repository + explicitly; `origin` is not a fallback. It writes the round's coordinates (`repo`, `pr`, `baseRefOid`, `headRefOid`) to `context.json` and refuses a tree that is not a clean checkout of the PR head — when it passes, the current tree is the head side of the A/B, so stay @@ -35,8 +37,9 @@ Two entry paths share the contract below. `/verify-pr ` proves the change and posts nothing. `--publish` adds the report comment and its image evidence; `--watch` installs a standing trigger. Before publishing a local round, run -`node scripts/verify-pr-check.mjs `; it applies the consistency -rules the publisher enforces in CI. +`node /scripts/verify-pr-check.mjs --context ` +from a checkout the PR cannot write; it applies the +consistency rules the publisher enforces in CI. ## Environment contract (CI verify job) @@ -145,8 +148,8 @@ profiles, or `gh` token. Do not run it in an ordinary working copy on a maintainer's machine; if that isolation is unavailable, ask the maintainer to trigger the sandboxed `@qwen-code /verify` lane instead. -Run `node scripts/verify-pr-prepare.mjs --repo /` on the host -before anything else: it fetches the metadata snapshot, resolves and verifies +Run `cd && node /scripts/verify-pr-prepare.mjs --repo /` +on the host before anything else: it fetches the metadata snapshot, resolves and verifies both OIDs against this clone, refuses a tree that is not a clean, fully materialised checkout of the PR head (a wrong HEAD names the `git checkout` to run; uncommitted changes name the entries and a stash form that clears @@ -157,9 +160,11 @@ creates `tmp/pr-verify-/` with a `context.json` holding `repo`, `pr`, `baseRefOid` and `headRefOid`, and reports the isolation signals it can see. It does not gate on them — resolving metadata needs `gh`, which the boundary excludes, so this path always runs on the host. -The boundary check belongs inside: run -`node scripts/verify-pr-prepare.mjs --require-isolation` as the round's first -command there, and treat exit 4 as "this is not a boundary". A container +The boundary check belongs inside: run the trusted copy of the preflight with +`--require-isolation` as the round's first command there, and treat exit 4 as +"this is not a boundary". The Docker recipe below mounts that copy at +`/trusted-scripts/verify-pr-prepare.mjs`; a VM boundary runs the same file +from the trusted checkout instead. A container marker alone does not pass: the CLI's own Docker/Podman launcher mounts `~/.config/gcloud`, the ADC file and every model API key it can see, so the check also requires `QWEN_VERIFY_CREDENTIAL_FREE=1` — set it only after @@ -205,6 +210,258 @@ intended `owner/repo` is a match to confirm, not a guess to avoid. `--watch` installs a standing trigger instead of running once; the contract is in **Watch mode** below. +## Local container recipe (Docker + model proxy) + +The boundary and the model credential are separate problems: the container +isolates PR code, and a host-side proxy keeps the real model key out of the +container. `scripts/verify-pr-model-proxy.mjs` is that proxy — the same relay +rules as the CI lane's inline one (only `POST /chat/completions`, a per-run +bearer token, a per-chunk idle watchdog), plus hop-by-hop request header +stripping. The CI lane deliberately keeps its own inline copy. + +Two checkouts are involved, and they are not interchangeable: + +- `` — a checkout of `main` (or another trusted revision) that + the PR cannot write. Every script the round runs on the host or inside the + boundary comes from here. +- `` — a **standalone** clone checked out at the PR head. The round + runs here, so its `.git` lives inside the tree; a linked worktree's gitfile + points at an absolute host path the container cannot see. Prepare is given + this tree as its cwd and refuses any other HEAD. + +Never execute a script from ``: the proxy, the prepare preflight, +the credential scan, the isolation gate, the publish guard and the checker all +come from ``. A PR can rewrite a script in its own tree, and +these steps run with the real key, the maintainer's `gh` access, or the +authority to certify the round. + +1. **Prepare from the trusted copy with the PR clone as its cwd**, then copy + the full snapshot out of the tree and base-pin `.qwen`: + + ```bash + cd && node /scripts/verify-pr-prepare.mjs --repo / + rm -rf ~/.cache/qwen-verify/pr/qwen-base + mkdir -p ~/.cache/qwen-verify/pr/qwen-base + cp /tmp/verify-context/pr.json ~/.cache/qwen-verify/pr/pr.json + ME="$(gh api user --jq .login)" || { echo "could not read the gh user" >&2; exit 1; } + PREV="$(gh api "repos///issues//comments" --paginate --slurp \ + | node -e 'let d="";process.stdin.on("data",c=>d+=c).on("end",()=>{const all=JSON.parse(d).flat();const hit=all.filter(c=>(/bot$/.test(c.user.login)||c.user.login===process.argv[1])&&c.body.split("\n\n")[0].includes("Verdict:")).pop();process.stdout.write(hit?hit.body+"\n":"")})' "$ME")" || + { echo "could not read PR comments" >&2; exit 1; } + if [ -n "$PREV" ]; then printf '%s\n' "$PREV" > ~/.cache/qwen-verify/pr/previous-report.md + else rm -f ~/.cache/qwen-verify/pr/previous-report.md; fi + BASE_OID="$(node -e 'const fs=require("node:fs"); console.log(JSON.parse(fs.readFileSync(process.argv[1], "utf8")).baseRefOid)' ~/.cache/qwen-verify/pr/pr.json)" + git -C archive "$BASE_OID" -- .qwen \ + | tar -x -C ~/.cache/qwen-verify/pr/qwen-base + ``` + + prepare resolves the tree from its cwd, so the trusted copy runs with + `cwd=` and still verifies that this tree is the PR head. The + artifact directory's `context.json` carries only the four coordinates; the + round reads the full gh snapshot (title, body, author, commits) from + `$QWEN_VERIFY_CONTEXT`, and the newest substantive verify comment lands + beside it as `previous-report.md` so a follow-up round keeps its + carried-forward findings. That file exists only when a report was actually + found — its presence is the follow-up trigger, so an empty file would + misclassify every first round — and the selector matches the first + paragraph of a bot's or your own report, not a reply quoting `Verdict:`. + The `.qwen` extraction starts from an empty directory, so a file the base + deleted or renamed cannot survive from an earlier round. The host copy is + also what pins the expected head after the round, because PR code can + rewrite anything inside `` — including `.git/config`. After a + round the clone is PR-controlled: re-clone before another round instead of + trusting it again, and note that prepare's host-side git commands disable + `core.fsmonitor`, hooks, credential helpers and `core.excludesFile`, and + empty every locally defined clean/smudge/process filter — the enumeration + follows `include.path`, reads key names NUL-framed and re-validated (a + filter value with a newline must not forge overrides), covers + `.git/config.worktree` when `extensions.worktreeConfig` is set, and the + fixed hardening is applied last so nothing outranks it. A filter name + containing `=` is refused outright: git splits `-c` at the first `=`, so + such a name cannot be safely disabled by an override. Prepare also + refuses a `.git/info/exclude` with active patterns: no `-c` can neutralize + that file, and a fresh clone has only the commented default. + Mounting the base commit's `.qwen` over the tree (step 3) keeps the + round's instructions and project settings out of PR control, as the CI + lane's base-pin does; the root `AGENTS.md`/`QWEN.md` are not covered and + still come from the PR clone. + +2. **Start the proxy from the trusted checkout, in the background**, with the + key read rather than typed onto the command line (a literal assignment + lands in shell history). An interactive shell may prompt; an agent's shell + has no TTY, so the key comes from `REVIEW_OPENAI_KEY_FILE` (default + `~/.config/qwen-verify/review-openai-key`, mode 600): + + ```bash + if [ -t 0 ]; then + read -rsp 'REVIEW_OPENAI_API_KEY: ' REVIEW_OPENAI_API_KEY; echo + else + # `read` returns 1 at EOF even when it assigned the variable, and key + # files written by `security find-generic-password -w` / `op read` / + # `printf '%s'` carry no trailing newline — judge the variable, not the + # exit status. + IFS= read -r REVIEW_OPENAI_API_KEY \ + < "${REVIEW_OPENAI_KEY_FILE:-$HOME/.config/qwen-verify/review-openai-key}" || true + [ -n "$REVIEW_OPENAI_API_KEY" ] || + { echo "REVIEW_OPENAI_API_KEY is unset and no key file was readable; set the file named by REVIEW_OPENAI_KEY_FILE" >&2; exit 1; } + fi + export REVIEW_OPENAI_API_KEY + REVIEW_OPENAI_BASE_URL= \ + node /scripts/verify-pr-model-proxy.mjs \ + --listen > ~/.cache/qwen-verify/pr/proxy.json 2> ~/.cache/qwen-verify/pr/proxy.err & + echo $! > ~/.cache/qwen-verify/pr/proxy.pid + unset REVIEW_OPENAI_API_KEY + for _ in {1..100}; do + node -e 'const j=JSON.parse(require("node:fs").readFileSync(process.argv[1], "utf8")); if (!j.token) process.exit(1)' ~/.cache/qwen-verify/pr/proxy.json 2>/dev/null && break + sleep 0.1 + done + chmod 600 ~/.cache/qwen-verify/pr/proxy.json + node -e 'const fs=require("node:fs"); const j=JSON.parse(fs.readFileSync(process.argv[1], "utf8")); const u=new URL(j.baseUrl); fetch(`http://${u.hostname}:${u.port}/__health`).then(r=>r.text()).then(t=>process.exit(t===j.nonce?0:1)).catch(()=>process.exit(1))' ~/.cache/qwen-verify/pr/proxy.json || + { echo "proxy health check failed"; exit 1; } + ``` + + Bind the Docker bridge address, not `127.0.0.1` — a container cannot reach + the host's loopback. On Docker Desktop (macOS/Windows) the bridge lives + inside the VM and has no host-bindable address: this recipe is tested on a + Linux host, and on Desktop you must bind an address the VM can reach and + dial that same address from the container. The bearer check stays + mandatory: the port is visible to every container on the bridge. Keep the + proxy running for the whole round and stop it at the end — it holds the + real key. The background process truncates `proxy.json` before it prints, + so the loop waits for a complete line with a token; stderr goes to a + separate `proxy.err`, because one diagnostic line folded into the JSON + file would break every parse of it. The nonce is checked at the origin + because the printed `baseUrl` carries the upstream path prefix. The key is + unset as soon as the proxy owns its copy, so later diagnostics cannot leak + it, and the token stays in the file rather than this shell. + +3. **Start the boundary as a named, detached container** so every later step + can be issued without a TTY. Each step is its own shell: the token is + re-derived from `proxy.json` here, not inherited from step 2. + + ```bash + export QWEN_VERIFY_PROXY_TOKEN="$(node -e 'const fs=require("node:fs"); console.log(JSON.parse(fs.readFileSync(process.argv[1], "utf8")).token)' ~/.cache/qwen-verify/pr/proxy.json)" + test -n "$QWEN_VERIFY_PROXY_TOKEN" || { echo "proxy.json has no token" >&2; exit 1; } + docker run -d --rm --name qwen-verify- \ + --user "$(id -u):$(id -g)" -e HOME=/tmp/qwen-home \ + -v :/work \ + -v /scripts:/trusted-scripts:ro \ + -v ~/.cache/qwen-verify/pr/qwen-base/.qwen:/work/.qwen:ro \ + -v ~/.cache/qwen-verify/pr:/verify-context:ro \ + -e QWEN_VERIFY_CONTEXT=/verify-context/pr.json \ + -e QWEN_VERIFY_PROXY_TOKEN \ + -w /work node:22-bookworm sleep infinity + ``` + + No host `HOME`, no `gh` token, no `SANDBOX_MOUNTS`. The token travels as + `QWEN_VERIFY_PROXY_TOKEN`, **not** `OPENAI_API_KEY`: `--require-isolation` + refuses any non-empty `OPENAI_API_KEY` (it cannot tell a proxy token from + a real key), and this name is not a credential signal. Running as the host + uid keeps files written into the mounted tree — including the round's + artifacts — owned by the maintainer, not root. + +4. **Point the provider at the proxy** in the container's fresh + `$HOME/.qwen/settings.json`; the runtime reads the credential from + `envKey`, so no `OPENAI_API_KEY` is needed: + + ```bash + docker exec -i qwen-verify- bash -lc 'mkdir -p "$HOME/.qwen" && cat > "$HOME/.qwen/settings.json"' <<'JSON' + { + "security": { "auth": { "selectedType": "openai" } }, + "model": { "name": "" }, + "modelProviders": { + "openai": [ + { + "id": "", + "envKey": "QWEN_VERIFY_PROXY_TOKEN", + "baseUrl": "" + } + ] + } + } + JSON + ``` + +5. **Attest inside, after trusted checks — never on the `docker run` line.** + The gate only sees known credential names and home paths, and gitignored + files are invisible to its clean-tree check, so scan for project + credential files and run the gate from the trusted copies first: + + ```bash + docker exec -i qwen-verify- bash -lc ' + node /trusted-scripts/verify-pr-credential-scan.mjs /work || exit 1 + export QWEN_VERIFY_CREDENTIAL_FREE=1 + node /trusted-scripts/verify-pr-prepare.mjs --require-isolation + ' + ``` + + The scan names credential-shaped ignored files (nested ones included, but + not public keys; the private-key families cover RSA, Ed25519, ECDSA and + DSA) and credential-bearing git config. It inspects every local config + value rather than a list of keys — any value can hide a token URL, + `http.proxy` included — and refuses http(s) userinfo (a token used as the + username counts; the bare `git@` account name, in any spelling, carries + no secret and stays benign, like `ssh://git@`) plus `Authorization` / + `Proxy-Authorization` header values. The URL-bearing key families are + judged too: `url.*.insteadOf` / `url.*.pushInsteadOf` rewrites and + `http..*` sections. The config enumeration follows `include.path` + and covers the enabled `.git/config.worktree` scope, so a credential in + an included worktree file or the worktree config is refused too. The + file is optional: absent means an empty scope, while an unreadable or + malformed one still refuses. It leaves installed dependencies and build + output alone — a vendored git dependency under `node_modules`/`dist` is + excluded, not refused — and refuses a nested git repository outright on + every surface (ignored clone, untracked clone, populated submodule): git + does not descend into it, so a credential file inside one would + otherwise pass silently. The abort is structural (`|| exit 1`), not a + side effect of `set -e`. Exit 4 means this is not a boundary: stop and + run nothing else. + +6. **Run the round inside**, installing the CLI into the fresh home because + the container is not root. Each `docker exec` is a new shell, so the + attestation is passed to the shell that starts the round; it stays off the + `docker run` line. The round's engine is the released CLI, and the PR + clone supplies the code under test; the round's instructions come from the + base commit's `.qwen` mounted in step 3, not from the PR. Root + `AGENTS.md`/`QWEN.md` do come from the PR clone — when the diff touches + them, treat the round's context as PR-influenced: + + ```bash + docker exec -i -e QWEN_VERIFY_CREDENTIAL_FREE=1 qwen-verify- bash -lc ' + set -e + export PATH="$HOME/.local/bin:$PATH" + npm install -g --prefix "$HOME/.local" @qwen-code/qwen-code + qwen --auth-type openai --approval-mode yolo \ + --prompt "/verify-pr --repo /" + ' + ``` + + To run the tree's own CLI instead, replace the install line with a build + and a local install after resolving the dependencies: + + ```bash + corepack pnpm install --frozen-lockfile + npm run build && npm run bundle + npm install -g --prefix "$HOME/.local" /work + ``` + +7. **After the round, check and clean up from the host.** The checker and its + expected head both come from outside the round tree: + + ```bash + node /scripts/verify-pr-check.mjs \ + --context ~/.cache/qwen-verify/pr/pr.json + docker rm -f qwen-verify- + kill "$(cat ~/.cache/qwen-verify/pr/proxy.pid)" + rm -f ~/.cache/qwen-verify/pr/proxy.json ~/.cache/qwen-verify/pr/proxy.err ~/.cache/qwen-verify/pr/proxy.pid + unset QWEN_VERIFY_PROXY_TOKEN + ``` + + `` is the artifact directory prepare printed — + `/tmp/pr-verify-`, never `tmp/verify-context`, which also + matches a loose `*verify*` glob. `` throughout this recipe + is `~/.cache/qwen-verify/pr/pr.json`, the host copy step 1 made. + Publishing still needs `gh`, so it stays on the host. + ## Watch mode (local only) `/verify-pr --watch` installs a standing trigger instead of running once: a @@ -220,6 +477,9 @@ directly: the probe also reports comments and failed checks, and every non-empty gate output starts a turn whether or not the head moved. The wrapper forwards only `action: "resync"` (a moved head) and `action: "stop"`. +`` below is the trusted checkout the watch scripts run from; its main +worktree holds the probe cursor under `.qwen/tmp/`. + Build the task in this order and stop at the first failure: 1. **Resolve a binary that carries the probe.** The gate runs in a fresh shell, @@ -423,17 +683,25 @@ _— via Qwen Code /verify-pr (v; skill )_ Resolve the three values from the round's own runtime, never from memory: the model is `$QWEN_CODE_MODEL_IDENTITY` when non-empty, else `$QWEN_CODE_MODEL`; the CLI version is `qwen --version`; the skill hash is -the first 12 characters of -`git hash-object --no-filters .qwen/skills/verify-pr/SKILL.md` — the content -hash of the instructions this round actually loaded, which needs no git -history. When the model cannot be resolved, omit the footer rather than +the first 12 characters of the blob hash of the instructions this round +actually loaded. In the container recipe that is the base-pinned copy, so +hash `git -C rev-parse ":.qwen/skills/verify-pr/SKILL.md"` +(equivalently, `git hash-object --no-filters` of the mounted +`/work/.qwen/skills/verify-pr/SKILL.md`) — never the PR head's copy: the two +blobs differ whenever the PR touches the skill, and hashing the head would +certify instructions the round never ran. A non-container round that loads +its own checkout hashes that file with +`git hash-object --no-filters .qwen/skills/verify-pr/SKILL.md`, which needs +no git history. When the model cannot be resolved, omit the footer rather than post an empty identity — and never claim the CI lane's `sandboxed verification` signature for a workstation round. Assemble the exact comment body — report content plus the resolved footer — into `comment.md` in the artifact directory and post that file: it is what the read-back below compares against, and it gives the checker a place to verify the footer actually landed. The local path has no publisher enforcing the fail/mismatch -rule, so run `node scripts/verify-pr-check.mjs ` and clear its +rule, so run +`node /scripts/verify-pr-check.mjs --context ` +and clear its errors and its verdict-shape and footer warnings before publishing (a missing footer is legitimate only when the model could not be resolved), act on a gates-size warning by recording the suites it ran under `gates` @@ -467,8 +735,9 @@ GitHub rewrites the six-character escape text for NUL and SOH (a backslash, `u`, then `0000` or `0001`) into a caret form, inside code spans and fences too. -**Run both halves of that guard through `scripts/verify-pr-guard.mjs`, because -the hand-run version fails open.** `--pr --repo / --snapshot +**Run both halves of that guard through +`/scripts/verify-pr-guard.mjs`, because the hand-run version +fails open.** `--pr --repo / --snapshot ` at round start and again before posting, then `--compare --start --now `; it exits 1 on anything dirty. A fetch that errors returns no items, so "is anything new?" answers "no" and a hand-rolled comparison reads clean — diff --git a/docs/design/verify-pr-local-model-proxy.md b/docs/design/verify-pr-local-model-proxy.md new file mode 100644 index 00000000000..ce572571fa1 --- /dev/null +++ b/docs/design/verify-pr-local-model-proxy.md @@ -0,0 +1,209 @@ +# Host-side model proxy for a containerised /verify-pr round + +[English](verify-pr-local-model-proxy.md) | [简体中文](verify-pr-local-model-proxy.zh-CN.md) + +Status: implemented. + +## Problem + +The local `/verify-pr` path must execute untrusted PR code inside a +credential-free boundary, and the skill's isolation gate says so. The recipe +stopped at that requirement: it had no way to let the round reach a model +without putting the real key inside the container, no container invocation, +and no host-side shape for the steps that must not run from the PR tree. + +Three concrete failures followed: + +- **The key had no safe path.** Exporting `OPENAI_API_KEY` inside the + container either hands the real credential to PR code or trips the isolation + gate, which refuses any non-empty `OPENAI_API_KEY` because it cannot tell a + proxy token from a real key. +- **The proxy and the checker are host steps with host privileges.** A + repo-relative `node scripts/...` executed from the round tree runs the PR's + own copy, while the proxy holds the real key and the checker reads the + artifact that certifies the round. The artifact directory is writable by the + code the round just executed. +- **The obvious container invocation does not support the round.** An + interactive `docker run -it` with no name cannot be driven by an agent; + mounting only the tree leaves a linked worktree's absolute gitfile + unreachable and produces root-owned files in the maintainer's checkout; and + the four-key artifact `context.json` is not the full metadata snapshot the + round reads. + +## Proposal + +A host-side relay plus a recipe that names its trust boundaries. + +- `scripts/verify-pr-model-proxy.mjs` listens on a chosen host address (the + Docker bridge for container clients), holds `REVIEW_OPENAI_API_KEY`, and + relays only the exact `${basePath}/chat/completions` path. Clients present a + per-run bearer token (`QWEN_VERIFY_PROXY_TOKEN`); the proxy swaps it for the + real key upstream. A nonce-echoing health endpoint distinguishes the process + from a squatter, and the local recipe compares that nonce before it uses + the proxy. Hop-by-hop request headers — both the fixed set and any name a + `Connection` header nominates — and the upstream `content-encoding` are + stripped; undici has already decoded the body, so forwarding that header + would make the client decompress it twice. The idle watchdog is per-chunk, + matches the qwen client's own 240 s stream tolerance, stays quiet while the + downstream is paused, and re-arms on drain; a disconnect before upstream + headers aborts the fetch instead of paying for a completion nobody reads. +- The recipe uses two checkouts. `` supplies every script the + round runs, including the prepare preflight, the credential scan, the + isolation gate, the publish guard and the post-round checker; `` + is a standalone clone at the PR head that the container mounts and that + prepare is given as its cwd. The clone's `.git` is PR-controlled after a + round, so it is not reused, and host-side git commands disable fsmonitor, + hooks, credential helpers and `core.excludesFile`, and empty every locally + defined clean/smudge/process filter. The enumeration follows + `include.path`, reads key names NUL-framed and re-validated (a filter + value carrying a newline must not forge extra `-c` entries), covers + `.git/config.worktree` when `extensions.worktreeConfig` is set, and the + fixed hardening is applied last so no enumerated entry can outrank it. A + filter name containing `=` cannot be represented by `-c key=value` at all + (git splits the argument at the first `=`), so it is refused with its own + remedy rather than emitted as a broken override. `.git/info/exclude` has + no `-c` override, so active patterns there are refused outright — a fresh + clone ships only the commented default. A linked worktree is rejected by + construction because its gitfile points outside the mount. +- The container is named and detached (`sleep infinity` plus `docker exec`) + and runs with `--user "$(id -u):$(id -g)"` and `HOME=/tmp/qwen-home`, so + files written into the mounted tree stay maintainer-owned; the CLI installs + into the fresh home because there is no writable global prefix. The base + commit's `.qwen` is mounted read-only over the tree, so the round's skill + and project settings cannot come from the PR; the root `AGENTS.md`/`QWEN.md` + are not covered and still come from the PR clone. The full snapshot is + mounted from a host copy outside the tree, and the per-run proxy token is + derived from the proxy's JSON file in the step that starts the container, so + it stays out of shell history. The credential-free attestation is passed to + the shell that starts the round, after `verify-pr-credential-scan.mjs` + (trusted copy) rejects credential-shaped ignored files and + credential-bearing git config. The config guard inspects VALUES, not a + list of keys — any value can hide a token URL (`http.proxy` included), so + a key allowlist never converges — plus the URL-bearing key families + (`url.*.insteadOf` / `url.*.pushInsteadOf` rewrites, `http..*` + sections), following `include.path` and covering the enabled + `.git/config.worktree` scope like prepare's filter enumeration. That file + is optional: absent means an empty scope (an explicit `--worktree --list` + would exit 128 and false-refuse a clean repo), while an existing but + unreadable or malformed one still refuses. + The file scan lists ignored files one by one and leaves + `node_modules`/`dist` alone, so a built tree is not refused; a nested git + repository is refused on every surface git collapses to a directory + entry — ignored clone, untracked clone, populated submodule. The round's + engine is the released CLI unless the tree's own build is installed over + it. +- The round reads `$QWEN_VERIFY_CONTEXT` from the full snapshot. After the + round, the checker runs from `` with `--context `, + so a writable artifact `context.json` cannot pin its own expected head. + +The CI lane keeps its inline proxy: that copy runs in a root-owned step on the +base-pinned workflow, and pointing it at the PR checkout would let a PR read +the real key. + +## Alternatives considered + +- **Export the real key into the container.** Rejected: the boundary exists to + keep it out, and PR code can read its own environment. +- **Reuse the CI heredoc by extracting it and calling it from the workflow.** + Rejected for the CI lane: the script would come from the PR checkout. The + local script is therefore a deliberate second copy. It diverges in more than + transport wiring: it matches the exact path instead of a suffix, strips + `Connection`-nominated headers and `content-encoding`, uses a 240 s + pause-aware idle window instead of the CI copy's fixed 120 s, and aborts a + pre-header disconnect. A sync between the copies must carry those + behavioural differences, not just the flags. +- **Print `0.0.0.0` or normalise the listen host to loopback.** Rejected: the + printed value must be the address clients actually dial. +- **Mount the tree read-only.** Rejected as the only control: the round must + build in the tree and write its artifacts there. The trust boundary is + enforced by running the privileged host steps from `` and by + pinning the expected head from a host-held snapshot. +- **Scan `credential.helper` commands for inline tokens.** Declined as out + of the URL/header contract: a helper is a command, and spotting an inline + token in one means parsing shell. Prepare's own git reads already disable + `credential.helper`, and the recipe re-clones rather than reusing a + PR-controlled `.git`; the residual shape is recorded here rather than + silently dropped. + +## Acceptance + +- Proxies: bearer rejection, exact path allowlist, base-path prefixing, + real-key injection, hop-by-hop stripping with a chunked body (fixed set + and `Connection`-nominated names, with a non-nominated custom header as + the passthrough control), a gzip upstream body with the header stripped, + pre-header disconnect abort, paused-downstream survival, slow-but-healthy + streams, a slow request upload kept alive by the per-chunk re-arm, + upstream reset survival, a genuinely refused dial (bound-then-closed + port, asserted `ECONNREFUSED` — a blocked port would never dial), an + empty or flag-looking `--listen` rejected, IPv6 listen formatting, the + idle default read from the core constant's source rather than restated, + and the CLI's JSON coordinates. +- Credential scan: a nested ignored `.env`, `.env~`, `backup.env~`, `*.pem`, + `*.key`, `*.p12`, `*.pfx`, `id_rsa*`, `id_ed25519*`, `id_ecdsa*`, `id_dsa*`, + `credentials*`, `.git-credentials`, `.npmrc` (at depth too), `.netrc` or + `gha-creds-*.json` is refused (a `.pub` public key is not). The config + guard inspects every local value: any http(s) userinfo is refused, + including a token used as the username (the bare `git@` account name + carries no secret and stays benign in any spelling, like `ssh://git@`), + and so is any `Authorization` / `Proxy-Authorization` header value (other + header names stay clean); the URL-bearing key families (`insteadOf` / + `pushInsteadOf` rewrites, `http..*` sections) are judged on the + embedded URL. A novel key outside any historical list (`custom.mirror`, + `http.proxy`) is the witness that value inspection, not a key allowlist, + is the contract; the refusal names the key, never the value. Credentials + arriving through `include.path` or in the enabled `.git/config.worktree` + scope are refused; a credential-free include and benign worktree config + stay clean, and with the extension disabled the worktree file is ignored + exactly as git ignores it. The worktree file is optional: an enabled + extension with no file (or an empty file) is an empty scope on both the + scan and prepare paths, while an unreadable or malformed file refuses. + Installed `node_modules`/`dist` are ignored at any + depth, including inside a workspace package, and a vendored git + repository under them is excluded rather than refused; file enumeration + is path-limited so a large ignored dependency tree is never buffered + (the buffer fixture's files match a credential pathspec, so dropping the + restriction really overflows it). A nested git repository is refused on + every surface — ignored clone, untracked non-ignored clone, populated + submodule — while an unpopulated gitlink stays clean (its URL is covered + by the value scan); a missing `--ignored` mode or a dropped pattern turns + its test red. +- Host git hardening: prepare disables fsmonitor, hooks, credential helpers + and `core.excludesFile`, and empties every locally defined + clean/smudge/process filter — following `include.path`, NUL-framed key + names re-validated against forgery, `.git/config.worktree` covered when + `extensions.worktreeConfig` is set, fixed hardening applied last — before + reading the PR-controlled tree. Planted-filter fixtures prove the + override for a directly defined filter, one arriving through an included + worktree file, one in the worktree config, and a newline-bearing value + that must not forge a `-c` outranking the hardening, each with a positive + control where the vector fires without the guard. A filter name containing + `=` is refused with its own remedy on the module and the CLI path — no + override can represent it, and the refusal precedes any guarded status. + A hostile + `core.excludesFile` cannot hide untracked files from the hardened status; + active patterns in `.git/info/exclude` are refused with a re-clone + remedy, while the commented default passes. A non-git cwd is refused with + the worktree message (exit 3), not an uncaught enumeration error. +- Checker: `--context` overrides the writable artifact copy, refuses an + unusable snapshot instead of falling back (with a usable writable copy + seeded), and keeps its existing exit codes. +- Recipe: every script names the trusted checkout (including prepare, the + scan, the gate, the guard and the checker), the base commit's `.qwen` is + mounted read-only and extracted into a freshly emptied directory, both + credentials come from files rather than command lines (the key falls back + to `REVIEW_OPENAI_KEY_FILE` when stdin is not a TTY, is judged by the + variable rather than `read`'s EOF status so a newline-less key file works, + and is unset once the proxy owns it), the proxy's coordinates and nonce + are awaited before the container starts with stderr in a separate file so + the JSON stays parseable, the token is re-derived in the `docker run` + step, and the attestation is on the round's shell but not the `docker run` + line. `previous-report.md` is written only when a report was found — its + presence is the follow-up trigger — and selected by first-paragraph marker + from a bot or the maintainer, not by a substring any reply could quote. + The publish footer hashes the base-pinned skill copy the round actually + loaded, never the PR head's; reverting any of those turns a recipe pin + red. +- Mutation evidence: removing each of those guards turns its pin red. +- A real Docker smoke covers the gate passing with `QWEN_VERIFY_PROXY_TOKEN`, + the gate refusing `OPENAI_API_KEY`, and a container request reaching a mock + upstream with the real key injected. diff --git a/docs/design/verify-pr-local-model-proxy.zh-CN.md b/docs/design/verify-pr-local-model-proxy.zh-CN.md new file mode 100644 index 00000000000..2a20857bda5 --- /dev/null +++ b/docs/design/verify-pr-local-model-proxy.zh-CN.md @@ -0,0 +1,44 @@ +# 容器化 /verify-pr 轮次的宿主机模型代理 + +[English](verify-pr-local-model-proxy.md) | [简体中文](verify-pr-local-model-proxy.zh-CN.md) + +状态：已实现。 + +## 问题 + +本地 `/verify-pr` 路径必须在无凭据边界内执行不可信的 PR 代码，skill 的隔离门禁也这样要求。但配方只停在要求上：它没有让轮次访问模型的途径而不把真实 key 放进容器，没有容器启动方式，也没有为「不能从 PR 树运行的宿主步骤」定义形态。 + +由此产生三个具体失败： + +- **key 没有安全通道。** 在容器里导出 `OPENAI_API_KEY`，要么把真实凭据交给 PR 代码，要么触发隔离门禁——门禁拒绝任何非空的 `OPENAI_API_KEY`，因为它无法区分代理 token 与真实 key。 +- **代理与检查器是持宿主权限的宿主步骤。** 从轮次树执行仓库相对路径的 `node scripts/...` 会运行 PR 自己的副本，而代理持有真实 key、检查器读取用于认证本轮产物的文件；产物目录恰恰可被轮次刚执行的代码写入。 +- **显而易见的容器调用方式支撑不了轮次。** 没有名字的交互式 `docker run -it` 无法由 agent 驱动；只挂载树会让链接 worktree 的绝对 gitfile 不可达，并在维护者检出里留下 root 属主文件；四键的产物 `context.json` 也不是轮次读取的完整元数据快照。 + +## 方案 + +一个宿主机侧转发器，加一套点明信任边界的配方。 + +- `scripts/verify-pr-model-proxy.mjs` 监听选定的宿主地址（容器客户端用 Docker 网桥），持有 `REVIEW_OPENAI_API_KEY`，只转发精确的 `${basePath}/chat/completions` 路径。客户端出示每轮一个、整轮有效的 bearer token（`QWEN_VERIFY_PROXY_TOKEN`，非单次使用），代理在上游替换为真实 key。回显 nonce 的健康端点用于区分本进程与抢占者，本地配方在使用代理前比较该 nonce。hop-by-hop 请求头（固定集合与 `Connection` 点名的头）与上游 `content-encoding` 被剥离（undici 已解码，转发该头会让客户端二次解压）。空闲看门狗按块续期，与 qwen 客户端自身的 240 秒流容忍一致；下游暂停期间不中止，drain 时续期；上游响应头之前客户端断开时中止 fetch，而不是为无人读取的补全付费。 +- 配方使用两个检出。`` 提供轮次运行的每个脚本，包括 prepare 预检、凭据扫描、隔离门禁、发布守卫与轮次后的检查器；`` 是停在 PR head 的独立克隆，供容器挂载，并作为 prepare 的 cwd。轮次结束后该克隆的 `.git` 属 PR 控制，不复用；宿主机侧 git 命令禁用 fsmonitor、hooks、credential helper 与 `core.excludesFile`，并把本地定义的 clean/smudge/process filter 全部置空——枚举跟随 `include.path`，以 NUL 分隔只读键名并逐个重新校验（filter 值里的换行不得伪造额外的 `-c` 条目），`extensions.worktreeConfig` 为真时连 `.git/config.worktree` 一起枚举，固定加固项放在最后使任何枚举项都无法压过它。含 `=` 的 filter 名根本无法用 `-c key=value` 表示（git 在第一个 `=` 处切分参数），因此带自己的补救被拒绝，而不是生成一个失效的 override。`.git/info/exclude` 没有 `-c` 可压制，因此其中的有效模式被直接拒绝——新鲜克隆只有注释默认内容。链接 worktree 因其 gitfile 指向挂载外而在构造上被排除。 +- 容器有名字且后台运行（`sleep infinity` 加 `docker exec`），以 `--user "$(id -u):$(id -g)"` 与 `HOME=/tmp/qwen-home` 运行，因此写入挂载树的文件保持维护者所有；CLI 装进新 home 是因为容器没有可写的全局前缀。base 提交的 `.qwen` 只读挂到树上（轮次的 skill 与项目设置不能来自 PR）；根 `AGENTS.md`/`QWEN.md` 不在覆盖范围内，仍来自 PR 克隆。完整快照从树外宿主副本挂载，代理 token 在启动容器的那一步从 JSON 文件重新取得，避免进入 shell history。无凭据宣誓传给启动轮次的 shell，且在可信的 `verify-pr-credential-scan.mjs` 拒绝凭据形状 ignored 文件与带凭据的 git config 之后才设置；config 守卫检查的是值而不是键名列表——任何值都可能藏 token URL（含 `http.proxy`），键名白名单永远不会收敛——外加携带 URL 的键族（`url.*.insteadOf`/`url.*.pushInsteadOf` 改写、`http..*` 段），并与 prepare 的 filter 枚举一样跟随 `include.path`、覆盖启用的 `.git/config.worktree` 作用域；该文件是可选的：缺失即空作用域（显式 `--worktree --list` 会退出 128 误拒干净仓库），存在但不可读或损坏仍拒绝；文件扫描逐文件列出 ignored 条目并放过 `node_modules`/`dist`，因此已构建的树不会被误拒；嵌套 git 仓库在 git 会折叠成目录条目的每个面上都被拒绝——被忽略的克隆、未跟踪的克隆、已填充的子模块。轮次的引擎是发布版 CLI，除非把树内构建装到同一前缀覆盖它。 +- 轮次从完整快照读取 `$QWEN_VERIFY_CONTEXT`。轮次结束后，检查器从 `` 运行并带 `--context <宿主副本>`，因此可写的产物 `context.json` 无法自证其期望 head。 + +CI 车道保留内联代理：那份副本运行在 base-pinned workflow 的 root 步骤里，若改为指向 PR 检出，PR 就能读到真实 key。 + +## 备选方案 + +- **把真实 key 导出进容器。** 否决：边界的存在就是为了把它挡在外面，而 PR 代码能读取自己的环境。 +- **抽取 CI heredoc 并由 workflow 调用。** 对 CI 车道否决：脚本会来自 PR 检出。因此本地脚本是有意的第二份副本。它的差异不止传输接线：精确路径而非后缀、剥离 `Connection` 点名的头与 `content-encoding`、240 秒 pause 感知空闲窗口而非 CI 的固定 120 秒、以及响应头前断连中止。两份同步时必须带上这些行为差异，而不只是参数。 +- **打印 `0.0.0.0`，或把监听地址归一化成 loopback。** 否决：打印值必须是客户端实际拨号的地址。 +- **仅把树挂成只读。** 否决：轮次必须在树里构建并写产物。信任边界靠「特权宿主步骤从 `` 运行」以及「从宿主快照钉住期望 head」来落实。 +- **扫描 `credential.helper` 命令里的内联 token。** 否决，超出 URL/头契约：helper 是命令，从中识别内联 token 需要解析 shell。prepare 自身的 git 读取已禁用 `credential.helper`，配方也要求重新克隆而不是复用 PR 可控的 `.git`；这一残余形态在此记录，而不是悄悄丢掉。 + +## 验收 + +- 代理：bearer 拒绝、精确路径白名单、base path 前缀、真实 key 注入、chunked 请求体下 hop-by-hop 剥离（固定集合与 `Connection` 点名的头，并以未被点名的自定义头作透传正对照）、gzip 上游响应（头被剥离）、响应头前断连中止、下游暂停存活、慢但健康的流、慢请求上传由按块续期保活、上游重置存活、真实被拒的拨号（先绑定后关闭的端口，断言 `ECONNREFUSED`——被屏蔽的端口根本不会拨号）、空值或形似 flag 的 `--listen` 被拒、IPv6 监听格式化、空闲默认值从 core 常量源码读取而非复述，以及 CLI 的 JSON 坐标。 +- 凭据扫描：嵌套 ignored 的 `.env`、`.env~`、`backup.env~`、`*.pem`、`*.key`、`*.p12`、`*.pfx`、`id_rsa*`、`id_ed25519*`、`id_ecdsa*`、`id_dsa*`、`credentials*`、`.git-credentials`、`.npmrc`（含深层目录）、`.netrc`、`gha-creds-*.json` 被拒绝（`.pub` 公钥不算凭据）。config 守卫检查每个本地值：http(s) 的任意 userinfo 都被拒绝（含把 token 当用户名；裸 `git@` 账号名不含秘密，任意大小写拼写都与 `ssh://git@` 一样保持良性），带 `Authorization`/`Proxy-Authorization` 头的值也被拒绝（其他头名保持干净）；携带 URL 的键族（`insteadOf`/`pushInsteadOf` 改写、`http..*` 段）按键中内嵌的 URL 判定。任何历史列表之外的新键（`custom.mirror`、`http.proxy`）是"契约是值检查而非键名白名单"的见证；拒绝消息只点名键、不打印值。经 `include.path` 引入或位于启用的 `.git/config.worktree` 作用域的凭据被拒绝；不含凭据的 include 与良性 worktree 配置保持干净，扩展禁用时 worktree 文件与 git 一样被忽略。worktree 文件是可选的：扩展启用但文件缺失（或为空）在 scan 与 prepare 两路都是空作用域，不可读或损坏的文件仍拒绝。`node_modules`/`dist` 在任意深度（含 workspace 包内）被忽略，其下的 vendored git 仓库被排除而非拒绝；文件枚举由 pathspec 限制，不会先缓冲整棵依赖树（缓冲夹具的文件本身就匹配凭据 pathspec，去掉限制真的会溢出）。嵌套 git 仓库在每个面上都被拒绝——被忽略的克隆、未跟踪且未被忽略的克隆、已填充的子模块——未填充的 gitlink 保持干净（其 URL 由值扫描覆盖）；去掉 `--ignored` 模式或删掉某条模式会让测试变红。 +- 宿主机 git 硬化：prepare 禁用 fsmonitor、hooks、credential helper 与 `core.excludesFile`，并在读取 PR 可控树之前把本地定义的 clean/smudge/process filter 全部置空——枚举跟随 `include.path`、NUL 分隔的键名逐个重新校验防伪造、`extensions.worktreeConfig` 为真时覆盖 `.git/config.worktree`、固定加固项放在最后。植入夹具证明四种 filter 形态的覆盖：直接定义、经被 include 的工作树文件、worktree config、以及值含换行不得伪造压过加固的 `-c`，各带一个不覆盖就会触发的正对照。含 `=` 的 filter 名在模块与 CLI 两路都带自己的补救被拒绝——没有 override 能表示它，且拒绝先于任何加固 status。恶意 `core.excludesFile` 无法对加固后的 status 隐藏未跟踪文件；`.git/info/exclude` 的有效模式以重新克隆为补救被拒绝，注释默认内容放行。非 git 目录以 worktree 消息干净拒绝（exit 3），而不是未捕获的枚举错误。 +- 检查器：`--context` 覆盖可写产物副本、不可用快照时拒绝而不是回退（产物副本先种一个合法 head），并保持既有退出码。 +- 配方：每个脚本都点名可信 checkout（含 prepare、扫描、门禁、守卫与检查器）、base 提交的 `.qwen` 只读挂载且解压到先清空的目录、两个凭据都来自文件而非命令行（stdin 非 TTY 时 key 从 `REVIEW_OPENAI_KEY_FILE` 读取，按变量而非 `read` 的 EOF 返回码判断，因此无结尾换行的 key 文件可用，代理拿到副本后立即 unset）、启动容器前等待代理坐标与 nonce 且 stderr 写入独立文件以保持 JSON 可解析、token 在 `docker run` 那一步重新取得、宣誓在轮次 shell 但不在 `docker run` 行。`previous-report.md` 只在确实找到报告时写入——它的存在就是跟进轮触发器——并按首段标记从 bot 或维护者本人的评论中选取，而不是任何回复都能引用的子串。发布 footer 哈希轮次实际加载的 base-pin skill 副本，绝不哈希 PR head 的；任一被还原都会让配方 pin 变红。 +- 变异证据：移除上述任一守卫，对应 pin 必须变红。 +- 真实 Docker 冒烟：`QWEN_VERIFY_PROXY_TOKEN` 时门禁通过、`OPENAI_API_KEY` 时门禁拒绝、容器请求到达 mock 上游且真实 key 已注入。 diff --git a/docs/design/verify-pr-local-round.md b/docs/design/verify-pr-local-round.md index 8cfb0651b5c..29cd4b766f1 100644 --- a/docs/design/verify-pr-local-round.md +++ b/docs/design/verify-pr-local-round.md @@ -71,7 +71,10 @@ them rather than at the end of the document. self-consistent assertion triple, and no `merge-ready` carrying a failed assertion. Report shape and evidence naming are warnings, because a documentation-only round has no capture and a checker cannot know whether a - harness ran. + harness ran. With `--context ` the expected head comes from that + host-held file instead of the artifact's writable `context.json`, and an + unusable snapshot is a hard exit 1, not a warning: the artifact directory is + writable by the PR code the round just ran. - `scripts/verify-pr-guard.mjs` owns both halves of the pre-publish guard. `--pr --repo / --snapshot ` fetches the head, the PR state and all three comment lists with `--paginate`, and writes the file only @@ -417,6 +420,11 @@ A published verification comment ends with the same attribution shape later preparation into a different artifact directory. Publishable round snapshots contain no PR title or body. +## Related designs + +- The container boundary and its host-side model proxy: + [verify-pr-local-model-proxy.md](verify-pr-local-model-proxy.md). + ## Open questions None at this scope. diff --git a/docs/design/verify-pr-local-round.zh-CN.md b/docs/design/verify-pr-local-round.zh-CN.md index 82551034437..e6e33f667a6 100644 --- a/docs/design/verify-pr-local-round.zh-CN.md +++ b/docs/design/verify-pr-local-round.zh-CN.md @@ -25,7 +25,7 @@ 新增三个独立脚本，并让 skill 在流程需要的位置指向它们，而不是放在文末。 - `scripts/verify-pr-prepare.mjs --repo /` 负责解析坐标：用 lane 相同的字段列表抓取快照、校验两个 OID 在本克隆中存在、拒绝不是 PR head 干净检出的树、创建带 `evidence/` 子目录的产物目录，并报告当前进程能观测到的隔离信号。它不据此设闸：解析元数据需要 `gh`，而 `gh` 与边界互斥，因此这条路径总是在宿主上运行。闸门是 `--require-isolation`，用于在边界内、轮次执行任何东西之前运行。 -- `scripts/verify-pr-check.mjs ` 把发布侧的规则套到本地产物目录：必需文件、四个判定词、为正且自洽的断言三元组、以及不允许带失败断言的 `merge-ready`。报告形态与证据命名只作为警告，因为纯文档轮次没有截图，检查器也无法知道是否跑过 harness。 +- `scripts/verify-pr-check.mjs ` 把发布侧的规则套到本地产物目录：必需文件、四个判定词、为正且自洽的断言三元组、以及不允许带失败断言的 `merge-ready`。报告形态与证据命名只作为警告，因为纯文档轮次没有截图，检查器也无法知道是否跑过 harness。带 `--context ` 时期望 head 来自该宿主持有的文件，而不是产物里可写的 `context.json`；不可用快照是硬性 exit 1 而非警告：产物目录可被轮次刚执行的 PR 代码写入。 - `scripts/verify-pr-guard.mjs` 接管发布前守卫的两个半程。`--pr --repo / --snapshot ` 抓取 head、PR 状态以及三个评论列表（带 `--paginate`），只有每次调用都成功才写文件。`--compare --start --now ` 列出本轮 dirty 的全部原因并退出 1。 检查器在产物契约要求轮次写出的产物上新增三条规则。产物契约里三者都要点名：产物清单没要求的文件，轮次就不会产出，而对一个不存在的文件做检查是静默的。 @@ -97,6 +97,11 @@ - 所有脚本都不执行 PR 代码。 - 对于自带轮次快照的报告，在不同产物目录中准备后续轮次不会改变检查所用的提交号。可发布的轮次快照不包含 PR 标题或正文。 +## 相关设计 + +- 容器边界与它的宿主机模型代理： + [verify-pr-local-model-proxy.zh-CN.md](verify-pr-local-model-proxy.zh-CN.md)。 + ## 未决问题 本范围内无。 diff --git a/scripts/tests/verify-pr-check.test.js b/scripts/tests/verify-pr-check.test.js index 137f5739ce4..79ee22e55e6 100644 --- a/scripts/tests/verify-pr-check.test.js +++ b/scripts/tests/verify-pr-check.test.js @@ -762,4 +762,78 @@ describe.skipIf(process.platform === 'win32')('check (CLI)', () => { 1, ); }); + + // The artifact directory is writable by the PR code the round just ran, so + // its own context.json cannot pin the head. --context reads a copy the round + // never saw and must not fall back to the writable one. + it('pins the head from a host-held snapshot over the artifact copy', () => { + const dir = makeArtifactDir(validFiles(), ['01-ab-cells.png']); + writeFileSync( + join(dir, 'context.json'), + JSON.stringify({ headRefOid: 'e'.repeat(40) }), + ); + const hostSnapshot = join(tmpdir(), `verify-pr-host-${Date.now()}.json`); + tmpDirs.push(hostSnapshot); + writeFileSync(hostSnapshot, JSON.stringify({ headRefOid: HEAD })); + + const pinned = spawnSync( + process.execPath, + [SCRIPT, '--context', hostSnapshot, dir], + { encoding: 'utf8' }, + ); + expect(pinned.status).toBe(0); + expect(pinned.stdout).toContain(`head source: ${hostSnapshot}`); + expect(pinned.stdout).not.toContain('does not name the verified head'); + + const artifactOnly = run(dir); + expect(artifactOnly.status).toBe(0); + expect(artifactOnly.stdout).toContain('does not name the verified head'); + }); + + it('refuses an unusable --context instead of falling back', () => { + const dir = makeArtifactDir(validFiles(), ['01-ab-cells.png']); + // Seed a usable writable copy: a fallback regression would exit 0 here + // and print its head source, so the exit-1 assertion pins the property. + writeFileSync( + join(dir, 'context.json'), + JSON.stringify({ headRefOid: HEAD }), + ); + const empty = join(tmpdir(), `verify-pr-host-empty-${Date.now()}.json`); + tmpDirs.push(empty); + writeFileSync(empty, '{}'); + for (const snapshot of [join(tmpdir(), 'verify-pr-no-such.json'), empty]) { + const result = spawnSync( + process.execPath, + [SCRIPT, '--context', snapshot, dir], + { encoding: 'utf8' }, + ); + expect(result.status).toBe(1); + expect(result.stderr).toContain('--context'); + expect(result.stdout).not.toContain('head source:'); + } + }); + + it('exits 2 for an unknown option or a missing --context value', () => { + const dir = makeArtifactDir(validFiles(), ['01-ab-cells.png']); + expect( + spawnSync(process.execPath, [SCRIPT, '--bogus', dir], { + encoding: 'utf8', + }).status, + ).toBe(2); + expect( + spawnSync(process.execPath, [SCRIPT, '--context'], { + encoding: 'utf8', + }).status, + ).toBe(2); + // A forgotten --context turns the snapshot into a second positional: + // refuse instead of silently using the artifact's writable copy. + const hostSnapshot = join(tmpdir(), `verify-pr-host-${Date.now()}.json`); + tmpDirs.push(hostSnapshot); + writeFileSync(hostSnapshot, JSON.stringify({ headRefOid: HEAD })); + expect( + spawnSync(process.execPath, [SCRIPT, dir, hostSnapshot], { + encoding: 'utf8', + }).status, + ).toBe(2); + }); }); diff --git a/scripts/tests/verify-pr-credential-scan.test.js b/scripts/tests/verify-pr-credential-scan.test.js new file mode 100644 index 00000000000..89499bc289a --- /dev/null +++ b/scripts/tests/verify-pr-credential-scan.test.js @@ -0,0 +1,702 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { execFileSync, spawnSync } from 'node:child_process'; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { afterEach, describe, expect, it } from 'vitest'; +import { + gitScanArgs, + isCredentialShaped, + scanIgnoredCredentials, +} from '../../scripts/verify-pr-credential-scan.mjs'; + +const SCRIPT = fileURLToPath( + new URL('../../scripts/verify-pr-credential-scan.mjs', import.meta.url), +); + +let tmpDirs = []; +afterEach(() => { + for (const dir of tmpDirs) rmSync(dir, { recursive: true, force: true }); + tmpDirs = []; +}); + +const gitEnv = { + ...process.env, + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_SYSTEM: '/dev/null', + GIT_AUTHOR_NAME: 't', + GIT_AUTHOR_EMAIL: 't@example.com', + GIT_COMMITTER_NAME: 't', + GIT_COMMITTER_EMAIL: 't@example.com', +}; + +function makeRepo(files, ignore) { + const dir = mkdtempSync(join(tmpdir(), 'verify-pr-scan-')); + tmpDirs.push(dir); + execFileSync('git', ['init', '-q', '.'], { cwd: dir, env: gitEnv }); + writeFileSync(join(dir, '.gitignore'), `${ignore.join('\n')}\n`); + for (const file of files) { + mkdirSync(join(dir, file, '..'), { recursive: true }); + writeFileSync(join(dir, file), 'secret'); + } + return dir; +} + +const run = (dir) => + spawnSync(process.execPath, [SCRIPT, dir], { encoding: 'utf8' }); + +describe('credential basename matching', () => { + it('matches the shapes a provider key or certificate uses', () => { + for (const path of [ + '.env', + '.env.local', + 'app.env', + 'secrets/prod.env', + '.env~', + 'tls.pem', + 'id.key', + 'auth.p12', + 'tls.pfx', + 'id_rsa', + 'id_ed25519', + 'id_ecdsa', + 'id_dsa', + 'credentials', + 'credentials.json', + '.git-credentials', + 'gha-creds-123.json', + '.npmrc', + '.netrc', + ]) { + expect(isCredentialShaped(path), path).toBe(true); + } + for (const path of [ + 'README.md', + 'package.json', + 'keymap.ts', + 'envelope.ts', + 'environment.md', + 'id_rsa.pub', + ]) { + expect(isCredentialShaped(path), path).toBe(false); + } + }); +}); + +describe('gitScanArgs', () => { + it('enumerates with depth-aware credential pathspecs', () => { + const args = gitScanArgs('/repo'); + expect(args).toContain(':(glob)**/.env'); + expect(args).toContain(':(glob)**/*.pem'); + expect(args).toContain(':(glob)**/*.env~'); + expect(args).toContain(':(glob)**/*.p12'); + expect(args).toContain(':(glob)**/*.pfx'); + expect(args).toContain(':(glob)**/id_rsa*'); + expect(args).toContain(':(glob)**/id_ed25519*'); + expect(args).toContain(':(glob)**/id_ecdsa*'); + expect(args).toContain(':(glob)**/id_dsa*'); + expect(args).toContain(':(glob)**/*credentials*'); + expect(args).toContain(':(glob)**/gha-creds-*.json'); + // Excludes apply at workspace depth, not only the repository root. + expect(args).toContain(':(exclude,glob)**/node_modules/**'); + expect(args).toContain(':(exclude,glob)**/dist/**'); + }); +}); + +describe('scanIgnoredCredentials', () => { + it('reports nested and top-level ignored credential files', () => { + const dir = makeRepo( + [ + 'vault/.env', + 'secrets/prod.env', + '.env~', + '.env.local', + 'tls.pem', + 'secrets/id.key', + 'auth.p12', + 'secrets/tls.pfx', + 'id_rsa', + 'id_rsa.pub', + 'secrets/id_ed25519', + 'secrets/id_ecdsa', + 'secrets/id_dsa', + 'backup.env~', + 'secrets/credentials.json', + '.git-credentials', + 'gha-creds-123.json', + '.npmrc', + 'packages/foo/.npmrc', + '.netrc', + ], + [ + '.env*', + '*.env', + '*.env~', + '*.pem', + '*.key', + '*.p12', + '*.pfx', + 'id_rsa*', + 'id_ed25519*', + 'id_ecdsa*', + 'id_dsa*', + '*credentials*', + '.npmrc', + '.netrc', + 'gha-creds-*.json', + 'vault/', + ], + ); + expect(scanIgnoredCredentials(dir)).toEqual([ + '.env.local', + '.env~', + '.git-credentials', + '.netrc', + '.npmrc', + 'auth.p12', + 'backup.env~', + 'gha-creds-123.json', + 'id_rsa', + 'packages/foo/.npmrc', + 'secrets/credentials.json', + 'secrets/id.key', + 'secrets/id_dsa', + 'secrets/id_ecdsa', + 'secrets/id_ed25519', + 'secrets/prod.env', + 'secrets/tls.pfx', + 'tls.pem', + 'vault/.env', + ]); + }); + + it('refuses a remote URL that carries a credential', () => { + const dir = makeRepo(['src/app.ts'], ['node_modules/']); + execFileSync( + 'git', + [ + 'config', + '--local', + 'remote.origin.url', + 'https://x-access-token:ghp_secret@github.com/o/r.git', + ], + { cwd: dir, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /credential-bearing git config/, + ); + const result = run(dir); + expect(result.status).toBe(1); + expect(result.stderr).toContain('credential-bearing git config'); + }); + + it('refuses a token used as the URL username and insteadOf credentials', () => { + const dir = makeRepo(['src/app.ts'], []); + execFileSync( + 'git', + [ + 'config', + '--local', + 'remote.origin.url', + 'https://ghp_FAKEPAT@github.com/o/r.git', + ], + { cwd: dir, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /credential-bearing git config/, + ); + const dir2 = makeRepo(['src/app.ts'], []); + execFileSync( + 'git', + [ + 'config', + '--local', + 'url.https://x:ghp_FAKEPAT@github.com/.insteadOf', + 'https://github.com/', + ], + { cwd: dir2, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir2)).toThrow( + /credential-bearing git config/, + ); + }); + + // The same credential class as remote.*.url: a push URL is the natural + // place for a PAT, and submodule URLs and the Actions-written extraheader + // are equally readable from inside the tree with plain `git config`. + it('refuses pushurl, submodule URL and Authorization extraheader credentials', () => { + const cases = [ + ['remote.origin.pushurl', 'https://ghp_FAKEPAT@github.com/o/r.git'], + ['submodule.lib.url', 'https://x:ghp_FAKEPAT@github.com/o/lib.git'], + [ + 'http.https://github.com/.extraheader', + 'AUTHORIZATION: basic eC1hY2Nlc3MtdG9rZW46Z2hwX0ZBS0VQQVQ=', + ], + [ + 'http.https://proxy.internal/.extraheader', + 'Proxy-Authorization: Bearer ghp_FAKEPAT', + ], + ]; + for (const [key, value] of cases) { + const dir = makeRepo(['src/app.ts'], []); + execFileSync('git', ['config', '--local', key, value], { + cwd: dir, + env: gitEnv, + }); + expect(() => scanIgnoredCredentials(dir), key).toThrow( + /credential-bearing git config/, + ); + const result = run(dir); + expect(result.status, key).toBe(1); + expect(result.stderr, key).toContain('credential-bearing git config'); + } + }); + + it('keeps a non-Authorization extraheader out of the refusal', () => { + const dir = makeRepo(['src/app.ts'], []); + execFileSync( + 'git', + [ + 'config', + '--local', + 'http.https://github.com/.extraheader', + 'X-Request-Id: trace-123', + ], + { cwd: dir, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir)).toEqual([]); + }); + + // The guard inspects VALUES, not a key allowlist: any config value can + // carry a token URL, and the URL-bearing key families (rewrites, http + // sections) carry theirs in the key. Restoring a fixed key list must turn + // these red — a novel key is the witness that no list converges. + it('refuses credential-bearing values and keys outside any fixed list', () => { + const cases = [ + // pushInsteadOf: the one-word sibling of insteadOf, credential in KEY. + [ + 'url.https://x:ghp_FAKEPAT@github.com/.pushInsteadOf', + 'https://github.com/', + ], + // http.proxy: credential in VALUE, a key no allowlist enumerated. + ['http.proxy', 'http://user:ghp_FAKEPAT@proxy.internal:3128'], + // A novel key entirely: only value inspection can see this. + ['custom.mirror', 'https://ghp_FAKEPAT@mirror.internal/o/r.git'], + // http. section with the token in the KEY, benign value. + ['http.https://ghp_FAKEPAT@github.com/.extraheader', 'X-Thing: 1'], + ]; + for (const [key, value] of cases) { + const dir = makeRepo(['src/app.ts'], []); + execFileSync('git', ['config', '--local', key, value], { + cwd: dir, + env: gitEnv, + }); + expect(() => scanIgnoredCredentials(dir), key).toThrow( + /credential-bearing git config/, + ); + const result = run(dir); + expect(result.status, key).toBe(1); + // The refusal names the key, never the value: when the credential sits + // in the value, the token must not appear in the output at all. Git + // lowercases the variable name but preserves the URL subsection's + // case, so only the last segment is folded here. + const reportedKey = key.replace(/\.[^.]+$/, (segment) => + segment.toLowerCase(), + ); + expect(result.stderr, key).toContain(reportedKey); + if (!key.toLowerCase().includes('ghp_fakepat')) { + expect(result.stderr, key).not.toContain('ghp_FAKEPAT'); + } + } + // A proxy without userinfo stays clean: the widening added recognition, + // not a blanket refusal of the key. + const dir = makeRepo(['src/app.ts'], []); + execFileSync( + 'git', + ['config', '--local', 'http.proxy', 'http://proxy.internal:3128'], + { cwd: dir, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir)).toEqual([]); + }); + + // The credential-side twin of prepare's include.path filter bypass: + // `--local` alone names one file, while the `git config --get` PR code + // uses follows includes — an included credential must be refused too. + it('refuses credentials that arrive through include.path', () => { + const cases = [ + '[remote "origin"]\n\turl = https://ghp_FAKEPAT@github.com/o/r.git\n', + '[remote "origin"]\n\tpushurl = https://ghp_FAKEPAT@github.com/o/r.git\n', + '[http "https://github.com/"]\n\textraheader = AUTHORIZATION: basic eC1hY2Nlc3MtdG9rZW46Z2hwX0ZBS0VQQVQ=\n', + ]; + for (const content of cases) { + const dir = makeRepo(['src/app.ts'], ['tmp/']); + mkdirSync(join(dir, 'tmp'), { recursive: true }); + writeFileSync(join(dir, 'tmp', 'creds.inc'), content); + execFileSync( + 'git', + ['config', '--local', 'include.path', '../tmp/creds.inc'], + { cwd: dir, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir), content).toThrow( + /credential-bearing git config/, + ); + const result = run(dir); + expect(result.status, content).toBe(1); + expect(result.stderr, content).toContain('credential-bearing git config'); + } + // A credential-free include is zero collateral: same shape, benign URL. + const dir = makeRepo(['src/app.ts'], ['tmp/']); + mkdirSync(join(dir, 'tmp'), { recursive: true }); + writeFileSync( + join(dir, 'tmp', 'creds.inc'), + '[remote "origin"]\n\turl = https://github.com/o/r.git\n', + ); + execFileSync( + 'git', + ['config', '--local', 'include.path', '../tmp/creds.inc'], + { cwd: dir, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir)).toEqual([]); + }); + + // Ordinary `git config --get` reads .git/config.worktree when the + // extension is enabled; --local alone would report a credential there as + // clean while PR code reads it. The scan must enumerate the enabled scope. + it('refuses credentials in the enabled worktree config scope', () => { + const dir = makeRepo(['src/app.ts'], []); + execFileSync( + 'git', + ['config', '--local', 'extensions.worktreeConfig', 'true'], + { cwd: dir, env: gitEnv }, + ); + execFileSync( + 'git', + [ + 'config', + '--worktree', + 'http.https://github.com/.extraheader', + 'Authorization: Bearer FAKE_WORKTREE', + ], + { cwd: dir, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /credential-bearing git config/, + ); + expect(run(dir).status).toBe(1); + execFileSync( + 'git', + [ + 'config', + '--worktree', + '--unset', + 'http.https://github.com/.extraheader', + ], + { cwd: dir, env: gitEnv }, + ); + execFileSync( + 'git', + [ + 'config', + '--worktree', + 'remote.origin.url', + 'https://FAKE_WORKTREE@example.invalid/repo.git', + ], + { cwd: dir, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /credential-bearing git config/, + ); + expect(run(dir).status).toBe(1); + execFileSync( + 'git', + ['config', '--worktree', '--unset', 'remote.origin.url'], + { cwd: dir, env: gitEnv }, + ); + // Benign worktree config stays clean. + execFileSync( + 'git', + ['config', '--worktree', 'core.logAllRefUpdates', 'true'], + { cwd: dir, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir)).toEqual([]); + // With the extension disabled git itself ignores the worktree file, and + // so does the scan: the same planted credential reads as clean. + execFileSync( + 'git', + [ + 'config', + '--worktree', + 'remote.origin.url', + 'https://FAKE_WORKTREE@example.invalid/repo.git', + ], + { cwd: dir, env: gitEnv }, + ); + execFileSync( + 'git', + ['config', '--local', '--unset', 'extensions.worktreeConfig'], + { cwd: dir, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir)).toEqual([]); + expect(run(dir).status).toBe(0); + }); + + // Enabling the extension does not require config.worktree to exist: an + // absent optional file is an empty scope, not a refusal (explicit + // `--worktree --list` exits 128 on it). Unreadable or malformed files + // must still refuse — the tolerance is for absence only. + it('treats an absent optional worktree config as an empty scope', () => { + const dir = makeRepo(['src/app.ts'], []); + execFileSync( + 'git', + ['config', '--local', 'extensions.worktreeConfig', 'true'], + { cwd: dir, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir)).toEqual([]); + expect(run(dir).status).toBe(0); + // An existing empty file is the same empty scope. + writeFileSync(join(dir, '.git', 'config.worktree'), ''); + expect(scanIgnoredCredentials(dir)).toEqual([]); + expect(run(dir).status).toBe(0); + // A malformed file refuses on both paths. + writeFileSync(join(dir, '.git', 'config.worktree'), '[broken\n'); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /could not (read local git config|list nested repositories)/, + ); + expect(run(dir).status).toBe(1); + }); + + // POSIX permissions only: on Windows chmod 000 does not block reads. + it.skipIf(process.platform === 'win32')( + 'refuses an unreadable worktree config instead of treating it as absent', + () => { + const dir = makeRepo(['src/app.ts'], []); + execFileSync( + 'git', + ['config', '--local', 'extensions.worktreeConfig', 'true'], + { cwd: dir, env: gitEnv }, + ); + writeFileSync(join(dir, '.git', 'config.worktree'), ''); + chmodSync(join(dir, '.git', 'config.worktree'), 0o000); + try { + expect(() => scanIgnoredCredentials(dir)).toThrow( + /could not (read local git config|list nested repositories)/, + ); + expect(run(dir).status).toBe(1); + } finally { + chmodSync(join(dir, '.git', 'config.worktree'), 0o644); + } + }, + ); + + it('keeps ssh URLs, the bare git username and public keys out of the refusal', () => { + const dir = makeRepo(['id_rsa.pub'], ['id_rsa*']); + execFileSync( + 'git', + [ + 'config', + '--local', + 'remote.origin.url', + 'ssh://git@github.com/o/r.git', + ], + { cwd: dir, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir)).toEqual([]); + // The conventional account name carries no secret on http(s) either; + // refusing it would disagree with ssh://git@ on the same non-secret. + const dir2 = makeRepo(['src/app.ts'], []); + execFileSync( + 'git', + [ + 'config', + '--local', + 'remote.origin.url', + 'https://git@github.com/o/r.git', + ], + { cwd: dir2, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir2)).toEqual([]); + expect(run(dir2).status).toBe(0); + // Account names are case-insensitive: the uppercase spellings carry + // the same non-secret and must not re-introduce the false refusal. + for (const spelling of ['GIT', 'Git']) { + execFileSync( + 'git', + [ + 'config', + '--local', + 'remote.origin.url', + `https://${spelling}@github.com/o/r.git`, + ], + { cwd: dir2, env: gitEnv }, + ); + expect(scanIgnoredCredentials(dir2), spelling).toEqual([]); + expect(run(dir2).status, spelling).toBe(0); + } + // A password after the same username is still a credential. + execFileSync( + 'git', + [ + 'config', + '--local', + 'remote.origin.url', + 'https://git:ghp_FAKEPAT@github.com/o/r.git', + ], + { cwd: dir2, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir2)).toThrow( + /credential-bearing git config/, + ); + }); + + it('does not refuse a vendored real git repository under node_modules', () => { + const dir = makeRepo(['node_modules/pkg/index.js'], ['node_modules/']); + const nested = join(dir, 'node_modules', 'pkg'); + execFileSync('git', ['init', '-q', '.'], { cwd: nested, env: gitEnv }); + execFileSync('git', ['add', '.'], { cwd: nested, env: gitEnv }); + execFileSync('git', ['commit', '-q', '-m', 'dep'], { + cwd: nested, + env: gitEnv, + }); + expect(scanIgnoredCredentials(dir)).toEqual([]); + }); + + it('does not refuse an installed tree or build output at any depth', () => { + const dir = makeRepo( + [ + 'node_modules/pkg/credentials.json', + 'dist/app.pem', + 'packages/foo/node_modules/pkg/credentials.json', + 'packages/foo/dist/credentials.js', + 'packages/foo/dist/app.pem', + 'src/app.ts', + ], + ['node_modules/', 'dist/', '*.pem', 'credentials*'], + ); + expect(scanIgnoredCredentials(dir)).toEqual([]); + }); + + it('limits enumeration with pathspecs instead of buffering the tree', () => { + // The dependency files MATCH a credential pathspec (*credentials*): with + // the pathspecs dropped, this listing overflows the 64 KiB ceiling, so + // the test actually exercises the restriction it names. + const files = Array.from( + { length: 3000 }, + (_, index) => + `packages/foo/node_modules/pkg/pkg-credentials-${index}.json`, + ); + files.push('vault/.env'); + const dir = makeRepo(files, ['node_modules/', 'vault/']); + // A 64 KiB ceiling is far below the dependency listing: only the + // pathspec-restricted output (the nested .env) fits without ENOBUFS. + expect(scanIgnoredCredentials(dir, { maxBuffer: 64 * 1024 })).toEqual([ + 'vault/.env', + ]); + }); + + it('refuses a nested repository instead of silently skipping it', () => { + const dir = makeRepo(['tmp/pr-clone/.env'], ['tmp/']); + execFileSync('git', ['init', '-q', '.'], { + cwd: join(dir, 'tmp', 'pr-clone'), + env: gitEnv, + }); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /nested git repository under tmp\/pr-clone/, + ); + const result = run(dir); + expect(result.status).toBe(1); + expect(result.stderr).toContain('nested git repository'); + }); + + // A round's own `git clone` leaves an UNTRACKED, non-ignored clone: the + // ignored-only enumeration never saw it, and git does not descend into it + // for the credential file scan either. + it('refuses an untracked non-ignored nested clone', () => { + const dir = makeRepo(['src/app.ts'], []); + const nested = join(dir, 'tools', 'clone'); + mkdirSync(nested, { recursive: true }); + execFileSync('git', ['init', '-q', '.'], { cwd: nested, env: gitEnv }); + writeFileSync(join(nested, '.env'), 'OPENAI_API_KEY=sk-from-clone\n'); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /nested git repository under tools\/clone/, + ); + const result = run(dir); + expect(result.status).toBe(1); + expect(result.stderr).toContain('nested git repository'); + }); + + // A populated submodule is a gitlink whose working tree git will not + // descend into; an unpopulated one has no tree to read (its URL is covered + // by the config value scan instead). + it('refuses a populated submodule but not an unpopulated gitlink', () => { + const dir = makeRepo(['src/app.ts'], []); + const nested = join(dir, 'vendor', 'up'); + mkdirSync(nested, { recursive: true }); + execFileSync('git', ['init', '-q', '.'], { cwd: nested, env: gitEnv }); + writeFileSync(join(nested, '.env'), 'OPENAI_API_KEY=sk-from-submodule\n'); + execFileSync('git', ['add', '.'], { cwd: nested, env: gitEnv }); + execFileSync('git', ['commit', '-q', '-m', 'sub'], { + cwd: nested, + env: gitEnv, + }); + const sha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: nested, + env: gitEnv, + encoding: 'utf8', + }).trim(); + execFileSync( + 'git', + ['update-index', '--add', '--cacheinfo', `160000,${sha},vendor/up`], + { cwd: dir, env: gitEnv }, + ); + expect(() => scanIgnoredCredentials(dir)).toThrow( + /nested git repository under vendor\/up/, + ); + expect(run(dir).status).toBe(1); + + // Unpopulated: the gitlink stays, the working tree goes. + rmSync(nested, { recursive: true, force: true }); + expect(scanIgnoredCredentials(dir)).toEqual([]); + expect(run(dir).status).toBe(0); + }); + + it('stays silent on a clean tree', () => { + const dir = makeRepo(['src/app.ts'], ['node_modules/']); + const result = run(dir); + expect(result.status).toBe(0); + expect(result.stdout).toBe(''); + }); + + it('exits 1 and names each entry when one is found', () => { + const dir = makeRepo(['vault/.env'], ['.env', 'vault/']); + const result = run(dir); + expect(result.status).toBe(1); + expect(result.stdout).toContain( + 'credential-shaped ignored entry: vault/.env', + ); + }); + + it('exits 2 without a root and 1 when git cannot list files', () => { + expect( + spawnSync(process.execPath, [SCRIPT], { encoding: 'utf8' }).status, + ).toBe(2); + const notRepo = mkdtempSync(join(tmpdir(), 'verify-pr-scan-nogit-')); + tmpDirs.push(notRepo); + const result = run(notRepo); + expect(result.status).toBe(1); + expect(result.stderr).toMatch( + /could not list (nested repositories|ignored files)/, + ); + }); +}); diff --git a/scripts/tests/verify-pr-model-proxy.test.js b/scripts/tests/verify-pr-model-proxy.test.js new file mode 100644 index 00000000000..3bc718ee11d --- /dev/null +++ b/scripts/tests/verify-pr-model-proxy.test.js @@ -0,0 +1,906 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { spawn, spawnSync } from 'node:child_process'; +import { once } from 'node:events'; +import { readFileSync } from 'node:fs'; +import http from 'node:http'; +import { fileURLToPath } from 'node:url'; +import { gzipSync } from 'node:zlib'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + DEFAULT_IDLE_TIMEOUT_MS, + createModelProxy, + formatLocalBaseUrl, + normalizeBasePath, + parseArgs, +} from '../../scripts/verify-pr-model-proxy.mjs'; + +const SCRIPT = fileURLToPath( + new URL('../../scripts/verify-pr-model-proxy.mjs', import.meta.url), +); + +const TOKEN = 'test-proxy-token'; +const NONCE = 'test-nonce'; +const REAL_KEY = 'real-upstream-key'; + +let cleanups = []; +afterEach(async () => { + for (const cleanup of cleanups) await cleanup(); + cleanups = []; +}); + +function listen(server) { + return new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, '127.0.0.1', () => { + const { port } = server.address(); + cleanups.push( + () => + new Promise((done) => { + server.closeAllConnections(); + server.close(done); + }), + ); + resolve(port); + }); + }); +} + +const sleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +async function waitFor(predicate, timeoutMs = 1000) { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (predicate()) return; + await sleep(5); + } + throw new Error('condition not met before timeout'); +} + +/** + * A local upstream stand-in: records the request and the sequence it took to + * answer (or to be aborted). No outbound network is involved. + */ +async function startUpstream({ + status = 200, + chunks = ['data: ok\n\n'], + delayMs = 0, + chunkDelayMs = 0, + resetAfterFirstChunk = false, + respectBackpressure = false, + gzipBody = false, + headersOnly = false, +} = {}) { + const requests = []; + const events = []; + const server = http.createServer((req, res) => { + const body = []; + req.on('data', (chunk) => body.push(chunk)); + req.on('end', async () => { + requests.push({ + method: req.method, + url: req.url, + headers: req.headers, + authorization: req.headers.authorization, + body: Buffer.concat(body).toString('utf8'), + }); + res.on('finish', () => events.push('upstream-finish')); + if (delayMs) await sleep(delayMs); + if (res.destroyed) { + events.push('aborted-before-headers'); + return; + } + if (gzipBody) { + // The gzip header is what the relay must drop: undici hands the proxy + // decoded bytes, so forwarding `content-encoding: gzip` makes the + // client gunzip plain text. + res.writeHead(status, { + 'content-type': 'text/event-stream', + 'content-encoding': 'gzip', + 'content-length': String( + gzipSync(Buffer.from(chunks.join(''))).length, + ), + }); + events.push('write-head'); + requests.at(-1).sentContentEncoding = 'gzip'; + res.end(gzipSync(Buffer.from(chunks.join('')))); + return; + } + res.writeHead(status, { 'content-type': 'text/event-stream' }); + events.push('write-head'); + if (headersOnly) { + // Flush so the proxy's fetch resolves on headers; the test is the + // first-body idle window, not the pre-header one. + res.flushHeaders(); + return; + } + for (const [index, chunk] of chunks.entries()) { + if (chunkDelayMs) await sleep(chunkDelayMs); + if (res.destroyed) { + events.push('aborted-mid-body'); + return; + } + // Respecting backpressure keeps the stream alive while the proxy's + // downstream is paused; without it the mock buffers the whole body in + // its own process and the pause never propagates. + if (!res.write(chunk) && respectBackpressure) { + await once(res, 'drain'); + } + if (resetAfterFirstChunk && index === 0) { + // Register on the socket, not unconditionally: the test asserts + // the upstream really reset after headers. + res.on('close', () => events.push('upstream-reset')); + await sleep(50); + res.destroy(); + return; + } + } + res.end(); + }); + }); + const port = await listen(server); + return { port, requests, events }; +} + +async function startProxy(upstream, overrides = {}, basePath = '/v1') { + const server = createModelProxy({ + baseUrl: `http://127.0.0.1:${upstream.port}${basePath}`, + apiKey: REAL_KEY, + token: TOKEN, + nonce: NONCE, + ...overrides, + }); + const port = await listen(server); + return `http://127.0.0.1:${port}`; +} + +const chatRequest = (body = '{"model":"m"}') => ({ + method: 'POST', + headers: { + authorization: `Bearer ${TOKEN}`, + 'content-type': 'application/json', + }, + body, +}); + +describe('verify-pr-model-proxy', () => { + it('health endpoint echoes the nonce without touching the upstream', async () => { + const upstream = await startUpstream(); + const proxy = await startProxy(upstream); + const res = await fetch(`${proxy}/__health`); + expect(res.status).toBe(200); + expect(await res.text()).toBe(NONCE); + expect(upstream.requests).toHaveLength(0); + }); + + it('rejects a missing or wrong bearer token before the upstream', async () => { + const upstream = await startUpstream(); + const proxy = await startProxy(upstream); + for (const authorization of [undefined, 'Bearer wrong']) { + const res = await fetch(`${proxy}/chat/completions`, { + method: 'POST', + headers: authorization ? { authorization } : {}, + body: '{}', + }); + expect(res.status).toBe(401); + } + expect(upstream.requests).toHaveLength(0); + }); + + it('allows only POST /chat/completions', async () => { + const upstream = await startUpstream(); + const proxy = await startProxy(upstream); + const get = await fetch(`${proxy}/v1/models`, { + headers: { authorization: `Bearer ${TOKEN}` }, + }); + expect(get.status).toBe(403); + const post = await fetch(`${proxy}/v1/models`, chatRequest()); + expect(post.status).toBe(403); + // Exact path, not a suffix: this must not reach the upstream as + // /v1/admin/chat/completions. + const admin = await fetch(`${proxy}/admin/chat/completions`, chatRequest()); + expect(admin.status).toBe(403); + // The method half matters on its own: a PUT to the allowed path would + // otherwise spend the real key upstream. + const put = await fetch(`${proxy}/chat/completions`, { + ...chatRequest(), + method: 'PUT', + }); + expect(put.status).toBe(403); + expect(upstream.requests).toHaveLength(0); + }); + + it('relays chat completions with the real key and the base-path prefix', async () => { + const upstream = await startUpstream({ + chunks: ['data: a\n\n', 'data: b\n\n'], + }); + const proxy = await startProxy(upstream); + const res = await fetch( + `${proxy}/chat/completions?stream=true`, + chatRequest('{"model":"m","stream":true}'), + ); + expect(res.status).toBe(200); + expect(await res.text()).toBe('data: a\n\ndata: b\n\n'); + expect(upstream.requests).toHaveLength(1); + const seen = upstream.requests[0]; + expect(seen.method).toBe('POST'); + // The proxy's base URL carries /v1; the client path does not repeat it. + expect(seen.url).toBe('/v1/chat/completions?stream=true'); + expect(seen.authorization).toBe(`Bearer ${REAL_KEY}`); + expect(seen.body).toBe('{"model":"m","stream":true}'); + }); + + it('does not double-prefix a client path that already carries the base path', async () => { + const upstream = await startUpstream(); + const proxy = await startProxy(upstream); + const res = await fetch(`${proxy}/v1/chat/completions`, chatRequest()); + expect(res.status).toBe(200); + expect(upstream.requests[0].url).toBe('/v1/chat/completions'); + }); + + it('matches the exact path even without a base-path prefix', async () => { + const upstream = await startUpstream(); + const proxy = await startProxy(upstream, {}, ''); + const admin = await fetch(`${proxy}/admin/chat/completions`, chatRequest()); + expect(admin.status).toBe(403); + expect(upstream.requests).toHaveLength(0); + const ok = await fetch(`${proxy}/chat/completions`, chatRequest()); + expect(ok.status).toBe(200); + expect(upstream.requests[0].url).toBe('/chat/completions'); + }); + + it('strips the upstream content-encoding instead of forwarding it', async () => { + const upstream = await startUpstream({ + gzipBody: true, + chunks: ['data: gzip-ok\n\n'], + }); + const proxy = await startProxy(upstream); + const res = await fetch(`${proxy}/chat/completions`, chatRequest()); + expect(res.status).toBe(200); + // The fixture must really have compressed; otherwise this test would + // pass against an uncompressed upstream and pin nothing. + expect(upstream.requests[0].sentContentEncoding).toBe('gzip'); + expect(res.headers.get('content-type')).toBe('text/event-stream'); + // undici already decoded the body; forwarding the header would make the + // client gunzip plain text. + expect(res.headers.get('content-encoding')).toBeNull(); + expect(await res.text()).toBe('data: gzip-ok\n\n'); + }); + + it('strips hop-by-hop headers and relays a chunked request body', async () => { + const upstream = await startUpstream(); + const proxy = await startProxy(upstream); + const { port } = new URL(proxy); + const status = await new Promise((resolve, reject) => { + const req = http.request( + { + host: '127.0.0.1', + port, + path: '/chat/completions', + method: 'POST', + headers: { + authorization: `Bearer ${TOKEN}`, + connection: ' x-client-hop , keep-alive ', + 'x-client-hop': 'client-secret', + 'x-keep-me': 'kept', + 'proxy-authorization': 'Basic client-proxy-secret', + 'transfer-encoding': 'chunked', + expect: '100-continue', + 'content-type': 'application/json', + }, + }, + (res) => { + res.resume(); + res.on('end', () => resolve(res.statusCode)); + }, + ); + req.on('error', reject); + req.write('{"model":'); + req.end('"m"}'); + }); + expect(status).toBe(200); + const seen = upstream.requests[0]; + expect(seen.body).toBe('{"model":"m"}'); + expect(seen.authorization).toBe(`Bearer ${REAL_KEY}`); + expect(seen.headers['proxy-authorization']).toBeUndefined(); + expect(seen.headers.expect).toBeUndefined(); + // The Connection-nominated name must not survive under its own spelling; + // this is the assertion that goes red when the nominated-strip loop is + // removed (the fixed-set strip alone would still drop `connection`). + expect(seen.headers['x-client-hop']).toBeUndefined(); + // Passthrough in the positive direction: a custom header NOT nominated + // by Connection, and the content type, must survive — the stripping is + // selective, not a wholesale header drop. + expect(seen.headers['x-keep-me']).toBe('kept'); + expect(seen.headers['content-type']).toBe('application/json'); + }); + + it('propagates an upstream error status and body', async () => { + const upstream = await startUpstream({ + status: 429, + chunks: ['slow down'], + }); + const proxy = await startProxy(upstream); + const res = await fetch(`${proxy}/chat/completions`, chatRequest()); + expect(res.status).toBe(429); + expect(await res.text()).toBe('slow down'); + }); + + it('answers 504 when the upstream stalls past the idle window', async () => { + const server = http.createServer(() => { + // Never respond: the watchdog must fire. + }); + const port = await listen(server); + const proxy = await startProxy({ port }, { idleTimeoutMs: 50 }); + const res = await fetch(`${proxy}/chat/completions`, chatRequest()); + expect(res.status).toBe(504); + }); + + it('answers 502 and stays alive when the upstream is unreachable', async () => { + // Bind an ephemeral port and close it: the dial must really be refused. + // Port 9 would never dial at all — it is on undici's blocked-port list, + // so fetch rejects with `bad port` before a socket opens and the + // connect-refused path this test names would go unexercised. + const dead = http.createServer(() => {}); + const deadPort = await listen(dead); + await new Promise((resolve) => { + dead.closeAllConnections(); + dead.close(resolve); + }); + const logged = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const proxy = await startProxy({ port: deadPort }, {}, ''); + const res = await fetch(`${proxy}/chat/completions`, chatRequest()); + expect(res.status).toBe(502); + const failure = logged.mock.calls + .map((call) => call.find((arg) => arg instanceof Error)) + .find(Boolean); + expect(failure?.cause?.code).toBe('ECONNREFUSED'); + const health = await fetch(`${proxy}/__health`); + expect(health.status).toBe(200); + } finally { + logged.mockRestore(); + } + }); + + // A slow request UPLOAD is activity: fetch resolves only on the upstream + // headers, so a window meant for stream silence must not tear a large + // prompt down mid-upload. + it('keeps a slow request upload alive past the idle window', async () => { + const upstream = await startUpstream(); + const proxy = await startProxy(upstream, { idleTimeoutMs: 300 }); + const { port } = new URL(proxy); + const status = await new Promise((resolve, reject) => { + const req = http.request( + { + host: '127.0.0.1', + port, + path: '/chat/completions', + method: 'POST', + headers: { + authorization: `Bearer ${TOKEN}`, + 'content-type': 'application/json', + }, + }, + (res) => { + res.resume(); + res.on('end', () => resolve(res.statusCode)); + }, + ); + req.on('error', reject); + // Three chunks 150 ms apart: total 450 ms outlives the 300 ms window, + // and no single gap exceeds it — only the per-chunk re-arm survives. + req.write('{"model"'); + setTimeout(() => req.write(':"m"'), 150); + setTimeout(() => req.write(',"stream":false}'), 300); + setTimeout(() => req.end(), 450); + }); + expect(status).toBe(200); + expect(upstream.requests).toHaveLength(1); + expect(upstream.requests[0].body).toBe('{"model":"m","stream":false}'); + }); + + it('aborts when headers arrive but no body follows', async () => { + const upstream = await startUpstream({ headersOnly: true }); + const proxy = await startProxy(upstream, { idleTimeoutMs: 100 }); + // The upstream flushes headers, then stalls: the first-body idle window + // must tear the stream down instead of letting it hang. + await expect( + fetch(`${proxy}/chat/completions`, chatRequest()), + ).rejects.toThrow(); + const health = await fetch(`${proxy}/__health`); + expect(health.status).toBe(200); + }); + + it('refuses to start without the full configuration', () => { + expect(() => + createModelProxy({ + baseUrl: 'http://x', + apiKey: '', + token: 't', + nonce: 'n', + }), + ).toThrow('required'); + }); + + it('aborts the upstream when the client disconnects before headers', async () => { + const upstream = await startUpstream({ delayMs: 100 }); + const proxy = await startProxy(upstream); + const { port } = new URL(proxy); + const req = http.request({ + host: '127.0.0.1', + port, + path: '/chat/completions', + method: 'POST', + headers: { authorization: `Bearer ${TOKEN}` }, + }); + req.on('error', () => {}); + req.end('{}'); + // The end callback does not prove the proxy's upstream request exists yet; + // wait for the mock to record it, then disconnect while headers are still + // delayed so the pre-header abort is what the test exercises. + await waitFor(() => upstream.requests.length === 1); + req.destroy(); + await sleep(400); + expect(upstream.requests).toHaveLength(1); + // The upstream must never answer a client that is already gone: with the + // abort registered only after the fetch, it writes headers and the whole + // completion to a socket nobody reads. + expect(upstream.events).toContain('aborted-before-headers'); + expect(upstream.events).not.toContain('write-head'); + }); + + it('aborts the upstream when the client disconnects after headers', async () => { + const upstream = await startUpstream({ + chunks: ['data: first\n\n', 'data: second\n\n', 'data: third\n\n'], + chunkDelayMs: 100, + }); + const proxy = await startProxy(upstream, { idleTimeoutMs: 5000 }); + const { port } = new URL(proxy); + const req = http.request( + { + host: '127.0.0.1', + port, + path: '/chat/completions', + method: 'POST', + headers: { authorization: `Bearer ${TOKEN}` }, + }, + (res) => { + res.once('data', () => { + res.destroy(); + req.destroy(); + }); + res.on('error', () => {}); + }, + ); + req.on('error', () => {}); + req.end('{}'); + await waitFor(() => upstream.events.includes('aborted-mid-body'), 3000); + expect(upstream.events).toContain('aborted-mid-body'); + }); + + it('does not abort a healthy stream while the downstream is paused', async () => { + const chunks = Array.from( + { length: 128 }, + (_, index) => `${String(index).padStart(3, '0')}:${'x'.repeat(65_520)}\n`, + ); + chunks.push('TAIL-MARKER'); + const upstream = await startUpstream({ + chunks, + respectBackpressure: true, + }); + // The pause (800 ms) must exceed the window (300 ms) or the pause guard + // is never exercised; the margin keeps a loaded runner from flipping it. + const proxy = await startProxy(upstream, { idleTimeoutMs: 300 }); + const { port } = new URL(proxy); + const body = await new Promise((resolve, reject) => { + const req = http.request( + { + host: '127.0.0.1', + port, + path: '/chat/completions', + method: 'POST', + headers: { authorization: `Bearer ${TOKEN}` }, + }, + (res) => { + const parts = []; + res.on('data', (chunk) => parts.push(chunk)); + res.on('end', () => resolve(Buffer.concat(parts).toString('utf8'))); + res.on('error', reject); + // Pause past the idle window while the upstream keeps producing: + // a slow but live client must not be read as upstream idleness. + res.pause(); + setTimeout(() => res.resume(), 800); + }, + ); + req.on('error', reject); + req.end('{}'); + }); + expect(body).toBe(chunks.join('')); + }); + + it('keeps a slow-but-healthy stream alive past the window', async () => { + const upstream = await startUpstream({ + chunks: [ + 'data: 1\n\n', + 'data: 2\n\n', + 'data: 3\n\n', + 'data: 4\n\n', + 'data: 5\n\n', + ], + chunkDelayMs: 80, + }); + const proxy = await startProxy(upstream, { idleTimeoutMs: 300 }); + const res = await fetch(`${proxy}/chat/completions`, chatRequest()); + expect(res.status).toBe(200); + // Total delivery (400 ms) outlives the 300 ms window while every gap + // (80 ms) stays well inside it: only the per-chunk re-arm keeps this + // from being cut, and the 220 ms margin survives a loaded runner. + expect(await res.text()).toBe( + 'data: 1\n\ndata: 2\n\ndata: 3\n\ndata: 4\n\ndata: 5\n\n', + ); + }); + + it('survives an upstream reset after headers', async () => { + const upstream = await startUpstream({ + chunks: ['data: partial\n\n'], + resetAfterFirstChunk: true, + }); + const child = spawn(process.execPath, [SCRIPT], { + env: { + PATH: process.env.PATH, + REVIEW_OPENAI_BASE_URL: `http://127.0.0.1:${upstream.port}/v1`, + REVIEW_OPENAI_API_KEY: REAL_KEY, + }, + // The reset is the point of this test; the proxy's diagnostic for it + // would only look like a failure in the suite output. + stdio: ['ignore', 'pipe', 'ignore'], + }); + cleanups.push(async () => { + child.kill('SIGTERM'); + }); + const line = await new Promise((resolve, reject) => { + let buffer = ''; + child.stdout.on('data', (data) => { + buffer += data; + if (buffer.includes('\n')) resolve(buffer.split('\n')[0]); + }); + child.on('error', reject); + child.on('exit', (code) => + reject(new Error(`proxy exited before printing: ${code}`)), + ); + }); + const info = JSON.parse(line); + // The CLI-generated token is the container's only credential. + expect(info.token).toMatch(/^[0-9a-f]{48}$/); + const res = await fetch(`http://127.0.0.1:${info.port}/chat/completions`, { + method: 'POST', + headers: { authorization: `Bearer ${info.token}` }, + body: '{}', + }).catch(() => null); + if (res) await res.text().catch(() => {}); + await waitFor(() => upstream.events.includes('upstream-reset'), 2000); + await sleep(200); + // The client may see a torn stream; the proxy process must not die on it. + expect(child.exitCode).toBeNull(); + expect(upstream.events).toContain('upstream-reset'); + const health = await fetch(`http://127.0.0.1:${info.port}/__health`); + expect(health.status).toBe(200); + }); +}); + +describe('proxy defaults and base url formatting', () => { + it('keeps the idle window at the client default', () => { + // Read the core constant from its source instead of restating it: two + // hardcoded copies can drift with the suite green. A tighter proxy would + // cut streams its own client still tolerates. + const constants = readFileSync( + fileURLToPath( + new URL( + '../../packages/core/src/core/openaiContentGenerator/constants.ts', + import.meta.url, + ), + ), + 'utf8', + ); + const match = /DEFAULT_STREAM_IDLE_TIMEOUT_MS\s*=\s*(\d+)/.exec(constants); + expect(match).not.toBeNull(); + expect(DEFAULT_IDLE_TIMEOUT_MS).toBe(Number(match[1])); + }); + + it('brackets an IPv6 listen host', () => { + expect(formatLocalBaseUrl('::1', 8080, '/v1')).toBe('http://[::1]:8080/v1'); + }); + + it('leaves an IPv4 host alone and tolerates an empty base path', () => { + expect(formatLocalBaseUrl('192.0.2.10', 8080)).toBe( + 'http://192.0.2.10:8080', + ); + expect(formatLocalBaseUrl('192.0.2.10', 8080, '/v1')).toBe( + 'http://192.0.2.10:8080/v1', + ); + }); + + it('normalizes base paths', () => { + expect(normalizeBasePath('/v1/')).toBe('/v1'); + expect(normalizeBasePath('/v1///')).toBe('/v1'); + expect(normalizeBasePath('/')).toBe(''); + expect(normalizeBasePath('')).toBe(''); + }); +}); + +describe('parseArgs', () => { + it('defaults to loopback with an ephemeral port', () => { + expect(parseArgs([])).toEqual({ listen: '127.0.0.1' }); + }); + + it('parses --listen', () => { + expect(parseArgs(['--listen', '172.17.0.1'])).toEqual({ + listen: '172.17.0.1', + }); + }); + + it('rejects --port, which the recipe does not use', () => { + expect(() => parseArgs(['--port', '8080'])).toThrow('unknown argument'); + }); + + it('rejects unknown arguments and missing values', () => { + expect(() => parseArgs(['--docker'])).toThrow('unknown argument'); + expect(() => parseArgs(['--listen'])).toThrow('requires a value'); + }); + + // server.listen(port, '') binds EVERY interface — past the bridge a + // key-holding proxy is meant to stay on — and a flag-looking value is a + // missing value, not a host. + it('rejects an empty or flag-looking --listen value', () => { + expect(() => parseArgs(['--listen', ''])).toThrow('requires a value'); + expect(() => parseArgs(['--listen', '--other'])).toThrow( + 'requires a value', + ); + }); +}); + +describe('CLI', () => { + it('exits 2 when the upstream configuration is missing', () => { + const res = spawnSync(process.execPath, [SCRIPT], { + env: { PATH: process.env.PATH }, + encoding: 'utf8', + }); + expect(res.status).toBe(2); + expect(res.stderr).toContain('REVIEW_OPENAI_BASE_URL'); + }); + + it('prints the listen coordinates as JSON and honors the token override', async () => { + const upstream = await startUpstream(); + const child = spawn(process.execPath, [SCRIPT, '--listen', '0.0.0.0'], { + env: { + PATH: process.env.PATH, + REVIEW_OPENAI_BASE_URL: `http://127.0.0.1:${upstream.port}/v1`, + REVIEW_OPENAI_API_KEY: REAL_KEY, + QWEN_VERIFY_PROXY_TOKEN: 'fixed-token', + }, + stdio: ['ignore', 'pipe', 'inherit'], + }); + cleanups.push(async () => { + child.kill('SIGTERM'); + }); + const line = await new Promise((resolve, reject) => { + let buffer = ''; + child.stdout.on('data', (data) => { + buffer += data; + if (buffer.includes('\n')) resolve(buffer.split('\n')[0]); + }); + child.on('error', reject); + child.on('exit', (code) => + reject(new Error(`proxy exited before printing: ${code}`)), + ); + }); + const info = JSON.parse(line); + expect(info.token).toBe('fixed-token'); + // The listen value must reach the printed URL; hard-coding loopback here + // would hand the container an address it cannot dial. + expect(info.baseUrl).toBe(`http://0.0.0.0:${info.port}/v1`); + expect(info.nonce).toMatch(/^[0-9a-f]{48}$/); + // The printed baseUrl must not leak the real key, and the health + // endpoint must echo the printed nonce. + expect(line).not.toContain(REAL_KEY); + const health = await fetch(`http://127.0.0.1:${info.port}/__health`); + expect(await health.text()).toBe(info.nonce); + }); +}); + +/** + * The recipe is the only place where the trust delegation lives, and a + * pleasing-looking revert (relative path, attestation on the run line) is + * invisible to the runtime tests. Pin the load-bearing lines. + */ +describe('local container recipe pins', () => { + const skill = readFileSync( + fileURLToPath( + new URL('../../.qwen/skills/verify-pr/SKILL.md', import.meta.url), + ), + 'utf8', + ); + + it('runs every round script from the trusted checkout', () => { + expect(skill).toContain( + 'cd && node /scripts/verify-pr-prepare.mjs', + ); + expect(skill).toContain( + 'node /trusted-scripts/verify-pr-prepare.mjs --require-isolation', + ); + expect(skill).toContain( + 'node /trusted-scripts/verify-pr-credential-scan.mjs /work', + ); + expect(skill).toContain( + 'node /scripts/verify-pr-model-proxy.mjs', + ); + expect(skill).toContain('/scripts/verify-pr-guard.mjs'); + expect(skill).toContain( + '`` below is the trusted checkout the watch scripts run from', + ); + // Enumerate the checker call sites: the preamble also names the trusted + // path, so a single positive assertion cannot detect step 7 reverting to + // /scripts. + const checkerCalls = [ + ...skill.matchAll(/node (\S+)\/scripts\/verify-pr-check\.mjs/g), + ].map((match) => match[1]); + expect(checkerCalls.length).toBeGreaterThanOrEqual(3); + for (const prefix of checkerCalls) expect(prefix).toBe(''); + expect(skill).not.toMatch( + /node \/scripts\/verify-pr-(prepare|check|model-proxy|guard)/, + ); + expect(skill).not.toMatch( + /node scripts\/verify-pr-(prepare|check|model-proxy|guard)\.mjs/, + ); + }); + + it('base-pins `.qwen` and states that root AGENTS/QWEN.md still come from the PR', () => { + expect(skill).toContain('git -C archive "$BASE_OID" -- .qwen'); + expect(skill).toContain( + '-v ~/.cache/qwen-verify/pr/qwen-base/.qwen:/work/.qwen:ro', + ); + expect(skill).toContain('`AGENTS.md`/`QWEN.md` do come from the PR clone'); + }); + + it('mounts the round tree writable and the metadata snapshot read-only', () => { + expect(skill).toContain('--user "$(id -u):$(id -g)"'); + expect(skill).toContain('-v :/work'); + // toContain alone is satisfied by `:/work:ro` — the exact mutation this + // pin exists to catch — so the read-only spelling is excluded outright. + expect(skill).not.toContain('-v :/work:ro'); + expect(skill).toContain('-v /scripts:/trusted-scripts:ro'); + expect(skill).toContain('-v ~/.cache/qwen-verify/pr:/verify-context:ro'); + expect(skill).toContain('QWEN_VERIFY_CONTEXT=/verify-context/pr.json'); + }); + + it('reads both credentials from files, including without a TTY', () => { + expect(skill).toContain('if [ -t 0 ]; then'); + expect(skill).toContain('REVIEW_OPENAI_KEY_FILE'); + // `read` returns 1 at EOF even when it assigned the variable: a key file + // without a trailing newline must be judged by the variable, not the + // exit status, or the documented agent path aborts with the opposite + // diagnosis. + expect(skill).toContain('[ -n "$REVIEW_OPENAI_API_KEY" ]'); + expect(skill).not.toContain('REVIEW_OPENAI_API_KEY='); + // The proxy owns its copy: later diagnostics must not see the key. + expect(skill).toContain('unset REVIEW_OPENAI_API_KEY'); + expect(skill).toContain('QWEN_VERIFY_PROXY_TOKEN="$(node -e'); + expect(skill).toContain('chmod 600'); + // The background process truncates proxy.json before printing; the loop + // and the non-empty check keep an empty read from reaching Docker. + expect(skill).toContain('for _ in {1..100}; do'); + expect(skill).not.toContain('$(seq 1 100)'); + expect(skill).toContain('test -n "$QWEN_VERIFY_PROXY_TOKEN"'); + // The nonce, not just the JSON shape, is checked before use. + expect(skill).toContain('/__health'); + expect(skill).toContain('fetch(`http://${u.hostname}:${u.port}/__health`)'); + expect(skill).toContain('process.exit(t===j.nonce?0:1)'); + // Each step is its own shell: the token is re-derived where it is used. + const dockerRun = + skill.match(/export QWEN_VERIFY_PROXY_TOKEN[\s\S]*?```/)?.[0] ?? ''; + expect(dockerRun).toContain('proxy.json'); + expect(dockerRun).toContain('test -n "$QWEN_VERIFY_PROXY_TOKEN"'); + expect(skill).toMatch(/-e QWEN_VERIFY_PROXY_TOKEN[ \t]*\\?\n/); + expect(skill).not.toContain('-e QWEN_VERIFY_PROXY_TOKEN='); + expect(skill).not.toContain('--port'); + }); + + it('keeps the attestation off the docker run line and on the round shell', () => { + const dockerRunMatch = skill.match(/docker run -d[\s\S]*?```/); + expect(dockerRunMatch).not.toBeNull(); + expect(dockerRunMatch[0]).not.toContain('QWEN_VERIFY_CREDENTIAL_FREE'); + // A fresh docker exec is a new shell: the round's shell must carry the + // attestation or its first isolation check exits 4. + expect(skill).toContain( + 'docker exec -i -e QWEN_VERIFY_CREDENTIAL_FREE=1 qwen-verify-', + ); + const roundBlock = skill.split('qwen --auth-type openai')[0].split('```'); + expect(roundBlock.at(-1)).toContain('QWEN_VERIFY_CREDENTIAL_FREE'); + // The credential hit aborts structurally, not via `set -e`. + expect(skill).toContain( + 'node /trusted-scripts/verify-pr-credential-scan.mjs /work || exit 1', + ); + }); + + it('names the released CLI as the engine and gives the tree-build route', () => { + expect(skill).toContain("The round's engine is the released CLI"); + expect(skill).toContain('npm run build && npm run bundle'); + expect(skill).toContain('npm install -g --prefix "$HOME/.local" /work'); + }); + + it('refuses a nested repository in the scan description', () => { + expect(skill).toMatch(/nested\s+git repository outright/); + expect(skill).toMatch(/vendored\s+git\s+dependency/); + expect(skill).toContain('not public keys'); + expect(skill).toContain('untracked clone, populated submodule'); + }); + + it('names the value-inspection contract and the include-following enumeration', () => { + expect(skill).toContain('inspects every local config'); + expect(skill).toContain('`http.proxy` included'); + expect(skill).toContain('`url.*.pushInsteadOf`'); + expect(skill).toContain('`Authorization` /'); + expect(skill).toContain('`Proxy-Authorization` header values'); + expect(skill).toContain('bare `git@` account name'); + expect(skill).toMatch(/enumeration\s+follows\s+`include\.path`/); + expect(skill).toContain('covers the enabled `.git/config.worktree` scope'); + expect(skill).toContain('absent means an empty scope'); + expect(skill).toMatch(/filter name\s+containing `=` is refused outright/); + }); + + it('snapshots the previous report and pins both checker call sites', () => { + expect(skill).toContain('previous-report.md'); + expect(skill).toContain('issues//comments'); + const contextCalls = [ + ...skill.matchAll(/verify-pr-check\.mjs --context /g), + ]; + expect(contextCalls.length).toBeGreaterThanOrEqual(2); + // The prose occurrences alone cannot catch step 7 dropping the flag on + // its continuation line — pin the executable call too. + expect(skill).toContain( + '--context ~/.cache/qwen-verify/pr/pr.json ', + ); + }); + + it('writes previous-report.md only when a report exists', () => { + // The file's PRESENCE is the follow-up trigger: an empty file from a + // first round (or gh error text) misclassifies every round. + expect(skill).toContain('if [ -n "$PREV" ]; then'); + expect(skill).toContain( + 'else rm -f ~/.cache/qwen-verify/pr/previous-report.md; fi', + ); + expect(skill).toContain( + '{ echo "could not read PR comments" >&2; exit 1; }', + ); + // First-paragraph marker + bot-or-self author, not a bare substring any + // reply quoting `Verdict:` would satisfy. gh's --slurp cannot combine + // with --jq, so the selection runs in node over the piped pages. + expect(skill).toContain('c.body.split("\\n\\n")[0].includes("Verdict:")'); + expect(skill).toContain( + '/bot$/.test(c.user.login)||c.user.login===process.argv[1]', + ); + }); + + it('keeps the proxy JSON parseable and the base extraction fresh', () => { + // One stderr line folded into proxy.json breaks every parse of it. + expect(skill).toContain( + '> ~/.cache/qwen-verify/pr/proxy.json 2> ~/.cache/qwen-verify/pr/proxy.err', + ); + // Add-only extraction would keep a .qwen file the base deleted. + expect(skill).toContain('rm -rf ~/.cache/qwen-verify/pr/qwen-base'); + }); + + it('hashes the base-pinned skill copy in the publish footer', () => { + // The round loads the base commit's .qwen, not the PR head's: hashing + // the head would certify instructions the round never ran. + expect(skill).toContain( + 'git -C rev-parse ":.qwen/skills/verify-pr/SKILL.md"', + ); + expect(skill).toContain("never the PR head's copy"); + }); +}); diff --git a/scripts/tests/verify-pr-prepare.test.js b/scripts/tests/verify-pr-prepare.test.js index 5318828e651..43619605a4c 100644 --- a/scripts/tests/verify-pr-prepare.test.js +++ b/scripts/tests/verify-pr-prepare.test.js @@ -20,9 +20,12 @@ import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterEach, describe, expect, it } from 'vitest'; import { + GIT_HARDENING, artifactDirName, detectCredentialSignals, detectIsolation, + gitStatusArgs, + localFilterOverrides, parseArgs, parseRepo, validateContext, @@ -213,6 +216,264 @@ describe('detectCredentialSignals', () => { '/home/t', ), ).toEqual(['SANDBOX_MOUNTS']); + // The recipe's proxy credential must stay outside the signal set, or a + // container round would fail the gate before it starts. + expect( + detectCredentialSignals( + { QWEN_VERIFY_PROXY_TOKEN: 'x' }, + () => false, + '/home/t', + ), + ).toEqual([]); + }); +}); + +describe('git hardening', () => { + it('disables fsmonitor, hooks, credential helpers and excludesFile on every git read', () => { + expect(GIT_HARDENING).toEqual([ + '-c', + 'core.fsmonitor=false', + '-c', + 'core.hooksPath=/dev/null', + '-c', + 'credential.helper=', + '-c', + 'core.excludesFile=/dev/null', + ]); + const { dir } = makeRepo(); + expect(localFilterOverrides(dir)).toEqual([]); + expect(gitStatusArgs(dir)).toEqual([ + ...GIT_HARDENING, + '-C', + dir, + 'status', + '--porcelain', + '--untracked-files=normal', + ]); + }); + + // A filter VALUE may contain newlines; parsing the rendered `key value` + // output would accept the second line as a key and forge a `-c` that + // outranks the hardening (git applies the last -c for a key). + it('does not let a newline-bearing filter value forge overrides', () => { + const { dir } = makeRepo(); + const hooks = join(dir, '.githooks'); + mkdirSync(hooks, { recursive: true }); + const marker = join(dir, 'hook-fired'); + writeFileSync( + join(hooks, 'post-checkout'), + `#!/bin/sh\necho fired > ${marker}\n`, + ); + chmodSync(join(hooks, 'post-checkout'), 0o755); + execFileSync( + 'git', + [ + 'config', + '--local', + 'filter.decoy.clean', + `harmless\ncore.hooksPath=${hooks}`, + ], + { cwd: dir, env: gitEnv }, + ); + mkdirSync(join(dir, '.git', 'info'), { recursive: true }); + writeFileSync(join(dir, '.git', 'info', 'attributes'), '* filter=decoy\n'); + expect(localFilterOverrides(dir)).toEqual(['-c', 'filter.decoy.clean=']); + // The forged hooksPath must not reach git: the name list never contained + // it, and the fixed hardening is applied last in any case. + const args = gitStatusArgs(dir); + expect(args.filter((a) => a === 'core.hooksPath=/dev/null')).toHaveLength( + 1, + ); + expect(args.join(' ')).not.toContain(hooks); + spawnSync('git', args, { encoding: 'utf8' }); + expect(existsSync(marker)).toBe(false); + }); + + // git allows '=' inside a filter subsection (filter.a=b.clean), but -c + // splits its argument at the FIRST '=': the override would set a different + // key and leave the real filter armed. Such a name must be refused before + // any guarded status could run it, not emitted as a broken override. + it('refuses a filter name containing = instead of emitting a broken override', () => { + const { dir } = makeRepo(); + const marker = join(dir, 'filter-fired'); + execFileSync( + 'git', + [ + 'config', + '--local', + 'filter.a=b.clean', + `sh -c 'echo fired > ${marker} && cat'`, + ], + { cwd: dir, env: gitEnv }, + ); + mkdirSync(join(dir, '.git', 'info'), { recursive: true }); + writeFileSync(join(dir, '.git', 'info', 'attributes'), '* filter=a=b\n'); + // Same-byte-length change: a size change would let git skip the clean + // conversion and the positive control would prove nothing. + writeFileSync(join(dir, 'base.txt'), 'BaSe\n'); + spawnSync('git', [...GIT_HARDENING, '-C', dir, 'status', '--porcelain'], { + encoding: 'utf8', + }); + expect(existsSync(marker)).toBe(true); + rmSync(marker); + expect(() => localFilterOverrides(dir)).toThrow( + /cannot be safely disabled/, + ); + expect(() => gitStatusArgs(dir)).toThrow(/cannot be safely disabled/); + expect(existsSync(marker)).toBe(false); + }); + + // extensions.worktreeConfig makes git status read .git/config.worktree, + // which --local never reads; a filter planted there must be emptied too. + it('empties a filter planted in .git/config.worktree', () => { + const { dir } = makeRepo(); + const marker = join(dir, 'filter-fired'); + execFileSync( + 'git', + ['config', '--local', 'extensions.worktreeConfig', 'true'], + { cwd: dir, env: gitEnv }, + ); + execFileSync( + 'git', + [ + 'config', + '--worktree', + 'filter.wt.clean', + `sh -c 'echo fired > ${marker} && cat'`, + ], + { cwd: dir, env: gitEnv }, + ); + mkdirSync(join(dir, '.git', 'info'), { recursive: true }); + writeFileSync(join(dir, '.git', 'info', 'attributes'), '* filter=wt\n'); + // Same byte length as 'base\n': a size change lets git skip the clean + // conversion, which makes the positive control depend on racy timing. + writeFileSync(join(dir, 'base.txt'), 'BaSe\n'); + // Positive control: without the worktree-scope override the filter runs. + spawnSync( + 'git', + [ + '-c', + 'core.fsmonitor=false', + '-c', + 'core.hooksPath=/dev/null', + '-C', + dir, + 'status', + '--porcelain', + ], + { encoding: 'utf8' }, + ); + expect(existsSync(marker)).toBe(true); + rmSync(marker); + expect(localFilterOverrides(dir)).toEqual(['-c', 'filter.wt.clean=']); + spawnSync('git', gitStatusArgs(dir), { encoding: 'utf8' }); + expect(existsSync(marker)).toBe(false); + }); + + // The extension does not require config.worktree to exist. prepare's + // --get-regexp enumeration already treats the absent file as no matches + // (exit 1); pin that so a future switch to --list (exit 128 on absence) + // cannot silently false-refuse a clean repo the way the scan once did. + it('tolerates an enabled worktree extension with no config.worktree file', () => { + const { dir, base, head } = makeRepo(); + execFileSync( + 'git', + ['config', '--local', 'extensions.worktreeConfig', 'true'], + { cwd: dir, env: gitEnv }, + ); + expect(localFilterOverrides(dir)).toEqual([]); + const status = spawnSync('git', gitStatusArgs(dir), { encoding: 'utf8' }); + expect(status.status).toBe(0); + + const env = installFakeGh(snapshotFor({ base, head })); + const result = runPrepare(dir, ['42', '--repo', REPO], env); + expect(result.status).toBe(0); + }); + + // core.excludesFile is steerable from the same PR-controlled config; the + // hardened status must see untracked files even when it points at a + // catch-all exclude file. + it('sees untracked files through a hostile core.excludesFile', () => { + const { dir } = makeRepo(); + writeFileSync(join(dir, '.git', 'info', 'leave'), '*\n'); + execFileSync( + 'git', + ['config', '--local', 'core.excludesFile', '.git/info/leave'], + { cwd: dir, env: gitEnv }, + ); + writeFileSync(join(dir, 'leftover-modified.ts'), 'export {};\n'); + const steered = spawnSync( + 'git', + ['-C', dir, 'status', '--porcelain', '--untracked-files=normal'], + { encoding: 'utf8', env: gitEnv }, + ); + expect(steered.stdout.trim()).toBe(''); + const hardened = spawnSync('git', gitStatusArgs(dir), { + encoding: 'utf8', + env: gitEnv, + }); + expect(hardened.stdout).toContain('?? leftover-modified.ts'); + }); + + it('empties a planted clean filter before reading status', () => { + const { dir } = makeRepo(); + const marker = join(dir, 'filter-fired'); + execFileSync( + 'git', + [ + 'config', + '--local', + 'filter.pwn.clean', + `sh -c 'echo fired > ${marker}'`, + ], + { cwd: dir, env: gitEnv }, + ); + mkdirSync(join(dir, '.git', 'info'), { recursive: true }); + writeFileSync(join(dir, '.git', 'info', 'attributes'), '* filter=pwn\n'); + // Same byte length as 'base\n': a size change lets git skip the clean + // conversion, which makes the positive control depend on racy timing. + writeFileSync(join(dir, 'base.txt'), 'BaSe\n'); + // Positive control: without the filter overrides the planted filter runs + // on the host during an ordinary status. + spawnSync('git', [...GIT_HARDENING, '-C', dir, 'status', '--porcelain'], { + encoding: 'utf8', + }); + expect(existsSync(marker)).toBe(true); + rmSync(marker); + expect(localFilterOverrides(dir)).toEqual(['-c', 'filter.pwn.clean=']); + spawnSync('git', gitStatusArgs(dir), { encoding: 'utf8' }); + expect(existsSync(marker)).toBe(false); + }); + + // --local alone names one file and skips include.path, while the config + // parsing `git status` uses follows it: a filter defined in an included + // worktree file must be enumerated too, or it runs on the host unopposed. + it('empties a clean filter that arrives through include.path', () => { + const { dir } = makeRepo(); + const marker = join(dir, 'filter-fired'); + writeFileSync( + join(dir, 'evil.inc'), + `[filter "pwn"]\n\tclean = sh -c 'echo fired > ${marker}'\n`, + ); + execFileSync('git', ['config', '--local', 'include.path', '../evil.inc'], { + cwd: dir, + env: gitEnv, + }); + mkdirSync(join(dir, '.git', 'info'), { recursive: true }); + writeFileSync(join(dir, '.git', 'info', 'attributes'), '* filter=pwn\n'); + // Same byte length as 'base\n': a size change lets git skip the clean + // conversion, which makes the positive control depend on racy timing. + writeFileSync(join(dir, 'base.txt'), 'BaSe\n'); + // Positive control: an ordinary hardened status follows the include and + // executes the planted filter on the host. + spawnSync('git', [...GIT_HARDENING, '-C', dir, 'status', '--porcelain'], { + encoding: 'utf8', + }); + expect(existsSync(marker)).toBe(true); + rmSync(marker); + expect(localFilterOverrides(dir)).toEqual(['-c', 'filter.pwn.clean=']); + spawnSync('git', gitStatusArgs(dir), { encoding: 'utf8' }); + expect(existsSync(marker)).toBe(false); }); }); @@ -545,6 +806,49 @@ describe.skipIf(process.platform === 'win32')('prepare (CLI)', () => { expect(existsSync(join(realpathSync(dir), 'tmp'))).toBe(false); }); + // The refusal must reach the CLI as a clean exit 3 with its own remedy, + // before any guarded status runs — not as an uncaught enumeration stack. + it('refuses a tree whose filter name cannot be safely disabled', () => { + const { dir, base, head } = makeRepo(); + const env = installFakeGh(snapshotFor({ base, head })); + execFileSync('git', ['config', '--local', 'filter.a=b.clean', 'true'], { + cwd: dir, + env: gitEnv, + }); + + const result = runPrepare(dir, ['42', '--repo', REPO], env); + expect(result.status).toBe(3); + expect(result.stderr).toContain("contains '='"); + expect(result.stderr).toContain('cannot be safely disabled'); + expect(existsSync(join(realpathSync(dir), 'tmp'))).toBe(false); + }); + + // .git/info/exclude has no -c override: active patterns there could hide + // untracked files from the clean-tree check, so the gate refuses the file + // itself. A fresh clone carries only git's commented default. + it('refuses a .git/info/exclude with active patterns', () => { + const { dir, base, head } = makeRepo(); + const env = installFakeGh(snapshotFor({ base, head })); + mkdirSync(join(dir, '.git', 'info'), { recursive: true }); + writeFileSync(join(dir, '.git', 'info', 'exclude'), '*\n'); + writeFileSync(join(dir, 'hidden.ts'), 'export {};\n'); + + const refused = runPrepare(dir, ['42', '--repo', REPO], env); + expect(refused.status).toBe(3); + expect(refused.stderr).toContain('.git/info/exclude'); + expect(refused.stderr).toContain('re-clone'); + expect(existsSync(join(realpathSync(dir), 'tmp'))).toBe(false); + + // The commented default a fresh clone ships with must pass. + writeFileSync( + join(dir, '.git', 'info', 'exclude'), + '# git ls-files --others --exclude-standard\n# *.[oa]\n', + ); + rmSync(join(dir, 'hidden.ts')); + const clean = runPrepare(dir, ['42', '--repo', REPO], env); + expect(clean.status).toBe(0); + }); + // git status trusts the index: skip-worktree / assume-unchanged entries // (a sparse checkout sets them en masse) hide tracked divergence, so the // gate must refuse them instead of stamping a partial tree as clean. @@ -608,6 +912,16 @@ describe.skipIf(process.platform === 'win32')('prepare (CLI)', () => { expect(result.status).toBe(2); expect(result.stderr).toContain('--repo / is required'); }); + + // Outside a worktree the filter enumeration fails before any git read; + // the refusal must stay the clean exit 3, not an uncaught stack trace. + it('refuses a non-git cwd with the worktree message, not a stack trace', () => { + const dir = makeTmpDir(); + const result = runPrepare(dir, ['42', '--repo', REPO], gitEnv); + expect(result.status).toBe(3); + expect(result.stderr).toContain('not inside a git worktree'); + expect(result.stderr).not.toContain('Error: could not enumerate'); + }); }); // The gate requires an empty `git status`, and prepare writes its artifacts diff --git a/scripts/verify-pr-check.mjs b/scripts/verify-pr-check.mjs index 63dc3567a0f..374145f85a8 100644 --- a/scripts/verify-pr-check.mjs +++ b/scripts/verify-pr-check.mjs @@ -56,7 +56,7 @@ const HEADING_TITLE_MAX = 120; const FOOTER = /^_— .+ via Qwen Code \/verify-pr \(v[^;]+; skill [0-9a-f]{12}\)_$/; -const HELP = `Usage: node scripts/verify-pr-check.mjs +const HELP = `Usage: node scripts/verify-pr-check.mjs [--context ] Checks a local round against the rules the CI publisher enforces: @@ -80,6 +80,10 @@ ${SUITE_SIZED_TOTAL} assertions come with no gate results (an existing test suit counted as the round's own), or when comment.md — the exact body a local round posts, optional — repeats the heading shape or lacks the attribution footer. +With --context , the head-OID check reads that file instead of the +round's own context.json. Pass a copy the round never saw: the artifact +directory is writable by the PR code the round executed. + Exit codes: 0 no errors, 1 errors found, 2 usage. `; @@ -416,10 +420,29 @@ function main(argv) { process.stdout.write(HELP); return 0; } - const dir = argv.find((arg) => !arg.startsWith('-')); - if (!dir) { + let contextPath = null; + const positional = []; + for (let index = 0; index < argv.length; index++) { + const arg = argv[index]; + if (arg === '--context') { + contextPath = argv[++index]; + if (!contextPath) { + process.stderr.write( + `verify-pr check: --context requires a snapshot path\n\n${HELP}`, + ); + return 2; + } + } else if (arg.startsWith('-')) { + process.stderr.write(`verify-pr check: unknown option ${arg}\n\n${HELP}`); + return 2; + } else { + positional.push(arg); + } + } + const dir = positional[0]; + if (!dir || positional.length > 1) { process.stderr.write( - `verify-pr check: an artifact directory is required\n\n${HELP}`, + `verify-pr check: exactly one artifact directory is required\n\n${HELP}`, ); return 2; } @@ -448,25 +471,47 @@ function main(argv) { // rather than silently downgrading the check. let expectedHead = null; let snapshotWarning = null; - let snapshotPath = join(dir, 'context.json'); - const prNumber = /^pr(\d+)-verify-/.exec(basename(resolve(dir)))?.[1]; - if (!existsSync(snapshotPath) && prNumber) { - snapshotPath = join( - dirname(resolve(dir)), - 'verify-context', - `pr${prNumber}.json`, - ); - } - if (existsSync(snapshotPath)) { + let snapshotPath = null; + if (contextPath) { + // A host-held snapshot: the artifact directory is writable by the PR code + // the round just executed, so its own context.json cannot pin the head + // when the maintainer supplies a copy that code never saw. try { - const snapshot = JSON.parse(readFileSync(snapshotPath, 'utf8')); + const snapshot = JSON.parse(readFileSync(contextPath, 'utf8')); if (/^[0-9a-f]{40}$/.test(snapshot?.headRefOid ?? '')) { expectedHead = snapshot.headRefOid; - } else { - snapshotWarning = `snapshot ${snapshotPath} has no valid head OID; the head-OID check fell back to presence-only`; + snapshotPath = contextPath; } } catch { - snapshotWarning = `snapshot ${snapshotPath} is unreadable; the head-OID check fell back to presence-only`; + // Reported below; never falls back to the writable artifact copy. + } + if (!expectedHead) { + process.stderr.write( + `verify-pr check: --context ${contextPath} is missing, unreadable, or carries no valid head OID\n`, + ); + return 1; + } + } else { + snapshotPath = join(dir, 'context.json'); + const prNumber = /^pr(\d+)-verify-/.exec(basename(resolve(dir)))?.[1]; + if (!existsSync(snapshotPath) && prNumber) { + snapshotPath = join( + dirname(resolve(dir)), + 'verify-context', + `pr${prNumber}.json`, + ); + } + if (existsSync(snapshotPath)) { + try { + const snapshot = JSON.parse(readFileSync(snapshotPath, 'utf8')); + if (/^[0-9a-f]{40}$/.test(snapshot?.headRefOid ?? '')) { + expectedHead = snapshot.headRefOid; + } else { + snapshotWarning = `snapshot ${snapshotPath} has no valid head OID; the head-OID check fell back to presence-only`; + } + } catch { + snapshotWarning = `snapshot ${snapshotPath} is unreadable; the head-OID check fell back to presence-only`; + } } } diff --git a/scripts/verify-pr-credential-scan.mjs b/scripts/verify-pr-credential-scan.mjs new file mode 100644 index 00000000000..2fa2160e738 --- /dev/null +++ b/scripts/verify-pr-credential-scan.mjs @@ -0,0 +1,329 @@ +#!/usr/bin/env node + +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Refuses credential-shaped ignored files in a `/verify-pr` round tree. + * + * The isolation gate checks known environment names and home-relative paths, + * and `verify-pr-prepare.mjs` refuses untracked files; a gitignored credential + * file is invisible to both. This scan lists ignored files one by one (git + * status collapses ignored directories into one line, which is why a widened + * pathspec set used to refuse every installed tree) and matches the basenames + * a provider key or certificate typically uses. + * + * Installed dependencies and build output are excluded at any depth: a + * workspace package's own `node_modules` and `dist` are as irrelevant as the + * root ones, and `node_modules` alone ships hundreds of `credentials*` files. + * The pathspecs both restrict enumeration to credential-shaped names and + * exclude those directories, so an installed tree's multi-megabyte ignored + * listing never reaches the output buffer. A credential file placed in the + * project (including a nested ignored directory) is still reported. Two + * exclusions are deliberate: credentials inside `node_modules`/`dist` are + * accepted as installed/build content, and a nested git repository is refused + * outright because git will not descend into it — scan that clone separately. + * + * Usage: + * node scripts/verify-pr-credential-scan.mjs + * + * Exit codes: 0 clean, 1 credential-shaped entries found, 2 usage error. + */ + +import { spawnSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { basename, join } from 'node:path'; +import { isEntryPoint } from './lib/entry-point.js'; + +const CREDENTIAL_BASENAME = + /^(?:\.env.*|.*\.env|.*\.env~|.*\.pem|.*\.key|.*\.p12|.*\.pfx|id_(?:rsa|ed25519|ecdsa|dsa).*|.*credentials.*|\.git-credentials|\.npmrc|\.netrc|gha-creds-.*\.json)$/; + +// Git applies these pathspecs while enumerating, before any output is +// buffered: a full ignored listing of an installed checkout is several MB and +// used to overflow spawnSync's buffer before the JS filter could run. +const CREDENTIAL_PATHSPECS = [ + ':(glob)**/.env', + ':(glob)**/.env*', + ':(glob)**/*.env', + ':(glob)**/*.env~', + ':(glob)**/*.pem', + ':(glob)**/*.key', + ':(glob)**/*.p12', + ':(glob)**/*.pfx', + ':(glob)**/id_rsa*', + ':(glob)**/id_ed25519*', + ':(glob)**/id_ecdsa*', + ':(glob)**/id_dsa*', + ':(glob)**/*credentials*', + ':(glob)**/.git-credentials', + ':(glob)**/.npmrc', + ':(glob)**/.netrc', + ':(glob)**/gha-creds-*.json', + ':(exclude,glob)**/node_modules/**', + ':(exclude,glob)**/dist/**', +]; + +// A nested clone is a repository git does not descend into, so a credential +// file inside it would silently pass an ignored-file scan. Dependencies and +// build output keep their own exclusion: a vendored `.git` is no more a +// project credential than a vendored `.env`. +const NESTED_REPO_PATHSPECS = [ + ':(glob)**/.git', + ':(exclude,glob)**/node_modules/**', + ':(exclude,glob)**/dist/**', +]; + +const EXCLUDED_SEGMENTS = new Set(['node_modules', 'dist']); + +/** True for a path whose basename is credential-shaped. */ +export function isCredentialShaped(path) { + const name = basename(path); + // A public key is not a credential; refusing id_rsa.pub would be noise. + if (name.endsWith('.pub')) return false; + return CREDENTIAL_BASENAME.test(name); +} + +export function gitScanArgs(root) { + return [ + '-C', + root, + 'ls-files', + '--others', + '--ignored', + '--exclude-standard', + '-z', + '--', + ...CREDENTIAL_PATHSPECS, + ]; +} + +/** + * Returns the nested repositories found under `root`, sorted. Three surfaces + * are enumerated, because git collapses a nested working tree to a directory + * entry on each of them: an ignored clone, an untracked NON-ignored clone + * (the shape a round's own `git clone` leaves behind), and a populated + * submodule (a mode-160000 gitlink whose directory carries a `.git`). + * Dependencies and build output keep their exclusion on every surface: a + * vendored `.git` is no more a project credential than a vendored `.env`. + */ +export function nestedGitRepos(root, { maxBuffer = 8 * 1024 * 1024 } = {}) { + const git = (args) => { + const result = spawnSync('git', ['-C', root, ...args], { + encoding: 'utf8', + maxBuffer, + }); + if (result.status !== 0) { + throw new Error( + `could not list nested repositories: ${(result.stderr || '').trim() || 'git failed'}`, + ); + } + return result.stdout; + }; + const excluded = (path) => + path.split('/').some((segment) => EXCLUDED_SEGMENTS.has(segment)); + const found = new Set(); + const ignored = git([ + 'ls-files', + '--others', + '--ignored', + '--exclude-standard', + '-z', + '--', + ...NESTED_REPO_PATHSPECS, + ]); + for (const path of ignored.split('\0')) { + if (!path || excluded(path)) continue; + found.add(path.replace(/\/$/, '')); + } + const untracked = git(['ls-files', '--others', '--exclude-standard', '-z']); + for (const entry of untracked.split('\0')) { + if (!entry.endsWith('/') || excluded(entry)) continue; + if (existsSync(join(root, entry, '.git'))) { + found.add(entry.replace(/\/$/, '')); + } + } + const staged = git(['ls-files', '--stage', '-z']); + for (const record of staged.split('\0')) { + if (!record.startsWith('160000 ')) continue; + const tab = record.indexOf('\t'); + const path = tab === -1 ? '' : record.slice(tab + 1); + if (!path || excluded(path)) continue; + // An unpopulated gitlink has no working tree to read; its URL is covered + // by the config value scan instead. + if (existsSync(join(root, path, '.git'))) found.add(path); + } + return [...found].sort(); +} + +/** + * Returns local config entries that carry a credential PR code could read + * with plain `git config`. The scan inspects VALUES, not a list of keys: any + * config value can hold a URL with a token (`http.proxy`, a novel mirror + * key, tomorrow's sibling of `pushurl`), so a key allowlist never converges. + * A value hits when it embeds http(s) userinfo or when it is an + * `Authorization` / `Proxy-Authorization` header (the RFC 7235 pair GitHub + * Actions writes into `http.*.extraheader`); other header names stay clean. + * The two URL-bearing KEY families are judged too: `url.*.insteadOf` / + * `url.*.pushInsteadOf` rewrites and `http..*` sections carry their URL + * in the key, not the value. On an http(s) URL any userinfo counts — a token + * used as the username (`https://@…`) is as readable as `user:pass@` — + * except the bare conventional account name `git@` in any spelling, which + * carries no secret, matching `ssh://git@…`. `--includes` follows + * `include.path` the way the `git config --get` PR code uses does, and + * `--list --null` frames each `key\nvalue` record so a multi-line value + * cannot forge entries. When `extensions.worktreeConfig` is set, ordinary + * `git config --get` also reads `.git/config.worktree`, so that scope (with + * its includes) is enumerated too — `--local` alone would report a + * credential there as clean. The file is optional: absent means an empty + * scope, while an existing but unreadable or malformed one refuses. A + * `credential.helper` COMMAND is out of this + * URL/header contract by design (see the design record); prepare disables + * helpers for its own reads. + */ +export function credentialBearingConfig( + root, + { maxBuffer = 8 * 1024 * 1024 } = {}, +) { + const readScope = (scope) => { + const result = spawnSync( + 'git', + ['-C', root, 'config', scope, '--includes', '--list', '--null'], + { encoding: 'utf8', maxBuffer }, + ); + if (result.status !== 0) { + throw new Error( + `could not read local git config: ${(result.stderr || '').trim() || 'git failed'}`, + ); + } + return result.stdout; + }; + const worktreeConfig = spawnSync( + 'git', + [ + '-C', + root, + 'config', + '--local', + '--includes', + '--bool', + 'extensions.worktreeConfig', + ], + { encoding: 'utf8', maxBuffer }, + ); + const scopes = ['--local']; + if (worktreeConfig.status === 0 && worktreeConfig.stdout.trim() === 'true') { + // Enabling the extension does not require the file to exist: an absent + // optional config.worktree is an EMPTY scope (explicit `--worktree + // --list` would exit 128 on it and false-refuse a clean repo). An + // existing but unreadable or malformed file still refuses, because + // readScope throws on any git error. + const gitDir = spawnSync( + 'git', + ['-C', root, 'rev-parse', '--absolute-git-dir'], + { encoding: 'utf8', maxBuffer }, + ); + if (gitDir.status !== 0) { + throw new Error( + `could not read local git config: ${(gitDir.stderr || '').trim() || 'git failed'}`, + ); + } + if (existsSync(join(gitDir.stdout.trim(), 'config.worktree'))) { + scopes.push('--worktree'); + } + } + const hasUserinfo = (value) => { + const match = /^https?:\/\/([^/@\s]+)@/i.exec(value); + // `git` is the conventional account name, not a secret, and account + // names are case-insensitive; any other username-only userinfo may be + // a token, so it stays refused. + return match !== null && match[1].toLowerCase() !== 'git'; + }; + const isAuthHeader = (value) => + /^\s*(?:proxy-)?authorization\s*:/i.test(value); + const hits = []; + for (const scope of scopes) { + for (const record of readScope(scope).split('\0')) { + if (!record) continue; + const newline = record.indexOf('\n'); + const key = newline === -1 ? record : record.slice(0, newline); + const value = newline === -1 ? '' : record.slice(newline + 1); + const rewrite = /^url\.(.*)\.(insteadof|pushinsteadof)$/.exec(key); + const httpSection = /^http\.(.+)\.[^.]+$/.exec(key); + if ( + hasUserinfo(value) || + isAuthHeader(value) || + (rewrite && hasUserinfo(rewrite[1])) || + (httpSection && hasUserinfo(httpSection[1])) + ) { + if (!hits.includes(key)) hits.push(key); + } + } + } + return hits; +} + +/** + * Returns the ignored credential-shaped files under `root`, sorted. An empty + * array is the clean state; a git failure is thrown so the caller can refuse + * rather than treat it as clean, and a nested repository is refused because a + * scan of the outer tree cannot see into it. + */ +export function scanIgnoredCredentials( + root, + { maxBuffer = 8 * 1024 * 1024 } = {}, +) { + const nested = nestedGitRepos(root, { maxBuffer }); + if (nested.length > 0) { + throw new Error( + `nested git repository under ${nested[0]} — scan that clone separately or remove it`, + ); + } + const configHits = credentialBearingConfig(root, { maxBuffer }); + if (configHits.length > 0) { + throw new Error( + `credential-bearing git config in ${configHits[0]} — remove the credential from .git/config`, + ); + } + const result = spawnSync('git', gitScanArgs(root), { + encoding: 'utf8', + maxBuffer, + }); + if (result.status !== 0) { + throw new Error( + `could not list ignored files: ${(result.stderr || '').trim() || 'git failed'}`, + ); + } + return result.stdout + .split('\0') + .filter(Boolean) + .filter((path) => isCredentialShaped(path)) + .sort(); +} + +function main(argv) { + const root = argv[0]; + if (!root || argv.length > 1) { + process.stderr.write( + 'Usage: node scripts/verify-pr-credential-scan.mjs \n', + ); + return 2; + } + let found; + try { + found = scanIgnoredCredentials(root); + } catch (error) { + process.stderr.write(`verify-pr credential scan: ${error.message}\n`); + return 1; + } + for (const path of found) { + process.stdout.write(`credential-shaped ignored entry: ${path}\n`); + } + return found.length === 0 ? 0 : 1; +} + +if (isEntryPoint(import.meta.url, process.argv[1])) { + process.exitCode = main(process.argv.slice(2)); +} diff --git a/scripts/verify-pr-model-proxy.mjs b/scripts/verify-pr-model-proxy.mjs new file mode 100644 index 00000000000..05a677a42d7 --- /dev/null +++ b/scripts/verify-pr-model-proxy.mjs @@ -0,0 +1,345 @@ +#!/usr/bin/env node + +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Host-side model proxy for a local `/verify-pr` round. + * + * A local round needs the same isolation the CI lane provides: the container + * executes untrusted PR code, so the real model credential must stay outside + * it. This proxy holds the real key on the host and lets the container reach + * the model with a per-run bearer token instead. It mirrors the CI lane's + * inline proxy (qwen-triage.yml `start_openai_proxy`), with changes the local + * shape needs: the listen address is a flag (a container cannot reach the + * host's loopback, so bind the Docker bridge address instead), and the + * coordinates are printed as one JSON line for the maintainer to consume. + * Hop-by-hop request headers are stripped rather than forwarded. + * The printed baseUrl uses the listen address, so bind the address clients + * dial — 0.0.0.0 listens on every interface but is not a dialable URL. + * + * The CI lane deliberately keeps its own inline copy: the proxy handles the + * real key, so it must not execute a script from the PR-head checkout. + * + * Usage (from a trusted checkout, never the PR-head tree — this process holds + * the real key and a PR can rewrite a script in its own checkout): + * if [ -t 0 ]; then + * read -rsp 'REVIEW_OPENAI_API_KEY: ' REVIEW_OPENAI_API_KEY; echo + * else + * IFS= read -r REVIEW_OPENAI_API_KEY \ + * < ~/.config/qwen-verify/review-openai-key + * fi + * export REVIEW_OPENAI_API_KEY + * REVIEW_OPENAI_BASE_URL= \ + * node /scripts/verify-pr-model-proxy.mjs \ + * [--listen ] + * + * Reading the key from a terminal when one exists and from a file otherwise + * keeps it out of shell history (an inline assignment is recorded) and keeps + * an agent-driven run, whose shell has no TTY, from blocking on `read`. + * + * Options: + * --listen address to bind (default 127.0.0.1; use the Docker + * bridge address, e.g. 172.17.0.1, for container clients). + * The port is always ephemeral: a fixed port is a binding + * race the CI lane avoids too. + * + * Environment: + * REVIEW_OPENAI_BASE_URL upstream base URL, may carry a path prefix + * REVIEW_OPENAI_API_KEY the real key; never leaves this process + * QWEN_VERIFY_PROXY_TOKEN bearer clients must present (default: random) + * + * On readiness, prints one JSON line: {port, baseUrl, token, nonce}. Give the + * container `baseUrl` as the provider base URL and `token` as its credential; + * `GET /__health` echoes `nonce`. Only `POST /chat/completions` is + * relayed — the path is matched exactly, not as a suffix. + */ + +import { randomBytes } from 'node:crypto'; +import http from 'node:http'; +import { Readable } from 'node:stream'; +import { isEntryPoint } from './lib/entry-point.js'; + +export const DEFAULT_IDLE_TIMEOUT_MS = 240_000; + +// The qwen client tolerates 240 s of silence inside one stream +// (DEFAULT_STREAM_IDLE_TIMEOUT_MS in the OpenAI content generator), so a +// tighter proxy window would cut a completion its own client would have +// waited out. Keep the default equal; a caller may still pass its own value. + +// RFC 7230 hop-by-hop headers describe the client's connection to us, not the +// upstream one. Forwarding them is never correct, and a copied +// transfer-encoding or expect also fights the framing undici chooses for the +// relayed stream. `host` and `content-length` ride along for the same reason. +const STRIPPED_REQUEST_HEADERS = [ + 'host', + 'content-length', + 'connection', + 'keep-alive', + 'proxy-authorization', + 'proxy-authenticate', + 'te', + 'trailer', + 'transfer-encoding', + 'upgrade', + 'expect', +]; + +export function parseArgs(argv) { + const options = { listen: '127.0.0.1' }; + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === '--listen') { + const value = argv[++i]; + // An empty value would reach server.listen(port, ''), which binds + // every interface — past the bridge this proxy is meant to stay on. + if (value === undefined || value === '' || value.startsWith('--')) { + throw new Error('--listen requires a value'); + } + options.listen = value; + } else { + throw new Error(`unknown argument: ${arg}`); + } + } + return options; +} + +/** Strips the trailing slashes a base path may carry; `''` means none. */ +export function normalizeBasePath(pathname) { + const trimmed = (pathname ?? '').replace(/\/+$/, ''); + return trimmed === '/' ? '' : trimmed; +} + +/** + * Assembles the base URL the maintainer hands to the container. A host that + * carries a colon is an IPv6 literal and needs brackets, or `new URL()` + * rejects the string downstream. + */ +export function formatLocalBaseUrl(listen, port, basePath = '') { + const host = listen.includes(':') ? `[${listen}]` : listen; + return `http://${host}:${port}${basePath}`; +} + +/** + * Returns an http.Server that relays only POST /chat/completions to the + * upstream, replacing the client's bearer token with the real key. The idle + * watchdog is per-chunk, not total: qwen tolerates minutes of silence inside + * one stream, so a total cap would cut healthy long completions. It also + * stays quiet while the downstream is paused — backpressure from a slow but + * live client is not upstream idleness — and re-arms on 'drain'. + */ +export function createModelProxy({ + baseUrl, + apiKey, + token, + nonce, + idleTimeoutMs = DEFAULT_IDLE_TIMEOUT_MS, +}) { + if (!baseUrl || !apiKey || !token || !nonce) { + throw new Error('baseUrl, apiKey, token and nonce are all required'); + } + const base = new URL(baseUrl); + const basePath = normalizeBasePath(base.pathname); + + return http.createServer(async (req, res) => { + if (req.url === '/__health') { + // Identity, not just liveness: a squatter cannot know the nonce. + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end(nonce); + return; + } + + let timer; + try { + const incoming = new URL(req.url || '/', 'http://127.0.0.1'); + const target = new URL(base.origin); + + let path = incoming.pathname; + if ( + basePath && + basePath !== '/' && + path !== basePath && + !path.startsWith(`${basePath}/`) + ) { + path = `${basePath}${path.startsWith('/') ? '' : '/'}${path}`; + } + target.pathname = path; + target.search = incoming.search; + + // Exact path, not a suffix: `endsWith('/chat/completions')` would also + // relay `/admin/chat/completions` to the upstream. + const allowedPath = basePath + ? `${basePath}/chat/completions` + : '/chat/completions'; + if (req.method !== 'POST' || target.pathname !== allowedPath) { + res.writeHead(403, { 'content-type': 'text/plain' }); + res.end(`proxy: only POST ${allowedPath} is allowed\n`); + return; + } + + // Only the agent knows this run's token; anything else that finds the + // port cannot get the key used. + if (req.headers.authorization !== `Bearer ${token}`) { + res.writeHead(401, { 'content-type': 'text/plain' }); + res.end('proxy: unauthorized\n'); + return; + } + + const headers = new Headers(req.headers); + for (const name of STRIPPED_REQUEST_HEADERS) headers.delete(name); + // RFC 7230 §6.1: a header nominated by Connection is hop-by-hop too, + // so a client cannot smuggle one through under an unlisted name. + const nominated = String(req.headers.connection ?? '') + .split(',') + .map((name) => name.trim().toLowerCase()) + .filter(Boolean); + for (const name of nominated) headers.delete(name); + headers.set('authorization', `Bearer ${apiKey}`); + + const init = { method: req.method, headers }; + init.body = req; + init.duplex = 'half'; + + const controller = new AbortController(); + let body = null; + let finished = false; + const arm = () => { + if (finished) return; + clearTimeout(timer); + timer = setTimeout(() => { + // A paused body is downstream backpressure, not upstream silence: + // the client is alive but slow, and aborting here would cut a + // healthy completion. The 'drain' listener re-arms this window. + if (body?.isPaused()) { + arm(); + return; + } + controller.abort(); + }, idleTimeoutMs); + }; + // Registered before the fetch: a client that disconnects while the + // upstream headers are still in flight has already emitted 'close', so + // registering after the response would never abort that request. + res.on('close', () => { + finished = true; + clearTimeout(timer); + controller.abort(); + }); + res.on('drain', arm); + // A slow request UPLOAD is activity too: fetch resolves only on the + // upstream headers, so without this a large prompt on a slow link + // would be torn down mid-upload by a window meant for stream silence. + // 'readable', not 'data': a data listener switches the request to + // flowing mode and steals the body chunks from the relayed stream. + req.on('readable', arm); + arm(); + let upstream; + try { + upstream = await fetch(target, { ...init, signal: controller.signal }); + } catch (error) { + if (error instanceof Error && error.name === 'AbortError') { + if (!res.destroyed && !res.writableEnded) { + res.writeHead(504, { 'content-type': 'text/plain' }); + res.end('proxy error: upstream request timed out\n'); + } + return; + } + throw error; + } + if (res.destroyed) { + finished = true; + clearTimeout(timer); + return; + } + // The timer is deliberately NOT cleared here: fetch() resolves on + // HEADERS, and the armed timer doubles as the first idle window for + // the body; the per-chunk handler below refreshes it. + const responseHeaders = {}; + upstream.headers.forEach((value, key) => { + const lower = key.toLowerCase(); + if (lower !== 'content-encoding' && lower !== 'content-length') { + responseHeaders[key] = value; + } + }); + res.writeHead(upstream.status, responseHeaders); + if (upstream.body) { + body = Readable.fromWeb(upstream.body); + const done = () => { + finished = true; + clearTimeout(timer); + }; + body.on('end', done); + // An idle watchdog firing mid-body aborts the upstream, which + // surfaces here as an error: tear down the downstream response too. + body.on('error', () => { + done(); + res.destroy(); + }); + body.pipe(res); + body.on('data', arm); + } else { + finished = true; + clearTimeout(timer); + res.end(); + } + } catch (error) { + clearTimeout(timer); + console.error('proxy upstream failure:', error); + res.writeHead(502, { 'content-type': 'text/plain' }); + res.end('proxy error: upstream request failed\n'); + } + }); +} + +async function main(argv) { + let options; + try { + options = parseArgs(argv); + } catch (error) { + process.stderr.write(`${error.message}\n`); + return 2; + } + const baseUrl = process.env.REVIEW_OPENAI_BASE_URL; + const apiKey = process.env.REVIEW_OPENAI_API_KEY; + if (!baseUrl || !apiKey) { + process.stderr.write( + 'REVIEW_OPENAI_BASE_URL and REVIEW_OPENAI_API_KEY must both be set\n', + ); + return 2; + } + const token = + process.env.QWEN_VERIFY_PROXY_TOKEN || randomBytes(24).toString('hex'); + const nonce = randomBytes(24).toString('hex'); + const server = createModelProxy({ baseUrl, apiKey, token, nonce }); + await new Promise((resolve, reject) => { + server.once('error', reject); + server.listen(0, options.listen, resolve); + }); + const { port } = server.address(); + const localBaseUrl = formatLocalBaseUrl( + options.listen, + port, + normalizeBasePath(new URL(baseUrl).pathname), + ); + // One JSON line, on stdout: the maintainer pastes baseUrl/token into the + // container's provider configuration. The real key is never printed. + process.stdout.write( + `${JSON.stringify({ port, baseUrl: localBaseUrl, token, nonce })}\n`, + ); + return 0; +} + +if (isEntryPoint(import.meta.url, process.argv[1])) { + main(process.argv.slice(2)).then( + (code) => { + process.exitCode = code; + }, + (error) => { + console.error(error); + process.exitCode = 1; + }, + ); +} diff --git a/scripts/verify-pr-prepare.mjs b/scripts/verify-pr-prepare.mjs index 542497c866f..d5f5cb27ccf 100644 --- a/scripts/verify-pr-prepare.mjs +++ b/scripts/verify-pr-prepare.mjs @@ -46,7 +46,7 @@ */ import { spawnSync } from 'node:child_process'; -import { existsSync, mkdirSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from 'node:fs'; import { homedir } from 'node:os'; import { join } from 'node:path'; import { isEntryPoint } from './lib/entry-point.js'; @@ -272,6 +272,126 @@ function run(command, args) { return spawnSync(command, args, { encoding: 'utf8' }); } +// The tree under inspection is PR-controlled and can persist hostile git +// config in .git/config: fsmonitor, hooks and clean/smudge/process filters all +// execute on an ordinary `git status`. On the host these run as the maintainer, +// outside the boundary, so every git command disables the fixed vectors and +// empties every locally defined filter before reading the tree. The fixed +// vectors come LAST in every argument list: git applies the last `-c` for a +// key, so an enumerated entry must never be able to outrank them. +export const GIT_HARDENING = [ + '-c', + 'core.fsmonitor=false', + '-c', + 'core.hooksPath=/dev/null', + '-c', + 'credential.helper=', + '-c', + 'core.excludesFile=/dev/null', +]; + +const FILTER_NAME = /^filter\..+\.(clean|smudge|process)$/; + +/** + * A filter name git allows (e.g. `filter.a=b.clean`) but `-c key=value` + * cannot represent: git splits the argument at the FIRST `=`, so emitting an + * override would set a different key and leave the real filter armed. The + * gate refuses such names instead, with their own remedy. + */ +export class UnsafeFilterNameError extends Error { + constructor(name) { + super( + `filter name ${name} contains '=' and cannot be safely disabled with -c — remove the filter from the tree's git config, or re-clone, then re-run prepare`, + ); + this.name = 'UnsafeFilterNameError'; + } +} + +/** + * Empty `-c` overrides for every locally defined filter command. The filter + * name is attacker-chosen, so the override set has to be enumerated from the + * tree's own config instead of hardcoded. Three properties are load-bearing: + * `--includes` follows `include.path` the way the config parsing `git status` + * uses does; `--null --name-only` frames KEY NAMES only, because a filter + * VALUE may contain newlines that would forge extra entries if the rendered + * `key value` output were parsed; and each name is re-validated before it + * becomes a `-c` argument. When `extensions.worktreeConfig` is set, + * `git status` also reads `.git/config.worktree`, so that scope is enumerated + * too. Names containing `=` throw UnsafeFilterNameError — no override can + * represent them. + */ +export function localFilterOverrides(root) { + const prefix = root ? ['-C', root] : []; + const scopes = ['--local']; + const worktreeConfig = run('git', [ + ...prefix, + 'config', + '--local', + '--includes', + '--bool', + 'extensions.worktreeConfig', + ]); + if (worktreeConfig.status === 0 && worktreeConfig.stdout.trim() === 'true') { + scopes.push('--worktree'); + } + const overrides = []; + for (const scope of scopes) { + const result = run('git', [ + ...prefix, + 'config', + scope, + '--includes', + '--null', + '--name-only', + '--get-regexp', + '^filter\\..*\\.(clean|smudge|process)$', + ]); + if (result.status === 1) continue; + if (result.status !== 0) { + throw new Error('could not enumerate local git filters'); + } + for (const name of result.stdout.split('\0')) { + if (!name || !FILTER_NAME.test(name)) continue; + if (name.includes('=')) throw new UnsafeFilterNameError(name); + overrides.push('-c', `${name}=`); + } + } + return overrides; +} + +export function gitStatusArgs(root) { + return [ + ...localFilterOverrides(root), + ...GIT_HARDENING, + '-C', + root, + 'status', + '--porcelain', + '--untracked-files=normal', + ]; +} + +function runGit(args) { + return run('git', [...localFilterOverrides(), ...GIT_HARDENING, ...args]); +} + +/** + * True when `.git/info/exclude` carries active patterns. A fresh clone has + * only git's commented default there; patterns hide untracked files from the + * clean-tree check, and unlike core.excludesFile no `-c` can neutralize this + * file, so the gate refuses it instead of reading a steered status. + */ +export function hasHostileInfoExclude(root) { + const path = join(root, '.git', 'info', 'exclude'); + if (!existsSync(path)) return false; + return readFileSync(path, 'utf8') + .split('\n') + .some((line) => { + const trimmed = line.trim(); + return trimmed !== '' && !trimmed.startsWith('#'); + }); +} + function report(lines) { process.stdout.write(`${lines.join('\n')}\n`); } @@ -296,7 +416,17 @@ function main(argv) { const { owner, name } = parseRepo(options.repo); const slug = `${owner}/${name}`; - const toplevel = run('git', ['rev-parse', '--show-toplevel']); + let toplevel; + try { + toplevel = runGit(['rev-parse', '--show-toplevel']); + } catch (error) { + // A hostile filter name is a refusal with its own remedy, not a missing + // worktree; it must surface before any guarded status could run it. + if (error instanceof UnsafeFilterNameError) { + return fail(3, error.message); + } + return fail(3, 'not inside a git worktree — run this from the clone'); + } if (toplevel.status !== 0) { return fail(3, 'not inside a git worktree — run this from the clone'); } @@ -338,7 +468,7 @@ function main(argv) { ]; for (const [label, oid, ref] of probes) { if ( - run('git', ['-C', root, 'cat-file', '-e', `${oid}^{commit}`]).status !== 0 + runGit(['-C', root, 'cat-file', '-e', `${oid}^{commit}`]).status !== 0 ) { return fail( 3, @@ -351,7 +481,7 @@ function main(argv) { // than the PR head or an uncommitted change would measure code the report // does not name. Checked before the artifact directory exists, so a refusal // leaves nothing behind. - const treeHead = run('git', ['-C', root, 'rev-parse', 'HEAD']); + const treeHead = runGit(['-C', root, 'rev-parse', 'HEAD']); if (treeHead.status !== 0) { return fail(3, 'could not read HEAD in this clone'); } @@ -361,15 +491,19 @@ function main(argv) { `current tree is at ${treeHead.stdout.trim()}, not PR head ${context.head} — run: git checkout ${context.head}, then re-run prepare`, ); } + // The ignore channel is steerable from the same PR-controlled .git: + // core.excludesFile is neutralized by GIT_HARDENING, but .git/info/exclude + // has no `-c` override, so active patterns there are refused outright + // rather than read through. + if (hasHostileInfoExclude(root)) { + return fail( + 3, + '.git/info/exclude carries local exclude patterns, which can hide untracked files from the clean-tree check — re-clone (a fresh clone has only the commented default), or clear the file, then re-run prepare', + ); + } // Explicit mode: a maintainer's status.showUntrackedFiles=no would otherwise // hide the untracked files this check exists to refuse. - const dirty = run('git', [ - '-C', - root, - 'status', - '--porcelain', - '--untracked-files=normal', - ]); + const dirty = run('git', gitStatusArgs(root)); if (dirty.status !== 0) { const detail = (dirty.stderr || '').trim() || `exit ${dirty.status}`; return fail(3, `could not read the working tree status: ${detail}`); @@ -387,7 +521,7 @@ function main(argv) { // git status trusts the index: skip-worktree / assume-unchanged entries // (a sparse checkout sets them en masse) hide tracked divergence from it, // so refuse anything that is not a plain cached 'H' entry. - const flagged = run('git', ['-C', root, 'ls-files', '-v']); + const flagged = runGit(['-C', root, 'ls-files', '-v']); if (flagged.status !== 0) { const detail = (flagged.stderr || '').trim() || `exit ${flagged.status}`; return fail(3, `could not read the index flags: ${detail}`);