Visitar URL original
fix: reject glued quotes that string-argv would split into extra git … · EndBug/add-and-commit@66d71de · GitHub
Skip to content

Commit 66d71de

Browse files
EndBugcursoragent
andauthored
fix: reject glued quotes that string-argv would split into extra git flags (#778)
* fix: reject glued quotes that string-argv would split into extra git flags A balanced form like 'main'--force is still two argv tokens under string-argv, so refuse it before git push can see a standalone --force. Co-authored-by: Cursor <cursoragent@cursor.com> * docs: describe glued-quote rejection as an argument-boundary check Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 60ff4bb commit 66d71de

5 files changed

Lines changed: 73 additions & 11 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ Git child processes are also limited to the `https`, `http`, `ssh`, `file`, and
125125
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.
126126
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.
127127
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`).
128+
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.
128129
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.
129130

130131
### Allow unsafe git protocols

‎lib/index.js‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/util.ts‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -365,18 +365,34 @@ function consumesFollowingArgument(arg: string): boolean {
365365
}
366366

367367
/**
368-
* Rejects unmatched `'` / `"` so `string-argv` cannot silently retokenize at an
369-
* odd quote (e.g. `origin fix'--force` → `["origin","fix","--force"]`).
370-
* Balanced quotes and the opposite quote type inside a quoted segment are allowed.
368+
* Conservative argument-boundary check for quotes before `string-argv` runs.
369+
* Not every rejected form would become extra argv words.
370+
*
371+
* Unmatched `'` / `"` are rejected (e.g. `origin fix'--force`).
372+
* A quoted segment is accepted only when its closing quote is followed by
373+
* whitespace or end of input (`origin 'main' --force`, `--message='hello'`).
374+
* Interior glued quotes such as `a'b'c` are rejected even though `string-argv`
375+
* would keep that as one token. A start-quoted token with text after the closer
376+
* (`'main'--force`) is also rejected; that form would split into extra argv
377+
* words.
378+
*
379+
* The opposite quote type inside a quoted segment is allowed.
371380
*/
372-
function assertBalancedQuotes(input: string): void {
381+
function assertSafeQuotes(input: string): void {
373382
let open: "'" | '"' | null = null;
374-
for (const char of input) {
383+
for (let i = 0; i < input.length; i++) {
384+
const char = input[i];
375385
if (char !== "'" && char !== '"') continue;
376386
if (open === null) {
377387
open = char;
378388
} else if (open === char) {
379389
open = null;
390+
const next = input[i + 1];
391+
if (next !== undefined && !/\s/.test(next)) {
392+
throw new Error(
393+
'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).',
394+
);
395+
}
380396
}
381397
}
382398
if (open !== null) {
@@ -428,7 +444,7 @@ export type MatchGitArgsOptions = {
428444
* matchGitArgs(' ') => [ ]
429445
* ```
430446
* @returns An array, if there's no match it'll be empty
431-
* @throws If the args include unmatched quotes
447+
* @throws If the args include unmatched quotes, or a closing quote glued to following text
432448
* @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`
433449
* @throws If the args include a blocked message-from-file flag (`-F`, `--file`, abbreviations, or short-option clusters containing `F`)
434450
* @throws If the args include a blocked pathspec-from-file flag (`--pathspec-from-file`, `--pathspec-file-nul`, or abbreviations)
@@ -438,7 +454,7 @@ export function matchGitArgs(
438454
string: string,
439455
options: MatchGitArgsOptions = {},
440456
) {
441-
assertBalancedQuotes(string);
457+
assertSafeQuotes(string);
442458

443459
const parsed = parseArgsStringToArgv(string);
444460
core.debug(`Git args parsed:

‎test/integration/action.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,25 @@ describe('action integration', () => {
140140
expect(`${result.stdout}\n${result.stderr}`).toMatch(/not allowed/);
141141
});
142142

143+
it('rejects glued quotes that would inject --force into push', () => {
144+
const f = fixture!;
145+
writeFile(f.local, 'glued-quotes.txt', 'changed\n');
146+
const beforeRemote = gitRevParse(f.remote, 'HEAD');
147+
148+
const result = runAction(f, {
149+
message: 'Should not push with glued quotes',
150+
fetch: 'false',
151+
push: "origin 'main'--force --set-upstream",
152+
});
153+
154+
expect(result.status).not.toBe(0);
155+
expect(result.outputs.pushed).toBe('false');
156+
expect(gitRevParse(f.remote, 'HEAD')).toBe(beforeRemote);
157+
expect(`${result.stdout}\n${result.stderr}`).toMatch(
158+
/quoted segment immediately followed by non-whitespace/,
159+
);
160+
});
161+
143162
it('applies custom author and committer', () => {
144163
const f = fixture!;
145164
writeFile(f.local, 'id.txt', 'id\n');

‎test/util.test.ts‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -266,10 +266,33 @@ describe('matchGitArgs', () => {
266266
);
267267
});
268268

269-
it('parses balanced quotes without treating them as injection', () => {
270-
expect(matchGitArgs("origin a'b'c --set-upstream")).toStrictEqual([
269+
it('rejects a closing quote glued to following text (string-argv would split it)', () => {
270+
expect(() => matchGitArgs("origin 'main'--force --set-upstream")).toThrow(
271+
/quoted segment immediately followed by non-whitespace/,
272+
);
273+
expect(() => matchGitArgs('origin "main"--force --set-upstream')).toThrow(
274+
/quoted segment immediately followed by non-whitespace/,
275+
);
276+
expect(() => matchGitArgs("origin ''--force")).toThrow(
277+
/quoted segment immediately followed by non-whitespace/,
278+
);
279+
expect(() => matchGitArgs('origin \'foo\'"--force"')).toThrow(
280+
/quoted segment immediately followed by non-whitespace/,
281+
);
282+
expect(() => matchGitArgs("origin a'b'c --set-upstream")).toThrow(
283+
/quoted segment immediately followed by non-whitespace/,
284+
);
285+
});
286+
287+
it('parses balanced quotes that wrap a whole argument', () => {
288+
expect(matchGitArgs("origin 'main' --set-upstream")).toStrictEqual([
289+
'origin',
290+
'main',
291+
'--set-upstream',
292+
]);
293+
expect(matchGitArgs("origin 'a b c' --set-upstream")).toStrictEqual([
271294
'origin',
272-
"a'b'c",
295+
'a b c',
273296
'--set-upstream',
274297
]);
275298
expect(matchGitArgs("--longOption 'hello world'")).toStrictEqual([
@@ -279,6 +302,9 @@ describe('matchGitArgs', () => {
279302
expect(
280303
matchGitArgs('--longOption \'This uses the "other" quotes\''),
281304
).toStrictEqual(['--longOption', 'This uses the "other" quotes']);
305+
expect(matchGitArgs("--message='hello'")).toStrictEqual([
306+
"--message='hello'",
307+
]);
282308
});
283309

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

0 commit comments

Comments
 (0)