Commit24094fc
fix(security): refuse option-like arguments in git subprocess calls
fix(security): refuse option-like arguments in git subprocess calls Commands in src/git/repository.ts are arrays run through Bun.spawn, so no shell is involved and there is no shell injection. But git parses any leading-dash argument as an option wherever it appears, and several commands pass a caller-supplied ref / sha / treeish positionally with no `--` separator — e.g. `git log --format=… --skip=0 -30 <ref>`. A ref of `--output=/app/src/index.ts` therefore makes git write its output to that path: arbitrary file write as the app user, reachable unauthenticated through ordinary public-repo URLs such as /:owner/:repo/commits/:ref. Guard centrally in exec() rather than adding `--` at ~15 call sites, so a git call added later cannot reintroduce the hole by forgetting the separator. The two streaming paths that use Bun.spawn directly and bypass exec() get the same check inline. The hazard of a central allowlist is breaking legitimate calls, so a test walks every dash-argument literal in the module and asserts each one is allowlisted — that caught --sort, --numstat, --add and --cacheinfo missing from the first draft. Numeric counts (-30, -500) and long options with values (--format=%H, --skip=20) are matched on their head. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed+153−024094fc20cc2eca56fb0d5e8eae03106be04552e
2 changed files+153−0
Addedsrc/__tests__/git-arg-injection.test.ts+101−0View fileUnifiedSplit
@@ -0,0 +1,101 @@
1/**
2 * git argument injection.
3 *
4 * Commands in src/git/repository.ts are arrays run through Bun.spawn, so there
5 * is no shell involved and no shell injection. But git parses any leading-dash
6 * argument as an option wherever it appears, and several commands take a
7 * caller-supplied ref / sha / treeish positionally with no `--` separator.
8 *
9 * A ref of `--output=/app/src/index.ts` reaching `git log` makes git write its
10 * output to that path — unauthenticated arbitrary file write as the app user,
11 * reachable through ordinary public-repo URLs like /:owner/:repo/commits/:ref.
12 *
13 * exec() now refuses option-like arguments centrally. These lock that in and,
14 * just as importantly, prove the guard does not reject the flags the module
15 * legitimately passes.
16 */
17
18import { describe, expect, it } from "bun:test";
19import { readFileSync } from "fs";
20
21const SRC = readFileSync("src/git/repository.ts", "utf8");
22
23// Mirror of the module-private predicate, kept in step by the coverage test
24// at the bottom which asserts against the real source.
25function isAllowedGitFlag(arg: string): boolean {
26 const ALLOWED = new Set([
27 "--bare", "--format", "--skip", "--verify", "--porcelain", "--max-count",
28 "--full-tree", "--quiet", "--stdin", "--short", "--end-of-options", "--",
29 "--sort", "--numstat", "--add", "--cacheinfo",
30 "-",
31 "-a", "-d", "-l", "-r", "-t", "-n", "-I", "-m", "-p", "-s", "-e", "-w", "-1",
32 ]);
33 if (!arg.startsWith("-")) return true;
34 if (/^-\d+$/.test(arg)) return true;
35 return ALLOWED.has(arg.split("=", 1)[0]);
36}
37
38describe("option-like arguments are refused", () => {
39 const attacks = [
40 "--output=/app/src/index.ts", // git log: arbitrary file write
41 "--output=/tmp/pwned",
42 "--upload-pack=/bin/sh",
43 "--exec=/bin/sh",
44 "-o/tmp/pwned",
45 "--git-dir=/etc",
46 "--work-tree=/",
47 ];
48 for (const a of attacks) {
49 it(`rejects ${a}`, () => {
50 expect(isAllowedGitFlag(a)).toBe(false);
51 });
52 }
53
54 it("rejects a ref embedded in a rev:path argument", () => {
55 // `git show ${ref}:${filePath}` — a dash-leading ref poisons the whole arg.
56 expect(isAllowedGitFlag("--output=/tmp/x:README.md")).toBe(false);
57 });
58});
59
60describe("legitimate arguments still pass", () => {
61 const ok = [
62 "main", "HEAD", "refs/heads/main", "v1.2.3",
63 "abc123def456", "main:src/index.ts", "origin/main..HEAD",
64 "--format=%H%x00%s", "--skip=20", "--max-count=50", "--sort=-creatordate",
65 "--bare", "--porcelain", "--numstat", "--cacheinfo", "--full-tree", "--",
66 "-500", "-30", "-1", "-I", "-w",
67 // A branch legitimately containing a dash, just not leading.
68 "feature/my-branch", "release-2.0",
69 ];
70 for (const a of ok) {
71 it(`accepts ${a}`, () => {
72 expect(isAllowedGitFlag(a)).toBe(true);
73 });
74 }
75});
76
77describe("guard is wired in", () => {
78 it("exec() screens every argument after the binary", () => {
79 expect(SRC).toContain("refusing option-like argument");
80 expect(SRC).toMatch(/for \(let i = 1; i < cmd\.length; i\+\+\)/);
81 });
82
83 it("the streaming Bun.spawn paths are guarded too", () => {
84 // Two call sites stream instead of buffering and therefore skip exec().
85 const guards = SRC.match(/refused option-like git argument/g) ?? [];
86 expect(guards.length).toBe(2);
87 });
88
89 it("every dash-argument the module actually uses is allowlisted", () => {
90 // The real risk of a central guard is breaking legitimate git calls.
91 const found = new Set<string>();
92 for (const m of SRC.matchAll(/"(-[^"]*)"/g)) found.add(m[1]);
93 for (const m of SRC.matchAll(/`(-[^`]*)`/g)) found.add(m[1]);
94 const uncovered = [...found].filter((f) => {
95 if (f.includes("${")) return false; // template — resolved at runtime
96 if (f.startsWith("--output=")) return false; // appears only in a comment
97 return !isAllowedGitFlag(f);
98 });
99 expect(uncovered).toEqual([]);
100 });
101});
Modifiedsrc/git/repository.ts+52−0View fileUnifiedSplit
@@ -39,10 +39,52 @@ function repoPath(owner: string, name: string): string {
3939 return join(config.gitReposPath, owner, `${name}.git`);
4040}
4141
42/**
43 * Every option string this module legitimately passes to git.
44 *
45 * Commands are built as arrays and run via Bun.spawn (no shell), so there is
46 * no *shell* injection here — but git parses any leading-dash argument as an
47 * option regardless of where it sits, and several commands take a caller-
48 * supplied ref/sha/path positionally. A ref of `--output=/app/src/index.ts`
49 * reaching `git log` makes git write its output to that path: unauthenticated
50 * arbitrary file write as the app user, reachable through public-repo URLs
51 * like /:owner/:repo/commits/:ref.
52 *
53 * Guarding centrally in exec() rather than at ~15 call sites means a new git
54 * call added later cannot reintroduce the hole by forgetting a `--`.
55 */
56const GIT_ALLOWED_FLAGS = new Set([
57 "--bare", "--format", "--skip", "--verify", "--porcelain", "--max-count",
58 "--full-tree", "--quiet", "--stdin", "--short", "--end-of-options", "--",
59 "--sort", "--numstat", "--add", "--cacheinfo",
60 "-", // stdin marker
61 "-a", "-d", "-l", "-r", "-t", "-n", "-I", "-m", "-p", "-s", "-e", "-w", "-1",
62]);
63
64function isAllowedGitFlag(arg: string): boolean {
65 if (!arg.startsWith("-")) return true;
66 // `-500`, `-30` etc: a bare numeric count, as git log takes.
67 if (/^-\d+$/.test(arg)) return true;
68 // Long options may carry a value: --format=%H, --skip=20.
69 const head = arg.split("=", 1)[0];
70 return GIT_ALLOWED_FLAGS.has(head);
71}
72
4273async function exec(
4374 cmd: string[],
4475 opts?: { cwd?: string; env?: Record<string, string> }
4576): Promise<{ stdout: string; stderr: string; exitCode: number }> {
77 // Reject dash-leading arguments this module never intends to pass. Anything
78 // caught here is caller-supplied data masquerading as an option.
79 for (let i = 1; i < cmd.length; i++) {
80 if (!isAllowedGitFlag(cmd[i])) {
81 console.error(
82 `[git] refusing option-like argument at position ${i}: ${JSON.stringify(cmd[i])}`
83 );
84 return { stdout: "", stderr: "refused option-like argument", exitCode: 128 };
85 }
86 }
87
4688 let proc: ReturnType<typeof Bun.spawn>;
4789 try {
4890 proc = Bun.spawn(cmd, {
@@ -491,6 +533,12 @@ export async function getRawBlob(
491533 filePath: string
492534): Promise<Uint8Array | null> {
493535 const path = repoPath(owner, name);
536 // Streams the blob rather than buffering it, so it bypasses exec() and needs
537 // the same option-injection guard: a ref beginning with "-" would make the
538 // whole `${ref}:${filePath}` argument parse as a git option.
539 if (!isAllowedGitFlag(`${ref}:${filePath}`)) {
540 throw new Error("refused option-like git argument");
541 }
494542 const proc = Bun.spawn(["git", "show", `${ref}:${filePath}`], {
495543 cwd: path,
496544 stdout: "pipe",
@@ -677,6 +725,10 @@ export async function catBlobBytes(
677725 );
678726 const size = parseInt(sizeOut.trim(), 10) || 0;
679727
728 // Streaming path — same guard as the other direct spawn (see above).
729 if (!isAllowedGitFlag(sha)) {
730 throw new Error("refused option-like git argument");
731 }
680732 const proc = Bun.spawn(["git", "cat-file", "blob", sha], {
681733 cwd: path,
682734 stdout: "pipe",
683735