From ddf84fa6f2119cd68813f0e8155d908015c7bb80 Mon Sep 17 00:00:00 2001 From: John McLear Date: Wed, 10 Jun 2026 10:46:37 +0100 Subject: [PATCH] fix(admin): don't let one unreadable pad empty the Manage-pads list (#7935) (#7938) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(admin): don't let one unreadable pad empty the Manage-pads list (#7935) The admin /settings `padLoad` handler hydrates every pad via getPad() to build the listing (the default lastEdited sort forces a full scan). findKeys('pad:*','*:*:*') returns every key under the `pad:` prefix, including legacy/foreign or migration-corrupted records — e.g. a value stored as a JSON string rather than a pad object, which a botched dirty.db -> PostgreSQL migration produces. Loading such a record makes Pad.init throw `Cannot use 'in' operator to search for 'pool'`. The handler had no try/catch, so one bad record rejected the whole request: no `results:padLoad` was emitted (the SPA showed an empty "No results" state forever — the reported symptom) and the unhandled rejection could exit the server. Make loadMeta resilient — a failed getPad() logs a warning and returns zeroed metadata so the bad pad still surfaces for deletion instead of hiding every other pad — and wrap the handler so it always emits a terminal reply and never bubbles to the process-level handler. Tests: - backend: tests/backend/specs/admin/padLoadResilience.ts asserts a corrupt pad:* record no longer hides the readable pads (fails without the fix: no results:padLoad reply + server exit). - e2e: tests/frontend-new/admin-spec/admin_pads_page.spec.ts covers create pad -> welcome-page recent-pads -> /admin Manage-pads UI. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(admin): sanitize padLoad error logs to avoid leaking pad content Qodo review: the error logs (and the error field returned to the SPA) used `${err}` / err.message verbatim. For the corruption case the TypeError message embeds the raw stored value ("...'pool' in "), which for a real pad can be document text — so logging it verbatim could leak content, bloat logs, and let embedded newlines forge log lines. Add a safeErr() helper (error name + single-line, 120-char-capped message, control chars stripped) and use it in both the per-pad warning and the outer handler error log, and for the error field emitted to the client. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(admin): make surfaced corrupt pads deletable from the admin UI Qodo review: padLoad now surfaces unreadable pad:* records (zeroed metadata) so admins can delete them, but deletePad's normal path (doesPadExists -> getPad -> Pad.remove) can't touch such a record: doesPadExists() returns false for a non-object value and getPad() throws, so the surfaced pad was effectively undeletable and the handler emitted nothing. When doesPadExists() is false but a raw `pad:${id}` value is present, fall back to a raw key purge: sweep sub-keys (revs/chat/deletionToken/…) via findKeys, drop the readonly mapping, then padManager.removePad() (which removes the main key + pad-list + cache entry). Always emit a terminal results:deletePad reply (including for an already-absent id) so the UI clears the row instead of silently doing nothing, and wrap the handler so failures are logged (sanitized) rather than swallowed. Adds a backend test asserting a surfaced corrupt pad can be deleted and disappears from the listing. Co-Authored-By: Claude Opus 4.8 (1M context) * test(admin): assert corrupt-pad delete via listing, not raw db.get The delete test probed `db.get('pad:')` for null right after deletePad. That passed on postgres but failed on the dirty backend (Windows CI): ueberdb2's per-backend read/write buffering can return the just-removed value immediately after remove(), so it asserted storage internals rather than behaviour. The deletion is still durable (same db.remove() every pad uses; the in-memory pad-list entry is dropped synchronously). Assert the behavioural outcome instead — the corrupt pad disappears from the padLoad listing while the good pad remains. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- src/node/hooks/express/adminsettings.ts | 93 +++++++-- .../backend/specs/admin/padLoadResilience.ts | 180 ++++++++++++++++++ .../admin-spec/admin_pads_page.spec.ts | 54 ++++++ 3 files changed, 315 insertions(+), 12 deletions(-) create mode 100644 src/tests/backend/specs/admin/padLoadResilience.ts create mode 100644 src/tests/frontend-new/admin-spec/admin_pads_page.spec.ts diff --git a/src/node/hooks/express/adminsettings.ts b/src/node/hooks/express/adminsettings.ts index 9381f0de8..678da2568 100644 --- a/src/node/hooks/express/adminsettings.ts +++ b/src/node/hooks/express/adminsettings.ts @@ -11,6 +11,7 @@ import settings, {getEpVersion, getGitCommit, reloadSettings} from '../../utils/ import {getLatestVersion} from '../../utils/UpdateCheck'; import {redactSettings} from '../../utils/AdminSettingsRedact'; const padManager = require('../../db/PadManager'); +const db = require('../../db/DB'); const api = require('../../db/API'); import {deleteRevisions} from '../../utils/Cleanup'; @@ -26,6 +27,24 @@ const queryPadLimit = 12; const PAD_HYDRATE_CONCURRENCY = 16; const logger = log4js.getLogger('adminSettings'); +// Errors thrown while reading a pad record can embed the raw stored value +// in their message — e.g. Pad.init's `'pool' in value` TypeError stringifies +// the offending value ("Cannot use 'in' operator to search for 'pool' in +// "). For a corrupt record that value may be actual pad text, so +// logging it verbatim would leak content, bloat the log, and let embedded +// newlines forge log lines. Reduce any error to its name plus a single-line, +// length-capped message before logging. +const safeErr = (err: unknown): string => { + const e = err as {name?: unknown, message?: unknown} | null; + const name = (e && typeof e.name === 'string' && e.name) || 'Error'; + const msg = String((e && e.message) ?? err ?? '') + .replace(/[\r\n\t]+/g, ' ') + .replace(/\s{2,}/g, ' ') + .trim() + .slice(0, 120); + return `${name}: ${msg}`; +}; + // Concurrency-limited Promise.all replacement. Preserves the input // order in the returned array (caller slices later). Used by padLoad // to bound DB reads during hydration. @@ -147,6 +166,7 @@ exports.socketio = (hookName: string, {io}: any) => { socket.on('padLoad', async (query: PadSearchQuery) => { + try { const {padIDs} = await padManager.listAllPads(); // ── 1. Pattern filter (cheap, by name only) ───────────────────── @@ -172,13 +192,27 @@ exports.socketio = (hookName: string, {io}: any) => { const needsFullScan = filter !== 'all' || query.sortBy !== 'padName'; const loadMeta = async (padName: string): Promise => { - const pad = await padManager.getPad(padName); - return { - padName, - lastEdited: await pad.getLastEdit(), - userCount: api.padUsersCount(padName).padUsersCount, - revisionNumber: pad.getHeadRevisionNumber(), - }; + // A single unreadable record must not take out the whole listing. + // `findKeys('pad:*', '*:*:*')` returns every key under the `pad:` + // prefix, including legacy/foreign or migration-corrupted records + // (e.g. a value stored as a JSON *string* rather than a pad object, + // which makes Pad.init throw `'pool' in value`). Before this guard + // one such key rejected the whole `padLoad` handler — the admin + // "Manage pads" page then showed *no* pads at all (issue #7935) and + // the unhandled rejection could exit the server. Surfacing the bad + // pad with zeroed metadata lets an admin see and delete it instead. + try { + const pad = await padManager.getPad(padName); + return { + padName, + lastEdited: await pad.getLastEdit(), + userCount: api.padUsersCount(padName).padUsersCount, + revisionNumber: pad.getHeadRevisionNumber(), + }; + } catch (err) { + logger.warn(`padLoad: skipping unreadable pad "${padName}": ${safeErr(err)}`); + return {padName, lastEdited: 0 as any, userCount: 0, revisionNumber: 0}; + } }; // Lazily lifted so we don't load every pad twice on the fast path. @@ -256,16 +290,51 @@ exports.socketio = (hookName: string, {io}: any) => { const data: {total: number, results?: PadQueryResult[]} = {total, results}; socket.emit('results:padLoad', data); + } catch (err) { + // Never leave the SPA hanging on a missing reply (it would show an + // empty "No results" state forever) and never let this bubble up to + // the process-level unhandledRejection handler, which would exit the + // whole server. Always emit a terminal reply for the request. + logger.error(`padLoad failed: ${safeErr(err)}`); + socket.emit('results:padLoad', + {total: 0, results: [], error: safeErr(err)}); + } }) socket.on('deletePad', async (padId: string) => { - const padExists = await padManager.doesPadExists(padId); - if (padExists) { - logger.info(`Deleting pad: ${padId}`); - const pad = await padManager.getPad(padId); - await pad.remove(); + try { + if (await padManager.doesPadExists(padId)) { + // Healthy pad — full relational cleanup (revs, chat, readonly, + // authors, deletion token, hooks). + logger.info(`Deleting pad: ${padId}`); + const pad = await padManager.getPad(padId); + await pad.remove(); + socket.emit('results:deletePad', padId); + return; + } + + // doesPadExists() is false either because nothing is stored under + // this id, or because the record is unreadable (a non-object value + // leaves `value.atext` undefined). The latter is exactly what + // padLoad now surfaces with zeroed metadata — getPad()/Pad.remove() + // would throw on it, so fall back to a raw key purge. Without this + // the surfaced corrupt pad is undeletable from the admin UI. + const raw = await db.get(`pad:${padId}`); + if (raw != null) { + logger.info(`Deleting unreadable pad record via raw key purge: ${padId}`); + // Best-effort sweep of sub-keys (revs/chat/deletionToken/…) and + // the readonly mapping, then the main key + pad-list/cache entry. + const subKeys: string[] = (await db.findKeys(`pad:${padId}:*`, null)) || []; + await Promise.all(subKeys.map((k) => db.remove(k))); + await db.remove(`pad2readonly:${padId}`); + await padManager.removePad(padId); + } + // Always emit a terminal reply (even for an already-absent id) so the + // UI clears the row instead of silently doing nothing. socket.emit('results:deletePad', padId); + } catch (err) { + logger.error(`deletePad failed for "${padId}": ${safeErr(err)}`); } }) diff --git a/src/tests/backend/specs/admin/padLoadResilience.ts b/src/tests/backend/specs/admin/padLoadResilience.ts new file mode 100644 index 000000000..28c5d284f --- /dev/null +++ b/src/tests/backend/specs/admin/padLoadResilience.ts @@ -0,0 +1,180 @@ +'use strict'; + +// Regression test for issue #7935 ("Display issue of notes"): pads exist +// (visible on the welcome page, returned by the API `listAllPads`) but the +// admin "Manage pads" UI shows none. +// +// Root cause: the admin /settings `padLoad` handler hydrates every pad via +// `padManager.getPad()` to build the listing (the default `lastEdited` sort +// forces a full scan). `findKeys('pad:*', '*:*:*')` returns *every* key under +// the `pad:` prefix, including legacy / foreign / migration-corrupted records +// — e.g. a value stored as a JSON *string* rather than a pad object, which is +// exactly what a botched dirty.db → PostgreSQL migration produces. Loading +// such a record makes `Pad.init` throw `Cannot use 'in' operator to search +// for 'pool' in `. Before the fix that single rejection took out the +// whole handler: no `results:padLoad` was ever emitted (the SPA showed an +// empty "No results" state forever) and the unhandled rejection could exit +// the server. The handler now skips unreadable pads (surfacing them with +// zeroed metadata so an admin can still delete them) and always emits a +// terminal reply. + +import {strict as assert} from 'assert'; +import setCookieParser from 'set-cookie-parser'; + +const io = require('socket.io-client'); +const common = require('../../common'); +const settings = require('../../../../node/utils/Settings'); +const padManager = require('../../../../node/db/PadManager'); +const db = require('../../../../node/db/DB'); + +const adminSocket = async () => { + settings.users = settings.users || {}; + settings.users['test-admin'] = {password: 'test-admin-password', is_admin: true}; + const savedRequireAuthentication = settings.requireAuthentication; + settings.requireAuthentication = true; + let res: any; + try { + res = await (common.agent as any) + .get('/admin/') + .auth('test-admin', 'test-admin-password'); + } finally { + settings.requireAuthentication = savedRequireAuthentication; + } + const resCookies = setCookieParser.parse(res, {map: true}); + const reqCookieHdr = Object.entries(resCookies) + .map(([name, cookie]: [string, any]) => + `${name}=${encodeURIComponent(cookie.value)}`) + .join('; '); + const socket = io(`${common.baseUrl}/settings`, { + forceNew: true, + query: {cookie: reqCookieHdr}, + }); + await new Promise((resolve, reject) => { + const onErr = (err: any) => { socket.off('connect', onConn); reject(err); }; + const onConn = () => { socket.off('connect_error', onErr); resolve(); }; + socket.once('connect', onConn); + socket.once('connect_error', onErr); + }); + return socket; +}; + +const ask = (socket: any, evt: string, payload: any, replyEvt: string, timeoutMs = 10000) => + new Promise((resolve, reject) => { + const timer = setTimeout( + () => reject(new Error(`no \`${replyEvt}\` reply within ${timeoutMs}ms`)), timeoutMs); + socket.once(replyEvt, (data: any) => { clearTimeout(timer); resolve(data); }); + socket.emit(evt, payload); + }); + +describe(__filename, function () { + let socket: any; + let savedUsers: any; + let savedRequireAuthentication: boolean; + let setupCompleted = false; + const tag = `padLoadResilience-${Date.now()}-${Math.floor(Math.random() * 1e6)}`; + const goodId = `${tag}-good`; + const corruptId = `${tag}-corrupt`; + + before(async function () { + this.timeout(120000); + await common.init(); + + savedUsers = settings.users; + savedRequireAuthentication = settings.requireAuthentication; + setupCompleted = true; + + try { + socket = await adminSocket(); + } catch (err: any) { + console.warn( + `[padLoadResilience] admin socket connect failed (${err && err.message}); ` + + "skipping suite — likely an authenticate-hook plugin rejecting the test's " + + 'admin credentials.'); + this.skip(); + return; + } + + // A normal, readable pad — this is what must still show up. + await padManager.getPad(goodId, 'good content\n'); + + // A pad that enters the pad-name index normally, then has its stored + // value clobbered into a non-object (a JSON string) to mimic a + // migration-corrupted / foreign `pad:*` record. Evicting it from the + // in-memory cache forces the next getPad() to re-read the bad value. + await padManager.getPad(corruptId, 'temp\n'); + await db.set(`pad:${corruptId}`, 'corrupt-non-object-value'); + padManager.unloadPad(corruptId); + + // Sanity-check that the setup actually reproduces the failing read; if + // this stops throwing the test is no longer exercising the bug. + await assert.rejects(padManager.getPad(corruptId), + 'expected the corrupted pad record to make getPad throw'); + }); + + after(async function () { + if (socket) socket.disconnect(); + if (!setupCompleted) return; + if (settings.users) delete settings.users['test-admin']; + settings.users = savedUsers; + settings.requireAuthentication = savedRequireAuthentication; + for (const id of [goodId, corruptId]) { + try { await db.remove(`pad:${id}`); } catch { /* ignore */ } + try { await db.remove(`pad:${id}:revs:0`); } catch { /* ignore */ } + try { padManager.unloadPad(id); } catch { /* ignore */ } + } + }); + + it('a single corrupt pad does not hide every other pad (issue #7935)', async function () { + this.timeout(30000); + // The default query the SPA sends on initial load: lastEdited sort forces + // the full-scan hydration path that touches every pad — including the + // corrupt one. + const res = await ask(socket, 'padLoad', { + pattern: tag, offset: 0, limit: 12, + sortBy: 'lastEdited', ascending: false, filter: 'all', + }, 'results:padLoad'); + + const names = res.results.map((r: any) => r.padName); + assert.ok(names.includes(goodId), + `the readable pad must still be listed; got ${JSON.stringify(names)}`); + // The bad pad is surfaced (zeroed metadata) rather than silently dropped, + // so an admin can see and delete it. + assert.ok(names.includes(corruptId), + `the corrupt pad should surface for deletion; got ${JSON.stringify(names)}`); + assert.equal(res.total, 2, `expected total=2, got ${JSON.stringify(res)}`); + }); + + it('still replies on the fast path (padName sort) with a corrupt pad present', async function () { + this.timeout(30000); + const res = await ask(socket, 'padLoad', { + pattern: tag, offset: 0, limit: 12, + sortBy: 'padName', ascending: true, filter: 'all', + }, 'results:padLoad'); + const names = res.results.map((r: any) => r.padName); + assert.ok(names.includes(goodId), `got ${JSON.stringify(names)}`); + assert.ok(names.includes(corruptId), `got ${JSON.stringify(names)}`); + }); + + // Runs last: surfacing a corrupt pad is only useful if it can be removed. + // deletePad's normal path (doesPadExists + getPad + Pad.remove) can't touch + // an unreadable record, so it must fall back to a raw key purge. + it('a surfaced corrupt pad can be deleted from the admin UI', async function () { + this.timeout(30000); + const ack = await ask(socket, 'deletePad', corruptId, 'results:deletePad'); + assert.equal(ack, corruptId, `expected deletePad to ack "${corruptId}", got ${JSON.stringify(ack)}`); + + // Assert the user-facing outcome — the corrupt pad is gone from the + // listing (its pad-list entry was dropped) while the good pad stays. + // (We deliberately don't probe `db.get('pad:')` here: ueberdb2's + // per-backend read/write buffering can still return the just-removed + // value immediately after `remove()`, so that would be a flaky + // storage-internals assertion rather than a behavioural one.) + const res = await ask(socket, 'padLoad', { + pattern: tag, offset: 0, limit: 12, + sortBy: 'padName', ascending: true, filter: 'all', + }, 'results:padLoad'); + const names = res.results.map((r: any) => r.padName); + assert.ok(!names.includes(corruptId), `corrupt pad still listed: ${JSON.stringify(names)}`); + assert.ok(names.includes(goodId), `good pad missing after delete: ${JSON.stringify(names)}`); + }); +}); diff --git a/src/tests/frontend-new/admin-spec/admin_pads_page.spec.ts b/src/tests/frontend-new/admin-spec/admin_pads_page.spec.ts new file mode 100644 index 000000000..11b0e60cc --- /dev/null +++ b/src/tests/frontend-new/admin-spec/admin_pads_page.spec.ts @@ -0,0 +1,54 @@ +import {expect, test} from "@playwright/test"; +import {loginToAdmin} from "../helper/adminhelper"; +import {goToPad, writeToPad} from "../helper/padHelper"; + +// End-to-end coverage for issue #7935: a pad that exists must be visible +// both on the welcome page's "recent pads" list (driven by localStorage, +// i.e. gated on the browser having opened the pad) and in the admin +// "Manage pads" UI (driven by the /settings socket `padLoad` handler, +// which enumerates the DB). The admin side regressed when a single +// unreadable pad record made `padLoad` throw and silently return nothing +// — see specs/admin/padLoadResilience.ts for the server-side guard. + +// /admin tests mutate global server state, so keep them serial. +test.describe.configure({mode: 'serial'}); + +const ADMIN_URL = 'http://localhost:9001/admin'; + +test.describe('a created pad shows up on the home page and in /admin', () => { + // Unique, URL-safe id so the recent-pads localStorage entry and the admin + // search both target exactly this pad and ignore leftovers from other suites. + const padId = `pw-pads-7935-${Date.now()}`; + + test('opening a pad lists it in the welcome page recent-pads', async ({page}) => { + await goToPad(page, padId); + await writeToPad(page, 'hello from 7935'); + + // Opening the pad writes it to `recentPads` localStorage (colibris + // pad.js). The welcome page renders that list — same browser context, + // so the entry carries over. + await page.goto('http://localhost:9001/'); + const recentPad = page.locator('.recent-pad', {hasText: padId}); + await expect(recentPad).toBeVisible({timeout: 10000}); + await expect(recentPad.locator('a')).toHaveText(padId); + }); + + test('the same pad is listed in the admin Manage pads UI', async ({page}) => { + await loginToAdmin(page, 'admin', 'changeme1'); + await page.goto(`${ADMIN_URL}/pads`); + + await expect(page.getByRole('heading', {name: 'Manage pads'})) + .toBeVisible({timeout: 30000}); + + // Narrow the (full-scan) listing to our pad. The search is debounced + // server-side; allow the round-trip to settle. + const search = page.getByPlaceholder('Search for pads'); + await search.fill(padId); + + await expect(page.locator('.pm-pad-title', {hasText: padId})) + .toBeVisible({timeout: 15000}); + // The "No results" empty state must NOT be showing — the exact #7935 + // symptom was an empty Manage-pads list for pads that demonstrably exist. + await expect(page.locator('.pm-empty')).toHaveCount(0); + }); +});