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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ Add a step like this to your workflow:
Multiple options let you provide the `git` arguments that you want the action to use. It's important to note that these arguments **are not actually used with a CLI command**, but they are parsed by a package called [`string-argv`](https://npm.im/string-argv), and then used with [`simple-git`](https://npm.im/simple-git).
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.
Remote-helper overrides (`--upload-pack`, `--receive-pack`, `--exec`, and abbreviations of those) are rejected on every token, including values after `-u` / `-m`: they can make git run an arbitrary Git transport program during fetch/pull/push.
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.
Unmatched `'` / `"` quotes are also rejected: `string-argv` can otherwise split on an odd quote and turn part of a value into extra flags (for example a branch name like `fix'--force` becoming `fix` plus `--force`).
Do not interpolate untrusted data (for example values from `github.event.*`, `github.head_ref`, or repository content that contributors can edit) into `fetch`, `pull`, `push`, `tag`, `tag_push`, or `commit` without sanitizing them first. When the branch name is dynamic, prefer the default `push: true` with [`new_branch`](#creating-a-new-branch) instead of embedding the ref in a custom `push` string.
Expand Down
2 changes: 1 addition & 1 deletion lib/index.js

Large diffs are not rendered by default.

25 changes: 17 additions & 8 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,14 @@ const LONG_OPTIONS_WITH_SEPARATE_ARG: ReadonlyArray<{
{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']);
/**
* Short options that take a value (glued or as the following argv token).
* `-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`.
*/
const SHORT_OPTIONS_WITH_ARG = new Set(['m', 'F']);

function getLongOptionName(arg: string): string | undefined {
if (!arg.startsWith('--') || arg === '--') return undefined;
Expand Down Expand Up @@ -348,7 +354,7 @@ function assertBalancedQuotes(input: string): void {
* ```
* @returns An array, if there's no match it'll be empty
* @throws If the args include unmatched quotes
* @throws If the args include a blocked remote-helper override (`--upload-pack`, `--receive-pack`, `--exec`, or abbreviations)
* @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`)
*/
export function matchGitArgs(string: string) {
Expand All @@ -361,16 +367,19 @@ export function matchGitArgs(string: string) {

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

// 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 '${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 '${arg}' is not allowed: reading a tag/commit message from a file (-F/--file) can exfiltrate runner filesystem contents into git history.`,
Expand Down
17 changes: 17 additions & 0 deletions test/integration/action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,23 @@ describe('action integration', () => {
expect(`${result.stdout}\n${result.stderr}`).toMatch(/gitlink/i);
});

it('rejects remote-helper overrides after -u in fetch args', () => {
const f = fixture!;
writeFile(f.local, 'fetch-args.txt', 'changed\n');
const before = gitRevParse(f.local, 'HEAD');

const result = runAction(f, {
message: 'Should not fetch with blocked args',
fetch: '-u --upl=evil',
push: 'false',
});

expect(result.status).not.toBe(0);
expect(result.outputs.committed).toBe('false');
expect(gitRevParse(f.local, 'HEAD')).toBe(before);
expect(`${result.stdout}\n${result.stderr}`).toMatch(/not allowed/);
});

it('applies custom author and committer', () => {
const f = fixture!;
writeFile(f.local, 'id.txt', 'id\n');
Expand Down
15 changes: 15 additions & 0 deletions test/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ describe('matchGitArgs', () => {
'release',
]);
expect(matchGitArgs('v1.0.0 -f')).toStrictEqual(['v1.0.0', '-f']);
expect(matchGitArgs('v1.0.0 -u ABCDEF')).toStrictEqual([
'v1.0.0',
'-u',
'ABCDEF',
]);
});

it('returns an empty array for blank input', () => {
Expand Down Expand Up @@ -182,6 +187,16 @@ describe('matchGitArgs', () => {
expect(() => matchGitArgs('--exe=evil')).toThrow(/not allowed/);
});

it('rejects remote-helper overrides after -u (fetch/push flag, not a value option)', () => {
expect(() => matchGitArgs('-u --upl=evil')).toThrow(/not allowed/);
expect(() => matchGitArgs('-u --upload-pack=evil')).toThrow(/not allowed/);
});

it('rejects remote-helper overrides after -m / --message', () => {
expect(() => matchGitArgs('-m --upl=evil')).toThrow(/not allowed/);
expect(() => matchGitArgs('--message --exec=evil')).toThrow(/not allowed/);
});

it('rejects unmatched quotes that would inject flags via string-argv', () => {
expect(() => matchGitArgs("origin fix'--force --set-upstream")).toThrow(
/unmatched ' quote/,
Expand Down