fix: validate denylisted git args on every token (#784)

Short-option clusters such as -Sm were treated as consuming the next
argument, which let --pathspec-from-file reach git and leak file
contents into workflow logs.

Co-authored-by: Cursor <cursoragent@cursor.com>

src/util.ts

@@ -256,30 +256,12 @@ const DANGEROUS_PATHSPEC_FILE_OPTIONS: ReadonlyArray<{
   {canonical: 'pathspec-file-nul', minPrefix: 'pathspec-fi'},
 ];
 
-/**
- * Long options whose next argv token is a value, not another option.
- * Used so literals like `-m '-F'` are not treated as a message-file flag.
- */
-const LONG_OPTIONS_WITH_SEPARATE_ARG: ReadonlyArray<{
-  canonical: string;
-  minPrefix: string;
-}> = [
-  {canonical: 'message', minPrefix: 'mes'},
-  {canonical: 'local-user', minPrefix: 'local-'},
-  {canonical: 'cleanup', minPrefix: 'cleanup'},
-  {canonical: 'file', minPrefix: 'fi'},
-  {canonical: 'pathspec-from-file', minPrefix: 'pathspec-fr'},
-  {canonical: 'upload-pack', minPrefix: 'upl'},
-  {canonical: 'receive-pack', minPrefix: 'rece'},
-  {canonical: 'exec', minPrefix: 'e'},
-];
-
 /**
  * Short options that take a value (glued or as the following argv token).
+ * Used so glued values after `-m` (e.g. `-m-F`) are not treated as `-F`.
  * `-u` is intentionally omitted: it only takes a key-id for `git tag`, while
  * `git fetch` (`--update-head-ok`) and `git push` (`--set-upstream`) treat it
- * as a flag. Tag signing still uses `--local-user` in
- * `LONG_OPTIONS_WITH_SEPARATE_ARG`.
+ * as a flag.
  */
 const SHORT_OPTIONS_WITH_ARG = new Set(['m', 'F']);
 
@@ -290,11 +272,6 @@ function getLongOptionName(arg: string): string | undefined {
   return (eq === -1 ? body : body.slice(0, eq)).toLowerCase();
 }
 
-function longOptionHasInlineValue(arg: string): boolean {
-  if (!arg.startsWith('--') || arg === '--') return false;
-  return arg.slice(2).includes('=');
-}
-
 function matchesLongOptionPrefix(
   arg: string,
   options: ReadonlyArray<{canonical: string; minPrefix: string}>,
@@ -342,28 +319,6 @@ function isDangerousPathspecFileOption(arg: string): boolean {
   return matchesLongOptionPrefix(arg, DANGEROUS_PATHSPEC_FILE_OPTIONS);
 }
 
-/**
- * Whether this token causes Git to treat the next argv element as a value
- * (so that value must not be classified as an option).
- */
-function consumesFollowingArgument(arg: string): boolean {
-  if (arg.startsWith('--') && arg !== '--') {
-    if (longOptionHasInlineValue(arg)) return false;
-    return matchesLongOptionPrefix(arg, LONG_OPTIONS_WITH_SEPARATE_ARG);
-  }
-  if (!arg.startsWith('-') || arg.startsWith('--')) return false;
-
-  const body = arg.slice(1);
-  for (let i = 0; i < body.length; i++) {
-    const ch = body[i];
-    if (SHORT_OPTIONS_WITH_ARG.has(ch)) {
-      // Glued value after the option letter → no separate following argv.
-      return i === body.length - 1;
-    }
-  }
-  return false;
-}
-
 /**
  * Conservative argument-boundary check for quotes before `string-argv` runs.
  * Not every rejected form would become extra argv words.
@@ -446,9 +401,9 @@ export type MatchGitArgsOptions = {
  * @returns An array, if there's no match it'll be empty
  * @throws If the args include unmatched quotes, or a closing quote glued to following text
  * @throws If the args include a blocked remote-helper override (`--upload-pack`, `--receive-pack`, `--exec`, or abbreviations) on any token, including values after `-u` / `-m`
- * @throws If the args include a blocked message-from-file flag (`-F`, `--file`, abbreviations, or short-option clusters containing `F`)
- * @throws If the args include a blocked pathspec-from-file flag (`--pathspec-from-file`, `--pathspec-file-nul`, or abbreviations)
- * @throws If the args include a `scheme::` remote-helper URL (unless `allowUnsafeGitProtocols`)
+ * @throws If the args include a blocked message-from-file flag (`-F`, `--file`, abbreviations, or short-option clusters containing `F`) on any token, including values after `-m` / `--message`
+ * @throws If the args include a blocked pathspec-from-file flag (`--pathspec-from-file`, `--pathspec-file-nul`, or abbreviations) on any token, including values after `-m` / `--message`
+ * @throws If the args include a `scheme::` remote-helper URL on any token (unless `allowUnsafeGitProtocols`)
  */
 export function matchGitArgs(
   string: string,
@@ -463,21 +418,15 @@ export function matchGitArgs(
 
   const allowUnsafe = options.allowUnsafeGitProtocols === true;
 
-  let skipNext = false;
+  // All denylists run on every token. Skipping the next argv after a guessed
+  // value-taking option (e.g. treating `-Sm` as `-m`) disagrees with Git's
+  // cluster parser and smuggles `--pathspec-from-file` / `scheme::` through.
   for (const arg of parsed) {
-    // Remote-helper overrides are rejected on every token, including values
-    // after `-m` / `--message`. `-u` must not skip `--upl=` / `--upload-pack`.
     if (isDangerousRemoteHelperOption(arg)) {
       throw new Error(
         `Git argument '${neutralizeLogString(arg)}' is not allowed: overriding the remote helper (--upload-pack, --receive-pack, --exec) can execute arbitrary commands on the runner.`,
       );
     }
-
-    if (skipNext) {
-      skipNext = false;
-      continue;
-    }
-
     if (isDangerousMessageFileOption(arg)) {
       throw new Error(
         `Git argument '${neutralizeLogString(arg)}' is not allowed: reading a tag/commit message from a file (-F/--file) can exfiltrate runner filesystem contents into git history.`,
@@ -493,8 +442,6 @@ export function matchGitArgs(
         `Git argument '${neutralizeLogString(arg)}' is not allowed: remote-helper URLs (scheme::…) can execute arbitrary commands on the runner. Set allow_unsafe_git_protocols to true only if you fully trust this input.`,
       );
     }
-
-    skipNext = consumesFollowingArgument(arg);
   }
 
   return parsed;

test/integration/action.test.ts

@@ -559,6 +559,21 @@ describe('action integration', () => {
       expect(gitRevParse(f.local, 'HEAD')).toBe(before);
     });
 
+    it('rejects commit when a short-option cluster precedes the same options', () => {
+      const f = fixture!;
+      const dummyPath = writeDummyOutsideClone(f);
+      writeFile(f.local, 'commit-cluster-args.txt', 'changed\n');
+      const before = gitRevParse(f.local, 'HEAD');
+
+      const result = runAction(f, {
+        commit: `-Sm --pathspec-from-file=${dummyPath} -Sm --pathspec-file-nul`,
+        push: 'false',
+      });
+
+      expectBlockedWithoutDisclosure(result);
+      expect(gitRevParse(f.local, 'HEAD')).toBe(before);
+    });
+
     it('rejects a YAML array element with the same options', () => {
       const f = fixture!;
       const dummyPath = writeDummyOutsideClone(f);

test/util.test.ts

@@ -332,20 +332,19 @@ describe('matchGitArgs', () => {
     expect(() => matchGitArgs('-F../secrets')).toThrow(/message from a file/);
   });
 
-  it('preserves -m / --message values that look like -F/--file', () => {
-    expect(matchGitArgs('-m "-F"')).toStrictEqual(['-m', '-F']);
-    expect(matchGitArgs('-m --file=/tmp/value')).toStrictEqual([
-      '-m',
-      '--file=/tmp/value',
-    ]);
-    expect(matchGitArgs('--message "-F"')).toStrictEqual(['--message', '-F']);
+  it('rejects -F / --file even when they follow -m / --message', () => {
+    expect(() => matchGitArgs('-m "-F"')).toThrow(/message from a file/);
+    expect(() => matchGitArgs('-m --file=/tmp/value')).toThrow(
+      /message from a file/,
+    );
+    expect(() => matchGitArgs('--message "-F"')).toThrow(/message from a file/);
+    expect(() => matchGitArgs('v1.0.0 -a -m "-F"')).toThrow(
+      /message from a file/,
+    );
+  });
+
+  it('treats -m-F as a glued message value, not a file flag', () => {
     expect(matchGitArgs('-m-F')).toStrictEqual(['-m-F']);
-    expect(matchGitArgs('v1.0.0 -a -m "-F"')).toStrictEqual([
-      'v1.0.0',
-      '-a',
-      '-m',
-      '-F',
-    ]);
   });
 
   it('still rejects a real -F after a message value', () => {
@@ -387,15 +386,13 @@ describe('matchGitArgs', () => {
     );
   });
 
-  it('preserves -m / --message values that look like --pathspec-from-file', () => {
-    expect(matchGitArgs('-m "--pathspec-from-file=/x"')).toStrictEqual([
-      '-m',
-      '--pathspec-from-file=/x',
-    ]);
-    expect(matchGitArgs('--message --pathspec-from-file=/x')).toStrictEqual([
-      '--message',
-      '--pathspec-from-file=/x',
-    ]);
+  it('rejects --pathspec-from-file even when it follows -m / --message', () => {
+    expect(() => matchGitArgs('-m "--pathspec-from-file=/x"')).toThrow(
+      /pathspecs from a file/,
+    );
+    expect(() => matchGitArgs('--message --pathspec-from-file=/x')).toThrow(
+      /pathspecs from a file/,
+    );
   });
 
   it('still rejects a real --pathspec-from-file after a message value', () => {
@@ -404,6 +401,15 @@ describe('matchGitArgs', () => {
     );
   });
 
+  it('rejects --pathspec-from-file after a short-option cluster whose last letter is m', () => {
+    expect(() =>
+      matchGitArgs('-Sm --pathspec-from-file=/x -Sm --pathspec-file-nul'),
+    ).toThrow(/pathspecs from a file/);
+    expect(() => matchGitArgs('-tm --pathspec-from-file=/x')).toThrow(
+      /pathspecs from a file/,
+    );
+  });
+
   it('still rejects --pathspec-from-file when allowUnsafeGitProtocols is true', () => {
     expect(() =>
       matchGitArgs('--pathspec-from-file=/path', {
@@ -449,12 +455,30 @@ describe('matchGitArgs', () => {
     ).toThrow(/not allowed/);
   });
 
-  it('allows :: inside option values via skipNext', () => {
-    expect(matchGitArgs('-m "foo::bar"')).toStrictEqual(['-m', 'foo::bar']);
-    expect(matchGitArgs('--message foo::bar')).toStrictEqual([
-      '--message',
-      'foo::bar',
-    ]);
+  it('rejects scheme:: even when it follows -m / --message', () => {
+    expect(() => matchGitArgs('-m "foo::bar"')).toThrow(/remote-helper URLs/);
+    expect(() => matchGitArgs('--message foo::bar')).toThrow(
+      /remote-helper URLs/,
+    );
+  });
+
+  it('rejects scheme:: after a short-option cluster that used to skip the next token', () => {
+    expect(() => matchGitArgs('-Sm ext::sh')).toThrow(/remote-helper URLs/);
+    expect(() => matchGitArgs('-om ext::sh origin')).toThrow(
+      /remote-helper URLs/,
+    );
+  });
+
+  it('allows scheme:: after -m when allowUnsafeGitProtocols is true', () => {
+    expect(
+      matchGitArgs('-m "foo::bar"', {allowUnsafeGitProtocols: true}),
+    ).toStrictEqual(['-m', 'foo::bar']);
+    expect(
+      matchGitArgs('--message foo::bar', {allowUnsafeGitProtocols: true}),
+    ).toStrictEqual(['--message', 'foo::bar']);
+    expect(
+      matchGitArgs('-Sm ext::sh', {allowUnsafeGitProtocols: true}),
+    ).toStrictEqual(['-Sm', 'ext::sh']);
   });
 });