Skip to content

fix: validate denylisted git args on every token #784

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.

Sign up for GitHub

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

Merged
EndBug merged 2 commits into from
Aug 19, 2026
+76 −90
Merged

fix: validate denylisted git args on every token #784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae97483
fix: validate denylisted git args on every token
EndBug Aug 18, 2026
d1b4ac7
Merge branch 'main' into cursor/fix-skipnext-denylist-bypass
EndBug Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/index.js
View file
Open in desktop

Large diffs are not rendered by default.

69 changes: 8 additions & 61 deletions src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']);

Expand All @@ -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}>,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand All @@ -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.`,
Expand All @@ -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;
Expand Down
15 changes: 15 additions & 0 deletions test/integration/action.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
80 changes: 52 additions & 28 deletions test/util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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', {
Expand Down Expand Up @@ -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']);
});
});

Expand Down