fix(schedule): a schedule file reached through a symlink is listed and runnable - #294
Conversation
…d runnable 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.
devsuitup
left a comment
devsuitup
left a comment
There was a problem hiding this comment.
Reviewed at 5a4f1c2 against main 155fcef, with the path checks replayed against the real modules (run-schedule-now-target.js, scan-md-files.js, ipc-path-validator.js), not the test stubs. The scheduler-side diagnosis and the resolveRunNowTarget reshaping read right; the new tests exercise the production modules and the symlink cases did run on windows-2022 (job log checked, none skipped). Two things block the merge, one is housekeeping.
1. scanMdFiles now reads through links with no sensitivity gate — blocking
scan-md-files.js:36-41 accepts a symlink entry and statSync/readFileSync its target; get-memories (main.js:1234, 1259) feeds every returned path into the FTS index with a plain readFileSync, and I find no isSensitivePath / isAllowedMemoryPath anywhere in that handler. Before this change a symlink dirent failed isFile() and never reached a read, so the absence of a gate never mattered. Now a notes.md → ~/.ssh/id_rsa link under any scanned directory — .claude, .claude/commands, the project root, of every project ever indexed, including a cloned third-party repo added as a project — lands verbatim in the searchable index. Reproduced with exactly that layout: the private-key content came back from the same read get-memories performs.
Ask: gate the resolved target — isSensitivePath at least, isAllowedMemoryPath against the known roots preferably — inside scanMdFiles before accepting a symlink entry (the file-panel handlers at main.js:917-992 show the pattern), and add a test with a link pointing at a sensitive path. One sentence in .ai/contexts/ipc-bridge.md's guard inventory naming that gate.
2. projectPath and realPath are two independent resolutions — say so, and pin it
run-schedule-now-target.js:51 resolves the file, :60 resolves the listing project from the requested path's components; nothing relates the two afterwards. That is the point of the change (a linked-in schedule's bytes live in the dotfiles repo, the run belongs to the listing project), and both still have to pass isPathAllowed, so I am not asking for the pre-PR derivation back. But the consequence is that projA/.claude/commands → projB/.claude/commands (both known projects) now runs projB's schedule content with cwd = projA, auto-approved — pre-PR it ran with cwd = projB. Reproduced. That is a deliberate semantic, not a bug, provided it is written down: one line in .ai/contexts/schedule-runner.md stating that the file and the cwd may legitimately resolve to different allowed roots and that both are allowlisted independently, plus a test pinning the two-known-projects case so the next reader does not "fix" it either way by accident.
3. Comment ceiling
run-schedule-now-target.js:23-37, :56-59 and the scan-md-files.js:9-28 header carry the incident narrative that .ai/contexts/schedule-runner.md already has. Repo rule is a one-line pointer in code (PRs #127/#130); please sweep.
Also checked, no finding: .. segments are collapsed by path.resolve before any check; ELOOP/junction/dangling links fall to resolveOnDisk → null → refused; trailing dot/space fails the anchored filename regex; the /proj vs /proj-evil boundary holds (isWithinRoot appends the separator); a plain non-linked schedule gets a byte-identical projectPath. The two "pre-existing" failures you list both pass on my machine (~/.claude/CLAUDE.md is not a link here), consistent with your machine-dependent explanation.
…just where it is linked from
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:
<repo>/.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.
`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.
|
All three addressed, in Before the three points — the reviewed commit had a hole of its ownYour review is against Confirmed A/B against That commit also documents a second finding you would want to know about: 1.
|
devsuitup
left a comment
devsuitup
left a comment
There was a problem hiding this comment.
Re-reviewed 39f5c35 and f7c0c01 against the real modules; CI green on f7c0c01 (9 checks, windows-2022 included, symlink tests ran unskipped).
- Gate — resolved.
scanMdFileschecksisSensitivePathand the injectedisAllowedMemoryPathon every entry before reading, both resolving the target on disk; wired at the fiveget-memoriescall sites. Replayed:notes.md → ~/.ssh/id_rsaexcluded; a link to a.mdinside another known project listed. - Independent resolutions — resolved as documented semantics: doc line in
schedule-runner.md, test pinningprojA → projB(content from B, cwd A) passes against the module. - Comment sweep — done, both files down to header + JSDoc.
The extra fix in 39f5c35 (filename pattern and isSensitivePath re-checked on the resolved target, so a non-schedule file linked in under a schedule-*.md name cannot become the prompt) is a real hole closed; dropping isPathAllowed(real) now fails the suite, confirmed by mutation.
One thing outside this diff, opened as #295: the project-root CLAUDE.md/GEMINI.md/agents.md block in the same handler still reads with a bare readFileSync and no gate — same class, pre-existing, follow-up.
2 participants
The bug, as it was found
A weekly
schedule-audit-memory.mdwas created through the schedule creator, then moved into a git-versioned dotfiles repo, with the project's.claude/commands/keeping a symlink to it — so the schedule itself is versioned rather than sitting loose in~. From that moment the brain tab showed the project as having no schedules at all.Nothing had actually stopped:
scanScheduleslists names and reads through the link, so the cron loop kept firing the task every Monday. The only broken part was the half a user can see. That is the worst shape for this class of bug — the UI says "no schedule" while the task runs unattended, and the disagreement is silent on both sides.Two readers of these files disagreed with the scheduler about what a schedule file is.
1. The brain tab list —
scanMdFilesget-memoriesaccepted a directory entry ondirent.isFile(). For a symlink that is false however ordinary the file at the other end is, so the entry was dropped with no error, no log, nothing. Same fate for a symlinked~/.claude/CLAUDE.mdor memory note, and for the memory FTS index built from the same list.It now accepts on what the entry resolves to (
fs.statSync, which follows the link), so:read-memoryandrun-schedule-now;Moved to
scan-md-files.js:main.jsneeds Electron to load, and this is the part worth testing directly. Per-entry failures are contained while we are here — one unreadable file used to abort the scan of every file after it in the same directory.2. The play button's path guard —
resolveRunNowTargetSame file, refused with
not inside a project .claude/commands directory. The guard checked the.claude/commandsshape against the symlink-free path, which for a linked-in schedule points at the dotfiles repo.The guard now answers each question with the path that question is about:
.claude/commandsparents)schedule-*.mdisSensitivePathisAllowedMemoryPathThe second commit exists because the first got that table wrong, in a way worth stating plainly rather than squashing away. Checking the filename only on the requested path left nothing constraining the target: any file the allowlist reaches — anything under
~/.claude, anything in an indexed project — became the prompt of a spawnedclaude -pby being linked under a schedule-shaped name.git stores symlinks natively and a relative link survives cloning, so a cloned repository carries this with no local access needed.
run-schedule-nowreads the target,createScheduleSessionwrites it verbatim into a session JSONL as a user message, and the spawned CLI sends it as conversation context. Confirmed A/B againstmain(not a schedule filebefore, accepted after), and closed by re-asserting the filename on the resolved target.projectPathis now the project whose.claude/commandslists the schedule, which is also the cwd the run should have had all along; previously a linked-in schedule would have been spawned in the dotfiles repo.Scope this does not cover
A dotfiles repo that is outside every allowed root — one the user has never opened as a project — is still refused, now with
path not allowedrather thannot inside a project .claude/commands directory. WideningisAllowedMemoryPathto reach it is a separate decision and not one to make inside a bug fix. The case this does fix is the common one: the configuration repo lives under a directory that is already a known project.Tests
test/scan-md-files.test.js(new, 6 cases) and 6 new cases intest/run-schedule-now-target.test.js, including the linked-in schedule end to end and each refusal above.One existing test was found to be proving nothing. "a symlinked commands directory escaping the project is caught by disk resolution" used a containment stub with no equality branch, so it answered
falsefor the project root, refused on the project check, never exercised the check on the file, and asserted onlyok === false. DeletingisPathAllowed(real)outright left the whole suite green. The stub now has its equality branch and the test asserts the error string.Mutation matrix, run against the current tests — each of the five load-bearing checks is now killed by at least one test:
isPathAllowed(real)isPathAllowed(projectPath)resolveOnDiskon the project rootisSensitivePathFull suite on this machine: 1505/1509 pass. The two failures are pre-existing on
mainhere and unrelated:isAllowedMemoryPath: allows files under ~/.claude/— on this machine~/.claude/CLAUDE.mdreally is a symlink out of~/.claude, and the test passes an empty project list, so nothing allows the target. In the app the real project list covers it.real git: an absolute pathspec outside the repo is refused by git itself— the assertion matches on git's refusal message, which differs for this git version/locale.Both reproduce on a clean
maincheckout; neither is touched by this change.Known, not fixed here
.CLAUDE\commandsis refused). In practice the renderer always builds these segments withpath.join(projectPath, '.claude', 'commands'), so the spelling is literal.schedule-*.mdis added, resolve the link rather than the dirent; both context docs now say so.