feat(gdpr): HttpOnly author-token cookie (PR3 of #6701)#7548
Merged
JohnMcLear merged 10 commits intodevelopfrom May 3, 2026
Merged
feat(gdpr): HttpOnly author-token cookie (PR3 of #6701)#7548JohnMcLear merged 10 commits intodevelopfrom
JohnMcLear merged 10 commits intodevelopfrom
Conversation
socket.io handshake doesn't run cookie-parser, so socket.request.cookies is undefined. Parse the Cookie header directly in handleClientReady so the HttpOnly token actually resolves. Playwright spec covers HttpOnly attribute, reload-stability, and context-isolation.
Review Summary by Qodo(Agentic_describe updated until commit 89fde85)Move author-token cookie to server-set HttpOnly (GDPR PR3)
WalkthroughsDescription• Server now mints and sets author-token as HttpOnly cookie on pad routes • Browser JavaScript no longer reads, writes, or generates the token • Socket.io handshake reads token from cookie; legacy message.token supported with deprecation warning • Comprehensive backend and frontend tests validate cookie lifecycle and identity stability Diagramflowchart LR
A["Browser GET /p/:pad"] -->|ensureAuthorTokenCookie| B["Server mints t.* token"]
B -->|Set-Cookie HttpOnly| C["Browser receives cookie"]
C -->|socket.io handshake| D["handleClientReady reads from socket.request.cookies"]
D -->|resolves token| E["Author identity established"]
F["Legacy client sends message.token"] -->|fallback path| D
D -->|warn once| G["Deprecation logged"]
File Changes1. src/node/utils/ensureAuthorTokenCookie.ts
|
Code Review by Qodo
1. /tokenTransfer ignores body.token
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
|
||
|
|
||
| setRouteHandler("/p/:pad", (req: any, res: any, next: Function) => { | ||
| ensureAuthorTokenCookie(req, res, settings); |
There was a problem hiding this comment.
1. No flag for httponly token 📘 Rule violation ☼ Reliability
The new server-minted HttpOnly author-token cookie behavior is enabled unconditionally on pad routes, with no feature flag or default-off toggle. This violates the requirement that new features be gated to reduce rollout risk and allow safe disablement.
Agent Prompt
## Issue description
The new HttpOnly author-token cookie behavior is always enabled, but PR compliance requires new features to be behind a feature flag and disabled by default.
## Issue Context
`ensureAuthorTokenCookie()` is now called on all pad and timeslider routes with no conditional gating, changing behavior immediately for all deployments.
## Fix Focus Areas
- src/node/hooks/express/specialpages.ts[191-191]
- src/node/hooks/express/specialpages.ts[222-222]
- src/node/hooks/express/specialpages.ts[354-354]
- src/node/hooks/express/specialpages.ts[375-375]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This was referenced Apr 19, 2026
Qodo review:
- Timeslider still ran the pre-PR3 JS-cookie path: it read
Cookies.get('${cp}token') (which HttpOnly hides), then generated a
fresh plaintext token and overwrote the server's HttpOnly cookie with
it, and sent token in every socket message. Strip the token read/
write entirely from timeslider.ts and from the outgoing message
shape; the server reads the cookie off the socket.io handshake just
like on /p/:pad.
- tokenTransfer re-issued the author cookie without HttpOnly, undoing
the hardening the first time a user transferred a session. Re-set
it as HttpOnly + Secure (on HTTPS) + SameSite=Lax. Also stop
trusting the body-supplied token on POST: read it off req.cookies
server-side so the client never needs JS access to the token.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SamTV12345
approved these changes
Apr 26, 2026
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
|
Persistent review updated to latest commit 89fde85 |
Comment on lines
+16
to
27
| app.post('/tokenTransfer', async (req: any, res) => { | ||
| // The author token is HttpOnly (ether/etherpad#6701 PR3) so the browser | ||
| // cannot read it. Read it off the request's own cookie jar instead of | ||
| // trusting the request body. The client still supplies non-HttpOnly | ||
| // prefs via body because `prefsHttp` is intentionally JS-readable. | ||
| const cp = settings.cookie.prefix || ''; | ||
| const authorToken: string | undefined = | ||
| req.cookies?.[`${cp}token`] || req.cookies?.token; | ||
| const body = (req.body || {}) as Partial<TokenTransferRequest>; | ||
| if (!authorToken) { | ||
| return res.status(400).send({error: 'No author cookie to transfer'}); | ||
| } |
There was a problem hiding this comment.
1. /tokentransfer ignores body.token 📘 Rule violation ☼ Reliability
The /tokenTransfer POST handler now rejects requests without an author-token cookie and no longer accepts token from the request body, which can break existing clients calling this endpoint. The removed behavior is not deprecated with WARN logging.
Agent Prompt
## Issue description
`POST /tokenTransfer` no longer accepts the author token via `body.token` and hard-requires a cookie (`req.cookies[<prefix>token]`), returning `400` otherwise. This is a breaking change for any existing callers and removes previous behavior without a WARN-based deprecation window.
## Issue Context
For one release, keep backwards compatibility similarly to the CLIENT_READY `message.token` approach: accept `body.token` as a fallback when the cookie is absent, validate it, and log a WARN indicating deprecation and timeline.
## Fix Focus Areas
- src/node/hooks/express/tokenTransfer.ts[16-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+300
to
322
| const cookiePrefix = settings.cookie?.prefix || ''; | ||
| const cookieHeader: string = socket.request?.headers?.cookie || ''; | ||
| const cookieName = `${cookiePrefix}token`; | ||
| const cookieMatch = cookieHeader.split(/;\s*/).find( | ||
| (c) => c.split('=')[0] === cookieName); | ||
| const cookieToken = cookieMatch ? decodeURIComponent(cookieMatch.split('=').slice(1).join('=')) : null; | ||
| const legacyToken = typeof message.token === 'string' ? message.token : null; | ||
| const resolvedToken = cookieToken || legacyToken; | ||
| if (!cookieToken && legacyToken && !thisSession.legacyTokenWarned) { | ||
| messageLogger.warn( | ||
| 'client sent author token via CLIENT_READY message; cookie migration ' + | ||
| 'will take effect on next HTTP response. ' + | ||
| 'See docs/superpowers/specs/2026-04-19-gdpr-pr3-anon-identity-design.md'); | ||
| thisSession.legacyTokenWarned = true; | ||
| } | ||
| // Remember this information since we won't have the cookie in further socket.io messages. This | ||
| // information will be used to check if the sessionId of this connection is still valid since it | ||
| // could have been deleted by the API. | ||
| thisSession.auth = { | ||
| sessionID: message.sessionID, | ||
| padID: message.padId, | ||
| token: message.token, | ||
| token: resolvedToken, | ||
| }; |
There was a problem hiding this comment.
2. Missing token creates shared author 🐞 Bug ⛨ Security
PadMessageHandler can persist a null token into thisSession.auth.token (cookie missing/unparseable and client no longer sends message.token), and SecurityManager.checkAccess() does not deny missing tokens, so AuthorManager.getAuthorId() will map the literal DB key token2author:null and grant access under a shared authorID. This can cause unintended cross-user author identity collisions and can allow tokenless socket connections to be treated as authenticated for public pads.
Agent Prompt
### Issue description
Socket authentication can proceed with `token === null`, which then gets used as a DB key (`token2author:null`) and results in shared/incorrect author identity.
### Issue Context
New clients no longer send `message.token`, so correctness depends on successfully extracting a valid token from the socket.io handshake Cookie header. If the cookie is missing or parsing fails, the current code stores `null` and downstream code does not reject it.
### Fix Focus Areas
- src/node/handler/PadMessageHandler.ts[293-322]
- src/node/db/SecurityManager.ts[112-126]
- src/node/db/AuthorManager.ts[117-127]
- src/static/js/pad.ts[179-212]
### Suggested fix
- In `PadMessageHandler`, after resolving the token, require `typeof resolvedToken === 'string' && padutils.isValidAuthorToken(resolvedToken)` (or equivalent) when there is no valid HTTP API session; otherwise emit `accessStatus: 'deny'` / disconnect with a clear error.
- Defense-in-depth: in `SecurityManager.checkAccess()`, add an explicit deny when `!sessionAuthorID && (token == null || token === '')`.
- Consider using a robust cookie parser and trimming cookie names/values so legitimate cookies don’t accidentally parse as missing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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
HttpOnly; Secure (on HTTPS); SameSite=Lax(orNonewhen cross-site embedded).tokenfield is dropped from the CLIENT_READY message.handleClientReadyparses the token from the socket.io handshake Cookie header (socket.io doesn't run cookie-parser). Legacymessage.tokenis honoured for one release with a one-time WARN per session.Part of the GDPR work tracked in #6701. PR1 #7546 landed deletion controls; PR2 #7547 landed the IP/privacy audit. Remaining PR4 (cookie banner) and PR5 (author erasure) stay in follow-ups.
Design spec:
docs/superpowers/specs/2026-04-19-gdpr-pr3-anon-identity-design.mdImplementation plan:
docs/superpowers/plans/2026-04-19-gdpr-pr3-anon-identity.mdTest plan
pnpm --filter ep_etherpad-lite run ts-check