diff --git a/.ai/contexts/ipc-bridge.md b/.ai/contexts/ipc-bridge.md index 0e0a69cf..fc25f504 100644 --- a/.ai/contexts/ipc-bridge.md +++ b/.ai/contexts/ipc-bridge.md @@ -147,7 +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` | +| `get-memories` (the list, and the FTS bodies it indexes) | `acceptMdFile` (`scan-md-files.js`) applies `isSensitivePath` itself and the `isAllowedMemoryPath` predicate `get-memories` passes in; `scanMdFiles` is the directory-wide form of it, and the project-root `CLAUDE.md`/`GEMINI.md`/`agents.md` go through it by name | 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/main.js b/main.js index d327f909..987e361a 100644 --- a/main.js +++ b/main.js @@ -73,7 +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 { scanMdFiles, acceptMdFile } = 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'); @@ -1230,8 +1230,22 @@ ipcMain.handle('get-memories', () => { const global = getSetting('global') || {}; const hiddenProjects = new Set(global.hiddenProjects || []); + // The known-project set is rebuilt from disk on every isAllowedMemoryPath + // call, so binding it once per handler call keeps this off a per-file path. + const knownRoots = [...getKnownProjectPaths()]; + const isAllowed = (filePath) => _isAllowedMemoryPath(filePath, knownRoots); + + // Bodies of the files accepted below, kept for the FTS index at the end of + // this handler. They travel here rather than on the entries themselves: the + // entries go to the renderer, the bodies have no business going with them. + const bodies = new Map(); + const listed = ({ content, ...entry }) => { + bodies.set(entry.filePath, content); + return entry; + }; + // --- Global files --- - const globalFiles = scanMdFiles(CLAUDE_DIR, isAllowedMemoryPath).map(f => ({ ...f, displayPath: '~/.claude' })); + const globalFiles = scanMdFiles(CLAUDE_DIR, isAllowed).map(f => ({ ...listed(f), displayPath: '~/.claude' })); // --- Per-project files --- const projects = []; @@ -1256,16 +1270,16 @@ ipcMain.handle('get-memories', () => { const seenPaths = new Set(); // 1. ~/.claude/projects/{folder}/ — claude-home .md files - const claudeHomeFiles = scanMdFiles(folderPath, isAllowedMemoryPath); + const claudeHomeFiles = scanMdFiles(folderPath, isAllowed); for (const f of claudeHomeFiles) { - files.push({ ...f, displayPath: '~/.claude', source: 'claude-home' }); + files.push({ ...listed(f), displayPath: '~/.claude', source: 'claude-home' }); seenPaths.add(f.filePath); } // memory/MEMORY.md const memoryDir = path.join(folderPath, 'memory'); - const memoryFiles = scanMdFiles(memoryDir, isAllowedMemoryPath); + const memoryFiles = scanMdFiles(memoryDir, isAllowed); for (const f of memoryFiles) { - files.push({ ...f, displayPath: '~/.claude', source: 'claude-home' }); + files.push({ ...listed(f), displayPath: '~/.claude', source: 'claude-home' }); seenPaths.add(f.filePath); } @@ -1273,33 +1287,28 @@ ipcMain.handle('get-memories', () => { if (projectPath) { for (const name of ['CLAUDE.md', 'GEMINI.md', 'agents.md']) { const fp = path.join(projectPath, name); - try { - if (fs.existsSync(fp)) { - const content = fs.readFileSync(fp, 'utf8').trim(); - if (content && !seenPaths.has(fp)) { - const stat = fs.statSync(fp); - files.push({ filename: name, filePath: fp, modified: stat.mtime.toISOString(), displayPath: shortName + '/', source: 'project' }); - seenPaths.add(fp); - } - } - } catch {} + const accepted = acceptMdFile(fp, isAllowed); + if (accepted && !seenPaths.has(fp)) { + files.push({ ...listed(accepted), displayPath: shortName + '/', source: 'project' }); + seenPaths.add(fp); + } } // 3. {projectPath}/.claude/ — commands/*.md and other .md files const dotClaudeDir = path.join(projectPath, '.claude'); - const dotClaudeFiles = scanMdFiles(dotClaudeDir, isAllowedMemoryPath); + const dotClaudeFiles = scanMdFiles(dotClaudeDir, isAllowed); for (const f of dotClaudeFiles) { if (!seenPaths.has(f.filePath)) { - files.push({ ...f, displayPath: shortName + '/.claude/', source: 'project' }); + files.push({ ...listed(f), displayPath: shortName + '/.claude/', source: 'project' }); seenPaths.add(f.filePath); } } // commands/*.md const commandsDir = path.join(dotClaudeDir, 'commands'); - const commandFiles = scanMdFiles(commandsDir, isAllowedMemoryPath); + const commandFiles = scanMdFiles(commandsDir, isAllowed); for (const f of commandFiles) { if (!seenPaths.has(f.filePath)) { - files.push({ ...f, displayPath: shortName + '/.claude/commands/', source: 'project' }); + files.push({ ...listed(f), displayPath: shortName + '/.claude/commands/', source: 'project' }); seenPaths.add(f.filePath); } } @@ -1339,7 +1348,7 @@ ipcMain.handle('get-memories', () => { upsertSearchEntries(allFiles.map(f => ({ id: f.filePath, type: 'memory', folder: null, title: f.label + ' ' + f.filename, - body: fs.readFileSync(f.filePath, 'utf8'), + body: bodies.get(f.filePath) ?? '', }))); } } catch {} diff --git a/scan-md-files.js b/scan-md-files.js index 7f78c524..41d91760 100644 --- a/scan-md-files.js +++ b/scan-md-files.js @@ -1,5 +1,6 @@ -// 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. +// scan-md-files.js — the Markdown files the Memory/brain tab lists: one +// acceptance rule, and a non-recursive scan built on it. Electron-free so it +// can be tested directly. // See .ai/contexts/ipc-bridge.md, "IPC path-guard inventory". 'use strict'; @@ -8,7 +9,33 @@ const path = require('path'); const { isSensitivePath } = require('./ipc-path-validator'); /** - * Scan `dir` for non-empty `.md` files (non-recursive). + * Accept `filePath` as a listable Markdown file, or return null. + * + * The accepted content comes back with the entry. A caller that needs the body + * must use it rather than read the path again: a second read is a second + * resolution of the same string, free to land somewhere the checks above never + * saw. See resolve-path-on-disk.js. + * + * @param {string} filePath + * @param {(filePath: string) => boolean} [isAllowed] - caller's allowlist, given the literal path + * @returns {{filename: string, filePath: string, modified: string, content: string}|null} + */ +function acceptMdFile(filePath, isAllowed) { + try { + const stat = fs.statSync(filePath); + if (!stat.isFile()) return null; + if (isSensitivePath(filePath)) return null; + if (isAllowed && !isAllowed(filePath)) return null; + const content = fs.readFileSync(filePath, 'utf8'); + if (!content.trim()) return null; + return { filename: path.basename(filePath), filePath, modified: stat.mtime.toISOString(), content }; + } catch { + return null; // unreadable, dangling or absent + } +} + +/** + * Scan `dir` for listable `.md` files (non-recursive). * * @param {string} dir * @param {(filePath: string) => boolean} [isAllowed] - caller's allowlist, given the literal path @@ -18,24 +45,14 @@ 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) { + for (const e of fs.readdirSync(dir, { withFileTypes: true })) { 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 */ } + const accepted = acceptMdFile(path.join(dir, e.name), isAllowed); + if (accepted) results.push(accepted); } } catch { /* unreadable directory */ } return results; } -module.exports = { scanMdFiles }; +module.exports = { scanMdFiles, acceptMdFile }; diff --git a/test/get-memories-wiring.test.js b/test/get-memories-wiring.test.js new file mode 100644 index 00000000..2ce09161 --- /dev/null +++ b/test/get-memories-wiring.test.js @@ -0,0 +1,86 @@ +// test/get-memories-wiring.test.js — source-text assertions on the get-memories +// handler in main.js. +// +// The acceptance rule and its tests live in scan-md-files.js, which can be +// required directly; the handler cannot (Electron + better-sqlite3 are compiled +// against the Electron ABI), so it is checked the same way the FTS dirty-flag +// helpers are — by extracting the source and reading it. These are the +// properties that make the unit tests next door mean anything: a handler that +// stops calling the rule, or reads a file a second time behind its back, passes +// every test in scan-md-files.test.js. +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const mainSrc = fs.readFileSync(path.join(__dirname, '..', 'main.js'), 'utf8'); + +const start = mainSrc.indexOf("ipcMain.handle('get-memories'"); +const end = mainSrc.indexOf('// --- IPC: read-memory ---'); +assert.ok(start !== -1, "get-memories handler not found in main.js"); +assert.ok(end > start, '"// --- IPC: read-memory ---" marker not found after the handler'); + +const handler = mainSrc.slice(start, end); + +test('get-memories: every listing goes through the shared acceptance rule', () => { + // The project-root files are looked up by name, the rest by directory scan. + // Both must end up in acceptMdFile — a second hand-rolled lookup beside it is + // how the gate came to be missing from the root files in the first place. + // Asserted as the whole assignment, not just a call somewhere in the handler: + // `null && acceptMdFile(fp, isAllowed)` stops listing the root files entirely + // and still contains the call. + assert.match(handler, /const accepted = acceptMdFile\(fp, isAllowed\);/, + 'the project-root files must go through acceptMdFile, unconditionally'); + assert.doesNotMatch(handler, /existsSync\([^)]*\bfp\b/, + 'a by-name lookup must not probe the file itself — acceptMdFile owns that'); +}); + +test('get-memories: no scan is made without an allowlist', () => { + const calls = handler.match(/scanMdFiles\([^)]*\)/g) || []; + assert.ok(calls.length >= 4, `expected the handler to scan several directories, found ${calls.length}`); + for (const call of calls) { + assert.match(call, /,\s*isAllowed\s*\)/, `${call} must pass the allowlist`); + } + assert.match(handler, /acceptMdFile\([^)]*,\s*isAllowed\s*\)/, 'the by-name lookup must pass it too'); +}); + +test('get-memories: the FTS index uses the body already read, never a second read', () => { + // A second fs read of the same path is a second resolution of it, free to + // land somewhere the allowlist and the denylist never saw — and, if what + // lands there is a FIFO, to freeze the main process for good. + const ftsStart = handler.indexOf('upsertSearchEntries'); + assert.ok(ftsStart !== -1, 'the FTS block was not found in the handler'); + + assert.doesNotMatch(handler, /body:\s*fs\.readFileSync/, 'the index must not re-read the file'); + assert.match(handler, /body:\s*bodies\.get\(/, 'the index must use the body acceptMdFile returned'); + assert.doesNotMatch(handler.slice(ftsStart), /fs\.readFileSync/, + 'nothing in the FTS block may touch the filesystem again'); +}); + +test('get-memories: the bodies it collects do not travel to the renderer', () => { + // `listed` is what strips them; every push of a listing entry must go + // through it, or a file's full content rides the IPC payload to the renderer. + assert.match(handler, /const listed = \(\{ content, \.\.\.entry \}\)/, '`listed` must destructure content off the entry'); + const pushes = handler.match(/files\.push\(\{ \.\.\.[a-zA-Z]+/g) || []; + assert.ok(pushes.length >= 4, `expected several listing pushes, found ${pushes.length}`); + for (const push of pushes) { + assert.match(push, /\.\.\.listed$/, `${push}...) must spread listed(...), not the raw entry`); + } +}); + +test('get-memories: the known-project set is resolved once, not per file', () => { + // isAllowedMemoryPath rebuilds it from disk on every call — readdir of every + // project folder, plus a 256 KiB read off a JSONL each — so calling the + // module-level wrapper per file makes this handler quadratic in projects, + // synchronously, on the Electron main thread. + assert.match(handler, /const knownRoots = \[\.\.\.getKnownProjectPaths\(\)\]/, + 'the handler must bind the known roots once'); + assert.match(handler, /_isAllowedMemoryPath\([^)]*knownRoots\)/, + 'the predicate the handler passes around must close over that binding'); + assert.doesNotMatch(handler, /isAllowed\s*=\s*isAllowedMemoryPath\b/, + 'aliasing the per-call wrapper puts the rebuild back on the per-file path'); + assert.doesNotMatch(handler, /,\s*isAllowedMemoryPath\s*\)/, + 'the per-call wrapper must not be passed into a per-file path'); +}); diff --git a/test/scan-md-files.test.js b/test/scan-md-files.test.js index 75f939b3..c2c3daab 100644 --- a/test/scan-md-files.test.js +++ b/test/scan-md-files.test.js @@ -16,7 +16,7 @@ const os = require('os'); const path = require('path'); const { spawnSync } = require('child_process'); -const { scanMdFiles } = require('../scan-md-files'); +const { scanMdFiles, acceptMdFile } = require('../scan-md-files'); function rig() { const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'sb-scan-md-'))); @@ -174,3 +174,105 @@ test('scanMdFiles: a directory that does not exist scans to an empty list, not a assert.deepEqual(scanMdFiles(path.join(r.root, 'nope')), []); } finally { r.cleanup(); } }); + +// ── acceptMdFile ───────────────────────────────────────────────────────────── +// The project-root CLAUDE.md / GEMINI.md / agents.md are looked up by name +// rather than scanned, so they reach the list through this function directly. +// It is the single acceptance rule behind both paths — the memory listing had +// two of them, and only one carried the guards. + +test('acceptMdFile: accepts an ordinary file and reports its own name, mtime and content', () => { + const r = rig(); + try { + const fp = path.join(r.dir, 'CLAUDE.md'); + fs.writeFileSync(fp, 'project instructions\n'); + const out = acceptMdFile(fp); + assert.equal(out.filename, 'CLAUDE.md'); + assert.equal(out.filePath, fp); + assert.equal(out.modified, fs.statSync(fp).mtime.toISOString()); + // The body comes back so the caller never has to read the path again. + assert.equal(out.content, 'project instructions\n'); + } finally { r.cleanup(); } +}); + +test('acceptMdFile: the content returned is the one that passed the checks, not whatever the path holds later', (t) => { + const r = rig(); + try { + // The checks run against a path, the read has to happen while that path + // still means what was checked. A caller that re-reads it afterwards gets + // whatever has been moved into place since — which is the whole point of + // handing the body back instead of the path alone. + const fp = path.join(r.dir, 'CLAUDE.md'); + fs.writeFileSync(fp, 'the legitimate note'); + const out = acceptMdFile(fp, () => true); + + const swapped = path.join(r.elsewhere, 'swapped.md'); + fs.writeFileSync(swapped, 'content moved in afterwards'); + const tmp = path.join(r.dir, 'tmp-link'); + try { fs.symlinkSync(swapped, tmp); } + catch { return t.skip('cannot create a symlink on this machine'); } + fs.renameSync(tmp, fp); + + assert.equal(out.content, 'the legitimate note'); + assert.notEqual(fs.readFileSync(out.filePath, 'utf8'), out.content); + } finally { r.cleanup(); } +}); + +test('acceptMdFile: refuses a root CLAUDE.md that is a link to a credential location', (t) => { + const r = rig(); + try { + // A cloned repository added as a project brings its own CLAUDE.md. Reading + // one that points at a key puts the key in the searchable FTS index. + const sshDir = path.join(r.elsewhere, '.ssh'); + fs.mkdirSync(sshDir, { recursive: true }); + const key = path.join(sshDir, 'id_rsa'); + fs.writeFileSync(key, '-----BEGIN OPENSSH PRIVATE KEY-----'); + const fp = path.join(r.dir, 'CLAUDE.md'); + if (!symlink(key, fp, t)) return; + + assert.equal(acceptMdFile(fp, () => true), null); + } finally { r.cleanup(); } +}); + +test('acceptMdFile: refuses a file the allowlist does not cover', (t) => { + const r = rig(); + try { + const outside = path.join(r.elsewhere, 'notes.md'); + fs.writeFileSync(outside, 'content kept elsewhere'); + const fp = path.join(r.dir, 'CLAUDE.md'); + if (!symlink(outside, fp, t)) return; + + const allowed = (p) => { + const real = fs.realpathSync(p); + return real === r.dir || real.startsWith(r.dir + path.sep); + }; + assert.equal(acceptMdFile(fp, allowed), null); + assert.ok(acceptMdFile(fp), 'without an allowlist the link itself is fine'); + } finally { r.cleanup(); } +}); + +test('acceptMdFile: refuses a FIFO instead of blocking forever on it', (t) => { + const r = rig(); + try { + // Same hazard as the scan, reached by name instead: a project root holding + // a FIFO called CLAUDE.md would freeze the Electron main process, which + // runs this synchronously. See the scan's FIFO test for why a regression + // here hangs the suite rather than reddening it. + const fifo = path.join(r.dir, 'CLAUDE.md'); + const mk = spawnSync('mkfifo', [fifo]); + if (mk.error || mk.status !== 0) return t.skip('mkfifo unavailable on this machine'); + + assert.equal(acceptMdFile(fifo), null); + } finally { r.cleanup(); } +}); + +test('acceptMdFile: refuses an absent path, a directory and an empty file', () => { + const r = rig(); + try { + assert.equal(acceptMdFile(path.join(r.dir, 'nothing-here.md')), null); + assert.equal(acceptMdFile(r.dir), null); + const empty = path.join(r.dir, 'empty.md'); + fs.writeFileSync(empty, ' \n'); + assert.equal(acceptMdFile(empty), null); + } finally { r.cleanup(); } +});