From 39d7ae6e8f909f5f1baa6e39190b917603711bbe Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 5 May 2026 17:07:08 +0100 Subject: [PATCH 1/2] fix(socketio): don't kick authenticated duplicate-author sessions (#7656) The CLIENT_READY handler kicks any prior socket whose authorID matches the joining socket's, originally as a workaround for stale tabs in the same browser (cookie-derived authorIDs were per-browser, so "same authorID, same pad" reliably meant "page refresh / second tab in this browser"). With stable identities (basic auth, SSO, apikey, getAuthorId hook) the same authorID can legitimately appear across windows or devices, so the kick disconnects real concurrent sessions. Skip the kick when the joining socket has req.session.user set; cookie-only sessions keep the existing behavior so the userdup modal and the xxauto_reconnect path still work. --- src/node/handler/PadMessageHandler.ts | 31 ++++++++++------- src/tests/backend/specs/socketio.ts | 50 +++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index 5fe717615b9..bab19b5a94a 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -1020,22 +1020,29 @@ const handleClientReady = async (socket:any, message: ClientReadyMessage) => { // Check if the user has disconnected during any of the above awaits. if (sessionInfo !== sessioninfos[socket.id]) throw new Error('client disconnected'); - // Check if this author is already on the pad, if yes, kick the other sessions! - const roomSockets = _getRoomSockets(pad.id); + const {session: {user} = {}} = socket.client.request as SocketClientRequest; - for (const otherSocket of roomSockets) { - // The user shouldn't have joined the room yet, but check anyway just in case. - if (otherSocket.id === socket.id) continue; - const sinfo = sessioninfos[otherSocket.id]; - if (sinfo && sinfo.author === sessionInfo.author) { - // fix user's counter, works on page refresh or if user closes browser window and then rejoins - sessioninfos[otherSocket.id] = {}; - otherSocket.leave(sessionInfo.padId); - otherSocket.emit('message', {disconnect: 'userdup'}); + // The duplicate-author kick exists because cookie-derived authorIDs are + // per-browser, so "same authorID, same pad" historically meant "stale tab in + // the same browser" — see #7656. Authenticated sessions (req.session.user + // set, e.g. via basic auth, SSO, or a getAuthorId plugin hook) carry a + // stable identity across windows and devices, so concurrent same-author + // sessions are legitimate and must not be kicked. + const roomSockets = _getRoomSockets(pad.id); + if (user == null) { + for (const otherSocket of roomSockets) { + // The user shouldn't have joined the room yet, but check anyway just in case. + if (otherSocket.id === socket.id) continue; + const sinfo = sessioninfos[otherSocket.id]; + if (sinfo && sinfo.author === sessionInfo.author) { + // fix user's counter, works on page refresh or if user closes browser window and then rejoins + sessioninfos[otherSocket.id] = {}; + otherSocket.leave(sessionInfo.padId); + otherSocket.emit('message', {disconnect: 'userdup'}); + } } } - const {session: {user} = {}} = socket.client.request as SocketClientRequest; /* eslint-disable prefer-template -- it doesn't support breaking across multiple lines */ accessLogger.info(`[${pad.head > 0 ? 'ENTER' : 'CREATE'}]` + ` pad:${sessionInfo.padId}` + diff --git a/src/tests/backend/specs/socketio.ts b/src/tests/backend/specs/socketio.ts index b235974aa71..c4525223532 100644 --- a/src/tests/backend/specs/socketio.ts +++ b/src/tests/backend/specs/socketio.ts @@ -324,6 +324,56 @@ describe(__filename, function () { }); }); + describe('Duplicate-author handling (#7656)', function () { + let socketA: any; + let socketB: any; + + afterEach(async function () { + for (const s of [socketA, socketB]) if (s) s.close(); + socketA = null; + socketB = null; + // The outer afterEach only knows about the singleton `socket`. Null it so + // it doesn't try to close one of ours twice. + socket = null; + }); + + // Records `{disconnect: ...}` payloads delivered to a socket so a test can + // assert whether the userdup kick fired. + const watchDisconnects = (s: any): string[] => { + const seen: string[] = []; + s.on('message', (msg: any) => { if (msg && msg.disconnect) seen.push(msg.disconnect); }); + return seen; + }; + + it('cookie identity: same-author second socket kicks the first (regression)', async function () { + const res = await agent.get('/p/pad').expect(200); + socketA = await common.connect(res); + assert.equal((await common.handshake(socketA, 'pad')).type, 'CLIENT_VARS'); + const seen = watchDisconnects(socketA); + + // Same cookie => same author token => same authorID. This is the original + // "stale tab in the same browser" case the kick was designed for. + socketB = await common.connect(res); + assert.equal((await common.handshake(socketB, 'pad')).type, 'CLIENT_VARS'); + // Let the kick emit drain. + await new Promise((r) => setTimeout(r, 200)); + assert.deepEqual(seen, ['userdup']); + }); + + it('authenticated identity: second socket does NOT kick the first', async function () { + settings.requireAuthentication = true; + const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); + socketA = await common.connect(res); + assert.equal((await common.handshake(socketA, 'pad')).type, 'CLIENT_VARS'); + const seen = watchDisconnects(socketA); + + socketB = await common.connect(res); + assert.equal((await common.handshake(socketB, 'pad')).type, 'CLIENT_VARS'); + await new Promise((r) => setTimeout(r, 200)); + assert.deepEqual(seen, []); + }); + }); + describe('SocketIORouter.js', function () { const Module = class { setSocketIO(io:any) {} From ce368cfa60d4ebdc6dc34dd50e9123cb528bc748 Mon Sep 17 00:00:00 2001 From: John McLear Date: Tue, 5 May 2026 18:03:37 +0100 Subject: [PATCH 2/2] fix(socketio): suppress USER_LEAVE when other same-author sockets remain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the duplicate-author kick disabled for authenticated sessions, a single authorID can legitimately span multiple sockets in one pad. handleDisconnect was emitting USER_LEAVE on every socket close, which made clients (whose presence is keyed by authorID) drop the author entirely even when another socket of theirs was still online. Only broadcast USER_LEAVE — and only run the userLeave hook — when the disconnecting socket is the last one in the pad for that author. Adds two backend tests: - authenticated identity: closing one of two same-author sockets does NOT emit USER_LEAVE on the other. - different authors (regression): closing socket A still emits USER_LEAVE for socket B. Action of Qodo review feedback on PR #7678. --- src/node/handler/PadMessageHandler.ts | 40 +++++++++++------- src/tests/backend/specs/socketio.ts | 58 +++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 15 deletions(-) diff --git a/src/node/handler/PadMessageHandler.ts b/src/node/handler/PadMessageHandler.ts index bab19b5a94a..5a0a4c8544b 100644 --- a/src/node/handler/PadMessageHandler.ts +++ b/src/node/handler/PadMessageHandler.ts @@ -239,22 +239,32 @@ exports.handleDisconnect = async (socket:any) => { ` authorID:${session.author}` + (user && user.username ? ` username:${user.username}` : '')); /* eslint-enable prefer-template */ - socket.broadcast.to(session.padId).emit('message', { - type: 'COLLABROOM', - data: { - type: 'USER_LEAVE', - userInfo: { - colorId: await authorManager.getAuthorColorId(session.author), - userId: session.author, + // Client presence is keyed by authorID. With the #7656 fix, multiple sockets + // can share an authorID (same authenticated identity across windows/devices), + // so emitting USER_LEAVE on every socket disconnect would drop the author + // from presence even when another socket of theirs is still connected. Only + // broadcast — and only run the userLeave hook — when the *last* socket for + // this author leaves the pad. + const isLastSocketForAuthor = !_getRoomSockets(session.padId).some( + (s: any) => sessioninfos[s.id]?.author === session.author); + if (isLastSocketForAuthor) { + socket.broadcast.to(session.padId).emit('message', { + type: 'COLLABROOM', + data: { + type: 'USER_LEAVE', + userInfo: { + colorId: await authorManager.getAuthorColorId(session.author), + userId: session.author, + }, }, - }, - }); - await hooks.aCallAll('userLeave', { - ...session, // For backwards compatibility. - authorId: session.author, - readOnly: session.readonly, - socket, - }); + }); + await hooks.aCallAll('userLeave', { + ...session, // For backwards compatibility. + authorId: session.author, + readOnly: session.readonly, + socket, + }); + } }; diff --git a/src/tests/backend/specs/socketio.ts b/src/tests/backend/specs/socketio.ts index c4525223532..fa7a7dd2723 100644 --- a/src/tests/backend/specs/socketio.ts +++ b/src/tests/backend/specs/socketio.ts @@ -372,6 +372,64 @@ describe(__filename, function () { await new Promise((r) => setTimeout(r, 200)); assert.deepEqual(seen, []); }); + + // Records USER_LEAVE userIds so a test can assert whether other clients + // were told the author went offline. + const watchUserLeaves = (s: any): string[] => { + const seen: string[] = []; + s.on('message', (msg: any) => { + if (msg?.type === 'COLLABROOM' && msg?.data?.type === 'USER_LEAVE') { + seen.push(msg.data.userInfo?.userId); + } + }); + return seen; + }; + + it('authenticated identity: closing one socket does NOT remove the author for the other', async function () { + // Two authenticated sockets sharing one identity (same authorID). When + // socketA closes, presence-key clients keyed on authorID would drop the + // author entirely if USER_LEAVE were broadcast — but socketB is still + // online. The server must only emit USER_LEAVE when the *last* socket + // for that author leaves. + settings.requireAuthentication = true; + const res = await agent.get('/p/pad').auth('user', 'user-password').expect(200); + socketA = await common.connect(res); + const cvA: any = await common.handshake(socketA, 'pad'); + const authorIdA = cvA.data.userId; + socketB = await common.connect(res); + const cvB: any = await common.handshake(socketB, 'pad'); + assert.equal(cvB.data.userId, authorIdA, 'precondition: same author'); + + const leaves = watchUserLeaves(socketB); + socketA.close(); + socketA = null; + // Give the server a beat to broadcast. + await new Promise((r) => setTimeout(r, 300)); + assert.deepEqual(leaves, [], + 'remaining same-author socket should not see USER_LEAVE for itself'); + }); + + it('different authors: closing one socket DOES emit USER_LEAVE for the other (regression)', async function () { + // socketA and socketB are different anonymous browsers (separate cookie + // jars => separate authorIDs). Closing socketA must still tell socketB + // that authorA left. + const supertest = require('supertest'); + const browserA = supertest(common.baseUrl); + const browserB = supertest(common.baseUrl); + const resA = await browserA.get('/p/pad').expect(200); + const resB = await browserB.get('/p/pad').expect(200); + socketA = await common.connect(resA); + const cvA: any = await common.handshake(socketA, 'pad'); + socketB = await common.connect(resB); + const cvB: any = await common.handshake(socketB, 'pad'); + assert.notEqual(cvA.data.userId, cvB.data.userId, 'precondition: different authors'); + + const leaves = watchUserLeaves(socketB); + socketA.close(); + socketA = null; + await new Promise((r) => setTimeout(r, 300)); + assert.deepEqual(leaves, [cvA.data.userId]); + }); }); describe('SocketIORouter.js', function () {