From b235792fde5ea6b3628df9fc9d871ec1a4c8ca7c Mon Sep 17 00:00:00 2001 From: oudi Date: Wed, 5 Aug 2026 15:30:28 +0800 Subject: [PATCH] fix: compare git-emitted paths correctly on Windows `git rev-parse --path-format=absolute` prints POSIX-style absolute paths even on Windows (`D:/repo`), while `realpathSync()` and the session cwds recorded by pi are native (`D:\repo`). `resolveProject()` compared the two with `===`, so on Windows: - `isTopLevel` was permanently false, which hid the worktree switcher entirely and left only the disabled "Open repo root" hint; - `isWorktreeTopLevel` was therefore also always false, so linked worktrees never collapsed into the main repo's `projectRoot` and each showed up as a separate phantom project instead of being grouped; - `listWorktrees()` returned slash-style paths that never matched `selectedCwd`, breaking the current-worktree highlight in the sidebar. Route every path read out of git through `toNativePath()` and compare with `samePath()`, which also tolerates drive-letter case since Windows paths are case-insensitive. Branch names deliberately skip normalization so `feature/x` does not become `feature\x`. Both helpers live in a new `lib/paths.ts` alongside `isWindowsAbsolutePath()`, which had been duplicated in `lib/file-access.ts` and `lib/path-security.ts`. `isFilePathAllowed()` was a byte-for-byte copy of `isPathWithinRoots()` and now delegates to it, leaving one implementation of the access-control check. The allowed-roots set keeps its slash-normalized form: it is an internal Set key that is never displayed, and containment checks re-normalize both sides anyway, so both path forms authorize identically. Verified on Windows against real repositories: repo roots now resolve as top-level, subdirectories still do not, linked worktrees collapse to the main repo, and `feature/x` keeps its slash. Adds `lib/paths.test.mjs` covering separator style, drive-letter case and UNC paths. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 2 ++ lib/allowed-roots.ts | 9 +++++- lib/file-access.test.mjs | 6 +++- lib/file-access.ts | 24 +++------------- lib/path-security.ts | 12 ++++---- lib/paths.test.mjs | 54 ++++++++++++++++++++++++++++++++++++ lib/paths.ts | 59 ++++++++++++++++++++++++++++++++++++++++ lib/worktree.ts | 16 +++++++---- 8 files changed, 148 insertions(+), 34 deletions(-) create mode 100644 lib/paths.test.mjs create mode 100644 lib/paths.ts diff --git a/AGENTS.md b/AGENTS.md index ad4735ecb..aae2f7943 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -159,10 +159,12 @@ Newer pi emits `compaction_start` / `compaction_end`; older versions emitted `au - New worktrees are created under `-worktrees/`. Existing branches are reused; otherwise `git worktree add -b` creates the branch. - Removing a dirty worktree returns `409` with `{ dirty: true }` so the UI can ask before retrying with `force`. - Sessions whose cwd points at a removed worktree are inferred back into the main project instead of becoming a phantom project row. +- git prints POSIX-style absolute paths even on Windows, so every path read out of git goes through `toNativePath()` (`lib/paths.ts`) before it is compared or returned. Compare paths with `samePath()`, never `===` — raw equality made `isTopLevel` permanently false on Windows and hid the worktree switcher entirely. Branch names are not paths and must keep their forward slashes. ### File access allow-list - `/api/files` is intentionally not a general filesystem browser. Allowed roots come from session cwds, their resolved project roots, `~/pi-cwd-*`, and roots explicitly added with `allowFileRoot()`. - `/api/cwd/validate`, `/api/default-cwd`, and `/api/worktrees` call `allowFileRoot()` when they make a new location browsable. +- Allowed roots are stored slash-normalized, but that is a Set-key convention, not a correctness requirement: `isPathWithinRoots()` (`lib/path-security.ts`, the single implementation behind `isFilePathAllowed()`) re-resolves and case-folds both sides, so either path form authorizes correctly. Keep that one implementation — it is the security boundary. ### Plugins and skills - `/api/plugins` uses pi's `SettingsManager` + `DefaultPackageManager` for global/project package install, remove, update, enable, and disable. Disabling writes empty `extensions/skills/prompts/themes` arrays for that package entry. diff --git a/lib/allowed-roots.ts b/lib/allowed-roots.ts index 02b20fbd2..5ef2aabdf 100644 --- a/lib/allowed-roots.ts +++ b/lib/allowed-roots.ts @@ -1,3 +1,5 @@ +import { toSlashPath } from "./paths"; + // In-memory roots that should be browsable in addition to roots derived from // persisted sessions. Stored on globalThis so Next.js hot-reload keeps them. declare global { @@ -5,8 +7,13 @@ declare global { var __piAdditionalAllowedRoots: Set | undefined; } +/** + * Allowed roots are internal bookkeeping keys that are never displayed, so they + * are stored slash-normalized for consistent Set membership. Correctness does + * not depend on it — isPathWithinRoots() re-normalizes whatever it is given. + */ export function normalizeSlashes(filePath: string): string { - return filePath.replace(/\\/g, "/"); + return toSlashPath(filePath); } export function getAdditionalAllowedRoots(): Set { diff --git a/lib/file-access.test.mjs b/lib/file-access.test.mjs index f7acaf781..7ef521211 100644 --- a/lib/file-access.test.mjs +++ b/lib/file-access.test.mjs @@ -4,8 +4,12 @@ import os from "node:os"; import path from "node:path"; import test from "node:test"; +// Loaded through jiti so the module's own extensionless imports resolve the way +// the app resolves them (tsconfig moduleResolution: "bundler"); bare +// `import("./path-security.ts")` only works while that file has no imports. async function loadSubject() { - return import("./path-security.ts"); + const { createJiti } = await import("jiti"); + return createJiti(import.meta.url).import("./path-security.ts"); } test("rejects an existing path that escapes an allowed root through a symlink", async (t) => { diff --git a/lib/file-access.ts b/lib/file-access.ts index 8a31f1cef..4eb08f3ef 100644 --- a/lib/file-access.ts +++ b/lib/file-access.ts @@ -2,9 +2,10 @@ import { readdirSync } from "fs"; import { homedir } from "os"; import path from "path"; import { getAdditionalAllowedRoots, normalizeSlashes } from "./allowed-roots"; -import { isExistingPathWithinRoots } from "./path-security"; +import { isExistingPathWithinRoots, isPathWithinRoots } from "./path-security"; import { listAllSessions } from "./session-reader"; export { allowFileRoot, normalizeSlashes } from "./allowed-roots"; +export { isWindowsAbsolutePath } from "./paths"; // Short-TTL cache for the allowed-roots set. Without this, every file list/read // request re-scans every pi session on disk just to check access. 5s is short @@ -15,11 +16,6 @@ declare global { } const ALLOWED_ROOTS_TTL_MS = 5_000; -const WINDOWS_ABSOLUTE_RE = /^[a-zA-Z]:[\\/]/; - -export function isWindowsAbsolutePath(filePath: string): boolean { - return WINDOWS_ABSOLUTE_RE.test(filePath) || filePath.startsWith("\\\\") || filePath.startsWith("//"); -} export async function getAllowedFileRoots(): Promise> { const now = Date.now(); @@ -52,21 +48,9 @@ export async function getAllowedFileRoots(): Promise> { return roots; } +/** Authorize a path lexically, without touching the filesystem. */ export function isFilePathAllowed(target: string, allowedRoots: Set): boolean { - for (const root of allowedRoots) { - const useWindowsRules = isWindowsAbsolutePath(target) || isWindowsAbsolutePath(root); - const resolver = useWindowsRules ? path.win32 : path; - const sep = useWindowsRules ? "\\" : path.sep; - const normalized = resolver.resolve(target); - const normalizedRoot = resolver.resolve(root); - const comparable = useWindowsRules ? normalized.toLowerCase() : normalized; - const comparableRoot = useWindowsRules ? normalizedRoot.toLowerCase() : normalizedRoot; - const rootWithSep = comparableRoot.endsWith(sep) ? comparableRoot : comparableRoot + sep; - if (comparable === comparableRoot || comparable.startsWith(rootWithSep)) { - return true; - } - } - return false; + return isPathWithinRoots(target, allowedRoots); } /** Authorize an existing path after resolving symbolic links. */ diff --git a/lib/path-security.ts b/lib/path-security.ts index 2a7e7b364..91af8c76f 100644 --- a/lib/path-security.ts +++ b/lib/path-security.ts @@ -1,12 +1,12 @@ import { realpathSync } from "fs"; import path from "path"; +import { isWindowsAbsolutePath } from "./paths"; -const WINDOWS_ABSOLUTE_RE = /^[a-zA-Z]:[\\/]/; - -function isWindowsAbsolutePath(filePath: string): boolean { - return WINDOWS_ABSOLUTE_RE.test(filePath) || filePath.startsWith("\\\\") || filePath.startsWith("//"); -} - +/** + * Lexical containment check. Accepts either canonical form on both sides: it + * re-resolves through path.win32/path.posix and case-folds on Windows, so + * separator style and drive-letter case never decide the answer. + */ export function isPathWithinRoots(target: string, roots: Set): boolean { for (const root of roots) { const useWindowsRules = isWindowsAbsolutePath(target) || isWindowsAbsolutePath(root); diff --git a/lib/paths.test.mjs b/lib/paths.test.mjs new file mode 100644 index 000000000..970c8f36f --- /dev/null +++ b/lib/paths.test.mjs @@ -0,0 +1,54 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +const isWindows = process.platform === "win32"; + +async function loadSubject() { + const { createJiti } = await import("jiti"); + return createJiti(import.meta.url).import("./paths.ts"); +} + +test("toNativePath converts git's POSIX output to native separators", async () => { + const { toNativePath } = await loadSubject(); + if (isWindows) { + // The regression this guards: `git rev-parse --path-format=absolute` prints + // `D:/repo` on Windows, which never string-compares equal to a session cwd. + assert.equal(toNativePath("D:/repo/sub"), "D:\\repo\\sub"); + assert.equal(toNativePath("D:\\repo\\sub"), "D:\\repo\\sub"); + } else { + assert.equal(toNativePath("/repo/sub"), "/repo/sub"); + } + assert.equal(toNativePath(""), ""); +}); + +test("samePath ignores separator style and Windows case", async () => { + const { samePath } = await loadSubject(); + assert.equal(samePath("/a/b", "/a/b"), true); + assert.equal(samePath("/a/b", "/a/c"), false); + assert.equal(samePath("", ""), true); + assert.equal(samePath("", "/a"), false); + + if (isWindows) { + assert.equal(samePath("D:/repo", "D:\\repo"), true, "separator style must not matter"); + assert.equal(samePath("d:\\repo", "D:\\repo"), true, "drive-letter case must not matter"); + assert.equal(samePath("D:\\Repo\\Sub", "d:/repo/sub"), true); + assert.equal(samePath("D:\\repo", "D:\\repo2"), false); + } else { + // POSIX is case-sensitive and backslash is a legal filename character. + assert.equal(samePath("/Repo", "/repo"), false); + } +}); + +test("toSlashPath normalizes to forward slashes", async () => { + const { toSlashPath } = await loadSubject(); + assert.equal(toSlashPath("D:\\repo\\sub"), "D:/repo/sub"); + assert.equal(toSlashPath("/repo/sub"), "/repo/sub"); +}); + +test("isWindowsAbsolutePath recognizes drive and UNC paths", async () => { + const { isWindowsAbsolutePath } = await loadSubject(); + assert.equal(isWindowsAbsolutePath("D:\\repo"), true); + assert.equal(isWindowsAbsolutePath("d:/repo"), true); + assert.equal(isWindowsAbsolutePath("\\\\server\\share"), true); + assert.equal(isWindowsAbsolutePath("relative/path"), false); +}); diff --git a/lib/paths.ts b/lib/paths.ts new file mode 100644 index 000000000..f82e929c8 --- /dev/null +++ b/lib/paths.ts @@ -0,0 +1,59 @@ +import { normalize } from "path"; + +// ============================================================================ +// Path primitives. +// +// Two canonical forms coexist deliberately — pick by where the path is going: +// +// toNativePath() Native separators (`D:\repo` on Windows). Use for anything +// that reaches fs/path APIs, gets compared against a session +// cwd, or is shown to the user. This is the form pi records +// cwds in, so it is the default for user-facing paths. +// +// toSlashPath() Forward slashes (`D:/repo`). Use only for internal, +// never-displayed bookkeeping — the allowed-roots set, and +// separator-insensitive text matching. Containment checks +// re-normalize their inputs anyway (see path-security.ts), +// so this form is about consistent keys, not correctness. +// +// Comparison always goes through samePath()/isPathWithinRoots(), never `===`: +// git emits POSIX-style paths even on Windows, and Windows itself is +// case-insensitive, so raw string equality silently fails on both counts. +// ============================================================================ + +const WINDOWS_ABSOLUTE_RE = /^[a-zA-Z]:[\\/]/; + +export function isWindowsAbsolutePath(filePath: string): boolean { + return WINDOWS_ABSOLUTE_RE.test(filePath) || filePath.startsWith("\\\\") || filePath.startsWith("//"); +} + +/** + * Convert a path to native separators. Chiefly for git output: git prints + * POSIX-style absolute paths even on Windows (`D:/repo/sub`), which never + * string-compares equal to the native paths Node and pi produce. + * + * Only pass paths — a branch name like `feature/x` would become `feature\x`. + */ +export function toNativePath(p: string): string { + if (!p || process.platform !== "win32") return p; + return normalize(p); +} + +/** Convert a path to forward slashes. See the form guidance above. */ +export function toSlashPath(p: string): string { + return p.replace(/\\/g, "/"); +} + +/** + * Whether two paths denote the same location, tolerating separator style and — + * on Windows, where the filesystem is case-insensitive — case, including the + * drive letter (`d:\repo` vs `D:\repo`). + * + * Compares lexically: callers wanting symlinks resolved should realpath first. + */ +export function samePath(a: string, b: string): boolean { + if (a === b) return true; + if (!a || !b) return false; + if (process.platform !== "win32") return false; + return toNativePath(a).toLowerCase() === toNativePath(b).toLowerCase(); +} diff --git a/lib/worktree.ts b/lib/worktree.ts index 1f8fe36cb..c7e8a99e2 100644 --- a/lib/worktree.ts +++ b/lib/worktree.ts @@ -3,6 +3,7 @@ import { existsSync, mkdirSync, realpathSync } from "fs"; import { basename, dirname, join, resolve } from "path"; import { promisify } from "util"; import { allowFileRoot } from "./allowed-roots"; +import { samePath, toNativePath } from "./paths"; const execFileAsync = promisify(execFile); @@ -90,7 +91,10 @@ export async function resolveProject(cwd: string): Promise { "--git-common-dir", "--git-dir", "--show-toplevel", "--abbrev-ref", "HEAD", ]); - const [commonDir, gitDir, toplevel, ref] = out.split("\n").map((l) => l.trim()); + const [commonDirRaw, gitDirRaw, toplevelRaw, ref] = out.split("\n").map((l) => l.trim()); + // Only the first three lines are paths — `ref` is a branch name and must + // keep its forward slashes (`feature/foo`). + const [commonDir, gitDir, toplevel] = [commonDirRaw, gitDirRaw, toplevelRaw].map(toNativePath); // git prints resolved (symlink-free) paths; normalize cwd the same way let realCwd = cwd; try { realCwd = realpathSync(cwd); } catch { /* keep as-is */ } @@ -99,8 +103,8 @@ export async function resolveProject(cwd: string): Promise { // cwd is a subdirectory of a repo keeps its own project identity — // grouping subdirs under the repo root would change where new sessions // are created for existing users. - const isTopLevel = toplevel === realCwd; - const isWorktreeTopLevel = gitDir !== commonDir && isTopLevel; + const isTopLevel = samePath(toplevel, realCwd); + const isWorktreeTopLevel = !samePath(gitDir, commonDir) && isTopLevel; info = { projectRoot: isWorktreeTopLevel ? dirname(commonDir) : cwd, branch: ref && ref !== "HEAD" ? ref : null, @@ -126,7 +130,7 @@ export async function resolveProject(cwd: string): Promise { /** Main repo root (parent of the shared .git dir), or throws for non-git dirs */ async function getRepoRoot(cwd: string): Promise { const commonDir = await git(cwd, ["rev-parse", "--path-format=absolute", "--git-common-dir"]); - return dirname(commonDir); + return dirname(toNativePath(commonDir)); } export async function listWorktrees(cwd: string): Promise { @@ -153,7 +157,7 @@ export async function listWorktrees(cwd: string): Promise { for (const line of out.split("\n")) { if (line.startsWith("worktree ")) { flush(); - current = { path: line.slice("worktree ".length).trim() }; + current = { path: toNativePath(line.slice("worktree ".length).trim()) }; } else if (line.startsWith("branch ") && current) { current.branch = line.slice("branch ".length).trim().replace(/^refs\/heads\//, ""); } else if (line.startsWith("prunable") && current) { @@ -211,7 +215,7 @@ export async function addWorktree(cwd: string, branch: string): Promise<{ path: export async function removeWorktree(cwd: string, worktreePath: string, force = false): Promise { const worktrees = await listWorktrees(cwd); - const target = worktrees.find((w) => w.path === worktreePath); + const target = worktrees.find((w) => samePath(w.path, worktreePath)); if (!target) throw new Error(`Not a worktree of this repository: ${worktreePath}`); if (target.isMain) throw new Error("Cannot remove the main worktree");