From 21e1ae2fa3b83a4860af2f37484e5693b672988e Mon Sep 17 00:00:00 2001 From: John McLear Date: Fri, 15 May 2026 19:44:55 +0100 Subject: [PATCH] security: allow integrator sessionID cookie to be HttpOnly (#7045) (#7755) * security: allow integrator sessionID cookie to be HttpOnly (#7045) The integrator-set sessionID cookie was forced to be non-HttpOnly because Etherpad's own client JS read it via document.cookie and forwarded it in the socket.io CLIENT_READY payload, exposing it to XSS. Mirror the GDPR PR3 author-token migration: read sessionID from the socket.io handshake's Cookie header in PadMessageHandler.handleClientReady, falling back to the legacy message-level field with a one-time deprecation warning per socket. Drop the client-side Cookies.get('sessionID') reads in pad.ts and timeslider.ts so the field is no longer sent by current clients. Existing integrators that set sessionID without HttpOnly keep working unchanged; the field on the message becomes optional and integrators should now mark the cookie HttpOnly; Secure; SameSite=Lax. Closes #7045 Co-Authored-By: Claude Opus 4.7 (1M context) * fix(security): treat undecodable handshake cookies as absent (Qodo #7755) decodeURIComponent() throws URIError on malformed values like `%ZZ`. The unguarded call in PadMessageHandler.handleClientReady's readCookie() let a single bad cookie abort CLIENT_READY for that socket, allowing unauthenticated peers to spam server error logs and lock themselves out of pads. Catch URIError and treat the value as absent so the legacy message-level field still serves as a fallback. Other error classes still propagate. Add a backend test that asserts a `sessionID=%ZZ` cookie no longer aborts the handshake. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- doc/cookies.md | 2 +- src/node/handler/PadMessageHandler.ts | 50 ++++++++-- src/static/js/pad.ts | 10 +- src/static/js/timeslider.ts | 4 +- src/static/js/types/SocketIOMessage.ts | 12 ++- src/tests/backend/specs/sessionIdCookie.ts | 106 +++++++++++++++++++++ 6 files changed, 166 insertions(+), 18 deletions(-) create mode 100644 src/tests/backend/specs/sessionIdCookie.ts diff --git a/doc/cookies.md b/doc/cookies.md index ce6eac222..c2dbfd268 100644 --- a/doc/cookies.md +++ b/doc/cookies.md @@ -15,4 +15,4 @@ Etherpad HTTP API clients may make use (if they choose so) to send another cooki | Name | Sample value | Domain | Usage description | |-----------|------------------------------------|-------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| sessionID | s.1c70968b333b25476a2c7bdd0e0bed17 | example.org | Sessions can be created between a group and an author. This allows an author to access more than one group. The sessionID will be set as a cookie to the client and is valid until a certain date. The session cookie can also contain multiple comma-separated sessionIDs, allowing a user to edit pads in different groups at the same time. More info - https://github.com/ether/etherpad-lite/blob/develop/doc/api/http_api.md#session | +| sessionID | s.1c70968b333b25476a2c7bdd0e0bed17 | example.org | Sessions can be created between a group and an author. This allows an author to access more than one group. The sessionID is set as a cookie by the integrator and is valid until a certain date. The session cookie can also contain multiple comma-separated sessionIDs, allowing a user to edit pads in different groups at the same time. Since [#7045](https://github.com/ether/etherpad/issues/7045) Etherpad reads this cookie server-side from the socket.io handshake, so integrators **should** set it as `HttpOnly; Secure; SameSite=Lax` to mitigate XSS. More info - https://github.com/ether/etherpad-lite/blob/develop/doc/api/http_api.md#session | diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index 8e4cd8f2b..66d1cdbe6 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -85,9 +85,12 @@ exports.socketio = () => { * - auth: Object with the following properties copied from the client's CLIENT_READY message: * - padID: Pad ID requested by the user. Unlike the padId property described below, this * may be a read-only pad ID. - * - sessionID: Copied from the client's sessionID cookie, which should be the value - * returned from the createSession() HTTP API. This will be null/undefined if - * createSession() isn't used or the portal doesn't set the sessionID cookie. + * - sessionID: The value returned from the createSession() HTTP API, normally set as + * the `sessionID` cookie by the integrator. Read from the socket.io handshake's + * Cookie header (so the cookie can be HttpOnly — issue #7045) and falls back to a + * deprecated `sessionID` field on the CLIENT_READY message for legacy clients. + * This will be null/undefined if createSession() isn't used or the integrator + * doesn't set the sessionID cookie. * - token: User-supplied token. * - author: The user's author ID. * - padId: The real (not read-only) ID of the pad. @@ -395,12 +398,31 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => { // once so the migration is visible in logs. The socket.io handshake does // not run cookie-parser, so pull the cookie directly from the Cookie // header. + // + // The same applies to the integrator-set `sessionID` cookie (issue #7045): + // historically the client read it from `document.cookie`, which forced the + // cookie to be non-HttpOnly and exposed it to XSS. Now we read it from the + // handshake Cookie header so integrators can set it `HttpOnly`. 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 readCookie = (name: string): string | null => { + const match = cookieHeader.split(/;\s*/).find( + (c) => c.split('=')[0] === name); + if (!match) return null; + const raw = match.split('=').slice(1).join('='); + // A malformed value (e.g. `name=%ZZ`) makes decodeURIComponent throw + // URIError. Without this guard a single bad cookie aborts CLIENT_READY, + // letting an unauthenticated peer spam server error logs and block + // itself from joining (flagged by Qodo on #7755). Treat undecodable + // values as absent. + try { + return decodeURIComponent(raw); + } catch (err) { + if (err instanceof URIError) return null; + throw err; + } + }; + const cookieToken = readCookie(`${cookiePrefix}token`); const legacyToken = typeof message.token === 'string' ? message.token : null; const resolvedToken = cookieToken || legacyToken; if (!cookieToken && legacyToken && !thisSession.legacyTokenWarned) { @@ -410,11 +432,23 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => { 'See docs/superpowers/specs/2026-04-19-gdpr-pr3-anon-identity-design.md'); thisSession.legacyTokenWarned = true; } + const cookieSessionID = + readCookie(`${cookiePrefix}sessionID`) || readCookie('sessionID'); + const legacySessionID = + typeof message.sessionID === 'string' ? message.sessionID : null; + const resolvedSessionID = cookieSessionID || legacySessionID; + if (!cookieSessionID && legacySessionID && !thisSession.legacySessionIdWarned) { + messageLogger.warn( + 'client sent sessionID via CLIENT_READY message; integrators should ' + + 'set the sessionID cookie as HttpOnly (issue #7045). The in-message ' + + 'field is deprecated and will be removed in a future release.'); + thisSession.legacySessionIdWarned = 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, + sessionID: resolvedSessionID, padID: message.padId, token: resolvedToken, }; diff --git a/src/static/js/pad.ts b/src/static/js/pad.ts index 3b4e7d58a..8a8360d55 100644 --- a/src/static/js/pad.ts +++ b/src/static/js/pad.ts @@ -305,10 +305,10 @@ const sendClientReady = (isReconnect) => { document.title = `${padId.replace(/_+/g, ' ')} | ${title}`; } - const cp = (window as any).clientVars?.cookiePrefix || ''; // The author token lives in an HttpOnly cookie set by the server (GDPR PR3 / - // ether/etherpad#6701). The browser never reads or writes it; the server - // reads the cookie from the socket.io handshake inside handleClientReady. + // ether/etherpad#6701). The integrator-set `sessionID` cookie can also be + // HttpOnly now (issue #7045). The browser never reads or writes either; the + // server reads them from the socket.io handshake inside handleClientReady. // If known, propagate the display name and color to the server in the CLIENT_READY message. This // allows the server to include the values in its reply CLIENT_VARS message (which avoids @@ -320,11 +320,13 @@ const sendClientReady = (isReconnect) => { name: params.get('userName'), }; + // The integrator-set `sessionID` cookie is read server-side from the + // socket.io handshake (issue #7045) so it can be HttpOnly. We no longer + // forward it via the CLIENT_READY payload. const msg: any = { component: 'pad', type: 'CLIENT_READY', padId, - sessionID: Cookies.get(`${cp}sessionID`) || Cookies.get('sessionID'), userInfo, }; const overrides = getMyViewOverrides(); diff --git a/src/static/js/timeslider.ts b/src/static/js/timeslider.ts index 5b2f1e79a..dacc93b93 100644 --- a/src/static/js/timeslider.ts +++ b/src/static/js/timeslider.ts @@ -157,13 +157,15 @@ const init = () => { }; // sends a message over the socket +// The integrator-set `sessionID` cookie is consumed server-side from the +// socket.io handshake (issue #7045). It does not need to ride on every +// message; the server only reads it during CLIENT_READY. const sendSocketMsg = (type, data) => { socket.emit("message", { component: 'pad', // FIXME: Remove this stupidity! type, data, padId, - sessionID: Cookies.get(`${cp}sessionID`) || Cookies.get('sessionID'), }); }; diff --git a/src/static/js/types/SocketIOMessage.ts b/src/static/js/types/SocketIOMessage.ts index e1c01a7ae..914c5c472 100644 --- a/src/static/js/types/SocketIOMessage.ts +++ b/src/static/js/types/SocketIOMessage.ts @@ -190,8 +190,10 @@ export type ClientReadyMessage = { type: 'CLIENT_READY', component: string, padId: string, - sessionID: string, - token: string, + /** @deprecated since #7045 — read server-side from the HttpOnly cookie. */ + sessionID?: string, + /** @deprecated since GDPR PR3 — read server-side from the HttpOnly cookie. */ + token?: string, userInfo: UserInfo, padSettingsDefaults?: PadOption, reconnect?: boolean @@ -329,8 +331,10 @@ export type SocketClientReadyMessage = { type: string component: string padId: string - sessionID: string - token: string + /** @deprecated since #7045 — read server-side from the HttpOnly cookie. */ + sessionID?: string + /** @deprecated since GDPR PR3 — read server-side from the HttpOnly cookie. */ + token?: string userInfo: { colorId: string|null name: string|null diff --git a/src/tests/backend/specs/sessionIdCookie.ts b/src/tests/backend/specs/sessionIdCookie.ts new file mode 100644 index 000000000..0928daf45 --- /dev/null +++ b/src/tests/backend/specs/sessionIdCookie.ts @@ -0,0 +1,106 @@ +'use strict'; + +/** + * Regression test for https://github.com/ether/etherpad/issues/7045. + * + * Before the fix, Etherpad's client-side JavaScript read the integrator-set + * `sessionID` cookie via `document.cookie` and forwarded it in the socket.io + * CLIENT_READY payload. That forced integrators to mark the cookie as + * non-HttpOnly, exposing it to XSS. + * + * The fix moves the read to the server: `PadMessageHandler.handleClientReady` + * now pulls `sessionID` out of the socket.io handshake's `Cookie` header so + * integrators can mark the cookie `HttpOnly; Secure; SameSite=Lax`. + * + * The legacy message-level `sessionID` field is still accepted as a one-release + * fallback, with a one-time warning per socket. + */ + +const assert = require('assert').strict; +const common = require('../common'); +const padManager = require('../../../node/db/PadManager'); +const {sessioninfos} = require('../../../node/handler/PadMessageHandler'); +import settings from '../../../node/utils/Settings'; +const io = require('socket.io-client'); + +const cookiePrefix = () => settings.cookie?.prefix || ''; + +describe(__filename, function () { + this.timeout(30000); + let socket: any; + + before(async function () { await common.init(); }); + + beforeEach(async function () { + assert(socket == null); + }); + + afterEach(async function () { + if (socket) socket.close(); + socket = null; + if (await padManager.doesPadExist('pad')) { + const pad = await padManager.getPad('pad'); + await pad.remove(); + } + }); + + const connectWithCookie = async (cookieHeader: string) => { + const s = io(`${common.baseUrl}/`, { + forceNew: true, + query: {cookie: cookieHeader, padId: 'pad'}, + }); + await common.waitForSocketEvent(s, 'connect', 5000); + return s; + }; + + const sendClientReady = async (s: any, message: any) => { + s.emit('message', { + component: 'pad', + type: 'CLIENT_READY', + padId: 'pad', + ...message, + }); + const reply: any = await common.waitForSocketEvent(s, 'message', 5000); + assert.equal(reply.type, 'CLIENT_VARS'); + }; + + it('reads sessionID from the handshake Cookie header', async function () { + socket = await connectWithCookie('sessionID=s.aaaaaaaaaaaaaaaa'); + await sendClientReady(socket, {}); + assert.equal(sessioninfos[socket.id].auth.sessionID, 's.aaaaaaaaaaaaaaaa'); + }); + + it('honours the configured cookie prefix', async function () { + socket = await connectWithCookie(`${cookiePrefix()}sessionID=s.bbbbbbbbbbbbbbbb`); + await sendClientReady(socket, {}); + assert.equal(sessioninfos[socket.id].auth.sessionID, 's.bbbbbbbbbbbbbbbb'); + }); + + it('falls back to message.sessionID for legacy clients (no cookie)', async function () { + socket = await connectWithCookie(''); + await sendClientReady(socket, {sessionID: 's.cccccccccccccccc'}); + assert.equal(sessioninfos[socket.id].auth.sessionID, 's.cccccccccccccccc'); + }); + + it('prefers the cookie over the legacy message field', async function () { + socket = await connectWithCookie('sessionID=s.dddddddddddddddd'); + await sendClientReady(socket, {sessionID: 's.eeeeeeeeeeeeeeee'}); + assert.equal(sessioninfos[socket.id].auth.sessionID, 's.dddddddddddddddd'); + }); + + it('records null when no sessionID is provided', async function () { + socket = await connectWithCookie(''); + await sendClientReady(socket, {}); + assert.equal(sessioninfos[socket.id].auth.sessionID, null); + }); + + it('treats a malformed (undecodable) cookie as absent rather than aborting', async function () { + // %ZZ is not a valid percent-encoded sequence; decodeURIComponent() throws + // URIError. Without the guard this would tear down CLIENT_READY and let + // any client log-spam the server (Qodo bug on #7755). The handshake must + // still complete and fall through to the message-field fallback. + socket = await connectWithCookie('sessionID=%ZZ'); + await sendClientReady(socket, {sessionID: 's.ffffffffffffffff'}); + assert.equal(sessioninfos[socket.id].auth.sessionID, 's.ffffffffffffffff'); + }); +});