From 05879913a489f496a0dd519688ea28ab7f478ca6 Mon Sep 17 00:00:00 2001 From: Bryandero98 Date: Tue, 1 Sep 2026 12:59:04 -0500 Subject: [PATCH 1/2] test: guard user-action-events file-backend writes against leaking to real data/ server/services/userActions.js writes through createPgFileFacade, which selects the file backend under NODE_ENV=test. Any suite that exercises an instrumented route (recordUserAction) without redirecting PATHS.data to a temp root therefore writes data/user-action-events.json into the developer's live data/ tree - the same bug class as #3683/ #3687. #5594 patched the three suites that tripped on it, but that is a per-suite fix: the next hook added to a route an untethered suite exercises silently re-opens the hole (epic #5593 phase 3/#5596 is explicitly about growing that allowlist). Add a structural guard directly on the write path (option 1 from the issue): right before the file-backend's atomicWrite, compare the live PATHS.data against the real repo data/ dir - computed independently via the same fileURLToPath/resolveInstallRoot technique lib/paths.js itself uses, so a suite's PATHS mock can't spoof the comparison too. A write attempted against the unredirected real path now throws a clear, actionable error instead of landing on disk. userActionsDataRootGuard.test.js proves the guard fires (and that nothing lands in the real data/ tree) by deliberately NOT mocking fileUtils.js - the one test in the suite meant to run unredirected. Verified the three previously-patched suites, every other known recordUserAction caller (settings.js, taskSchedule.js), and userActions.js's own suite all still pass with the guard active. Fixes atomantic#5605 --- server/services/userActions.js | 39 ++++++++++++++++++- .../services/userActionsDataRootGuard.test.js | 38 ++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 server/services/userActionsDataRootGuard.test.js diff --git a/server/services/userActions.js b/server/services/userActions.js index d9542157d1..31be6d4259 100644 --- a/server/services/userActions.js +++ b/server/services/userActions.js @@ -40,13 +40,15 @@ * fire-and-forget layer. */ -import { join } from 'path'; +import { dirname, join } from 'path'; +import { fileURLToPath } from 'url'; import { v4 as uuidv4 } from '../lib/uuid.js'; import { atomicWrite, ensureDir, PATHS, readJSONFile } from '../lib/fileUtils.js'; import { createFileWriteQueue } from '../lib/fileWriteQueue.js'; import { isPlainObject } from '../lib/objects.js'; import { createPgFileFacade, resolvePgBackend } from '../lib/pgFileFacade.js'; import { isTestRunner } from '../lib/db.js'; +import { resolveInstallRoot } from '../lib/dataRoot.js'; import { isUserActionActor, isUserActionType } from '../lib/userActionTypes.js'; import { insertUserActionEvent, listUserActionEvents, pruneUserActionEvents } from './userActionsDb.js'; @@ -55,6 +57,40 @@ import { insertUserActionEvent, listUserActionEvents, pruneUserActionEvents } fr // pre-mock value and crash any suite whose fileUtils stub omits PATHS.data. const eventsFile = () => join(PATHS.data, 'user-action-events.json'); +// The REAL repo data/ dir, computed independently of the (possibly test-mocked) +// `PATHS` import above via the same fileURLToPath/resolveInstallRoot technique +// lib/paths.js itself uses — so a suite that redirects PATHS.data to a temp +// root can't accidentally spoof this comparison too. `dataRoot.js` reads its +// own env var directly rather than through anything a PATHS mock would touch. +const REAL_REPO_DATA_DIR = join( + resolveInstallRoot(join(dirname(fileURLToPath(import.meta.url)), '../..')), + 'data', +); + +/** + * Structural guard against the bug class in #3683/#3687/#5605: a suite that + * exercises a route wired to `recordUserAction` without redirecting + * PATHS.data to a temp root would otherwise silently write + * `user-action-events.json` into the developer's live `data/` tree the next + * time such a route gets exercised — #5594 patched three known offenders + * one at a time, which is a per-suite fix, not a guard against the next one. + * Fires only under the test runner, and only at the moment a write is + * actually attempted, so a suite that merely reads (or never triggers a + * `recordUserAction` call) is unaffected either way. + */ +function assertTestDataRootRedirected() { + if (!isTestRunner() || PATHS.data !== REAL_REPO_DATA_DIR) return; + throw new Error( + 'recordUserAction attempted a file-backend write of user-action-events.json ' + + "into the repo's real data/ tree. This suite exercises a route wired to " + + 'recordUserAction but never redirected PATHS.data to a temp root - mock ' + + "`../lib/fileUtils.js` with lib/mockPathsDataRoot.js's makePathsProxy/" + + 'createTempDataRoot (the same fix #5594 applied to cos.test.js / ' + + 'cosTaskRoutes.test.js / cosAgentFeedback.test.js) rather than letting the ' + + 'write land here.', + ); +} + /** LLM-readable one-liner cap — the ledger is read by a model, not paged through. */ export const SUMMARY_MAX_CHARS = 240; /** Per-string payload cap; the containing object gets `truncated: true`. */ @@ -283,6 +319,7 @@ function makeFileBackend() { record: (event) => queueWrite(async () => { const events = await loadFileEvents(); if (events.some((row) => row.type === event.type && row.dedupeKey === event.dedupeKey)) return null; + assertTestDataRootRedirected(); await ensureDir(PATHS.data); await atomicWrite(eventsFile(), { events: pruneEvents([...events, event]) }); return event; diff --git a/server/services/userActionsDataRootGuard.test.js b/server/services/userActionsDataRootGuard.test.js new file mode 100644 index 0000000000..43325fd868 --- /dev/null +++ b/server/services/userActionsDataRootGuard.test.js @@ -0,0 +1,38 @@ +/** + * Regression coverage for the #5605 structural guard: a suite that exercises + * a route wired to `recordUserAction` without redirecting PATHS.data to a + * temp root must fail loudly instead of writing user-action-events.json into + * the repo's real data/ tree (the bug class #5594 patched per-suite for + * cos.test.js / cosTaskRoutes.test.js / cosAgentFeedback.test.js). + * + * Deliberately does NOT mock `../lib/fileUtils.js` — this is the one test in + * the suite that is SUPPOSED to run with PATHS.data unredirected, so it can + * prove the guard rejects that exact condition. If the guard regresses, this + * would otherwise be the test that writes a real file into the repo's data/ + * tree, so it also asserts that never happens. + */ +import { describe, it, expect, afterEach } from 'vitest'; +import { existsSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { PATHS } from '../lib/fileUtils.js'; +import { recordUserAction } from './userActions.js'; + +const REAL_EVENTS_FILE = join(PATHS.data, 'user-action-events.json'); + +afterEach(() => { + // Belt-and-suspenders: if the guard ever regresses, don't leave the leaked + // file behind for the next run to trip over. + rmSync(REAL_EVENTS_FILE, { force: true }); +}); + +describe('recordUserAction — data-root guard (#5605)', () => { + it('throws instead of writing user-action-events.json into the real data/ tree', async () => { + await expect(recordUserAction({ + type: 'cos.task.create', + summary: 'Guard regression probe', + dedupeKey: `guard-probe-${Math.random().toString(36).slice(2)}`, + })).rejects.toThrow(/real data\/ tree/); + + expect(existsSync(REAL_EVENTS_FILE)).toBe(false); + }); +}); From a17b6cf492d0b1122c4e91478f10193cb3e6388f Mon Sep 17 00:00:00 2001 From: Bryandero98 Date: Wed, 2 Sep 2026 02:02:45 -0500 Subject: [PATCH 2/2] fix: close the two /do:review holes in the data-root guard Addresses both blocking findings from the maintainer's review of #5627: 1. userActionsDataRootGuard.test.js's afterEach ran an unconditional rmSync on the repo's real user-action-events.json. On an install using the documented MEMORY_BACKEND=file escape hatch, that file can legitimately already be the developer's real ledger - the old `expect(existsSync(...)).toBe(false)` assertion failed spuriously against it, and the afterEach then deleted it regardless. Rewritten to snapshot present/absent + content before exercising the guard, assert the tree comes back unchanged (not merely absent), and only clean up a file this run's own guard regression actually created. 2. The guard in userActions.js's file-backend record() sat after the dedupe short-circuit's `return null`, so an un-redirected suite replaying an existing (type, dedupeKey) silently no-op'd past the guard with no throw - and by then loadFileEvents() had already read the real ledger into the test process regardless. Hoisted the assertion above both loadFileEvents() and the dedupe check. Also addresses the non-blocking note: REAL_REPO_DATA_DIR was a second, independent copy of paths.js's CODE_ROOT formula (fails open if it ever drifts). Extracted resolveCodeRootForModule() into dataRoot.js as the single source of truth; both paths.js and userActions.js now derive their root through it, with paths.js's own PATHS object unchanged in every value it produces. Verified: full `cd server && npm test` - 1793/1810 files pass, same 16 files/58 tests fail with or without this change (confirmed via git stash), all pre-existing Windows-only environment noise (symlink creation without Developer Mode privileges, EBUSY temp-file locks, one missing local Python module) - none touch paths.js/dataRoot.js/ userActions.js. Also re-ran the guard test and userActions.test.js in isolation, both green. Co-Authored-By: Claude Sonnet 5 --- server/lib/dataRoot.js | 19 +++++++- server/lib/paths.js | 10 ++-- server/services/userActions.js | 27 ++++++----- .../services/userActionsDataRootGuard.test.js | 46 +++++++++++++------ 4 files changed, 68 insertions(+), 34 deletions(-) diff --git a/server/lib/dataRoot.js b/server/lib/dataRoot.js index 304710072a..aae52f132f 100644 --- a/server/lib/dataRoot.js +++ b/server/lib/dataRoot.js @@ -15,11 +15,28 @@ * worktree-rooted process so callers (boot migrations) can skip work that * assumes the real install tree instead of crashing. */ -import { isAbsolute, resolve as resolvePath, sep } from 'path'; +import { dirname, isAbsolute, join, resolve as resolvePath, sep } from 'path'; +import { fileURLToPath } from 'url'; /** Env var a real launch sets to pin the install root (see ecosystem.config.cjs). */ export const DATA_ROOT_ENV = 'PORTOS_DATA_ROOT'; +/** + * The executing checkout's root, derived from a caller's `import.meta.url` + * (two directories up from wherever that module physically lives). Single + * source of truth for this depth assumption — `lib/paths.js`'s `CODE_ROOT` + * and any code that must independently re-derive the real (non-test-mocked) + * install root, such as `userActions.js`'s data-root guard, both resolve it + * through here so the two can never silently drift apart if this formula + * ever changes. + * + * @param {string} moduleUrl the caller's `import.meta.url` + * @returns {string} absolute path two directories above the caller's file + */ +export function resolveCodeRootForModule(moduleUrl) { + return join(dirname(fileURLToPath(moduleUrl)), '../..'); +} + /** * Resolve the PortOS install root (the parent of `data/` and `data.reference/`). * diff --git a/server/lib/paths.js b/server/lib/paths.js index d44ff67d6e..e6b74acf5f 100644 --- a/server/lib/paths.js +++ b/server/lib/paths.js @@ -1,16 +1,12 @@ /** Canonical install/source roots and path construction helpers. */ import { homedir } from 'os'; -import { dirname, join } from 'path'; -import { fileURLToPath } from 'url'; -import { resolveInstallRoot } from './dataRoot.js'; - -const __lib_filename = fileURLToPath(import.meta.url); -const __lib_dirname = dirname(__lib_filename); +import { join } from 'path'; +import { resolveCodeRootForModule, resolveInstallRoot } from './dataRoot.js'; // The executing checkout's root (where THIS file physically lives). Code/source // paths — the repo root for git ops, `lib/slashdo` — must stay anchored here so // they always point at the checkout that loaded the code. -const CODE_ROOT = join(__lib_dirname, '../..'); +const CODE_ROOT = resolveCodeRootForModule(import.meta.url); // The install root that holds the runtime `data/` tree. Prefer an explicit // PORTOS_DATA_ROOT env var over the executing-file location so a process booted diff --git a/server/services/userActions.js b/server/services/userActions.js index 31be6d4259..6e68fb3147 100644 --- a/server/services/userActions.js +++ b/server/services/userActions.js @@ -40,15 +40,14 @@ * fire-and-forget layer. */ -import { dirname, join } from 'path'; -import { fileURLToPath } from 'url'; +import { join } from 'path'; import { v4 as uuidv4 } from '../lib/uuid.js'; import { atomicWrite, ensureDir, PATHS, readJSONFile } from '../lib/fileUtils.js'; import { createFileWriteQueue } from '../lib/fileWriteQueue.js'; import { isPlainObject } from '../lib/objects.js'; import { createPgFileFacade, resolvePgBackend } from '../lib/pgFileFacade.js'; import { isTestRunner } from '../lib/db.js'; -import { resolveInstallRoot } from '../lib/dataRoot.js'; +import { resolveCodeRootForModule, resolveInstallRoot } from '../lib/dataRoot.js'; import { isUserActionActor, isUserActionType } from '../lib/userActionTypes.js'; import { insertUserActionEvent, listUserActionEvents, pruneUserActionEvents } from './userActionsDb.js'; @@ -58,14 +57,12 @@ import { insertUserActionEvent, listUserActionEvents, pruneUserActionEvents } fr const eventsFile = () => join(PATHS.data, 'user-action-events.json'); // The REAL repo data/ dir, computed independently of the (possibly test-mocked) -// `PATHS` import above via the same fileURLToPath/resolveInstallRoot technique -// lib/paths.js itself uses — so a suite that redirects PATHS.data to a temp -// root can't accidentally spoof this comparison too. `dataRoot.js` reads its -// own env var directly rather than through anything a PATHS mock would touch. -const REAL_REPO_DATA_DIR = join( - resolveInstallRoot(join(dirname(fileURLToPath(import.meta.url)), '../..')), - 'data', -); +// `PATHS` import above via the same resolveCodeRootForModule/resolveInstallRoot +// technique lib/paths.js itself uses (both go through the shared helper, so +// they can't silently drift apart) — so a suite that redirects PATHS.data to a +// temp root can't accidentally spoof this comparison too. `dataRoot.js` reads +// its own env var directly rather than through anything a PATHS mock would touch. +const REAL_REPO_DATA_DIR = join(resolveInstallRoot(resolveCodeRootForModule(import.meta.url)), 'data'); /** * Structural guard against the bug class in #3683/#3687/#5605: a suite that @@ -317,9 +314,15 @@ function makeFileBackend() { return { name: 'file', record: (event) => queueWrite(async () => { + // Hoisted above loadFileEvents()/the dedupe check on purpose (#5627 + // review): an un-redirected suite replaying an existing + // (type, dedupeKey) used to hit the dedupe short-circuit's `return null` + // BEFORE this guard ever ran, silently no-op'ing past it with no throw — + // and by then loadFileEvents() had already read the real ledger into the + // test process regardless. Running the guard first closes both holes. + assertTestDataRootRedirected(); const events = await loadFileEvents(); if (events.some((row) => row.type === event.type && row.dedupeKey === event.dedupeKey)) return null; - assertTestDataRootRedirected(); await ensureDir(PATHS.data); await atomicWrite(eventsFile(), { events: pruneEvents([...events, event]) }); return event; diff --git a/server/services/userActionsDataRootGuard.test.js b/server/services/userActionsDataRootGuard.test.js index 43325fd868..be961d4824 100644 --- a/server/services/userActionsDataRootGuard.test.js +++ b/server/services/userActionsDataRootGuard.test.js @@ -11,28 +11,46 @@ * would otherwise be the test that writes a real file into the repo's data/ * tree, so it also asserts that never happens. */ -import { describe, it, expect, afterEach } from 'vitest'; -import { existsSync, rmSync } from 'node:fs'; +import { describe, it, expect } from 'vitest'; +import { existsSync, readFileSync, rmSync } from 'node:fs'; import { join } from 'node:path'; import { PATHS } from '../lib/fileUtils.js'; import { recordUserAction } from './userActions.js'; const REAL_EVENTS_FILE = join(PATHS.data, 'user-action-events.json'); -afterEach(() => { - // Belt-and-suspenders: if the guard ever regresses, don't leave the leaked - // file behind for the next run to trip over. - rmSync(REAL_EVENTS_FILE, { force: true }); -}); - describe('recordUserAction — data-root guard (#5605)', () => { it('throws instead of writing user-action-events.json into the real data/ tree', async () => { - await expect(recordUserAction({ - type: 'cos.task.create', - summary: 'Guard regression probe', - dedupeKey: `guard-probe-${Math.random().toString(36).slice(2)}`, - })).rejects.toThrow(/real data\/ tree/); + // Snapshot whatever is already there BEFORE exercising the guard. An + // install using the documented MEMORY_BACKEND=file escape hatch may + // legitimately already have this file as its real ledger — this test + // must prove the guard leaves it untouched, not assert it's absent (the + // previous version's `expect(existsSync(...)).toBe(false)` failed + // spuriously on exactly that install shape, and its unconditional + // afterEach `rmSync` then deleted the developer's real ledger). + const existedBefore = existsSync(REAL_EVENTS_FILE); + const contentBefore = existedBefore ? readFileSync(REAL_EVENTS_FILE, 'utf8') : null; + + try { + await expect(recordUserAction({ + type: 'cos.task.create', + summary: 'Guard regression probe', + dedupeKey: `guard-probe-${Math.random().toString(36).slice(2)}`, + })).rejects.toThrow(/real data\/ tree/); - expect(existsSync(REAL_EVENTS_FILE)).toBe(false); + // The guard must leave the real tree exactly as it found it — present + // and unchanged, or still absent — never newly created or modified. + expect(existsSync(REAL_EVENTS_FILE)).toBe(existedBefore); + if (existedBefore) { + expect(readFileSync(REAL_EVENTS_FILE, 'utf8')).toBe(contentBefore); + } + } finally { + // Only clean up a file THIS run's own guard regression created — never + // touch one that was already there before it ran (which could be a + // developer's real MEMORY_BACKEND=file ledger). + if (!existedBefore && existsSync(REAL_EVENTS_FILE)) { + rmSync(REAL_EVENTS_FILE, { force: true }); + } + } }); });