From 61db7dda5cf9db734628c6d999e7c30cd36d1b64 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Thu, 6 Aug 2026 16:11:23 +0200 Subject: [PATCH] Refuse to delete a path that is not a registered site (#133) 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 --- src/main.js | 24 +++++--- src/site-registry.js | 77 ++++++++++++++++++++++++ test/site-registry.test.cjs | 114 ++++++++++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+), 7 deletions(-) create mode 100644 src/site-registry.js create mode 100644 test/site-registry.test.cjs diff --git a/src/main.js b/src/main.js index 50d3fa6..a6d80d4 100644 --- a/src/main.js +++ b/src/main.js @@ -30,6 +30,7 @@ const { killChildTree } = require('./kill-tree'); const { normalizeEol } = require('./git-update.cjs'); const { ensureAutocrlf, readTrunkInfo, collectDirtyFiles, discardChanges, updateToLatestTrunk } = require('./trunk-update'); const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url'); +const { deleteRegisteredSite } = require('./site-registry'); const WORDPRESS_GIT_URL = 'https://github.com/WordPress/wordpress-develop.git'; @@ -644,15 +645,24 @@ ipcMain.handle('sites:forget', async (_e, sitePath) => { return true; }); +// Only a path the app has on record gets removed from disk — see site-registry.js +// for why. A refusal is logged rather than dropped so a future caller that trips +// the guard shows up in the log file instead of just doing nothing. ipcMain.handle('sites:delete', async (_e, sitePath) => { const s = await getStore(); - const sites = s.get('sites').filter((p) => p !== sitePath); - s.set('sites', sites); - const meta = s.get('siteMeta'); - delete meta[sitePath]; - s.set('siteMeta', meta); - try { await fse.remove(sitePath); } catch {} - return true; + return deleteRegisteredSite(sitePath, { + sites: s.get('sites'), + forget: () => { + s.set('sites', s.get('sites').filter((p) => p !== sitePath)); + const meta = s.get('siteMeta'); + delete meta[sitePath]; + s.set('siteMeta', meta); + }, + // Best-effort, as before: a site whose registry entry is gone should not be + // stuck undeletable because its directory is missing or locked. + remove: async (p) => { try { await fse.remove(p); } catch {} }, + onRefused: (description) => logEvent('sites', `refused to delete ${description} — not a registered site`) + }); }); ipcMain.handle('sites:set-label', async (_e, sitePath, label) => { diff --git a/src/site-registry.js b/src/site-registry.js new file mode 100644 index 0000000..6557f26 --- /dev/null +++ b/src/site-registry.js @@ -0,0 +1,77 @@ +// The gate in front of the recursive directory removal in `sites:delete`. +// +// `sites:delete` is handed a path and calls `fse.remove` on it — the least +// recoverable action reachable from the window. The renderer is the only caller +// and every call site passes a path the app itself registered, so nothing today +// misuses it. But the renderer also displays content the app does not author: +// child-process output from an install or a build, and the site being developed. +// This module is the step that keeps any future influence over that path from +// turning "delete this site" into "remove this directory". +// +// The `sites` array in the app's store is its own record of what it created or +// adopted, so it is the boundary: a request to remove anything not in it is not +// one the app should carry out. This is the same shape as external-url.js — a +// pure check, a safe log formatter, and a wrapper whose effects are injected so +// both branches can be tested without an Electron process. + +// True only for a path the app has on record. Exact string match, the same +// convention `sites:add`/`sites:delete` already use (`sites.includes(sitePath)`, +// `filter((p) => p !== sitePath)`): the registry stores the paths verbatim, so a +// parent, a child, or a differently-normalized form is deliberately not a match. +// Anything that is not a non-empty string is refused rather than throwing. +function isRegisteredSite(sitePath, sites) { + if (typeof sitePath !== 'string' || sitePath === '') return false; + if (!Array.isArray(sites)) return false; + return sites.includes(sitePath); +} + +// Line breaks, and everything else that would let a refused path end a log line +// and start another one. +const CONTROL_CHARACTERS = /[\x00-\x1f\x7f-\x9f\u2028\u2029]/g; + +// A refused path is attacker-influenced by hypothesis, and it is about to be +// written into the file contributors attach to bug reports. It has to stay on +// one line — a newline would otherwise let it forge a second entry in the app's +// own timestamp-and-scope format — and it has to be bounded so a very long path +// cannot flood the file. Control characters are escaped rather than dropped so +// the line still says what the caller actually sent; truncation comes after +// escaping, since escaping is what decides the final length. +// +// This is the same concern, and the same escaping, as `describeRefusedUrl` in +// external-url.js. If a third caller ever needs it, the two should move into a +// shared safe-log helper rather than gain a third copy. +function describeRefusedSite(sitePath) { + if (typeof sitePath !== 'string') return `<${sitePath === null ? 'null' : typeof sitePath}>`; + + const oneLine = sitePath.replace(CONTROL_CHARACTERS, (c) => { + const code = c.codePointAt(0); + return code <= 0xff + ? `\\x${code.toString(16).padStart(2, '0')}` + : `\\u${code.toString(16).padStart(4, '0')}`; + }); + + if (oneLine.length <= 120) return oneLine; + return `${oneLine.slice(0, 120)}…`; +} + +// The `sites:delete` handler's body, kept out of main.js so both sides of the +// guard can be tested without an Electron process. `forget` drops the path from +// the store, `remove` is the real `fse.remove` in the app, and both are recording +// stubs in the tests. A path that is not registered performs neither: no store +// mutation and no removal, just a logged refusal. +async function deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused } = {}) { + if (!isRegisteredSite(sitePath, sites)) { + if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath)); + return false; + } + + forget(); + await remove(sitePath); + return true; +} + +module.exports = { + isRegisteredSite, + describeRefusedSite, + deleteRegisteredSite +}; diff --git a/test/site-registry.test.cjs b/test/site-registry.test.cjs new file mode 100644 index 0000000..a7a3536 --- /dev/null +++ b/test/site-registry.test.cjs @@ -0,0 +1,114 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { isRegisteredSite, describeRefusedSite, deleteRegisteredSite } = require('../src/site-registry.js'); + +// A couple of paths the app might actually hold in its registry, one per platform +// shape, so the tests aren't accidentally tied to POSIX separators. +const REGISTERED = ['/Users/dev/sites/my-site', 'C:\\Users\\dev\\sites\\other']; + +// Stands in for the store mutation and fse.remove, so "did this touch the store?" +// and "did this reach the disk?" are assertions rather than something the test has +// to take on trust. +function recorder(sites = REGISTERED) { + const removed = []; + const refused = []; + let forgotten = 0; + return { + removed, + refused, + forgotten: () => forgotten, + options: { + sites, + forget: () => { forgotten += 1; }, + remove: async (p) => { removed.push(p); }, + onRefused: (description) => { refused.push(description); } + } + }; +} + +test('a registered site is forgotten and removed', async () => { + const rec = recorder(); + + assert.equal(await deleteRegisteredSite('/Users/dev/sites/my-site', rec.options), true); + + assert.equal(rec.forgotten(), 1); + assert.deepEqual(rec.removed, ['/Users/dev/sites/my-site']); + assert.deepEqual(rec.refused, []); +}); + +test('a path the registry does not hold is neither forgotten nor removed', async () => { + const rec = recorder(); + + // A real directory, just not one this app created or adopted. + assert.equal(await deleteRegisteredSite('/Users/dev/somewhere-else', rec.options), false); + + assert.equal(rec.forgotten(), 0); + assert.deepEqual(rec.removed, []); + assert.equal(rec.refused.length, 1); +}); + +test('the match is exact — a parent or child of a registered site is not registered', () => { + // The registry stores paths verbatim, so removing a site must not become a + // lever on the directory above it or a sibling beside it. + assert.equal(isRegisteredSite('/Users/dev/sites/my-site', REGISTERED), true); + assert.equal(isRegisteredSite('/Users/dev/sites', REGISTERED), false); + assert.equal(isRegisteredSite('/Users/dev/sites/my-site/wp-content', REGISTERED), false); + assert.equal(isRegisteredSite('/Users/dev/sites/my-site/', REGISTERED), false); +}); + +test('junk input is refused rather than thrown', async () => { + for (const sitePath of ['', ' ', null, undefined, 42, {}, ['/Users/dev/sites/my-site']]) { + const rec = recorder(); + assert.equal(await deleteRegisteredSite(sitePath, rec.options), false); + assert.equal(rec.forgotten(), 0); + assert.deepEqual(rec.removed, []); + } + + // And with no registry at all, nothing is ever registered. + assert.equal(isRegisteredSite('/Users/dev/sites/my-site', undefined), false); +}); + +test('a refusal reports the path, truncated', () => { + const long = `/Users/dev/${'a'.repeat(500)}`; + const description = describeRefusedSite(long); + + assert.ok(description.length < 200, 'the log line should not carry a 500-character path'); + assert.ok(description.startsWith('/Users/dev/aaa'), 'enough of the path to diagnose the caller'); +}); + +// The path is about to be written into the file contributors attach to bug +// reports, and electron-log passes newlines through unchanged. Left as-is, a +// refused path could close the log line and open another one in the app's own +// timestamp-and-scope format — a log that describes events that never happened. +test('a refused path cannot forge a second log line', async () => { + const rec = recorder(); + const forged = '/tmp/x\n[2026-08-06 10:00:00.000] [info] (app) site deleted successfully'; + + await deleteRegisteredSite(forged, rec.options); + + assert.equal(rec.refused.length, 1); + assert.ok(!rec.refused[0].includes('\n'), 'the description must stay on one line'); + // Escaped, not dropped: the line still says what the caller actually sent. + assert.ok(rec.refused[0].includes('/tmp/x\\x0a[2026-08-06')); +}); + +test('every control character is escaped, not just newlines', () => { + // Carriage return alone ends a line in some viewers, and U+2028/U+2029 do it + // in others, so the whole class is escaped rather than the obvious member. + assert.equal(describeRefusedSite('/a\rb'), '/a\\x0db'); + assert.equal(describeRefusedSite('/a\tb'), '/a\\x09b'); + assert.equal(describeRefusedSite('/a\u2028b'), '/a\\u2028b'); + assert.equal(describeRefusedSite('/a\u0000b'), '/a\\x00b'); + // Ordinary paths are untouched. + assert.equal(describeRefusedSite('/Users/dev/sites/my-site'), '/Users/dev/sites/my-site'); +}); + +test('truncation is applied to the escaped form', () => { + // Escaping expands the string, so truncating first would let a path of control + // characters land in the log several times over the cap. + const description = describeRefusedSite(`/${'\n'.repeat(500)}`); + + assert.ok(description.length <= 121, `escaped description was ${description.length} characters`); + assert.ok(!description.includes('\n')); +});