diff --git a/docs/specs/dor-cli.md b/docs/specs/dor-cli.md index 5242731f9..298bd6612 100644 --- a/docs/specs/dor-cli.md +++ b/docs/specs/dor-cli.md @@ -101,9 +101,7 @@ tab/eval/screenshot commands, and anything added later. It owns the Windows recipe: cross-spawn rather than Node's own `spawn`, `windowsHide`, and resolution on `exit` with an exit-time output snapshot (rationale). -**Never forward an argument containing a literal `%VAR%`** — `cmd.exe` expands -it through a `.cmd` shim, an unavoidable batch limitation; today's forwarded -arguments carry none. +**Must leave Windows shim quoting to `spawnAndCapture`.** Forwarded argv can contain literal percent expressions; never interpolate them into an independently constructed shell command. (rationale) - **`dor agent-browser`, `dor playwright` and both browser hosts spawn the `PATH`-resolved absolute path, never the bare name** — cross-spawn resolves a bare name @@ -153,9 +151,7 @@ SIGKILLs the child; **Windows must end the whole tree** with child is `cmd.exe` and the real CLI is its descendant (`treeKillCommand`, pinned through its `isWindows` argument). -**Resolution.** `dor-lib-common`'s `exports` point at its built `dist` -(Node-type-free `.d.ts`, since `dor`'s `tsc` avoids `@types/node`); every -esbuild/Vite consumer inlines it. **The `dor` and `dormouse-lib` prebuilds must +`dor-lib-common`'s `exports` point at built `dist`; esbuild/Vite consumers inline it. **The `dor` and `dormouse-lib` prebuilds must build `dor-lib-common` first**, or those `.d.ts` files are missing when either typechecks. @@ -219,6 +215,7 @@ spec carries: - **The POSIX socket name is 8 random bytes rather than 16**, so the path clears macOS's `sun_path` cap (rationale). +- **Must reject malformed control requests before reading their fields or forwarding them.** `standalone/sidecar/dor-control-server.test.js` pins continued service after a refused request. - **A connection that says nothing at all is dropped after 10s.** - The two proof domains are mirrored between client and server, pinned by `lib/src/lib/mirrored-constants.test.ts`. @@ -267,9 +264,7 @@ and each host's hop in `standalone/src/tauri-adapter.ts`, ## Handle Model -`Window ⊃ Workspace ⊃ Pane ⊃ Surface` (`docs/specs/glossary.md`). **Must treat a command's primary target as a Surface; `dor move` also takes a destination Workspace, while `--workspace ` scopes the source** ([dor workspace](#dor-workspace)). `Reserved:` no command targets -another Window; a request reaches the window that owns its Surface instead -(§Standalone), and the ref grammar for one is [Future](#future). +`Window ⊃ Workspace ⊃ Pane ⊃ Surface` (`docs/specs/glossary.md`). **Must use Surface handles for Surface verbs and container refs for container verbs.** `dor move` takes a destination Workspace, while `--workspace ` scopes the source ([dor workspace](#dor-workspace)). Cross-window request routing follows [Standalone](#standalone). Invariants: @@ -391,7 +386,7 @@ It picks the style (`cmd` / `posix` / `powershell`) with the same classifier clipboard/drop path escaping uses ([mouse-and-clipboard.md](mouse-and-clipboard.md) §8.6). -**Every first-party command except the `dor agent-browser` and `dor playwright` +**Every public first-party command except the `dor agent-browser` and `dor playwright` passthrough accepts `--json`**, emitting a stable object with the same handles as its text output; single-Surface responses always carry both `surface_id` (stable) and `surface_ref` (Workspace-stable short ref). Text output carries the @@ -545,13 +540,7 @@ nonstandard port is the one case needing the scheme typed. This overrides server on https just SSL-errors. **Reject** an input that is neither a URL nor a `host:port`, including a purely numeric "host" like `800:600` (rationale). -**Resolution is CLI-side**, so `dor agent-browser` hands `agent-browser` a real URL rather -than a handle a binary would resolve differently. Only the `open` / `goto` / -`navigate` verbs resolve, matching the target by **shape, not position** since -`dor` can't know agent-browser's flag arity (`open --headed surface:3` -resolves); **only the first special-shaped argument is rewritten**, these verbs -taking a single target. A Surface handle requires a live control endpoint -(failing clearly outside Dormouse); the `host:port` inference does not. +**Must resolve navigation targets CLI-side before forwarding to the browser provider.** Only the first target of a navigation verb is eligible. Skip known option values; an unknown option leaves the argv unchanged rather than guessing its arity. A Surface handle requires a live control endpoint; `host:port` inference does not. The provider descriptors and `resolveOpenTargetArgs` own recognized verbs and option arities. A Surface handle resolves through the `surface.resolveOpen` control method, which runs the same host port scan as `dor list --ports` (visible panes **and** @@ -588,13 +577,7 @@ Source of truth: `runBrowserCli` in `dor/src/commands/browser-cli.ts`; `BrowserV before stricli parses provider arguments.** One runner drives both; what differs is each provider's descriptor: -| | `dor agent-browser` | `dor playwright` | -| --- | --- | --- | -| Session flag forwarded | `--session ` | `--session=`; native `-s` read as `--session` | -| Targets resolved in | `open`, `goto`, `navigate` | `open`, `goto` | -| Never binds a Surface | `close` | `close`, `detach`, `close-all`, `kill-all`, `delete-data`, `list`, `show`, `install`, `install-browser` | -| Informational flags | `--help`, `-h` | `--help`, `-h`, `--version`, `-v` | -| Runs in / with | the caller's cwd and executable | the binding's project cwd and pinned executable | +`BrowserCliDescriptor`, the provider descriptors, and `BROWSER_PROVIDERS` own session argv, navigation verbs, nonbinding/informational controls, and execution scope. - **Exactly one identity flag**: `--key` (default `default`), `--session`, or `--surface`, plus `--workspace`; any two fail (`--key and --surface are @@ -642,7 +625,7 @@ stderr warning, when the bound one is gone**, and **must fail naming a bound cwd that no longer exists** rather than report the CLI missing. Source of truth: `runBrowserCli`, `resolveBinding` and `extractSessionFlags` in -`dor/src/commands/browser-cli.ts`; `runAgentBrowserCli` in +`dor/src/commands/browser-cli.ts`; `BROWSER_PROVIDERS` in `dor-lib-common/src/browser-providers.ts`; `runAgentBrowserCli` in `dor/src/commands/agent-browser.ts`; `runPlaywrightCli` in `dor/src/commands/playwright.ts`; `SURFACE_CONTROL_METHODS` in `dor/src/protocol.ts`; `ResolveBrowserRequest`, `BrowserSurfaceRequest` and @@ -662,9 +645,7 @@ stays unsupported. named by its Workspace-stable `surface:N` ref, or rediscovered after layout churn by `--command` / `--cwd` / `--port`, and `dor ensure`'s command+cwd match is an implicit key that also lets an agent adopt a command the user started by -hand. Only browser Surfaces carry an explicit join key (`dor agent-browser --key `, -`dor playwright --key `), because their session is held externally by the browser -CLI. +hand. Browser join keys follow `docs/specs/dor-browser.md` → Managed identity; Tool keys follow `docs/specs/dor-tool.md` → Identity and dedupe. The worked examples are `dor/skill.md`'s "## Recipes", which ships with the CLI ([Agent Skill](#agent-skill)). diff --git a/docs/specs/dor-cli.rationale.md b/docs/specs/dor-cli.rationale.md index f2516eb23..d2a43e387 100644 --- a/docs/specs/dor-cli.rationale.md +++ b/docs/specs/dor-cli.rationale.md @@ -83,6 +83,8 @@ redirects Unix stderr to a log or `/dev/null`, and daemon diagnostics discard write errors. Closing capture's read ends does not signal or kill descendants; a descendant that continues writing must tolerate a closed output sink. +A Windows reproduction in 2026-10 (Node 22.22.3, cross-spawn 7.0.6) passed a literal percent-delimited environment expression unchanged through simple and npm-shaped global/local batch shims. The earlier claim that forwarded arguments contain no such expression did not describe browser passthrough; bypassing the shared shim escaping would reintroduce expansion. + ## Control-channel security **Who the threat is.** Not the network — the channel is a local socket or named pipe. The attacker is a second account on the same box, or any process running as the user; interposing inherits the whole verb set at once — keystrokes in, screen and scrollback out, pane destroyed. diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index d92a776f7..aa567a78f 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -36,13 +36,7 @@ Source of truth: `surfaceKindFromParams` / `isToolParams` in `lib/src/components **Must read user Tools from `$XDG_CONFIG_HOME/dormouse/dormouse.yml` only when that environment value is absolute**, else `~/.config/dormouse/dormouse.yml`; both local hosts use this location. User Tools require no project grant; malformed or unreadable user configuration fails lookup. Project and user Tools occupy separate reuse scopes. -| Field | Behavior | -| --- | --- | -| `run` | Required shell command string or argument list, typed into the configured shell after integration readiness | -| `render` | `iframe` by default, `agent-browser-screencast`, or `playwright-screencast` | -| `viewport` | Initial browser sizing; `docs/specs/dor-browser.md` → Viewport presets owns resolution and defaults | -| `port` | `announced` by default, or `auto`; [Serving](#serving) owns selection | -| `prespawn_dedupe` | Optional scalar or list of literal key elements with substitutions | +`ToolEntry` and `parseToolFile` own declaration fields and defaults. [Serving](#serving) owns port selection; `docs/specs/dor-browser.md` → Viewport presets owns sizing resolution. - **Must reject unknown `prespawn_*` fields and unknown substitutions**; unknown ordinary fields produce warnings. `$PROJECT_ROOT` is the declaring directory, `$CWD` the caller's resolved directory, and `$TARGET` the canonical local file or directory input. (rationale) - **Must deliver the parsed file's warnings on the untrusted answer and on a built-in open**, not only on an already-trusted lookup — those are the paths a Tool's first run takes. @@ -55,7 +49,7 @@ Source of truth: `surfaceKindFromParams` / `isToolParams` in `lib/src/components **Must require exactly one existing local regular file or directory when `$TARGET` appears in the run list or dedupe key.** Resolve relative paths against the invocation CWD and follow symlinks to a canonical absolute path before substitution and reuse. **Must accept a `file:` URL only when its host is empty, `localhost`, or this machine's name** (case-insensitive, either side in its short form before the first dot), converting it with the host platform's `fileURLToPath`; reject other URLs, missing paths, and every other file kind (fifo, socket, device). Validate run and key inputs before showing approval. Pending approval distinguishes the original arguments and invocation CWD; [Trust](#trust) owns re-resolution and recovery. Input control-character restrictions belong to `docs/specs/security-local.md` → Dor Tool configuration. -Source of truth: `lookupTool` in `lib/src/host/tool-trust.ts`; `parseToolFile` / `resolveDedupeKey` in `lib/src/host/tool-registry.ts`; `resolveToolInput` / `resolveLocalToolTarget` in `lib/src/host/tool-input.ts`; `readUserToolFile` in `lib/src/host/tool-user-config.ts`; `toolRunCommand` in `lib/src/components/wall/use-dor-control.ts`; `lib/src/host/tool-host.test.ts`, `lib/src/host/tool-trust.test.ts`, `lib/src/host/tool-input.test.ts`, `lib/src/host/tool-open.test.ts`, `lib/src/components/Wall.test.tsx`. +Source of truth: `lookupTool` in `lib/src/host/tool-trust.ts`; `ToolEntry` / `parseToolFile` / `resolveDedupeKey` in `lib/src/host/tool-registry.ts`; `resolveToolInput` / `resolveLocalToolTarget` in `lib/src/host/tool-input.ts`; `readUserToolFile` in `lib/src/host/tool-user-config.ts`; `toolRunCommand` in `lib/src/components/wall/use-dor-control.ts`; `lib/src/host/tool-host.test.ts`, `lib/src/host/tool-trust.test.ts`, `lib/src/host/tool-input.test.ts`, `lib/src/host/tool-open.test.ts`, `lib/src/components/Wall.test.tsx`. **Must resolve a Tool's initial viewport host-side with its declaration, including after approval.** Iframe Tools accept only `pane-sync`; automated Tools accept a preset or inline dimensions. **Must preserve live user/agent sizing when reusing a Tool**, rather than reapplying its declaration. @@ -279,8 +273,9 @@ Source of truth: `toolTakesOverCaller` / `toolRerunsInCaller` / `callerStillPlac **Must consume OSC 367 at the PTY owner's parser**, including malformed and unknown verbs, and emit no reply. `serve`, `state`, and `open` are implemented verbs. The escape registry is `docs/specs/terminal-escapes.md`. - **Must sanitize and bound the payload before retaining it.** `ToolAnnounce` / `parseToolAnnounce` and `ToolState` / `parseToolState` own the field shapes and validation limits. +- **Must reject invalid encoder inputs and serialized payloads exceeding the host's limit**, including JSON escaping that expands an otherwise valid field. (rationale) - **Must reject a payload naming a version this contract does not speak.** `state` requires `v: 1`; `serve` reads an omitted `v` as 1 and refuses any other value — a future v2's rejection path. -- **Must treat an optional serve `path` as a path/query on the discovered port, never as another authority.** Accept at most 2,048 characters starting with one `/`, with no backslash, ASCII whitespace/control, or DEL; invalid paths are ignored and the default is `/`. The port still must belong to the designated Session's process tree. Live binding memory includes the path; durable saves omit it. +- **Must treat an optional serve `path` as a path/query on the discovered port, never as another authority.** Accept at most 2,048 characters starting with one `/`, with no backslash, ASCII whitespace, C0/C1 control, or DEL; invalid paths are ignored and the default is `/`. The port still must belong to the designated Session's process tree. Live binding memory includes the path; durable saves omit it. - **Must forward parsed announcements, state reports, open requests, and command-start resets in stream order to the owning renderer.** A start clears the previous command's announcement and unsaved state; later reports in that chunk survive. Both hosts forward each parse's as one `terminal:toolEvents`, which the owning renderer applies with `applyLiveToolEvents`; the fake adapter applies locally. - **Must reconstruct announcements, state, and resets from raw replay without emitting replies or acting on an `open`**, preserving transferred announcements when since-mark replay has no command start, and clear the renderer record on Session disposal. Ordinary terminal announcements stay inert. - Reserved: **Must retain `name`, `dehydrate`, and `persist` as inert parsed fields**, serving the announced-name and D1/D2 items under [Future](#future). Neither `persist: never` nor a `dehydrate` verb changes current persistence. @@ -288,7 +283,7 @@ Source of truth: `toolTakesOverCaller` / `toolRerunsInCaller` / `callerStillPlac - **Must show a failed `open` in the preview slot**: a preview whose lookup fails runs the built-in error viewer there instead (`docs/specs/dor-tools-builtin.md` → Error viewer), and a failed activate is sent again as a preview unless a newer `open` from that Session followed it. - Reserved: **Never assign an OSC 367 verb beyond `serve`, `state`, `open`, and `dehydrate`**; `dehydrate` belongs to D2 under [Future](#future), while existing title/progress protocols keep those roles. -Source of truth: `TerminalProtocolParser` / `collectTerminalToolEvents` in `lib/src/lib/terminal-protocol.ts`; `parseToolAnnounce` / `parseToolOpen` in `dor-tools-lib/src/osc.ts`; `applyLiveToolEvents` in `lib/src/lib/tool-events.ts`; `dispatchToolOpens` in `lib/src/lib/tool-open-requests.ts`; `surface.tool` in `lib/src/components/wall/use-dor-control.ts`; `recordToolAnnounce` in `lib/src/lib/tool-announce-store.ts`; `recordToolEvents` in `lib/src/lib/tool-events.ts`; `createOwnerPtyStream` in `lib/src/host/owner-pty.ts`. Tests: `dor-tools-lib/test/osc.test.mjs`, `lib/src/lib/tool-announce.test.ts`, `an OSC 367 open` in `lib/src/components/wall/preview-slot.test.tsx`, `lib/src/host/remote/sidecar-entry.test.ts`, `vscode-ext/test/message-router.test.ts`, `standalone/scripts/dev-agent-browser-announce.test.mjs`. +Source of truth: `TerminalProtocolParser` / `collectTerminalToolEvents` in `lib/src/lib/terminal-protocol.ts`; `serveSequence` / `stateSequence` / `openSequence` / `parseToolAnnounce` / `parseToolOpen` in `dor-tools-lib/src/osc.ts`; `applyLiveToolEvents` in `lib/src/lib/tool-events.ts`; `dispatchToolOpens` in `lib/src/lib/tool-open-requests.ts`; `surface.tool` in `lib/src/components/wall/use-dor-control.ts`; `recordToolAnnounce` in `lib/src/lib/tool-announce-store.ts`; `recordToolEvents` in `lib/src/lib/tool-events.ts`; `createOwnerPtyStream` in `lib/src/host/owner-pty.ts`. Tests: `dor-tools-lib/test/osc.test.mjs`, `lib/src/lib/tool-announce.test.ts`, `an OSC 367 open` in `lib/src/components/wall/preview-slot.test.tsx`, `lib/src/host/remote/sidecar-entry.test.ts`, `vscode-ext/test/message-router.test.ts`, `standalone/scripts/dev-agent-browser-announce.test.mjs`. ## Unsaved changes @@ -311,7 +306,7 @@ Source of truth: `parseToolState` in `dor-tools-lib/src/osc.ts`; `getToolDirty` ### Closing unsaved Tools -The iframe save channel, connected only to a `builtin:file` frame (`docs/specs/dor-tools-builtin.md` → Editing files), binds its window, proxy origin, and a per-mount connection nonce. Save completion carries the request id and current dirty state; a timeout or disconnected editor never permits a Save closure. **Must ignore a save-channel message naming another `dorTool` version or malformed for its kind**; a save error reaches the prompt control-stripped and bounded. +The iframe save channel, connected only to a `builtin:file` frame (`docs/specs/dor-tools-builtin.md` → Editing files), binds its window, proxy origin, and a per-mount connection nonce. Save completion carries the request id and current dirty state; a timeout or disconnected editor never permits a Save closure. **Must bind each save completion to its accepted connection generation and discard it after reconnect or close**, even when a replacement connection reuses the request id or nonce. (rationale) **Must ignore a save-channel message naming another `dorTool` version or malformed for its kind**; a save error reaches the prompt control-stripped and bounded. **Must offer Save / Discard / Cancel before closing dirty Tools through Dormouse**: Pane closure, standalone window/app teardown, iframe reload or renderer change, and Workspace movement to another Window; **a Workspace close asks once, before any Surface closes.** Discard authorizes that action without declaring the edit clean; Save proceeds only after successful acknowledgement and no newer edits. A Tool without a connected save handler must be saved in its own UI or discarded. **Never prompt for a command close or move**: `dor kill`, `dor workspace close` (even `--force`), and cross-window `dor workspace move` (even `--dangerously-destroy-iframe-page-state`) refuse a dirty Tool. VS Code webview/host closure, forced termination, and crashes cannot be vetoed; drafts are not persisted. diff --git a/docs/specs/dor-tool.rationale.md b/docs/specs/dor-tool.rationale.md index cf1a3c336..74c548f51 100644 --- a/docs/specs/dor-tool.rationale.md +++ b/docs/specs/dor-tool.rationale.md @@ -68,7 +68,7 @@ A name read from a browser or terminal passes through whatever those show mid-sw ## Opening local files -The VS Code host supports Node 18, which lacks native glob matching. Bundled picomatch keeps association behavior the same across hosts. Patterns with separators test both the CWD-relative and canonical absolute path: files above the CWD otherwise start with `../` and can miss patterns intended to cover an absolute directory. Canonicalization also gives symlink aliases one matching identity. Canonicalizing only the target mixed physical and logical paths under a symlinked CWD, so relative slash patterns missed files inside that directory. An absolute target can still be opened after its caller's CWD disappears; matching falls back to the supplied directory in that case. +The supported VS Code host floor runs Node 20 (2026-10), which lacks native glob matching; `docs/specs/vscode.md` records the pinned runtime floor. Bundled picomatch keeps association behavior the same across hosts. Patterns with separators test both the CWD-relative and canonical absolute path: files above the CWD otherwise start with `../` and can miss patterns intended to cover an absolute directory. Canonicalization also gives symlink aliases one matching identity. Canonicalizing only the target mixed physical and logical paths under a symlinked CWD, so relative slash patterns missed files inside that directory. An absolute target can still be opened after its caller's CWD disappears; matching falls back to the supplied directory in that case. ## Folders @@ -129,3 +129,11 @@ A derived URL or browser daemon binding belongs to one execution. Reusing it aft Routing `dor tool` to a native editor on one host would change its result from a Surface handle to a host-specific side effect. Native file opening remains a separate operation. A Workspace transfer carries the live browser binding separately from its durable record. The arrival record can reach disk while the windows coordinate, whereas the content channel stays in memory; reusing the saved-record projection alone would reopen a Tool browser and lose its current page state. Pending approvals and unfinished browser startup still own asynchronous work in the source window, so the move waits for the user to resolve the approval or retry after startup. + +## OSC 367 + +JSON escaping can double the source length of a valid path, so a field within its own bound can still exceed the serialized payload cap of the host. C1 OSC and ST are literal JSON characters, unlike escaped C0 controls; allowing them in a serve path inserts terminal framing into the emitted sequence. + +## Closing unsaved Tools + +Host save request ids restart at 1 on a replacement connection. A completion that reads the current frame connection can therefore acknowledge a new request after reconnect, permitting closure before the new save finishes. The accepted host object distinguishes connection generations even when a reconnect repeats its nonce. diff --git a/docs/specs/dor-tools-builtin.md b/docs/specs/dor-tools-builtin.md index 1c3d222e1..6671493e5 100644 --- a/docs/specs/dor-tools-builtin.md +++ b/docs/specs/dor-tools-builtin.md @@ -16,8 +16,6 @@ ## Packaging -`dor-tools-builtin` is a private workspace package with a separate Node runtime bundle. - - **Must bundle the viewers and their runtime dependencies into `dist/runtime.js` in `dor-tools-builtin`**, without workspace or installed-package resolution at runtime. `dor`'s prebuild builds this package first. - **Must stage the runtime and its adjacent `viewer` assets together under `dor/dist/builtin`**. Both hosts copy that tree with the CLI; `viewerAsset` resolves assets relative to the runtime module. - **Must keep `file-viewer-format` free of Node runtime dependencies**: lib's renderer and host modules import it, and every build of lib source maps `dor-tools-builtin/*` to this package's `src`, as it maps `dor/*`. `dor-tools-builtin/test/browser-shared.test.mjs` bundles it for a browser. @@ -36,35 +34,37 @@ Source of truth: `dor/package.json`, `dor-tools-builtin/package.json`; `dor-tool **Must require a user Tool for PDFs**, including files named `README.pdf`. (rationale) -**Must retain media/HTML grant descriptors until the Tool exits.** Refresh reads those files again; replacements and dependency-graph changes require restarting. Text editing follows [Editing files](#editing-files). Cold restore creates a fresh URL capability; Workspace movement keeps the live binding. The listener's authority is `docs/specs/security-local.md` → Local-file viewer. +**Must retain media/HTML grant descriptors until the Tool exits.** Refresh reads those files again; replacements and dependency-graph changes require restarting. Text editing follows [Editing files](#editing-files). Cold restore creates a fresh URL capability; Workspace movement keeps the live binding. Source of truth: `fileViewerFormat` / `viewerTitle` in `dor-tools-builtin/src/file-viewer-format.ts`; `startFileViewer` / `runFileViewer` in `dor-tools-builtin/src/file-viewer.ts`; `announceViewer` in `dor-tools-builtin/src/viewer-server.ts`. Tests: `dor-tools-builtin/test/file-viewer.test.mjs`, `dor/test/builtin-viewers.test.mjs`. ## Editing files -**Must render supported UTF-8 text in bundled Monaco**, with line numbers, find/replace, undo, selection, and optional wrapping. Use the workbench's editor colors and fonts with Monaco's light/dark syntax defaults; iframe theme delivery belongs to `docs/specs/theme.md` → Tool iframe themes. Never execute source text or load its referenced assets. +**Must render supported UTF-8 text in bundled Monaco**. Use the workbench's editor colors and fonts with Monaco's light/dark syntax defaults; iframe theme delivery belongs to `docs/specs/theme.md` → Tool iframe themes. Never execute source text or load its referenced assets. + +**Must save only on Save or Cmd/Ctrl+S, to the opened canonical file.** Preserve UTF-8 BOM and the dominant line ending (mixed endings are normalized). Bound text to 8 MiB; reject invalid UTF-8. Atomically replace only after comparing the submitted revision with current disk bytes and file identity; a conflict or write failure keeps the edit dirty. New edits during a save remain dirty after that save succeeds. Reload asks before discarding edits and reads the current file at the authorized path, including atomic replacements. -**Must save only on Save or Cmd/Ctrl+S, to the opened canonical file.** Preserve UTF-8 BOM, the dominant line ending (mixed endings are normalized), and permissions. Bound text to 8 MiB; reject invalid UTF-8. Atomically replace only after comparing the submitted revision with current disk bytes and file identity; a conflict or write failure keeps the edit dirty. New edits during a save remain dirty after that save succeeds. Reload asks before discarding edits and reads the current file at the authorized path, including atomic replacements. +**Must preserve document permissions on replacement.** Drafts sit beside the document (POSIX `0600`, then the document's mode; Windows inherits the directory ACL). Windows replaces via one PowerShell `[IO.File]::Replace`, which backs up the original beside it. **Must keep the draft and backup when replacement is unconfirmed** and report their location: failure can mean partial replacement or a commit before its reply. (rationale) **Must report dirty state immediately to the containing iframe and in order through OSC 367** (`docs/specs/dor-tool.md` → Unsaved changes), and answer the host's iframe save channel (`docs/specs/dor-tool.md` → Closing unsaved Tools) with `connectToolFrame`. -Source of truth: `readEditableFile` / `saveEditableFile` in `dor-tools-builtin/src/editable-file.ts`; `editorPage` in `dor-tools-builtin/src/editor-page.ts`; `dor-tools-builtin/viewer/editor.ts`; `runFileViewer` in `dor-tools-builtin/src/file-viewer.ts`. Tests: `dor-tools-builtin/test/editable-file.test.mjs`, `dor-tools-builtin/test/file-viewer.test.mjs`. +Source of truth: `readEditableFile` / `saveEditableFile` in `dor-tools-builtin/src/editable-file.ts`; `saveFileOperations` in `dor-tools-builtin/src/atomic-save.ts`; `editorPage` in `dor-tools-builtin/src/editor-page.ts`; `dor-tools-builtin/viewer/editor.ts`; `runFileViewer` in `dor-tools-builtin/src/file-viewer.ts`. Tests: `dor-tools-builtin/test/atomic-save.test.mjs`, `dor-tools-builtin/test/editable-file.test.mjs`, `dor-tools-builtin/test/file-viewer.test.mjs`. ## Folder viewer `builtin:folder`, the default folder viewer (`docs/specs/dor-tool.md` → Folders), is a Tool-owned `dor` process, titled as in [File viewer](#file-viewer): -- **Must list names and entry types only, lazily loading expanded directories and probing compactable chains, and never serve file contents** (rationale). The listener's audited rules are `docs/specs/security-local.md` → Local-file viewer. +- **Must list names and entry types only, lazily loading expanded directories and probing compactable chains, and never serve file contents** (rationale). - **Must list dotfiles.** Git-ignored entries are dimmed and shown by default, with a show/hide toggle. - **Must compact a directory containing exactly one child directory and nothing else into a slash-separated row**, continuing up to 32 levels per listing; hidden/ignored entries still count as siblings. Never compact through a symlink child. Refresh and Collapse all preserve the ordinary select/activate contract. -- **Must cut a listing to its first 5,000 entries in display order**, directories first, from at most 100,000 names read. -- **Must route the page's select and activate through its own process**: a same-origin POST to its capability listener, which writes it to the Tool's terminal as an OSC 367 `open` (`docs/specs/dor-tool.md` → OSC 367), `preview` for a select, in arrival order. The page learns only that it was sent, or, for a path `validToolOpenPath` refuses, an error instead of a write. +- **Must read at most 100,000 names and retain at most 5,000 entries before following symlinks**, selected by raw directory kind and name; return the retained entries in display order by resolved kind, directories first. +- **Must route the page's select and activate through its own process**: a same-origin POST to its capability listener, which writes it to the Tool's terminal as an OSC 367 `open` (`docs/specs/dor-tool.md` → OSC 367), `preview` for a select, in arrival order. The page learns only that it was sent, or, for a path or serialized payload the OSC encoder refuses, an error instead of a write. - **Must hold an activate until every select in flight settles**, keeping selects concurrent. (rationale) Source of truth: `startFolderViewer` / `runFolderViewer` / `oscOpen` in `dor-tools-builtin/src/folder-viewer.ts`; `folderViewerPage` in `dor-tools-builtin/src/folder-viewer-page.ts`. Tests: `dor-tools-builtin/test/folder-viewer.test.mjs`, `the folder entry selects and activates with OSC 367 open, in the order the page sends them` in `dor/test/builtin-viewers.test.mjs`. ## Error viewer -**Must show why an OSC 367 `open` failed** when the host runs `dor __view-error ` in the preview slot (`docs/specs/dor-tool.md` → OSC 367): one page naming the target's basename and the message, both escaped, with no script, served by the shared capability listener and titled as in [File viewer](#file-viewer). No handler name selects it. The listener's audited rules are `docs/specs/security-local.md` → Local-file viewer. +**Must show why an OSC 367 `open` failed** when the host runs `dor __view-error ` in the preview slot (`docs/specs/dor-tool.md` → OSC 367): one page naming the target's basename and the message, both escaped, with no script, served by the shared capability listener and titled as in [File viewer](#file-viewer). No handler name selects it. Source of truth: `startErrorViewer` / `errorViewerPage` / `runErrorViewer` in `dor-tools-builtin/src/error-viewer.ts`; `VIEW_ERROR_ARGV` in `dor-tools-builtin/src/file-viewer-format.ts`. Tests: `dor-tools-builtin/test/error-viewer.test.mjs`, `the error entry titles itself after its target and serves the escaped message` in `dor/test/builtin-viewers.test.mjs`. diff --git a/docs/specs/dor-tools-builtin.rationale.md b/docs/specs/dor-tools-builtin.rationale.md index 878d2d520..2a35bf09a 100644 --- a/docs/specs/dor-tools-builtin.rationale.md +++ b/docs/specs/dor-tools-builtin.rationale.md @@ -21,3 +21,11 @@ Without a title, a viewer's header falls back to its running command, `dor __vie A names-only folder viewer leaves file contents behind the existing one-file grant. A content-serving folder grant cannot hold descriptors for a whole tree from launch, so it would need open-per-request containment and a rewrite of the Local-file viewer checks. Each page POST is its own connection, so two can arrive out of order. In a live run (2026-09-28), when POSTs still became `dor open` calls, a double-click's activate reached the renderer before its select, so the file opened as an ordinary split beside the folder viewer instead of pinning the slot the select was creating. Selects stay concurrent because supersession needs a newer select to reach the renderer while an older one is still in flight. + +## Editing files + +Windows tests in 2026-10 (Node 22.22.3) showed an atomic Node rename failing with EPERM while the raw grant retained a descriptor. A protected owner-only document also acquired its parent directory's broader DACL after the ordinary rename path. Native replacement succeeded with the original grant still open and retained the document ACL. [Microsoft's ReplaceFileW contract](https://learn.microsoft.com/windows/win32/api/winbase/nf-winbase-replacefilew) records the metadata and ACL merge; ignoring merge errors would weaken the permission guarantee. + +Microsoft documents ReplaceFile failures that have already moved the original or replacement file (1176/1177), as well as a native commit preceding a lost helper reply. Deleting the draft or backup after an unconfirmed replacement can therefore delete the only surviving bytes. Confirmed saves and failures before replacement still delete both. + +The Windows draft inherits its directory's ACL, so a protected document in a more broadly readable directory exposes its unsaved draft to that directory's readers until replacement merges the document's ACL. An owner-only stage was tried and dropped (2026-10): creating it needed a PowerShell `Add-Type` P/Invoke to `CreateDirectoryW`, which compiled C# and spawned PowerShell a second time on every save, failed under Constrained Language Mode, and protected draft bytes only in that rare configuration. diff --git a/docs/specs/security-local.md b/docs/specs/security-local.md index 3d59f6b22..f4f1fe5a4 100644 --- a/docs/specs/security-local.md +++ b/docs/specs/security-local.md @@ -205,7 +205,9 @@ unlinked as it is read (`docs/compatible-agents.md` -> "Recovery record"). **The VS Code peer-link token is a local credential at rest** — `burrow.peer-token` in the extension's global storage, written mode `0600` with `wx`, its socket directory re-checked on every contention round. **Neither -control does anything on Windows** (rationale). +control does anything on Windows** (rationale). VS Code's `tool-trust` receipts +likewise inherit the extension storage ACL; Dormouse applies no dedicated +Windows DACL to them. **The standalone log is unprotected and names the control socket.** `$DORMOUSE_LOG_FILE`, else `%LOCALAPPDATA%\Dormouse Terminal\dormouse.log`, else diff --git a/docs/specs/security.md b/docs/specs/security.md index 8ae66e8d1..e7f2ad74f 100644 --- a/docs/specs/security.md +++ b/docs/specs/security.md @@ -118,10 +118,11 @@ Gaps rather than accepted risks: we intend to close them. WebSocket cookie headers are stripped, but `document.cookie` remains shared; cookie-authenticated iframe pages are unsupported ([Loopback Listeners](./security-local.md#loopback-listeners)). -- **Neither VS Code's peer-link token nor the `recovery.json` beside it carries - a Windows ACL applied by Dormouse.** Both are written owner-only by unix - mode, which Windows makes a no-op; standalone locks its state directory - instead ([Persisted state](./security-local.md#persisted-state)). +- **Neither VS Code's peer-link token, its Tool trust receipts, nor the + `recovery.json` beside them carries a Windows ACL applied by Dormouse.** + They are written owner-only by unix mode, which Windows makes a no-op; + standalone locks its state directory instead + ([Persisted state](./security-local.md#persisted-state)). - **The standalone log file is written at the umask** and records the `dor` socket path ([Persisted state](./security-local.md#persisted-state)). - **Revocation has no mechanism.** Revoking a lost phone is editing the Burrow's diff --git a/dor-lib-common/test/spawn.test.mjs b/dor-lib-common/test/spawn.test.mjs index 58449c414..9412dca86 100644 --- a/dor-lib-common/test/spawn.test.mjs +++ b/dor-lib-common/test/spawn.test.mjs @@ -65,7 +65,9 @@ test('drains normal command output before releasing capture pipes', async () => test('releases inherited pipes so the capture caller can exit while the daemon lives', async () => { // This test launches a capture caller, whose short-lived command starts a // daemon sharing its pipes. A resolved promise alone does not prove that the - // caller's event loop can exit. + // caller's event loop can exit. Detach the daemon from the short-lived + // Windows console too; inheriting capture pipes alone does not make it + // independent of the command's console lifetime. const daemonScript = ` process.stdout.on('error', () => {}); process.stderr.on('error', () => {}); @@ -78,7 +80,7 @@ test('releases inherited pipes so the capture caller can exit while the daemon l const commandScript = ` const { spawn } = require('node:child_process'); const daemon = spawn(process.execPath, ['-e', ${JSON.stringify(daemonScript)}, String(process.pid)], { - stdio: ['ignore', 'inherit', 'inherit'], windowsHide: true, + stdio: ['ignore', 'inherit', 'inherit'], windowsHide: true, detached: true, }); daemon.unref(); process.stdout.write(String(daemon.pid)); @@ -123,7 +125,7 @@ test('runs relative paths in the requested cwd without changing the caller cwd', try { const result = await spawnAndCapture(node, ['-e', 'process.stdout.write(process.cwd())'], { cwd }); assert.equal(result.ok, true); - assert.equal(result.stdout, await realpath(cwd)); + assert.equal(await realpath(result.stdout), await realpath(cwd)); assert.equal(process.cwd(), before); } finally { await rm(cwd, { recursive: true, force: true }); } }); diff --git a/dor-tools-builtin/src/atomic-save.ts b/dor-tools-builtin/src/atomic-save.ts new file mode 100644 index 000000000..3126f8ca7 --- /dev/null +++ b/dor-tools-builtin/src/atomic-save.ts @@ -0,0 +1,33 @@ +import { rename } from 'node:fs/promises'; +import { execFile } from 'node:child_process'; +import { win32 } from 'node:path'; +import { promisify } from 'node:util'; + +export interface SaveFileOperations { + /** Move `temporary` over `target`; on Windows the original moves to `backup`. */ + replace(temporary: string, target: string, backup: string): Promise; +} + +// A Node rename on Windows fails with EPERM while the file viewer holds the +// target open, and drops the document's DACL. ReplaceFile succeeds with that +// descriptor open and merges the original's ACL and metadata. The fixed script +// receives paths as environment data. Any failure, including a timeout, may +// follow a partial or committed replacement, so the caller keeps both files. +const REPLACE = '$ErrorActionPreference = "Stop"; $ProgressPreference = "SilentlyContinue"; [IO.File]::Replace($env:DORMOUSE_SAVE_SIBLING, $env:DORMOUSE_SAVE_TARGET, $env:DORMOUSE_SAVE_BACKUP, $false)'; + +export function windowsReplaceCommand(temporary: string, target: string, backup: string) { + return { + // The absolute system .exe needs no PATHEXT or batch-shim handling. + file: win32.join(process.env.SystemRoot || process.env.SYSTEMROOT || 'C:\\Windows', 'System32', 'WindowsPowerShell', 'v1.0', 'powershell.exe'), + args: ['-NoLogo', '-NoProfile', '-NonInteractive', '-EncodedCommand', Buffer.from(REPLACE, 'utf16le').toString('base64')], + env: { ...process.env, DORMOUSE_SAVE_SIBLING: temporary, DORMOUSE_SAVE_TARGET: target, DORMOUSE_SAVE_BACKUP: backup }, + }; +} + +export const saveFileOperations: SaveFileOperations = { + async replace(temporary, target, backup) { + if (process.platform !== 'win32') { await rename(temporary, target); return; } + const { file, args, env } = windowsReplaceCommand(temporary, target, backup); + await promisify(execFile)(file, args, { env, windowsHide: true, timeout: 10_000, maxBuffer: 4096 }); + }, +}; diff --git a/dor-tools-builtin/src/editable-file.ts b/dor-tools-builtin/src/editable-file.ts index 765a6c863..b01a20ca3 100644 --- a/dor-tools-builtin/src/editable-file.ts +++ b/dor-tools-builtin/src/editable-file.ts @@ -1,7 +1,8 @@ import { constants } from 'node:fs'; -import { open, realpath, rename, unlink, type FileHandle } from 'node:fs/promises'; +import { open, realpath, unlink, type FileHandle } from 'node:fs/promises'; import { createHash, randomBytes } from 'node:crypto'; import { dirname, join } from 'node:path'; +import { saveFileOperations, type SaveFileOperations } from './atomic-save.js'; import { HttpError } from './viewer-server.js'; export const TEXT_LIMIT = 8 * 1024 * 1024; @@ -46,10 +47,10 @@ export async function readEditableFile(target: string): Promise<{ text: string; catch { throw new HttpError(415, 'This editor supports UTF-8 text. Open this file in another editor.'); } } -/** Optimistic concurrency: write and flush a private sibling, recheck contents +/** Optimistic concurrency: write and flush a sibling, recheck contents * and identity, then replace atomically. No arbitrary destination or force API. * Like other local editors, this cannot lock out an uncooperative writer. */ -export async function saveEditableFile(target: string, text: string, version: string) { +export async function saveEditableFile(target: string, text: string, version: string, operations: SaveFileOperations = saveFileOperations) { const current = await readEditableBytes(target); if (current.version !== version) throw new HttpError(409, 'The file changed on disk. Your edits are safe here; compare or reload before saving.'); const bom = current.bytes.subarray(0, UTF8_BOM.length).equals(UTF8_BOM); @@ -61,8 +62,10 @@ export async function saveEditableFile(target: string, text: string, version: st const stat = await writable.stat(); if (!stat.isFile() || stat.dev !== current.stat.dev || stat.ino !== current.stat.ino) throw new HttpError(409, 'The file changed on disk.'); } finally { await writable.close(); } - const temporary = join(dirname(target), `.dor-save-${randomBytes(16).toString('hex')}.tmp`); + const name = join(dirname(target), `.dor-save-${randomBytes(16).toString('hex')}`); + const temporary = `${name}.tmp`, backup = `${name}.orig`; const file = await open(temporary, constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL, 0o600); + let cleanup = true; try { try { await file.writeFile(bytes); @@ -74,9 +77,18 @@ export async function saveEditableFile(target: string, text: string, version: st || latest.stat.mtimeMs !== current.stat.mtimeMs || latest.stat.ctimeMs !== current.stat.ctimeMs) { throw new HttpError(409, 'The file changed on disk. Reopen or reload it before saving.'); } - await rename(temporary, target); + // Once native replacement starts, an error may mean a partial rename or + // a committed save whose reply was lost. Keep every recovery byte until + // replacement is confirmed; never delete a possible sole surviving copy. + cleanup = false; + try { await operations.replace(temporary, target, backup); } + catch { + const recovery = process.platform === 'win32' ? `${temporary} and ${backup}` : temporary; + throw new HttpError(500, `The save could not be confirmed. Recovery files were kept at ${recovery}. Reload before saving again.`); + } + cleanup = true; return { version: revision(bytes) }; } finally { - await unlink(temporary).catch(() => {}); + if (cleanup) await Promise.all([temporary, backup].map(path => unlink(path).catch(() => {}))); } } diff --git a/dor-tools-builtin/src/folder-viewer.ts b/dor-tools-builtin/src/folder-viewer.ts index 650cbbdda..b179c2eee 100644 --- a/dor-tools-builtin/src/folder-viewer.ts +++ b/dor-tools-builtin/src/folder-viewer.ts @@ -169,8 +169,16 @@ export async function runFolderViewer(dir: string): Promise { * page shows, rather than throwing into a 500. */ export function oscOpen(write: (text: string) => void): FolderOpen { return async (path, preview) => { - if (!validToolOpenPath(path)) return { ok: false, error: `Cannot open ${JSON.stringify(path)} from a Tool` }; - write(openSequence({ path, preview })); + let sequence: string; + try { + if (!validToolOpenPath(path)) throw new RangeError('invalid Tool open path'); + sequence = openSequence({ path, preview }); + } catch { + // A field can fit its own bound while JSON escaping exceeds the host's + // serialized payload cap. Report that refusal to the page without a write. + return { ok: false, error: `Cannot open ${JSON.stringify(path)} from a Tool` }; + } + write(sequence); return { ok: true, status: 'sent' }; }; } diff --git a/dor-tools-builtin/test/atomic-save.test.mjs b/dor-tools-builtin/test/atomic-save.test.mjs new file mode 100644 index 000000000..3a6c85580 --- /dev/null +++ b/dor-tools-builtin/test/atomic-save.test.mjs @@ -0,0 +1,73 @@ +import assert from 'node:assert/strict'; +import { mkdtemp, realpath, rm, readFile, readdir, writeFile, rename } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join, win32 } from 'node:path'; +import { spawn } from 'node:child_process'; +import { once } from 'node:events'; +import { beforeEach, afterEach, test } from 'node:test'; +import { saveFileOperations, windowsReplaceCommand } from '../dist/atomic-save.js'; +import { readEditableFile, saveEditableFile } from '../dist/editable-file.js'; +let root, file; +beforeEach(async () => { root=await realpath(await mkdtemp(join(tmpdir(),'dor-save-platform-'))); file=join(root,'source.txt'); await writeFile(file,'original'); }); +afterEach(async () => { await rm(root,{recursive:true,force:true}); }); + +test('the Windows replacement passes paths as environment data, never script text', () => { + const [temporary,target,backup]=["C:\\d\\.dor-save-1.tmp","C:\\d\\it's $(calc).txt","C:\\d\\.dor-save-1.orig"]; + const command=windowsReplaceCommand(temporary,target,backup); + assert.match(command.file,/System32\\WindowsPowerShell\\v1\.0\\powershell\.exe$/); + assert.deepEqual(command.args.slice(0,4),['-NoLogo','-NoProfile','-NonInteractive','-EncodedCommand']); + const script=Buffer.from(command.args[4],'base64').toString('utf16le'); + assert.match(script,/\[IO\.File\]::Replace\(\$env:DORMOUSE_SAVE_SIBLING, \$env:DORMOUSE_SAVE_TARGET, \$env:DORMOUSE_SAVE_BACKUP, \$false\)$/); + assert.ok(!script.includes('calc')); + assert.equal(command.env.DORMOUSE_SAVE_SIBLING,temporary); + assert.equal(command.env.DORMOUSE_SAVE_TARGET,target); + assert.equal(command.env.DORMOUSE_SAVE_BACKUP,backup); +}); + +const saveFiles=async()=>(await readdir(root)).filter(name=>name.startsWith('.dor-save-')); + +test('a native replacement sharing failure preserves the old file and retains the draft', { skip:process.platform!=='win32', timeout:15000 }, async () => { + const source=await readEditableFile(file); + const script='$ErrorActionPreference="Stop"; $f=[IO.File]::Open($env:DORMOUSE_LOCK_TEST_TARGET,[IO.FileMode]::Open,[IO.FileAccess]::Read,[IO.FileShare]::ReadWrite); [Console]::Out.WriteLine("ready"); [Console]::Out.Flush(); [Console]::ReadLine() | Out-Null; $f.Dispose()'; + const binary=win32.join(process.env.SystemRoot||process.env.SYSTEMROOT||'C:\\Windows','System32/WindowsPowerShell/v1.0/powershell.exe'); + const child=spawn(binary,['-NoLogo','-NoProfile','-NonInteractive','-EncodedCommand',Buffer.from(script,'utf16le').toString('base64')],{env:{...process.env,DORMOUSE_LOCK_TEST_TARGET:file},stdio:['pipe','pipe','pipe'],windowsHide:true}); + const exited=once(child,'exit'); + try { + await Promise.race([once(child.stdout,'data'),exited.then(()=>{throw new Error('lock helper exited');})]); + await assert.rejects(saveEditableFile(file,'new',source.version), /could not be confirmed/); + assert.equal(await readFile(file,'utf8'),'original'); + const drafts=(await saveFiles()).filter(name=>name.endsWith('.tmp')); + assert.equal(drafts.length,1); + assert.equal(await readFile(join(root,drafts[0]),'utf8'),'new'); + } finally { child.stdin.end('\n'); await exited; } +}); + +for (const partial of ['target-missing', 'committed-without-confirmation']) { + test('retains both versions after a native-like ' + partial + ' replacement outcome', async () => { + const source=await readEditableFile(file); + let draft, original; + await assert.rejects(saveEditableFile(file,'new',source.version,{ + async replace(temporary,target,backup) { + draft=temporary; original=backup; + await rename(target,backup); + if (partial==='committed-without-confirmation') await rename(temporary,target); + throw new Error('native partial replacement or lost completion'); + }, + }),error=>error.status===500&&error.message.includes(draft)); + assert.equal(await readFile(original,'utf8'),'original'); + if (partial==='target-missing') { + await assert.rejects(readFile(file),{code:'ENOENT'}); + assert.equal(await readFile(draft,'utf8'),'new'); + } else assert.equal(await readFile(file,'utf8'),'new'); + assert.equal((await saveFiles()).length,partial==='target-missing'?2:1); + }); +} + +test('a confirmed replacement removes the draft and the backup', async () => { + const source=await readEditableFile(file); + await saveEditableFile(file,'new',source.version,{ + async replace(temporary,target,backup) { await rename(target,backup); await rename(temporary,target); }, + }); + assert.equal(await readFile(file,'utf8'),'new'); + assert.deepEqual(await saveFiles(),[]); +}); diff --git a/dor-tools-builtin/test/editable-file.test.mjs b/dor-tools-builtin/test/editable-file.test.mjs index edd1ba2a1..e3600a211 100644 --- a/dor-tools-builtin/test/editable-file.test.mjs +++ b/dor-tools-builtin/test/editable-file.test.mjs @@ -1,9 +1,11 @@ import assert from 'node:assert/strict'; -import { mkdtemp, realpath, rm, readFile, writeFile, symlink, rename, stat } from 'node:fs/promises'; +import { mkdtemp, realpath, rm, readFile, writeFile, symlink, rename, stat, readdir } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, test } from 'node:test'; import { readEditableFile, saveEditableFile } from '../dist/editable-file.js'; +import { execFileSync } from 'node:child_process'; +import { win32 } from 'node:path'; let root, file; beforeEach(async () => { root = await realpath(await mkdtemp(join(tmpdir(), 'dor-edit-'))); file = join(root, 'source.ts'); }); afterEach(async () => { await rm(root, { recursive: true, force: true }); }); @@ -39,3 +41,38 @@ test('refuses a substituted symlink and invalid UTF-8 without changing either fi await rm(file); await writeFile(file, Buffer.from([255, 254, 0])); await assert.rejects(readEditableFile(file), { status: 415 }); }); + + +function powershell(script, target) { + const binary = win32.join(process.env.SystemRoot || process.env.SYSTEMROOT || 'C:\\Windows', 'System32/WindowsPowerShell/v1.0/powershell.exe'); + return execFileSync(binary, ['-NoLogo', '-NoProfile', '-NonInteractive', '-EncodedCommand', Buffer.from('$ErrorActionPreference="Stop"; $ProgressPreference="SilentlyContinue"; ' + script, 'utf16le').toString('base64')], { + env: { ...process.env, DORMOUSE_PERMISSION_TEST_TARGET: target }, encoding: 'utf8', windowsHide: true, timeout: 10_000, + }).trim(); +} +// Compare protection and ACEs; Windows may reserialize the cosmetic AI flag. +const security = target => powershell('$acl=[IO.File]::GetAccessControl($env:DORMOUSE_PERMISSION_TEST_TARGET); [pscustomobject]@{ protected=$acl.AreAccessRulesProtected; rules=@($acl.GetAccessRules($true,$true,[Security.Principal.SecurityIdentifier]) | ForEach-Object { [pscustomobject]@{ sid=$_.IdentityReference.Value; rights=[int]$_.FileSystemRights; type=[int]$_.AccessControlType; inherited=$_.IsInherited; inheritance=[int]$_.InheritanceFlags; propagation=[int]$_.PropagationFlags } }) } | ConvertTo-Json -Depth 4 -Compress', target); + +for (const broad of [false, true]) test('Windows saves preserve ' + (broad ? 'shared' : 'owner-only') + ' document permissions', { skip: process.platform !== 'win32' }, async () => { + file = join(root, "source' $literal.ts"); + await writeFile(file, 'original'); + powershell('$p=$env:DORMOUSE_PERMISSION_TEST_TARGET; $acl=[IO.File]::GetAccessControl($p); $acl.SetAccessRuleProtection($true,$false); $sid=[Security.Principal.WindowsIdentity]::GetCurrent().User; $acl.AddAccessRule([Security.AccessControl.FileSystemAccessRule]::new($sid,"FullControl","Allow")); [IO.File]::SetAccessControl($p,$acl)', file); + if (broad) powershell('$p=$env:DORMOUSE_PERMISSION_TEST_TARGET; $acl=[IO.File]::GetAccessControl($p); $acl.AddAccessRule([Security.AccessControl.FileSystemAccessRule]::new([Security.Principal.SecurityIdentifier]::new("S-1-1-0"),"Read","Allow")); [IO.File]::SetAccessControl($p,$acl)', file); + const before = security(file); + const source = await readEditableFile(file); + await saveEditableFile(file, 'saved', source.version); + assert.equal(await readFile(file, 'utf8'), 'saved'); + assert.equal(security(file), before); + assert.deepEqual(await readdir(root), ["source' $literal.ts"]); +}); + +test('a replacement failure preserves prior bytes and permissions and retains the draft', async () => { + await writeFile(file, 'original'); + const source = await readEditableFile(file); + const before = process.platform === 'win32' ? security(file) : (await stat(file)).mode; + await assert.rejects(saveEditableFile(file, 'new', source.version, { + replace: async () => { throw new Error('replacement refused'); }, + }), /could not be confirmed/); + assert.equal(await readFile(file, 'utf8'), 'original'); + assert.equal(process.platform === 'win32' ? security(file) : (await stat(file)).mode, before); + assert.equal((await readdir(root)).filter(name => name.startsWith('.dor-save-')).length, 1); +}); diff --git a/dor-tools-builtin/test/file-viewer.test.mjs b/dor-tools-builtin/test/file-viewer.test.mjs index 87238a89b..8949a267d 100644 --- a/dor-tools-builtin/test/file-viewer.test.mjs +++ b/dor-tools-builtin/test/file-viewer.test.mjs @@ -210,3 +210,18 @@ test('titles a viewer with its target\'s basename, controls stripped', () => { // C0 (BEL, ESC), DEL, and C1 (NEL, CSI, ST) could end or open a sequence. assert.equal(viewerTitle(join(root, 'a\x07\x1b]2;x\x7f\u0085\u009b\u009cb.txt')), 'a]2;xb.txt'); }); + + +test('editor saves atomically while its retained raw grant still reads the original file', async () => { + const viewer = await start('save.ts', 'original'); + const sourcePath = viewer.path.replace(/view$/, 'source'); + const first = JSON.parse((await get(viewer, sourcePath)).body); + const saved = await post(viewer, 'save', { text: 'edited', version: first.version }); + assert.equal(saved.status, 200, saved.body); + const next = JSON.parse((await get(viewer, sourcePath)).body); + assert.equal(next.text, 'edited'); + assert.equal(next.version, JSON.parse(saved.body).version); + const raw = await get(viewer, viewer.path.replace(/view$/, 'file/save.ts')); + assert.equal(raw.status, 200); + assert.equal(raw.body, 'original'); +}); diff --git a/dor-tools-builtin/test/folder-viewer.test.mjs b/dor-tools-builtin/test/folder-viewer.test.mjs index 5b54fb420..39f717e02 100644 --- a/dor-tools-builtin/test/folder-viewer.test.mjs +++ b/dor-tools-builtin/test/folder-viewer.test.mjs @@ -344,3 +344,15 @@ test('writes each open as OSC 367, and answers an error for a path the host woul } assert.equal(written.length, 1); }); + + +test('answers a page-visible error when JSON escaping makes an open payload too large', async () => { + const written = []; + const open = oscOpen(text => written.push(text)); + for (const path of ['/' + '"'.repeat(2047), 'C:' + '\\'.repeat(2046)]) { + const result = await open(path, false); + assert.equal(result.ok, false); + assert.match(result.error, /^Cannot open /); + } + assert.deepEqual(written, []); +}); diff --git a/dor-tools-lib/src/frame.ts b/dor-tools-lib/src/frame.ts index a11caf3da..fccedbb1c 100644 --- a/dor-tools-lib/src/frame.ts +++ b/dor-tools-lib/src/frame.ts @@ -42,9 +42,20 @@ export function connectToolFrame({ dirty, save }: ToolFrameOptions, scope: Windo } if (!host || event.origin !== host.origin || message.connection !== host.connection || dirty() === undefined) return; const { request } = message; - save().then( - () => post({ kind: 'saved', request, dirty: dirty() ?? true }), - reason => post({ kind: 'saved', request, error: reason instanceof Error ? reason.message : String(reason), dirty: dirty() ?? true }), + // A new connect (even with the same nonce) or close retires this generation. + // Request ids can repeat on a replacement connection; old disk writes may + // finish, but their replies must never settle that connection's save. + const savingHost = host; + const completed = (error?: string) => { + if (host !== savingHost) return; + post({ kind: 'saved', request, dirty: dirty() ?? true, ...(error === undefined ? {} : { error }) }); + }; + let result: Promise; + try { result = save(); } + catch (reason) { completed(reason instanceof Error ? reason.message : String(reason)); return; } + Promise.resolve(result).then( + () => completed(), + reason => completed(reason instanceof Error ? reason.message : String(reason)), ); }; scope.addEventListener('message', receive); diff --git a/dor-tools-lib/src/osc.ts b/dor-tools-lib/src/osc.ts index ac60e5231..74752603b 100644 --- a/dor-tools-lib/src/osc.ts +++ b/dor-tools-lib/src/osc.ts @@ -26,7 +26,7 @@ export type ToolAnnounce = { port: number | null; /** Same-origin path/query for the discovered port; never an authority. */ path?: string; - /** Title candidate, feeding the existing channel in terminal-state.md. */ + /** Reserved announced-name title candidate; currently retained but inert. */ name: string | null; /** Re-key request. Never dedupes — a runtime re-key only re-labels its own * Surface, because a late collision between two Surfaces that both hold work @@ -102,7 +102,7 @@ export function parseToolAnnounce(content: string): ToolAnnounce | null { /** Reject authority changes rather than trying to repair process output. */ export function validToolServePath(value: unknown): value is string { return typeof value === 'string' && value.length <= 2048 && value.startsWith('/') - && !value.startsWith('//') && !/[\\\u0000-\u0020\u007f]/.test(value); + && !value.startsWith('//') && !/[\\\u0000-\u0020\u007f-\u009f]/.test(value); } export interface ToolState { dirty: boolean } @@ -146,6 +146,7 @@ export function serveSequence({ port, path }: { port: number; path?: string }): /** The `state` report a Tool writes whenever its unsaved state changes. */ export function stateSequence({ dirty }: ToolState): string { + if (typeof dirty !== 'boolean') throw new TypeError('dirty must be a boolean'); return sequence('state', { v: 1, dirty }); } @@ -154,7 +155,14 @@ export function stateSequence({ dirty }: ToolState): string { * a failure shows in the preview slot. Throws on a path the host would ignore. */ export function openSequence({ path, preview = false }: { path: string; preview?: boolean }): string { if (!validToolOpenPath(path)) throw new RangeError(`not an absolute path: ${JSON.stringify(path)}`); + if (typeof preview !== 'boolean') throw new TypeError('preview must be a boolean'); return sequence('open', { v: 1, path, preview }); } -const sequence = (verb: string, payload: object) => `\x1b]367;${verb};${JSON.stringify(payload)}\x07`; +function sequence(verb: string, payload: object): string { + // JSON escaping can expand a field beyond its own bound. The host caps the + // serialized payload before parsing, so never emit a sequence it will ignore. + const raw = JSON.stringify(payload); + if (raw.length > PAYLOAD_LIMIT) throw new RangeError('Tool payload exceeds its serialized size limit'); + return `\x1b]367;${verb};${raw}\x07`; +} diff --git a/dor-tools-lib/test/frame.test.mjs b/dor-tools-lib/test/frame.test.mjs index a72ef6ec2..fa087e62c 100644 --- a/dor-tools-lib/test/frame.test.mjs +++ b/dor-tools-lib/test/frame.test.mjs @@ -63,3 +63,70 @@ test('close stops answering', () => { window.deliver({ dorTool: 1, kind: 'connect', connection: 'd' }); assert.equal(posted.length, 1); }); + + +test('save snapshots current editor state before a synchronous reconnect', async () => { + const { window, posted } = scope(); + let text = 'old'; + const saved = []; + connectToolFrame({ dirty: () => false, save: () => { saved.push(text); return Promise.resolve(); } }, window); + window.deliver({ dorTool: 1, kind: 'connect', connection: 'old' }); + window.deliver({ dorTool: 1, kind: 'save', connection: 'old', request: '1' }); + text = 'new'; + window.deliver({ dorTool: 1, kind: 'connect', connection: 'new' }); + await settle(); + assert.deepEqual(saved, ['old']); + assert.equal(posted.filter(({ message }) => message.kind === 'saved').length, 0); +}); + + +for (const outcome of ['resolve', 'reject']) { + test('an old ' + outcome + ' cannot answer a replacement connection with a reused save id', async () => { + const { window, posted } = scope(); + const saves = []; + connectToolFrame({ dirty: () => false, save: () => new Promise((resolve, reject) => saves.push({ resolve, reject })) }, window); + window.deliver({ dorTool: 1, kind: 'connect', connection: 'old' }); + window.deliver({ dorTool: 1, kind: 'save', connection: 'old', request: '1' }); + await settle(); + window.deliver({ dorTool: 1, kind: 'connect', connection: 'new' }); + window.deliver({ dorTool: 1, kind: 'save', connection: 'new', request: '1' }); + await settle(); + saves[0][outcome](outcome === 'reject' ? new Error('Old failure') : undefined); + await settle(); + assert.equal(posted.filter(({ message }) => message.kind === 'saved').length, 0); + saves[1].resolve(); + await settle(); + assert.deepEqual(posted.at(-1).message, { dorTool: 1, connection: 'new', kind: 'saved', request: '1', dirty: false }); + }); +} + +test('a repeated connect retires in-flight saves even when its nonce is unchanged', async () => { + const { window, posted } = scope(); + let complete; + connectToolFrame({ dirty: () => false, save: () => new Promise(resolve => { complete = resolve; }) }, window); + window.deliver({ dorTool: 1, kind: 'connect', connection: 'same' }); + window.deliver({ dorTool: 1, kind: 'save', connection: 'same', request: '1' }); + await settle(); + window.deliver({ dorTool: 1, kind: 'connect', connection: 'same' }); + complete(); await settle(); + assert.equal(posted.filter(({ message }) => message.kind === 'saved').length, 0); +}); + +test('close retires in-flight save replies', async () => { + const { window, posted } = scope(); + let complete; + const frame = connectToolFrame({ dirty: () => false, save: () => new Promise(resolve => { complete = resolve; }) }, window); + window.deliver({ dorTool: 1, kind: 'connect', connection: 'c' }); + window.deliver({ dorTool: 1, kind: 'save', connection: 'c', request: '1' }); + await settle(); frame.close(); complete(); await settle(); + assert.equal(posted.filter(({ message }) => message.kind === 'saved').length, 0); +}); + +test('a save that throws before returning a promise answers with an error', async () => { + const { window, posted } = scope(); + connectToolFrame({ dirty: () => true, save: () => { throw new Error('Write failed'); } }, window); + window.deliver({ dorTool: 1, kind: 'connect', connection: 'c' }); + window.deliver({ dorTool: 1, kind: 'save', connection: 'c', request: '1' }); + await settle(); + assert.deepEqual(posted.at(-1).message, { dorTool: 1, connection: 'c', kind: 'saved', request: '1', dirty: true, error: 'Write failed' }); +}); diff --git a/dor-tools-lib/test/osc.test.mjs b/dor-tools-lib/test/osc.test.mjs index 9c160a197..11ad308ae 100644 --- a/dor-tools-lib/test/osc.test.mjs +++ b/dor-tools-lib/test/osc.test.mjs @@ -110,3 +110,30 @@ test('serveSequence refuses a value the host would ignore', () => { for (const port of [0, 65536, 1.5]) assert.throws(() => serveSequence({ port }), RangeError); for (const path of ['relative', '//evil.test/', '/a b']) assert.throws(() => serveSequence({ port: 1, path }), RangeError); }); + + +test('stateSequence rejects runtime non-boolean dirty values', () => { + for (const dirty of ['true', 1, null, undefined]) assert.throws(() => stateSequence({ dirty }), TypeError); +}); + +test('openSequence rejects runtime non-boolean preview values', () => { + for (const preview of ['true', 1, null]) assert.throws(() => openSequence({ path: '/a', preview }), TypeError); +}); + +test('serveSequence bounds serialized JSON after escaping a valid path', () => { + assert.equal(parseToolAnnounce(content(serveSequence({ port: 1, path: '/' + 'a'.repeat(2047) })))?.path.length, 2048); + assert.throws(() => serveSequence({ port: 1, path: '/' + '"'.repeat(2047) }), RangeError); +}); + +test('openSequence bounds serialized JSON after escaping a valid path', () => { + assert.equal(parseToolOpen(content(openSequence({ path: '/' + 'a'.repeat(2047) })))?.path.length, 2048); + assert.throws(() => openSequence({ path: 'C:' + '\\'.repeat(2046) }), RangeError); +}); + +test('serve paths reject C1 controls that could terminate or inject OSCs', () => { + for (let code = 0x80; code <= 0x9f; code++) { + const path = '/a' + String.fromCharCode(code) + 'b'; + assert.throws(() => serveSequence({ port: 1, path }), RangeError); + assert.equal(parseToolAnnounce(serve({ port: 1, path }))?.path, undefined); + } +}); diff --git a/dor/src/commands/browser-cli.ts b/dor/src/commands/browser-cli.ts index 50a660099..0d9e16b8a 100644 --- a/dor/src/commands/browser-cli.ts +++ b/dor/src/commands/browser-cli.ts @@ -114,14 +114,9 @@ export function extractSessionFlags( * `surface:` handles resolve via the host port scan, a bare `:port`/`host:port` * sugars to http. Non-navigation commands and plain URLs pass through unchanged. * - * The target is matched by shape (not position), which is what lets `open - * --headed surface:3` resolve — dor can't know the provider's flag arity, so it - * can't reliably find "the positional". The trade-off is that a *flag value* - * shaped like a target would be grabbed; this is safe because no agent-browser - * `open` flag takes a `surface:`/`:port`/`host:port`-shaped value (`--headers` is - * JSON, `--init-script` a path, `--enable` a feature name), and `inferredHttpUrl` - * rejects a bare-integer host so a stray `n:n` value can't become a URL. Only the - * first special-shaped arg is rewritten — these verbs take a single target. + * Only the first positional target after the actual navigation command is + * rewritten. Skip known option values; preserve the entire argv when an + * unknown option makes command or target position ambiguous. */ export async function resolveOpenTargetArgs( rest: string[], @@ -129,10 +124,9 @@ export async function resolveOpenTargetArgs( workspace: string | undefined, verbs: ReadonlySet, ): Promise> { - const subcommand = rest.find((arg) => verbs.has(arg)); - if (subcommand === undefined || !verbs.has(subcommand)) return { ok: true, value: rest }; - - const commandIndex = rest.findIndex((arg) => verbs.has(arg)); + const provider = verbs.has('navigate') ? 'agent-browser' : 'playwright'; + const commandIndex = nativeCommandIndex(provider, rest); + if (commandIndex === undefined || !verbs.has(rest[commandIndex]!)) return { ok: true, value: rest }; const valueFlags = verbs.has('navigate') ? AGENT_BROWSER_VALUE_FLAGS : PLAYWRIGHT_OPEN_VALUE_FLAGS; const booleanFlags = verbs.has('navigate') ? AGENT_BROWSER_BOOLEAN_FLAGS : PLAYWRIGHT_OPEN_BOOLEAN_FLAGS; let index = -1; @@ -144,7 +138,9 @@ export async function resolveOpenTargetArgs( if (booleanFlags.has(name)) { if (rest[i + 1] === 'true' || rest[i + 1] === 'false') i += 1; continue; } return { ok: true, value: rest }; } - if (isSpecialOpenTarget(arg)) { index = i; break; } + if (!isSpecialOpenTarget(arg)) return { ok: true, value: rest }; + index = i; + break; } if (index === -1) return { ok: true, value: rest }; @@ -240,8 +236,8 @@ const AGENT_BROWSER_BOOLEAN_FLAGS = new Set([ * destination page run before its viewport is ready. */ function blankNavigationArgs(args: string[]): string[] | Error { const result = [...args]; - const verbIndex = result.findIndex((arg) => arg === 'open' || arg === 'goto' || arg === 'navigate'); - if (verbIndex < 0) return new Error('No navigation command to prepare'); + const verbIndex = nativeCommandIndex('agent-browser', result); + if (verbIndex === undefined || !['open', 'goto', 'navigate'].includes(result[verbIndex]!)) return new Error('No navigation command to prepare'); const positions: number[] = []; for (let i = 0; i < result.length; i += 1) { const arg = result[i] ?? ''; @@ -295,8 +291,8 @@ const PLAYWRIGHT_OPEN_VALUE_FLAGS = new Set(['--browser', '--config', '--device' const PLAYWRIGHT_OPEN_BOOLEAN_FLAGS = new Set(['--headed', '--mobile', '--persistent', '--json', '--raw']); function playwrightDestination(args: string[]): { blank: string[]; goto: string[] } | Error { - const openIndex = args.indexOf('open'); - if (openIndex < 0) return new Error('No playwright open command to prepare'); + const openIndex = nativeCommandIndex('playwright', args); + if (openIndex === undefined || args[openIndex] !== 'open') return new Error('No playwright open command to prepare'); const targetIndices: number[] = []; for (let i = 0; i < args.length; i += 1) { if (i === openIndex) continue; @@ -317,11 +313,12 @@ function playwrightDestination(args: string[]): { blank: string[]; goto: string[ return { blank, goto }; } -function nativeCommand(provider: BrowserAutomationProvider, args: string[]): string | undefined { +/** First command after known options; never mistake a flag value for a verb. */ +function nativeCommandIndex(provider: BrowserAutomationProvider, args: string[]): number | undefined { if (provider === 'playwright') { for (let i = 0; i < args.length; i += 1) { const arg = args[i] ?? ''; - if (!arg.startsWith('-')) return arg; + if (!arg.startsWith('-')) return i; const [name] = arg.split('=', 1); if (PLAYWRIGHT_OPEN_VALUE_FLAGS.has(name)) { if (!arg.includes('=')) i += 1; continue; } if (!PLAYWRIGHT_OPEN_BOOLEAN_FLAGS.has(name)) return undefined; @@ -330,7 +327,7 @@ function nativeCommand(provider: BrowserAutomationProvider, args: string[]): str } for (let i = 0; i < args.length; i += 1) { const arg = args[i] ?? ''; - if (!arg.startsWith('-')) return arg; + if (!arg.startsWith('-')) return i; const [name] = arg.split('=', 1); if (AGENT_BROWSER_VALUE_FLAGS.has(name)) { if (!arg.includes('=')) i += 1; continue; } if (AGENT_BROWSER_BOOLEAN_FLAGS.has(name)) { @@ -342,6 +339,11 @@ function nativeCommand(provider: BrowserAutomationProvider, args: string[]): str return undefined; } +function nativeCommand(provider: BrowserAutomationProvider, args: string[]): string | undefined { + const index = nativeCommandIndex(provider, args); + return index === undefined ? undefined : args[index]; +} + /** * Forward `args` to the provider's CLI against the session the identity flags * name, then open or reuse the Surface bound to it (docs/specs/dor-browser.md diff --git a/dor/test/browser-target.test.mjs b/dor/test/browser-target.test.mjs new file mode 100644 index 000000000..e17b1743a --- /dev/null +++ b/dor/test/browser-target.test.mjs @@ -0,0 +1,29 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { resolveOpenTargetArgs } from '../dist/commands/browser-cli.js'; +const agent = new Set(['open','goto','navigate']); +const playwright = new Set(['open','goto']); + +for (const [verbs, argv] of [ + [agent, ['open','https://example.com','surface:3']], + [agent, ['--init-script','open','screenshot','surface:3']], + [agent, ['--init-script','navigate','snapshot','surface:3']], + [agent, ['--new-option','open','surface:3']], + [playwright, ['--config','open','screenshot','surface:3']], + [playwright, ['open','https://example.com',':3000']], +]) test('preserves argv without resolving a non-target: '+argv.join(' '), async () => { + const requests=[]; + const client={ resolveOpenTarget:async value=>{requests.push(value);return {url:'http://localhost:3000/'};} }; + assert.deepEqual(await resolveOpenTargetArgs(argv,{client},undefined,verbs),{ok:true,value:argv}); + assert.deepEqual(requests,[]); +}); + +for (const [verbs, argv, expected] of [ + [agent, ['--init-script','open','open','--headed','surface:3'], ['--init-script','open','open','--headed','http://localhost:3000/']], + [playwright, ['--config','open','open','--headed','surface:3'], ['--config','open','open','--headed','http://localhost:3000/']], +]) test('resolves the actual target after known option values: '+argv.join(' '), async () => { + const requests=[]; + const client={ resolveOpenTarget:async value=>{requests.push(value);return {url:'http://localhost:3000/'};} }; + assert.deepEqual(await resolveOpenTargetArgs(argv,{client},undefined,verbs),{ok:true,value:expected}); + assert.deepEqual(requests,[{surface:'surface:3'}]); +}); diff --git a/dor/test/browser-viewport.test.mjs b/dor/test/browser-viewport.test.mjs index 9435e5e00..da67d6d37 100644 --- a/dor/test/browser-viewport.test.mjs +++ b/dor/test/browser-viewport.test.mjs @@ -256,3 +256,23 @@ test('playwright device, mobile, and headed options leave viewport choice to nat assert.equal(surfaces[0].initialViewport, undefined); } }); + +for (const provider of ['agent-browser', 'playwright']) test(provider+' viewport preparation skips a pre-command value named open', async () => { + const calls=[]; + const options={ + env:{PWD:process.cwd()}, + client:{ + resolveBrowser:async()=>({binding:{session:'dormouse.1.default'},fresh:true,initialViewport:{mode:'fixed',width:1440,height:900,dpr:2},launchViewport:{width:1440,height:900,dpr:2}}), + browserSurface:async request=>{calls.push(['surface',request]);return {};}, + browserViewport:async()=>({actual:{width:1440,height:900,dpr:2}}), + }, + [provider === 'agent-browser' ? 'execAgentBrowser' : 'execPlaywright']:async(_binary,args)=>{ + calls.push(['exec',args]);return {exitCode:0,stdout:args.includes('eval')?'2\n':'native\n',stderr:''}; + }, + }; + const nativeArgs=provider==='agent-browser'?['--init-script','open','open','http://localhost:5173']:['--config','open','open','http://localhost:5173']; + const result=await runCli([provider,...nativeArgs],options); + assert.equal(result.exitCode,0,result.stderr); + const blank=calls.filter(c=>c[0]==='exec')[0][1].slice(provider==='agent-browser'?2:1); + assert.deepEqual(blank,[nativeArgs[0],'open','open','about:blank']); +}); diff --git a/dor/test/builtin-viewers.test.mjs b/dor/test/builtin-viewers.test.mjs index 22ebac804..c41734286 100644 --- a/dor/test/builtin-viewers.test.mjs +++ b/dor/test/builtin-viewers.test.mjs @@ -88,7 +88,13 @@ test('the file entry titles itself, announces its port and path, serves the stag assert.equal((await call(viewer, `${prefix}assets/${name}`)).status, 200, name); } await terminates(child, viewer); - } finally { child.kill('SIGKILL'); } + } finally { + if (child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'exit'); + child.kill('SIGKILL'); + await exited; + } + } }); test('ordinary staged CLI commands work without the builtin runtime', async () => { @@ -112,7 +118,13 @@ test('the folder entry titles itself, announces its port and path, then exits on assert.equal(viewer.v, 1); assert.deepEqual(JSON.parse((await call(viewer, `${viewer.path}list?dir=`)).body).entries, [{ name: 'a.txt', kind: 'file', ignored: false }]); await terminates(child, viewer); - } finally { child.kill('SIGKILL'); } + } finally { + if (child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'exit'); + child.kill('SIGKILL'); + await exited; + } + } }); test('the folder entry selects and activates with OSC 367 open, in the order the page sends them', { timeout: 10_000 }, async () => { @@ -129,7 +141,13 @@ test('the folder entry selects and activates with OSC 367 open, in the order the { v: 1, path: file, preview: true }, { v: 1, path: file, preview: false }, ]); - } finally { child.kill('SIGKILL'); } + } finally { + if (child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'exit'); + child.kill('SIGKILL'); + await exited; + } + } }); test('the error entry titles itself after its target and serves the escaped message', { timeout: 10_000 }, async () => { @@ -142,5 +160,11 @@ test('the error entry titles itself after its target and serves the escaped mess assert.match(page.body, /no Tool matches <report\.pdf>/); assert.equal((await call(viewer, `${viewer.path}anything`)).status, 404); await terminates(child, viewer); - } finally { child.kill('SIGKILL'); } + } finally { + if (child.exitCode === null && child.signalCode === null) { + const exited = once(child, 'exit'); + child.kill('SIGKILL'); + await exited; + } + } }); diff --git a/dor/test/cli-output.test.mjs b/dor/test/cli-output.test.mjs index c144eb1c1..43957179d 100644 --- a/dor/test/cli-output.test.mjs +++ b/dor/test/cli-output.test.mjs @@ -1832,10 +1832,13 @@ test('list json schema includes ids and refs regardless of id-format', async () }); test('list filters by kind, view, command, and cwd without port scanning', async () => { - const client = fixtureClient(); + const cwd = join(tmpdir(), 'dor-list-site'); + const client = fixtureClient(fixtureSurfaces.map(surface => ({ + ...surface, cwd: surface.cwd === '/Users/me/projects/site' ? cwd : surface.cwd, + }))); const result = await runCli( ['list', '--json', '--kind', 'terminal', '--view', 'paned', '--command', 'pnpm dev', '--cwd', '.'], - { client, env: { ...listEnv, PWD: '/Users/me/projects/site' } }, + { client, env: { ...listEnv, PWD: cwd } }, ); assert.equal(result.exitCode, 0); assert.equal(result.stderr, ''); diff --git a/dor/test/playwright.test.mjs b/dor/test/playwright.test.mjs index e8fa16039..bf1c8daf7 100644 --- a/dor/test/playwright.test.mjs +++ b/dor/test/playwright.test.mjs @@ -58,7 +58,7 @@ test('a pinned executable that is gone runs the caller\'s own, and says so', asy const result = await runCli(['playwright', 'snapshot'], options); assert.deepEqual(calls.find(c => c[0] === 'exec'), ['exec', '/tools/playwright-cli', ['--session=gui-123', 'snapshot'], firstProject]); assert.equal(result.exitCode, 0); - assert.match(result.stderr, new RegExp(`playwright-cli \\(${gone}\\) is gone`)); + assert.ok(result.stderr.includes(`playwright-cli (${gone}) is gone`), result.stderr); // The pane then learns the executable that ran. assert.equal(calls.at(-1)[1].binaryPath, '/tools/playwright-cli'); }); @@ -67,7 +67,7 @@ test('a pinned directory that is gone is named, not reported as a missing playwr const { calls, options } = fixture({ session: 'gui-123', cwd: gone, binaryPath: pinnedCli }); const result = await runCli(['playwright', '--key', 'app', 'snapshot'], options); assert.equal(result.exitCode, 1); - assert.match(result.stderr, new RegExp(`no longer exists: ${gone}`)); + assert.ok(result.stderr.includes(`no longer exists: ${gone}`), result.stderr); assert.doesNotMatch(result.stderr, /not installed/); assert.equal(calls.some(c => c[0] === 'exec'), false); }); diff --git a/lib/src/components/wall/browser-surface.ts b/lib/src/components/wall/browser-surface.ts index c39a1444c..730e11034 100644 --- a/lib/src/components/wall/browser-surface.ts +++ b/lib/src/components/wall/browser-surface.ts @@ -135,7 +135,7 @@ export function toolPendingFromParams(params: unknown): ToolPending | null { } /** - * Which of a tool's faces is forward. A three-state answer rather than a + * Which Tool view is shown. A named state rather than a * boolean because the header and the body must agree: a port conflict occupies * the browser's place (there is nothing to frame, so the pane shows *why* * where the browser would have been) but has no URL to edit, so it must not diff --git a/lib/src/host/atomic-json-file.test.ts b/lib/src/host/atomic-json-file.test.ts new file mode 100644 index 000000000..6791bd7be --- /dev/null +++ b/lib/src/host/atomic-json-file.test.ts @@ -0,0 +1,63 @@ +import { afterEach, beforeEach, expect, it, vi } from 'vitest'; +import { mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const probe = vi.hoisted(() => ({ failures: 0, attempts: 0, code: 'EPERM' })); +vi.mock('node:fs/promises', async (original) => { + const real = await original(); + return { + ...real, + rename: async (from: string, to: string) => { + probe.attempts++; + if (probe.failures-- > 0) throw Object.assign(new Error('rename refused'), { code: probe.code }); + return real.rename(from, to); + }, + }; +}); +import { writeJsonAtomic } from './atomic-json-file'; + +let dir: string; +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'dor-atomic-json-')); + probe.failures = 0; probe.attempts = 0; probe.code = 'EPERM'; +}); +afterEach(async () => { + vi.unstubAllGlobals(); + await rm(dir, { recursive: true, force: true }); +}); + +it('retries brief Windows sharing failures without exposing partial JSON', async () => { + vi.stubGlobal('process', { ...process, platform: 'win32' }); + const file = join(dir, 'state.json'); + await writeFile(file, 'old'); + probe.failures = 2; + await writeJsonAtomic(dir, file, { committed: true }); + expect(probe.attempts).toBe(3); + expect(JSON.parse(await readFile(file, 'utf8'))).toEqual({ committed: true }); + expect(await readdir(dir)).toEqual(['state.json']); +}); + +it('bounds persistent Windows sharing failure, preserves prior bytes, and removes the temporary secret', async () => { + vi.stubGlobal('process', { ...process, platform: 'win32' }); + const file = join(dir, 'state.json'); + await writeFile(file, 'old'); + probe.failures = Infinity; + await expect(writeJsonAtomic(dir, file, { secret: true })).rejects.toMatchObject({ code: 'EPERM' }); + expect(probe.attempts).toBe(11); + expect(await readFile(file, 'utf8')).toBe('old'); + expect(await readdir(dir)).toEqual(['state.json']); +}); + +for (const [platform, code] of [['linux', 'EPERM'], ['win32', 'EIO']]) { + it(`does not retry ${platform} ${code}`, async () => { + vi.stubGlobal('process', { ...process, platform }); + const file = join(dir, 'state.json'); + await writeFile(file, 'old'); + probe.failures = 1; probe.code = code; + await expect(writeJsonAtomic(dir, file, { secret: true })).rejects.toMatchObject({ code }); + expect(probe.attempts).toBe(1); + expect(await readFile(file, 'utf8')).toBe('old'); + expect(await readdir(dir)).toEqual(['state.json']); + }); +} diff --git a/lib/src/host/atomic-json-file.ts b/lib/src/host/atomic-json-file.ts index 17e518716..fc855ea73 100644 --- a/lib/src/host/atomic-json-file.ts +++ b/lib/src/host/atomic-json-file.ts @@ -1,12 +1,14 @@ /** - * The one way a Node-side host commits a private JSON file: owner-only, and - * temp-then-rename so a crash can never publish a half-written one. Shared by + * The atomic JSON writer for host state: temp-then-rename so a crash cannot + * publish a half-written file. Unix modes are set here; Windows writers inherit + * the caller's storage DACL. Shared by * the Burrow state store and the Tool trust store, whose files hold a bearer * credential and a security decision respectively. */ import { randomUUID } from 'node:crypto'; import { chmod, mkdir, rename, rm, writeFile } from 'node:fs/promises'; +import { setTimeout as delay } from 'node:timers/promises'; /** Write `value` as JSON to `path`, creating `dir` (which contains it) first. */ export async function writeJsonAtomic(dir: string, path: string, value: unknown): Promise { @@ -23,8 +25,8 @@ export async function writeJsonAtomic(dir: string, path: string, value: unknown) // below, so neither call protects anything. What protects it there is the // owner-only DACL that `burrow_state_dir` in // `standalone/src-tauri/src/lib.rs` applies to this directory before - // spawning us; the files written below inherit it. Node cannot set an ACL, - // which is why the guarantee lives on the Rust side rather than here. + // spawning us; the files written below inherit it. This writer relies on + // its caller supplying that private parent, rather than invoking an ACL helper. if (process.platform !== 'win32') await chmod(dir, 0o700).catch(() => {}); // Temp-then-rename in the same directory, so a crash mid-write leaves the // previous contents intact rather than a truncated file that reads as empty. @@ -34,7 +36,17 @@ export async function writeJsonAtomic(dir: string, path: string, value: unknown) let renamed = false; try { await writeFile(tmp, JSON.stringify(value), { mode: 0o600 }); - await rename(tmp, path); + // Concurrent replacements can briefly leave a Windows destination pending + // deletion. Retry only that platform's sharing failures, with ten 10ms waits; + // keep the old file intact and propagate persistent or unrelated failures. + for (let attempt = 0; ; attempt++) { + try { await rename(tmp, path); break; } + catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (process.platform !== 'win32' || attempt === 10 || (code !== 'EPERM' && code !== 'EBUSY')) throw error; + await delay(10); + } + } renamed = true; } finally { // A failed rename must not accumulate temp files holding the same secret. diff --git a/lib/src/host/tool-list.ts b/lib/src/host/tool-list.ts index 68c1d62ca..965da4d15 100644 --- a/lib/src/host/tool-list.ts +++ b/lib/src/host/tool-list.ts @@ -1,7 +1,7 @@ /** * `dor tool --list` (`docs/specs/dor-tool.md` -> CLI): the Tools `dor tool * ` would resolve from a directory, each with the comment its author - * wrote above it. Listing reads and executes nothing, so a project needs no + * wrote above it. Listing only reads configuration and executes no Tool, so a project needs no * grant to be listed; the answer reports whether it has one. */ import type { ToolListEntry, ToolListRequest, ToolListResponse } from 'dor/commands/types'; diff --git a/lib/src/host/tool-trust.test.ts b/lib/src/host/tool-trust.test.ts index eea9b577e..5d1e2621c 100644 --- a/lib/src/host/tool-trust.test.ts +++ b/lib/src/host/tool-trust.test.ts @@ -304,10 +304,17 @@ describe('the pre-approval read (regression: review finding 13, PR #493 review)' expect((await lookupTool('storybook', root, new MemoryToolTrustStore(), { resolveUpstream: noUpstream })).status).toBe('untrusted'); }); - it('refuses a symlink instead of following it before trust', async () => { + it('refuses a symlink instead of following it before trust', async (context) => { const target = join(root, 'repo-controlled-target.yml'); await writeFile(target, YML); - await symlink(target, join(root, 'dormouse.yml')); + try { + await symlink(target, join(root, 'dormouse.yml')); + } catch (error) { + if (process.platform === 'win32' && (error as NodeJS.ErrnoException).code === 'EPERM') { + context.skip('Windows file symlinks require Developer Mode or symlink privilege'); + } + throw error; + } const result = await lookupTool('storybook', root, new MemoryToolTrustStore(), { resolveUpstream: noUpstream }); expect(result).toMatchObject({ status: 'error' }); diff --git a/lib/src/stories/HelperPlacement.stories.tsx b/lib/src/stories/HelperPlacement.stories.tsx index 0ff93fcec..d49305889 100644 --- a/lib/src/stories/HelperPlacement.stories.tsx +++ b/lib/src/stories/HelperPlacement.stories.tsx @@ -2,7 +2,7 @@ import type { Meta, StoryObj } from '@storybook/react'; import { expect, userEvent, waitFor, within } from 'storybook/test'; import { Wall } from '../components/Wall'; import { disposeHelper, getHelper } from '../lib/helper-terminal'; -import { getTerminalInstance, refitSession } from '../lib/terminal-registry'; +import { flushTerminal, getTerminalInstance, refitSession } from '../lib/terminal-registry'; import { flattenScenario, SCENARIO_SHELL_PROMPT } from '../lib/platform'; import { leaves, normalizeWeights, type LathNode } from '../lib/lath/model'; import type { LathPersistedLayout } from '../lib/lath/persistence'; @@ -57,7 +57,14 @@ async function rightClickSourceHeader() { async function openContext() { await rightClickSourceHeader(); await settleTerminalContext(); - expect(getHelper(SOURCE)?.status).toBe('completed'); + const helper = getHelper(SOURCE)!; + expect(helper.status).toBe('completed'); + // Protocol completion precedes xterm's async parsing of the command echo. + // Drain that queue before the paint gate captures a wrapped narrow terminal. + let written = false; + void flushTerminal(helper.id).then(() => { written = true; }); + await waitFor(() => expect(written).toBe(true), { timeout: 4000 }); + await settleTerminals(); } function expectedSide({ layout, zoomed, cursor, sourceAtEnd }: Props) { // Alone in the Wall, the helper avoids the cursor; beside a neighbor, it takes the neighbor's side. diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index f9fc124de..19f48d5fe 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -7,7 +7,7 @@ "docs/specs/auto-update.md": 1450, "docs/specs/deploy.md": 1950, "docs/specs/dor-browser.md": 9350, - "docs/specs/dor-cli.md": 6650, + "docs/specs/dor-cli.md": 6600, "docs/specs/dor-tool.md": 6250, "docs/specs/dor-tools-builtin.md": 1100, "docs/specs/dor-tools-lib.md": 400, diff --git a/standalone/sidecar/dor-control-server.js b/standalone/sidecar/dor-control-server.js index afc410824..73a735b31 100644 --- a/standalone/sidecar/dor-control-server.js +++ b/standalone/sidecar/dor-control-server.js @@ -262,7 +262,8 @@ function createDorControlServer({ socketPath, socketDir, token, send, timeoutMs return; } - if (typeof request.requestId !== 'string' || typeof request.method !== 'string') { + if (!request || typeof request !== 'object' || Array.isArray(request) + || typeof request.requestId !== 'string' || typeof request.method !== 'string') { writeResponse(socket, { ok: false, error: 'invalid Dormouse control request' }); return; } diff --git a/standalone/sidecar/dor-control-server.test.js b/standalone/sidecar/dor-control-server.test.js index 57741cc6e..d605bd376 100644 --- a/standalone/sidecar/dor-control-server.test.js +++ b/standalone/sidecar/dor-control-server.test.js @@ -29,7 +29,7 @@ function testSocketPath(name) { * peer that gets the handshake wrong. */ function sendSocketRequest(socketPath, request, options = {}) { - const { token = 'secret', hello, expectLines = 3 } = options; + const { token = 'secret', hello, expectLines = 3, sendNull = false } = options; return new Promise((resolve, reject) => { const socket = net.createConnection({ path: socketPath }); const nonce = 'client-nonce'; @@ -50,7 +50,7 @@ function sendSocketRequest(socketPath, request, options = {}) { nonce, proof: proveToken(token, CLIENT_PROOF_DOMAIN, lines[0].nonce), })}\n`); - } else if (lines.length === 2 && request) { + } else if (lines.length === 2 && (request || sendNull)) { socket.write(`${JSON.stringify(request)}\n`); } if (lines.length >= expectLines) { @@ -516,3 +516,21 @@ test('a lost bind is fatal to the channel, not to the host', { skip: process.pla await assert.rejects(server.ready); server.close(); }); + + +test('authenticated null and non-object requests are rejected without crashing the control server', async () => { + const sent = []; + const server = createDorControlServer({ socketPath: testSocketPath('malformed-request'), token: 'secret', send: (...args) => sent.push(args) }); + await server.ready; + try { + for (const request of [null, true, 42, 'request', []]) { + const { lines } = await sendSocketRequest(server.socketPath, request, { sendNull: true }); + assert.deepEqual(lines[2], { ok: false, error: 'invalid Dormouse control request' }); + } + assert.deepEqual(sent, []); + const response = sendSocketRequest(server.socketPath, { requestId: 'after-invalid', method: 'surface.list' }); + await waitFor(() => sent.length === 1, 'request after malformed input'); + server.respond({ requestId: 'after-invalid', ok: true, result: { surfaces: [] } }); + assert.equal((await response).lines[2].ok, true); + } finally { server.close(); } +});