security(collab): apply queued USER_CHANGES to the enqueue-time pad (cross-pad write TOCTOU)

src/node/handler/PadMessageHandler.ts

@@ -204,7 +204,14 @@ class Channels {
 /**
  * A changeset queue per pad that is processed by handleUserChanges()
  */
-const padChannels = new Channels((ch, {socket, message}) => handleUserChanges(socket, message));
+// The channel key `ch` is the pad id captured at enqueue time (see the enqueue
+// call). Pass it through to handleUserChanges so the write targets the pad that
+// was authorized when the message arrived — NOT whatever pad the mutable
+// sessioninfos[socket.id].padId happens to point at by apply time. A concurrent
+// same-socket CLIENT_READY can swap that padId between enqueue and apply, which
+// otherwise redirects the queued write onto a read-only / unauthorized pad
+// (GHSA-6mcx-x5h6-rpw2).
+const padChannels = new Channels((ch, {socket, message}) => handleUserChanges(socket, message, ch));
 
 /**
  * This Method is called by server.ts to tell the message handler on which socket it should send
@@ -505,6 +512,17 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => {
     throw new Error(`pre-CLIENT_READY message from IP ${ip}: ${msg}`);
   }
 
+  // Pin the pad this message is authorized against — together with `auth` and
+  // the read-only flag — BEFORE any of the awaits below (checkAccess and the
+  // handleMessageSecurity/handleMessage hooks). A concurrent same-socket
+  // CLIENT_READY can mutate sessioninfos[socket.id] (padId/readonly/auth) IN
+  // PLACE during those awaits (the object identity check below does not catch an
+  // in-place mutation). Using these pinned values for the read-only gate, the
+  // queue key and the write keeps them all referring to the SAME pad, closing
+  // the cross-pad write TOCTOU (GHSA-6mcx-x5h6-rpw2).
+  const messagePadId = thisSession.padId;
+  const messageReadonly = thisSession.readonly;
+
   const {session: {user} = {}} = socket.client.request as SocketClientRequest;
   const {accessStatus, authorID} =
       await securityManager.checkAccess(auth.padID, auth.sessionID, auth.token, user);
@@ -526,14 +544,15 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => {
   }
   thisSession.author = authorID;
 
-  // Allow plugins to bypass the readonly message blocker
-  let readOnly = thisSession.readonly;
+  // Allow plugins to bypass the readonly message blocker. Base the decision on
+  // the value pinned above so a concurrent CLIENT_READY can't flip it mid-message.
+  let readOnly = messageReadonly;
   const context = {
     message,
     sessionInfo: {
       authorId: thisSession.author,
-      padId: thisSession.padId,
-      readOnly: thisSession.readonly,
+      padId: messagePadId,
+      readOnly: messageReadonly,
     },
     socket,
     get client() {
@@ -580,7 +599,10 @@ exports.handleMessage = async (socket:any, message: ClientVarMessage) => {
           switch (type) {
             case 'USER_CHANGES':
               stats.counter('pendingEdits').inc();
-              await padChannels.enqueue(thisSession.padId, {socket, message});
+              // Queue key = the pinned pad, NOT the (possibly-swapped) live
+              // session padId. This value is forwarded to handleUserChanges as
+              // the write target (see the padChannels executor).
+              await padChannels.enqueue(messagePadId, {socket, message});
               break;
             case 'PAD_DELETE': await handlePadDelete(socket, message.data as unknown as PadDeleteMessage); break;
             case 'USERINFO_UPDATE': await handleUserInfoUpdate(socket, message as unknown as UserNewInfoMessage); break;
@@ -823,7 +845,7 @@ const handleUserInfoUpdate = async (socket:any, {data: {userInfo: {name, colorId
  */
 const handleUserChanges = async (socket:any, message: {
   data: ClientUserChangesMessage
-}) => {
+}, authorizedPadId: string) => {
   // This one's no longer pending, as we're gonna process it now
   stats.counter('pendingEdits').dec();
 
@@ -850,7 +872,9 @@ const handleUserChanges = async (socket:any, message: {
     if (apool == null) throw new Error('missing apool');
     if (changeset == null) throw new Error('missing changeset');
     const wireApool = (new AttributePool()).fromJsonable(apool);
-    const pad = await padManager.getPad(thisSession.padId, null, thisSession.author);
+    // Use the pad id captured at enqueue time, not the (mutable) session padId,
+    // which a concurrent CLIENT_READY may have swapped (GHSA-6mcx-x5h6-rpw2).
+    const pad = await padManager.getPad(authorizedPadId, null, thisSession.author);
 
     // Verify that the changeset has valid syntax and is in canonical form
     checkRep(changeset);
@@ -1004,7 +1028,7 @@ const handleUserChanges = async (socket:any, message: {
     socket.emit('message', {disconnect: 'badChangeset'});
     stats.meter('failedChangesets').mark();
     messageLogger.warn(`Failed to apply USER_CHANGES from author ${thisSession.author} ` +
-                       `(socket ${socket.id}) on pad ${thisSession.padId}: ${err.stack || err}`);
+                       `(socket ${socket.id}) on pad ${authorizedPadId}: ${err.stack || err}`);
   } finally {
     stopWatch.end();
   }

src/tests/backend/specs/crossPadWriteToctou.ts

@@ -0,0 +1,92 @@
+'use strict';
+
+/**
+ * Regression for GHSA-6mcx-x5h6-rpw2 (PadMessageHandler cross-pad write TOCTOU).
+ *
+ * A USER_CHANGES message is authorized (access + read-only gate) against the pad
+ * the session points at when the message arrives, but the write ran later off
+ * mutable session state. A concurrent same-socket CLIENT_READY could swap
+ * sessioninfos[socket.id].padId during the awaits in handleMessage(), redirecting
+ * the queued write onto a read-only / unauthorized pad.
+ *
+ * This test drives a real USER_CHANGES over a socket and, via a
+ * handleMessageSecurity hook (which runs during those awaits), swaps the
+ * session's padId to a victim pad — exactly the concurrent-CLIENT_READY race.
+ * The change must still land on the pad the message was authorized against, and
+ * never on the victim.
+ */
+
+import {MapArrayType} from '../../../node/types/MapType';
+
+const assert = require('assert').strict;
+const common = require('../common');
+const padManager = require('../../../node/db/PadManager');
+const plugins = require('../../../static/js/pluginfw/plugin_defs');
+const padMessageHandler = require('../../../node/handler/PadMessageHandler');
+
+describe(__filename, function () {
+  this.timeout(30000);
+  let agent: any;
+  const backups: MapArrayType<any> = {};
+
+  before(async function () { agent = await common.init(); });
+
+  beforeEach(function () {
+    backups.hooks = {handleMessageSecurity: plugins.hooks.handleMessageSecurity};
+    plugins.hooks.handleMessageSecurity = [];
+  });
+  afterEach(function () {
+    Object.assign(plugins.hooks, backups.hooks);
+  });
+
+  it('a mid-message pad swap cannot redirect the write onto another pad', async function () {
+    // Authorized (writable) pad the client is editing.
+    const authorizedId = `toctou_ok_${common.randomString()}`;
+    const authorizedPad = await padManager.getPad(authorizedId, 'seed\n');
+    await authorizedPad.setText('seed\n');
+    // Victim pad with identical text, so the changeset is valid against either
+    // pad — the test distinguishes purely by which pad receives the write.
+    const victimId = `toctou_victim_${common.randomString()}`;
+    const victimPad = await padManager.getPad(victimId, 'seed\n');
+    await victimPad.setText('seed\n');
+
+    const res = await agent.get(`/p/${authorizedId}`).expect(200);
+    const socket = await common.connect(res);
+    try {
+      const {data: clientVars} = await common.handshake(socket, authorizedId);
+      const rev = clientVars.collab_client_vars.rev;
+      const authorId = clientVars.userId;
+
+      // Simulate the concurrent same-socket CLIENT_READY: while THIS USER_CHANGES
+      // is mid-flight (handleMessageSecurity runs during handleMessage's awaits),
+      // swap the session's padId to the victim pad in place.
+      plugins.hooks.handleMessageSecurity = [{
+        hook_fn: (hookName: string, ctx: any) => {
+          if (ctx.message?.data?.type === 'USER_CHANGES') {
+            padMessageHandler.sessioninfos[ctx.socket.id].padId = victimId;
+          }
+        },
+        hook_fn_name: 'toctou/handleMessageSecurity',
+        hook_name: 'handleMessageSecurity',
+        part: {plugin: 'toctou'},
+      }];
+
+      const accept = common.waitForSocketEvent(socket, 'message');
+      await common.sendUserChanges(socket, {
+        baseRev: rev,
+        changeset: 'Z:5>2*0+2$ZZ',
+        apool: {numToAttrib: {0: ['author', authorId]}, nextNum: 1},
+      });
+      await accept; // wait for the server to finish applying
+
+      assert.equal(victimPad.text(), 'seed\n',
+          'the victim pad (swapped session padId) must NOT be modified');
+      assert.equal(authorizedPad.text(), 'ZZseed\n',
+          'the change must land on the pad the message was authorized against');
+    } finally {
+      socket.close();
+      await authorizedPad.remove();
+      await victimPad.remove();
+    }
+  });
+});