diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index 4bec2ace..cf56c6b9 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -187,6 +187,17 @@ Every handler that takes a renderer-supplied path or derives a spawn location fr | `git-changes-save` | `isSafeRepoRelativePath` + the same `resolveTargetInsideRepo`, plus a version token that must still match the bytes on disk; the write runs on the path the guard returned, never on a re-derived one | shape + disk-resolved containment + denylist — **the only write handler in the app whose entire input is a relative path from the renderer**, so containment is the guard, not an afterthought; `save-file-for-panel` next to it has none (it takes an absolute path and checks only `isSensitivePath`) and is not the precedent to copy here | | `git-changes-diff` | `isSafeGitPath`, or `isSafeNoIndexPath` + containment when `untracked` (`git-changes-runner.js`) | a git pathspec relative to an arbitrary (possibly remote) cwd; see `.ai/contexts/changes-view.md` ("Quoting rule") for why this is a denylist, not an allowlist. The untracked variant is a real filesystem operand of `git diff --no-index`, which has no repository-boundary check of its own: on top of the syntactic guard it is resolved with `realpath`/`stat` against the resolved cwd (local) or checked against `git ls-files --others` (remote), git receives the guard's operand rather than the caller's, and the returned diff must name that same path in its `diff --git` line — see "Untracked files" in the same doc | +### Sensitive-path candidates + +`isSensitivePath` and `isSensitivePathAsync` (`ipc-path-validator.js`) share one candidate builder, `sensitiveCandidates` / `sensitiveCandidatesAsync` (`resolve-path-on-disk.js`), and test the denylist against every spelling it returns: + +- the literal path, and the same path with a Windows `\\?\` / `\\.\` prefix removed (`\\?\UNC\host\share` becomes `\\host\share`); +- the JS realpath **and** the native realpath. They diverge on Windows: the JS walker keeps 8.3 short names (`SSH~1` for `.ssh`) and throws `EISDIR` on a `\\?\` path through a junction, while `fs.realpath.native` expands the short name and resolves the junction. Either one alone misses a spelling the other catches, so both are tried and every result is matched; +- on win32, the same path with trailing dots and spaces removed from each segment (`.ssh.\id_rsa`, `.netrc `): Win32 normalisation drops them, so `shell.openPath` reaches the real file while the literal string matches nothing. Not applied to `\\?\` paths, which Win32 does not normalise (the `\\.\` device prefix is normalised, so those are trimmed); +- when nothing exists at the path yet (a file about to be saved), the first existing ancestor — found with one `lstat` per level going up, then resolved the two ways — with the missing tail re-attached, otherwise `SSH~1\new-key` would pass because there is nothing to resolve. + +Fail closed: if **either** realpath fails for a reason other than `ENOENT`/`ENOTDIR` (`EACCES`, `EPERM`, `ELOOP`, ...), or an ancestor cannot be `lstat`ed, the path is treated as sensitive even when the other realpath succeeded: the one that failed may have been the one that sees the credential location. `resolveOnDisk` / `resolveOnDiskAsync` are unchanged and stay on the JS realpath (the allowlist and containment checks compare against it). The tests are in `test/sensitive-path-windows.test.js`; the 8.3 cases skip, with a stated reason, on a volume that generates no short names. + ### Non-obvious behaviors - **`preload.js` is the *single* surface the renderer sees**. If you add `ipcMain.handle('xyz', ...)` but forget to add `xyz: () => ipcRenderer.invoke('xyz')` in preload, the renderer can't call it. Symptom: `window.api.xyz is not a function`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 39b57bd1..1b62aaf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ What changes for you in each release of Switchboard. How to write an entry: [doc ## Unreleased +### Fixed +- On Windows, the file panel no longer opens or saves a credential file (such as one under `.ssh`) through its 8.3 short name or a `\\?\` path. (#390) ### New - A session's Changes panel also lists the changes in the worktrees its subagents are working in, under a header naming the agent and its branch. Those rows open as read-only diffs; a subagent that works in the session's own directory adds nothing. (#303) - A live session on a remote host that is not open in a terminal has a Send a prompt… button on its row: type a text and it is written to the running session as a new prompt, without attaching. It needs `ncat` or an OpenBSD `nc` on the host, and is refused for a Windows host. The dialog says "Sent": the session's own status shows whether it picked the prompt up. (#219) diff --git a/ipc-path-validator.js b/ipc-path-validator.js index bf1e89b0..dbd2f931 100644 --- a/ipc-path-validator.js +++ b/ipc-path-validator.js @@ -28,7 +28,7 @@ const os = require('os'); const path = require('path'); -const { resolveOnDisk, resolveOnDiskAsync } = require('./resolve-path-on-disk'); +const { resolveOnDisk, sensitiveCandidates, sensitiveCandidatesAsync } = require('./resolve-path-on-disk'); const CLAUDE_DIR = path.join(os.homedir(), '.claude'); @@ -61,28 +61,17 @@ const SENSITIVE_PATH_PATTERNS = [ * @returns {boolean} */ function isSensitivePath(filePath) { - const resolved = path.resolve(filePath); - if (SENSITIVE_PATH_PATTERNS.some(pattern => pattern.test(resolved))) return true; - - // Also test the on-disk real path: a symlink (e.g. a "notes" directory that - // is actually a link to ~/.ssh) makes the literal string look harmless - // while the file it opens is not. Skipped when nothing exists there yet — - // see the file header. - const real = resolveOnDisk(resolved); - if (real && real !== resolved) { - return SENSITIVE_PATH_PATTERNS.some(pattern => pattern.test(real)); - } - return false; + const { paths, unresolved } = sensitiveCandidates(filePath); + return unresolved || matchesDenylist(paths); } async function isSensitivePathAsync(filePath) { - const resolved = path.resolve(filePath); - if (SENSITIVE_PATH_PATTERNS.some(pattern => pattern.test(resolved))) return true; - const real = await resolveOnDiskAsync(resolved); - if (real && real !== resolved) { - return SENSITIVE_PATH_PATTERNS.some(pattern => pattern.test(real)); - } - return false; + const { paths, unresolved } = await sensitiveCandidatesAsync(filePath); + return unresolved || matchesDenylist(paths); +} + +function matchesDenylist(paths) { + return paths.some(candidate => SENSITIVE_PATH_PATTERNS.some(pattern => pattern.test(candidate))); } /** diff --git a/resolve-path-on-disk.js b/resolve-path-on-disk.js index e1a351a9..43570145 100644 --- a/resolve-path-on-disk.js +++ b/resolve-path-on-disk.js @@ -57,6 +57,105 @@ async function resolveOnDiskAsync(filePath) { } } +const EXTENDED_UNC = /^[\\/]{2}[?.][\\/]UNC[\\/]/i; +const EXTENDED_DRIVE = /^[\\/]{2}[?.][\\/](?=[A-Za-z]:)/; +const VERBATIM_PREFIX = /^[\\/]{2}\?[\\/]/; +const MISSING = new Set(['ENOENT', 'ENOTDIR']); + +// see .ai/contexts/ipc-bridge.md, "Sensitive-path candidates" +function stripExtendedPrefix(p) { + if (typeof p !== 'string') return p; + if (EXTENDED_UNC.test(p)) return '\\\\' + p.replace(EXTENDED_UNC, ''); + return p.replace(EXTENDED_DRIVE, ''); +} + +function attempt(fn, arg) { + try { return { real: fn(arg) }; } catch (e) { return { code: e && e.code }; } +} + +function attemptAsync(fn, arg) { + return new Promise((resolve) => { + try { + fn(arg, (e, real) => resolve(e ? { code: e.code } : { real })); + } catch (e) { + resolve({ code: e && e.code }); + } + }); +} + +function stripTrailingDotsAndSpaces(p) { + const root = path.parse(p).root; + const rest = p.slice(root.length).split(path.sep).map((seg) => seg.replace(/[. ]+$/, '')); + return root + rest.join(path.sep); +} + +function literalsOf(filePath) { + const literal = path.resolve(stripExtendedPrefix(filePath)); + const out = [literal]; + if (process.platform === 'win32' && !VERBATIM_PREFIX.test(filePath)) { + const trimmed = stripTrailingDotsAndSpaces(literal); + if (trimmed !== literal) out.push(trimmed); + } + return out; +} + +const failed = (results) => results.some((r) => !r.real && !MISSING.has(r.code)); + +function addResolved(paths, results, tail) { + for (const r of results) if (r.real) paths.add(tail ? path.join(r.real, tail) : r.real); +} + +// see .ai/contexts/ipc-bridge.md, "Sensitive-path candidates" +function sensitiveCandidates(filePath) { + const paths = new Set([path.resolve(filePath)]); + let unresolved = false; + for (const literal of literalsOf(filePath)) { + paths.add(literal); + const first = [attempt(fs.realpathSync, literal), attempt(fs.realpathSync.native, literal)]; + if (failed(first)) { unresolved = true; continue; } + if (first.some((r) => r.real)) { addResolved(paths, first); continue; } + let dir = literal; + for (;;) { + const parent = path.dirname(dir); + if (parent === dir) break; + dir = parent; + const st = attempt(fs.lstatSync, dir); + if (st.code && MISSING.has(st.code)) continue; + if (st.code) { unresolved = true; break; } + const results = [attempt(fs.realpathSync, dir), attempt(fs.realpathSync.native, dir)]; + if (failed(results)) unresolved = true; + else addResolved(paths, results, path.relative(dir, literal)); + break; + } + } + return { paths: [...paths], unresolved }; +} + +async function sensitiveCandidatesAsync(filePath) { + const paths = new Set([path.resolve(filePath)]); + let unresolved = false; + for (const literal of literalsOf(filePath)) { + paths.add(literal); + const first = [await attemptAsync(fs.realpath, literal), await attemptAsync(fs.realpath.native, literal)]; + if (failed(first)) { unresolved = true; continue; } + if (first.some((r) => r.real)) { addResolved(paths, first); continue; } + let dir = literal; + for (;;) { + const parent = path.dirname(dir); + if (parent === dir) break; + dir = parent; + const st = await attemptAsync(fs.lstat, dir); + if (st.code && MISSING.has(st.code)) continue; + if (st.code) { unresolved = true; break; } + const results = [await attemptAsync(fs.realpath, dir), await attemptAsync(fs.realpath.native, dir)]; + if (failed(results)) unresolved = true; + else addResolved(paths, results, path.relative(dir, literal)); + break; + } + } + return { paths: [...paths], unresolved }; +} + /** * True when `child` is `parent` itself or lies beneath it. * @@ -76,4 +175,4 @@ function isInsideDir(child, parent) { return c === p || c.startsWith(p + path.sep); } -module.exports = { resolveOnDisk, resolveOnDiskAsync, isInsideDir }; +module.exports = { resolveOnDisk, resolveOnDiskAsync, isInsideDir, stripExtendedPrefix, stripTrailingDotsAndSpaces, sensitiveCandidates, sensitiveCandidatesAsync }; diff --git a/test/sensitive-path-windows.test.js b/test/sensitive-path-windows.test.js new file mode 100644 index 00000000..101a3c69 --- /dev/null +++ b/test/sensitive-path-windows.test.js @@ -0,0 +1,197 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); + +const { stripExtendedPrefix } = require('../resolve-path-on-disk'); +const { isSensitivePath, isSensitivePathAsync } = require('../ipc-path-validator'); + +const BS = String.fromCharCode(92); +const EXT = BS + BS + '?' + BS; +const DEV = BS + BS + '.' + BS; + +test('stripExtendedPrefix drops the \\\\?\\ and \\\\.\\ drive prefixes', () => { + assert.strictEqual(stripExtendedPrefix(EXT + 'C:' + BS + 'a' + BS + 'b'), 'C:' + BS + 'a' + BS + 'b'); + assert.strictEqual(stripExtendedPrefix(DEV + 'C:' + BS + 'a'), 'C:' + BS + 'a'); +}); + +test('stripExtendedPrefix turns \\\\?\\UNC\\host\\share into \\\\host\\share, whatever the case', () => { + assert.strictEqual(stripExtendedPrefix(EXT + 'UNC' + BS + 'h' + BS + 's' + BS + 'a'), BS + BS + 'h' + BS + 's' + BS + 'a'); + assert.strictEqual(stripExtendedPrefix(EXT + 'unc' + BS + 'h' + BS + 's'), BS + BS + 'h' + BS + 's'); +}); + +test('stripExtendedPrefix leaves ordinary, relative and plain UNC paths alone', () => { + for (const p of ['C:' + BS + 'a', 'a' + BS + 'b', BS + BS + 'h' + BS + 's' + BS + 'a', '/home/u/.ssh/k', '']) { + assert.strictEqual(stripExtendedPrefix(p), p); + } +}); + +const win = process.platform === 'win32'; + +function shortPathOf(dir) { + const out = execFileSync('powershell', [ + '-NoProfile', '-Command', + '(New-Object -ComObject Scripting.FileSystemObject).GetFolder($env:SB_P).ShortPath', + ], { encoding: 'utf8', env: { ...process.env, SB_P: dir }, timeout: 30000 }); + return out.trim(); +} + +const root = fs.mkdtempSync(path.join(os.tmpdir(), 'sb-sens-')); +const realRoot = fs.realpathSync.native(root); +const sshDir = path.join(realRoot, '.ssh'); +fs.mkdirSync(sshDir); +fs.writeFileSync(path.join(sshDir, 'id_rsa'), 'key\n'); + +let shortSsh = null; +let junction = null; +if (win) { + try { + const s = shortPathOf(sshDir); + if (s && s.toLowerCase() !== sshDir.toLowerCase() && fs.existsSync(path.join(s, 'id_rsa'))) shortSsh = s; + } catch {} + const j = path.join(realRoot, 'notes'); + try { fs.symlinkSync(sshDir, j, 'junction'); junction = j; } catch {} +} + +test.after(() => { + if (junction) fs.rmdirSync(junction); + fs.rmSync(root, { recursive: true, force: true }); +}); + +const bothGuards = async (p) => [isSensitivePath(p), await isSensitivePathAsync(p)]; + +test('a plain file next to the credential directory stays allowed', async () => { + const ok = path.join(realRoot, 'plain.txt'); + fs.writeFileSync(ok, 'x\n'); + assert.deepStrictEqual(await bothGuards(ok), [false, false]); +}); + +test('an 8.3 short name for a credential directory is refused by both guards', + { skip: !win ? 'win32 only' : !shortSsh && 'this volume generates no 8.3 short names' }, + async () => { + assert.deepStrictEqual(await bothGuards(path.join(shortSsh, 'id_rsa')), [true, true]); + }); + +test('an 8.3 short name for a credential directory is refused for a file not created yet', + { skip: !win ? 'win32 only' : !shortSsh && 'this volume generates no 8.3 short names' }, + async () => { + assert.deepStrictEqual(await bothGuards(path.join(shortSsh, 'not-yet')), [true, true]); + }); + +test('a \\\\?\\ path through a junction into a credential directory is refused by both guards', + { skip: !win ? 'win32 only' : !junction && 'cannot create a junction here' }, + async () => { + assert.deepStrictEqual(await bothGuards(EXT + path.join(junction, 'id_rsa')), [true, true]); + }); + +test('a \\\\?\\ path through a junction is still refused when only the JS realpath can resolve it', + { skip: !win ? 'win32 only' : !junction && 'cannot create a junction here' }, + async () => { + const nativeSync = fs.realpathSync.native; + const nativeAsync = fs.realpath.native; + const missing = () => { throw Object.assign(new Error('missing'), { code: 'ENOENT' }); }; + fs.realpathSync.native = missing; + fs.realpath.native = (p, cb) => cb(Object.assign(new Error('missing'), { code: 'ENOENT' })); + try { + assert.deepStrictEqual(await bothGuards(EXT + path.join(junction, 'id_rsa')), [true, true]); + } finally { + fs.realpathSync.native = nativeSync; + fs.realpath.native = nativeAsync; + } + }); + +test('a \\\\?\\ path with an 8.3 credential directory is refused by both guards', + { skip: !win ? 'win32 only' : !shortSsh && 'this volume generates no 8.3 short names' }, + async () => { + assert.deepStrictEqual(await bothGuards(EXT + path.join(shortSsh, 'id_rsa')), [true, true]); + }); + +test('a \\\\?\\ path to an ordinary file is not refused', { skip: !win && 'win32 only' }, async () => { + assert.deepStrictEqual(await bothGuards(EXT + path.join(realRoot, 'plain.txt')), [false, false]); +}); + +test('a realpath failure other than "does not exist" fails closed in both guards', { skip: !win && 'win32 only' }, async () => { + const target = path.join(realRoot, 'plain.txt'); + const fail = () => { throw Object.assign(new Error('denied'), { code: 'EACCES' }); }; + const realSync = fs.realpathSync; + const nativeSync = fs.realpathSync.native; + const realAsync = fs.realpath; + const nativeAsync = fs.realpath.native; + const failCb = (p, cb) => cb(Object.assign(new Error('denied'), { code: 'EACCES' })); + fs.realpathSync = Object.assign(fail, { native: fail }); + fs.realpath = Object.assign(failCb, { native: failCb }); + try { + assert.strictEqual(isSensitivePath(target), true); + assert.strictEqual(await isSensitivePathAsync(target), true); + } finally { + fs.realpathSync = Object.assign(realSync, { native: nativeSync }); + fs.realpath = Object.assign(realAsync, { native: nativeAsync }); + } +}); + +test('a missing path under an ordinary directory is not refused for failing to resolve', { skip: !win && 'win32 only' }, async () => { + const missing = path.join(realRoot, 'nowhere', 'file.txt'); + assert.deepStrictEqual(await bothGuards(missing), [false, false]); +}); + +test('a failure of one realpath fails closed even when the other resolves', { skip: !win && 'win32 only' }, async () => { + const target = path.join(realRoot, 'plain.txt'); + const nativeSync = fs.realpathSync.native; + const nativeAsync = fs.realpath.native; + fs.realpathSync.native = () => { throw Object.assign(new Error('denied'), { code: 'EPERM' }); }; + fs.realpath.native = (p, cb) => cb(Object.assign(new Error('denied'), { code: 'EPERM' })); + try { + assert.deepStrictEqual(await bothGuards(target), [true, true]); + } finally { + fs.realpathSync.native = nativeSync; + fs.realpath.native = nativeAsync; + } +}); + +test('trailing dots and spaces on a credential name are refused by both guards', { skip: !win && 'win32 only' }, async () => { + for (const name of [path.join('.ssh.', 'id_rsa'), '.git-credentials.', '.netrc ', path.join('.ssh. ', 'x')]) { + assert.deepStrictEqual(await bothGuards(path.join(realRoot, name)), [true, true], name); + } +}); + +test('stripTrailingDotsAndSpaces trims each segment and keeps the root', { skip: !win && 'win32 only' }, () => { + const { stripTrailingDotsAndSpaces } = require('../resolve-path-on-disk'); + assert.strictEqual(stripTrailingDotsAndSpaces('C:' + BS + 'a. ' + BS + 'b.' + BS + 'c '), 'C:' + BS + 'a' + BS + 'b' + BS + 'c'); +}); + +test('a missing file is resolved with a bounded number of realpath calls', { skip: !win && 'win32 only' }, () => { + const real = fs.realpathSync; + const nativeSync = real.native; + let calls = 0; + const count = (fn) => Object.assign((...a) => { calls++; return fn(...a); }, { native: nativeSync }); + fs.realpathSync = count(real); + fs.realpathSync.native = (...a) => { calls++; return nativeSync(...a); }; + try { + isSensitivePath(path.join(realRoot, 'a', 'b', 'c', 'd', 'e', 'f', 'file.txt')); + } finally { + fs.realpathSync = Object.assign(real, { native: nativeSync }); + } + assert.ok(calls <= 6, 'realpath calls: ' + calls); +}); + +test('trailing dots on a credential name are refused behind a \\.\ prefix, which Win32 still normalises', { skip: !win && 'win32 only' }, async () => { + const drive = realRoot.slice(0, 2); + const rest = realRoot.slice(2); + const spellings = [ + DEV + path.join(realRoot, '.ssh.', 'id_rsa'), + '//./' + path.join(realRoot, '.git-credentials.').split(BS).join('/'), + DEV + 'UNC' + BS + 'localhost' + BS + drive[0] + '$' + path.join(rest, '.ssh.', 'id_rsa'), + ]; + for (const p of spellings) { + assert.deepStrictEqual(await bothGuards(p), [true, true], p); + } +}); + +test('trailing dots are not trimmed behind a \\?\ prefix, which Win32 does not normalise', { skip: !win && 'win32 only' }, async () => { + const p = EXT + path.join(realRoot, 'notes.'); + assert.deepStrictEqual(await bothGuards(p), [false, false]); +});