fix(socketio): don't kick authenticated duplicate-author sessions (#7656)

[JohnMcLear]

Summary

• Skip the userdup kick in CLIENT_READY when the joining socket has req.session.user set; cookie-only sessions keep the existing kick + modal so xxauto_reconnect continues to work.
• Adds backend coverage in tests/backend/specs/socketio.ts: regression test for the cookie-identity kick + new test that asserts authenticated duplicates are not kicked.

Why

Closes #7656. The kick was added when authorIDs were cookie-derived — "same authorID, same pad" reliably meant "stale tab in the same browser." Stable identities (basic auth, SSO, apikey, or any getAuthorId plugin hook) reuse one authorID across windows and devices, so the kick now disconnects legitimate concurrent sessions. The reporter sees this with AUTHENTICATION_METHOD: "apikey" + REQUIRE_AUTHENTICATION: "true".

Test plan

• tests/backend/specs/socketio.ts — Duplicate-author handling describe block, both new cases pass
• Full tests/backend/specs/socketio.ts (35/35 pass)
• Full backend specs (952 pass; 6 pre-existing favicon/robots failures unrelated)
• Manual: open the same pad in two browsers logged in as the same user with requireAuthentication: true — both stay connected
• Manual: open the same pad in two anonymous tabs in the same browser — original tab still gets the userdup modal

[qodo-code-review]

ⓘ 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.

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

Review Summary by Qodo

Skip duplicate-author kick for authenticated sessions

🐞 Bug fix 🧪 Tests

Walkthroughs

Description

• Skip duplicate-author kick for authenticated sessions with stable identities
• Preserve kick behavior for cookie-only sessions to maintain userdup modal
• Add regression test for cookie-identity duplicate handling
• Add test asserting authenticated duplicates are not kicked

Diagram

flowchart LR
  A["CLIENT_READY handler"] --> B{"req.session.user set?"}
  B -->|Yes - Authenticated| C["Allow concurrent sessions"]
  B -->|No - Cookie only| D["Kick duplicate author"]
  C --> E["No userdup disconnect"]
  D --> F["Emit userdup message"]

Loading

File Changes

1. src/node/handler/PadMessageHandler.ts 🐞 Bug fix +19/-12

Conditionally skip duplicate-author kick for authenticated users

• Extract req.session.user before duplicate-author check
• Wrap duplicate-author kick logic in if (user == null) condition
• Add detailed comment explaining why authenticated sessions bypass the kick
• Preserve existing kick behavior for cookie-derived identities

src/node/handler/PadMessageHandler.ts

---

2. src/tests/backend/specs/socketio.ts 🧪 Tests +50/-0

Add duplicate-author handling regression and feature tests

• Add new test suite Duplicate-author handling (#7656) with custom afterEach
• Implement watchDisconnects helper to track disconnect messages
• Add regression test verifying cookie-identity duplicates still trigger userdup kick
• Add test asserting authenticated duplicate sessions are not kicked

src/tests/backend/specs/socketio.ts

---

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

Code Review by Qodo

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

1. Duplicate leave removes active user ✓ Resolved 🐞 Bug ≡ Correctness

Description

With the new logic, authenticated duplicates can stay connected with the same authorId, but
handleDisconnect still broadcasts USER_LEAVE for every socket that disconnects. Because the
client tracks presence keyed by userId (authorId) and deletes that entry on USER_LEAVE,
disconnecting one of multiple same-author sockets makes the author appear offline even though
another socket is still connected.

Code

src/node/handler/PadMessageHandler.ts[R1023-1044]

+  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'});
+      }
   }
 }

Evidence

The PR change allows multiple authenticated sockets with the same sessionInfo.author to remain in
the pad by skipping the userdup kick when req.session.user is set. However, disconnect handling
is still per-socket: it always broadcasts USER_LEAVE with userId: session.author. On the client,
USER_LEAVE deletes userSet[userId] (keyed by authorId), so a disconnect from one socket removes
the shared author entry even if another socket with the same authorId is still connected (including
on the remaining socket itself, because it receives the broadcast).

src/node/handler/PadMessageHandler.ts[1023-1044]
src/node/handler/PadMessageHandler.ts[227-257]
src/static/js/collab_client.ts[250-268]

Agent prompt

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

## Issue description
Authenticated duplicate sessions are now allowed (same authorId, same pad), but the server still emits `USER_LEAVE` on *any* socket disconnect. This makes clients drop the author from presence even if another socket with the same authorId is still connected.
### Issue Context
Client presence is keyed by `userId` (authorId). If multiple sockets share an authorId, the server must only emit `USER_LEAVE` when the *last* socket for that author leaves the pad.
### Fix Focus Areas
- src/node/handler/PadMessageHandler.ts[227-257]
- src/node/handler/PadMessageHandler.ts[1023-1044]
### Implementation notes
- In `handleDisconnect`, before broadcasting `USER_LEAVE`, check `_getRoomSockets(session.padId)` for any remaining socket whose `sessioninfos[roomSocket.id]?.author === session.author`.
- Only broadcast `USER_LEAVE` (and run `userLeave` hook if appropriate) when no remaining sockets exist for that author in that pad.

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

---

2. Flaky fixed-delay socket tests 🐞 Bug ☼ Reliability

Description

The new socket.io tests wait for the userdup message using a fixed 200ms sleep, which can be too
short on slow or loaded CI runners. This can cause intermittent failures even if the behavior is
correct.

Code

src/tests/backend/specs/socketio.ts[R348-373]

+    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, []);

Evidence

Both new test cases rely on setTimeout(..., 200) to “let the kick emit drain” instead of awaiting
a specific socket event or message arrival, which is timing-dependent and prone to CI flakiness.

src/tests/backend/specs/socketio.ts[348-374]
src/tests/backend/common.ts[126-179]

Agent prompt

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

## Issue description
The tests use `await new Promise((r) => setTimeout(r, 200))` to wait for async socket events, which can be flaky.
### Issue Context
The backend test helper already provides `waitForSocketEvent(socket, event, timeoutMs)`.
### Fix Focus Areas
- src/tests/backend/specs/socketio.ts[348-374]
- src/tests/backend/common.ts[126-179]
### Implementation notes
- Replace the fixed sleep with event-driven waiting:
- Either await a `message` event that contains `{disconnect: 'userdup'}` (loop/race with a timeout), or
- Use a Promise that resolves when `watchDisconnects()` observes the expected entry, with a reasonable timeout (e.g., 2–5s).
- This will make the tests deterministic across environments.

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

---