From 5a4f1c2807cb31fcf4da1877d9ca939a5e0cb5b8 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Thu, 17 Sep 2026 11:36:38 +0200 Subject: [PATCH 1/3] fix(schedule): a schedule file reached through a symlink is listed and runnable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `schedule-*.md` symlinked into a project's `.claude/commands` from a git-versioned dotfiles repo vanished from the brain tab, and the cron loop went on firing it every week. Nothing failed anywhere: the scheduler reads the file through the link, so the task ran unattended while the UI showed the project had no schedules at all — the one place a user goes to check. Two readers disagreed with the scheduler about what a schedule file is. `scanMdFiles` (the brain tab's list, `get-memories`) accepted a directory entry on `dirent.isFile()`, which a symlink reports as false however ordinary the file at the other end is. It now accepts on what the entry resolves to, so a link to a file is listed and a link to a directory or a dangling one still is not. Moved to its own module — main.js needs Electron to load, and this is the part worth testing. Per-entry failures are now contained too: one unreadable file used to abort the scan of every file after it in the same directory. `resolveRunNowTarget` (the play button's path guard) checked the `.claude/commands` shape against the symlink-free path, so the same file was refused with "not inside a project .claude/commands directory" — the directory that *lists* a schedule is what makes it one, and that is the requested path, not wherever the file is kept. The shape checks move to the requested path; the allowlist stays on disk-resolved paths and now covers the project root the run would be spawned in as well as the file, since with a linked-in schedule the two are no longer the same branch of the filesystem. A link pointing out of every allowed root is still refused. Test coverage for both, including the linked-in case end to end. --- .ai/contexts/ipc-bridge.md | 4 +- .ai/contexts/schedule-runner.md | 2 + main.js | 21 +----- run-schedule-now-target.js | 41 +++++++++-- scan-md-files.js | 51 +++++++++++++ test/run-schedule-now-target.test.js | 52 ++++++++++++- test/scan-md-files.test.js | 105 +++++++++++++++++++++++++++ 7 files changed, 244 insertions(+), 32 deletions(-) create mode 100644 scan-md-files.js create mode 100644 test/scan-md-files.test.js diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index 99175d97..c722a83f 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. Note the division of labour inside it: the *shape* checks (filename, `.claude/commands` parents) run on the requested path, because a schedule is defined by the directory that lists it and may be a symlink to a file kept in a versioned dotfiles repo; the *allowlist* runs on the disk-resolved file **and** on the disk-resolved project root the run would be spawned in, which with a linked-in schedule is no longer derivable from the file. 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. @@ -150,7 +150,7 @@ Every handler that takes a renderer-supplied path or derives a spawn location fr | `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 filename shape (on the requested path) + `isAllowedMemoryPath` on both the resolved file and the resolved project root | 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** | | `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..c2c2d833 100644 --- a/.ai/contexts/schedule-runner.md +++ b/.ai/contexts/schedule-runner.md @@ -53,6 +53,7 @@ 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. - **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 +63,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 - `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..976f9f2e 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,26 +1226,6 @@ 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 || []); diff --git a/run-schedule-now-target.js b/run-schedule-now-target.js index 93c2c779..b9b37af9 100644 --- a/run-schedule-now-target.js +++ b/run-schedule-now-target.js @@ -20,24 +20,49 @@ function resolveRunNowTarget(filePath, isPathAllowed) { return { ok: false, error: 'invalid path' }; } - const real = resolveOnDisk(filePath); - if (!real) { - return { ok: false, error: 'file not found' }; - } + // The shape checks run on the requested location, not on the symlink-free + // one. A schedule file is defined by the directory that *lists* it — the + // project's .claude/commands — and a user who keeps their configuration in a + // versioned dotfiles repo links it in from elsewhere on disk. Testing the + // real path instead refused exactly those, with "not inside a project + // .claude/commands directory", while the cron loop went on firing the same + // file every week: the scheduler reads through the link, this guard did not. + // + // Nothing is relaxed by the move. The two path-guard duties stay where they + // were: the file is still resolved on disk, and both the resolved file and + // the project the run would be rooted at still have to pass isPathAllowed + // below — a link pointing out of every allowed root is refused there, which + // is what the "symlinked commands directory escaping the project" test + // pins. What changes is only which string has to *look* like a schedule: + // the one the renderer listed. + const requested = path.resolve(filePath); - if (!SCHEDULE_FILENAME_RE.test(path.basename(real))) { + if (!SCHEDULE_FILENAME_RE.test(path.basename(requested))) { return { ok: false, error: 'not a schedule file' }; } - const commandsDir = path.dirname(real); + const commandsDir = path.dirname(requested); const dotClaudeDir = path.dirname(commandsDir); - const projectPath = path.dirname(dotClaudeDir); if (path.basename(commandsDir) !== 'commands' || path.basename(dotClaudeDir) !== '.claude') { return { ok: false, error: 'not inside a project .claude/commands directory' }; } - if (typeof isPathAllowed !== 'function' || !isPathAllowed(real)) { + const real = resolveOnDisk(requested); + if (!real) { + return { ok: false, error: 'file not found' }; + } + + // The spawn is rooted here, so it is resolved and allowlisted in its own + // right rather than inferred from the file: with a linked-in schedule the + // two live on different branches of the filesystem, and the project the run + // belongs to is the one whose .claude/commands lists it. + const projectPath = resolveOnDisk(path.dirname(dotClaudeDir)); + if (!projectPath) { + return { ok: false, error: 'file not found' }; + } + + 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..c46bb0fe --- /dev/null +++ b/scan-md-files.js @@ -0,0 +1,51 @@ +// scan-md-files.js — non-recursive scan of a directory for the Markdown files +// the Memory/brain tab lists (CLAUDE.md, memory notes, schedule-*.md commands). +// Electron-free so it can be tested directly. +'use strict'; + +const fs = require('fs'); +const path = require('path'); + +/** + * Scan `dir` for non-empty `.md` files (non-recursive). + * + * Entries are accepted through `fs.statSync`, which follows symlinks, rather + * than through the directory entry's own type: a `schedule-*.md` or `CLAUDE.md` + * that is a symlink to a file kept in a git-versioned dotfiles repo is a real + * Markdown file to every reader of this list, but its dirent reports + * `isSymbolicLink()`, not `isFile()`. Checking the dirent alone dropped those + * files from the brain tab (and from the memory FTS index) with no error + * anywhere — the schedule still fired on cron, it just could not be seen or + * run from the UI. + * + * A symlink pointing at a directory, or a dangling one, still fails the + * `isFile()` check on the resolved target and is skipped. Per-entry failures + * are contained: one unreadable file no longer aborts the scan of everything + * after it in the same directory. + * + * @param {string} dir + * @returns {Array<{filename: string, filePath: string, modified: string}>} + */ +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.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; + 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..4d10400e 100644 --- a/test/run-schedule-now-target.test.js +++ b/test/run-schedule-now-target.test.js @@ -31,19 +31,67 @@ function rig() { const allowAll = () => true; +// The predicate stands in for isAllowedMemoryPath: containment in an allowed +// root, not an exact match on one file. The guard asks it about the project +// root as well as the file, since the two are no longer the same branch of +// the filesystem once a schedule is symlinked in. +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 outside the project, 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 somewhere else on disk, and the project's .claude/commands + // holds a symlink to it. Both the link's target and the project root are + // inside the allowed root here (a repo checked out under the workspace), + // which is what makes it different from the escaping case below. + 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: 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 { diff --git a/test/scan-md-files.test.js b/test/scan-md-files.test.js new file mode 100644 index 00000000..08be4673 --- /dev/null +++ b/test/scan-md-files.test.js @@ -0,0 +1,105 @@ +// 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 { 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 other extensions', () => { + const r = rig(); + try { + fs.writeFileSync(path.join(r.dir, 'CLAUDE.md'), 'hello'); + fs.writeFileSync(path.join(r.dir, 'notes.txt'), '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 { + const real = path.join(r.elsewhere, 'schedule-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); + assert.ok(!Number.isNaN(Date.parse(files[0].modified))); + } 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: 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(); } +}); From 39f5c35c547ea24320ceb43171f8a17615f41357 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Thu, 17 Sep 2026 12:31:26 +0200 Subject: [PATCH 2/3] fix(schedule): constrain what a linked-in schedule may point at, not just where it is linked from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checking the `schedule-*.md` filename only on the requested path let the listing directory decide what the target is. Any file the allowlist reaches — anything under `~/.claude`, anything in an indexed project — became the prompt of a spawned `claude -p` by being linked under a schedule-shaped name: /.claude/commands/schedule-weekly.md -> ../../../../.claude/.credentials.json git stores symlinks natively and a relative link survives cloning, so a cloned repository carries this without any local access. `run-schedule-now` reads that target, `createScheduleSession` writes it verbatim into a session JSONL as a user message, and the spawned CLI sends it as conversation context. The filename is now checked on the resolved target as well. The directory shape stays on the requested path, which is the whole point of the change: where a schedule may be linked *from* is the listing directory's business, what it may point *at* is not. `isSensitivePath` refuses a resolved target in a credential location — this handler turns file content into a prompt, so it earns the denylist the file panel already has. Also: `resolveOnDisk` failing on the project root reported `file not found`, which names the wrong path. Tests: the allowlist on the resolved file was provable-dead — deleting `isPathAllowed(real)` left the suite green, because the one test that claimed to pin it used a containment stub with no equality branch, refused on the project root instead, and asserted only `ok === false`. Five mutants (drop either allowlist call, drop either disk resolution, drop the target filename check) are now each caught by a distinct test. --- .ai/contexts/ipc-bridge.md | 4 +- run-schedule-now-target.js | 30 +++----- test/run-schedule-now-target.test.js | 110 ++++++++++++++++++++++++--- 3 files changed, 110 insertions(+), 34 deletions(-) diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index c722a83f..ca38631a 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. Note the division of labour inside it: the *shape* checks (filename, `.claude/commands` parents) run on the requested path, because a schedule is defined by the directory that lists it and may be a symlink to a file kept in a versioned dotfiles repo; the *allowlist* runs on the disk-resolved file **and** on the disk-resolved project root the run would be spawned in, which with a linked-in schedule is no longer derivable from the file. + - `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. @@ -150,7 +150,7 @@ Every handler that takes a renderer-supplied path or derives a spawn location fr | `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 (on the requested path) + `isAllowedMemoryPath` on both the resolved file and the resolved project root | 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/run-schedule-now-target.js b/run-schedule-now-target.js index b9b37af9..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,21 +21,6 @@ function resolveRunNowTarget(filePath, isPathAllowed) { return { ok: false, error: 'invalid path' }; } - // The shape checks run on the requested location, not on the symlink-free - // one. A schedule file is defined by the directory that *lists* it — the - // project's .claude/commands — and a user who keeps their configuration in a - // versioned dotfiles repo links it in from elsewhere on disk. Testing the - // real path instead refused exactly those, with "not inside a project - // .claude/commands directory", while the cron loop went on firing the same - // file every week: the scheduler reads through the link, this guard did not. - // - // Nothing is relaxed by the move. The two path-guard duties stay where they - // were: the file is still resolved on disk, and both the resolved file and - // the project the run would be rooted at still have to pass isPathAllowed - // below — a link pointing out of every allowed root is refused there, which - // is what the "symlinked commands directory escaping the project" test - // pins. What changes is only which string has to *look* like a schedule: - // the one the renderer listed. const requested = path.resolve(filePath); if (!SCHEDULE_FILENAME_RE.test(path.basename(requested))) { @@ -53,13 +39,17 @@ function resolveRunNowTarget(filePath, isPathAllowed) { return { ok: false, error: 'file not found' }; } - // The spawn is rooted here, so it is resolved and allowlisted in its own - // right rather than inferred from the file: with a linked-in schedule the - // two live on different branches of the filesystem, and the project the run - // belongs to is the one whose .claude/commands lists it. + if (!SCHEDULE_FILENAME_RE.test(path.basename(real))) { + return { ok: false, error: 'not a schedule file' }; + } + + if (isSensitivePath(real)) { + return { ok: false, error: 'path not allowed' }; + } + const projectPath = resolveOnDisk(path.dirname(dotClaudeDir)); if (!projectPath) { - return { ok: false, error: 'file not found' }; + return { ok: false, error: 'project root not found' }; } if (typeof isPathAllowed !== 'function' || !isPathAllowed(real) || !isPathAllowed(projectPath)) { diff --git a/test/run-schedule-now-target.test.js b/test/run-schedule-now-target.test.js index 4d10400e..bcf61d90 100644 --- a/test/run-schedule-now-target.test.js +++ b/test/run-schedule-now-target.test.js @@ -32,9 +32,10 @@ function rig() { const allowAll = () => true; // The predicate stands in for isAllowedMemoryPath: containment in an allowed -// root, not an exact match on one file. The guard asks it about the project -// root as well as the file, since the two are no longer the same branch of -// the filesystem once a schedule is symlinked in. +// 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); @@ -52,14 +53,14 @@ test('resolveRunNowTarget: accepts a schedule-*.md file inside a project .claude } finally { r.cleanup(); } }); -test('resolveRunNowTarget: accepts a schedule linked in from outside the project, and roots the run at the listing project', (t) => { +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 somewhere else on disk, and the project's .claude/commands - // holds a symlink to it. Both the link's target and the project root are - // inside the allowed root here (a repo checked out under the workspace), - // which is what makes it different from the escaping case below. + // 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'); @@ -77,6 +78,89 @@ test('resolveRunNowTarget: accepts a schedule linked in from outside the project } 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: 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 { @@ -180,10 +264,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(); } }); From f7c0c0184ce9d9b24c9f1d3eab66c03783ed5af4 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Renard Date: Thu, 17 Sep 2026 12:39:58 +0200 Subject: [PATCH 3/3] fix(memory): guard the file list, not just the readers behind it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scanMdFiles` feeds three consumers and only two of them guard their own reads. `read-memory` and `save-memory` go through `resolveAllowedMemoryPath`; the FTS indexer in `get-memories` reads each listed file's body with a plain `readFileSync` and stores it in `search_content`, where it is searchable by substring. While the list held nothing but regular files inside the directory being scanned, that asymmetry cost nothing. Listing what a link resolves to changes that. A `notes.md -> ~/.ssh/id_rsa` under any scanned directory — `.claude`, `.claude/commands`, the project root, in any project ever indexed, including a cloned third-party repo — puts the target's content in the index, while the panel that would display it stays empty because the reader behind it refuses the same path. So the scan now answers for what it lists: `isSensitivePath` on every entry, and the caller's allowlist on top, which `get-memories` supplies as `isAllowedMemoryPath`. The denylist is not the caller's to choose — containment in an allowed project root says nothing about a cloned repo's own `.ssh` or `.env`. A file the app would refuse to open is no longer in the list at all, which also ends the rows that opened blank and saved with "path not allowed". The `stat.isFile()` line was load-bearing and untested: deleting it hangs the Electron main process forever on a link to a FIFO, since the scan is synchronous inside an ipcMain.handle. Pinned, along with three other surviving mutants — the listed name must be the link's and not its target's (the play button keys on it), the mtime must stay ISO, and `.md` must be a suffix and not a substring. `resolveRunNowTarget` resolving the file and the cwd independently means a schedule linked between two known projects runs one project's content in the other's directory. That follows from what the change is for; it is now stated in the schedule-runner context and pinned by a test. --- .ai/contexts/ipc-bridge.md | 1 + .ai/contexts/schedule-runner.md | 3 +- main.js | 10 ++-- scan-md-files.js | 24 +++------ test/run-schedule-now-target.test.js | 30 +++++++++++ test/scan-md-files.test.js | 77 ++++++++++++++++++++++++++-- 6 files changed, 119 insertions(+), 26 deletions(-) diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index ca38631a..0e0a69cf 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -147,6 +147,7 @@ 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 | diff --git a/.ai/contexts/schedule-runner.md b/.ai/contexts/schedule-runner.md index c2c2d833..d89838dd 100644 --- a/.ai/contexts/schedule-runner.md +++ b/.ai/contexts/schedule-runner.md @@ -54,6 +54,7 @@ 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. @@ -63,7 +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 +- `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 976f9f2e..d327f909 100644 --- a/main.js +++ b/main.js @@ -1231,7 +1231,7 @@ ipcMain.handle('get-memories', () => { 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 = []; @@ -1256,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); @@ -1287,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' }); @@ -1296,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/scan-md-files.js b/scan-md-files.js index c46bb0fe..7f78c524 100644 --- a/scan-md-files.js +++ b/scan-md-files.js @@ -1,32 +1,20 @@ // scan-md-files.js — non-recursive scan of a directory for the Markdown files -// the Memory/brain tab lists (CLAUDE.md, memory notes, schedule-*.md commands). -// Electron-free so it can be tested directly. +// 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). * - * Entries are accepted through `fs.statSync`, which follows symlinks, rather - * than through the directory entry's own type: a `schedule-*.md` or `CLAUDE.md` - * that is a symlink to a file kept in a git-versioned dotfiles repo is a real - * Markdown file to every reader of this list, but its dirent reports - * `isSymbolicLink()`, not `isFile()`. Checking the dirent alone dropped those - * files from the brain tab (and from the memory FTS index) with no error - * anywhere — the schedule still fired on cron, it just could not be seen or - * run from the UI. - * - * A symlink pointing at a directory, or a dangling one, still fails the - * `isFile()` check on the resolved target and is skipped. Per-entry failures - * are contained: one unreadable file no longer aborts the scan of everything - * after it in the same directory. - * * @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) { +function scanMdFiles(dir, isAllowed) { const results = []; try { if (!fs.existsSync(dir)) return results; @@ -38,6 +26,8 @@ function scanMdFiles(dir) { 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() }); diff --git a/test/run-schedule-now-target.test.js b/test/run-schedule-now-target.test.js index bcf61d90..952cace1 100644 --- a/test/run-schedule-now-target.test.js +++ b/test/run-schedule-now-target.test.js @@ -99,6 +99,36 @@ test('resolveRunNowTarget: a project reached through a symlinked ancestor is roo } 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 { diff --git a/test/scan-md-files.test.js b/test/scan-md-files.test.js index 08be4673..75f939b3 100644 --- a/test/scan-md-files.test.js +++ b/test/scan-md-files.test.js @@ -14,6 +14,7 @@ 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'); @@ -33,11 +34,15 @@ function symlink(target, linkPath, t) { const names = (files) => files.map(f => f.filename).sort(); -test('scanMdFiles: lists plain .md files and ignores other extensions', () => { +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(); } }); @@ -45,7 +50,11 @@ test('scanMdFiles: lists plain .md files and ignores other extensions', () => { test('scanMdFiles: lists a .md file that is a symlink to a file kept outside the directory', (t) => { const r = rig(); try { - const real = path.join(r.elsewhere, 'schedule-audit.md'); + // 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; @@ -55,7 +64,9 @@ test('scanMdFiles: lists a .md file that is a symlink to a file kept outside the // 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); - assert.ok(!Number.isNaN(Date.parse(files[0].modified))); + // 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(); } }); @@ -97,6 +108,66 @@ test('scanMdFiles: skips empty and whitespace-only files, including through a sy } 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 {