From f8918627005e7a49c1a0b9f7fe130e7d6f0ec686 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Mon, 19 Oct 2026 15:47:24 -0700 Subject: [PATCH 1/4] Open the standalone log read through open_log The webview's update-report log read opened the log with a plain File::open, so a symlink at the log's name was followed. It now goes through open_log like the write handles: no symlink is followed, and anything but a regular file (a FIFO, a directory) is refused before its mode is touched, with O_NONBLOCK so a FIFO cannot hang the open. Co-Authored-By: Claude Opus 5.5 --- docs/specs/security-local.md | 2 +- docs/specs/security-local.rationale.md | 2 +- standalone/src-tauri/src/lib.rs | 62 +++++++++++++++++++++++--- 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/docs/specs/security-local.md b/docs/specs/security-local.md index 8c30ca403..2d52e47ec 100644 --- a/docs/specs/security-local.md +++ b/docs/specs/security-local.md @@ -161,7 +161,7 @@ The attacker is another local account reading disk; what the remote stack leaves No standalone log call (`docs/specs/standalone.md` -> "Logging") carries PTY bytes, but the log records the `dor` socket path (rationale). - **FAIL IF** `S162` `write_file_atomically` in `standalone/src-tauri/src/lib.rs` stops restricting the directory and the file it writes to the owning user on **every** platform `restrict_to_owner` has an arm for — `0700`/`0600` on unix, and on Windows a DACL protected from inheritance carrying exactly one ACE for the current user — or the mode stops reaching the temp file *before* any bytes are written, or **any** of its callers stops going through it. Enumerate them from the file rather than from this line: every writer under the state root is one, the legacy-transcript scrub and `arrivals.json` included. The one exception is the macOS hang sample `/usr/bin/sample` writes into `hangs/`; it goes through `write_owner_only_with`, which restricts the directory before the child runs and the file after. Pinned by the Rust tests in the same file (rationale). -- **FAIL IF** `S163` a standalone log handle opens other than through `open_log` in `standalone/src-tauri/src/lib.rs`, or `open_log` stops leaving the log owner-only on **every** platform — unix: `0600`, an existing file tightened, a symlink refused; Windows: `restrict_to_owner`'s DACL — or the Linux default is not a per-user directory whenever `XDG_STATE_HOME` or `HOME` is absolute. Pinned by the Rust tests in the same file (rationale). +- **FAIL IF** `S163` a standalone log handle, read or write, opens other than through `open_log` in `standalone/src-tauri/src/lib.rs`, or `open_log` stops leaving the log owner-only on **every** platform — unix: `0600`, an existing file tightened, a symlink or anything but a regular file refused; Windows: `restrict_to_owner`'s DACL — or the Linux default is not a per-user directory whenever `XDG_STATE_HOME` or `HOME` is absolute. Pinned by the Rust tests in the same file (rationale). Source of truth: `SESSION_STATE_KEY` in `vscode-ext/src/session-state.ts`, `ensureToken` in `vscode-ext/src/peer-link.ts`, `default_log_location` in `standalone/src-tauri/src/lib.rs`, `createManagedVoiceHost` in `lib/src/host/managed-voice-host.ts`. diff --git a/docs/specs/security-local.rationale.md b/docs/specs/security-local.rationale.md index de56d1831..77efd1001 100644 --- a/docs/specs/security-local.rationale.md +++ b/docs/specs/security-local.rationale.md @@ -82,7 +82,7 @@ Why session snapshots earn the strongest protection on disk. They are `Persisted Why the peer-link token is listed here. It is a `randomUUID()` in the VS Code extension's global storage, and its own comment says it is the only thing between another local process and this installation's terminals — a local credential at rest that the remote spec's credentials table does not cover, because nothing about it is remote. Both of its controls are unix-only: `peerDirIsSafe()` returns true immediately on `win32`, and Node's `mode: 0o600` there touches only the read-only attribute, so unlike `burrow_state_dir` no DACL work is done for it. -Why the standalone log is guarded at all. The socket path arrives via the sidecar's stderr, which Rust appends verbatim. Linux used to fall back to `env::temp_dir()`, which with `TMPDIR` unset is `/tmp` at `1777`, and still does without a usable `HOME`: another account could pre-plant `dormouse.log` as a symlink, which the truncating open followed, or as a world-writable file of its own, which then collected the log. Kernel `fs.protected_symlinks` / `fs.protected_regular` blunt both only where a distribution enables them (2026-10 audit, #1027). macOS's `$TMPDIR` is the per-user `/var/folders/.../T` at `0700` (measured 2026-09). The open is hardened as well as the directory moved because `$DORMOUSE_LOG_FILE` can point anywhere. +Why the standalone log is guarded at all. The socket path arrives via the sidecar's stderr, which Rust appends verbatim. Linux used to fall back to `env::temp_dir()`, which with `TMPDIR` unset is `/tmp` at `1777`, and still does without a usable `HOME`: another account could pre-plant `dormouse.log` as a symlink, which the truncating open followed, or as a world-writable file of its own, which then collected the log. Kernel `fs.protected_symlinks` / `fs.protected_regular` blunt both only where a distribution enables them (2026-10 audit, #1027). macOS's `$TMPDIR` is the per-user `/var/folders/.../T` at `0700` (measured 2026-09). The open is hardened as well as the directory moved because `$DORMOUSE_LOG_FILE` can point anywhere. The read takes the same open because the webview asks for the log's tail to attach to an update report: a symlink at the log's name would hand it another file's tail, and a FIFO would hang the read. ## Local-file viewer diff --git a/standalone/src-tauri/src/lib.rs b/standalone/src-tauri/src/lib.rs index 818d27cef..3ea6b3eec 100644 --- a/standalone/src-tauri/src/lib.rs +++ b/standalone/src-tauri/src/lib.rs @@ -1071,20 +1071,35 @@ fn prepare_log_dir(path: &Path, own_dir: bool) -> bool { builder.create(dir).is_ok() && restrict_to_owner(dir, 0o700).is_ok() } -/// Open a log handle owner-only. On unix it never follows a symlink at `path`, -/// and it refuses a file another account owns, which it cannot `fchmod`. +/// Open a log handle owner-only, for reading or writing. On unix it never +/// follows a symlink at `path`, and it refuses a file another account owns, +/// which it cannot `fchmod`. Anything but a regular file is refused before its +/// mode is touched. fn open_log(path: &Path, options: &mut OpenOptions) -> std::io::Result { + let regular_only = |file: File| { + if file.metadata()?.is_file() { + Ok(file) + } else { + Err(std::io::Error::other(format!("{} is not a regular file", path.display()))) + } + }; #[cfg(unix)] { use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; - let file = options.mode(0o600).custom_flags(libc::O_NOFOLLOW).open(path)?; + // O_NONBLOCK keeps a FIFO at `path` from hanging the open; it has no + // effect on the regular file that is all this ever returns. + let file = options + .mode(0o600) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) + .open(path)?; + let file = regular_only(file)?; // An existing file keeps its mode through `open`; this tightens it. file.set_permissions(std::fs::Permissions::from_mode(0o600))?; Ok(file) } #[cfg(not(unix))] { - let file = options.open(path)?; + let file = regular_only(options.open(path)?)?; restrict_to_owner(path, 0o600).map_err(std::io::Error::other)?; Ok(file) } @@ -1157,8 +1172,11 @@ fn set_macos_dock_icon() { } fn read_log_tail(max_bytes: usize) -> Result { - let path = log_path(); - File::open(path) + read_log_tail_at(log_path(), max_bytes) +} + +fn read_log_tail_at(path: &Path, max_bytes: usize) -> Result { + open_log(path, OpenOptions::new().read(true)) .and_then(|mut file| log_tail::read_utf8_tail(&mut file, max_bytes)) .map_err(|e| format!("read {}: {e}", path.display())) } @@ -6449,6 +6467,38 @@ mod tests { assert_eq!(fs::read(&victim).unwrap(), b"keep"); } + /// The webview's log read goes through the same open: a planted symlink + /// is refused rather than read back, and so is anything but a regular + /// file, a FIFO without hanging. + #[cfg(unix)] + #[test] + fn log_read_refuses_a_symlink_or_non_regular_file() { + let root = TempDir::new("log-read-symlink"); + let victim = root.path().join("victim"); + fs::write(&victim, b"secret").unwrap(); + let path = root.path().join("dormouse.log"); + std::os::unix::fs::symlink(&victim, &path).unwrap(); + assert!(super::read_log_tail_at(&path, 10_000).is_err()); + + let fifo = root.path().join("fifo.log"); + let c_path = std::ffi::CString::new(fifo.as_os_str().as_encoded_bytes()).unwrap(); + assert_eq!(unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }, 0); + assert!(super::read_log_tail_at(&fifo, 10_000).is_err()); + + // A directory opens for reading on unix; it is refused before its + // mode is tightened. + use std::os::unix::fs::PermissionsExt; + let dir = root.path().join("dir.log"); + fs::create_dir(&dir).unwrap(); + fs::set_permissions(&dir, fs::Permissions::from_mode(0o755)).unwrap(); + assert!(super::open_log(&dir, fs::OpenOptions::new().read(true)).is_err()); + assert_eq!(fs::metadata(&dir).unwrap().permissions().mode() & 0o777, 0o755); + + let real = root.path().join("real.log"); + fs::write(&real, b"line\n").unwrap(); + assert_eq!(super::read_log_tail_at(&real, 10_000).unwrap(), "line\n"); + } + #[cfg(unix)] #[test] fn session_write_tightens_directory_and_existing_temp_file() { From 568af9437417111ea3ecbe12f0e5753b6ce2342c Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Mon, 19 Oct 2026 15:47:56 -0700 Subject: [PATCH 2/4] Refuse C1 controls in browser binary paths and session names The browser-binary gate refused C0 and DEL but let U+0080-U+009F through, and the agent-browser session-name check had the same gap. Both now use one shared class, CONTROL_CHARACTERS in the new dor-lib-common/control-characters module, which dor's shell-quote and dor-tools-builtin's CONTROLS now reuse instead of keeping their own copies. Co-Authored-By: Claude Opus 5.5 --- .github/audit/canaries/unc-binary-path.patch | 16 ++++++++-------- docs/specs/security-local.md | 2 +- dor-lib-common/package.json | 4 ++++ dor-lib-common/src/browser-providers.ts | 8 ++++++-- dor-lib-common/src/control-characters.ts | 11 +++++++++++ dor-lib-common/test/browser-providers.test.mjs | 15 +++++++++++++-- dor-tools-builtin/src/file-viewer-format.ts | 4 +++- dor/src/commands/shell-quote.ts | 13 +++++++------ lib/src/lib/agent-browser-binary.test.ts | 5 ++++- scripts/spec-word-budgets.json | 2 +- 10 files changed, 58 insertions(+), 22 deletions(-) create mode 100644 dor-lib-common/src/control-characters.ts diff --git a/.github/audit/canaries/unc-binary-path.patch b/.github/audit/canaries/unc-binary-path.patch index 71b9fd3e3..41409d792 100644 --- a/.github/audit/canaries/unc-binary-path.patch +++ b/.github/audit/canaries/unc-binary-path.patch @@ -4,10 +4,10 @@ Expect: dor-lib-common/src/browser-providers.ts Source: reverses #1040 (a UNC binaryPath passed the executable gate) diff --git a/dor-lib-common/src/browser-providers.ts b/dor-lib-common/src/browser-providers.ts -index 3147d2795..a43376867 100644 +index 6dc70bbf6..c20ed4767 100644 --- a/dor-lib-common/src/browser-providers.ts +++ b/dor-lib-common/src/browser-providers.ts -@@ -67,10 +67,10 @@ export const DEFAULT_PLAYWRIGHT_BIN = 'playwright-cli'; +@@ -69,10 +69,10 @@ export const DEFAULT_PLAYWRIGHT_BIN = 'playwright-cli'; const PLAYWRIGHT_FILENAME_RE = /^playwright-cli(?:\.(?:cmd|bat|exe|com|ps1))?$/i; const AGENT_BROWSER_FILENAME_RE = /^agent-browser(?:\.(?:cmd|bat|exe|com|ps1))?$/i; @@ -22,8 +22,8 @@ index 3147d2795..a43376867 100644 /** * True for a path opening with two separators in either spelling: a UNC share -@@ -109,7 +109,7 @@ function isAllowedBrowserBinary(candidate: unknown, configuredPath: string | und - if (/[\u0000-\u001f\u007f]/.test(candidate)) return false; +@@ -111,7 +111,7 @@ function isAllowedBrowserBinary(candidate: unknown, configuredPath: string | und + if (hasControlCharacter(candidate)) return false; if (configuredPath && candidate === configuredPath) return true; if (candidate === name) return true; - if (isUncOrDevicePath(candidate) || !ABSOLUTE_RE.test(candidate)) return false; @@ -32,7 +32,7 @@ index 3147d2795..a43376867 100644 if (segments.includes('..')) return false; return filename.test(segments[segments.length - 1] ?? ''); diff --git a/lib/src/lib/agent-browser-binary.test.ts b/lib/src/lib/agent-browser-binary.test.ts -index 786476cec..624b85659 100644 +index 882cb1214..ddbb7d570 100644 --- a/lib/src/lib/agent-browser-binary.test.ts +++ b/lib/src/lib/agent-browser-binary.test.ts @@ -35,14 +35,6 @@ describe('isAllowedAgentBrowserBinary', () => { @@ -50,11 +50,11 @@ index 786476cec..624b85659 100644 // Control characters are how one argument becomes two. '/usr/local/bin/agent-browser\n/bin/sh', '/usr/local/bin/agent-browser\u0000', -@@ -59,6 +51,6 @@ describe('isAllowedAgentBrowserBinary', () => { +@@ -62,6 +54,6 @@ describe('isAllowedAgentBrowserBinary', () => { it('validates Playwright executable hints independently of agent-browser', () => { for (const candidate of ['playwright-cli', '/opt/bin/playwright-cli', 'C:\\tools\\playwright-cli.cmd']) expect(isAllowedPlaywrightBinary(candidate)).toBe(true); -- for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '\\\\host\\share\\playwright-cli.cmd', '//host/share/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); -+ for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); +- for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/playwright-cli\u0085', '/opt/playwright-cli\u009f', '\\\\host\\share\\playwright-cli.cmd', '//host/share/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); ++ for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/playwright-cli\u0085', '/opt/playwright-cli\u009f']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); expect(isAllowedPlaywrightBinary('/opt/custom-wrapper', '/opt/custom-wrapper')).toBe(true); }); diff --git a/docs/specs/security-local.md b/docs/specs/security-local.md index 2d52e47ec..f9ad7048f 100644 --- a/docs/specs/security-local.md +++ b/docs/specs/security-local.md @@ -186,7 +186,7 @@ The attacker is a file planted in a folder the user opens: Windows looks a bare - **FAIL IF** `S165` a host, `dor`, or a built-in Tool spawns a program by bare name on Windows: a system program goes by its path under `%SystemRoot%` (`cmd.exe` by `ComSpec`), a user-installed one such as git by the file `docs/specs/dor-cli.md` -> "Spawning External Binaries" resolves on `PATH`, or not at all. Search `standalone/sidecar/`, `lib/src/host/`, `vscode-ext/src/`, `dor/src/`, `dor-lib-common/src/`, and `dor-tools-builtin/src/`. Pinned by `standalone/sidecar/pty-core.test.js`, `standalone/sidecar/clipboard-ops.test.js`, and `lib/src/host/git-upstream.test.ts`. - **FAIL IF** `S166` the VS Code pty host forks in any cwd but the extension's own directory. Pinned by `vscode-ext/test/pty-manager.test.ts`. - **FAIL IF** `S167` git run in a repository the user has not trusted can run a program that repository's config names: every git spawn in the directories above must lead with `UNTRUSTED_REPO_GIT_ARGS` and run only a subcommand its comment says it covers. Pinned by `lib/src/host/git-cli.test.ts`, `dor/test/open-picker.test.mjs`, and `dor-tools-builtin/test/folder-viewer.test.mjs`, each planting a `core.fsmonitor`. -- **FAIL IF** `S168` a host spawns a browser `binaryPath` the gate did not accept, or for Playwright loads `playwright-core` beside one, or `dor` runs such a pinned executable; the gate accepts, beyond the host's own override value by exact match, a UNC or device path, a relative path but the bare executable name, a `..` segment, a control character, or a file name other than the provider's executable; or the webview sends or stores one it refuses. The path crosses from the webview realm, so a same-user writer is no reason to rate a break below WARNING. Read `isAllowedBrowserBinary` in `dor-lib-common/src/browser-providers.ts`, `runWithBinaryFallback` in `lib/src/host/agent-browser-host.ts`, `findPlaywrightInstall` in `lib/src/host/playwright-install.ts`, the pin in `dor/src/commands/browser-cli.ts`, and the callers of `isAllowedBinaryFor` in `lib/src/lib/agent-browser-binary.ts`. Pinned by `lib/src/lib/agent-browser-binary.test.ts`, `lib/src/components/Wall.test.tsx`, and `dor/test/playwright.test.mjs`. +- **FAIL IF** `S168` a host spawns a browser `binaryPath` the gate did not accept, or for Playwright loads `playwright-core` beside one, or `dor` runs such a pinned executable; the gate accepts, beyond the host's own override value by exact match, a UNC or device path, a relative path but the bare executable name, a `..` segment, a C0 or C1 control character or DEL, or a file name other than the provider's executable; or the webview sends or stores one it refuses. The path crosses from the webview realm, so a same-user writer is no reason to rate a break below WARNING. Read `isAllowedBrowserBinary` in `dor-lib-common/src/browser-providers.ts`, `runWithBinaryFallback` in `lib/src/host/agent-browser-host.ts`, `findPlaywrightInstall` in `lib/src/host/playwright-install.ts`, the pin in `dor/src/commands/browser-cli.ts`, and the callers of `isAllowedBinaryFor` in `lib/src/lib/agent-browser-binary.ts`. Pinned by `lib/src/lib/agent-browser-binary.test.ts`, `lib/src/components/Wall.test.tsx`, and `dor/test/playwright.test.mjs`. - **FAIL IF** `S169` the Playwright host loads `@playwright/cli` or `playwright-core` from outside the launcher's own installation: a directory above it, a shim script path that climbs out of the shim's directory, or a `playwright-core` neither nested in the CLI package nor beside it in its `node_modules`, though Node's lookup reaches further. Read `findPlaywrightInstall` in `lib/src/host/playwright-install.ts`. Pinned by `lib/src/host/playwright-install.test.ts`. Source of truth: `windowsSystemPath` in `standalone/sidecar/windows-system.js`; `resolveBinaryPath` in `dor-lib-common/src/resolve-binary.ts`; `ensureChild` in `vscode-ext/src/pty-manager.ts`; `UNTRUSTED_REPO_GIT_ARGS` in `dor-lib-common/src/git.ts`. diff --git a/dor-lib-common/package.json b/dor-lib-common/package.json index 200f9bc91..13653bc1e 100644 --- a/dor-lib-common/package.json +++ b/dor-lib-common/package.json @@ -16,6 +16,10 @@ "./browser-providers": { "types": "./dist/browser-providers.d.ts", "default": "./dist/browser-providers.js" + }, + "./control-characters": { + "types": "./dist/control-characters.d.ts", + "default": "./dist/control-characters.js" } }, "scripts": { diff --git a/dor-lib-common/src/browser-providers.ts b/dor-lib-common/src/browser-providers.ts index b21569180..6dc70bbf6 100644 --- a/dor-lib-common/src/browser-providers.ts +++ b/dor-lib-common/src/browser-providers.ts @@ -9,6 +9,8 @@ * webview, the Node hosts, and `dor`. */ +import { CONTROL_CHARACTERS, hasControlCharacter } from './control-characters.js'; + /** A browser-automation provider: whose CLI drives a browser Surface. */ export type BrowserAutomationProvider = 'agent-browser' | 'playwright'; @@ -106,7 +108,7 @@ function isAllowedBrowserBinary(candidate: unknown, configuredPath: string | und if (typeof candidate !== 'string' || candidate.length === 0 || candidate.length > 4096) return false; // Control characters have no place in a path and are how one argument // becomes two on the platforms that take a command string. - if (/[\u0000-\u001f\u007f]/.test(candidate)) return false; + if (hasControlCharacter(candidate)) return false; if (configuredPath && candidate === configuredPath) return true; if (candidate === name) return true; if (isUncOrDevicePath(candidate) || !ABSOLUTE_RE.test(candidate)) return false; @@ -123,12 +125,14 @@ export interface BrowserBinding { binaryPath?: string; } +const AGENT_BROWSER_SESSION_RE = new RegExp(`^(?!-)[^/\\\\${CONTROL_CHARACTERS}]{1,200}$`); + /** An agent-browser session name. `dor agent-browser --session` passes a user's raw name * through, so anything goes but what agent-browser would read as an option or * its socket directory as a path: the name lands after `--session` and in * `/.pid`, whose pid a relaunch signals. */ function isAgentBrowserSession(value: unknown): value is string { - return typeof value === 'string' && /^(?!-)[^/\\\x00-\x1f\x7f]{1,200}$/.test(value); + return typeof value === 'string' && AGENT_BROWSER_SESSION_RE.test(value); } /** A Playwright session name: Dormouse mints these, and the CLI takes them diff --git a/dor-lib-common/src/control-characters.ts b/dor-lib-common/src/control-characters.ts new file mode 100644 index 000000000..200f3e5d3 --- /dev/null +++ b/dor-lib-common/src/control-characters.ts @@ -0,0 +1,11 @@ +/** C0, DEL, and C1 controls, as a character class body. Every gate on a path or + * spawn input refuses the whole set, so a gate cannot miss the C1 half (U+0080 + * to U+009F), which some consumers read as a line break or an escape. */ +export const CONTROL_CHARACTERS = '\\x00-\\x1f\\x7f-\\x9f'; + +const CONTROL = new RegExp(`[${CONTROL_CHARACTERS}]`); + +/** Whether `value` holds any of `CONTROL_CHARACTERS`. */ +export function hasControlCharacter(value: string): boolean { + return CONTROL.test(value); +} diff --git a/dor-lib-common/test/browser-providers.test.mjs b/dor-lib-common/test/browser-providers.test.mjs index 8d62fef9f..76bfb8cb1 100644 --- a/dor-lib-common/test/browser-providers.test.mjs +++ b/dor-lib-common/test/browser-providers.test.mjs @@ -1,6 +1,6 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { BROWSER_PROVIDER_IDS, BROWSER_PROVIDERS, parseRenderMode, parseStreamPort, renderModeFor, sessionForKey } from '../dist/browser-providers.js'; +import { BROWSER_PROVIDER_IDS, BROWSER_PROVIDERS, isAllowedAgentBrowserBinary, isAllowedPlaywrightBinary, parseRenderMode, parseStreamPort, renderModeFor, sessionForKey } from '../dist/browser-providers.js'; test('sessionForKey namespaces a key under the workspace', () => { assert.equal(sessionForKey('default'), 'dormouse.1.default'); @@ -39,7 +39,7 @@ test('each provider refuses a session name its CLI would read as an option or a for (const provider of BROWSER_PROVIDER_IDS) { const { isSessionName } = BROWSER_PROVIDERS[provider]; assert.equal(isSessionName('dormouse.1.default'), true, provider); - for (const bad of ['../../tmp/evil', 'a/b', 'a\\b', 'a\nb', '', 7]) { + for (const bad of ['../../tmp/evil', 'a/b', 'a\\b', 'a\nb', 'a\u0085b', 'a\u009fb', '', 7]) { assert.equal(isSessionName(bad), false, `${provider} ${JSON.stringify(bad)}`); } } @@ -58,3 +58,14 @@ test('parseStreamPort reads a top-level or nested port, and nothing from malform assert.equal(parseStreamPort(stdout), undefined, stdout); } }); + +test('the binary gates refuse a C0, DEL, or C1 control anywhere in the path', () => { + for (const control of ['\u0000', '\n', '\u007f', '\u0085', '\u009f']) { + assert.equal(isAllowedAgentBrowserBinary(`/usr/local/bin/agent-browser${control}`), false, JSON.stringify(control)); + assert.equal(isAllowedAgentBrowserBinary(`/usr/local/bin${control}/agent-browser`), false, JSON.stringify(control)); + assert.equal(isAllowedPlaywrightBinary(`/opt/bin/playwright-cli${control}`), false, JSON.stringify(control)); + } + // The neighbours of the C1 block are ordinary path characters. + assert.equal(isAllowedAgentBrowserBinary('/opt/\u00a0/agent-browser'), true); + assert.equal(isAllowedAgentBrowserBinary('/opt/~/agent-browser'), true); +}); diff --git a/dor-tools-builtin/src/file-viewer-format.ts b/dor-tools-builtin/src/file-viewer-format.ts index c6c8cebc7..fd01aa0e9 100644 --- a/dor-tools-builtin/src/file-viewer-format.ts +++ b/dor-tools-builtin/src/file-viewer-format.ts @@ -1,3 +1,5 @@ +import { CONTROL_CHARACTERS } from 'dor-lib-common/control-characters'; + /** Formats supported by the built-in local viewer. Unknown files need a user * Tool association rather than being guessed to be text. */ const MIME: Record = { @@ -85,7 +87,7 @@ export function defaultBuiltin(folder: boolean): BuiltinHandler { } /** C0, DEL, and C1 controls. */ -export const CONTROLS = /[\x00-\x1f\x7f-\x9f]/; +export const CONTROLS = new RegExp(`[${CONTROL_CHARACTERS}]`); /** The title naming a viewer's canonical `target`, `/` or `\` separated: its * basename, or the whole path for a filesystem root. Controls are stripped, diff --git a/dor/src/commands/shell-quote.ts b/dor/src/commands/shell-quote.ts index c55f01c15..5f7c7d256 100644 --- a/dor/src/commands/shell-quote.ts +++ b/dor/src/commands/shell-quote.ts @@ -1,14 +1,15 @@ /** - * Pure, dependency-free shell quoting. The webview (the only layer that knows - * the target pane's shell) turns a raw argv array into a single command string - * for that shell. Keep this module free of Node imports so it can be bundled - * into the browser-side webview as well as the CLI. + * Pure shell quoting. The webview (the only layer that knows the target pane's + * shell) turns a raw argv array into a single command string for that shell. + * Keep this module free of Node imports so it can be bundled into the + * browser-side webview as well as the CLI. */ +import { CONTROL_CHARACTERS } from 'dor-lib-common/control-characters'; + export type ShellCommandKind = 'cmd' | 'posix' | 'powershell'; -/** C0, DEL, and C1, as a character class body. */ -export const CONTROL_CHARACTERS = '\\x00-\\x1f\\x7f-\\x9f'; +export { CONTROL_CHARACTERS }; /** Unicode format characters that reorder or hide the text around them: every * bidi control, the zero-width space and joiners, the word joiner and * invisible operators, and the BOM — the set `boundedPushText` in diff --git a/lib/src/lib/agent-browser-binary.test.ts b/lib/src/lib/agent-browser-binary.test.ts index 49d8ae567..882cb1214 100644 --- a/lib/src/lib/agent-browser-binary.test.ts +++ b/lib/src/lib/agent-browser-binary.test.ts @@ -46,6 +46,9 @@ describe('isAllowedAgentBrowserBinary', () => { // Control characters are how one argument becomes two. '/usr/local/bin/agent-browser\n/bin/sh', '/usr/local/bin/agent-browser\u0000', + // C1 too: U+0085 is a line break to some readers, U+009F ends the set. + '/usr/local/bin/agent-browser\u0085/bin/sh', + '/usr/local/bin/agent-browser\u009f', '', undefined, null, @@ -59,6 +62,6 @@ describe('isAllowedAgentBrowserBinary', () => { it('validates Playwright executable hints independently of agent-browser', () => { for (const candidate of ['playwright-cli', '/opt/bin/playwright-cli', 'C:\\tools\\playwright-cli.cmd']) expect(isAllowedPlaywrightBinary(candidate)).toBe(true); - for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '\\\\host\\share\\playwright-cli.cmd', '//host/share/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); + for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/playwright-cli\u0085', '/opt/playwright-cli\u009f', '\\\\host\\share\\playwright-cli.cmd', '//host/share/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); expect(isAllowedPlaywrightBinary('/opt/custom-wrapper', '/opt/custom-wrapper')).toBe(true); }); diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 1dc2ab76b..d42150a04 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -27,7 +27,7 @@ "docs/specs/security-audit.md": 4700, "docs/specs/security-ci.md": 3350, "docs/specs/security-hosted.md": 2900, - "docs/specs/security-local.md": 5600, + "docs/specs/security-local.md": 5650, "docs/specs/security-remote.md": 6800, "docs/specs/security-supply-chain.md": 1600, "docs/specs/security.md": 2350, From b8164ba343e2f5cfdc1b9d80b06b652853b6809d Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Mon, 19 Oct 2026 16:00:57 -0700 Subject: [PATCH 3/4] Confirm a document link that opens a page running its own script A Tool's document link (OSC 367 open with a disposition, e.g. a relative Markdown link) names its own target, which can climb with `..` anywhere on disk under any link text. When that target is a format the built-in preview serves as a page running the file's own script (HTML, SVG), the host now asks in the external-link dialog, showing the full resolved path, before it opens or focuses it. The dialog lives in host UI the Tool's page cannot reach; it only answers, and a cancel, a replacing link, or an aborted request answers no. Markdown, text, images, and active files opened as source or as a folder keep opening on a click. Back/Forward to such a page asks again. Adds FAIL IF S269 under security-local.md "Terminal output", with a guarantee row, and points dor-tool.md, dor-tools-builtin.md, terminal-escapes.md, and security.md at it. Co-Authored-By: Claude Opus 5.5 --- docs/specs/dor-tool.md | 1 + docs/specs/dor-tools-builtin.md | 2 +- docs/specs/security-local.md | 4 ++ docs/specs/security.md | 4 +- docs/specs/terminal-escapes.md | 2 +- dor-tools-builtin/src/file-viewer-format.ts | 8 +++ lib/src/components/ExternalLinkModal.tsx | 5 ++ .../components/ExternalLinkModalHost.test.tsx | 41 +++++++++++++- lib/src/components/ExternalLinkModalHost.tsx | 13 ++++- lib/src/components/wall/preview-slot.test.tsx | 55 +++++++++++++++++++ .../components/wall/tool-navigation.test.ts | 18 +++++- lib/src/components/wall/tool-navigation.ts | 17 +++++- lib/src/components/wall/use-dor-control.ts | 21 ++++++- lib/src/lib/external-link-confirmation.ts | 55 ++++++++++++++++++- lib/src/stories/ExternalLinkModal.stories.tsx | 15 ++++- scripts/security-rule-ids.json | 2 +- scripts/spec-word-budgets.json | 8 +-- 17 files changed, 251 insertions(+), 20 deletions(-) diff --git a/docs/specs/dor-tool.md b/docs/specs/dor-tool.md index 841419af4..43a78e3d2 100644 --- a/docs/specs/dor-tool.md +++ b/docs/specs/dor-tool.md @@ -238,6 +238,7 @@ Source of truth: `runFilePicker` in `dor/src/commands/open-picker.ts`; `listOpen - **Must resolve a Tool's document-link request through ordinary open dispatch and trust checks**, relative to the current canonical document's directory. The request chooses the current Surface or another Surface; it never requests a preview or a fresh instance. Existing matching Tools are focused through ordinary reuse without changing either Surface's history. - **Must keep Back/Forward history on the navigating Surface**, covering every file/folder handler, including user Tools. Traversal reopens the recorded canonical target with its selected handler and lookup directory in that Surface even when another Surface shows the target; a new in-place visit after Back drops the forward branch. Pin a preview on a committed in-place file visit. - **Must obtain Save / Discard / Cancel before replacing a dirty document**, using [Closing unsaved Tools](#closing-unsaved-tools). A same-document anchor or reuse of another Surface leaves the draft alone. Never cache drafts or undo state in history. +- **Must ask in the external-link dialog before a document link opens a page that runs its own script**, Back/Forward included (`docs/specs/security-local.md` -> "Terminal output"); a cancel is a navigation failure, and a same-document anchor never asks. - **Must validate the destination before interrupting the current Tool**, and keep the current location/history on lookup failure or cancelled consent. Refuse overlapping navigation from one Surface and stale requests whose source run changed. When the interrupt or the new run fails, keep or restore the source's run, never typing over user input. Report navigation failures on the source, without opening the preview error viewer. - **Must retain history through minimize, renderer changes, and live Workspace transfer, but never in durable saves**. Only the current document restarts after cold restore. Ending designation, Break, or closing discards history. Refuse Workspace transfer during navigation or pending reading-position restoration. - **Must release pending reading-position restoration on timeout**, even when the destination never serves a page or never answers; renderer rebinding must not renew the deadline. diff --git a/docs/specs/dor-tools-builtin.md b/docs/specs/dor-tools-builtin.md index b700083af..62525a1e6 100644 --- a/docs/specs/dor-tools-builtin.md +++ b/docs/specs/dor-tools-builtin.md @@ -57,7 +57,7 @@ Source of truth: `dor-tools-builtin/viewer/markdown.tsx`; `safeCreateDOM` in `do ### Relative links -- **Must follow a relative link on plain click**, and offer Open in another surface and Edit link on its context menu. Cmd/Ctrl-click chooses another Surface. Dispatch follows `docs/specs/dor-tool.md` → Document navigation. (rationale) +- **Must follow a relative link on plain click**, and offer Open in another surface and Edit link on its context menu. Cmd/Ctrl-click chooses another Surface. Dispatch, and any confirmation the host asks first, follows `docs/specs/dor-tool.md` → Document navigation. (rationale) - **Must open a local PNG or JPEG image's file the same way on Edit or double-click**, with a link's context menu whose edit item opens it. - **Must let an enclosing link own double-click and context-menu behavior.** - **Must treat only a scheme-less relative reference as a document link**, resolved from the opened canonical file's directory with parent references allowed and its query ignored; HTTP(S)/mail links keep their existing handling. diff --git a/docs/specs/security-local.md b/docs/specs/security-local.md index f9ad7048f..bbf6d2292 100644 --- a/docs/specs/security-local.md +++ b/docs/specs/security-local.md @@ -10,6 +10,7 @@ | Guarantee | Feature | Pinned by | | --- | --- | --- | | **Terminal output cannot write your clipboard or steal focus.** File access requires a user action or the gated OSC 367 `open` of a Tool run Dormouse launched. OSC 52 only offers a copy format; links require confirmation, naming any app scheme, or an allowed local preview; deceptive links and dangerous Windows handlers have no open action. | | `lib/src/lib/terminal-protocol.test.ts`, `lib/src/lib/mouse-selection.test.ts`, `lib/src/lib/copy-editor.test.ts`, `lib/src/lib/external-links.test.ts`, `lib/src/lib/terminal-link-activation.test.ts`, `lib/src/components/ExternalLinkModalHost.test.tsx`, `lib/src/lib/tool-launch-latch.test.ts`, `lib/src/components/wall/preview-slot.test.tsx` | +| **A link in a document you are viewing cannot open a page that runs its own scripts without asking.** Dormouse's own dialog shows the file's full path first; Markdown, text, and images still open on a click. | | `lib/src/components/wall/preview-slot.test.tsx`, `lib/src/components/wall/tool-navigation.test.ts`, `lib/src/components/ExternalLinkModalHost.test.tsx` | The attacker is any program writing to a PTY. @@ -19,6 +20,8 @@ Output reaches the screen, Session state, and bounded terminal reports — rende **An `OSC 8` hyperlink opens only after a confirmation dialog**, except a local `file:` link whose display text names its target, which previews through the user's `open` rules (`docs/specs/dor-tool.md` -> "Terminal links"); any scheme but `http`, `https`, `mailto`, and `file` gets a dialog naming it as another app, and a target whose display text names a different host gets **no open action at all** (`docs/specs/mouse-and-clipboard.md` -> "OSC 8 hyperlinks"). File opens use `docs/specs/dor-tool.md` -> "Opening local files". +A Tool's document link names its own target, anywhere on disk under any link text (`docs/specs/dor-tool.md` -> "Document navigation"). + Notification text: `docs/specs/alert.md` -> "Text And Security". **Agent recovery is the one path from terminal output to typed input**: at shutdown each host interrupts every live PTY and scans its later output for an agent's resume hint, which the next cold restore types unconfirmed (`docs/compatible-agents.md` -> "Capture", "Cold restore"). It types a rebuilt invocation, never sliced output; what a terminal still chooses is `docs/specs/security.md` -> "What is not defended". @@ -34,6 +37,7 @@ Notification text: `docs/specs/alert.md` -> "Text And Security". - **FAIL IF** `S130` a host launches an external URL without revalidating it through `normalizeExternalUri` in `lib/src/lib/external-links.ts`, consent notwithstanding — VS Code's in the extension host (`vscode-ext/src/message-router.ts`), standalone's in `standalone/src/tauri-adapter.ts` and `openUrl` in `standalone/src/updater.ts` — or that function accepts a scheme in `PSEUDO_SCHEMES` or `REFUSED_APP_SCHEMES`, or the latter drops `ms-msdt`, `search-ms`, `ms-officecmd`, or `ms-appinstaller` (rationale). Pinned by `vscode-ext/test/message-router.test.ts` and `standalone/src/tauri-adapter.test.ts`. - **FAIL IF** `S131` an unconfirmed `OSC 8` activation reaches the preview path when its display text is not a whole-component suffix of its decoded target, or the host opens a `file:` URL whose host is not empty, `localhost`, or this machine: `localFileLinkPreviewPath` in `lib/src/lib/external-links.ts`, `activateTerminalLink` in `lib/src/lib/terminal-link-activation.ts`, `resolveLocalToolTarget` in `lib/src/host/tool-input.ts`. Pinned by `lib/src/lib/external-links.test.ts`, `lib/src/lib/terminal-link-activation.test.ts`, and `lib/src/host/tool-input.test.ts`. - **FAIL IF** `S132` an OSC 367 `open` reaches `surface.tool` from replay, from a Session that is not a Tool, from output outside the run the host launched in that PTY (`docs/specs/dor-tool.md` → OSC 367), or naming anything but an absolute path free of controls; if anything output reports, a shell-integration command line included, can begin that run; if a write but the host's own command launch can carry `launch`; or if a control-socket request can set its `oscOpen` flag, or its failure reaches the error viewer's argv with a control character. Read `parseToolOpen` in `dor-tools-lib/src/osc.ts`; `ToolLaunchLatch` in `lib/src/lib/tool-launch-latch.ts` and its one user, `createOwnerPtyStream` in `lib/src/host/owner-pty.ts`; the `launch` writes in `typeToolCommand` in `lib/src/components/wall/use-dor-control.ts`, `typeCommandWhenPromptReady` in `lib/src/lib/terminal-lifecycle.ts`, and `watchHelper` in `lib/src/lib/helper-terminal.ts` and each host's arm (`onPtyLaunch` in `lib/src/host/remote/sidecar-entry.ts`, `vscode-ext/src/message-router.ts`); `dispatchToolOpens` in `lib/src/lib/tool-open-requests.ts` (reached from `applyLiveToolEvents`, never from `parseReplay` in `lib/src/lib/platform/replay-parse.ts`); the `oscOpen` argument of `dispatchDorControlRequest` in `lib/src/lib/platform/dor-control-dispatch.ts`; and the `oscOpen` gate in `lib/src/components/wall/use-dor-control.ts`. Pinned by `lib/src/lib/tool-launch-latch.test.ts` and `lib/src/components/wall/preview-slot.test.tsx`. +- **FAIL IF** `S269` a document-link `open` — one carrying a disposition, Back/Forward through a Surface's history included — runs or focuses a target `builtin:file` would serve as a page running that file's own script, under any handler but `builtin:code` or `builtin:folder`, before the user confirms its full resolved path in host UI the Tool's page cannot reach; or a cancelled, replaced, or aborted confirmation lets it proceed; or `runsOwnScript` misses a format the viewer CSP lets run script. Read `documentLinkNeedsConfirmation` in `lib/src/components/wall/tool-navigation.ts`, `runsOwnScript` in `dor-tools-builtin/src/file-viewer-format.ts`, its call in the `surface.tool` handler of `lib/src/components/wall/use-dor-control.ts`, and `confirmDocumentLinkOpen` in `lib/src/lib/external-link-confirmation.ts`. Pinned by `lib/src/components/wall/preview-slot.test.tsx`, `lib/src/components/wall/tool-navigation.test.ts`, and `lib/src/components/ExternalLinkModalHost.test.tsx`. - **FAIL IF** `S133` cold restore types a command `normalizeResumeCommand` in `lib/src/lib/resume-patterns.ts` did not rebuild at the typing boundary (`restoreTerminal` in `lib/src/lib/terminal-lifecycle.ts`), its grammar admits more than the invocations `docs/compatible-agents.md` -> "Detection" names — another argument, whitespace, or a shell metacharacter — or capture scans output from before its first interrupt (`captureAgentRecovery` in `lib/src/host/recovery-capture.ts`). Pinned by `lib/src/lib/coding-agents.test.ts`, `lib/src/lib/terminal-registry.alert.test.ts`, and `lib/src/host/recovery-capture.test.ts`. ## Browser panes diff --git a/docs/specs/security.md b/docs/specs/security.md index 031636551..72bdc3a69 100644 --- a/docs/specs/security.md +++ b/docs/specs/security.md @@ -44,8 +44,8 @@ Source of truth: root scripts in `package.json`; native jobs in `.github/workflo - **A process running as you.** `dor`, its socket, and every file mode bound other local accounts, never a program already running under your own account; an agent holding `dor` has exactly the power of the person at the keyboard, and the Tool trust prompt is no boundary against it ([The dor control socket](./security-local.md#the-dor-control-socket)). - **The Windows `dor` pipe carries no ACL of ours.** A named pipe has no directory to harden, so an unguessable name and the token handshake are the whole of it ([The dor control socket](./security-local.md#the-dor-control-socket)). -- **A local HTML or SVG document you open can send its contents off the machine.** The file viewer's content policy confines what the page loads, not where its scripts navigate ([Local-file viewer](./security-local.md#local-file-viewer)). -- **A mermaid sanitizer bypass in a Markdown document you open.** Its diagrams reach the editor page through mermaid's strict sanitizer, not the editor's allowlist, so a bypass would run script holding the editor's save and image routes ([Local-file viewer](./security-local.md#local-file-viewer)). +- **A local HTML or SVG document you open, or confirm from a document link, can send its contents off the machine.** The file viewer's content policy confines what the page loads, not where its scripts navigate ([Local-file viewer](./security-local.md#local-file-viewer)). +- **A mermaid sanitizer bypass in a Markdown document you open.** Its diagrams reach the editor page through mermaid's strict sanitizer, not the editor's allowlist, so a bypass would run script holding the editor's save, image, and document-open routes; an open of a page that runs its own scripts still waits on Dormouse's own dialog ([Local-file viewer](./security-local.md#local-file-viewer), [Terminal output](./security-local.md#terminal-output)). - **A Tool trust grant on an upstream URL trusts the URL a checkout claims.** Any directory whose own `.git/config` claims an already-granted upstream shares that grant; a folder-only grant is not shared ([Dor Tool configuration](./security-local.md#dor-tool-configuration)). - **Output already flowing into a Tool's terminal at launch** can, between Dormouse typing the command and the shell reporting it, make that run's OSC 367 `open` requests ([Terminal output](./security-local.md#terminal-output)). - **A terminal can choose which of your own agent sessions resumes.** Output a pane prints as Dormouse quits can name any session of a supported agent, which the next restore resumes in that pane's directory without asking; it can type nothing else ([Terminal output](./security-local.md#terminal-output)). diff --git a/docs/specs/terminal-escapes.md b/docs/specs/terminal-escapes.md index e8cbb51d4..95f292d5a 100644 --- a/docs/specs/terminal-escapes.md +++ b/docs/specs/terminal-escapes.md @@ -78,7 +78,7 @@ Replay (`pty:replay`) is the raw stream requiring re-parse: **the webview runs a | `OSC 777 ; notify ; ; <body> ST` | rxvt/WezTerm notification | [alert.md](alert.md#terminal-reports) | | `OSC 367 ; serve ; <json> ST` | Dor Tool announcement: selects a bound port and optional same-origin path, plus a reserved name and runtime re-key | [dor-tool.md](dor-tool.md#osc-367) | | `OSC 367 ; state ; <json> ST` | Tool-reported unsaved state | [dor-tool.md](dor-tool.md#unsaved-changes) | -| `OSC 367 ; open ; <json> ST` | A Tool run's request to open a local path as `dor open`, preview or not; live output of the run the host launched only | [dor-tool.md](dor-tool.md#osc-367) | +| `OSC 367 ; open ; <json> ST` | A Tool run's request to open a local path as `dor open`, preview or not, or with a disposition and optional fragment as a document-link navigation; live output of the run the host launched only | [dor-tool.md](dor-tool.md#osc-367), [document navigation](dor-tool.md#document-navigation) | | `OSC 367 ; dehydrate ; <json> ST` | A stopping Tool's restore payload; live output only | [dor-tool.md](dor-tool.md#reaping) | | `OSC 367 ; <any other verb> ST` | Consumed and ignored | [dor-tool.md](dor-tool.md#osc-367) | | `OSC 1337 ; CurrentDir=<cwd> ST` | CWD (iTerm2 compatibility) | [terminal-state.md](terminal-state.md#supported-osc-inputs) | diff --git a/dor-tools-builtin/src/file-viewer-format.ts b/dor-tools-builtin/src/file-viewer-format.ts index fd01aa0e9..01d1f74c5 100644 --- a/dor-tools-builtin/src/file-viewer-format.ts +++ b/dor-tools-builtin/src/file-viewer-format.ts @@ -81,6 +81,14 @@ export function builtinHandler(by: 'tool' | 'kind' | 'argv', value: unknown): Bu return BUILTIN_HANDLERS.find(handler => handler[by] === value); } +/** Whether `builtin:file` serves this format as a page that runs the file's + * own script: HTML, and SVG, which it serves as its own document under the + * viewer CSP's inline scripts. Everything else it shows inertly or in its own + * shipped editor. */ +export function runsOwnScript(format: FileFormat): boolean { + return !format.text && /^(text\/html|image\/svg\+xml)\b/.test(format.mime); +} + /** The built-in a folder or a file opens with when no rule matches. */ export function defaultBuiltin(folder: boolean): BuiltinHandler { return BUILTIN_HANDLERS.find(handler => handler.opens === (folder ? 'folder' : 'file'))!; diff --git a/lib/src/components/ExternalLinkModal.tsx b/lib/src/components/ExternalLinkModal.tsx index 16c908764..ae208db93 100644 --- a/lib/src/components/ExternalLinkModal.tsx +++ b/lib/src/components/ExternalLinkModal.tsx @@ -18,6 +18,8 @@ export interface ExternalLinkModalRequest { displayText: string; verdict: DisplayMatchVerdict; decision: ExternalUriDecision; + /** Why this open is asked about, below the target. */ + note?: string; } // "Open ___" button label suffix: the scheme noun sits next to the cursor, so @@ -107,6 +109,9 @@ export function ExternalLinkModal({ {displayUri} </ModalReviewBlock> + {request.note && openableDecision && !isDeceptive && ( + <p className="mt-3 text-xs text-muted">{request.note}</p> + )} {openableDecision?.otherApp && !isDeceptive && ( <p className="mt-3 text-xs text-muted"> Dormouse hands this link to whichever app is registered for{' '} diff --git a/lib/src/components/ExternalLinkModalHost.test.tsx b/lib/src/components/ExternalLinkModalHost.test.tsx index 99a8f7809..84c9f1300 100644 --- a/lib/src/components/ExternalLinkModalHost.test.tsx +++ b/lib/src/components/ExternalLinkModalHost.test.tsx @@ -1,10 +1,10 @@ /** @vitest-environment jsdom */ import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; -import { afterEach, beforeEach, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { ExternalLinkModalHost } from './ExternalLinkModalHost'; import type { DorControlResult } from 'dor/protocol'; -import { clearExternalLinkConfirmation, getExternalLinkConfirmationSnapshot, requestExternalLinkConfirmation } from '../lib/external-link-confirmation'; +import { clearExternalLinkConfirmation, confirmDocumentLinkOpen, getExternalLinkConfirmationSnapshot, requestExternalLinkConfirmation } from '../lib/external-link-confirmation'; const mocks = vi.hoisted(() => ({ open: vi.fn(), toolControl: vi.fn(), confirm: () => {} })); vi.mock('../lib/platform', () => ({ getPlatform: () => ({ openExternal: mocks.open, toolControl: mocks.toolControl }) })); @@ -201,3 +201,40 @@ it('explains a file link with no originating terminal instead of passing it to t expect(mocks.open).not.toHaveBeenCalled(); expect(requests).toHaveLength(0); }); + +describe('a document link\'s confirmation', () => { + const path = '/elsewhere/page.html'; + const ask = (signal?: AbortSignal) => { + let answer!: Promise<boolean>; + act(() => { answer = confirmDocumentLinkOpen(path, signal); }); + return answer; + }; + + it('shows the full path and answers yes only on an explicit open, opening nothing itself', async () => { + const answer = ask(); + expect(document.body.textContent).toContain(path); + expect(document.activeElement?.textContent).toBe('Cancel'); + act(() => button('Open file').click()); + expect(await answer).toBe(true); + expect(getExternalLinkConfirmationSnapshot()).toBeNull(); + expect(mocks.open).not.toHaveBeenCalled(); + expect(requests).toHaveLength(0); + }); + + it('answers no on cancel, on another link replacing it, and on abort', async () => { + const cancelled = ask(); + act(() => button('Cancel').click()); + expect(await cancelled).toBe(false); + + const replaced = ask(); + act(() => requestExternalLinkConfirmation('https://example.com/')); + expect(await replaced).toBe(false); + act(() => clearExternalLinkConfirmation()); + + const controller = new AbortController(); + const aborted = ask(controller.signal); + act(() => controller.abort()); + expect(await aborted).toBe(false); + expect(getExternalLinkConfirmationSnapshot()).toBeNull(); + }); +}); diff --git a/lib/src/components/ExternalLinkModalHost.tsx b/lib/src/components/ExternalLinkModalHost.tsx index 8b3b42875..7512035c6 100644 --- a/lib/src/components/ExternalLinkModalHost.tsx +++ b/lib/src/components/ExternalLinkModalHost.tsx @@ -19,7 +19,18 @@ export function ExternalLinkModalHost() { ); useDialogKeyboardOwner(pending !== null); // Each click owns its lookup, selection and launch result, even for the same URL. - return pending ? <PendingLinkDialog key={pending.id} request={pending} /> : null; + if (!pending) return null; + return pending.answer ? <PendingAnswerDialog key={pending.id} request={pending} /> : <PendingLinkDialog key={pending.id} request={pending} />; +} + +/** A question another flow awaits: confirming only answers it. */ +function PendingAnswerDialog({ request }: { request: PendingExternalLink }) { + const confirm = () => { + if (getExternalLinkConfirmationSnapshot() !== request) return; + request.answer!(true); + clearExternalLinkConfirmation(); + }; + return <ExternalLinkModal request={request} onCancel={clearExternalLinkConfirmation} onConfirm={confirm} />; } function PendingLinkDialog({ request }: { request: PendingExternalLink }) { diff --git a/lib/src/components/wall/preview-slot.test.tsx b/lib/src/components/wall/preview-slot.test.tsx index e6007b91a..2ae929f28 100644 --- a/lib/src/components/wall/preview-slot.test.tsx +++ b/lib/src/components/wall/preview-slot.test.tsx @@ -1328,6 +1328,61 @@ describe('Tool document navigation', () => { }); +describe('a document link to a page that runs its own script', () => { + /** The Tool in `surface:slot` follows a document link, as its OSC 367 `open`. */ + const follow = (path: string) => act(async () => applyLiveToolEvents('surface:slot', + [{ kind: 'toolOpen', open: { path, preview: false, disposition: 'current' } }])); + const dialogButton = (label: string) => [...document.body.querySelectorAll('button')].find(button => button.textContent === label)!; + /** Wait for the slot to show `target`. */ + async function untilTarget(target: string): Promise<void> { + for (let tries = 0; tries < 100; tries++) { + if ((await paramsOf('surface:slot'))?.toolTarget === target) return; + await act(async () => { await new Promise(resolve => setTimeout(resolve, 20)); }); + } + expect((await paramsOf('surface:slot'))?.toolTarget).toBe(target); + } + + it('waits for the user to confirm its full path in host UI, and a cancel runs nothing', async () => { + await mountSlot(); + await follow('/elsewhere/page.html'); + await waitUntil(() => getExternalLinkConfirmationSnapshot() !== null); + expect(getExternalLinkConfirmationSnapshot()?.uri).toBe('/elsewhere/page.html'); + expect(document.body.textContent).toContain('/elsewhere/page.html'); + await flush(); + expect(typed['surface:slot']).toEqual([]); + expect((await paramsOf('surface:slot'))?.toolTarget).toBe('/repo/a.md'); + + act(() => dialogButton('Cancel').click()); + await flush(); + expect(typed['surface:slot']).toEqual([]); + expect((await paramsOf('surface:slot'))?.toolTarget).toBe('/repo/a.md'); + + await follow('/elsewhere/page.html'); + await waitUntil(() => getExternalLinkConfirmationSnapshot() !== null); + act(() => dialogButton('Open file').click()); + await untilTarget('/elsewhere/page.html'); + }); + + it('opens an inert target on the click alone, and asks again on Forward to the page', async () => { + await mountSlot(); + await follow('/elsewhere/notes.md'); + await untilTarget('/elsewhere/notes.md'); + expect(getExternalLinkConfirmationSnapshot()).toBeNull(); + await follow('/elsewhere/diagram.svg'); + await waitUntil(() => getExternalLinkConfirmationSnapshot() !== null); + act(() => dialogButton('Open file').click()); + await untilTarget('/elsewhere/diagram.svg'); + await answer(await request({ history: 'back', disposition: 'current' }, 'surface:slot')); + expect(getExternalLinkConfirmationSnapshot()).toBeNull(); + const forward = await request({ history: 'forward', disposition: 'current' }, 'surface:slot'); + await waitUntil(() => getExternalLinkConfirmationSnapshot()?.uri === '/elsewhere/diagram.svg'); + act(() => clearExternalLinkConfirmation()); + await waitUntil(() => forward.mock.calls.length > 0); + expect(forward.mock.calls[0][0]).toMatchObject({ ok: false, error: 'Navigation cancelled.' }); + expect((await paramsOf('surface:slot'))?.toolTarget).toBe('/elsewhere/notes.md'); + }); +}); + it('document navigation Cancel keeps the draft and history; Discard permits replacement', async () => { await mountSlot(); recordToolDirty('surface:slot', true); diff --git a/lib/src/components/wall/tool-navigation.test.ts b/lib/src/components/wall/tool-navigation.test.ts index 1b5cb1f19..51bea540d 100644 --- a/lib/src/components/wall/tool-navigation.test.ts +++ b/lib/src/components/wall/tool-navigation.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { commitToolHistory, toolHistory } from './tool-navigation'; +import { commitToolHistory, documentLinkNeedsConfirmation, toolHistory } from './tool-navigation'; import { directoryOf } from '../../lib/tool-open-requests'; const location = (target: string, top = 0) => ({ target, cwd: '/repo', handler: 'builtin:file', position: { top } }); @@ -28,3 +28,19 @@ describe('Tool document history', () => { expect(directoryOf('C:\\repo\\a.md')).toBe('C:\\repo'); }); }); + +describe('which document links wait for confirmation', () => { + it('asks before a page that runs its own script, under any handler that would render it', () => { + for (const target of ['/x/page.html', '/x/PAGE.HTM', '/x/diagram.svg', 'C:\\x\\page.html']) { + expect(documentLinkNeedsConfirmation('builtin', 'file', target), target).toBe(true); + expect(documentLinkNeedsConfirmation('user', 'viewer', target), target).toBe(true); + } + }); + it('opens inert targets, and active ones shown as source or as a folder, on the click alone', () => { + for (const target of ['/x/notes.md', '/x/a.txt', '/x/photo.png', '/x/anim.gif', '/x/clip.mp4', '/x/script.js', '/x/feed.xml', '/x/doc.pdf', '/x/README']) { + expect(documentLinkNeedsConfirmation('builtin', 'file', target), target).toBe(false); + } + expect(documentLinkNeedsConfirmation('builtin', 'code', '/x/page.html')).toBe(false); + expect(documentLinkNeedsConfirmation('builtin', 'folder', '/x/site.html')).toBe(false); + }); +}); diff --git a/lib/src/components/wall/tool-navigation.ts b/lib/src/components/wall/tool-navigation.ts index a1bc87c2e..059c69fc3 100644 --- a/lib/src/components/wall/tool-navigation.ts +++ b/lib/src/components/wall/tool-navigation.ts @@ -1,7 +1,7 @@ import { readToolViewPosition, type ToolViewPosition } from 'dor-tools-lib/protocol'; import { SURFACE_CONTROL_METHODS } from 'dor/protocol'; import { parseRenderMode } from 'dor-lib-common/browser-providers'; -import { builtinHandler } from 'dor-tools-builtin/file-viewer-format'; +import { builtinHandler, fileViewerFormat, runsOwnScript } from 'dor-tools-builtin/file-viewer-format'; import { getPlatform } from '../../lib/platform'; import { dispatchDorControlRequest } from '../../lib/platform/dor-control-dispatch'; import { toolEditorView } from '../../lib/tool-editor'; @@ -34,6 +34,21 @@ export function toolHandlerName(scope: unknown, name: unknown): string | undefin return scope === 'builtin' ? builtinHandler('kind', name)?.tool : name; } +/** + * Whether a document-link open of `target`, resolved to the handler `scope` and + * `name` name, waits for the user to confirm it in host UI: the target is a + * format whose built-in preview runs the file's own script, and the handler is + * not a built-in that shows it inertly. A user or repository Tool given such a + * file is assumed to render it too. A link may climb with `..` to anywhere on + * disk under any link text, so the page cannot be the one that asks. + */ +export function documentLinkNeedsConfirmation(scope: unknown, name: unknown, target: string): boolean { + const format = fileViewerFormat(target); + if (!format || !runsOwnScript(format)) return false; + const handler = toolHandlerName(scope, name); + return handler !== 'builtin:code' && handler !== 'builtin:folder'; +} + export function toolLocation(params: Record<string, unknown>, cwd: string): ToolLocation | null { if (typeof params.toolTarget !== 'string') return null; return { target: params.toolTarget, handler: toolHandlerName(params.toolScope, params.toolName), cwd }; diff --git a/lib/src/components/wall/use-dor-control.ts b/lib/src/components/wall/use-dor-control.ts index 92858fccb..f692f4dcf 100644 --- a/lib/src/components/wall/use-dor-control.ts +++ b/lib/src/components/wall/use-dor-control.ts @@ -75,8 +75,9 @@ import { commitPreviewTransition, endPreviewTransition } from '../../lib/preview import { retireToolRun } from './use-tool-serving'; import { validToolFragment } from 'dor-tools-lib/osc'; import { confirmToolEditorsClose } from '../../lib/tool-editor'; +import { confirmDocumentLinkOpen } from '../../lib/external-link-confirmation'; import { directoryOf } from '../../lib/tool-open-requests'; -import { commitToolHistory, hasToolView, toolHandlerName, toolHistory, toolLocation, toolNavigationPending, toolView } from './tool-navigation'; +import { commitToolHistory, documentLinkNeedsConfirmation, hasToolView, toolHandlerName, toolHistory, toolLocation, toolNavigationPending, toolView } from './tool-navigation'; import { listenerUrlsByPort } from './port-url'; import { becomeToolMeta, dorDirectionForEdge, toolLeafMeta, type LathWallEngine } from './lath-wall-engine'; @@ -1451,6 +1452,24 @@ export function useDorControl({ toolRun = argv!; } + // A document link's target is the Tool's to name: a Markdown link can + // climb with `..` to anywhere on disk under any link text. One whose + // preview would run that file's own script therefore opens only once + // the user confirms its full path here, in host UI the page cannot + // reach. An anchor in the document already shown loads nothing new. + // Back/Forward asks again: re-asking costs one click, and no rule then + // depends on how a history entry got there. + if (documentLink && openTarget !== undefined + && !(sourceParams!.toolTarget === openTarget && sourceParams!.toolName === toolName && sourceParams!.toolScope === toolScope) + && documentLinkNeedsConfirmation(toolScope, toolName, openTarget)) { + const confirmed = await confirmDocumentLinkOpen(openTarget, detail.signal); + if (unavailable()) return; + if (!confirmed) { + detail.respond({ ok: false, error: 'Navigation cancelled.' }); + return; + } + } + const identity: ToolIdentityParams = { command, ...(typeof toolRun === 'string' ? {} : { toolArgv: [...toolRun] }), diff --git a/lib/src/lib/external-link-confirmation.ts b/lib/src/lib/external-link-confirmation.ts index d8fab5301..1938274e1 100644 --- a/lib/src/lib/external-link-confirmation.ts +++ b/lib/src/lib/external-link-confirmation.ts @@ -17,6 +17,11 @@ export interface PendingExternalLink { displayText: string; verdict: DisplayMatchVerdict; decision: ExternalUriDecision; + /** What the dialog adds below the target. */ + note?: string; + /** Set when another flow awaits the decision: the dialog only answers, and + * that flow opens. Called once; a cancel, replacement, or abort answers false. */ + answer?: (confirmed: boolean) => void; } let nextId = 0; @@ -24,20 +29,64 @@ let pendingExternalLink: PendingExternalLink | null = null; const listeners = new Set<() => void>(); export function requestExternalLinkConfirmation(uri: string, displayText: string = '', source?: ExternalLinkSource): void { - pendingExternalLink = { + replacePending({ id: ++nextId, source, uri, displayText, verdict: classifyDisplayMatch(uri, displayText), decision: inspectExternalUri(uri), + }); +} + +/** What the dialog shows for a document link to the local file `path`. */ +export function documentLinkRequest(path: string): Omit<PendingExternalLink, 'id' | 'answer'> { + return { + uri: path, + displayText: '', + verdict: 'match', + decision: { status: 'openable', rawUri: path, uri: path, scheme: 'file', displayUri: path, otherApp: false }, + note: 'A link in the document you are viewing opens this file, which runs its own scripts and can read files beside it. Open it only if you expected this file.', }; - emitExternalLinkConfirmationChange(); +} + +/** + * Ask, in the same dialog, before a Tool's document link opens the local file + * `path`, shown in full; resolves whether the user confirmed. The caller does + * the open. Aborting `signal` withdraws the question. + */ +export function confirmDocumentLinkOpen(path: string, signal?: AbortSignal): Promise<boolean> { + return new Promise(resolve => { + if (signal?.aborted) return resolve(false); + let answered = false; + const request: PendingExternalLink = { + id: ++nextId, + ...documentLinkRequest(path), + answer: confirmed => { + if (answered) return; + answered = true; + signal?.removeEventListener('abort', withdraw); + resolve(confirmed); + }, + }; + const withdraw = () => { + if (pendingExternalLink === request) clearExternalLinkConfirmation(); + else request.answer!(false); + }; + signal?.addEventListener('abort', withdraw, { once: true }); + replacePending(request); + }); } export function clearExternalLinkConfirmation(): void { if (!pendingExternalLink) return; - pendingExternalLink = null; + replacePending(null); +} + +function replacePending(next: PendingExternalLink | null): void { + const previous = pendingExternalLink; + pendingExternalLink = next; + previous?.answer?.(false); emitExternalLinkConfirmationChange(); } diff --git a/lib/src/stories/ExternalLinkModal.stories.tsx b/lib/src/stories/ExternalLinkModal.stories.tsx index f20373c7f..d79a07845 100644 --- a/lib/src/stories/ExternalLinkModal.stories.tsx +++ b/lib/src/stories/ExternalLinkModal.stories.tsx @@ -2,8 +2,9 @@ import { useState, type ComponentProps } from 'react'; import type { Meta, StoryObj } from '@storybook/react'; import { ExternalLinkModal } from '../components/ExternalLinkModal'; import { classifyDisplayMatch, inspectExternalUri } from '../lib/external-links'; +import { documentLinkRequest } from '../lib/external-link-confirmation'; -function ExternalLinkModalStory({ uri, displayText, ...dialog }: { uri: string; displayText: string } & Pick<ComponentProps<typeof ExternalLinkModal>, 'handlers' | 'error' | 'busy'>) { +function ExternalLinkModalStory({ uri, displayText, documentLink = false, ...dialog }: { uri: string; displayText: string; documentLink?: boolean } & Pick<ComponentProps<typeof ExternalLinkModal>, 'handlers' | 'error' | 'busy'>) { const [selected, setSelected] = useState(0); return ( <div className="relative h-[360px] w-[680px] overflow-hidden rounded bg-app-bg font-mono text-terminal-fg"> @@ -15,7 +16,7 @@ function ExternalLinkModalStory({ uri, displayText, ...dialog }: { uri: string; {...dialog} selected={selected} onSelect={setSelected} - request={{ + request={documentLink ? documentLinkRequest(uri) : { uri, displayText, verdict: classifyDisplayMatch(uri, displayText), @@ -131,3 +132,13 @@ export const FileOpenFailed: Story = { error: 'no such file or folder: file:///Users/dev/project/image.png', }, }; + +// A Markdown link whose target runs its own scripts: the Tool's page named it, +// so the host asks with the full resolved path before it opens. +export const DocumentLinkToActivePage: Story = { + args: { + uri: '/Users/dev/Downloads/report/index.html', + displayText: '', + documentLink: true, + }, +}; diff --git a/scripts/security-rule-ids.json b/scripts/security-rule-ids.json index 2ab8934c3..18985fdaf 100644 --- a/scripts/security-rule-ids.json +++ b/scripts/security-rule-ids.json @@ -1,5 +1,5 @@ { - "next": 269, + "next": 270, "retired": [ 265 ] diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index d42150a04..beb9b74d7 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -8,8 +8,8 @@ "docs/specs/deploy.md": 2200, "docs/specs/dor-browser.md": 5800, "docs/specs/dor-cli.md": 5150, - "docs/specs/dor-tool.md": 6500, - "docs/specs/dor-tools-builtin.md": 1650, + "docs/specs/dor-tool.md": 6550, + "docs/specs/dor-tools-builtin.md": 1700, "docs/specs/dor-tools-lib.md": 350, "docs/specs/glossary.md": 2950, "docs/specs/hosted.md": 4300, @@ -27,10 +27,10 @@ "docs/specs/security-audit.md": 4700, "docs/specs/security-ci.md": 3350, "docs/specs/security-hosted.md": 2900, - "docs/specs/security-local.md": 5650, + "docs/specs/security-local.md": 5800, "docs/specs/security-remote.md": 6800, "docs/specs/security-supply-chain.md": 1600, - "docs/specs/security.md": 2350, + "docs/specs/security.md": 2400, "docs/specs/shortcuts.md": 1150, "docs/specs/standalone.md": 6450, "docs/specs/terminal-context.md": 950, From 1eb5dcf45aa470c3ec1cdfedae0cf34c36e27140 Mon Sep 17 00:00:00 2001 From: Ned Twigg <ned.twigg@diffplug.com> Date: Mon, 19 Oct 2026 17:46:34 -0700 Subject: [PATCH 4/4] Drop the shared control-character module; #1197 already covers C1 The new dor-lib-common subpath broke the standalone typecheck and the viewer bundle, which resolve it without a built dist/. The gates #1197 widened, dor's shell-quote, and the viewer's CONTROLS already cover C1, and the latter two stay dependency-free by design. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- .github/audit/canaries/unc-binary-path.patch | 16 ++++++++-------- dor-lib-common/package.json | 4 ---- dor-lib-common/src/browser-providers.ts | 12 ++++-------- dor-lib-common/src/control-characters.ts | 11 ----------- dor-lib-common/test/browser-providers.test.mjs | 15 ++------------- dor-tools-builtin/src/file-viewer-format.ts | 4 +--- dor/src/commands/shell-quote.ts | 13 ++++++------- lib/src/lib/agent-browser-binary.test.ts | 6 ++---- 8 files changed, 23 insertions(+), 58 deletions(-) delete mode 100644 dor-lib-common/src/control-characters.ts diff --git a/.github/audit/canaries/unc-binary-path.patch b/.github/audit/canaries/unc-binary-path.patch index bc373af8e..fc47ea412 100644 --- a/.github/audit/canaries/unc-binary-path.patch +++ b/.github/audit/canaries/unc-binary-path.patch @@ -4,10 +4,10 @@ Expect: dor-lib-common/src/browser-providers.ts Source: reverses #1040 (a UNC binaryPath passed the executable gate) diff --git a/dor-lib-common/src/browser-providers.ts b/dor-lib-common/src/browser-providers.ts -index 6dc70bbf6..c20ed4767 100644 +index 5ba6a6c46..1384983bf 100644 --- a/dor-lib-common/src/browser-providers.ts +++ b/dor-lib-common/src/browser-providers.ts -@@ -69,10 +69,10 @@ export const DEFAULT_PLAYWRIGHT_BIN = 'playwright-cli'; +@@ -67,10 +67,10 @@ export const DEFAULT_PLAYWRIGHT_BIN = 'playwright-cli'; const PLAYWRIGHT_FILENAME_RE = /^playwright-cli(?:\.(?:cmd|bat|exe|com|ps1))?$/i; const AGENT_BROWSER_FILENAME_RE = /^agent-browser(?:\.(?:cmd|bat|exe|com|ps1))?$/i; @@ -22,8 +22,8 @@ index 6dc70bbf6..c20ed4767 100644 /** * True for a path opening with two separators in either spelling: a UNC share -@@ -111,7 +111,7 @@ function isAllowedBrowserBinary(candidate: unknown, configuredPath: string | und - if (hasControlCharacter(candidate)) return false; +@@ -109,7 +109,7 @@ function isAllowedBrowserBinary(candidate: unknown, configuredPath: string | und + if (/[\u0000-\u001f\u007f-\u009f]/.test(candidate)) return false; if (configuredPath && candidate === configuredPath) return true; if (candidate === name) return true; - if (isUncOrDevicePath(candidate) || !ABSOLUTE_RE.test(candidate)) return false; @@ -32,7 +32,7 @@ index 6dc70bbf6..c20ed4767 100644 if (segments.includes('..')) return false; return filename.test(segments[segments.length - 1] ?? ''); diff --git a/lib/src/lib/agent-browser-binary.test.ts b/lib/src/lib/agent-browser-binary.test.ts -index 882cb1214..ddbb7d570 100644 +index 4bf54d1e7..c45055dda 100644 --- a/lib/src/lib/agent-browser-binary.test.ts +++ b/lib/src/lib/agent-browser-binary.test.ts @@ -35,14 +35,6 @@ describe('isAllowedAgentBrowserBinary', () => { @@ -50,11 +50,11 @@ index 882cb1214..ddbb7d570 100644 // Control characters are how one argument becomes two. '/usr/local/bin/agent-browser\n/bin/sh', '/usr/local/bin/agent-browser\u0000', -@@ -62,6 +54,6 @@ describe('isAllowedAgentBrowserBinary', () => { +@@ -61,6 +53,6 @@ describe('isAllowedAgentBrowserBinary', () => { it('validates Playwright executable hints independently of agent-browser', () => { for (const candidate of ['playwright-cli', '/opt/bin/playwright-cli', 'C:\\tools\\playwright-cli.cmd']) expect(isAllowedPlaywrightBinary(candidate)).toBe(true); -- for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/playwright-cli\u0085', '/opt/playwright-cli\u009f', '/opt/a\u0085b/playwright-cli', '\\\\host\\share\\playwright-cli.cmd', '//host/share/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); -+ for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/playwright-cli\u0085', '/opt/playwright-cli\u009f', '/opt/a\u0085b/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); +- for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/a\u0085b/playwright-cli', '\\\\host\\share\\playwright-cli.cmd', '//host/share/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); ++ for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/a\u0085b/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); expect(isAllowedPlaywrightBinary('/opt/custom-wrapper', '/opt/custom-wrapper')).toBe(true); }); diff --git a/dor-lib-common/package.json b/dor-lib-common/package.json index 13653bc1e..200f9bc91 100644 --- a/dor-lib-common/package.json +++ b/dor-lib-common/package.json @@ -16,10 +16,6 @@ "./browser-providers": { "types": "./dist/browser-providers.d.ts", "default": "./dist/browser-providers.js" - }, - "./control-characters": { - "types": "./dist/control-characters.d.ts", - "default": "./dist/control-characters.js" } }, "scripts": { diff --git a/dor-lib-common/src/browser-providers.ts b/dor-lib-common/src/browser-providers.ts index 6dc70bbf6..5ba6a6c46 100644 --- a/dor-lib-common/src/browser-providers.ts +++ b/dor-lib-common/src/browser-providers.ts @@ -9,8 +9,6 @@ * webview, the Node hosts, and `dor`. */ -import { CONTROL_CHARACTERS, hasControlCharacter } from './control-characters.js'; - /** A browser-automation provider: whose CLI drives a browser Surface. */ export type BrowserAutomationProvider = 'agent-browser' | 'playwright'; @@ -106,9 +104,9 @@ export function isAllowedPlaywrightBinary(candidate: unknown, configuredPath?: s function isAllowedBrowserBinary(candidate: unknown, configuredPath: string | undefined, name: string, filename: RegExp): candidate is string { if (typeof candidate !== 'string' || candidate.length === 0 || candidate.length > 4096) return false; - // Control characters have no place in a path and are how one argument - // becomes two on the platforms that take a command string. - if (hasControlCharacter(candidate)) return false; + // Control characters, C0, DEL and C1, have no place in a path and are how + // one argument becomes two on the platforms that take a command string. + if (/[\u0000-\u001f\u007f-\u009f]/.test(candidate)) return false; if (configuredPath && candidate === configuredPath) return true; if (candidate === name) return true; if (isUncOrDevicePath(candidate) || !ABSOLUTE_RE.test(candidate)) return false; @@ -125,14 +123,12 @@ export interface BrowserBinding { binaryPath?: string; } -const AGENT_BROWSER_SESSION_RE = new RegExp(`^(?!-)[^/\\\\${CONTROL_CHARACTERS}]{1,200}$`); - /** An agent-browser session name. `dor agent-browser --session` passes a user's raw name * through, so anything goes but what agent-browser would read as an option or * its socket directory as a path: the name lands after `--session` and in * `<socket dir>/<session>.pid`, whose pid a relaunch signals. */ function isAgentBrowserSession(value: unknown): value is string { - return typeof value === 'string' && AGENT_BROWSER_SESSION_RE.test(value); + return typeof value === 'string' && /^(?!-)[^/\\\x00-\x1f\x7f-\x9f]{1,200}$/.test(value); } /** A Playwright session name: Dormouse mints these, and the CLI takes them diff --git a/dor-lib-common/src/control-characters.ts b/dor-lib-common/src/control-characters.ts deleted file mode 100644 index 200f3e5d3..000000000 --- a/dor-lib-common/src/control-characters.ts +++ /dev/null @@ -1,11 +0,0 @@ -/** C0, DEL, and C1 controls, as a character class body. Every gate on a path or - * spawn input refuses the whole set, so a gate cannot miss the C1 half (U+0080 - * to U+009F), which some consumers read as a line break or an escape. */ -export const CONTROL_CHARACTERS = '\\x00-\\x1f\\x7f-\\x9f'; - -const CONTROL = new RegExp(`[${CONTROL_CHARACTERS}]`); - -/** Whether `value` holds any of `CONTROL_CHARACTERS`. */ -export function hasControlCharacter(value: string): boolean { - return CONTROL.test(value); -} diff --git a/dor-lib-common/test/browser-providers.test.mjs b/dor-lib-common/test/browser-providers.test.mjs index 76bfb8cb1..efeed988d 100644 --- a/dor-lib-common/test/browser-providers.test.mjs +++ b/dor-lib-common/test/browser-providers.test.mjs @@ -1,6 +1,6 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { BROWSER_PROVIDER_IDS, BROWSER_PROVIDERS, isAllowedAgentBrowserBinary, isAllowedPlaywrightBinary, parseRenderMode, parseStreamPort, renderModeFor, sessionForKey } from '../dist/browser-providers.js'; +import { BROWSER_PROVIDER_IDS, BROWSER_PROVIDERS, parseRenderMode, parseStreamPort, renderModeFor, sessionForKey } from '../dist/browser-providers.js'; test('sessionForKey namespaces a key under the workspace', () => { assert.equal(sessionForKey('default'), 'dormouse.1.default'); @@ -39,7 +39,7 @@ test('each provider refuses a session name its CLI would read as an option or a for (const provider of BROWSER_PROVIDER_IDS) { const { isSessionName } = BROWSER_PROVIDERS[provider]; assert.equal(isSessionName('dormouse.1.default'), true, provider); - for (const bad of ['../../tmp/evil', 'a/b', 'a\\b', 'a\nb', 'a\u0085b', 'a\u009fb', '', 7]) { + for (const bad of ['../../tmp/evil', 'a/b', 'a\\b', 'a\nb', 'a\u0085b', '', 7]) { assert.equal(isSessionName(bad), false, `${provider} ${JSON.stringify(bad)}`); } } @@ -58,14 +58,3 @@ test('parseStreamPort reads a top-level or nested port, and nothing from malform assert.equal(parseStreamPort(stdout), undefined, stdout); } }); - -test('the binary gates refuse a C0, DEL, or C1 control anywhere in the path', () => { - for (const control of ['\u0000', '\n', '\u007f', '\u0085', '\u009f']) { - assert.equal(isAllowedAgentBrowserBinary(`/usr/local/bin/agent-browser${control}`), false, JSON.stringify(control)); - assert.equal(isAllowedAgentBrowserBinary(`/usr/local/bin${control}/agent-browser`), false, JSON.stringify(control)); - assert.equal(isAllowedPlaywrightBinary(`/opt/bin/playwright-cli${control}`), false, JSON.stringify(control)); - } - // The neighbours of the C1 block are ordinary path characters. - assert.equal(isAllowedAgentBrowserBinary('/opt/\u00a0/agent-browser'), true); - assert.equal(isAllowedAgentBrowserBinary('/opt/~/agent-browser'), true); -}); diff --git a/dor-tools-builtin/src/file-viewer-format.ts b/dor-tools-builtin/src/file-viewer-format.ts index 01d1f74c5..21da08071 100644 --- a/dor-tools-builtin/src/file-viewer-format.ts +++ b/dor-tools-builtin/src/file-viewer-format.ts @@ -1,5 +1,3 @@ -import { CONTROL_CHARACTERS } from 'dor-lib-common/control-characters'; - /** Formats supported by the built-in local viewer. Unknown files need a user * Tool association rather than being guessed to be text. */ const MIME: Record<string, string> = { @@ -95,7 +93,7 @@ export function defaultBuiltin(folder: boolean): BuiltinHandler { } /** C0, DEL, and C1 controls. */ -export const CONTROLS = new RegExp(`[${CONTROL_CHARACTERS}]`); +export const CONTROLS = /[\x00-\x1f\x7f-\x9f]/; /** The title naming a viewer's canonical `target`, `/` or `\` separated: its * basename, or the whole path for a filesystem root. Controls are stripped, diff --git a/dor/src/commands/shell-quote.ts b/dor/src/commands/shell-quote.ts index 5f7c7d256..c55f01c15 100644 --- a/dor/src/commands/shell-quote.ts +++ b/dor/src/commands/shell-quote.ts @@ -1,15 +1,14 @@ /** - * Pure shell quoting. The webview (the only layer that knows the target pane's - * shell) turns a raw argv array into a single command string for that shell. - * Keep this module free of Node imports so it can be bundled into the - * browser-side webview as well as the CLI. + * Pure, dependency-free shell quoting. The webview (the only layer that knows + * the target pane's shell) turns a raw argv array into a single command string + * for that shell. Keep this module free of Node imports so it can be bundled + * into the browser-side webview as well as the CLI. */ -import { CONTROL_CHARACTERS } from 'dor-lib-common/control-characters'; - export type ShellCommandKind = 'cmd' | 'posix' | 'powershell'; -export { CONTROL_CHARACTERS }; +/** C0, DEL, and C1, as a character class body. */ +export const CONTROL_CHARACTERS = '\\x00-\\x1f\\x7f-\\x9f'; /** Unicode format characters that reorder or hide the text around them: every * bidi control, the zero-width space and joiners, the word joiner and * invisible operators, and the BOM — the set `boundedPushText` in diff --git a/lib/src/lib/agent-browser-binary.test.ts b/lib/src/lib/agent-browser-binary.test.ts index 5a75c1225..4bf54d1e7 100644 --- a/lib/src/lib/agent-browser-binary.test.ts +++ b/lib/src/lib/agent-browser-binary.test.ts @@ -46,10 +46,8 @@ describe('isAllowedAgentBrowserBinary', () => { // Control characters are how one argument becomes two. '/usr/local/bin/agent-browser\n/bin/sh', '/usr/local/bin/agent-browser\u0000', - // C1 too: U+0085 is a line break to some readers, U+009F ends the set. - '/usr/local/bin/agent-browser\u0085/bin/sh', - '/usr/local/bin/agent-browser\u009f', '/opt/a\u0085b/agent-browser', + '/opt/a\u009fb/agent-browser', '', undefined, null, @@ -63,6 +61,6 @@ describe('isAllowedAgentBrowserBinary', () => { it('validates Playwright executable hints independently of agent-browser', () => { for (const candidate of ['playwright-cli', '/opt/bin/playwright-cli', 'C:\\tools\\playwright-cli.cmd']) expect(isAllowedPlaywrightBinary(candidate)).toBe(true); - for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/playwright-cli\u0085', '/opt/playwright-cli\u009f', '/opt/a\u0085b/playwright-cli', '\\\\host\\share\\playwright-cli.cmd', '//host/share/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); + for (const candidate of ['agent-browser', '/bin/sh', './playwright-cli', '/opt/../playwright-cli', '/opt/playwright-cli\n', '/opt/a\u0085b/playwright-cli', '\\\\host\\share\\playwright-cli.cmd', '//host/share/playwright-cli']) expect(isAllowedPlaywrightBinary(candidate)).toBe(false); expect(isAllowedPlaywrightBinary('/opt/custom-wrapper', '/opt/custom-wrapper')).toBe(true); });