From 43b308007831d4d2a3b0923493319fbec6ddf8ee Mon Sep 17 00:00:00 2001 From: Pranit Date: Thu, 22 Oct 2026 16:30:59 -0400 Subject: [PATCH] Review git fetch from repositories that name a remote helper git runs a git-remote- program, which it may find on PATH, for a repository written ::
and for a URL whose scheme has no built-in transport. Auto mode now sends those git fetch commands to review, including after --. https, ssh, git, and file URLs still run without review. --- src/core/shell_command/command_effect.zig | 58 +++++++++++++++++++++-- tests/e2e/auto-mode-reliability.test.ts | 43 +++++++++++++++++ 2 files changed, 97 insertions(+), 4 deletions(-) diff --git a/src/core/shell_command/command_effect.zig b/src/core/shell_command/command_effect.zig index e276b4be2..39841b005 100644 --- a/src/core/shell_command/command_effect.zig +++ b/src/core/shell_command/command_effect.zig @@ -189,12 +189,44 @@ const reversible_git_fetch_value_options = [_][]const u8{ }; fn reversibleGitFetch(words: []const command_lex.ArgvToken) bool { + var options_ended = false; for (words) |word| { const value = word.value; - // Everything after `--` names a repository or a refspec. - if (std.mem.eql(u8, value, "--")) return true; - if (value.len == 0 or value[0] != '-') continue; - if (!isReversibleGitFetchOption(value)) return false; + if (!options_ended and std.mem.eql(u8, value, "--")) { + // Everything after `--` names a repository or a refspec. + options_ended = true; + continue; + } + if (!options_ended and value.len > 0 and value[0] == '-') { + if (!isReversibleGitFetchOption(value)) return false; + continue; + } + if (namesGitRemoteHelper(value)) return false; + } + return true; +} + +/// Transports git runs itself, case-insensitively. git sends any other +/// `://` URL to a `git-remote-` program, and `http` and +/// `https` to the ones in its own exec path, which it searches before `PATH`. +const builtin_git_url_schemes = [_][]const u8{ "file", "git", "ssh", "git+ssh", "ssh+git", "http", "https" }; + +/// Whether git hands this repository to a remote helper it may find on +/// `PATH`: `::
`, or a URL whose scheme is not a built-in +/// transport. git reads the helper's name the way `transport_get` does. +fn namesGitRemoteHelper(value: []const u8) bool { + var end: usize = 0; + while (end < value.len) : (end += 1) { + const byte = value[end]; + const special = byte == '+' or byte == '-' or byte == '.'; + if (!std.ascii.isAlphanumeric(byte) and (end == 0 or !special)) break; + } + if (end == 0) return false; + const rest = value[end..]; + if (std.mem.startsWith(u8, rest, "::")) return true; + if (!std.mem.startsWith(u8, rest, "://")) return false; + for (builtin_git_url_schemes) |scheme| { + if (std.ascii.eqlIgnoreCase(value[0..end], scheme)) return false; } return true; } @@ -269,6 +301,12 @@ test "known reversible auto commands exclude destructive and hidden effects" { "git fetch --depth=1 origin main", "git fetch --no-tags origin +main:main", "git fetch origin -- --not-an-option", + "git fetch https://github.com/vercel-labs/fx main", + "git fetch HTTPS://github.com/vercel-labs/fx", + "git fetch ssh://git@github.com/vercel-labs/fx.git", + "git fetch git@github.com:vercel-labs/fx.git", + "git fetch file:///tmp/repository", + "git fetch ../other main", "npm install 2>&1", "npm run dev", "zig build test", @@ -306,6 +344,17 @@ test "known reversible auto commands exclude destructive and hidden effects" { "git fetch '--upload-pack=x' .", // A filter turns a full clone into a partial clone. "git fetch --filter=blob:none origin", + // git runs a `git-remote-` program, which it may find on + // `PATH`, for these repositories, also after `--`. + "git fetch probe::x", + "git fetch ext::sh", + "git fetch 'probe.v2+x::y' main", + "git fetch -- probe::x", + "git fetch origin -- probe::x", + "git fetch --multiple origin probe::x", + "git fetch probe://host/repository", + "git fetch PROBE://host/repository", + "git fetch ftp://host/repository", // zsh's `extendedglob` expands these to workspace file names. "git fetch ^nomatch .", "git fetch --prun#e", @@ -2296,6 +2345,7 @@ const parity_review_free_commands = [_][]const u8{ "git fetch --all --tags", "git fetch --depth=1 origin main", "git fetch -q origin -- main", + "git fetch https://example.com/repository main", "npm install", "npm i", "npm ci", diff --git a/tests/e2e/auto-mode-reliability.test.ts b/tests/e2e/auto-mode-reliability.test.ts index cea9eebf0..213f8dd58 100644 --- a/tests/e2e/auto-mode-reliability.test.ts +++ b/tests/e2e/auto-mode-reliability.test.ts @@ -1221,6 +1221,49 @@ describe("lean auto mode reliability", () => { TIMEOUT, ); + test( + "git fetch from a repository that names a remote helper remains reviewer owned", + async () => { + const root = createIsolatedRoot(); + runGit(root.workspace, ["init", "-q"]); + const marker = join(root.root, "remote-helper-must-not-run"); + const bin = installRecorder(root, "git-remote-probe", marker); + const gateway = startGateway( + [ + // git runs `git-remote-probe` from `PATH` for each of these. + cleanCommandCall("git fetch probe::x", "helper_prefix"), + cleanCommandCall("git fetch -- probe::x", "helper_after_separator"), + cleanCommandCall("git fetch probe://host/repository", "helper_scheme"), + (body) => { + expect(body).toContain("review_caution"); + return fakeGatewayFinalText("remote helpers stayed reviewed"); + }, + ], + [ + fakeGatewayPermissionDecision("caution", "helper_prefix_review"), + fakeGatewayPermissionDecision("caution", "helper_after_separator_review"), + fakeGatewayPermissionDecision("caution", "helper_scheme_review"), + ], + ); + const result = await runFx( + ["ask", "--quiet", "--json", "--no-save", "Fetch the latest changes."], + { + cwd: root.workspace, + env: { + ...gatewayEnv(root, gateway), + PATH: `${bin}:${process.env.PATH ?? "/usr/bin:/bin"}`, + }, + timeoutMs: TIMEOUT, + }, + ); + + expect(result.code, `stdout: ${result.stdout}\nstderr: ${result.stderr}`).toBe(0); + expect(gateway.classifierRequests).toHaveLength(3); + expect(existsSync(marker)).toBe(false); + }, + TIMEOUT, + ); + test( "existing startup targets remain reviewer owned", async () => {