Refuse to delete a path that is not a registered site - #138
Merged
Conversation
sites:delete took a path from the renderer and called fse.remove on it without first checking that the path was one the app registered. The sites array in the store is the app's own record of what it created or adopted, so it is the boundary: a request to remove anything outside it is refused and logged, and no directory is removed. Same shape as the external-URL guard: a pure check, a safe log formatter that keeps a crafted path from forging a log line, and a wrapper whose effects are injected so both branches test without an Electron process. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a registry-membership guard before recursively deleting site directories.
Changes:
- Introduces guarded deletion and safe refusal logging.
- Wires the guard into
sites:delete. - Adds focused unit tests.
Review result: 1 security issue requiring changes: renderer code can register an arbitrary path before deleting it.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/site-registry.js |
Implements registry checks and safe logging. |
src/main.js |
Integrates guarded site deletion. |
test/site-registry.test.cjs |
Tests matching, refusal, and log sanitization. |
| try { await fse.remove(sitePath); } catch {} | ||
| return true; | ||
| return deleteRegisteredSite(sitePath, { | ||
| sites: s.get('sites'), |
This was referenced Aug 7, 2026
juanmaguitar
added a commit
that referenced
this pull request
Aug 7, 2026
Deleting a site is the least recoverable action in the app. #138 put a gate in front of it — `deleteRegisteredSite` refuses any path the app does not have on record — and the module is well tested, but nothing proved the handler still calls it. `test/ipc-wiring.test.cjs` classified `sites:delete` under NO_DELEGATION as "electron-store write plus a directory removal", which was false: replacing the handler body with a bare `fse.remove` kept the whole suite green, the exact failure that suite exists to catch. The reason it was classified around the hole rather than through it is that the handler reads the settings store first, and the store arrives via a dynamic `import()` that the harness's `Module._load` hook cannot intercept. So the store moves into `src/settings-store.js`, a seam the harness can stand in for, the same shape the neighbouring registry calls already have. `getStore` is unchanged — same singleton, same deferred import, same reasons for deferring it. With the seam in place, `sites:delete` moves into WIRED and gains two tests: one that it asks site-registry at all, and one end-of-wire test on real directories with the real `fse.remove`, proving an unregistered path reaches neither the removal nor the store. Both fail if the gate is cut. `site:status` was in NOT_REACHABLE for the same store limitation, so its recorded reason would have become untrue; it is wired too rather than left carrying a stale claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 7, 2026
Closes #145. **Stacked on #153** — review that one first; this PR targets its branch and its diff will shrink once #153 merges. #138 added the gate that refuses to delete a path that is not a registered site, and the gate module is well tested. Nothing tested that the delete handler *uses* it: replacing the handler body with a bare recursive removal kept the whole suite green. The channel was filed under `NO_DELEGATION` with a reason that was not true, because the handler reached the settings store through a dynamic ESM import the wiring harness cannot intercept — the same hole recorded for `site:status`. That import now lives in `src/settings-store.js`, a module the harness can stand in for. `getStore()` is unchanged for every caller. `sites:delete` moves into `WIRED` with two tests, and `site:status` leaves `NOT_REACHABLE`, since its recorded reason would otherwise have become false. Verified by temporarily replacing the handler body with a bare `fse.remove`: both new tests fail, and the suite was green before the change. The second test runs against real temporary directories with the real `fse.remove`, so it proves an unregistered path reaches neither the removal nor the store. ## Review `.github/instructions/code-review.instructions.md` run against the branch: 2 findings, both low — 1 fixed here (the fake store now returns a structured clone on `get`, so a test could not mutate shared state through it), 1 deferred: the handler swallows the error from its best-effort removal, which predates this change and is not what this PR is about. Test written before the fix. `npm test` and `npm run test:electron` both 188/188 on the stack, lint clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #133.
Deleting a site removed whatever directory the request named.
sites:deletetook asitePathstraight from the renderer and calledfse.removeon it — recursively, best-effort, error swallowed — without first checking that the path was one the app registered.The
sitesarray in the store is the app's own record of what it created or adopted, so it is the boundary: a delete request naming anything outside it is refused and logged, and no directory is removed. This is the second half of the same chain as #121 (opening external addresses) — the window displays content the app does not author, and removing a directory is the least recoverable action reachable from it.What changed
src/site-registry.js(new) — an Electron-free module mirroringexternal-url.js:isRegisteredSite(sitePath, sites)— pure exact-match check (same conventionsites:add/sites:deletealready use).describeRefusedSite(sitePath)— safe log formatter: escapes control chars/newlines and truncates, so a crafted path can't forge a second entry in the log contributors attach to bug reports.deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused })— the handler body with injected effects; refuses (logs, no store mutation, no removal) when the path isn't registered.src/main.js—sites:deletedelegates to that wrapper. Refusals log vialogEvent('sites', 'refused to delete … — not a registered site'), mirroring theurl:openwiring. Removal stays best-effort.test/site-registry.test.cjs(new) — 8 tests: registered/unregistered/junk paths, exact-match (parent and child of a registered site are rejected), truncation, and log-forging.Notes
falseon refusal instead of alwaystrue. The renderer (onDelete) ignores the result and just refreshes, so the UI is unaffected — and this path is unreachable from the UI today.sites:delete, where the outcome is genuinely unrecoverable (a recursivefse.remove). The other site-path handlers use their path as a git worktree or a process cwd, with far smaller blast radius, so they are deliberately left as-is; the guard is a reusable module if one of them ever needs it.Done when
🤖 Generated with Claude Code