fix(pad): outdated notice — resolve author from token cookie (Qodo #7804)

[JohnMcLear]

Summary

Follow-up to #7804 addressing two Qodo review findings on the merged outdated-notice redesign.

🐞 Author lookup always null in production (CRITICAL)

resolveRequestAuthor(req) read req.session.user.author, but Etherpad never populates that field for pad visitors — pad authorship is derived from the HttpOnly token cookie via authorManager.getAuthorId(token, user) (and gated by securityManager.checkAccess for the socket.io handshake).

Result: in production, resolveRequestAuthor always returned null, computeOutdated always returned EMPTY, and the new gritter would never fire.

This PR replaces the helper with a token-cookie-based path that mirrors the same resolution Etherpad's own socket.io handshake uses. The route shape and gating logic are unchanged.

The 9 e2e test cases for /api/version-status are updated to inject req.cookies = {token: TEST_TOKEN} and stub authorManager.getAuthorId. All 9 still pass.

📘 /api/version-status documented in admin OpenAPI spec

The route is public (called from pad_outdated_notice.ts during pad load) but lives in openapi-admin.ts, whose stated purpose is admin endpoints — misleading for tooling consumers.

Fixed by clarifying the admin spec's info.description to acknowledge that the doc may include pad-side endpoints for completeness. A structural split (separate openapi-public.ts) would be larger surgery for limited benefit since the routes share a single registration site.

❌ Not addressed: vulnerable-below removed without deprecation WARN

Qodo flagged this as a rule violation. The removal was an intentional product decision by the maintainer (issue #7799 explicitly says "drop it"). The vulnerable-below directive was an internal release-notes scrape — no plugin or external system depended on it — so a deprecation period adds friction without protecting any consumer. Documented in this PR description rather than in code.

Test plan

• pnpm --filter ep_etherpad-lite exec vitest run tests/backend-new/specs/hooks/express/updateStatus.test.ts — 9/9 green
• pnpm --filter ep_etherpad-lite exec vitest run — 702/703 green (the 1 failure is the unrelated backend-tests-glob.test.ts fresh-clone harness limitation, present on develop)
• tsc --noEmit clean

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Fix pad author resolution from token cookie, clarify OpenAPI scope

🐞 Bug fix 📝 Documentation

Walkthroughs

Description

• Fix critical author resolution bug in production by reading token cookie instead of session.user
  - Replace session-based lookup with authorManager.getAuthorId(token) call
  - Mirrors socket.io handshake identity resolution pattern
• Update test harness to mock AuthorManager and inject token cookie
  - All 9 e2e test cases continue to pass
• Clarify admin OpenAPI spec scope to include pad-side endpoints
  - Document that /api/version-status is public despite living in admin spec

Diagram

flowchart LR
  A["resolveRequestAuthor<br/>reads token cookie"] --> B["authorManager.getAuthorId<br/>token to authorID"]
  B --> C["Returns authorID<br/>or null"]
  D["Test harness<br/>mocks AuthorManager"] --> E["Injects TEST_TOKEN<br/>in req.cookies"]
  E --> F["9 e2e tests<br/>pass"]
  G["OpenAPI spec<br/>info.description"] --> H["Clarifies pad-side<br/>endpoints included"]

Loading

File Changes

1. src/node/hooks/express/updateStatus.ts 🐞 Bug fix +20/-19

Resolve pad author from token cookie via AuthorManager

• Replace session-based author lookup with token-cookie-based resolution
• Read token (or <prefix>token) cookie and call authorManager.getAuthorId(token, user)
• Mirrors socket.io handshake identity resolution pattern used by Etherpad
• Return null on any failure, treating request as anonymous

src/node/hooks/express/updateStatus.ts

---

2. src/tests/backend-new/specs/hooks/express/updateStatus.test.ts 🧪 Tests +30/-13

Update test harness to mock AuthorManager and inject token

• Mock AuthorManager.getAuthorId to control token-to-authorID resolution without database
• Replace session-injection middleware with direct req.cookies.token assignment
• Use fixed test token t.alice for all requests
• Update test harness documentation to reflect new author injection pattern

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

---

3. src/node/hooks/express/openapi-admin.ts 📝 Documentation +4/-1

Clarify admin OpenAPI spec includes pad-side endpoints

• Expand info.description to clarify that admin spec includes non-admin pad-side endpoints
• Document that /api/version-status is included for completeness due to shared route registration
• Prevent downstream tooling from misinterpreting public endpoints as admin-only

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

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

✅ Tickets: 🎫 Inappropriate update message

1. Token cookie not validated 🐞 Bug ⛨ Security

Description

resolveRequestAuthor() forwards any non-empty ${prefix}token cookie value to
AuthorManager.getAuthorId(), which creates/updates token2author:<token> DB mappings for previously
unseen values. Because /api/version-status is a public route and cookies are client-supplied
headers, an attacker can spam requests with unique/invalid tokens to cause unbounded DB growth and
write amplification.

Code

src/node/hooks/express/updateStatus.ts[R49-56]

+    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;

Evidence

The new implementation reads the cookie and calls getAuthorId() without any format validation, and
the endpoint is explicitly public. In contrast, existing server paths validate author-token format
(isValidAuthorToken) before using tokens, and AuthorManager.getAuthorId() will create/update DB
mappings for previously unseen token values.

src/node/hooks/express/updateStatus.ts[47-56]
src/node/hooks/express/updateStatus.ts[135-139]
src/node/db/AuthorManager.ts[117-166]
src/node/db/SecurityManager.ts[112-126]
src/node/utils/ensureAuthorTokenCookie.ts[20-25]
src/static/js/pad_utils.ts[373-386]
src/node/hooks/express.ts[186-202]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`resolveRequestAuthor()` accepts any non-empty token cookie value and passes it to `AuthorManager.getAuthorId()`. `AuthorManager.getAuthorId()` falls back to `token2author` DB mapping creation/updates, so a public caller can force persistent DB writes by sending arbitrary Cookie headers.

## Issue Context
Other parts of the codebase treat author tokens as a strict format (`t.` + base64url) and explicitly reject invalid tokens before calling `getAuthorId()`. This endpoint should mirror that validation to avoid DB pollution / write amplification from attacker-chosen cookie values.

## Fix Focus Areas
- src/node/hooks/express/updateStatus.ts[47-56]
- src/node/hooks/express/updateStatus.ts[135-139]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

1. Token cookie not validated 🐞 Bug ⛨ Security

resolveRequestAuthor() forwards any non-empty ${prefix}token cookie value to
AuthorManager.getAuthorId(), which creates/updates token2author:<token> DB mappings for previously
unseen values. Because /api/version-status is a public route and cookies are client-supplied
headers, an attacker can spam requests with unique/invalid tokens to cause unbounded DB growth and
write amplification.

Agent Prompt

## Issue description
`resolveRequestAuthor()` accepts any non-empty token cookie value and passes it to `AuthorManager.getAuthorId()`. `AuthorManager.getAuthorId()` falls back to `token2author` DB mapping creation/updates, so a public caller can force persistent DB writes by sending arbitrary Cookie headers.

## Issue Context
Other parts of the codebase treat author tokens as a strict format (`t.` + base64url) and explicitly reject invalid tokens before calling `getAuthorId()`. This endpoint should mirror that validation to avoid DB pollution / write amplification from attacker-chosen cookie values.

## Fix Focus Areas
- src/node/hooks/express/updateStatus.ts[47-56]
- src/node/hooks/express/updateStatus.ts[135-139]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools