diff --git a/src/node/services/artifactRuntimeStore.test.ts b/src/node/services/artifactRuntimeStore.test.ts index 80c16effba2..c99841c1238 100644 --- a/src/node/services/artifactRuntimeStore.test.ts +++ b/src/node/services/artifactRuntimeStore.test.ts @@ -11,6 +11,7 @@ import { parseArtifactListOutput, readArtifactBytesOnRuntime, readArtifactOnRuntime, + resolveRealDirOnRuntime, writeArtifactOnRuntime, } from "./artifactRuntimeStore"; import { @@ -269,6 +270,31 @@ describe("artifactRuntimeStore", () => { }); }); + test("a pinned read can use the filesystem root as its folder", async () => { + // A pinned file directly under `/` is read from `/`: containment must not require `//`. + const checkout = path.join(tempDir, "checkout"); + await fs.mkdir(checkout); + await fs.writeFile(path.join(checkout, "a.txt"), "rooted"); + const options = { allowHidden: true, requireArtifactsBasename: false }; + const relPath = path.join(checkout, "a.txt").slice(1); + expect( + await readArtifactBytesOnRuntime(runtime, "/", relPath, CAP, undefined, options) + ).toMatchObject({ status: "ok", bytes: Buffer.from("rooted") }); + expect(await readArtifactFromDir("/", relPath, CAP, options)).toMatchObject({ + success: true, + data: { status: "ok", content: "rooted" }, + }); + }); + + test("resolves a folder's real path on the runtime", async () => { + const real = path.join(tempDir, "real dir"); + await fs.mkdir(real); + await fs.symlink(real, path.join(tempDir, "link")); + expect(await resolveRealDirOnRuntime(runtime, path.join(tempDir, "link"))).toBe(real); + expect(await resolveRealDirOnRuntime(runtime, real)).toBe(real); + expect(await resolveRealDirOnRuntime(runtime, path.join(tempDir, "missing"))).toBeNull(); + }); + test("reads a pinned checkout file only when the artifacts name check is off", async () => { const checkout = path.join(tempDir, "checkout"); await fs.mkdir(path.join(checkout, ".config"), { recursive: true }); diff --git a/src/node/services/artifactRuntimeStore.ts b/src/node/services/artifactRuntimeStore.ts index 3908827c667..31d5fa165a6 100644 --- a/src/node/services/artifactRuntimeStore.ts +++ b/src/node/services/artifactRuntimeStore.ts @@ -31,6 +31,7 @@ import { */ const LIST_MAGIC = "XUMARTIFACTS1"; +const REAL_DIR_MAGIC = "XUMREALDIR1"; const READ_MAGIC = "XUMREAD1"; const WRITE_MAGIC = "XUMWRITE1"; const EXEC_TIMEOUT_SECONDS = 30; @@ -152,6 +153,38 @@ export function parseArtifactListOutput(stdout: string): RuntimeArtifactListing } } +/** + * Real path of a folder on the runtime (symlinks resolved), or null when it is missing or not a + * folder. Pins outside the checkout store it: the read script refuses a symlinked root folder. + */ +export async function resolveRealDirOnRuntime( + runtime: Runtime, + dir: string, + abortSignal?: AbortSignal +): Promise { + const result = await execBuffered( + runtime, + `cd -P -- "$XUM_DIR" 2>/dev/null || exit 0\nprintf '${REAL_DIR_MAGIC}\\0'\npwd -P\n`, + { + cwd: "/", + pathEnv: { XUM_DIR: dir }, + timeout: EXEC_TIMEOUT_SECONDS, + abortSignal, + maxOutputBytes: READ_OUTPUT_SLACK_BYTES, + } + ); + if (result.exitCode !== 0) { + throw new Error( + `Resolving a folder failed (exit ${result.exitCode}): ${result.stderr.trim().slice(0, 500)}` + ); + } + // A login banner can come first; pwd ends its line with one newline. + const start = result.stdout.indexOf(`${REAL_DIR_MAGIC}\0`); + if (start < 0) return null; + const real = result.stdout.slice(start + REAL_DIR_MAGIC.length + 1).replace(/\n$/, ""); + return real.startsWith("/") ? real : null; +} + export async function listArtifactsOnRuntime( runtime: Runtime, artifactsDir: string, @@ -198,21 +231,29 @@ d=$XUM_ARTIFACTS_DIR if [ -L "$d" ] || [ ! -d "$d" ]; then xum_missing; fi cd -- "$d" 2>/dev/null || xum_missing xum_root=$(pwd -P) || xum_missing -xum_parent=$(cd -P -- "$` + +xum_up=$` + "{d%/*}" + + String.raw` +xum_parent=$(cd -P -- "$` + + "{xum_up:-/}" + String.raw`" 2>/dev/null && pwd -P) || xum_missing -if [ "$xum_root" != "$xum_parent/${rootName}" ]; then xum_missing; fi +if [ "$xum_root" != "$` + + "{xum_parent%/}" + + String.raw`/${rootName}" ]; then xum_missing; fi +xum_pre=$` + + "{xum_root%/}" + + String.raw`/ for xum_seg in ${dirSegments.join(" ")}; do if [ -L "$xum_seg" ] || [ ! -d "$xum_seg" ]; then xum_missing; fi cd -- "$xum_seg" 2>/dev/null || xum_missing done -case $(pwd -P) in "$xum_root"/*|"$xum_root") ;; *) xum_missing ;; esac +case $(pwd -P) in "$xum_pre"*|"$xum_root") ;; *) xum_missing ;; esac xum_leaf=${leaf} if [ -L "$xum_leaf" ] || [ ! -f "$xum_leaf" ]; then xum_missing; fi ${STAT_PRELUDE} xum_send() { if xum_fd=$(readlink "/proc/$$/fd/3" 2>/dev/null); then - case $xum_fd in "$xum_root"/*) ;; *) xum_missing ;; esac + case $xum_fd in "$xum_pre"*) ;; *) xum_missing ;; esac fi if [ -L "$xum_leaf" ]; then xum_missing; fi xum_m=$(xum_meta "./$xum_leaf") || xum_missing diff --git a/src/node/services/artifactStore.ts b/src/node/services/artifactStore.ts index 2d93117b2c6..eff5549466d 100644 --- a/src/node/services/artifactStore.ts +++ b/src/node/services/artifactStore.ts @@ -78,6 +78,11 @@ export function getArtifactsDir(scratchDir: string): string { return path.join(scratchDir, ARTIFACTS_DIR_NAME); } +/** `dir` plus one separator: a pinned file's folder can be the filesystem root (`/`). */ +function dirPrefix(dir: string): string { + return dir.endsWith(path.sep) ? dir : dir + path.sep; +} + function isMissing(error: unknown): boolean { const code = (error as NodeJS.ErrnoException | undefined)?.code; return code === "ENOENT" || code === "ENOTDIR"; @@ -365,7 +370,7 @@ export async function readArtifactBytesFromDir( throw error; } // Symlinked parent folders could still point elsewhere: confine by real path. - if (!realTarget.startsWith(realDir + path.sep)) return notFound; + if (!realTarget.startsWith(dirPrefix(realDir))) return notFound; let handle: fs.FileHandle; try { @@ -610,7 +615,7 @@ async function verifyOpenedInsideDir( fdPath = undefined; } if (fdPath !== undefined) { - return fdPath === realTarget && fdPath.startsWith(realDir + path.sep); + return fdPath === realTarget && fdPath.startsWith(dirPrefix(realDir)); } if (requireDescriptorPath) return false; try { diff --git a/src/node/services/pinnedArtifactFiles.test.ts b/src/node/services/pinnedArtifactFiles.test.ts index 275825743e9..069b9ab1479 100644 --- a/src/node/services/pinnedArtifactFiles.test.ts +++ b/src/node/services/pinnedArtifactFiles.test.ts @@ -2,6 +2,8 @@ import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun: import * as fs from "fs/promises"; import * as os from "os"; import * as path from "path"; +import { LocalRuntime } from "@/node/runtime/LocalRuntime"; +import * as runtimeHelpers from "@/node/runtime/runtimeHelpers"; import { acquireCrossProcessLock } from "@/node/utils/main/crossProcessLock"; import * as artifactStoreModule from "./artifactStore"; import type { ArtifactsContext } from "./artifactsOperations"; @@ -24,6 +26,8 @@ describe("pinned workspace files", () => { let projects: Array<{ projectPath: string; projectName: string }> | undefined; let subProjectPath: string | undefined; let runtimeConfig: Record; + let workspaceName: string; + let projectPath: string; beforeEach(async () => { tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "pinned-files-")); @@ -34,6 +38,8 @@ describe("pinned workspace files", () => { projects = undefined; subProjectPath = undefined; runtimeConfig = { type: "worktree", srcBaseDir: tempDir }; + workspaceName = "checkout"; + projectPath = tempDir; context = { config: { sessionsDir: path.join(tempDir, "sessions") }, workspaceService: { @@ -42,8 +48,8 @@ describe("pinned workspace files", () => { workspaceId === "ws" ? { id: "ws", - name: "checkout", - projectPath: tempDir, + name: workspaceName, + projectPath, projectName: "project", namedWorkspacePath: checkout, runtimeConfig, @@ -100,7 +106,7 @@ describe("pinned workspace files", () => { test("a file outside the checkout pins by its absolute path and reads live", async () => { // File tool cards offer "Open as artifact" for any path the agent read, such as a plan or a // skill file under the home folder. - const outside = path.join(tempDir, "notes", "plan.md"); + const outside = path.join(await fs.realpath(tempDir), "notes", "plan.md"); await fs.mkdir(path.dirname(outside), { recursive: true }); await fs.writeFile(outside, "# Plan"); expect(await pinFile(context, { workspaceId: "ws", path: outside })).toEqual({ @@ -135,6 +141,94 @@ describe("pinned workspace files", () => { }); }); + test("one file reached through a symlinked folder is one pin", async () => { + const real = await fs.realpath(tempDir); + await fs.mkdir(path.join(real, "notes")); + await fs.writeFile(path.join(real, "notes", "plan.md"), "# Plan"); + await fs.symlink(path.join(real, "notes"), path.join(real, "notes-link")); + await fs.mkdir(path.join(checkout, "docs")); + await fs.writeFile(path.join(checkout, "docs", "x.md"), "x"); + await fs.symlink(path.join(checkout, "docs"), path.join(real, "docs-link")); + + // An alias of an outside folder stores the real path, so both spellings are one pin. + for (const alias of ["notes", "notes-link"]) { + expect( + await pinFile(context, { workspaceId: "ws", path: path.join(real, alias, "plan.md") }) + ).toEqual({ success: true, data: { path: path.join(real, "notes", "plan.md") } }); + } + // An alias that leads into the checkout is a checkout pin. + expect( + await pinFile(context, { workspaceId: "ws", path: path.join(real, "docs-link", "x.md") }) + ).toEqual({ success: true, data: { path: "docs/x.md" } }); + const listed = await listPinnedFiles(context, { workspaceId: "ws" }); + expect(listed.success && listed.data.available && listed.data.files.map((f) => f.path)).toEqual( + [path.join(real, "notes", "plan.md"), "docs/x.md"] + ); + }); + + test("an SSH checkout pins and reads an outside file in a symlinked folder", async () => { + // A LocalRuntime stands in for the SSH host: pins and reads run the real runtime scripts. + const real = await fs.realpath(tempDir); + runtimeConfig = { type: "ssh", host: "example", srcBaseDir: real }; + // An in-place workspace (name equals project path) roots the checkout at the project path. + projectPath = workspaceName = path.join(real, "checkout"); + const create = spyOn(runtimeHelpers, "createRuntimeForWorkspace").mockReturnValue( + new LocalRuntime(real) + ); + try { + await fs.mkdir(path.join(real, "notes")); + await fs.writeFile(path.join(real, "notes", "plan.md"), "# Plan"); + await fs.symlink(path.join(real, "notes"), path.join(real, "notes-link")); + const stored = path.join(real, "notes", "plan.md"); + expect( + await pinFile(context, { + workspaceId: "ws", + path: path.join(real, "notes-link", "plan.md"), + }) + ).toEqual({ success: true, data: { path: stored } }); + expect(await readPinnedFile(context, { workspaceId: "ws", path: stored })).toMatchObject({ + success: true, + data: { status: "ok", content: "# Plan" }, + }); + expect(create).toHaveBeenCalled(); + } finally { + create.mockRestore(); + } + }); + + test("re-pinning an alias stored before real folder paths keeps the one entry", async () => { + const real = await fs.realpath(tempDir); + await fs.mkdir(path.join(real, "notes")); + await fs.writeFile(path.join(real, "notes", "plan.md"), "# Plan"); + await fs.symlink(path.join(real, "notes"), path.join(real, "notes-link")); + const alias = path.join(real, "notes-link", "plan.md"); + // An older build stored the alias as given. + await addPinnedPath(path.join(tempDir, "sessions", "ws"), alias); + for (const request of [alias, path.join(real, "notes", "plan.md")]) { + expect(await pinFile(context, { workspaceId: "ws", path: request })).toEqual({ + success: true, + data: { path: alias }, + }); + } + const listed = await listPinnedFiles(context, { workspaceId: "ws" }); + expect(listed.success && listed.data.available && listed.data.files.map((f) => f.path)).toEqual( + [alias] + ); + }); + + test("a drive-letter entry on a POSIX host is invalid, not a path below the backend's folder", async () => { + if (process.platform === "win32") return; + // Only a hand edit of the store can add one. Read as a folder `C:` it would resolve against + // the backend's working folder. + const sessionDir = path.join(tempDir, "sessions", "ws"); + await addPinnedPath(sessionDir, "C:/x.md"); + const read = await readPinnedFile(context, { workspaceId: "ws", path: "C:/x.md" }); + expect(read.success).toBe(false); + expect(read).not.toHaveProperty("missing"); + const listed = await listPinnedFiles(context, { workspaceId: "ws" }); + expect(listed.success && listed.data.available && listed.data.files).toEqual([]); + }); + test("refuses outside files that are missing, folders or symlinks", async () => { const notes = path.join(tempDir, "notes"); await fs.mkdir(notes, { recursive: true }); diff --git a/src/node/services/pinnedArtifactFiles.ts b/src/node/services/pinnedArtifactFiles.ts index 7696f89fc88..abefcb14945 100644 --- a/src/node/services/pinnedArtifactFiles.ts +++ b/src/node/services/pinnedArtifactFiles.ts @@ -17,7 +17,7 @@ import { MutexMap } from "@/node/utils/concurrency/mutexMap"; import { nodeSessionLocks } from "@/node/services/sessionLocks/nodeSessionLocks"; import type { SessionLocks } from "@/node/services/sessionLocks/sessionLocks"; import writeFileAtomic from "@/node/utils/writeFileAtomic"; -import { readArtifactBytesOnRuntime } from "./artifactRuntimeStore"; +import { readArtifactBytesOnRuntime, resolveRealDirOnRuntime } from "./artifactRuntimeStore"; import { hostSupportsDescriptorPaths, parseArtifactRelativePath, @@ -215,6 +215,17 @@ export async function removePinnedPath( type AvailableLocation = Exclude; +/** Absolute by the location's own rules: a drive-letter path is not absolute on a POSIX host. */ +function isAbsoluteOn(location: AvailableLocation, filePath: string): boolean { + return location.kind === "runtime" ? filePath.startsWith("/") : path.isAbsolute(filePath); +} + +/** Real path of a folder where the checkout lives, or null when it is missing. */ +async function realDirOn(location: AvailableLocation, dir: string): Promise { + if (location.kind === "runtime") return resolveRealDirOnRuntime(location.runtime, dir); + return fs.realpath(dir).catch(() => null); +} + /** * The folder a stored pin is read from and its path below it: the checkout for a checkout pin, * the file's own folder for a pin outside the checkout (so the readers still refuse a symlinked @@ -224,32 +235,50 @@ function pinTarget( location: AvailableLocation, storedPath: string ): { root: string; relPath: string } { - if (!isOutsidePinPath(storedPath)) return { root: location.root, relPath: storedPath }; + // A hand-edited entry that is not absolute here stays relative, and the readers refuse it. + if (!isAbsoluteOn(location, storedPath)) return { root: location.root, relPath: storedPath }; const paths = location.kind === "runtime" ? path.posix : path; return { root: paths.dirname(storedPath), relPath: paths.basename(storedPath) }; } /** - * Map a pin request to the stored path: checkout-relative inside the checkout, else the - * normalized absolute path. Dev containers refuse outside files: tool paths there name files in - * the container, while pins are read on this host. + * Map a pin request to the stored path: checkout-relative inside the checkout, else the file + * name in its folder's real path. The real path makes one file reached through a symlinked + * folder one pin (and a checkout pin when the folder is in the checkout), and the runtime + * readers refuse a symlinked folder. Dev containers refuse outside files: tool paths there name + * files in the container, while pins are read on this host. */ -function toStoredPinPath( +async function toStoredPinPath( location: AvailableLocation, inputPath: string -): string | { error: string } { - // By its own rules: a drive-letter path is not absolute on a POSIX host. - const isAbsolute = - location.kind === "runtime" ? inputPath.startsWith("/") : path.isAbsolute(inputPath); - if (!isAbsolute) return toPinnedRelativePath(location.root, inputPath); +): Promise { + if (!isAbsoluteOn(location, inputPath)) return toPinnedRelativePath(location.root, inputPath); + const paths = location.kind === "runtime" ? path.posix : path; const absolute = - location.kind === "runtime" ? path.posix.normalize(inputPath) : path.resolve(inputPath); + location.kind === "runtime" ? paths.normalize(inputPath) : paths.resolve(inputPath); const relPath = toPinnedRelativePath(location.root, absolute); if (typeof relPath === "string" || relPath.error !== OUTSIDE_CHECKOUT_ERROR) return relPath; if (location.kind === "host" && location.containerWritable === true) { return { error: PINNED_FILES_DEVCONTAINER_OUTSIDE_REASON }; } - return absolute; + return await toRealOutsidePinPath(location, absolute); +} + +/** + * An outside file's stored path: its name in its folder's real path, or the checkout pin when + * that real folder is in the checkout. + */ +async function toRealOutsidePinPath( + location: AvailableLocation, + absolute: string +): Promise { + const paths = location.kind === "runtime" ? path.posix : path; + const realDir = await realDirOn(location, paths.dirname(absolute)); + if (realDir == null) return { error: `Not a readable file: ${absolute}` }; + const real = paths.join(realDir, paths.basename(absolute)); + const realRoot = await realDirOn(location, location.root); + const inCheckout = realRoot == null ? null : toPinnedRelativePath(realRoot, real); + return typeof inCheckout === "string" ? inCheckout : real; } async function statPinned( @@ -294,7 +323,13 @@ export async function listPinnedFiles( if (location.kind === "unavailable") { return { success: true, data: { available: false, reason: location.reason } }; } - const paths = await readPinnedPaths(sessionDirFor(context, input.workspaceId)); + // The store keeps any absolute entry; list only those absolute here (a hand-edited `C:/x.md` + // on a POSIX host would list as a pin that never reads). + const paths = (await readPinnedPaths(sessionDirFor(context, input.workspaceId))).filter( + (p) => + isAbsoluteOn(location, p) || + typeof parseArtifactRelativePath(p, { allowHidden: true }) !== "string" + ); const files = await Promise.all( paths.map(async (relPath) => { const stat = await statPinned(location, relPath); @@ -375,6 +410,25 @@ async function checkPinnable( } } +/** + * The stored entry that already names the file of `storedPath`, or null. An outside pin stored + * before pins used real folder paths can hold an alias of the same file, so absolute entries + * are compared by their real form too: re-pinning the alias must not add a second entry. + */ +async function findExistingPin( + location: AvailableLocation, + pinned: readonly string[], + storedPath: string +): Promise { + if (pinned.includes(storedPath)) return storedPath; + const absolute = pinned.filter((p) => isAbsoluteOn(location, p)); + const real = await Promise.all( + absolute.map((p) => toStoredPinPath(location, p).catch(() => null)) + ); + const index = real.indexOf(storedPath); + return index < 0 ? null : absolute[index]; +} + export async function pinFile( context: ArtifactsContext, input: { workspaceId: string; path: string; relativeTo?: "tool-cwd" | "checkout" | null } @@ -383,23 +437,29 @@ export async function pinFile( const location = await resolveForPinned(context, input.workspaceId); if (!location) return { success: false, error: `Workspace not found: ${input.workspaceId}` }; if (location.kind === "unavailable") return { success: false, error: location.reason }; - const relPath = toStoredPinPath( - location, - resolvePinInput(location, input.path, input.relativeTo) - ); + let relPath: string | { error: string }; + try { + relPath = await toStoredPinPath( + location, + resolvePinInput(location, input.path, input.relativeTo) + ); + } catch (error) { + return unreachableError(error); + } if (typeof relPath !== "string") return { success: false, error: relPath.error }; const sessionDir = sessionDirFor(context, input.workspaceId); + // Re-pinning an existing pin stays a no-op and opens the stored entry. + const existing = await findExistingPin(location, await readPinnedPaths(sessionDir), relPath); + if (existing != null) return { success: true, data: { path: existing } }; // Pins persist until unpinned: refuse missing files, folders and symlinks up front instead of - // storing a pin that can never be read. Re-pinning an existing pin stays a no-op. - if (!(await readPinnedPaths(sessionDir)).includes(relPath)) { - let refusal: string | null; - try { - refusal = await checkPinnable(location, relPath); - } catch (error) { - return unreachableError(error); - } - if (refusal != null) return { success: false, error: refusal }; + // storing a pin that can never be read. + let refusal: string | null; + try { + refusal = await checkPinnable(location, relPath); + } catch (error) { + return unreachableError(error); } + if (refusal != null) return { success: false, error: refusal }; await addPinnedPath(sessionDir, relPath); return { success: true, data: { path: relPath } }; }