Merge pull request #1419 from vercel-labs/fazxes/review-git-remote-helpers

Review git fetch from repositories that name a remote helper

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
+/// `<scheme>://` URL to a `git-remote-<scheme>` 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`: `<helper>::<address>`, 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-<helper>` 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",

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 () => {