From a98e087e5d08ea2a536bf73dbb0aebb87c3ef72e Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Tue, 2 Jun 2026 18:20:35 +0200 Subject: [PATCH] fix(coding-agent): harden git package install paths --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/core/package-manager.ts | 19 ++++- packages/coding-agent/src/utils/git.ts | 80 +++++++++++++------ .../coding-agent/test/git-ssh-url.test.ts | 13 +++ .../coding-agent/test/package-manager.test.ts | 23 ++++++ 5 files changed, 109 insertions(+), 27 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 46eebe64b..5ddda5242 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -11,6 +11,7 @@ ### Fixed +- Fixed git package source handling to reject unsafe host/path components and keep managed clone paths inside install roots. - Fixed stored XSS in HTML session exports by sanitizing Markdown link and image URLs with a scheme allow-list after stripping control characters. - Fixed SDK embedding in bundled Node apps failing with `ENOENT` when `package.json` is not present next to the bundle entrypoint. The package metadata reader now gracefully handles missing `package.json` by using defaults, enabling `createAgentSession()` without requiring package-adjacent files at runtime ([#5226](https://github.com/earendil-works/pi/issues/5226)). - Fixed HTTP timeout setting not being respected for non-Codex providers (e.g., llama.cpp via OpenAI-compatible API). The `httpIdleTimeoutMs` setting (set via `/settings` HTTP timeout) now applies as the default SDK request timeout for all providers that support it, not just OpenAI Codex Responses. Disabling the timeout (HTTP timeout = false) now correctly disables SDK timeouts for all supported providers by sending a maximum int32 value (effectively infinite) instead of 0, since SDKs treat timeout=0 as an immediate timeout ([#5294](https://github.com/earendil-works/pi/issues/5294)). diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 1d7dc27d3..7c0e96a17 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -1952,10 +1952,11 @@ export class DefaultPackageManager implements PackageManager { if (scope === "temporary") { return this.getTemporaryDir(`git-${source.host}`, source.path); } - if (scope === "project") { - return join(this.cwd, CONFIG_DIR_NAME, "git", source.host, source.path); + const installRoot = this.getGitInstallRoot(scope); + if (!installRoot) { + throw new Error("Missing git install root"); } - return join(this.agentDir, "git", source.host, source.path); + return this.resolveManagedPath(installRoot, source.host, source.path); } private getGitInstallRoot(scope: SourceScope): string | undefined { @@ -1969,11 +1970,21 @@ export class DefaultPackageManager implements PackageManager { } private getTemporaryDir(prefix: string, suffix?: string): string { + const root = this.resolveManagedPath(join(tmpdir(), "pi-extensions"), prefix); const hash = createHash("sha256") .update(`${prefix}-${suffix ?? ""}`) .digest("hex") .slice(0, 8); - return join(tmpdir(), "pi-extensions", prefix, hash, suffix ?? ""); + return this.resolveManagedPath(root, hash, suffix ?? ""); + } + + private resolveManagedPath(root: string, ...parts: string[]): string { + const resolvedRoot = resolve(root); + const resolvedPath = resolve(resolvedRoot, ...parts); + if (resolvedPath !== resolvedRoot && !resolvedPath.startsWith(`${resolvedRoot}${sep}`)) { + throw new Error(`Refusing to use path outside package install root: ${resolvedPath}`); + } + return resolvedPath; } private getBaseDirForScope(scope: SourceScope): string { diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index 9652e4018..1314edea7 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -73,6 +73,56 @@ function splitRef(url: string): { repo: string; ref?: string } { }; } +function decodeForValidation(value: string): string | null { + try { + return decodeURIComponent(value); + } catch { + return null; + } +} + +function hasUnsafeGitInstallPart(value: string, allowSlash: boolean): boolean { + const decoded = decodeForValidation(value); + if (decoded === null) { + return true; + } + const candidates = [value, decoded]; + for (const candidate of candidates) { + if (candidate.includes("\0") || candidate.includes("\\") || candidate.startsWith("/")) { + return true; + } + if (!allowSlash && candidate.includes("/")) { + return true; + } + if (candidate.split("/").includes("..")) { + return true; + } + } + return false; +} + +function buildGitSource(args: { repo: string; host: string; path: string; ref?: string }): GitSource | null { + if (args.path.startsWith("/")) { + return null; + } + const normalizedPath = args.path.replace(/\.git$/, "").replace(/^\/+/, ""); + if (!args.host || !normalizedPath || normalizedPath.split("/").length < 2) { + return null; + } + if (hasUnsafeGitInstallPart(args.host, false) || hasUnsafeGitInstallPart(normalizedPath, true)) { + return null; + } + + return { + type: "git", + repo: args.repo, + host: args.host, + path: normalizedPath, + ref: args.ref, + pinned: Boolean(args.ref), + }; +} + function parseGenericGitUrl(url: string): GitSource | null { const { repo: repoWithoutRef, ref } = splitRef(url); let repo = repoWithoutRef; @@ -109,19 +159,7 @@ function parseGenericGitUrl(url: string): GitSource | null { repo = `https://${repoWithoutRef}`; } - const normalizedPath = path.replace(/\.git$/, "").replace(/^\/+/, ""); - if (!host || !normalizedPath || normalizedPath.split("/").length < 2) { - return null; - } - - return { - type: "git", - repo, - host, - path: normalizedPath, - ref, - pinned: Boolean(ref), - }; + return buildGitSource({ repo, host, path, ref }); } /** @@ -157,14 +195,12 @@ export function parseGitUrl(source: string): GitSource | null { !split.repo.startsWith("ssh://") && !split.repo.startsWith("git://") && !split.repo.startsWith("git@"); - return { - type: "git", + return buildGitSource({ repo: useHttpsPrefix ? `https://${split.repo}` : split.repo, host: info.domain || "", - path: `${info.user}/${info.project}`.replace(/\.git$/, ""), + path: `${info.user}/${info.project}`, ref: info.committish || split.ref || undefined, - pinned: Boolean(info.committish || split.ref), - }; + }); } } @@ -177,14 +213,12 @@ export function parseGitUrl(source: string): GitSource | null { if (split.ref && info.project?.includes("@")) { continue; } - return { - type: "git", + return buildGitSource({ repo: `https://${split.repo}`, host: info.domain || "", - path: `${info.user}/${info.project}`.replace(/\.git$/, ""), + path: `${info.user}/${info.project}`, ref: info.committish || split.ref || undefined, - pinned: Boolean(info.committish || split.ref), - }; + }); } } diff --git a/packages/coding-agent/test/git-ssh-url.test.ts b/packages/coding-agent/test/git-ssh-url.test.ts index 6913b70c4..87b8b9400 100644 --- a/packages/coding-agent/test/git-ssh-url.test.ts +++ b/packages/coding-agent/test/git-ssh-url.test.ts @@ -62,6 +62,19 @@ describe("Git URL Parsing", () => { }); }); + it("should reject unsafe git install path inputs", () => { + for (const source of [ + "git:git@evil.example:../../victim/repo", + "https://evil.example/..%2F..%2Fvictim/repo", + "https://evil.example/..%2F..%2Fvictim/repo%", + "git:git@evil.example:/absolute/repo", + "git:git@evil.example:user\\repo/name", + "git:git@evil.example:user/repo\0name", + ]) { + expect(parseGitUrl(source)).toBeNull(); + } + }); + describe("unsupported without git: prefix", () => { it("should reject git@host:path without git: prefix", () => { expect(parseGitUrl("git@github.com:user/repo")).toBeNull(); diff --git a/packages/coding-agent/test/package-manager.test.ts b/packages/coding-agent/test/package-manager.test.ts index af3bc99d3..848b37bde 100644 --- a/packages/coding-agent/test/package-manager.test.ts +++ b/packages/coding-agent/test/package-manager.test.ts @@ -33,6 +33,10 @@ interface PackageManagerInternals { options?: { cwd?: string; timeoutMs?: number; env?: Record }, ): Promise; getLocalGitUpdateTarget(installedPath: string): Promise<{ ref: string; head: string; fetchArgs: string[] }>; + getGitInstallPath( + source: { type: "git"; repo: string; host: string; path: string; pinned: boolean; ref?: string }, + scope: "user" | "project" | "temporary", + ): string; } // Helper to check if a resource is enabled @@ -1138,6 +1142,25 @@ Content`, }); }); + describe("git install paths", () => { + it("should reject paths outside git install roots", () => { + const managerWithInternals = packageManager as unknown as PackageManagerInternals; + const traversalSource = { + type: "git" as const, + repo: "git@evil.example:../../victim/repo", + host: "evil.example", + path: "../../victim/repo", + pinned: false, + }; + + for (const scope of ["user", "project", "temporary"] as const) { + expect(() => managerWithInternals.getGitInstallPath(traversalSource, scope)).toThrow( + "outside package install root", + ); + } + }); + }); + describe("settings source normalization", () => { it("should store global local packages relative to agent settings base", () => { const pkgDir = join(tempDir, "packages", "local-global-pkg");