fix: prevent hang in restoreConfigFromBase on repos with .gitmodules (#1166)
When a PR head contains `.gitmodules`, git's default `fetch.recurseSubmodules=on-demand` config causes `git fetch` to attempt submodule object fetches. In CI (no credentials), this blocks indefinitely waiting for auth — producing ~4-hour hangs reported in #1088. Two changes, both defence-in-depth: 1. Delete SENSITIVE_PATHS *before* fetching. The attacker-controlled `.gitmodules` is absent during the network operation, so git never sees a submodule config to follow regardless of git settings. 2. Pass `--no-recurse-submodules` to the fetch. Suppresses submodule fetching explicitly, independent of any git config on the runner. The original order (fetch-then-delete) was a brief window where `.gitmodules` from the PR head could influence the fetch. Reordering also tightens the security property: if `git checkout` below fails, the attacker-controlled file is already gone rather than present during fetch. Fixes #1088. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
b15d4751a6
commit
f328a5c889
@ -44,20 +44,30 @@ export function restoreConfigFromBase(baseBranch: string): void {
|
|||||||
`Restoring ${SENSITIVE_PATHS.join(", ")} from origin/${baseBranch} (PR head is untrusted)`,
|
`Restoring ${SENSITIVE_PATHS.join(", ")} from origin/${baseBranch} (PR head is untrusted)`,
|
||||||
);
|
);
|
||||||
|
|
||||||
// Fetch base first — if this fails we haven't touched the workspace and the
|
// Delete PR-controlled versions BEFORE fetching so the attacker-controlled
|
||||||
// caller sees a clean error.
|
// .gitmodules is absent during the network operation. If git reads .gitmodules
|
||||||
execFileSync("git", ["fetch", "origin", baseBranch, "--depth=1"], {
|
// during fetch (fetch.recurseSubmodules=on-demand, the git default), it will
|
||||||
stdio: "inherit",
|
// attempt to fetch submodule objects and block on credential prompts in CI —
|
||||||
env: process.env,
|
// causing an indefinite hang. Deleting first closes that window.
|
||||||
});
|
//
|
||||||
|
// If the restore below fails for a given path, that path stays deleted —
|
||||||
// Delete PR-controlled versions. If the restore below fails for a given path,
|
// the safe fallback (no attacker-controlled config). A bare `git checkout`
|
||||||
// that path stays deleted — the safe fallback (no attacker-controlled config).
|
// alone wouldn't remove files the PR added, so nuke first.
|
||||||
// A bare `git checkout` alone wouldn't remove files the PR added, so nuke first.
|
|
||||||
for (const p of SENSITIVE_PATHS) {
|
for (const p of SENSITIVE_PATHS) {
|
||||||
rmSync(p, { recursive: true, force: true });
|
rmSync(p, { recursive: true, force: true });
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// --no-recurse-submodules: explicitly suppress submodule fetching regardless of
|
||||||
|
// fetch.recurseSubmodules config. Defense-in-depth alongside the delete above.
|
||||||
|
execFileSync(
|
||||||
|
"git",
|
||||||
|
["fetch", "origin", baseBranch, "--depth=1", "--no-recurse-submodules"],
|
||||||
|
{
|
||||||
|
stdio: "inherit",
|
||||||
|
env: process.env,
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
for (const p of SENSITIVE_PATHS) {
|
for (const p of SENSITIVE_PATHS) {
|
||||||
try {
|
try {
|
||||||
execFileSync("git", ["checkout", `origin/${baseBranch}`, "--", p], {
|
execFileSync("git", ["checkout", `origin/${baseBranch}`, "--", p], {
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user