fix(pad): redesign outdated-version notice (#7799)#7804
Merged
Conversation
Per-pad first-author gating, dismissable gritter, minor-or-more rule, drop vulnerable UI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
12 bite-sized tasks, TDD-first where applicable; closes the spec end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds isMinorOrMoreBehind(current, latest) which returns true only when the latest release is at least one minor version ahead (patch-only deltas return false). Removes isMajorBehind, parseVulnerableBelow, and isVulnerable from versionCompare.ts — callers in updateStatus.ts, VersionChecker.ts, and index.ts will be updated in subsequent tasks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove VulnerableBelowDirective type, UpdateState.vulnerableBelow field, and all related scraping/checking logic (parseVulnerableBelow, isVulnerable imports). Clean up Notifier, OpenAPI schema, and all test fixtures to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove `vulnerableAt` and `vulnerableNewReleaseTag` from the `EmailSendLog` interface, `EMPTY_STATE`, and the `isValidEmail` validator — these backed the removed `vulnerable`/`vulnerable-new-release` email kinds and are now dead code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Export firstAuthorOf() from updateStatus.ts — finds the lowest-numbered author attrib in a pad's pool, skipping empty-string placeholders. Covered by 6 vitest cases in tests/backend-new/specs/hooks/express/. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace global badge cache with a per-(padId, authorId) LRU cache. The
new response shape is {outdated: 'minor' | null, isFirstAuthor: boolean};
the old 'severe'/'vulnerable' enum is dropped entirely. computeOutdated
now resolves the pad's first author and compares it against the session
author before returning outdated:'minor', so the notice is only shown to
the person who created the pad.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… behind Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add the /api/version-status GET operation to the admin OpenAPI spec with the new pad-aware response shape: outdated enum reduced to [minor]|null, isFirstAuthor boolean, and an optional padId query param. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Renames pad_version_badge.ts → pad_outdated_notice.ts and rewrites it as a fire-and-forget gritter notice that only shows when the API reports outdated=minor AND the current user is the pad's first author. Wires the new maybeShowOutdatedNotice() call into pad.ts immediately after showPrivacyBannerIfEnabled(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Six Playwright specs exercise maybeShowOutdatedNotice: null response, isFirstAuthor:false guard, positive appearance + text, X-dismiss, 500 server error tolerance, and 8 s auto-fade. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Delete the GET /api/version-status describe block from the legacy mocha spec (asserted outdated:null and outdated:'severe' — both no longer match the new response shape). The new vitest spec at tests/backend-new/specs/hooks/express/updateStatus.test.ts covers this surface comprehensively. Delete src/tests/frontend-new/specs/pad-version-badge.spec.ts entirely: all three tests reference the #version-badge DOM element removed in Task 8 and stub 'severe'/'vulnerable' enum values that no longer exist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…docs - Remove OutdatedLevel type (null|'severe') from types.ts — no consumers remain after the badge redesign removed the severe tier. - Fix Notifier severe-email body: was "more than one major release behind" but isSevere now fires on minor-or-more, so update to "at least one minor release behind the latest published version". - Drop "vulnerability directives" from the /admin/update/status OpenAPI description; replace with the actual response fields. - Remove stale vulnerableBelow field from UpdateStatusPayload in admin/src/store/store.ts — server no longer sends it. - Fix docs/admin/updates.md: "pad-side badge" → "pad-side notice". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Review Summary by QodoRedesign outdated-version notice from persistent banner to dismissable first-author-only gritter
WalkthroughsDescription• Redesigned the outdated-version notice from a persistent bottom-right banner to a dismissable
gritter toast notification
• Changed visibility to first-author-only (pad creator) instead of showing to all visitors
• Updated version comparison logic from major-behind to minor-or-more-behind, excluding patch-only
updates
• Rewrote /api/version-status endpoint to be pad-aware with optional ?padId query parameter
• Added per-(padId, authorId) LRU cache (1000 entries, 60s TTL) with single-flight pattern for
concurrent misses
• Updated response schema from {outdated: enum} to `{outdated: 'minor'|null, isFirstAuthor:
boolean}`
• Removed all vulnerability-related code: vulnerable enum values, vulnerable-below directive
scraping, vulnerableBelow state field, and vulnerable email kinds
• Simplified isSevere notifier signal to fire on minor-or-more-behind instead of major-behind
• Added comprehensive test coverage: 18 semver comparison cases, 6 first-author extraction cases, 9
endpoint e2e cases, 6 frontend Playwright cases
• Updated documentation (AsciiDoc, Markdown, admin guide) and CHANGELOG to reflect new behavior
• Deleted obsolete pad_version_badge.ts and related tests
Diagramflowchart LR
A["Persistent<br/>Version Badge"] -->|Redesign| B["Dismissable<br/>Gritter Toast"]
C["All Visitors"] -->|Gating| D["First Author Only"]
E["Major-Behind<br/>Logic"] -->|Update| F["Minor-or-More<br/>Behind Logic"]
G["Simple Boolean<br/>Cache"] -->|Replace| H["Per-padId-authorId<br/>LRU Cache"]
I["Vulnerable<br/>Email Logic"] -->|Remove| J["Simplified<br/>Notifier"]
B --> K["8s Auto-fade<br/>+ X-Dismiss"]
D --> K
F --> K
H --> K
File Changes1. src/tests/backend-new/specs/hooks/express/updateStatus.test.ts
|
Code Review by Qodo
Context used✅ Tickets:
🎫 Inappropriate update message 1. vulnerable-below removed without deprecation
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+61
to
100
| interface OutdatedResponse { | ||
| outdated: 'minor' | null; | ||
| isFirstAuthor: boolean; | ||
| } | ||
|
|
||
| const EMPTY: OutdatedResponse = {outdated: null, isFirstAuthor: false}; | ||
|
|
||
| const TTL_MS = 60 * 1000; | ||
| let cache = new LRUCache<string, {value: OutdatedResponse; at: number}>({max: 1000}); | ||
| const inFlight = new Map<string, Promise<OutdatedResponse>>(); | ||
|
|
||
| /** Test-only setter: rebuild the LRU with a smaller cap so eviction can be asserted. */ | ||
| export const _setBadgeCacheCapForTests = (max: number): void => { | ||
| cache = new LRUCache<string, {value: OutdatedResponse; at: number}>({max}); | ||
| }; | ||
|
|
||
| /** Test-only: clear the in-memory badge cache so integration tests see fresh state. */ | ||
| export const _resetBadgeCacheForTests = (): void => { | ||
| badgeCache = {value: null, at: 0}; | ||
| badgeInFlight = null; | ||
| cache.clear(); | ||
| inFlight.clear(); | ||
| }; | ||
|
|
||
| const computeOutdated = async ( | ||
| padId: string | null, | ||
| authorId: string | null, | ||
| ): Promise<OutdatedResponse> => { | ||
| const state = await loadState(stateFilePath()); | ||
| if (!state.latest) return EMPTY; | ||
| const current = getEpVersion(); | ||
| if (!isMinorOrMoreBehind(current, state.latest.version)) return EMPTY; | ||
| if (!padId || !authorId) return EMPTY; | ||
| // padManager is loaded via dynamic import to avoid circular-init w/ updater. | ||
| const padManagerMod: any = await import('../../db/PadManager'); | ||
| const padManager = padManagerMod.default ?? padManagerMod; | ||
| if (typeof padManager.isValidPadId !== 'function' || !padManager.isValidPadId(padId)) return EMPTY; | ||
| if (!(await padManager.doesPadExist(padId))) return EMPTY; | ||
| const pad = await padManager.getPad(padId); | ||
| if (firstAuthorOf(pad) !== authorId) return EMPTY; | ||
| return {outdated: 'minor', isFirstAuthor: true}; | ||
| }; |
There was a problem hiding this comment.
1. vulnerable-below removed without deprecation 📘 Rule violation ☼ Reliability
This PR removes the vulnerable-below directive parsing and the severe/vulnerable outdated signals without any deprecation-period WARN log, which can break deployments and workflows that relied on those signals. Per policy, the feature should emit a WARN and remain supported until the next version before being removed.
Agent Prompt
## Issue description
The PR removes the `vulnerable-below` functionality and the `severe`/`vulnerable` signals immediately, without emitting a deprecation WARN and without a deprecation period.
## Issue Context
- `CHANGELOG.md` explicitly states that `vulnerable-below`, `severe`, and `vulnerable` were removed.
- The `/api/version-status` implementation now only supports `{outdated: 'minor' | null, ...}`.
- The release-notes directive scraping that powered vulnerability signalling has been removed.
## Fix Focus Areas
- src/node/hooks/express/updateStatus.ts[61-100]
- src/node/updater/VersionChecker.ts[14-75]
- src/node/updater/Notifier.ts[13-64]
- src/node/updater/index.ts[159-165]
- doc/admin/updates.md[52-76]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+41
to
+59
| export const resolveRequestAuthor = async (req: any): Promise<string | null> => { | ||
| 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<void>((resolve, reject) => { | ||
| mw(req, {} as any, (err?: unknown) => (err ? reject(err) : resolve())); | ||
| }); | ||
| } catch { | ||
| return null; | ||
| } | ||
| return readAuthor(); | ||
| }; |
There was a problem hiding this comment.
2. Author lookup always null 🐞 Bug ≡ Correctness
resolveRequestAuthor() reads req.session.user.author, but Etherpad does not populate an authorId into the express-session user object for pad visitors, so /api/version-status will return the EMPTY response and the outdated notice will never show. In Etherpad, the pad author identity is resolved from the HttpOnly token/sessionID cookies via securityManager.checkAccess(), not from express-session.
Agent Prompt
## Issue description
`GET /api/version-status` currently derives the requester identity from `req.session.user.author`, but Etherpad’s pad-user authorID is not stored on the express-session `user` object. This causes `authorId` to be `null` in real pad requests, making `computeOutdated()` always return `EMPTY` and preventing the pad-side outdated notice from ever firing.
## Issue Context
Pad authorship is determined via the HttpOnly `${prefix}token` (and optionally `${prefix}sessionID`) cookies and enforced via `securityManager.checkAccess(...)` during the socket.io handshake.
## Fix Focus Areas
- src/node/hooks/express/updateStatus.ts[33-100]
- Replace `resolveRequestAuthor()` + `req.session.user.author` usage with a cookie-based resolution.
- Recommended approach: in the route handler or `computeOutdated()`, call `securityManager.checkAccess(padId, sessionIDCookie, tokenCookie, req.session?.user)` and use the returned `authorID` when `accessStatus === 'grant'`.
- Read cookies from `req.cookies` using `settings.cookie.prefix` (mirrors `ensureAuthorTokenCookie`).
- Keep the “quiet” behavior: on any failure/deny, return `EMPTY` (don’t throw).
- Update cache key to incorporate the resolved `authorID` (or the token if you prefer), not `req.session.user.author`.
- src/tests/backend-new/specs/hooks/express/updateStatus.test.ts[1-315]
- Adjust tests to match the new identity mechanism (e.g., mock `securityManager.checkAccess()` or inject token/sessionID cookies and mock AuthorManager).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
3 tasks
Member
Author
|
Qodo follow-up — addressed in #7805:
|
JohnMcLear
added a commit
that referenced
this pull request
May 18, 2026
) (#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #7799.
Replaces the persistent, all-visitor "Etherpad on this server is severely outdated" banner with a dismissable gritter toast, shown only to a pad's first author (pool position 0 in the attribute pool), only when the running server is at least one minor version behind the latest published release.
Behaviour changes
#version-badgebottom-right banner is gone. A gritter (auto-fades after 8s, X-dismissable) takes its place.3.0.1 → 3.0.2) no longer fire.updates.tier = 'off'remains the full kill-switch and is unchanged.API changes
GET /api/version-statusnow takes an optional?padId=<id>query parameter and returns{outdated: 'minor' | null, isFirstAuthor: boolean}.severeandvulnerableenum values, thevulnerable-belowdirective scraping, thevulnerableBelowstate field, and thevulnerable/vulnerable-new-releaseemail kinds have all been removed.isSevere(the Notifier signal that triggers the "outdated" admin email) now fires on minor-or-more-behind, matching the new gritter signal. The email copy is updated to match.Implementation notes
resolveRequestAuthor) and only computes the per-pad first-author match when bothstate.latestexists AND the current version is minor-or-more behind. ThepadIdis validated viapadManager.isValidPadIdbefore any DB call.(padId, authorId)for 60s in a 1000-entry LRU with single-flight on misses.Design spec + implementation plan committed under
docs/superpowers/specs/docs/superpowers/plans.Test plan
backend-tests-glob.test.tswhich expects../node_modules/ep_*/static/tests/backend/specsto match — a fresh-clone harness limitation, present on develop)tests/backend-new/specs/updater/versionCompare.test.ts(semver helper, 18 cases),tests/backend-new/specs/hooks/express/firstAuthorOf.test.ts(6 cases),tests/backend-new/specs/hooks/express/updateStatus.test.ts(9 cases including LRU eviction, single-flight, first-author gating)src/tests/frontend-new/specs/outdated_notice.spec.ts(6 cases incl. auto-fade, X-dismiss, server-500, client-sideisFirstAuthor=falseguard)tsc --noEmitcleanvar/update.state.jsonpinned to a higherlatest.version— verify gritter appears once for the pad's first author, absent for second visitor🤖 Generated with Claude Code