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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ Git child processes are also limited to the `https`, `http`, `ssh`, `file`, and
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.
Pathspec-from-file flags (`--pathspec-from-file`, `--pathspec-file-nul`, and abbreviations such as `--pathspec-fr` / `--pathspec-fi`) are rejected on `add`, `remove`, and `commit`: they can read an arbitrary runner file and leak its contents into the action log.
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`).
A quoted segment is accepted only when its closing quote is followed by whitespace or the end of the input. That is a conservative argument-boundary check, not a claim that every rejected form would become extra argv words: `'main'--force` is rejected (and would split into `main` plus `--force`), and so is `a'b'c` (which `string-argv` would keep as one token). `--message='hello'` is allowed because the closer is at the end of the word. Put a space after a wrapping closer (`origin 'main' --force`) or omit the quotes.
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.

### Allow unsafe git protocols
Expand Down
2 changes: 1 addition & 1 deletion lib/index.js

Large diffs are not rendered by default.

30 changes: 23 additions & 7 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,18 +365,34 @@ function consumesFollowingArgument(arg: string): boolean {
}

/**
* Rejects unmatched `'` / `"` so `string-argv` cannot silently retokenize at an
* odd quote (e.g. `origin fix'--force` → `["origin","fix","--force"]`).
* Balanced quotes and the opposite quote type inside a quoted segment are allowed.
* Conservative argument-boundary check for quotes before `string-argv` runs.
* Not every rejected form would become extra argv words.
*
* Unmatched `'` / `"` are rejected (e.g. `origin fix'--force`).
* A quoted segment is accepted only when its closing quote is followed by
* whitespace or end of input (`origin 'main' --force`, `--message='hello'`).
* Interior glued quotes such as `a'b'c` are rejected even though `string-argv`
* would keep that as one token. A start-quoted token with text after the closer
* (`'main'--force`) is also rejected; that form would split into extra argv
* words.
*
* The opposite quote type inside a quoted segment is allowed.
*/
function assertBalancedQuotes(input: string): void {
function assertSafeQuotes(input: string): void {
let open: "'" | '"' | null = null;
for (const char of input) {
for (let i = 0; i < input.length; i++) {
const char = input[i];
if (char !== "'" && char !== '"') continue;
if (open === null) {
open = char;
} else if (open === char) {
open = null;
const next = input[i + 1];
if (next !== undefined && !/\s/.test(next)) {
throw new Error(
'Git arguments contain a quoted segment immediately followed by non-whitespace. string-argv would split that into extra arguments (for example a quoted name glued to --force).',
);
}
}
}
if (open !== null) {
Expand Down Expand Up @@ -428,7 +444,7 @@ export type MatchGitArgsOptions = {
* matchGitArgs(' ') => [ ]
* ```
* @returns An array, if there's no match it'll be empty
* @throws If the args include unmatched quotes
* @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)
Expand All @@ -438,7 +454,7 @@ export function matchGitArgs(
string: string,
options: MatchGitArgsOptions = {},
) {
assertBalancedQuotes(string);
assertSafeQuotes(string);

const parsed = parseArgsStringToArgv(string);
core.debug(`Git args parsed:
Expand Down
19 changes: 19 additions & 0 deletions test/integration/action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,25 @@ describe('action integration', () => {
expect(`${result.stdout}\n${result.stderr}`).toMatch(/not allowed/);
});

it('rejects glued quotes that would inject --force into push', () => {
const f = fixture!;
writeFile(f.local, 'glued-quotes.txt', 'changed\n');
const beforeRemote = gitRevParse(f.remote, 'HEAD');

const result = runAction(f, {
message: 'Should not push with glued quotes',
fetch: 'false',
push: "origin 'main'--force --set-upstream",
});

expect(result.status).not.toBe(0);
expect(result.outputs.pushed).toBe('false');
expect(gitRevParse(f.remote, 'HEAD')).toBe(beforeRemote);
expect(`${result.stdout}\n${result.stderr}`).toMatch(
/quoted segment immediately followed by non-whitespace/,
);
});

it('applies custom author and committer', () => {
const f = fixture!;
writeFile(f.local, 'id.txt', 'id\n');
Expand Down
32 changes: 29 additions & 3 deletions test/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,10 +266,33 @@ describe('matchGitArgs', () => {
);
});

it('parses balanced quotes without treating them as injection', () => {
expect(matchGitArgs("origin a'b'c --set-upstream")).toStrictEqual([
it('rejects a closing quote glued to following text (string-argv would split it)', () => {
expect(() => matchGitArgs("origin 'main'--force --set-upstream")).toThrow(
/quoted segment immediately followed by non-whitespace/,
);
expect(() => matchGitArgs('origin "main"--force --set-upstream')).toThrow(
/quoted segment immediately followed by non-whitespace/,
);
expect(() => matchGitArgs("origin ''--force")).toThrow(
/quoted segment immediately followed by non-whitespace/,
);
expect(() => matchGitArgs('origin \'foo\'"--force"')).toThrow(
/quoted segment immediately followed by non-whitespace/,
);
expect(() => matchGitArgs("origin a'b'c --set-upstream")).toThrow(
/quoted segment immediately followed by non-whitespace/,
);
});

it('parses balanced quotes that wrap a whole argument', () => {
expect(matchGitArgs("origin 'main' --set-upstream")).toStrictEqual([
'origin',
'main',
'--set-upstream',
]);
expect(matchGitArgs("origin 'a b c' --set-upstream")).toStrictEqual([
'origin',
"a'b'c",
'a b c',
'--set-upstream',
]);
expect(matchGitArgs("--longOption 'hello world'")).toStrictEqual([
Expand All @@ -279,6 +302,9 @@ describe('matchGitArgs', () => {
expect(
matchGitArgs('--longOption \'This uses the "other" quotes\''),
).toStrictEqual(['--longOption', 'This uses the "other" quotes']);
expect(matchGitArgs("--message='hello'")).toStrictEqual([
"--message='hello'",
]);
});

it('rejects -F / --file message-from-file flags (PoC form)', () => {
Expand Down