diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index 99175d97..0e0a69cf 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -134,7 +134,7 @@ adapter applies it. - **Path-touching IPCs (`read-work-file`, `delete-work-file`, `read-memory`, `run-schedule-now`, `delete-worktree`, etc.) MUST guard their paths, and the guard must resolve on disk, not just `path.resolve()`**. `path.resolve()` normalises `..` but does not follow symlinks — a path can look contained by every string check and still open a file somewhere else through a symlinked directory. The shared primitive is `resolveOnDisk()` in `resolve-path-on-disk.js` (realpath, or `null` when nothing exists there yet); it is called *inside* the guards, not duplicated at each call site: - `isSensitivePath` / `resolveAllowedMemoryPath` (+ its boolean form `isAllowedMemoryPath`) / `isKnownProjectRoot` (`ipc-path-validator.js`) — denylist, allowlist, and exact-match-against-known-projects, used by the memory, file-panel and worktree handlers. `resolveAllowedMemoryPath` returns the resolved path, not a boolean, and callers that go on to read/write MUST use that returned value for the operation — a caller that re-resolves its own literal path afterwards reopens a TOCTOU (checked path and used path can diverge if a symlinked ancestor is swapped in between). See `resolve-path-on-disk.js` for the documented gap when the target does not exist yet. - `resolveDeletionTargets` (`delete-session-target.js`) — session deletion, the first handler to need this and the source the primitive was extracted from. - - `resolveRunNowTarget` (`run-schedule-now-target.js`) — `run-schedule-now`'s shape + allowlist check, run before any read or spawn. + - `resolveRunNowTarget` (`run-schedule-now-target.js`) — `run-schedule-now`'s shape + allowlist check, run before any read or spawn. Four duties, and which path each one answers for matters: the **directory shape** (`.claude/commands` parents) is checked on the *requested* path, because what makes a file a schedule is the directory that lists it, and a versioned configuration is linked in there from a repo elsewhere on disk; the **filename** is checked on the requested path *and again on the disk-resolved target*, so the listing directory decides where a schedule may be linked from but never what it may point at; `isSensitivePath` refuses a resolved target in a credential location; and `isAllowedMemoryPath` runs on the disk-resolved file **and** on the disk-resolved project root the run is spawned in, which with a linked-in schedule is not derivable from the file. Dropping any one of the four is caught by `test/run-schedule-now-target.test.js`. Work-files still use the narrower `.includes('/.work-files/')` substring check — same weakness against a symlinked ancestor, not yet migrated (see "IPC path-guard inventory" below). A handler that reads *content* back to the renderer (file panel, memory) and a handler that only decides *whether to run a command* (`run-schedule-now`) both need a guard, and neither one is safe with `path.resolve()` alone. - **Trust boundary is the contextBridge call**. Anything passed across must survive structured-clone serialization. No functions, no DOM nodes, no class instances — only plain JSON. - **Async handlers return promises**. Renderer uses `await window.api.foo(...)`. Throws cross the boundary as rejected promises; return `{ok, error}` if you want graceful failure handling on the renderer side. @@ -147,10 +147,11 @@ Every handler that takes a renderer-supplied path or derives a spawn location fr | IPC | Guard | Kind | |---|---|---| | `read-memory` / `save-memory` | `resolveAllowedMemoryPath` (read/write the returned path, not the caller's own re-resolved one) | disk-resolved allowlist | +| `get-memories` (the list, and the FTS bodies it indexes) | `scanMdFiles` (`scan-md-files.js`) applies `isSensitivePath` itself and the `isAllowedMemoryPath` predicate `get-memories` passes in | disk-resolved denylist + allowlist — a listed file is read twice more downstream, once by `read-memory` behind `resolveAllowedMemoryPath` and once by the FTS indexer behind nothing at all, so the guard belongs on the list. The denylist is not the caller's to choose: containment in an allowed project root says nothing about a cloned repo's own `.ssh`/`.env` | | `open-path` / `read-file-for-panel` / `save-file-for-panel` / `watch-file` | `isSensitivePath` | disk-resolved denylist | | `delete-worktree` / `worktree-status` | `WORKTREE_PATH_RE` (shape) + `isKnownProjectRoot` (disk-resolved exact match) | shape + disk-resolved allowlist | | `delete-session-preview` / `delete-session` | `resolveDeletionTargets` (`delete-session-target.js`) | disk-resolved containment | -| `run-schedule-now` | `resolveRunNowTarget` (`run-schedule-now-target.js`), which composes filename shape + `isAllowedMemoryPath` | disk-resolved allowlist — **the only handler in the app that both reads a file and spawns a process from a renderer-supplied path; had no guard at all before this pass** | +| `run-schedule-now` | `resolveRunNowTarget` (`run-schedule-now-target.js`), which composes directory shape (requested path) + filename shape (requested **and** resolved) + `isSensitivePath` + `isAllowedMemoryPath` on both the resolved file and the resolved project root | disk-resolved allowlist + denylist — **the only handler in the app that both reads a file and spawns a process from a renderer-supplied path; the file's content becomes a prompt sent to the model, so the resolved target is constrained, not just its location** | | `read-work-file` / `delete-work-file` | `.includes('/.work-files/')` substring | ad hoc string, **not disk-resolved** — a symlinked ancestor defeats it the same way it defeated `isSensitivePath`/`isAllowedMemoryPath` before they were fixed here. Not migrated in this pass; same fix shape (`resolveOnDisk` + a `.work-files` component check instead of a substring test) would close it | | `read-activity-trace-file` / `delete-activity-trace-file` | `resolveTraceFilePath` (`activity-trace.js`) | ad hoc string (directory + basename pattern), **not disk-resolved** — narrower surface (one generated file family) lowers the stakes, not migrated | | `add-project` / `remap-project` | none on the probe (`fs.statSync`/`fs.existsSync`/`fs.lstatSync`); the actual write is confined through `encodeProjectPath` | existence/type oracle only — inherent to the feature (both accept an arbitrary disk location by design), not cheaply fixable without breaking it | diff --git a/.ai/contexts/schedule-runner.md b/.ai/contexts/schedule-runner.md index e9a21002..d89838dd 100644 --- a/.ai/contexts/schedule-runner.md +++ b/.ai/contexts/schedule-runner.md @@ -53,6 +53,8 @@ cli: ## Non-obvious behaviors +- **A schedule file may be a symlink.** `scanSchedules` lists names and reads through them, so a `schedule-*.md` linked in from a versioned dotfiles repo fires on cron like any other. The two places that *don't* go through that scanner had to be taught the same thing: the brain tab's listing (`scan-md-files.js` — accepted entries on `dirent.isFile()`, false for a symlink, so a linked-in schedule was invisible in the UI while still firing weekly) and the run-now guard (`run-schedule-now-target.js` — checked the `.claude/commands` shape against the resolved target instead of the listed path). If you add a third reader of these files, resolve the link rather than the dirent. +- **`run-schedule-now` resolves the file and the cwd separately, and they may differ.** `resolveRunNowTarget` takes the schedule's bytes from the link's target and roots the spawn at the project whose `.claude/commands` lists it; each is disk-resolved and allowlisted in its own right, neither is derived from the other. With `projA/.claude/commands/schedule-x.md -> projB/.claude/commands/schedule-x.md` and both projects known, the run executes projB's content with `cwd = projA`. That is the semantic, not an oversight — `test/run-schedule-now-target.test.js` pins it so it is not "corrected" in either direction by accident. - **Hand-rolled cron parser** in `cronFieldMatches` / `cronMatches`. Supports `*`, comma lists (`1,2,3`), ranges (`1-5`), steps (`*/5`). No support for `@daily`/`@hourly` aliases. No DST awareness — `new Date()` is local-time. - **No persistence across app close** — the scheduler runs in-process. If Switchboard isn't running at 9am, the 9am schedule doesn't fire. By design (this is a personal tool, not a daemon). - **The "schedule creator" is itself a Claude command**: when the user clicks the clock icon on a project, Switchboard opens an interactive Claude session pre-injected with `SCHEDULE_CREATOR_TEMPLATE` as its system prompt. Claude then has a conversation with the user about what they want scheduled, and **Claude itself writes the schedule `.md` file** with the Write tool. The runner just consumes whatever files appear. @@ -62,6 +64,7 @@ cli: - `public/dialogs.js` (`launchScheduleCreator`) — UI entry point for the schedule creator flow - `public/memory-workfiles-view.js` brain tab — lists existing `schedule-*.md` files, surfaces the "run now" play button +- `scan-md-files.js` — what that brain tab list is actually built from (`get-memories` in `main.js`); it decides whether a schedule file is visible at all, and takes the memory allowlist so the list carries nothing the readers behind it would refuse to open - `public/sidebar.js` — `.project-schedule-btn` clock icon wiring per project - `schedule-ipc.js` `SCHEDULE_CREATOR_TEMPLATE` — if you change the schedule file format, update the template's instructions - `main.js:2517` (or wherever `startScheduler(log, runScheduleCommand)` is invoked at app boot — checked 2026-09, it moves as main.js grows) diff --git a/main.js b/main.js index 714b1d00..d327f909 100644 --- a/main.js +++ b/main.js @@ -73,6 +73,7 @@ function spawnPty(file, args, opts) { const { discoverShellProfiles, getShellProfiles, resolveShell, isWindows, isWslShell, windowsToWslPath, shellArgs, quoteArgvForShell } = require('./shell-profiles'); const { startScheduler } = require('./schedule-runner'); const { encodeProjectPath } = require('./encode-project-path'); +const { scanMdFiles } = require('./scan-md-files'); const { isSensitivePath, isAllowedMemoryPath: _isAllowedMemoryPath, resolveAllowedMemoryPath: _resolveAllowedMemoryPath, isKnownProjectRoot: _isKnownProjectRoot } = require('./ipc-path-validator'); const { validatePreLaunchCmd } = require('./pre-launch-cmd-guard'); const { normalizePtySize } = require('./pty-size'); @@ -1225,32 +1226,12 @@ function folderToShortPath(folder) { return meaningful.slice(-2).join('/'); } -/** Scan a directory for .md files (non-recursive). Returns array of { filename, filePath, modified }. */ -function scanMdFiles(dir) { - const results = []; - try { - if (!fs.existsSync(dir)) return results; - const entries = fs.readdirSync(dir, { withFileTypes: true }); - for (const e of entries) { - if (e.isFile() && e.name.endsWith('.md')) { - const fp = path.join(dir, e.name); - const content = fs.readFileSync(fp, 'utf8').trim(); - if (content) { - const stat = fs.statSync(fp); - results.push({ filename: e.name, filePath: fp, modified: stat.mtime.toISOString() }); - } - } - } - } catch {} - return results; -} - ipcMain.handle('get-memories', () => { const global = getSetting('global') || {}; const hiddenProjects = new Set(global.hiddenProjects || []); // --- Global files --- - const globalFiles = scanMdFiles(CLAUDE_DIR).map(f => ({ ...f, displayPath: '~/.claude' })); + const globalFiles = scanMdFiles(CLAUDE_DIR, isAllowedMemoryPath).map(f => ({ ...f, displayPath: '~/.claude' })); // --- Per-project files --- const projects = []; @@ -1275,14 +1256,14 @@ ipcMain.handle('get-memories', () => { const seenPaths = new Set(); // 1. ~/.claude/projects/{folder}/ — claude-home .md files - const claudeHomeFiles = scanMdFiles(folderPath); + const claudeHomeFiles = scanMdFiles(folderPath, isAllowedMemoryPath); for (const f of claudeHomeFiles) { files.push({ ...f, displayPath: '~/.claude', source: 'claude-home' }); seenPaths.add(f.filePath); } // memory/MEMORY.md const memoryDir = path.join(folderPath, 'memory'); - const memoryFiles = scanMdFiles(memoryDir); + const memoryFiles = scanMdFiles(memoryDir, isAllowedMemoryPath); for (const f of memoryFiles) { files.push({ ...f, displayPath: '~/.claude', source: 'claude-home' }); seenPaths.add(f.filePath); @@ -1306,7 +1287,7 @@ ipcMain.handle('get-memories', () => { // 3. {projectPath}/.claude/ — commands/*.md and other .md files const dotClaudeDir = path.join(projectPath, '.claude'); - const dotClaudeFiles = scanMdFiles(dotClaudeDir); + const dotClaudeFiles = scanMdFiles(dotClaudeDir, isAllowedMemoryPath); for (const f of dotClaudeFiles) { if (!seenPaths.has(f.filePath)) { files.push({ ...f, displayPath: shortName + '/.claude/', source: 'project' }); @@ -1315,7 +1296,7 @@ ipcMain.handle('get-memories', () => { } // commands/*.md const commandsDir = path.join(dotClaudeDir, 'commands'); - const commandFiles = scanMdFiles(commandsDir); + const commandFiles = scanMdFiles(commandsDir, isAllowedMemoryPath); for (const f of commandFiles) { if (!seenPaths.has(f.filePath)) { files.push({ ...f, displayPath: shortName + '/.claude/commands/', source: 'project' }); diff --git a/run-schedule-now-target.js b/run-schedule-now-target.js index 93c2c779..07c371d8 100644 --- a/run-schedule-now-target.js +++ b/run-schedule-now-target.js @@ -5,6 +5,7 @@ const path = require('path'); const { resolveOnDisk } = require('./resolve-path-on-disk'); +const { isSensitivePath } = require('./ipc-path-validator'); const SCHEDULE_FILENAME_RE = /^schedule-.*\.md$/i; @@ -20,7 +21,20 @@ function resolveRunNowTarget(filePath, isPathAllowed) { return { ok: false, error: 'invalid path' }; } - const real = resolveOnDisk(filePath); + const requested = path.resolve(filePath); + + if (!SCHEDULE_FILENAME_RE.test(path.basename(requested))) { + return { ok: false, error: 'not a schedule file' }; + } + + const commandsDir = path.dirname(requested); + const dotClaudeDir = path.dirname(commandsDir); + + if (path.basename(commandsDir) !== 'commands' || path.basename(dotClaudeDir) !== '.claude') { + return { ok: false, error: 'not inside a project .claude/commands directory' }; + } + + const real = resolveOnDisk(requested); if (!real) { return { ok: false, error: 'file not found' }; } @@ -29,15 +43,16 @@ function resolveRunNowTarget(filePath, isPathAllowed) { return { ok: false, error: 'not a schedule file' }; } - const commandsDir = path.dirname(real); - const dotClaudeDir = path.dirname(commandsDir); - const projectPath = path.dirname(dotClaudeDir); + if (isSensitivePath(real)) { + return { ok: false, error: 'path not allowed' }; + } - if (path.basename(commandsDir) !== 'commands' || path.basename(dotClaudeDir) !== '.claude') { - return { ok: false, error: 'not inside a project .claude/commands directory' }; + const projectPath = resolveOnDisk(path.dirname(dotClaudeDir)); + if (!projectPath) { + return { ok: false, error: 'project root not found' }; } - if (typeof isPathAllowed !== 'function' || !isPathAllowed(real)) { + if (typeof isPathAllowed !== 'function' || !isPathAllowed(real) || !isPathAllowed(projectPath)) { return { ok: false, error: 'path not allowed' }; } diff --git a/scan-md-files.js b/scan-md-files.js new file mode 100644 index 00000000..7f78c524 --- /dev/null +++ b/scan-md-files.js @@ -0,0 +1,41 @@ +// scan-md-files.js — non-recursive scan of a directory for the Markdown files +// the Memory/brain tab lists. Electron-free so it can be tested directly. +// See .ai/contexts/ipc-bridge.md, "IPC path-guard inventory". +'use strict'; + +const fs = require('fs'); +const path = require('path'); +const { isSensitivePath } = require('./ipc-path-validator'); + +/** + * Scan `dir` for non-empty `.md` files (non-recursive). + * + * @param {string} dir + * @param {(filePath: string) => boolean} [isAllowed] - caller's allowlist, given the literal path + * @returns {Array<{filename: string, filePath: string, modified: string}>} + */ +function scanMdFiles(dir, isAllowed) { + const results = []; + try { + if (!fs.existsSync(dir)) return results; + const entries = fs.readdirSync(dir, { withFileTypes: true }); + for (const e of entries) { + if (!e.name.endsWith('.md')) continue; + if (!e.isFile() && !e.isSymbolicLink()) continue; + const fp = path.join(dir, e.name); + try { + const stat = fs.statSync(fp); + if (!stat.isFile()) continue; + if (isSensitivePath(fp)) continue; + if (isAllowed && !isAllowed(fp)) continue; + const content = fs.readFileSync(fp, 'utf8').trim(); + if (content) { + results.push({ filename: e.name, filePath: fp, modified: stat.mtime.toISOString() }); + } + } catch { /* unreadable or dangling — skip this entry, keep the rest */ } + } + } catch { /* unreadable directory */ } + return results; +} + +module.exports = { scanMdFiles }; diff --git a/test/run-schedule-now-target.test.js b/test/run-schedule-now-target.test.js index df026624..952cace1 100644 --- a/test/run-schedule-now-target.test.js +++ b/test/run-schedule-now-target.test.js @@ -31,19 +31,181 @@ function rig() { const allowAll = () => true; +// The predicate stands in for isAllowedMemoryPath: containment in an allowed +// root, including the root itself. The guard asks it about the project root as +// well as the file, and a stub without the equality branch answers false for +// the root — which refuses on the project check and leaves the check on the +// file untested. +const allowUnder = (root) => (p) => { + const real = fs.realpathSync(root); + return p === real || p.startsWith(real + path.sep); +}; + test('resolveRunNowTarget: accepts a schedule-*.md file inside a project .claude/commands dir that is allowed', () => { const r = rig(); try { const filePath = path.join(r.commandsDir, 'schedule-nightly.md'); fs.writeFileSync(filePath, '---\nname: nightly\n---\ndo the thing'); - const isPathAllowed = (p) => p === fs.realpathSync(filePath); - const out = resolveRunNowTarget(filePath, isPathAllowed); + const out = resolveRunNowTarget(filePath, allowUnder(r.projectPath)); assert.equal(out.ok, true); assert.equal(out.realPath, fs.realpathSync(filePath)); assert.equal(out.projectPath, fs.realpathSync(r.projectPath)); } finally { r.cleanup(); } }); +test('resolveRunNowTarget: accepts a schedule linked in from a versioned repo elsewhere under the allowed root, and roots the run at the listing project', (t) => { + const r = rig(); + try { + // The shape a versioned dotfiles setup produces: the schedule file lives in + // a repo of its own, outside .claude/commands, and the project's + // .claude/commands holds a symlink to it. The repo is under the allowed + // root here — that is the boundary, and the two tests below pin both sides + // of it. + const repoDir = path.join(r.projectPath, 'dotfiles', 'switchboard'); + fs.mkdirSync(repoDir, { recursive: true }); + const realFile = path.join(repoDir, 'schedule-audit.md'); + fs.writeFileSync(realFile, '---\nname: audit\ncron: 17 12 * * 1\n---\naudit the memory'); + + const linkPath = path.join(r.commandsDir, 'schedule-audit.md'); + try { fs.symlinkSync(realFile, linkPath, 'file'); } + catch { return t.skip('cannot create a symlink on this machine'); } + + const out = resolveRunNowTarget(linkPath, allowUnder(r.projectPath)); + assert.equal(out.ok, true, out.error); + assert.equal(out.realPath, fs.realpathSync(realFile), 'the file read is the link target'); + assert.equal(out.projectPath, fs.realpathSync(r.projectPath), + 'the run is rooted at the project whose .claude/commands lists it, not at the repo the file happens to live in'); + } finally { r.cleanup(); } +}); + +test('resolveRunNowTarget: a project reached through a symlinked ancestor is rooted at its canonical path', (t) => { + const r = rig(); + try { + // The project root goes through disk resolution in its own right. Handing + // back the spelling that was asked for would give the spawn a cwd the + // allowlist never saw, and compare unequal against the known project list. + const alias = path.join(r.root, 'alias'); + try { fs.symlinkSync(r.projectPath, alias, 'dir'); } + catch { return t.skip('cannot create a symlink on this machine'); } + + const filePath = path.join(r.commandsDir, 'schedule-nightly.md'); + fs.writeFileSync(filePath, '---\nname: nightly\n---\ndo the thing'); + + const viaAlias = path.join(alias, '.claude', 'commands', 'schedule-nightly.md'); + const out = resolveRunNowTarget(viaAlias, allowUnder(r.projectPath)); + assert.equal(out.ok, true, out.error); + assert.equal(out.projectPath, fs.realpathSync(r.projectPath)); + assert.equal(out.realPath, fs.realpathSync(filePath)); + } finally { r.cleanup(); } +}); + +test('resolveRunNowTarget: a schedule linked between two known projects reads one and roots the run in the other', (t) => { + const r = rig(); + try { + // The file and the cwd are resolved and allowlisted independently, so when + // both land in allowed roots they may legitimately be different roots: the + // bytes come from projB, the run belongs to projA because projA's + // .claude/commands is what lists it. Pinned so it is not "fixed" either way + // by accident — see .ai/contexts/schedule-runner.md. + const projB = path.join(r.root, 'project-b'); + const bCommands = path.join(projB, '.claude', 'commands'); + fs.mkdirSync(bCommands, { recursive: true }); + const realFile = path.join(bCommands, 'schedule-shared.md'); + fs.writeFileSync(realFile, '---\nname: shared\n---\nrun the shared task'); + + const linkPath = path.join(r.commandsDir, 'schedule-shared.md'); + try { fs.symlinkSync(realFile, linkPath, 'file'); } + catch { return t.skip('cannot create a symlink on this machine'); } + + const bothKnown = (p) => [r.projectPath, projB].some((root) => { + const real = fs.realpathSync(root); + return p === real || p.startsWith(real + path.sep); + }); + + const out = resolveRunNowTarget(linkPath, bothKnown); + assert.equal(out.ok, true, out.error); + assert.equal(out.realPath, fs.realpathSync(realFile), 'content comes from project B'); + assert.equal(out.projectPath, fs.realpathSync(r.projectPath), 'cwd is project A, which lists it'); + } finally { r.cleanup(); } +}); + +test('resolveRunNowTarget: refuses a link whose target is not itself named schedule-*.md, however the link is named', (t) => { + const r = rig(); + try { + // The listing directory decides where a schedule may be *linked from*; it + // does not decide what the link may point at. Without this, any file the + // allowlist reaches — a project file, anything under ~/.claude — becomes + // the prompt of a spawned `claude -p` by being linked under a + // schedule-shaped name. + const secret = path.join(r.projectPath, 'credentials.json'); + fs.writeFileSync(secret, '{"token":"sk-not-a-real-token"}'); + + const linkPath = path.join(r.commandsDir, 'schedule-weekly.md'); + try { fs.symlinkSync(secret, linkPath, 'file'); } + catch { return t.skip('cannot create a symlink on this machine'); } + + const out = resolveRunNowTarget(linkPath, allowUnder(r.projectPath)); + assert.equal(out.ok, false); + assert.equal(out.error, 'not a schedule file'); + } finally { r.cleanup(); } +}); + +test('resolveRunNowTarget: refuses a link to a sensitive location even under a schedule-shaped name at both ends', (t) => { + const r = rig(); + try { + // The denylist answers for the resolved target as well: a name that passes + // both filename checks still must not open a credential store. + const sshDir = path.join(r.projectPath, '.ssh'); + fs.mkdirSync(sshDir, { recursive: true }); + const target = path.join(sshDir, 'schedule-keys.md'); + fs.writeFileSync(target, 'id_rsa contents'); + + const linkPath = path.join(r.commandsDir, 'schedule-keys.md'); + try { fs.symlinkSync(target, linkPath, 'file'); } + catch { return t.skip('cannot create a symlink on this machine'); } + + const out = resolveRunNowTarget(linkPath, allowUnder(r.projectPath)); + assert.equal(out.ok, false); + assert.equal(out.error, 'path not allowed'); + } finally { r.cleanup(); } +}); + +test('resolveRunNowTarget: refuses a schedule linked in from a repo outside every allowed root', (t) => { + const r = rig(); + try { + // The counterpart of the accepting case above: a dotfiles repo the user has + // never opened as a project is not a place this handler reads and spawns + // from, however correctly the link is named and placed. + const repoDir = path.join(r.outsideDir, 'dotfiles'); + fs.mkdirSync(repoDir, { recursive: true }); + const realFile = path.join(repoDir, 'schedule-audit.md'); + fs.writeFileSync(realFile, '---\nname: audit\ncron: 17 12 * * 1\n---\naudit'); + + const linkPath = path.join(r.commandsDir, 'schedule-audit.md'); + try { fs.symlinkSync(realFile, linkPath, 'file'); } + catch { return t.skip('cannot create a symlink on this machine'); } + + const out = resolveRunNowTarget(linkPath, allowUnder(r.projectPath)); + assert.equal(out.ok, false); + assert.equal(out.error, 'path not allowed'); + } finally { r.cleanup(); } +}); + +test('resolveRunNowTarget: refuses a schedule whose project root is not allowed, even when the file itself is', () => { + const r = rig(); + try { + const filePath = path.join(r.commandsDir, 'schedule-nightly.md'); + fs.writeFileSync(filePath, '---\nname: nightly\n---\ndo the thing'); + // The file passes, the project the run would be spawned in does not: the + // cwd of the spawn is its own thing to allowlist, not something to infer + // from the file once the two can live apart. + const isPathAllowed = (p) => p === fs.realpathSync(filePath); + const out = resolveRunNowTarget(filePath, isPathAllowed); + assert.equal(out.ok, false); + assert.equal(out.error, 'path not allowed'); + } finally { r.cleanup(); } +}); + test('resolveRunNowTarget: refuses a path outside every known project — before any read or spawn', () => { const r = rig(); try { @@ -132,10 +294,12 @@ test('resolveRunNowTarget: a symlinked commands directory escaping the project i catch { linked = false; } if (!linked) return t.skip('cannot create a symlink on this machine'); - // isPathAllowed only allows the known project root, not the outside one. - const isPathAllowed = (p) => p.startsWith(fs.realpathSync(r.projectPath) + path.sep); - const out = resolveRunNowTarget(linkPath, isPathAllowed); + // allowUnder allows the known project root and everything under it, so the + // project check passes and the refusal can only come from the check on the + // resolved file — the one this test is about. + const out = resolveRunNowTarget(linkPath, allowUnder(r.projectPath)); assert.equal(out.ok, false, 'the resolved real target lives outside the known project'); + assert.equal(out.error, 'path not allowed'); } finally { r.cleanup(); } }); diff --git a/test/scan-md-files.test.js b/test/scan-md-files.test.js new file mode 100644 index 00000000..75f939b3 --- /dev/null +++ b/test/scan-md-files.test.js @@ -0,0 +1,176 @@ +// test/scan-md-files.test.js — unit tests for the Memory/brain tab's directory +// scan. +// +// The regression this pins: a `schedule-*.md` (or `CLAUDE.md`, or a memory +// note) symlinked in from a git-versioned dotfiles repo disappeared from the +// brain tab, because the scan accepted an entry on `dirent.isFile()` — false +// for a symlink — instead of on what the entry resolves to. Nothing failed +// loudly: the cron loop kept firing the same schedule every week while the UI +// showed no schedule at all. +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawnSync } = require('child_process'); + +const { scanMdFiles } = require('../scan-md-files'); + +function rig() { + const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'sb-scan-md-'))); + const dir = path.join(root, 'commands'); + const elsewhere = path.join(root, 'dotfiles'); + fs.mkdirSync(dir, { recursive: true }); + fs.mkdirSync(elsewhere, { recursive: true }); + return { root, dir, elsewhere, cleanup: () => fs.rmSync(root, { recursive: true, force: true }) }; +} + +function symlink(target, linkPath, t) { + try { fs.symlinkSync(target, linkPath, 'file'); return true; } + catch { t.skip('cannot create a symlink on this machine'); return false; } +} + +const names = (files) => files.map(f => f.filename).sort(); + +test('scanMdFiles: lists plain .md files and ignores every other extension, including ones containing .md', () => { + const r = rig(); + try { + fs.writeFileSync(path.join(r.dir, 'CLAUDE.md'), 'hello'); + fs.writeFileSync(path.join(r.dir, 'notes.txt'), 'hello'); + // '.md' as a substring rather than a suffix: an editor backup and an MDX + // file are not Markdown notes this list should carry. + fs.writeFileSync(path.join(r.dir, 'notes.md.bak'), 'hello'); + fs.writeFileSync(path.join(r.dir, 'README.mdx'), 'hello'); + assert.deepEqual(names(scanMdFiles(r.dir)), ['CLAUDE.md']); + } finally { r.cleanup(); } +}); + +test('scanMdFiles: lists a .md file that is a symlink to a file kept outside the directory', (t) => { + const r = rig(); + try { + // Deliberately different names at the two ends: the renderer decides + // whether to draw the "run now" play button from the *listed* name + // (memory-workfiles-view.js keys on filename.startsWith('schedule-')), so a + // scan that reported the target's name would move that button. + const real = path.join(r.elsewhere, 'audit.md'); + fs.writeFileSync(real, '---\ncron: 0 9 * * 1\n---\naudit'); + const link = path.join(r.dir, 'schedule-audit.md'); + if (!symlink(real, link, t)) return; + + const files = scanMdFiles(r.dir); + assert.deepEqual(names(files), ['schedule-audit.md']); + // The link's own path is what the renderer shows and hands back to + // read-memory / run-schedule-now — not the target it resolves to. + assert.equal(files[0].filePath, link); + // computeIndexSignature and the project sort both parse this back. + assert.match(files[0].modified, /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/); + assert.equal(files[0].modified, fs.statSync(real).mtime.toISOString()); + } finally { r.cleanup(); } +}); + +test('scanMdFiles: skips a symlink to a directory, and a dangling one', (t) => { + const r = rig(); + try { + const targetDir = path.join(r.elsewhere, 'a-directory.md'); + fs.mkdirSync(targetDir); + if (!symlink(targetDir, path.join(r.dir, 'a-directory.md'), t)) return; + if (!symlink(path.join(r.elsewhere, 'gone.md'), path.join(r.dir, 'dangling.md'), t)) return; + + assert.deepEqual(names(scanMdFiles(r.dir)), []); + } finally { r.cleanup(); } +}); + +test('scanMdFiles: one dangling entry does not hide the files listed after it', (t) => { + const r = rig(); + try { + // Accepting an entry now means stat-ing it, which throws on a dangling + // link. 'a-dangling.md' sorts before 'z-real.md', so a scan that let that + // throw escape the loop would return nothing at all. + if (!symlink(path.join(r.elsewhere, 'gone.md'), path.join(r.dir, 'a-dangling.md'), t)) return; + fs.writeFileSync(path.join(r.dir, 'z-real.md'), 'still here'); + + assert.deepEqual(names(scanMdFiles(r.dir)), ['z-real.md']); + } finally { r.cleanup(); } +}); + +test('scanMdFiles: skips empty and whitespace-only files, including through a symlink', (t) => { + const r = rig(); + try { + fs.writeFileSync(path.join(r.dir, 'empty.md'), ''); + fs.writeFileSync(path.join(r.dir, 'blank.md'), ' \n\t\n'); + const real = path.join(r.elsewhere, 'also-empty.md'); + fs.writeFileSync(real, '\n'); + if (!symlink(real, path.join(r.dir, 'also-empty.md'), t)) return; + + assert.deepEqual(names(scanMdFiles(r.dir)), []); + } finally { r.cleanup(); } +}); + +test('scanMdFiles: skips a symlink to a FIFO instead of blocking forever on it', (t) => { + const r = rig(); + try { + // readFileSync on a FIFO with no writer never returns, and this scan runs + // synchronously inside an ipcMain.handle — a hang here is the whole + // Electron main process, with no way back. stat() on a FIFO does not + // block, so resolving the entry first is what keeps the read unreachable. + // A regression here shows up as a suite that hangs rather than one that + // goes red: the block is synchronous, so no test timeout can interrupt it. + const fifo = path.join(r.elsewhere, 'pipe.md'); + const mk = spawnSync('mkfifo', [fifo]); + if (mk.error || mk.status !== 0) return t.skip('mkfifo unavailable on this machine'); + if (!symlink(fifo, path.join(r.dir, 'linked.md'), t)) return; + fs.writeFileSync(path.join(r.dir, 'real.md'), 'still here'); + + assert.deepEqual(names(scanMdFiles(r.dir)), ['real.md']); + } finally { r.cleanup(); } +}); + +test('scanMdFiles: refuses a link to a credential location even when the allowlist would accept it', (t) => { + const r = rig(); + try { + // The denylist is not the caller's to choose: a cloned repository added as + // a project carries its own .ssh/.env, which containment in an allowed root + // says nothing about. Reading one here would put its content in the FTS + // index, which is searchable by substring. + const sshDir = path.join(r.elsewhere, '.ssh'); + fs.mkdirSync(sshDir, { recursive: true }); + const key = path.join(sshDir, 'id_rsa.md'); + fs.writeFileSync(key, '-----BEGIN OPENSSH PRIVATE KEY-----'); + if (!symlink(key, path.join(r.dir, 'notes.md'), t)) return; + fs.writeFileSync(path.join(r.dir, 'ordinary.md'), 'an ordinary note'); + + // allowAll: only the denylist can refuse it here. + assert.deepEqual(names(scanMdFiles(r.dir, () => true)), ['ordinary.md']); + } finally { r.cleanup(); } +}); + +test('scanMdFiles: a file the caller\'s allowlist refuses is not listed', (t) => { + const r = rig(); + try { + // The list feeds readers that apply this allowlist before opening a file + // (read-memory, save-memory) and an FTS indexer that reads the body with no + // guard at all. Listing a file the allowlist refuses puts its content in + // the search index while the panel that displays it stays empty. + const outside = path.join(r.elsewhere, 'creds.md'); + fs.writeFileSync(outside, 'SECRET=value'); + if (!symlink(outside, path.join(r.dir, 'notes.md'), t)) return; + fs.writeFileSync(path.join(r.dir, 'allowed.md'), 'ordinary note'); + + const allowed = (fp) => { + const real = fs.realpathSync(fp); + return real === r.dir || real.startsWith(r.dir + path.sep); + }; + assert.deepEqual(names(scanMdFiles(r.dir, allowed)), ['allowed.md']); + // Without a predicate the scan lists both — the allowlist is the caller's. + assert.deepEqual(names(scanMdFiles(r.dir)), ['allowed.md', 'notes.md']); + } finally { r.cleanup(); } +}); + +test('scanMdFiles: a directory that does not exist scans to an empty list, not a throw', () => { + const r = rig(); + try { + assert.deepEqual(scanMdFiles(path.join(r.root, 'nope')), []); + } finally { r.cleanup(); } +});