Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ Multiple options let you provide the `git` arguments that you want the action to
What does this mean for you? It means that strings that contain a lot of nested quotes may be parsed incorrectly, and that specific ways of declaring arguments may not be supported by these libraries. If you're having issues with your argument strings you can check whether they're being parsed correctly either by [enabling debug logging](https://docs.github.com/en/actions/managing-workflow-runs/enabling-debug-logging) for your workflow runs or by testing it directly with `string-argv` ([RunKit demo](https://npm.runkit.com/string-argv)): if each argument and option is parsed correctly you'll see an array where every string is an option or value.

Remote-helper overrides (`--upload-pack`, `--receive-pack`, `--exec`, and abbreviations of those) are rejected: they can make git run an arbitrary Git transport program during fetch/pull/push.
Do not interpolate untrusted data (for example values from `github.event.*`) into `fetch`, `pull`, `push`, or `tag_push` without sanitizing them first.
Message-from-file flags (`-F`, `--file`, abbreviations such as `--fi`, and short-option clusters that include `F` such as `-aF`) are rejected: they can embed arbitrary runner filesystem contents into a tag or commit message and, with a push, into the repository history.
Do not interpolate untrusted data (for example values from `github.event.*` or repository content that contributors can edit) into `fetch`, `pull`, `push`, `tag_push`, `tag`, or `commit` without sanitizing them first.

### Adding files

Expand Down
4 changes: 2 additions & 2 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ inputs:
description: The email of the user that will be displayed as the author of the commit
required: false
commit:
description: Additional arguments for the git commit command
description: Additional arguments for the git commit command. -F/--file, abbreviations (e.g. --fi), and short-option clusters containing F (e.g. -aF) are not allowed.
required: false
committer_name:
description: The name of the custom committer you want to use
Expand Down Expand Up @@ -54,7 +54,7 @@ inputs:
description: Arguments for the git rm command
required: false
tag:
description: Arguments for the git tag command (the tag name always needs to be the first word not preceded by a hyphen)
description: Arguments for the git tag command (the tag name always needs to be the first word not preceded by a hyphen). -F/--file, abbreviations (e.g. --fi), and short-option clusters containing F (e.g. -aF) are not allowed.
required: false
tag_push:
description: Arguments for the git push --tags command (any additional argument will be added after --tags)
Expand Down
6 changes: 3 additions & 3 deletions lib/index.js

Large diffs are not rendered by default.

117 changes: 111 additions & 6 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,22 +119,113 @@ const DANGEROUS_REMOTE_HELPER_OPTIONS: ReadonlyArray<{
{canonical: 'exec', minPrefix: 'e'},
];

/**
* Long options that read a commit/tag message from a filesystem path.
* Git accepts unique abbreviations (`--fi` → `--file`); `minPrefix` is the
* shortest unambiguous abbreviation currently accepted for `git tag`.
*/
const DANGEROUS_MESSAGE_FILE_OPTIONS: ReadonlyArray<{
canonical: string;
minPrefix: string;
}> = [{canonical: 'file', minPrefix: '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: '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). */
const SHORT_OPTIONS_WITH_ARG = new Set(['m', 'u', 'F']);

function getLongOptionName(arg: string): string | undefined {
if (!arg.startsWith('--') || arg === '--') return undefined;
const body = arg.slice(2);
const eq = body.indexOf('=');
return (eq === -1 ? body : body.slice(0, eq)).toLowerCase();
}

function isDangerousRemoteHelperOption(arg: string): boolean {
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}>,
): boolean {
const name = getLongOptionName(arg);
if (!name) return false;
return DANGEROUS_REMOTE_HELPER_OPTIONS.some(
return options.some(
({canonical, minPrefix}) =>
name.length >= minPrefix.length && canonical.startsWith(name),
);
}

function isDangerousRemoteHelperOption(arg: string): boolean {
return matchesLongOptionPrefix(arg, DANGEROUS_REMOTE_HELPER_OPTIONS);
}

/**
* True for `-F`, glued forms like `-F/path`, and short-option clusters that
* include `F` as an option letter (e.g. `-aF`). Values glued after an
* argument-taking option (e.g. `-m-F`) are not treated as flags. Lowercase
* `-f` (force) is intentionally allowed.
*/
function isDangerousMessageFileShortOption(arg: string): boolean {
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 (ch === 'F') return true;
if (SHORT_OPTIONS_WITH_ARG.has(ch)) {
// Remainder is the option's value, not further option letters.
return false;
}
}
return false;
}

function isDangerousMessageFileOption(arg: string): boolean {
return (
matchesLongOptionPrefix(arg, DANGEROUS_MESSAGE_FILE_OPTIONS) ||
isDangerousMessageFileShortOption(arg)
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

/**
* 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;
}

/**
* Matches the given string to an array of arguments.
* The parsing is made by `string-argv`: if your way of using argument is not supported, the issue is theirs!
Expand All @@ -145,34 +236,48 @@ function isDangerousRemoteHelperOption(arg: string): boolean {
-s
--longOption 'This uses the "other" quotes'
--foo 1234
--file=message.txt
--file2="Application 'Support'/\"message\".txt"
--force
--path="Application 'Support'/\"message\".txt"
`) => [
'-s',
'--longOption',
'This uses the "other" quotes',
'--foo',
'1234',
'--file=message.txt',
`--file2="Application 'Support'/\\"message\\".txt"`
'--force',
`--path="Application 'Support'/\\"message\\".txt"`
]
* matchGitArgs(' ') => [ ]
* ```
* @returns An array, if there's no match it'll be empty
* @throws If the args include a blocked remote-helper override (`--upload-pack`, `--receive-pack`, `--exec`, or abbreviations)
* @throws If the args include a blocked message-from-file flag (`-F`, `--file`, abbreviations, or short-option clusters containing `F`)
*/
export function matchGitArgs(string: string) {
const parsed = parseArgsStringToArgv(string);
core.debug(`Git args parsed:
- Original: ${string}
- Parsed: ${JSON.stringify(parsed)}`);

let skipNext = false;
for (const arg of parsed) {
if (skipNext) {
skipNext = false;
continue;
}

if (isDangerousRemoteHelperOption(arg)) {
throw new Error(
`Git argument '${arg}' is not allowed: overriding the remote helper (--upload-pack, --receive-pack, --exec) can execute arbitrary commands on the runner.`,
);
}
if (isDangerousMessageFileOption(arg)) {
throw new Error(
`Git argument '${arg}' is not allowed: reading a tag/commit message from a file (-F/--file) can exfiltrate runner filesystem contents into git history.`,
);
}

skipNext = consumesFollowingArgument(arg);
}

return parsed;
Expand Down
56 changes: 55 additions & 1 deletion test/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@ describe('matchGitArgs', () => {
'--force',
]);
expect(matchGitArgs('--set-upstream')).toStrictEqual(['--set-upstream']);
expect(matchGitArgs('v1.0.0 --force')).toStrictEqual(['v1.0.0', '--force']);
expect(matchGitArgs('-a -m "release"')).toStrictEqual([
'-a',
'-m',
'release',
]);
expect(matchGitArgs('v1.0.0 -f')).toStrictEqual(['v1.0.0', '-f']);
});

it('returns an empty array for blank input', () => {
Expand All @@ -159,7 +166,7 @@ describe('matchGitArgs', () => {
expect(() => matchGitArgs('--exec=/bin/sh')).toThrow(/not allowed/);
});

it('rejects abbreviations of blocked options', () => {
it('rejects abbreviations of blocked remote-helper options', () => {
expect(() => matchGitArgs('--upl=evil')).toThrow(/not allowed/);
expect(() => matchGitArgs('--uplo=evil')).toThrow(/not allowed/);
expect(() => matchGitArgs('--upload-pac=evil')).toThrow(/not allowed/);
Expand All @@ -170,6 +177,53 @@ describe('matchGitArgs', () => {
expect(() => matchGitArgs('--e=evil')).toThrow(/not allowed/);
expect(() => matchGitArgs('--exe=evil')).toThrow(/not allowed/);
});

it('rejects -F / --file message-from-file flags (PoC form)', () => {
expect(() =>
matchGitArgs('1.0.0 -F ../runner-secrets/aws-credentials.txt'),
).toThrow(/not allowed/);
expect(() => matchGitArgs('-F ../secrets')).toThrow(/message from a file/);
expect(() => matchGitArgs('--file=../secrets')).toThrow(
/message from a file/,
);
expect(() => matchGitArgs('--file ../secrets')).toThrow(
/message from a file/,
);
});

it('rejects --file abbreviations and short-option clusters containing F', () => {
expect(() => matchGitArgs('--fi=../secrets')).toThrow(
/message from a file/,
);
expect(() => matchGitArgs('--fil=../secrets')).toThrow(
/message from a file/,
);
expect(() => matchGitArgs('-aF ../secrets')).toThrow(/message from a file/);
expect(() => matchGitArgs('-Fa')).toThrow(/message from a file/);
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']);
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', () => {
expect(() => matchGitArgs('-m "ok" -F ../secrets')).toThrow(
/message from a file/,
);
});
});

describe('pickGitIdentityConfig', () => {
Expand Down