Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/dotagents-lib/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,10 @@ export type {

// General-purpose utilities used by callers
export { exec, ExecError } from "./utils/exec.js";
export { copyDir, stripTrailingSlashes } from "./utils/fs.js";
export {
copyDir,
isAbsolutePathString,
isInsideDir,
isWindowsDrivePath,
stripTrailingSlashes,
} from "./utils/fs.js";
69 changes: 68 additions & 1 deletion packages/dotagents-lib/src/sources/cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest";
import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
import { join } from "node:path";
import { tmpdir } from "node:os";
import { ensureCached, validateCacheKey, CacheError } from "./cache.js";
import {
ensureCached,
sanitizeCacheKey,
validateCacheKey,
assertUrlUsableOnPlatform,
CacheError,
} from "./cache.js";
import { exec } from "../utils/exec.js";

async function configureTestGitRepo(repoDir: string): Promise<void> {
Expand Down Expand Up @@ -37,6 +43,67 @@ describe("validateCacheKey", () => {
});
});

describe("sanitizeCacheKey", () => {
it.each([
["https URL", "https://github.com/owner/repo.git", "github.com/owner/repo"],
["posix absolute path", "/home/foo/skills", "home/foo/skills"],
["file:// URL", "file:///home/foo/skills", "home/foo/skills"],
// Windows local sources: a `git:C:\...` spec reaches here as a drive-lettered
// path. Both separator styles must reduce to a `/`-separated relative key with
// the colon dropped, keeping the drive as a segment so C: and D: don't collide.
["windows backslash path", "C:\\Users\\ofek\\skills", "C/Users/ofek/skills"],
["windows forward-slash path", "C:/Users/ofek/skills", "C/Users/ofek/skills"],
["file:// windows path", "file:///C:/Users/ofek/skills", "C/Users/ofek/skills"],
// A doubled separator must not let the drive letter be read as a URL
// scheme and stripped; a bare drive must not collapse to an empty key.
["windows doubled separator", "C:\\\\Users\\\\skills", "C/Users/skills"],
["bare windows drive", "C:\\\\", "C"],
// Drive-relative paths have no separator after the colon; the colon still
// has to go, or the key is an invalid dir on Windows / an scp target.
["drive-relative path", "C:foo", "Cfoo"],
["file:// drive-relative", "file:///C:foo", "Cfoo"],
])("derives %s -> a valid relative key", (_label, url, expected) => {
const key = sanitizeCacheKey(url);
expect(key).toBe(expected);
// The whole point: the derived key must survive validateCacheKey so
// ensureCached can join it under stateDir without throwing.
expect(() => validateCacheKey(key)).not.toThrow();
});

it("keeps different drives distinct", () => {
expect(sanitizeCacheKey("C://team/skills")).toBe("C/team/skills");
expect(sanitizeCacheKey("D://team/skills")).toBe("D/team/skills");
});
});

describe("assertUrlUsableOnPlatform", () => {
it("rejects a Windows drive path off Windows", () => {
// Off win32 git reads `C:` as an scp host and tries to SSH to host "C";
// fail with a legible error instead.
expect(() => assertUrlUsableOnPlatform("C:\\Users\\me\\repo", "linux")).toThrow(
CacheError,
);
expect(() => assertUrlUsableOnPlatform("C:/Users/me/repo", "darwin")).toThrow(
/Windows drive path/,
);
});

it("allows a Windows drive path on Windows", () => {
expect(() =>
assertUrlUsableOnPlatform("C:\\Users\\me\\repo", "win32"),
).not.toThrow();
});

it.each([
"https://example.com/repo.git",
"/home/me/repo",
"git@example.com:owner/repo.git",
"file:///home/me/repo",
])("allows %s on any platform", (url) => {
expect(() => assertUrlUsableOnPlatform(url, "linux")).not.toThrow();
});
});

describe("ensureCached", () => {
let tmpDir: string;
let stateDir: string;
Expand Down
43 changes: 38 additions & 5 deletions packages/dotagents-lib/src/sources/cache.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { join } from "node:path";
import { mkdir, rm } from "node:fs/promises";
import { clone, fetchAndReset, fetchRef, headCommit, headCommitDate, findCommitOlderThan, checkout, isGitRepo } from "./git.js";
import { isAbsolutePathString, isWindowsDrivePath } from "../utils/fs.js";

export class CacheError extends Error {
constructor(message: string) {
Expand All @@ -10,19 +11,30 @@ export class CacheError extends Error {
}

/**
* Derive a safe relative cache-key from a clone URL.
* Derive a safe relative cache-key from a clone URL or local path.
*
* Strips schemes, leading slashes (so `file:///abs/path` and `/abs/path`
* both land under `<stateDir>/abs/path/`), and `.git` suffixes. Drops `.`
* Normalizes Windows separators to `/`, strips schemes, leading slashes (so
* `file:///abs/path` and `/abs/path` both land under `<stateDir>/abs/path/`),
* a Windows drive-letter colon (`C:\repo` -> `C/repo`, keeping the drive as a
* segment so different drives don't collide), and `.git` suffixes. Drops `.`
* and `..` segments to keep the result inside the cache root.
*
* The result always satisfies `validateCacheKey` (relative, `/`-separated, no
* drive prefix), so a local `git:C:\...` source on Windows caches cleanly.
*
* Callers that build a cacheKey from a hosted owner/repo (e.g. GitHub) can
* skip this and use the parsed values directly.
*/
export function sanitizeCacheKey(url: string): string {
const stripped = url
.replace(/^[a-z]+:\/\//i, "")
.replaceAll("\\", "/")
// Require 2+ scheme letters so a Windows drive (`C://...`) isn't mistaken
// for a URL scheme and stripped (which would drop the drive and collide).
.replace(/^[a-z]{2,}:\/\//i, "")
.replace(/^\/+/, "")
// Drop a leading drive-letter colon (`C:` -> `C`); no lookahead, so a
// drive-relative `C:foo` collapses to `Cfoo` rather than leaking a colon.
.replace(/^([a-zA-Z]):/, "$1")
.replace(/\.git$/, "");
const segments = stripped
.split("/")
Expand All @@ -43,7 +55,7 @@ export function validateCacheKey(cacheKey: string): void {
if (!cacheKey) {
throw new CacheError("cacheKey must be a non-empty string");
}
if (cacheKey.startsWith("/") || /^[a-zA-Z]:[\\/]/.test(cacheKey)) {
if (isAbsolutePathString(cacheKey)) {
throw new CacheError(`cacheKey must be a relative path, got "${cacheKey}"`);
}
if (cacheKey.includes("\\")) {
Expand All @@ -56,6 +68,26 @@ export function validateCacheKey(cacheKey: string): void {
}
}

/**
* Reject a Windows drive path (`C:\repo`, `C:/repo`) on a non-Windows host.
*
* Only Windows git treats a leading drive letter as a local directory.
* Everywhere else the colon before the first slash makes `git clone` read it as
* scp syntax (`host:path`) and attempt an SSH connection to a host named `C`,
* which fails confusingly or hangs on a network timeout.
*
* The config schema stays platform-independent — it validates the *form* of a
* source — so this checks whether that form is usable on *this* machine.
*/
export function assertUrlUsableOnPlatform(url: string, platform: string): void {
if (platform !== "win32" && isWindowsDrivePath(url)) {
throw new CacheError(
`Source "${url}" is a Windows drive path and cannot be used on ${platform}. ` +
`Use a POSIX absolute path or a remote git URL.`,
);
}
}

export interface CacheResult {
/** Path to the cached repo checkout */
repoDir: string;
Expand Down Expand Up @@ -109,6 +141,7 @@ export async function ensureCached(opts: {
/** When set, resolve to the newest commit at least this many minutes old. */
minimumReleaseAge?: number;
}): Promise<CacheResult> {
assertUrlUsableOnPlatform(opts.url, process.platform);
validateCacheKey(opts.cacheKey);
const repoDir = join(opts.stateDir, opts.cacheKey);

Expand Down
62 changes: 62 additions & 0 deletions packages/dotagents-lib/src/sources/local.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import { describe, it, expect, beforeEach, afterEach } from "vitest";
import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises";
import { join } from "node:path";
import { tmpdir } from "node:os";
import { resolveLocalSource, LocalSourceError } from "./local.js";

describe("resolveLocalSource", () => {
let root: string;

beforeEach(async () => {
root = await mkdtemp(join(tmpdir(), "dotagents-local-"));
await mkdir(join(root, "skills", "review"), { recursive: true });
});

afterEach(async () => {
await rm(root, { recursive: true, force: true });
});

it("resolves a nested child directory", async () => {
// The containment check must be separator-agnostic: on Windows `root`
// uses `\`, so a `${root}/` string prefix would never match this child.
const resolved = await resolveLocalSource(root, "skills/review");
expect(resolved).toBe(join(root, "skills", "review"));
});

it("resolves the project root itself", async () => {
expect(await resolveLocalSource(root, ".")).toBe(root);
});

it("rejects a parent-traversal path that escapes the root", async () => {
await expect(resolveLocalSource(root, "../escape")).rejects.toThrow(
LocalSourceError,
);
});

it("rejects the bare parent directory", async () => {
// Covers the `rel === ".."` guard specifically — the other traversal cases
// all produce `..${sep}<child>` and only exercise the startsWith branch.
await expect(resolveLocalSource(root, "..")).rejects.toThrow(
/resolves outside project root/,
);
});

it("rejects traversal that dips out and back in", async () => {
await expect(
resolveLocalSource(root, "skills/../../elsewhere"),
).rejects.toThrow(/resolves outside project root/);
});

it("errors when the source does not exist", async () => {
await expect(resolveLocalSource(root, "skills/missing")).rejects.toThrow(
/not found/,
);
});

it("errors when the source is a file, not a directory", async () => {
await writeFile(join(root, "afile.md"), "not a dir\n");
await expect(resolveLocalSource(root, "afile.md")).rejects.toThrow(
/not a directory/,
);
});
});
6 changes: 3 additions & 3 deletions packages/dotagents-lib/src/sources/local.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { resolve } from "node:path";
import { stat } from "node:fs/promises";
import { isInsideDir } from "../utils/fs.js";

export class LocalSourceError extends Error {
constructor(message: string) {
Expand All @@ -16,11 +17,10 @@ export async function resolveLocalSource(
projectRoot: string,
relativePath: string,
): Promise<string> {
const absRoot = resolve(projectRoot);
const absPath = resolve(projectRoot, relativePath);

// Prevent path traversal outside the project root
if (!absPath.startsWith(`${absRoot}/`) && absPath !== absRoot) {
// Prevent path traversal outside the project root.
if (!isInsideDir(projectRoot, absPath)) {
throw new LocalSourceError(
`Local source "${relativePath}" resolves outside project root`,
);
Expand Down
45 changes: 45 additions & 0 deletions packages/dotagents-lib/src/trust/validator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,51 @@ describe("validateTrustedSource", () => {
});
});

describe("local filesystem git: sources", () => {
// These read files already on the user's disk — the same trust surface as
// `path:` — and expose no domain, so gating them on `git_domains` would
// make them impossible to allow at all.
const trust = makeTrust({ git_domains: ["git.corp.example.com"] });

it.each([
"git:/home/me/local-repo",
"git:C:\\Users\\me\\local-repo",
"git:C:/Users/me/local-repo",
"git:file:///home/me/local-repo",
])("allows %s with no matching domain rule", (source) => {
expect(() => validateTrustedSource(source, trust)).not.toThrow();
});

it("still gates a file:// URL that names a host", () => {
expect(() =>
validateTrustedSource("git:file://evil.com/share/repo", trust),
).toThrow(TrustError);
});

it.each([
// UNC is a remote host in absolute-path clothing: `git:` followed by
// `//server/...`, any longer run of leading separators, a host-empty
// `file:` URL whose path is UNC, and the backslash spelling. Letting any
// of these through the local bypass would reach SMB with no git_domains
// entry — on Windows, a credential-exposure vector.
"git://server/share/repo",
"git://///server/share/repo",
"git:file:////server/share/repo",
"git:\\\\server\\share\\repo",
])("does not treat UNC path %s as local", (source) => {
expect(() => validateTrustedSource(source, trust)).toThrow(TrustError);
});

it("still gates remote sources and bare relative paths", () => {
expect(() =>
validateTrustedSource("git:https://evil.com/repo.git", trust),
).toThrow(TrustError);
expect(() =>
validateTrustedSource("git:relative/path", trust),
).toThrow(TrustError);
});
});

describe("git_domains with path prefixes", () => {
it("allows repos under a trusted domain path (https)", () => {
const trust = makeTrust({ git_domains: ["gitlab.com/myorg"] });
Expand Down
50 changes: 49 additions & 1 deletion packages/dotagents-lib/src/trust/validator.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { parseSource } from "../skills/resolver.js";
import { isAbsolutePathString } from "../utils/fs.js";
import type { TrustPolicy } from "./policy.js";

/**
Expand Down Expand Up @@ -101,6 +102,47 @@ export function extractDomainPath(url: string): string | undefined {
}
}

/**
* Whether `s` is a UNC network path (`\\server\share`, `//server/share`).
*
* UNC looks absolute but names a remote host reached over SMB, which is exactly
* what `git_domains` exists to gate. Matches any run of two or more leading
* separators, in either slash direction.
*/
function isUncPath(s: string): boolean {
return /^[\\/]{2}/.test(s);
}

/**
* Whether a `git:` URL points at the local filesystem rather than a remote host.
*
* Local git sources read files the user already has on disk — the same trust
* surface as a `path:` source, which is unconditionally allowed — and they
* expose no domain for `git_domains` to match, so gating them on domain would
* make them impossible to allow at all (short of `allow_all`).
*
* Only absolute *local* forms count: a bare relative path and any UNC network
* path stay subject to the domain rules so neither can be smuggled through.
*/
function isLocalFilesystemUrl(url: string): boolean {
// Reject UNC before the absolute-path check, which would otherwise accept
// `//server/share` on its leading slash.
if (isUncPath(url)) {return false;}
if (isAbsolutePathString(url)) {return true;}
// `file:///abs/path` is local. `file://host/share` names a host, and
// `file:////server/share` has an empty host but a UNC path — both stay
// subject to the domain rules below.
if (/^file:\/\//i.test(url)) {
try {
const { host, pathname } = new URL(url);
return !host && !isUncPath(pathname);
} catch {
return false;
}
}
return false;
}

function formatAllowed(trust: TrustPolicy): string {
const parts: string[] = [];
if (trust.github_orgs.length > 0) {
Expand All @@ -120,7 +162,8 @@ function formatAllowed(trust: TrustPolicy): string {
*
* - No trust config → allow all (backward compat)
* - allow_all = true → allow all
* - Local path: sources → always allowed
* - Local sources → always allowed: `path:`, and `git:` pointing at an
* absolute local path (`git:/abs/repo`, `git:C:\repo`, `file:///abs/repo`)
* - Otherwise → must match at least one rule (org, repo, or domain)
*/
export function validateTrustedSource(
Expand Down Expand Up @@ -158,6 +201,11 @@ export function validateTrustedSource(
}

if (parsed.type === "git" || parsed.type === "well-known") {
// A `git:` source pointing at a local absolute path has no domain to match,
// and the same trust surface as `path:` — allow it rather than throwing an
// error the user has no `git_domains` entry to fix.
if (parsed.type === "git" && isLocalFilesystemUrl(parsed.url!)) {return;}

const domainPath = extractDomainPath(parsed.url!)?.toLowerCase();
if (domainPath && trust.git_domains.some((d) => {
const entry = d.toLowerCase();
Expand Down
Loading
Loading