From bf32003fcf6f40fee9ce527cfb8faabf99ee6aea Mon Sep 17 00:00:00 2001 From: Yuval Dinodia <102706514+yetval@users.noreply.github.com> Date: Thu, 9 Jul 2026 07:08:39 -0400 Subject: [PATCH] fix(plugins): terminate git clone args so git: specs cannot inject options (#102398) * fix(plugins): terminate git clone args so git: specs cannot inject options A git: install spec whose base ends in .git or is scp-form was passed to git clone as a bare positional. A spec beginning with a dash, such as git:--upload-pack=/path/pwn.git, was parsed by git as an option rather than a repository, reaching a command-execution primitive. The sibling git switch --detach -- in the same files was already hardened; the clone sites were missed. Add a -- option terminator before the URL at both clone call sites (installPluginFromGitSpec and installSkillFromSource) and reject a leading-dash URL in isGitUrl as defense in depth. * test(plugins): prove git option rejection boundary Co-authored-by: yetval --------- Co-authored-by: Peter Steinberger (cherry picked from commit 9706aae10aeec17c831d89727089e0af2bf6ebe0) --- src/plugins/git-install.test.ts | 35 ++++++++++++++++++--- src/plugins/git-install.ts | 7 +++-- src/skills/lifecycle/source-install.test.ts | 17 ++++++++++ src/skills/lifecycle/source-install.ts | 4 +-- 4 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/plugins/git-install.test.ts b/src/plugins/git-install.test.ts index 33433d24ec59..b0046997f2a9 100644 --- a/src/plugins/git-install.test.ts +++ b/src/plugins/git-install.test.ts @@ -128,6 +128,11 @@ describe("parseGitPluginSpec", () => { expect(parsed.ref).toBe("feature/foo"); expect(parsed.label).toBe("git@github.com:acme/demo"); }); + + it("rejects option-injection specs whose url would start with a dash", () => { + expect(parseGitPluginSpec("git:--upload-pack=/tmp/pwn.git")).toBeNull(); + expect(parseGitPluginSpec("git:-oProxyCommand=payload@example.com:acme/demo.git")).toBeNull(); + }); }); describe("isImmutableGitCommitRef", () => { @@ -167,6 +172,19 @@ describe("installPluginFromGitSpec", () => { ); }); + it("rejects option-leading clone sources before invoking git", async () => { + const result = await installPluginFromGitSpec({ + spec: "git:--upload-pack=/tmp/pwn.git", + dryRun: true, + }); + + expect(result).toEqual({ + ok: false, + error: "unsupported git: plugin spec: git:--upload-pack=/tmp/pwn.git", + }); + expect(runCommandWithTimeoutMock).not.toHaveBeenCalled(); + }); + it("clones, checks out refs, installs from the clone, and returns commit metadata", async () => { runCommandWithTimeoutMock .mockResolvedValueOnce({ code: 0, stdout: "", stderr: "" }) @@ -206,8 +224,13 @@ describe("installPluginFromGitSpec", () => { expect(result.git.ref).toBe("v1.2.3"); expect(result.git.commit).toBe("abc123"); const cloneArgv = commandArgvAt(0); - expect(cloneArgv.slice(0, 3)).toEqual(["git", "clone", "https://github.com/acme/demo.git"]); - expect(cloneArgv[3]).toContain("/repo"); + expect(cloneArgv.slice(0, 4)).toEqual([ + "git", + "clone", + "--", + "https://github.com/acme/demo.git", + ]); + expect(cloneArgv[4]).toContain("/repo"); expect(commandArgvAt(1)).toEqual(["git", "switch", "--detach", "--", "v1.2.3"]); expect(commandArgvAt(3)).toEqual([ "npm", @@ -310,14 +333,15 @@ describe("installPluginFromGitSpec", () => { } const cloneArgv = commandArgvAt(0); - expect(cloneArgv.slice(0, 5)).toEqual([ + expect(cloneArgv.slice(0, 6)).toEqual([ "git", "clone", "--depth", "1", + "--", "https://github.com/acme/demo.git", ]); - expect(cloneArgv[5]).toContain("/repo"); + expect(cloneArgv[6]).toContain("/repo"); }); it("runs install policy preflight before npm installs git dependencies", async () => { @@ -347,11 +371,12 @@ describe("installPluginFromGitSpec", () => { expect(result.error).toContain("git installs disabled"); } expect(runCommandWithTimeoutMock).toHaveBeenCalledTimes(2); - expect(commandArgvAt(0).slice(0, 5)).toEqual([ + expect(commandArgvAt(0).slice(0, 6)).toEqual([ "git", "clone", "--depth", "1", + "--", "https://github.com/acme/demo.git", ]); expect(commandArgvAt(1)).toEqual(["git", "rev-parse", "HEAD"]); diff --git a/src/plugins/git-install.ts b/src/plugins/git-install.ts index 6bc7e15260ce..e6776f52266d 100644 --- a/src/plugins/git-install.ts +++ b/src/plugins/git-install.ts @@ -116,6 +116,9 @@ function isHttpUrl(value: string): boolean { } function isGitUrl(value: string): boolean { + if (value.startsWith("-")) { + return false; + } return ( /^(?:ssh|git|file):\/\//i.test(value) || looksLikeScpGitUrl(value) || value.endsWith(".git") ); @@ -345,8 +348,8 @@ export async function installPluginFromGitSpec( `Cloning ${sanitizeForLog(redactSensitiveUrlLikeString(parsed.label))}...`, ); const cloneArgs = parsed.ref - ? ["git", "clone", parsed.url, repoDir] - : ["git", "clone", "--depth", "1", parsed.url, repoDir]; + ? ["git", "clone", "--", parsed.url, repoDir] + : ["git", "clone", "--depth", "1", "--", parsed.url, repoDir]; const clone = await runGitCommand({ argv: cloneArgs, action: "clone", diff --git a/src/skills/lifecycle/source-install.test.ts b/src/skills/lifecycle/source-install.test.ts index d1b313539a2a..112e856d68f3 100644 --- a/src/skills/lifecycle/source-install.test.ts +++ b/src/skills/lifecycle/source-install.test.ts @@ -436,4 +436,21 @@ describe("installSkillFromSource", () => { }); }); }); + + it("refuses git specs whose url would be consumed as a git clone option", async () => { + await withTempDir({ prefix: "openclaw-skill-source-opt-inject-" }, async (root) => { + const workspaceDir = path.join(root, "workspace"); + const payload = path.join(root, "payload.git"); + + const result = await installSkillFromSource({ + workspaceDir, + spec: `git:--upload-pack=${payload}`, + }); + + expect(result).toMatchObject({ + ok: false, + error: expect.stringContaining("Unsupported git skill spec"), + }); + }); + }); }); diff --git a/src/skills/lifecycle/source-install.ts b/src/skills/lifecycle/source-install.ts index c2a89797c7b5..e78efee085a8 100644 --- a/src/skills/lifecycle/source-install.ts +++ b/src/skills/lifecycle/source-install.ts @@ -283,8 +283,8 @@ async function installGitSkill(params: { `Cloning ${sanitizeForLog(redactSensitiveUrlLikeString(parsed.label))}...`, ); const cloneArgs = parsed.ref - ? ["git", "clone", parsed.url, repoDir] - : ["git", "clone", "--depth", "1", parsed.url, repoDir]; + ? ["git", "clone", "--", parsed.url, repoDir] + : ["git", "clone", "--depth", "1", "--", parsed.url, repoDir]; const clone = await runGitCommand({ argv: cloneArgs, action: "clone",