-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(ci): make autofix verification gates hermetic to runner git config #8961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ef871f3
1213ee8
e0031b8
e7cb3b4
1cfa5e0
aff0a97
0dcf714
39b9d12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| #!/usr/bin/env bash | ||
| set -uo pipefail | ||
|
|
||
| # Re-sanitizes the git config surfaces a PAT-bearing git step is about to | ||
| # read, AFTER branch/agent code has run on the host. The inlined job-start | ||
| # sanitize steps are pre-checkout hygiene; between them and the push, the | ||
| # verification gates run branch test code on the host and the sandboxed | ||
| # agent has the workspace mounted — either can plant exec keys in the | ||
| # repo's LOCAL .git/config (the highest-precedence file, which the push | ||
| # reads) or rewrite the runner user's REAL global config: the gates' env | ||
| # redirect is inherited-env enforcement, not a filesystem boundary — a | ||
| # direct file write, `env -u GIT_CONFIG_GLOBAL git config --global`, or | ||
| # `git config --file "$HOME/.gitconfig"` all bypass it (probe-verified in | ||
| # the #8961 review). | ||
| # | ||
| # Invoked as `bash "${RUNNER_TEMP}/resanitize-git-config.sh"` from the | ||
| # copy the staging step took off the TRUSTED base checkout — never from | ||
| # the working tree, which holds the branch under test at call time. | ||
| # | ||
| # The allowlist and denylist are copies of the inlined pre-checkout | ||
| # sanitize steps in qwen-autofix.yml (which cannot call this script: it | ||
| # does not exist on disk before their checkout). The workflow contract | ||
| # tests pin every copy byte-identical — edit them together. | ||
|
|
||
| if [ -e .git ]; then | ||
| # Repo-scope redirect files first. `.git/commondir` (the file twin of | ||
| # GIT_COMMON_DIR) repoints local config, refs AND objects — a plant makes | ||
| # the very --local sweep below act on the ATTACKER's config, and lets the | ||
| # PAT push deliver attacker content; `.git/shallow` (twin of | ||
| # GIT_SHALLOW_FILE) narrows the object graph. A normal actions/checkout is | ||
| # not a linked worktree, so neither file legitimately exists here — | ||
| # removing them cannot break a real checkout, only defuse a plant. Then | ||
| # config.worktree (can carry core.hooksPath, invisible to `git config | ||
| # --local`), then the local allowlist sweep. | ||
| GIT_DIR_PATH="$(git rev-parse --git-dir 2>/dev/null || echo .git)" | ||
| rm -f "${GIT_DIR_PATH}/commondir" "${GIT_DIR_PATH}/shallow" 2>/dev/null || true | ||
| rm -f "$(git rev-parse --git-path config.worktree 2>/dev/null || echo /nonexistent)" 2>/dev/null || true | ||
| git config --local --unset-all extensions.worktreeConfig 2>/dev/null || true | ||
|
Comment on lines
+37
to
+38
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] resanitize removes Failure scenario (probe-verified end-to-end with the PAT step's exact env shape — Suggested fix: in the 中文说明resanitize 删除 失败场景(已用 PAT 步骤的精确 env 形态端到端探针验证—— 修复建议: 在 — qwen3.8-max via Qwen Code /review (v0.21.10)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 1cfa5e0 — resanitize now |
||
| git config --local --name-only --list 2>/dev/null \ | ||
| | { grep -ivE '^(core\.(repositoryformatversion|bare|filemode|symlinks|ignorecase|precomposeunicode|logallrefupdates|worktree|hidedotfiles|protecthfs|protectntfs)|remote\..+\.(url|fetch|pushurl)|branch\.|extensions\.|gc\.|pack\.|fetch\.|index\.|safe\.|submodule\..+\.(url|active|branch))' || true; } \ | ||
| | while IFS= read -r key; do git config --local --unset-all "$key" 2>/dev/null || true; done | ||
| fi | ||
| # The GLOBAL scope spans TWO files — ~/.gitconfig and | ||
| # ${XDG_CONFIG_HOME:-~/.config}/git/config — but with both present, | ||
| # `git config --global` lists and unsets ONLY ~/.gitconfig (probed on | ||
| # git 2.43 and 2.55: the listing omits the XDG keys and --unset-all | ||
| # exits 5 with them live), so sweep each file explicitly by pointing | ||
| # GIT_CONFIG_GLOBAL at it — the env var replaces the whole global | ||
| # scope with exactly that file, for reads and writes alike. | ||
| for global_file in "${HOME}/.gitconfig" "${XDG_CONFIG_HOME:-${HOME}/.config}/git/config"; do | ||
| { GIT_CONFIG_GLOBAL="${global_file}" git config --global --name-only --list 2>/dev/null || true; } \ | ||
| | { grep -iE '^(core\.(hookspath|fsmonitor|pager|editor|sshcommand|askpass|alternaterefscommand|gitproxy)$|diff\.external$|diff\..+\.(command|textconv)$|merge\..+\.driver$|filter\.|alias\.|pager\.|difftool\.|mergetool\.|interactive\.difffilter$|sequence\.editor$|gpg\.(.+\.)?program$|init\.templatedir$|remote\..+\.(uploadpack|receivepack)$|submodule\..+\.update$|url\..+\.(insteadof|pushinsteadof)$|http\.(.+\.)?(sslverify|sslcainfo)$|include\.|includeif\.|protocol\.(ext\.)?allow$)' || true; } \ | ||
| | while IFS= read -r key; do GIT_CONFIG_GLOBAL="${global_file}" git config --global --unset-all "$key" 2>/dev/null || true; done | ||
| done | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,47 @@ set -eo pipefail | |||||||||||||||
| # environment from the caller. WORKDIR and BRANCH are job-level env; | ||||||||||||||||
| # GITHUB_OUTPUT and RUNNER_TEMP are runner-provided. None is defined here. | ||||||||||||||||
|
|
||||||||||||||||
| # Deterministic verification must not read the RUNNER's git config: the | ||||||||||||||||
| # persistent pool accumulates state, and a leaked global exec knob fails | ||||||||||||||||
| # branch tests the branch never caused. Measured counterexample, run | ||||||||||||||||
| # 31516789251: a stray `diff.external=global-driver` in the runner user's | ||||||||||||||||
| # ~/.gitconfig killed four per-hunk probe tests in packages/cli on #8613 — | ||||||||||||||||
| # charged to the round (package tests are A/B-exempt), which burned the | ||||||||||||||||
| # 18-minute repair on a failure no repair can reach and ended the round as | ||||||||||||||||
| # a timeout. Every git this script or its checks spawn (vitest fixture | ||||||||||||||||
| # repos included) reads a per-run throwaway global config instead — seeded | ||||||||||||||||
| # with the workspace safe.directory actions/checkout put in the real one — | ||||||||||||||||
| # and no system config — any system-level git setting the checks ever | ||||||||||||||||
| # come to depend on (a CA bundle, a proxy) must be replicated via per-job | ||||||||||||||||
| # env, not /etc/gitconfig, because the redirect silently drops it. The | ||||||||||||||||
| # redirect also keeps a branch-authored `git config --global` from writing | ||||||||||||||||
| # durable state onto the host: it lands in the throwaway file and dies | ||||||||||||||||
| # with the run. Enforcement is inherited-env only — branch code writing | ||||||||||||||||
| # the real file directly bypasses it, which is why the PAT-bearing steps | ||||||||||||||||
| # re-run resanitize-git-config.sh afterwards. | ||||||||||||||||
| # Environment-carried config outranks BOTH file redirects and defeats | ||||||||||||||||
| # every file-level guard: GIT_CONFIG_COUNT/_PARAMETERS carry config at | ||||||||||||||||
| # command-line precedence, GIT_SSL_* / GIT_PROXY_COMMAND steer transport, | ||||||||||||||||
| # GIT_EXEC_PATH swaps the transport-helper binary, GIT_DIR/GIT_WORK_TREE | ||||||||||||||||
| # repoint git, GIT_ASKPASS/GIT_SSH* hijack auth/exec — branch code in an | ||||||||||||||||
| # earlier step can inject any of them through $GITHUB_ENV. Strip them, then | ||||||||||||||||
| # redirect the file scopes. Keep this env+redirect block equal to the | ||||||||||||||||
| # issue-fix gate's copy (the contract test pins them). | ||||||||||||||||
| unset GIT_CONFIG_PARAMETERS GIT_ALLOW_PROTOCOL GIT_PROXY_COMMAND \ | ||||||||||||||||
| GIT_SSL_NO_VERIFY GIT_SSL_CAINFO GIT_EXEC_PATH GIT_DIR \ | ||||||||||||||||
|
Comment on lines
+34
to
+35
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R4-4: all four byte-identical env-strip blocks (this one + twins at workflow ~1469, ~1661, ~5096) still omit git's command-executing environment variables
Suggested change
中文说明四份逐字节一致的 env 清洗块(本块 + workflow ~1469、~1661、~5096 三处孪生)仍遗漏 git 的命令执行类环境变量 — qwen3.8-max via Qwen Code /review (v0.21.10)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged — a test-strengthening refinement. The round-4/5 pins already cover the core mechanism (full unset var set, mktemp path, exact-SHA push, digest verify line verbatim, three identical PAT preambles); this incremental pin-tightening is noted for the test-hardening follow-up rather than blocking the landing, per the maintainer's decision to close out this PR on the reliability fix + surface reduction. |
||||||||||||||||
| GIT_WORK_TREE GIT_COMMON_DIR GIT_OBJECT_DIRECTORY \ | ||||||||||||||||
| GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_SHALLOW_FILE \ | ||||||||||||||||
| GIT_ASKPASS GIT_SSH GIT_SSH_COMMAND | ||||||||||||||||
|
Comment on lines
+36
to
+38
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R5-13: None of the five strip lists (this script, the issue-gate inline twin, the three PAT preambles) unsets
Suggested change
(add to all five copies and to the contract test's per-variable unset pin.) 中文说明五份清除列表(本脚本、issue 门内联孪生、三个 PAT 前置块)都没有 unset — qwen3.8-max via Qwen Code /review (v0.21.10)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Acknowledged — a test-strengthening refinement. The round-4/5 pins already cover the core mechanism (full unset var set, mktemp path, exact-SHA push, digest verify line verbatim, three identical PAT preambles); this incremental pin-tightening is noted for the test-hardening follow-up rather than blocking the landing, per the maintainer's decision to close out this PR on the reliability fix + surface reduction. |
||||||||||||||||
| export GIT_CONFIG_COUNT=0 | ||||||||||||||||
| export GIT_TERMINAL_PROMPT=0 | ||||||||||||||||
| export GIT_CONFIG_SYSTEM=/dev/null | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Failure scenario: a self-hosted runner with corporate proxy in 中文说明[Suggestion] 失败场景:带企业代理的自托管 runner 在 — deepseek-v4-flash via Qwen Code /review (v0.21.10)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in e0031b8 — both gates emit a guarded |
||||||||||||||||
| export GIT_CONFIG_GLOBAL="${RUNNER_TEMP}/autofix-gate-gitconfig" | ||||||||||||||||
|
Comment on lines
+41
to
+42
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] GIT_CONFIG_SYSTEM=/dev/null redirect undocumented at the system level The throwaway redirect sets GIT_CONFIG_SYSTEM=/dev/null, which silently drops all system-level git config. A future maintainer adds a system-level git config entry that the gates depend on (e.g., a CA bundle for registry access, a proxy, or a credential helper). The redirect silently bypasses it, and the gate fails with an opaque error (TLS error, auth failure). The comment block documents the global config rationale (pool runner pollution) but the system config bypass is not separately explained.
Suggested change
— deepseek-v4-flash via Qwen Code /review (v0.21.8)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 1213ee8 — both gates' comment blocks now state that system config is bypassed too and that any system-level git setting the checks come to depend on (CA bundle, proxy) must be replicated via per-job env, not /etc/gitconfig.
Comment on lines
+41
to
+42
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] Every guard this diff adds operates on git config files, but gate branch code also inherits Failure scenario: probe-verified at the git level — env Suggested fix: add 中文说明[Critical] 本 diff 新增的所有防御都作用于 git 配置文件,但门里的分支代码同样继承了 失败场景:git 层面已探针验证——即使 修复建议:在此处、issue 门的重定向孪生处、以及 PAT 步骤 shell 的首条 git 调用前加 — qwen3.8-max via Qwen Code /review (v0.21.10)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in e0031b8 — |
||||||||||||||||
| : > "${GIT_CONFIG_GLOBAL}" | ||||||||||||||||
| git config --file "${GIT_CONFIG_GLOBAL}" safe.directory "$(pwd)" | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Test efficacy: hunk-survived — the Failure scenario: the redirect is silently removed or broken; the ordering test still passes (it checks string presence, not behavior), and the gate runs exposed to the host's global config. 中文说明[Suggestion] 测试有效性:hunk 存活—— 失败场景:重定向被静默移除或破坏;顺序测试仍然通过(它检查字符串存在而非行为),门暴露在宿主机的全局配置下运行。 — deepseek-v4-flash via Qwen Code /review (v0.21.10)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in e0031b8 — the contract test now EXECUTES the extracted redirect block: hostile HOME ( |
||||||||||||||||
| if [ -s /etc/gitconfig ]; then | ||||||||||||||||
| echo "::notice::/etc/gitconfig exists but is bypassed by the gate's GIT_CONFIG_SYSTEM redirect — replicate any setting the checks need via per-job env." | ||||||||||||||||
| fi | ||||||||||||||||
|
|
||||||||||||||||
| # Record whether the agent left a commit FIRST — this is a ref-only | ||||||||||||||||
| # diff, so it runs before the failure.md early-exits and covers an | ||||||||||||||||
| # agent that commits and then aborts. The failure handoff keys its | ||||||||||||||||
|
|
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] resanitize never touches
.git/shallow— the FILE twin ofGIT_SHALLOW_FILE. The env-unset remedy for that channel (the separate unset-list finding) cannot close a DIRECTLY PLANTED.git/shallowfile, which needs no env var at all. Both PAT-path checkouts usefetch-depth: 0(workflow ~870 and ~3733), so a legitimate clone carries no shallow file — anything present is planted, and removing it cannot damage legitimate state.Failure scenario (probe-verified, direct file plant, no env var):
echo garbage > .git/shallowmakesgit status --porcelain,commit,logandmergeall exit 128fatal: bad shallow line: garbage;rm -f .git/shallowflips them back to rc=0. Same parse path as the env-channel probe — so after a green gate the PAT step's push/salvage hard-fails and the verified round is discarded under a misleading diagnosis; the channel is unaffected by unsettingGIT_SHALLOW_FILE.Suggested fix: in the same
if [ -e .git ]block:rm -f "$(git rev-parse --git-path shallow 2>/dev/null || echo /nonexistent)" 2>/dev/null || true(safe for thesefetch-depth: 0checkouts), or fail closed when.git/shallowexists in afetch-depth: 0workspace.中文说明
resanitize 从不触及
.git/shallow——GIT_SHALLOW_FILE的文件孪生。针对该通道的 env unset 修复(另一条 unset 列表发现)无法封闭直接植入的.git/shallow文件——它完全不需要环境变量。两个 PAT 路径的 checkout 都用fetch-depth: 0(workflow ~870 与 ~3733),合法克隆不携带 shallow 文件——存在的必是植入,删除不会损害合法状态。失败场景(已探针验证,直接文件植入、无 env 变量):
echo garbage > .git/shallow让git status --porcelain、commit、log、merge全部 exit 128fatal: bad shallow line: garbage;rm -f .git/shallow翻回 rc=0。与 env 通道探针同一解析路径——于是绿色门之后 PAT 步骤的 push/salvage 硬失败,已验证轮次在误导性诊断下被丢弃;该通道不受 unsetGIT_SHALLOW_FILE影响。修复建议: 在同一
if [ -e .git ]块中:rm -f "$(git rev-parse --git-path shallow 2>/dev/null || echo /nonexistent)" 2>/dev/null || true(对这些fetch-depth: 0checkout 安全),或在fetch-depth: 0工作区中出现.git/shallow时失败关闭。— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 1cfa5e0 — resanitize now removes
.git/shallowalongside.git/commondir(both file twins of the env channels, both defused).