mirror of
https://github.com/badlogic/pi-mono.git
synced 2026-07-09 17:28:45 +00:00
fix(coding-agent): harden git package install paths
This commit is contained in:
parent
0462d44f56
commit
a98e087e5d
5 changed files with 109 additions and 27 deletions
|
|
@ -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)).
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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),
|
||||
};
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -33,6 +33,10 @@ interface PackageManagerInternals {
|
|||
options?: { cwd?: string; timeoutMs?: number; env?: Record<string, string> },
|
||||
): Promise<string>;
|
||||
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");
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue