main
3 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Larry Gordon
|
2c5a9c43aa |
test(tools): pinned the exclusive-create guard against the fallback seam
Review raised that `apply_patch`'s non-overwrite contract for `create` and a rename destination is decided with `Bun.file(dst).exists()`, which reports `false` when the parent hides the target's metadata rather than distinguishing "absent" from "unknown" — so a privileged handler could be asked to write over a protected file it was told not to touch. Reproduced all three shapes: no handler is consulted in any of them. A hidden-metadata destination is refused by the path resolver, because the same denied `lstat` that fools the existence check also leaves the final component unproven, and an unverifiable destination is never brokered. A destination that is a symlink onto a protected file is caught earlier — `exists()` follows the link and reports `true`. A plainly visible existing file is caught by the same check. So the contract holds, but it holds through two independent guards in two files. Pinned that with a regression test asserting the premise (the existence check cannot see the file), that no handler is consulted, and that the file is intact; deleting the resolver's symlink proof makes it fail. Recorded the coupling in the module header too, since relaxing the refusal to broker unverifiable paths would silently break exclusivity and needs an explicit intent field first. |
||
|
Larry Gordon
|
6ad935d093 |
fix(extensions): resolved brokered file paths and named their session
Review on #8052 found three problems in the write/delete fallback seam. A symlink guard that only `lstat`'d the final component let the same escape through a symlinked ancestor: `ws/link/file` under a `ws/link -> /outside` link reached a handler as a lexically innocent path, so a helper's prefix allowlist passed while the bytes landed outside. Refusing every symlinked component is not available, since `/var` and `/tmp` are links on macOS and every path under `os.tmpdir()` traverses one. So `req.dst` is now the path the failed syscall itself acted on, via `resolveSyscallTarget` beside `confineToWorkspace`: fully resolved for a write, resolved up to the last component for a delete, because `unlink` removes a link rather than following it. Resolving also closes the TOCTOU window a refusal left open. A path that cannot be canonicalized — a dangling final link, or an ancestor whose own resolution is denied — is not brokered at all. Per-handler throw isolation lived outside the per-extension trampoline, so a throw from one extension's first handler advanced the registry to the next extension and skipped every later handler that one had registered. Each handler call is now wrapped individually. The registry stays process-wide. A subagent spawned with restricted tools gets `preloadedExtensionPaths: []` and loads no extensions of its own, so scoping resolution to the originating session would turn its brokered writes into hard failures, and a host that registers once in its top-level session expects its subagents covered. The request names its origin instead: `req.sessionId` against the handler's own `ctx.sessionManager.getSessionId()`, entered by `ExtensionToolWrapper`, which `sdk.ts` already puts around the whole tool registry whenever a runner exists. Both registries are also walked over a snapshot, so a concurrent session shutdown cannot make another session's walk skip whichever handler shifted into the hole. Unit tests go 30 -> 35 and integration 6 -> 7, covering the resolved target, a symlinked ancestor on both seams, a dangling link, a target whose own metadata is behind the boundary, same-extension handler ordering after a throw, and `req.sessionId` matching the handler's own session end to end. |
||
|
Larry Gordon
|
6e4334c003 |
feat(extensions): broker denied file writes and deletes
A host that runs omp inside an OS sandbox can grant a path mid-session but cannot apply that grant to an in-process write: `write` and `edit` do their I/O in the agent process, so an out-of-workspace write fails and stays failed until the process restarts under a wider profile. Nothing available today closes that. A `tool_call` handler can block and a `tool_result` handler can rewrite content, but neither can re-run a tool. `ctx.invokeTool` delegates execution, but the delegated native tool runs in the same process under the same restrictions. And the failure lands AT the write syscall - after the tool computed the final content, before it returned - so the bytes are gone with the throw, and reconstructing them means reimplementing `edit`'s hashline protocol and the snapshot bookkeeping. The byte-write that `write`, `edit` and `apply_patch` perform on an ordinary file path already funnels through one two-line primitive (`file ? file.write(content) : Bun.write(dst, content)`) at four call sites. Routing that primitive through `writeFileWithFallback` gives an embedder a single seam to intercept a permission-denied write: the native tool still records its own snapshot under the real destination path once a handler reports success, so a follow-up hashline `edit` on that path keeps working. Only a permission boundary diverts - `EPERM`/`EACCES`/`EROFS`. Two cases needed more than that: - `Bun.write` creates missing parents itself, and when that `mkdir` is the denied operation it reports the subsequent `open()`'s `ENOENT` instead of the denial - making a sandboxed write into a new out-of-tree directory indistinguishable from an ordinary bad path. Redoing the `mkdir` explicitly recovers the real errno, and because it runs through the same enforcement path as the write it also sees kernel-level denials (Seatbelt, LSM) that a `stat`/`access` probe reports as writable. If no handler takes the write, the original `ENOENT` is still what propagates, with the recovered denial attached as its `cause`. - `apply_patch` creates the parent as a separate step before writing, so a denial there threw before the seam was ever reached. That `mkdir` now tolerates a permission denial when a fallback is registered, letting the write report it. 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 would land the bytes wherever the link points. That also defeats the obvious helper-side defence, since a prefix allowlist passes when 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 `confineToWorkspace` already gives an unresolvable link. Removing a file is a different primitive, so it gets its own seam (`deleteFileWithFallback`, `registerFileDeleteFallback`) covering `edit`'s `REM`, a hashline `MV`'s source unlink, and `apply_patch`'s delete op. Two differences from the write path: `ENOENT` is never diverted, since nothing is created on the way to an unlink and `REM` needs it to become a not-found error; and the seam refuses a target it can confirm is a directory, because `unlink` on a directory reports `EPERM` on Darwin and is otherwise indistinguishable from a sandbox denial. That check cannot always run - a sandbox denying the unlink usually denies the target's metadata too - so the request carries `confirmedFile`, and a handler is required to use a plain unlink rather than resolving or recursing. The two registries are deliberately separate. A write handler brokers `content` to `dst`, so a delete request reaching it with no content invites brokering an empty write and truncating the file it was asked to remove. With nothing registered both seams are inert: the primitives run exactly as before, a failure rethrows from the same place, and no extra syscalls are performed. Scope is deliberately narrow. Archive-member and SQLite writes are unchanged - neither is a byte-write to a path, so brokering them needs a different request shape - along with the ACP bridge's `writeTextFile`, the `lsp` tool's own workspace-edit and formatter writes, and directory removal. |