From 88610f6bee90485443d70d039dc50cdb8b1c23cd Mon Sep 17 00:00:00 2001 From: John McLear Date: Mon, 4 May 2026 08:14:45 +0100 Subject: [PATCH] =?UTF-8?q?fix(authors):=20action=20Qodo=20review=20?= =?UTF-8?q?=E2=80=94=20lastSeen,=20flag-gating,=20defensive=20payloads?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Qodo on PR #7667 surfaced three issues: 1. (Bug, Correctness) lastSeen lost or stale. - mapAuthorWithDBKey only updated `timestamp` for returning authors so the admin /authors 'Last seen' column drifted on every reconnect without an identity write. Now stamps both timestamp and lastSeen. - anonymizeAuthor's two db.set calls overwrote globalAuthor without preserving lastSeen, blanking the column for erased rows. Both writes now carry forward `existing.lastSeen ?? existing.timestamp`. - searchAuthors falls back to rec.timestamp when rec.lastSeen is missing so legacy records aren't blank. 2. (Rule violation, Security) /authors route not flag-gated. The new admin-socket read paths (authorLoad, anonymizeAuthorPreview) were always-on; only the destructive anonymizeAuthor was gated. Project rule (Compliance ID 6) requires new features behind a flag, disabled by default. All three handlers now check gdprAuthorErasure.enabled and return {error:'disabled'} when off. The sidebar 'Authors' link is hidden when the flag is off (deep-link to /admin/authors still works and renders the existing disabled banner so docs can point to it). 3. (Bug, Reliability) Socket destructure throws on missing payload. Handlers signed `async ({authorID}: {authorID: string}) => …` threw before try/catch when a client emitted with no payload, producing an unhandled rejection. Switched to `async (payload: any) => { const authorID = payload?.authorID; … }`. Test impact: anonymizeAuthorSocket gains two regressions (authorLoad disabled-shape, payload-less emits don't crash) and updates the preview-when-flag-off test to assert {error:'disabled'} per the new gating posture (was 'preview still works'). admintroubleshooting sidebar-count reverts 7 → 6 since the Authors link is now conditional on the flag (off by default in the test environment). Co-Authored-By: Claude Opus 4.7 (1M context) --- admin/src/App.tsx | 5 ++- src/node/db/AuthorManager.ts | 18 +++++++-- src/node/hooks/express/adminsettings.ts | 32 +++++++++++++-- .../specs/admin/anonymizeAuthorSocket.ts | 40 ++++++++++++++++++- .../admin-spec/admintroubleshooting.spec.ts | 6 ++- 5 files changed, 89 insertions(+), 12 deletions(-) diff --git a/admin/src/App.tsx b/admin/src/App.tsx index 169a8bab2..5d527f085 100644 --- a/admin/src/App.tsx +++ b/admin/src/App.tsx @@ -12,6 +12,7 @@ import {UpdateBanner} from "./components/UpdateBanner"; const WS_URL = import.meta.env.DEV ? 'http://localhost:9001' : '' export const App = () => { const setSettings = useStore(state => state.setSettings); + const erasureEnabled = useStore(state => state.gdprAuthorErasureEnabled) const {t} = useTranslation() const navigate = useNavigate() const [sidebarOpen, setSidebarOpen] = useState(true) @@ -106,7 +107,9 @@ export const App = () => {
    • -
    • + {erasureEnabled && ( +
    • + )}
  • Communication
    • diff --git a/src/node/db/AuthorManager.ts b/src/node/db/AuthorManager.ts index 1c7c3c0a9..8fafe51f5 100644 --- a/src/node/db/AuthorManager.ts +++ b/src/node/db/AuthorManager.ts @@ -130,8 +130,11 @@ const mapAuthorWithDBKey = async (mapperkey: string, mapper:string) => { } // there is an author with this mapper - // update the timestamp of this author - await db.setSub(`globalAuthor:${author}`, ['timestamp'], Date.now()); + // update the timestamp + lastSeen of this author so the admin + // /authors "Last seen" column reflects the most recent connect + const now = Date.now(); + await db.setSub(`globalAuthor:${author}`, ['timestamp'], now); + await db.setSub(`globalAuthor:${author}`, ['lastSeen'], now); // return the author return {authorID: author}; @@ -374,11 +377,15 @@ exports.anonymizeAuthor = async ( // Zero the display identity now — without the `erased` sentinel — so a // partial run still hides the name. The sentinel itself is only set at // the end (below) so a failure in chat scrub lets the next call resume. + // Preserve `lastSeen` so the admin /authors UI's column stays accurate + // for erased records (the operator can still see when the author was + // last active before erasure). if (!dryRun) { await db.set(`globalAuthor:${authorID}`, { colorId: 0, name: null, timestamp: Date.now(), + lastSeen: existing.lastSeen ?? existing.timestamp ?? null, padIDs: existing.padIDs || {}, }); } @@ -414,6 +421,7 @@ exports.anonymizeAuthor = async ( colorId: 0, name: null, timestamp: Date.now(), + lastSeen: existing.lastSeen ?? existing.timestamp ?? null, padIDs: existing.padIDs || {}, erased: true, erasedAt: new Date().toISOString(), @@ -513,7 +521,11 @@ exports.searchAuthors = async (query: { name: rec.name ?? null, colorId: rec.colorId ?? null, mapper: mappers, - lastSeen: typeof rec.lastSeen === 'number' ? rec.lastSeen : null, + // Prefer lastSeen; fall back to timestamp for legacy records that + // pre-date the new field so the admin /authors column isn't blank. + lastSeen: typeof rec.lastSeen === 'number' + ? rec.lastSeen + : (typeof rec.timestamp === 'number' ? rec.timestamp : null), erased, }); } diff --git a/src/node/hooks/express/adminsettings.ts b/src/node/hooks/express/adminsettings.ts index 8702910b6..302492117 100644 --- a/src/node/hooks/express/adminsettings.ts +++ b/src/node/hooks/express/adminsettings.ts @@ -309,8 +309,25 @@ exports.socketio = (hookName: string, {io}: any) => { const authorManager = require('../../db/AuthorManager'); - socket.on('authorLoad', async (query: any) => { + // The admin author-erasure UI (PR #7667) is gated as a single + // feature: when gdprAuthorErasure.enabled is false, all three + // socket handlers refuse so the page is fully off by default per + // project rule "new features behind a feature flag, disabled by + // default" (Qodo Compliance ID 6). The destructive + // anonymizeAuthor stays gated as before; the read paths + // (authorLoad / preview) are also gated so listing data isn't + // exposed without an explicit opt-in. + const erasureEnabled = () => + !!(settings.gdprAuthorErasure && settings.gdprAuthorErasure.enabled); + + socket.on('authorLoad', async (payload: any) => { try { + if (!erasureEnabled()) { + socket.emit('results:authorLoad', + {total: 0, results: [], error: 'disabled'}); + return; + } + const query = payload || {}; const data = await authorManager.searchAuthors({ pattern: query.pattern || '', offset: query.offset || 0, @@ -327,8 +344,14 @@ exports.socketio = (hookName: string, {io}: any) => { } }); - socket.on('anonymizeAuthorPreview', async ({authorID}: {authorID: string}) => { + socket.on('anonymizeAuthorPreview', async (payload: any) => { + const authorID = payload?.authorID; try { + if (!erasureEnabled()) { + socket.emit('results:anonymizeAuthorPreview', + {authorID, error: 'disabled'}); + return; + } if (!authorID) { socket.emit('results:anonymizeAuthorPreview', {authorID, error: 'authorID is required'}); @@ -346,9 +369,10 @@ exports.socketio = (hookName: string, {io}: any) => { } }); - socket.on('anonymizeAuthor', async ({authorID}: {authorID: string}) => { + socket.on('anonymizeAuthor', async (payload: any) => { + const authorID = payload?.authorID; try { - if (!settings.gdprAuthorErasure || !settings.gdprAuthorErasure.enabled) { + if (!erasureEnabled()) { socket.emit('results:anonymizeAuthor', {authorID, error: 'disabled'}); return; } diff --git a/src/tests/backend/specs/admin/anonymizeAuthorSocket.ts b/src/tests/backend/specs/admin/anonymizeAuthorSocket.ts index ef3984c05..486a2dad0 100644 --- a/src/tests/backend/specs/admin/anonymizeAuthorSocket.ts +++ b/src/tests/backend/specs/admin/anonymizeAuthorSocket.ts @@ -144,8 +144,12 @@ describe(__filename, function () { } }); - it('anonymizeAuthorPreview still works when flag is off (read-only)', + it('anonymizeAuthorPreview returns {error: "disabled"} when flag is off', async function () { + // Per Qodo Compliance ID 6 ('new features behind a feature flag, + // disabled by default') the preview event is also gated, not just + // the live anonymizeAuthor. The page renders its disabled banner + // off the socket reply when this fires. settings.gdprAuthorErasure.enabled = false; try { const tag = `prev-off-${Date.now()}`; @@ -153,9 +157,41 @@ describe(__filename, function () { `m-${tag}`, `PrevOff ${tag}`); const preview = await ask(socket, 'anonymizeAuthorPreview', {authorID}, 'results:anonymizeAuthorPreview'); - assert.ok(preview.removedExternalMappings >= 1); + assert.equal(preview.error, 'disabled'); + assert.equal(preview.removedExternalMappings, undefined, + 'no counters should leak when the flag is off'); } finally { settings.gdprAuthorErasure.enabled = true; } }); + + it('authorLoad returns {error: "disabled"} when flag is off', + async function () { + settings.gdprAuthorErasure.enabled = false; + try { + const res = await ask(socket, 'authorLoad', + {pattern: '', offset: 0, limit: 12, sortBy: 'name', + ascending: true, includeErased: false}, + 'results:authorLoad'); + assert.equal(res.error, 'disabled'); + assert.deepEqual(res.results, []); + } finally { + settings.gdprAuthorErasure.enabled = true; + } + }); + + it('handlers do not crash on payload-less emits', + async function () { + // Pre-Qodo-fix the destructure `({authorID}: ...)` threw before + // try/catch when client emitted with no payload. Both gated + // handlers now accept `payload: any` and read defensively. + const previewRes = await ask(socket, 'anonymizeAuthorPreview', + undefined, 'results:anonymizeAuthorPreview'); + assert.ok(previewRes.error, + `expected error, got ${JSON.stringify(previewRes)}`); + const eraseRes = await ask(socket, 'anonymizeAuthor', + undefined, 'results:anonymizeAuthor'); + assert.ok(eraseRes.error, + `expected error, got ${JSON.stringify(eraseRes)}`); + }); }); diff --git a/src/tests/frontend-new/admin-spec/admintroubleshooting.spec.ts b/src/tests/frontend-new/admin-spec/admintroubleshooting.spec.ts index 8cd0e93f6..346d2513d 100644 --- a/src/tests/frontend-new/admin-spec/admintroubleshooting.spec.ts +++ b/src/tests/frontend-new/admin-spec/admintroubleshooting.spec.ts @@ -14,8 +14,10 @@ test('Shows troubleshooting page manager', async ({page}) => { await page.goto('http://localhost:9001/admin/help') await page.waitForSelector('.menu') const menu = page.locator('.menu'); - // Sidebar nav: plugins, settings, help, pads, authors, shout, update. - await expect(menu.locator('li')).toHaveCount(7); + // Sidebar nav: plugins, settings, help, pads, shout, update. + // The Authors link only renders when gdprAuthorErasure.enabled = true, + // which the test environment leaves false by default. + await expect(menu.locator('li')).toHaveCount(6); }) test('Shows a version number', async function ({page}) {