Skip to content

Commit f6ab856

Browse files
JohnMcLearclaude
andauthored
fix(pad): outdated notice — resolve author from token cookie (Qodo #7804) (#7805)
* 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 `<prefix>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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 29dac6b commit f6ab856

3 files changed

Lines changed: 54 additions & 33 deletions

File tree

src/node/hooks/express/openapi-admin.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,10 @@ export const generateAdminDefinition = (): any => ({
2020
title: 'Etherpad Admin API',
2121
description:
2222
'Authenticated administrative endpoints consumed by the Etherpad admin UI. ' +
23-
'Distinct from the public /api/{version}/* surface served by /api/openapi.json.',
23+
'Distinct from the public /api/{version}/* surface served by /api/openapi.json. ' +
24+
'For completeness this document also includes non-admin endpoints that are ' +
25+
'consumed by the pad UI itself (e.g. /api/version-status) since they share the ' +
26+
'same internal route registration.',
2427
version: getEpVersion(),
2528
},
2629
paths: {

src/node/hooks/express/updateStatus.ts

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -31,31 +31,32 @@ export const firstAuthorOf = (pad: {pool?: {numToAttrib?: Record<number | string
3131
};
3232

3333
/**
34-
* Resolve the express-session author for a plain HTTP GET. The pad-side fetch
35-
* is `credentials: 'same-origin'`, so the `express_sid` cookie is sent
36-
* automatically. The global express-session middleware should have populated
37-
* `req.session` already — but if not (e.g. test harness without middleware),
38-
* we re-invoke it ourselves. On any failure path we return null and the
39-
* caller treats the request as anonymous.
34+
* Resolve the pad-visitor's authorID from the HttpOnly token cookie. This
35+
* mirrors how Etherpad's own socket.io handshake resolves pad-visitor identity:
36+
* the browser sends the `token` cookie (or `<prefix>token`) with every
37+
* same-origin request, and `authorManager.getAuthorId(token, user)` maps the
38+
* token to a stable authorID via the `token2author` DB key.
39+
*
40+
* We do NOT use `req.session.user.author` because Etherpad does not populate
41+
* an authorID into the express-session `user` object for pad visitors, so that
42+
* field is always undefined in production.
43+
*
44+
* On any failure path we return null and the caller treats the request as
45+
* anonymous, resulting in an EMPTY (no-badge) response.
4046
*/
4147
export const resolveRequestAuthor = async (req: any): Promise<string | null> => {
42-
const readAuthor = (): string | null => {
43-
const a = req?.session?.user?.author;
44-
return typeof a === 'string' && a !== '' ? a : null;
45-
};
46-
const fromSession = readAuthor();
47-
if (fromSession !== null) return fromSession;
4848
try {
49-
const expressModule = await import('../express');
50-
const mw = (expressModule as any).sessionMiddleware;
51-
if (typeof mw !== 'function') return null;
52-
await new Promise<void>((resolve, reject) => {
53-
mw(req, {} as any, (err?: unknown) => (err ? reject(err) : resolve()));
54-
});
49+
const cookiePrefix = (settings as any).cookie?.prefix ?? '';
50+
const token = req?.cookies?.[`${cookiePrefix}token`];
51+
if (typeof token !== 'string' || token === '') return null;
52+
const authorManagerMod: any = await import('../../db/AuthorManager');
53+
const authorManager = authorManagerMod.default ?? authorManagerMod;
54+
if (typeof authorManager.getAuthorId !== 'function') return null;
55+
const authorId = await authorManager.getAuthorId(token, req?.session?.user);
56+
return typeof authorId === 'string' && authorId !== '' ? authorId : null;
5557
} catch {
5658
return null;
5759
}
58-
return readAuthor();
5960
};
6061

6162
interface OutdatedResponse {

src/tests/backend-new/specs/hooks/express/updateStatus.test.ts

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,12 @@
55
* (same as production), then exercised via supertest. `loadState` is mocked so
66
* tests control the "latest" version without touching the filesystem.
77
* `PadManager` is mocked so pad-creation doesn't require a running database.
8+
* `AuthorManager` is mocked so token→authorID resolution doesn't require a DB.
89
*
9-
* Session injection: `resolveRequestAuthor` reads `req.session.user.author`
10-
* directly. A simple before-route middleware sets that property on each
11-
* request, keyed by the `X-Test-Author` request header, so individual tests
12-
* can choose which author the request "belongs to" without needing real
13-
* session middleware.
10+
* Author injection: `resolveRequestAuthor` reads the `token` cookie and calls
11+
* `authorManager.getAuthorId(token, user)`. Tests set `sessionAuthor` to
12+
* control which authorID the mock returns for the test token `t.alice`.
13+
* `null` means "anonymous" (getAuthorId returns null / empty for the token).
1414
*/
1515

1616
import {describe, it, expect, vi, beforeAll, beforeEach, afterEach} from 'vitest';
@@ -38,6 +38,14 @@ vi.mock('../../../../../node/updater', () => ({
3838
getDetectedInstallMethod: () => 'git',
3939
}));
4040

41+
// AuthorManager is dynamically imported inside resolveRequestAuthor(). Stubbing
42+
// it here lets tests control token→authorID resolution without a DB.
43+
vi.mock('../../../../../node/db/AuthorManager', () => ({
44+
default: {
45+
getAuthorId: vi.fn(),
46+
},
47+
}));
48+
4149
// PadManager is dynamically imported inside computeOutdated(). Stubbing it
4250
// here lets us control pad existence and author-pool contents without a DB.
4351
vi.mock('../../../../../node/db/PadManager', () => {
@@ -58,6 +66,7 @@ vi.mock('../../../../../node/db/PadManager', () => {
5866
// ---------------------------------------------------------------------------
5967

6068
import * as stateModule from '../../../../../node/updater/state';
69+
import * as authorManagerModule from '../../../../../node/db/AuthorManager';
6170
import {
6271
expressCreateServer,
6372
_resetBadgeCacheForTests,
@@ -112,21 +121,22 @@ let app: Express;
112121
let request: ReturnType<typeof supertest>;
113122

114123
/**
115-
* The author that the fake session middleware will inject into req.session.
116-
* Tests that need a specific author set this before making a request.
117-
* `null` means "anonymous" (no session author).
124+
* The author that `authorManager.getAuthorId` will return for the test token
125+
* `t.alice`. Tests set this before making a request. `null` means "anonymous"
126+
* (getAuthorId returns null/empty, so resolveRequestAuthor returns null).
118127
*/
119128
let sessionAuthor: string | null = null;
120129

130+
/** Fixed test token used in every request. The cookie name has no prefix in tests. */
131+
const TEST_TOKEN = 't.alice';
132+
121133
beforeAll(() => {
122134
app = express();
123135

124-
// Fake session middleware: sets req.session.user.author from our test
125-
// variable, so resolveRequestAuthor() in the route sees the right identity.
136+
// Inject the test token cookie so resolveRequestAuthor() sees it.
137+
// The real cookie-parser middleware is not needed: we set req.cookies directly.
126138
app.use((req: any, _res, next) => {
127-
if (sessionAuthor !== null) {
128-
req.session = {user: {author: sessionAuthor}};
129-
}
139+
req.cookies = {token: TEST_TOKEN};
130140
next();
131141
});
132142

@@ -143,6 +153,13 @@ beforeEach(() => {
143153
sessionAuthor = null;
144154
// Reset the loadState spy so each test controls its own return value.
145155
vi.mocked(stateModule.loadState).mockReset();
156+
// Wire up the AuthorManager mock: return sessionAuthor (or null) for our test token.
157+
vi.mocked((authorManagerModule as any).default.getAuthorId).mockImplementation(
158+
async (token: string) => {
159+
if (token === TEST_TOKEN && sessionAuthor !== null) return sessionAuthor;
160+
return null;
161+
},
162+
);
146163
});
147164

148165
afterEach(() => {

0 commit comments

Comments
 (0)