From f6ab8561ae85157f60de70c0972a690b6108f36e Mon Sep 17 00:00:00 2001 From: John McLear Date: Mon, 18 May 2026 12:45:03 +0100 Subject: [PATCH] =?UTF-8?q?fix(pad):=20outdated=20notice=20=E2=80=94=20res?= =?UTF-8?q?olve=20author=20from=20token=20cookie=20(Qodo=20#7804)=20(#7805?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(updater): resolve pad author from token cookie, not session.user Etherpad does not populate an authorID into the express-session user object for pad visitors, so resolveRequestAuthor() always returned null in production, causing computeOutdated() to return EMPTY and the pad-side gritter to never fire. Replace the session-based lookup with a cookie-based path that mirrors how the socket.io handshake resolves pad-visitor identity: read the HttpOnly `token` (or `token`) cookie and call authorManager.getAuthorId(token, user) via dynamic import (same circular-init guard pattern as the PadManager import). Update the test harness to mock AuthorManager instead of injecting a fake req.session.user.author, and to set req.cookies.token directly. All 9 cases continue to pass. Co-Authored-By: Claude Sonnet 4.6 * docs(openapi): clarify admin spec scope includes pad-side endpoints /api/version-status is a public pad-side endpoint but lives in the admin OpenAPI document because it shares the same internal route registration. Add a note to info.description so downstream tooling consumers are not misled into treating it as an admin-only route. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- src/node/hooks/express/openapi-admin.ts | 5 ++- src/node/hooks/express/updateStatus.ts | 39 +++++++++-------- .../specs/hooks/express/updateStatus.test.ts | 43 +++++++++++++------ 3 files changed, 54 insertions(+), 33 deletions(-) diff --git a/src/node/hooks/express/openapi-admin.ts b/src/node/hooks/express/openapi-admin.ts index b1074e3f8..3d659aa32 100644 --- a/src/node/hooks/express/openapi-admin.ts +++ b/src/node/hooks/express/openapi-admin.ts @@ -20,7 +20,10 @@ export const generateAdminDefinition = (): any => ({ title: 'Etherpad Admin API', description: 'Authenticated administrative endpoints consumed by the Etherpad admin UI. ' + - 'Distinct from the public /api/{version}/* surface served by /api/openapi.json.', + 'Distinct from the public /api/{version}/* surface served by /api/openapi.json. ' + + 'For completeness this document also includes non-admin endpoints that are ' + + 'consumed by the pad UI itself (e.g. /api/version-status) since they share the ' + + 'same internal route registration.', version: getEpVersion(), }, paths: { diff --git a/src/node/hooks/express/updateStatus.ts b/src/node/hooks/express/updateStatus.ts index 0371ccc58..bdeb1179a 100644 --- a/src/node/hooks/express/updateStatus.ts +++ b/src/node/hooks/express/updateStatus.ts @@ -31,31 +31,32 @@ export const firstAuthorOf = (pad: {pool?: {numToAttrib?: Recordtoken`) with every + * same-origin request, and `authorManager.getAuthorId(token, user)` maps the + * token to a stable authorID via the `token2author` DB key. + * + * We do NOT use `req.session.user.author` because Etherpad does not populate + * an authorID into the express-session `user` object for pad visitors, so that + * field is always undefined in production. + * + * On any failure path we return null and the caller treats the request as + * anonymous, resulting in an EMPTY (no-badge) response. */ export const resolveRequestAuthor = async (req: any): Promise => { - const readAuthor = (): string | null => { - const a = req?.session?.user?.author; - return typeof a === 'string' && a !== '' ? a : null; - }; - const fromSession = readAuthor(); - if (fromSession !== null) return fromSession; try { - const expressModule = await import('../express'); - const mw = (expressModule as any).sessionMiddleware; - if (typeof mw !== 'function') return null; - await new Promise((resolve, reject) => { - mw(req, {} as any, (err?: unknown) => (err ? reject(err) : resolve())); - }); + const cookiePrefix = (settings as any).cookie?.prefix ?? ''; + const token = req?.cookies?.[`${cookiePrefix}token`]; + if (typeof token !== 'string' || token === '') return null; + const authorManagerMod: any = await import('../../db/AuthorManager'); + const authorManager = authorManagerMod.default ?? authorManagerMod; + if (typeof authorManager.getAuthorId !== 'function') return null; + const authorId = await authorManager.getAuthorId(token, req?.session?.user); + return typeof authorId === 'string' && authorId !== '' ? authorId : null; } catch { return null; } - return readAuthor(); }; interface OutdatedResponse { diff --git a/src/tests/backend-new/specs/hooks/express/updateStatus.test.ts b/src/tests/backend-new/specs/hooks/express/updateStatus.test.ts index 9e4299bda..cfe95099d 100644 --- a/src/tests/backend-new/specs/hooks/express/updateStatus.test.ts +++ b/src/tests/backend-new/specs/hooks/express/updateStatus.test.ts @@ -5,12 +5,12 @@ * (same as production), then exercised via supertest. `loadState` is mocked so * tests control the "latest" version without touching the filesystem. * `PadManager` is mocked so pad-creation doesn't require a running database. + * `AuthorManager` is mocked so token→authorID resolution doesn't require a DB. * - * Session injection: `resolveRequestAuthor` reads `req.session.user.author` - * directly. A simple before-route middleware sets that property on each - * request, keyed by the `X-Test-Author` request header, so individual tests - * can choose which author the request "belongs to" without needing real - * session middleware. + * Author injection: `resolveRequestAuthor` reads the `token` cookie and calls + * `authorManager.getAuthorId(token, user)`. Tests set `sessionAuthor` to + * control which authorID the mock returns for the test token `t.alice`. + * `null` means "anonymous" (getAuthorId returns null / empty for the token). */ import {describe, it, expect, vi, beforeAll, beforeEach, afterEach} from 'vitest'; @@ -38,6 +38,14 @@ vi.mock('../../../../../node/updater', () => ({ getDetectedInstallMethod: () => 'git', })); +// AuthorManager is dynamically imported inside resolveRequestAuthor(). Stubbing +// it here lets tests control token→authorID resolution without a DB. +vi.mock('../../../../../node/db/AuthorManager', () => ({ + default: { + getAuthorId: vi.fn(), + }, +})); + // PadManager is dynamically imported inside computeOutdated(). Stubbing it // here lets us control pad existence and author-pool contents without a DB. vi.mock('../../../../../node/db/PadManager', () => { @@ -58,6 +66,7 @@ vi.mock('../../../../../node/db/PadManager', () => { // --------------------------------------------------------------------------- import * as stateModule from '../../../../../node/updater/state'; +import * as authorManagerModule from '../../../../../node/db/AuthorManager'; import { expressCreateServer, _resetBadgeCacheForTests, @@ -112,21 +121,22 @@ let app: Express; let request: ReturnType; /** - * The author that the fake session middleware will inject into req.session. - * Tests that need a specific author set this before making a request. - * `null` means "anonymous" (no session author). + * The author that `authorManager.getAuthorId` will return for the test token + * `t.alice`. Tests set this before making a request. `null` means "anonymous" + * (getAuthorId returns null/empty, so resolveRequestAuthor returns null). */ let sessionAuthor: string | null = null; +/** Fixed test token used in every request. The cookie name has no prefix in tests. */ +const TEST_TOKEN = 't.alice'; + beforeAll(() => { app = express(); - // Fake session middleware: sets req.session.user.author from our test - // variable, so resolveRequestAuthor() in the route sees the right identity. + // Inject the test token cookie so resolveRequestAuthor() sees it. + // The real cookie-parser middleware is not needed: we set req.cookies directly. app.use((req: any, _res, next) => { - if (sessionAuthor !== null) { - req.session = {user: {author: sessionAuthor}}; - } + req.cookies = {token: TEST_TOKEN}; next(); }); @@ -143,6 +153,13 @@ beforeEach(() => { sessionAuthor = null; // Reset the loadState spy so each test controls its own return value. vi.mocked(stateModule.loadState).mockReset(); + // Wire up the AuthorManager mock: return sessionAuthor (or null) for our test token. + vi.mocked((authorManagerModule as any).default.getAuthorId).mockImplementation( + async (token: string) => { + if (token === TEST_TOKEN && sessionAuthor !== null) return sessionAuthor; + return null; + }, + ); }); afterEach(() => {