diff --git a/docs/extensions.md b/docs/extensions.md index ea81a6719..21894229f 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -396,11 +396,24 @@ Handlers run in registration order; the first one to resolve `true` counts as th bytes being durably on disk, and the native tool continues exactly as if its own write had succeeded — including recording its file snapshot under the real destination path, so a later hashline `edit` on that path keeps working. A -throwing handler is logged and skipped in favor of the next one; if every handler +throwing handler is logged and skipped in favor of the next one — per handler, so a +later handler registered by the same extension still runs; if every handler returns `false` (or none are registered), the original error is rethrown unchanged. Intended for a host that embeds the agent inside a sandbox denying direct filesystem writes but exposing a privileged write channel. +`req.dst` is the **symlink-resolved** destination, not the path the tool was given. +The kernel follows every component above the last, so `ws/link/file` under a +`ws/link -> /elsewhere` link lands outside `ws` while still looking in-workspace, and +a prefix allowlist in your handler would pass on that innocent-looking path. For a +write the final component is followed too, so it is resolved as well; for a delete it +is not, because `unlink` removes a link rather than what it points at (so a delete +`req.dst` may itself name a link). Treat `req.dst` as authoritative and do not +re-derive the target from anything else. When the real destination cannot be +established — a dangling final link, or an ancestor this process may not resolve — no +handler is consulted at all and the original error is rethrown, because there is no +destination to hand a privileged writer. + Two details matter when the destination is outside what the host allows: - **A missing parent directory.** `Bun.write` creates missing parents itself, and @@ -455,8 +468,10 @@ succeed, and nothing happens at all when no handler is registered. Two differenc the target's own metadata sits behind the same boundary that denied the unlink — the common sandbox case — that check cannot be resolved, and `req.dst` may then be a directory. `req.confirmedFile` is `true` only when the seam positively established - the target is not one. A privileged helper that recursively removes `req.dst` would - delete a whole tree on behalf of a tool that only ever removes one file. + the target is a plain regular file; a symlink reports `false` too, since unlinking a + link is fine but resolving it acts on something else entirely. A privileged helper + that recursively removes `req.dst`, or realpaths it first, would act far outside + what a tool that only ever removes one file asked for. **Registering for deletes is deliberately separate from registering for writes.** A write handler brokers `req.content` to `req.dst`; if a delete request reached it, the @@ -470,9 +485,16 @@ Two lifecycle constraints, which apply to both seams: an extension that registered nothing by then is skipped entirely, so a first registration made later never takes effect. - **The registries are process-wide.** A process can host several sessions (a subagent - gets its own runner), and `req` carries no session identity, so a handler may be - consulted for a denied write or delete from any session in the process — not only - the one whose extension registered it. Handlers are removed on `session_shutdown`. + gets its own runner), so a handler may be consulted for a denied write or delete + from any session in the process — not only the one whose extension registered it. + This is deliberate: a subagent spawned with restricted tools loads no extensions of + its own, and a host that registers once in its top-level session still expects its + subagents' writes brokered. `req.sessionId` names the session that issued the + mutation (`undefined` when it did not come from a tool call), and + `ctx.sessionManager.getSessionId()` names the handler's own — compare them to make + the decision per session. It matters most before prompting: `ctx.ui` belongs to the + handler's session, not necessarily to the one being asked about. Handlers are + removed on `session_shutdown`. With nothing registered none of this engages: the primitive runs exactly as it did before and performs no extra syscalls. diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index af449a982..977ce179d 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- Added `ExtensionAPI.registerFileWriteFallback(handler)` and `ExtensionAPI.registerFileDeleteFallback(handler)`, letting an extension supply a fallback writer or deleter that is consulted when a native `write`, `edit`, or `apply_patch` byte-write or unlink is denied with a permission error (`EPERM`/`EACCES`/`EROFS`) — for hosts that embed the agent inside a sandbox that denies direct filesystem access but exposes a privileged channel. The brokered path is symlink-resolved so a handler's allowlist sees the real destination, a destination that cannot be resolved is not brokered at all, and `req.sessionId` names the session that issued the mutation so a handler sharing the process-wide registry can enforce policy per session. See [`docs/extensions.md`](../../docs/extensions.md). + ## [17.3.4] - 2026-08-14 ### Changed @@ -84,9 +88,6 @@ - Fixed retry-fallback selection switching to a fallback model with a context window too small to hold the current session context. - Fixed OpenCode discovery ignoring `opencode.jsonc` files and rejecting comments in `opencode.json`. - Fixed WSL2 startup hanging forever when the Windows interop pipe is wedged: the WSL host-home discovery probes (`cmd.exe`, `wslpath`) now run under a 500ms hard timeout and fall back to the Linux `$HOME`/`~/.omp` candidates ([#8402](https://github.com/can1357/oh-my-pi/issues/8402)). -### Added - -- Added `ExtensionAPI.registerFileWriteFallback(handler)` and `ExtensionAPI.registerFileDeleteFallback(handler)`, letting an extension supply a fallback writer or deleter that is consulted when a native `write`, `edit`, or `apply_patch` byte-write or unlink is denied with a permission error (`EPERM`/`EACCES`/`EROFS`) — for hosts that embed the agent inside a sandbox that denies direct filesystem access but exposes a privileged channel. See [`docs/extensions.md`](../../docs/extensions.md). ## [17.2.15] - 2026-08-12 diff --git a/packages/coding-agent/src/extensibility/extensions/runner.ts b/packages/coding-agent/src/extensibility/extensions/runner.ts index 8657bfdff..d91defb31 100644 --- a/packages/coding-agent/src/extensibility/extensions/runner.ts +++ b/packages/coding-agent/src/extensibility/extensions/runner.ts @@ -543,6 +543,18 @@ export class ExtensionRunner { return this.sessionManager.getCwd(); } + /** + * Stable id of the session this runner serves. Read through `sessionManager` + * for the same reason as {@link cwd}: it is this session's own, never a + * process-global, so a subagent runner reports itself and not its parent. + * + * Used to attribute a denied file write or delete to the session that issued + * it, since the fallback registry those handlers live in is process-wide. + */ + get sessionId(): string { + return this.sessionManager.getSessionId(); + } + initialize( actions: ExtensionActions, contextActions: ExtensionContextActions, @@ -616,11 +628,23 @@ export class ExtensionRunner { // and `createContext()` takes no extension argument so a single context per // extension is all any of its handlers would have received anyway. const ctx = this.createContext(); + // Isolation is per HANDLER, not per extension. The registry only sees one + // trampoline per extension, so a throw escaping this loop would advance the + // registry to the NEXT extension and skip every later handler this one + // registered — breaking both the documented "a throwing handler is skipped" + // contract and registration order for a backup-handler setup. if (ext.fileWriteFallbackHandlers.length > 0) { this.#fileFallbackDisposers.push( addFileWriteFallback(async req => { for (const handler of ext.fileWriteFallbackHandlers) { - if (await handler(req, ctx)) return true; + try { + if (await handler(req, ctx)) return true; + } catch (error) { + logger.warn("Extension file write fallback handler threw; trying next handler", { + extension: ext.path, + error: error instanceof Error ? error.message : String(error), + }); + } } return false; }), @@ -630,7 +654,14 @@ export class ExtensionRunner { this.#fileFallbackDisposers.push( addFileDeleteFallback(async req => { for (const handler of ext.fileDeleteFallbackHandlers) { - if (await handler(req, ctx)) return true; + try { + if (await handler(req, ctx)) return true; + } catch (error) { + logger.warn("Extension file delete fallback handler threw; trying next handler", { + extension: ext.path, + error: error instanceof Error ? error.message : String(error), + }); + } } return false; }), diff --git a/packages/coding-agent/src/extensibility/extensions/types.ts b/packages/coding-agent/src/extensibility/extensions/types.ts index 64fef3829..0c9ddd028 100644 --- a/packages/coding-agent/src/extensibility/extensions/types.ts +++ b/packages/coding-agent/src/extensibility/extensions/types.ts @@ -1269,12 +1269,21 @@ export interface ExtensionAPI { * may not create — also diverts here, with `req.dst`'s parent absent and the * handler responsible for creating it. * + * `req.dst` is symlink-RESOLVED: the path the failed write itself acted on, not + * the one the tool was given. A link anywhere in a lexical path redirects the + * bytes while still passing a prefix allowlist, so treat `req.dst` as + * authoritative. A destination that cannot be resolved is never brokered. + * * Call this during extension load, like the other `register*` methods: handlers * are installed when the runner initializes, so an extension that has registered * none by then is skipped and a first registration made later never takes effect. - * The underlying registry is process-wide and `req` carries no session identity, - * so a handler may be consulted for a denied write from any session in the - * process, not only its own. See `docs/extensions.md`. + * + * The underlying registry is process-wide, so a handler may be consulted for a + * denied write from any session in the process, not only its own. + * `req.sessionId` names the session that issued the write and + * `ctx.sessionManager.getSessionId()` names the handler's own; compare them + * before prompting, because `ctx.ui` belongs to the latter. See + * `docs/extensions.md`. */ registerFileWriteFallback(handler: FileWriteFallbackHandler): void; @@ -1291,6 +1300,10 @@ export interface ExtensionAPI { * check cannot be resolved and `dst` may be a directory. `req.confirmedFile` says * which situation the handler is in. * + * `req.dst` resolves every component ABOVE the last, for the same reason the + * write seam resolves all of them; the last is left alone because `unlink` + * removes a link rather than its target, so `req.dst` may name a link. + * * Separate from {@link registerFileWriteFallback} on purpose. A write handler * brokers `req.content` to `req.dst`, so a delete request reaching it with no * content invites brokering an empty write and truncating the file instead of diff --git a/packages/coding-agent/src/extensibility/extensions/wrapper.ts b/packages/coding-agent/src/extensibility/extensions/wrapper.ts index 335a7abcd..39bbeb517 100644 --- a/packages/coding-agent/src/extensibility/extensions/wrapper.ts +++ b/packages/coding-agent/src/extensibility/extensions/wrapper.ts @@ -14,6 +14,7 @@ import type { Settings } from "../../config/settings"; import type { Theme } from "../../modes/theme/theme"; import { type ApprovalMode, formatApprovalPrompt, resolveApproval, truncateForPrompt } from "../../tools/approval"; import { defaultLoadModeForToolName } from "../../tools/essential-tools"; +import { withFileMutationSession } from "../../tools/file-write-fallback"; import { normalizeToolEventInput, resolveToolEventInput } from "../tool-event-input"; import { applyToolProxy } from "../tool-proxy"; import type { ExtensionRunner } from "./runner"; @@ -343,7 +344,15 @@ export class ExtensionToolWrapper + this.tool.execute(toolCallId, effectiveParams, signal, onUpdate, context), + ); } catch (err) { executionError = err instanceof Error ? err : new Error(String(err)); result = { diff --git a/packages/coding-agent/src/tools/file-write-fallback.ts b/packages/coding-agent/src/tools/file-write-fallback.ts index eafb61e6f..18947fa02 100644 --- a/packages/coding-agent/src/tools/file-write-fallback.ts +++ b/packages/coding-agent/src/tools/file-write-fallback.ts @@ -71,26 +71,81 @@ * it did before, a failure rethrows from the same place, and no extra syscalls * are performed. * + * ## The path a handler is given + * + * A handler is more privileged than the syscall that just failed, so it is never + * handed the lexical path the tool used. A lexical path is not a destination: the + * kernel follows every component above the last, so `ws/link/file` under a + * `ws/link -> /elsewhere` link lands outside `ws` while still looking + * in-workspace. That defeats the defence a helper author reaches for first, since + * a prefix allowlist passes on the link's own path — and for writes the final + * component is followed too, so a plain `ws/link` is enough. + * + * `req.dst` is therefore resolved through {@link resolveSyscallTarget} to the path + * the failed syscall itself acted on: fully for a write, and up to the last + * component for a delete, since `unlink` removes a link rather than following it. + * Resolving rather than refusing also closes the TOCTOU window, because the + * handler no longer traverses a link the agent could re-point after the check. + * + * A path that cannot be canonicalized — a dangling final link, or an ancestor + * whose own resolution is denied — is not brokered at all. "Where would this + * land" has no answer there, and a privileged writer is the wrong place to guess. + * That narrows the seam for a sandbox that also hides the ancestors of a denied + * path, which is the honest cost of not handing over an unverifiable target. + * * ## Scope of the registry * * Handlers live in one process-wide list, and a process can host several sessions * (a subagent gets its own `ExtensionRunner`). A handler is therefore consulted - * for denied writes from ANY session in the process, not only the one whose - * extension registered it, and `FileWriteFallbackRequest` carries no session - * identity to distinguish them. A handler whose policy depends on which session - * asked must not assume its own; brokering the bytes is session-independent and - * is the intended use. + * for denied mutations from ANY session in the process, not only the one whose + * extension registered it. Filtering by session here would be wrong: a subagent + * spawned with `restrictToolNames` loads no extensions of its own, so scoping + * would leave its denied writes with nothing to broker them, and a host that + * registers once in its top-level session expects subagent writes covered. + * + * So the request names its origin instead, and the policy stays with the party + * that owns it. `req.sessionId` is the session that issued the mutation (see + * {@link withFileMutationSession}); a handler compares it with + * `ctx.sessionManager.getSessionId()` to decide. That matters most for a handler + * that prompts: `ctx.ui` belongs to the session whose extension registered the + * handler, which is not necessarily the session being asked about. + * + * Each list is iterated over a snapshot, because a concurrent session shutdown + * splices the live array and a `for` over it would skip whichever handler shifted + * into the hole. */ +import { AsyncLocalStorage } from "node:async_hooks"; import * as fs from "node:fs/promises"; import * as path from "node:path"; import { isEnoent, isFsError, logger } from "@oh-my-pi/pi-utils"; import type { BunFile } from "bun"; import type { ExtensionContext } from "../extensibility/extensions/types"; +import { resolveSyscallTarget } from "./path-utils"; /** A denied write, captured for a registered fallback to retry through a privileged channel. */ export interface FileWriteFallbackRequest { - /** Absolute destination path the write was denied for. */ + /** + * Absolute, symlink-resolved path to write the bytes to. + * + * This is where the failed in-process write would itself have landed, which is + * not necessarily the path the tool was given: `open` follows every component, + * so a link anywhere in that path redirects the bytes. Resolving it here is + * what lets a handler's allowlist see the real destination instead of a + * lexically innocent path, so a handler MUST treat this as authoritative and + * MUST NOT re-derive the target from anything else. + */ dst: string; + /** + * Session the denied write was issued from, or `undefined` when the mutation + * did not happen inside a tool call (an external `applyPatch` caller, a test). + * + * The registry is process-wide, so a handler can be consulted for a write from + * a session other than the one whose extension registered it. Compare this with + * `ctx.sessionManager.getSessionId()` to tell the two apart — a handler that + * prompts through `ctx.ui` needs to, since that UI belongs to ITS session and + * not necessarily to the one being asked about. + */ + sessionId: string | undefined; /** The exact bytes the tool intended to write. */ content: string; /** @@ -110,7 +165,14 @@ type BoundFileWriteFallbackHandler = (req: FileWriteFallbackRequest) => Promise< /** A denied unlink, captured for a registered fallback to perform through a privileged channel. */ export interface FileDeleteFallbackRequest { - /** Absolute path the unlink was denied for. */ + /** + * Absolute, symlink-resolved path the unlink was denied for. + * + * Every component ABOVE the last is resolved, so a handler cannot be walked + * outside its allowed roots through a link in the path. The last component is + * deliberately NOT resolved, because `unlink` removes a link itself rather + * than its target — which is also why this may still name a symlink. + */ dst: string; /** The `EPERM`/`EACCES`/`EROFS` that proves the unlink hit a permission boundary. */ cause: unknown; @@ -128,6 +190,8 @@ export interface FileDeleteFallbackRequest { * points at instead of the link. */ confirmedFile: boolean; + /** See {@link FileWriteFallbackRequest.sessionId}. */ + sessionId: string | undefined; } /** Extension-authored handler. Return `true` once `dst` is gone from disk. */ @@ -192,6 +256,34 @@ export function addFileDeleteFallback(handler: BoundFileDeleteFallbackHandler): }; } +const mutationSessionStorage = new AsyncLocalStorage(); + +/** + * Name the session whose tool call is about to run, so a denied mutation inside it + * can tell a handler where the request came from. + * + * Entered once per tool call by `ExtensionToolWrapper` (`extensibility/extensions/ + * wrapper.ts`), which `sdk.ts` puts around the whole tool registry whenever an + * `ExtensionRunner` exists — so the component that owns the handlers is the one + * naming its own session, and no caller has to thread an `AgentToolContext` + * through for attribution to work. + * + * That covers the deferred LSP write batch too: a batch id belongs to one + * assistant turn of one session, and its flush is awaited inside a tool call of + * that same session, so a write performed during a later call of the group is + * still attributed to the session that issued it. + * + * Deliberately NOT a general "current session" accessor: nothing else enters this + * scope, so outside a tool call it is empty by design — an external `applyPatch` + * caller reports `undefined` rather than borrowing someone else's identity. + */ +export function withFileMutationSession(sessionId: string | undefined, fn: () => T): T { + // With nothing registered no scope is entered, keeping the seam's inertness + // promise: a stock host pays one length check per tool call and no more. + if (sessionId === undefined || (fallbackHandlers.length === 0 && deleteFallbackHandlers.length === 0)) return fn(); + return mutationSessionStorage.run(sessionId, fn); +} + /** * Remove a file, consulting registered delete fallbacks when the unlink is denied. * @@ -208,6 +300,14 @@ export async function deleteFileWithFallback(dst: string, file?: BunFile): Promi } } catch (error) { if (deleteFallbackHandlers.length === 0 || !isPermissionDeniedError(error)) throw error; + // A handler is more privileged than the unlink that just failed, so it is told + // which path really gets removed, not the lexical one the tool used. `unlink` + // follows every component ABOVE the last, so `ws/link/victim` under a + // `ws/link -> /elsewhere` link removes a file outside `ws` while a helper's + // prefix allowlist still passes. The final component is deliberately left + // unresolved: `unlink` removes the link itself, never its target. + const target = await resolveSyscallTarget(dst, false); + if (target === null) throw error; // `unlink` on a directory reports EPERM on Darwin (EISDIR on Linux), which is // indistinguishable from a sandbox denial by code alone, so check the target // before diverting: asking a privileged deleter to remove a DIRECTORY on @@ -215,7 +315,7 @@ export async function deleteFileWithFallback(dst: string, file?: BunFile): Promi // intent. `lstat` rather than `stat`, so the link itself is judged — removing // a symlink is a legitimate file removal, and following it here would ask the // wrong question. - const stat = await fs.lstat(dst).catch((statError: unknown) => { + const stat = await fs.lstat(target).catch((statError: unknown) => { // A sandbox that denies the unlink usually denies the target's metadata // too, so a denied `lstat` is expected here and must still divert — it // just leaves the question unresolved, which `confirmedFile` reports. @@ -228,16 +328,23 @@ export async function deleteFileWithFallback(dst: string, file?: BunFile): Promi // realpaths `dst` for auditing, or removes it recursively, would act on the // link's target instead. Only a plain regular file is a confirmed file. const confirmedFile = stat?.isFile() ?? false; - for (const handler of deleteFallbackHandlers) { + // The process-wide registry can hand this to a handler from another session, + // so the request names the one that issued it. + const sessionId = mutationSessionStorage.getStore(); + // Snapshot: a concurrent session shutdown splices the live array, and + // iterating it directly would skip whichever handler shifted into the hole. + for (const handler of [...deleteFallbackHandlers]) { try { - if (await handler({ dst, cause: error, confirmedFile })) return; + if (await handler({ dst: target, cause: error, confirmedFile, sessionId })) return; } catch (handlerError) { logger.warn("File delete fallback handler threw; trying next handler", { - dst, + dst: target, error: handlerError instanceof Error ? handlerError.message : String(handlerError), }); } } + // Always the ORIGINAL error, never a handler's, so behaviour matches a host + // with no fallback registered. throw error; } } @@ -306,32 +413,29 @@ export async function writeFileWithFallback(dst: string, content: string, file?: : ({ kind: "rethrow" } as const); if (failure.kind === "retry") continue; if (failure.kind === "denied") { - // A denial reached through a SYMLINK is never brokered. The in-process - // write follows the link, so the kernel denied the link's TARGET — but a - // handler receives `dst`, and a privileged helper opening it with ordinary - // follow semantics lands these bytes wherever the link points. That also - // defeats the obvious helper-side defence: a prefix allowlist passes, - // because the LINK sits inside the allowed root while its target does not. - // omp cannot vouch for the destination, so it refuses rather than hand the - // ambiguity to a privileged writer — the same answer, for the same reason, - // that `confineToWorkspace` gives an unresolvable link - // (`tools/path-utils.ts`). - // - // An unreadable `lstat` cannot prove a link, and the dangerous shape (a - // link inside a directory the profile allows) always has readable - // metadata, so an undecidable check never blocks the case this seam - // exists for. - const viaSymlink = await fs.lstat(dst).then( - stat => stat.isSymbolicLink(), - () => false, - ); - if (!viaSymlink) { - for (const handler of fallbackHandlers) { + // A handler is more privileged than the write that just failed, so it is + // told where the bytes would REALLY have landed rather than the lexical + // path the tool used. `open` follows EVERY component, so `ws/link/file` + // under a `ws/link -> /elsewhere` link writes outside `ws` while still + // looking in-workspace — which defeats the defence a helper author + // reaches for first, since a prefix allowlist passes on the link's own + // path. Resolving closes that, and closes the TOCTOU window with it: the + // helper no longer traverses a link the agent could re-point after the + // check. A path that cannot be canonicalized is not brokered at all, + // because "where would this land" then has no answer to hand over. + const target = await resolveSyscallTarget(dst, true); + // Snapshot: a concurrent session shutdown splices the live array, and + // iterating it directly would skip whichever handler shifted into the hole. + if (target !== null) { + // The process-wide registry can hand this to a handler from another + // session, so the request names the one that issued it. + const sessionId = mutationSessionStorage.getStore(); + for (const handler of [...fallbackHandlers]) { try { - if (await handler({ dst, content, cause: failure.cause })) return; + if (await handler({ dst: target, content, cause: failure.cause, sessionId })) return; } catch (handlerError) { logger.warn("File write fallback handler threw; trying next handler", { - dst, + dst: target, error: handlerError instanceof Error ? handlerError.message : String(handlerError), }); } diff --git a/packages/coding-agent/src/tools/path-utils.ts b/packages/coding-agent/src/tools/path-utils.ts index 696f5799a..df153ee27 100644 --- a/packages/coding-agent/src/tools/path-utils.ts +++ b/packages/coding-agent/src/tools/path-utils.ts @@ -618,6 +618,85 @@ function isSymlink(target: string): boolean { } } +/** + * Resolve the path a syscall on `filePath` would really act on, or `null` when + * that cannot be established. + * + * A lexical path is not a destination. The kernel follows every component above + * the last, so `ws/link/file` under a `ws/link -> /elsewhere` link lands outside + * `ws` while still looking relative and `..`-free. Handing such a path to a + * privileged helper defeats the defence a helper author reaches for first — a + * prefix allowlist passes, because the link sits inside the allowed root while + * its target does not. Callers that hand a path to something more privileged + * than the syscall that just failed resolve it here first. + * + * Rejecting symlinked components outright is not an option: `/var` and `/tmp` + * are links on macOS, so every path under `os.tmpdir()` traverses one. They are + * resolved instead, and only a path whose real destination cannot be established + * is refused, because "where would this land" then has no answer to hand over. + * {@link confineToWorkspace} refuses an unresolvable link for the same reason. + * + * @param followFinal `true` for a syscall that follows a link at the final + * component (`open`, so every write), `false` for one that acts on the link + * itself (`unlink`) and therefore needs it left alone. + */ +export async function resolveSyscallTarget(filePath: string, followFinal: boolean): Promise { + const target = path.resolve(filePath); + if (followFinal) { + const real = await tryRealpathAsync(target); + if (real !== null) return real; + // `realpath` also fails on a DANGLING link, which a write follows to a place + // this cannot name, and on a path whose ancestor may not be searched. Neither + // is proof the final component is a plain name, and only proof continues. + if (!(await isProvenNotSymlink(target))) return null; + } + // Walk up to the deepest ancestor that does resolve, then re-apply the + // components below it. A resolved ancestor vouches for the ones above it, so + // re-applying them lexically matches what the kernel would have done. + const tail: string[] = [path.basename(target)]; + let ancestor = path.dirname(target); + for (;;) { + const real = await tryRealpathAsync(ancestor); + if (real !== null) return path.join(real, ...tail.reverse()); + // This component is about to be re-applied lexically without a resolved + // ancestor vouching for it, which is exactly the escape being closed — so it + // has to prove itself. `realpath` fails here for a component that does not + // exist yet AND for one inside a directory the caller may not search (the + // usual shape when a sandbox hides a denied path), and the second still + // permits `lstat`. + if (!(await isProvenNotSymlink(ancestor))) return null; + const parent = path.dirname(ancestor); + // Ran past the filesystem root: `realpath("/")` cannot fail, so only a + // filesystem disappearing mid-walk gets here. + if (parent === ancestor) return null; + tail.push(path.basename(ancestor)); + ancestor = parent; + } +} + +async function tryRealpathAsync(target: string): Promise { + try { + // `fs.promises.realpath` has no `.native` variant under Bun, unlike its sync + // counterpart; the JS implementation resolves links identically. + return await fs.promises.realpath(target); + } catch { + return null; + } +} + +/** + * Whether `target` is known NOT to redirect. A path that does not exist cannot + * redirect anything, and nothing below it exists either; any other `lstat` + * failure leaves the question unanswered, which is not proof. + */ +async function isProvenNotSymlink(target: string): Promise { + try { + return !(await fs.promises.lstat(target)).isSymbolicLink(); + } catch (error) { + return isEnoent(error); + } +} + export function formatPathRelativeToCwd( filePath: string, cwd: string, diff --git a/packages/coding-agent/test/sdk-file-write-fallback-extension.test.ts b/packages/coding-agent/test/sdk-file-write-fallback-extension.test.ts index cc378731f..a3d720ba5 100644 --- a/packages/coding-agent/test/sdk-file-write-fallback-extension.test.ts +++ b/packages/coding-agent/test/sdk-file-write-fallback-extension.test.ts @@ -88,9 +88,13 @@ describe("registerFileWriteFallback end-to-end (real extension, real session)", let registryAuthDir: string; const makeTempDir = (): string => { - const tempDir = path.join(os.tmpdir(), `pi-file-write-fallback-e2e-${Snowflake.next()}`); + const created = path.join(os.tmpdir(), `pi-file-write-fallback-e2e-${Snowflake.next()}`); + fs.mkdirSync(created, { recursive: true }); + // The seam brokers a symlink-RESOLVED path, and `os.tmpdir()` sits under `/var` + // — itself a link — on macOS. Canonicalizing the fixture up front keeps a + // handler's `req.dst` comparable to the path a test built. + const tempDir = fs.realpathSync.native(created); tempDirs.push(tempDir); - fs.mkdirSync(tempDir, { recursive: true }); return tempDir; }; @@ -163,9 +167,11 @@ describe("registerFileWriteFallback end-to-end (real extension, real session)", lock(lockedDir, 0o500); // no write bit: creating a file here needs dir-write const received: FileWriteFallbackRequest[] = []; + const ownSessionIds: string[] = []; const factory: ExtensionFactory = pi => { - pi.registerFileWriteFallback(async req => { + pi.registerFileWriteFallback(async (req, ctx) => { received.push(req); + ownSessionIds.push(ctx.sessionManager.getSessionId()); // Stand-in for an out-of-process privileged broker: this test's own // user cannot write into `lockedDir`, so relax the permission bit // just long enough to place the exact bytes the tool intended, then @@ -199,6 +205,12 @@ describe("registerFileWriteFallback end-to-end (real extension, real session)", expect(received).toHaveLength(1); expect(received[0]?.dst).toBe(targetPath); expect(received[0]?.content).toBe(content); + // (ii-b) and it can tell WHOSE write it was: the registry is process-wide, so + // the request names the issuing session and `ctx` names the handler's own. + // Both defined and equal here, which only holds if the tool-execution scope + // that carries the session id is actually entered. + expect(ownSessionIds[0]).toMatch(/./); + expect(received[0]?.sessionId).toBe(ownSessionIds[0]); expect(fs.readFileSync(targetPath, "utf8")).toBe(content); const headerLine = resultText(writeResult).split("\n")[0] ?? ""; @@ -413,6 +425,57 @@ describe("registerFileWriteFallback end-to-end (real extension, real session)", } }); + itDenied("write: a throwing handler does not skip later handlers from the SAME extension", async () => { + // The registry sees ONE trampoline per extension, so per-handler isolation has to + // live inside that trampoline. Without it, a throw from the first handler escapes + // to the registry, which advances to the next EXTENSION — so every later handler + // this extension registered is skipped, breaking both the documented "a throwing + // handler is skipped" rule and registration order for a backup-handler setup. + const tempDir = makeTempDir(); + const lockedDir = path.join(tempDir, "locked-order"); + fs.mkdirSync(lockedDir, { recursive: true }); + lock(lockedDir, 0o500); + + const order: string[] = []; + const factory: ExtensionFactory = pi => { + pi.registerFileWriteFallback(async () => { + order.push("throws"); + throw new Error("first handler blew up"); + }); + pi.registerFileWriteFallback(async () => { + order.push("declines"); + return false; + }); + pi.registerFileWriteFallback(async req => { + order.push("brokers"); + fs.chmodSync(lockedDir, 0o700); + try { + fs.writeFileSync(req.dst, req.content); + } finally { + fs.chmodSync(lockedDir, 0o500); + } + return true; + }); + }; + + const { session } = await createAgentSession(baseOptions(tempDir, [factory])); + initializeRunnerForTest(session.extensionRunner); + + try { + const targetPath = path.join(lockedDir, "ordered.txt"); + const content = "export const value = 3;\n"; + const writeTool = session.getToolByName("write") as AgentTool | undefined; + const writeResult = await writeTool!.execute("call-write-order", { path: targetPath, content }); + + expect(writeResult.isError).not.toBe(true); + expect(order).toEqual(["throws", "declines", "brokers"]); + expect(fs.readFileSync(targetPath, "utf8")).toBe(content); + } finally { + fs.chmodSync(lockedDir, 0o700); + await session.dispose(); + } + }); + itDenied( "edit: a hashline MV OUT of an undeletable directory removes the source through the delete seam", async () => { diff --git a/packages/coding-agent/test/tools/file-write-fallback.test.ts b/packages/coding-agent/test/tools/file-write-fallback.test.ts index 5a8c9da7a..ad128f5a3 100644 --- a/packages/coding-agent/test/tools/file-write-fallback.test.ts +++ b/packages/coding-agent/test/tools/file-write-fallback.test.ts @@ -8,6 +8,7 @@ import { addFileWriteFallback, deleteFileWithFallback, isPermissionDeniedError, + withFileMutationSession, writeFileWithFallback, } from "@oh-my-pi/pi-coding-agent/tools/file-write-fallback"; @@ -79,6 +80,29 @@ describe("writeFileWithFallback", () => { expect(seen).toEqual([{ dst: "/denied/path.txt", content: "payload" }]); }); + it("names the session that issued the write, and reports none outside a tool call", async () => { + // The registry is process-wide, so a handler can be asked about a write from a + // session other than its own. Without this it cannot tell the difference, which + // is what makes a per-session decision (or a prompt through the right session's + // UI) impossible. + const seen: Array = []; + disposers.push( + addFileWriteFallback(async req => { + seen.push(req.sessionId); + return true; + }), + ); + + await withFileMutationSession("session-a", () => + writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never), + ); + // No scope: an external `applyPatch` caller is not attributable to a session, + // and inventing one would be worse than saying so. + await writeFileWithFallback("/denied/path.txt", "payload", denyingFile(fsError("EACCES")) as never); + + expect(seen).toEqual(["session-a", undefined]); + }); + it("rethrows a non-permission error without consulting any handler", async () => { let called = false; disposers.push( @@ -221,7 +245,11 @@ describe("writeFileWithFallback", () => { let root = ""; beforeEach(async () => { - root = await fs.mkdtemp(path.join(os.tmpdir(), "fallback-kernel-")); + // Canonical from the start: the seam hands handlers a symlink-resolved path, + // and `os.tmpdir()` is under `/var` — itself a link — on macOS, so a lexical + // fixture path would differ from the brokered one for a reason unrelated to + // what these tests are about. + root = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "fallback-kernel-"))); }); afterEach(async () => { // Restore the mode first: a 0o500 directory cannot be emptied. @@ -301,13 +329,13 @@ describe("writeFileWithFallback", () => { expect(called).toBe(false); }); - it("refuses to broker a denied write reached through a symlink", async () => { - // The escape this blocks: the agent creates a link inside a directory the + it("brokers the RESOLVED target for a write through a symlink", async () => { + // The escape this closes: the agent creates a link inside a directory the // sandbox permits, pointing at a target it does not. The in-process write - // follows the link and the kernel denies the TARGET, so the failure looks - // like any other denial — but a privileged helper handed `dst` would follow - // the same link and land the bytes outside the sandbox. A helper-side prefix - // allowlist cannot catch it, because the link is inside the allowed root. + // follows the link, so the kernel denied the TARGET — but a handler given + // the LINK would pass its own prefix allowlist, because the link sits inside + // the allowed root while its target does not. The handler is told where the + // bytes would really land, so its allowlist judges the real destination. const secretDir = path.join(root, "off-limits"); await fs.mkdir(secretDir); const secret = path.join(secretDir, "authorized_keys"); @@ -318,6 +346,95 @@ describe("writeFileWithFallback", () => { const link = path.join(root, "innocent-link"); await fs.symlink(secret, link); + const seen: string[] = []; + disposers.push( + addFileWriteFallback(async req => { + seen.push(req.dst); + // Declining stands in for the allowlist refusal a real helper makes. + return false; + }), + ); + + try { + await expect(writeFileWithFallback(link, "pwned\n")).rejects.toMatchObject({ + code: expect.stringMatching(/^(EACCES|EPERM)$/), + }); + expect(seen).toEqual([secret]); + expect(await Bun.file(secret).text()).toBe("original\n"); + } finally { + await fs.chmod(secretDir, 0o700); + await fs.chmod(secret, 0o600); + } + }); + + it("resolves a symlinked ANCESTOR, not just a link at the last component", async () => { + // `lstat(dst)` alone judges only the final component, so `ws/link/file` under + // a `ws/link -> /outside` link is a lexically innocent path whose bytes land + // outside. Every component above the last is followed by the kernel, so the + // handler has to be told the resolved path for this shape too. + const outside = path.join(root, "off-limits"); + await fs.mkdir(outside); + const victim = path.join(outside, "secret.txt"); + await Bun.write(victim, "original\n"); + await fs.chmod(victim, 0o400); + await fs.chmod(outside, 0o500); + + const linkDir = path.join(root, "innocent-dir"); + await fs.symlink(outside, linkDir); + + const seen: string[] = []; + disposers.push( + addFileWriteFallback(async req => { + seen.push(req.dst); + return false; + }), + ); + + try { + await expect(writeFileWithFallback(path.join(linkDir, "secret.txt"), "pwned\n")).rejects.toMatchObject({ + code: expect.stringMatching(/^(EACCES|EPERM)$/), + }); + expect(seen).toEqual([victim]); + expect(await Bun.file(victim).text()).toBe("original\n"); + } finally { + await fs.chmod(outside, 0o700); + await fs.chmod(victim, 0o600); + } + }); + + it("refuses to broker a write through a dangling symlink", async () => { + // `realpath` cannot name where a dangling link points, and the write follows + // it, so there is no destination to hand a privileged writer. Refusing is the + // only honest answer, and it is the one `confineToWorkspace` already gives. + const dir = path.join(root, "locked"); + await fs.mkdir(dir); + const dangling = path.join(dir, "dangling"); + await fs.symlink(path.join(dir, "nowhere"), dangling); + await fs.chmod(dir, 0o500); + + let called = false; + disposers.push( + addFileWriteFallback(async () => { + called = true; + return true; + }), + ); + + await expect(writeFileWithFallback(dangling, "payload")).rejects.toMatchObject({ + code: expect.stringMatching(/^(EACCES|EPERM)$/), + }); + expect(called).toBe(false); + }); + + it("refuses to broker a write whose own metadata is behind the boundary", async () => { + // A sandbox that denies the write often hides the target's metadata too, so + // the final component cannot be shown to be a plain name rather than a link — + // and `open` follows a link there. The delete seam keeps working in this shape + // because `unlink` never follows the last component; a write cannot. + const opaque = path.join(root, "opaque"); + await fs.mkdir(opaque); + await fs.chmod(opaque, 0o000); + let called = false; disposers.push( addFileWriteFallback(async () => { @@ -327,14 +444,12 @@ describe("writeFileWithFallback", () => { ); try { - await expect(writeFileWithFallback(link, "pwned\n")).rejects.toMatchObject({ + await expect(writeFileWithFallback(path.join(opaque, "new.txt"), "payload")).rejects.toMatchObject({ code: expect.stringMatching(/^(EACCES|EPERM)$/), }); expect(called).toBe(false); - expect(await Bun.file(secret).text()).toBe("original\n"); } finally { - await fs.chmod(secretDir, 0o700); - await fs.chmod(secret, 0o600); + await fs.chmod(opaque, 0o700); } }); }); @@ -346,7 +461,7 @@ describe("writeFileWithFallback", () => { let locked = ""; beforeEach(async () => { - root = await fs.mkdtemp(path.join(os.tmpdir(), "fallback-patch-")); + root = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "fallback-patch-"))); locked = path.join(root, "locked"); await fs.mkdir(locked); await fs.chmod(locked, 0o500); @@ -423,7 +538,8 @@ describe("deleteFileWithFallback", () => { let locked = ""; beforeEach(async () => { - root = await fs.mkdtemp(path.join(os.tmpdir(), "fallback-del-")); + // Canonical from the start; see the write-side note above. + root = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "fallback-del-"))); locked = path.join(root, "locked"); await fs.mkdir(locked); }); @@ -440,19 +556,21 @@ describe("deleteFileWithFallback", () => { return target; } - it("diverts a real denied unlink to a registered handler", async () => { + it("diverts a real denied unlink to a registered handler, naming its session", async () => { const target = await lockedFile(); - const seen: Array<{ dst: string; code: unknown }> = []; + const seen: Array<{ dst: string; code: unknown; sessionId: string | undefined }> = []; disposers.push( addFileDeleteFallback(async req => { - seen.push({ dst: req.dst, code: (req.cause as NodeJS.ErrnoException).code }); + seen.push({ dst: req.dst, code: (req.cause as NodeJS.ErrnoException).code, sessionId: req.sessionId }); return true; }), ); - await deleteFileWithFallback(target); + await withFileMutationSession("session-del", () => deleteFileWithFallback(target)); - expect(seen).toEqual([{ dst: target, code: expect.stringMatching(/^(EACCES|EPERM)$/) }]); + expect(seen).toEqual([ + { dst: target, code: expect.stringMatching(/^(EACCES|EPERM)$/), sessionId: "session-del" }, + ]); }); it("rethrows the ORIGINAL error when the handler declines", async () => { @@ -537,11 +655,45 @@ describe("deleteFileWithFallback", () => { expect(called).toBe(false); }); - it("reports confirmedFile false for a symlink, so a handler cannot safely resolve it", async () => { - // Unlinking a symlink is a legitimate file removal, so this still diverts. But - // a handler that realpaths `dst` for auditing, or removes it recursively, - // would act on the link's TARGET — a directory tree, here. `confirmedFile` - // must therefore be false even though `lstat` succeeded. + it("resolves a symlinked ANCESTOR before brokering a delete", async () => { + // `unlink` follows every component above the last, so a link in the path + // removes a file outside the allowed root while the lexical path still looks + // contained. The handler must be told which file actually disappears. + const outside = path.join(root, "off-limits"); + await fs.mkdir(outside); + const victim = path.join(outside, "keep.txt"); + await Bun.write(victim, "keep me"); + await fs.chmod(outside, 0o500); + + const linkDir = path.join(root, "innocent-dir"); + await fs.symlink(outside, linkDir); + + const seen: Array<{ dst: string; confirmedFile: boolean }> = []; + disposers.push( + addFileDeleteFallback(async req => { + seen.push({ dst: req.dst, confirmedFile: req.confirmedFile }); + return false; + }), + ); + + try { + await expect(deleteFileWithFallback(path.join(linkDir, "keep.txt"))).rejects.toMatchObject({ + code: expect.stringMatching(/^(EACCES|EPERM)$/), + }); + expect(seen).toEqual([{ dst: victim, confirmedFile: true }]); + expect(await Bun.file(victim).text()).toBe("keep me"); + } finally { + await fs.chmod(outside, 0o700); + } + }); + + it("leaves the LAST component unresolved, reporting confirmedFile false for a link", async () => { + // `unlink` removes the link itself, so resolving the final component would + // name the wrong file. Diverting is still right — unlinking a link is a + // legitimate file removal — but a handler that realpaths `dst` for auditing, + // or removes it recursively, would act on the link's TARGET, a directory tree + // here. So the link is brokered as itself, and `confirmedFile` is false even + // though `lstat` succeeded. const targetDir = path.join(root, "link-target-dir"); await fs.mkdir(targetDir); await Bun.write(path.join(targetDir, "keep.txt"), "keep me"); @@ -549,17 +701,17 @@ describe("deleteFileWithFallback", () => { await fs.symlink(targetDir, link); await fs.chmod(locked, 0o500); - const seen: boolean[] = []; + const seen: Array<{ dst: string; confirmedFile: boolean }> = []; disposers.push( addFileDeleteFallback(async req => { - seen.push(req.confirmedFile); + seen.push({ dst: req.dst, confirmedFile: req.confirmedFile }); return true; }), ); await deleteFileWithFallback(link); - expect(seen).toEqual([false]); + expect(seen).toEqual([{ dst: link, confirmedFile: false }]); // The target must be untouched: the seam only ever asked for the link. expect(await Bun.file(path.join(targetDir, "keep.txt")).text()).toBe("keep me"); });