Merge branch 'master' into develop

CHANGELOG.md

@@ -80,6 +80,22 @@
     generator function.
   * `newOp()`: Deprecated in favor of the new `Op` class.
 
+# 1.8.17
+
+Released: 2022-02-23
+
+### Security fixes
+
+* Fixed a vunlerability in the `CHANGESET_REQ` message handler that allowed a
+  user with any access to read any pad if the pad ID is known.
+
+### Notable enhancements and fixes
+
+* Fixed a bug that caused all pad edit messages received at the server to go
+  through a single queue. Now there is a separate queue per pad as intended,
+  which should reduce message processing latency when many pads are active at
+  the same time.
+
 # 1.8.16
 
 ### Security fixes

src/node/handler/PadMessageHandler.js

@@ -241,6 +241,14 @@ exports.handleMessage = async (socket, message) => {
     thisSession.readonly =
         padIds.readonly || !webaccess.userCanModify(thisSession.auth.padID, socket.client.request);
   }
+  // Outside of the checks done by this function, message.padId must not be accessed because it is
+  // too easy to introduce a security vulnerability that allows malicious users to read or modify
+  // pads that they should not be able to access. Code should instead use
+  // sessioninfos[socket.id].padId if the real pad ID is needed or
+  // sessioninfos[socket.id].auth.padID if the original user-supplied pad ID is needed.
+  Object.defineProperty(message, 'padId', {get: () => {
+    throw new Error('message.padId must not be accessed (for security reasons)');
+  }});
 
   const auth = thisSession.auth;
   if (!auth) {
@@ -330,7 +338,7 @@ exports.handleMessage = async (socket, message) => {
       messageLogger.warn('Dropped message, COLLABROOM for readonly pad');
     } else if (message.data.type === 'USER_CHANGES') {
       stats.counter('pendingEdits').inc();
-      await padChannels.enqueue(message.padId, {socket, message});
+      await padChannels.enqueue(thisSession.padId, {socket, message});
     } else if (message.data.type === 'USERINFO_UPDATE') {
       await handleUserInfoUpdate(socket, message);
     } else if (message.data.type === 'CHAT_MESSAGE') {
@@ -1121,11 +1129,6 @@ const handleChangesetRequest = async (socket, message) => {
     return;
   }
 
-  if (message.padId == null) {
-    messageLogger.warn('Dropped message, changeset request has no padId!');
-    return;
-  }
-
   if (message.data.granularity == null) {
     messageLogger.warn('Dropped message, changeset request has no granularity!');
     return;
@@ -1151,16 +1154,16 @@ const handleChangesetRequest = async (socket, message) => {
   const start = message.data.start;
   const end = start + (100 * granularity);
 
-  const padIds = await readOnlyManager.getIds(message.padId);
+  const {padId} = sessioninfos[socket.id];
 
   // build the requested rough changesets and send them back
   try {
-    const data = await getChangesetInfo(padIds.padId, start, end, granularity);
+    const data = await getChangesetInfo(padId, start, end, granularity);
     data.requestID = message.data.requestID;
     socket.json.send({type: 'CHANGESET_REQ', data});
   } catch (err) {
     messageLogger.error(`Error while handling a changeset request ${message.data} ` +
-                        `for ${padIds.padId}: ${err.stack || err}`);
+                        `for ${padId}: ${err.stack || err}`);
   }
 };
 

src/tests/backend/specs/messages.js

@@ -25,7 +25,7 @@ describe(__filename, function () {
     plugins.hooks.handleMessageSecurity = [];
     padId = common.randomString();
     assert(!await padManager.doesPadExist(padId));
-    pad = await padManager.getPad(padId, 'dummy text');
+    pad = await padManager.getPad(padId, 'dummy text\n');
     await pad.setText('\n'); // Make sure the pad is created.
     assert.equal(pad.text(), '\n');
     let res = await agent.get(`/p/${padId}`).expect(200);
@@ -50,6 +50,35 @@ describe(__filename, function () {
     pad = null;
   });
 
+  describe('CHANGESET_REQ', function () {
+    it('users are unable to read changesets from other pads', async function () {
+      const otherPadId = `${padId}other`;
+      assert(!await padManager.doesPadExist(otherPadId));
+      const otherPad = await padManager.getPad(otherPadId, 'other text\n');
+      try {
+        await otherPad.setText('other text\n');
+        const resP = common.waitForSocketEvent(roSocket, 'message');
+        await common.sendMessage(roSocket, {
+          component: 'pad',
+          padId: otherPadId, // The server should ignore this.
+          type: 'CHANGESET_REQ',
+          data: {
+            granularity: 1,
+            start: 0,
+            requestID: 'requestId',
+          },
+        });
+        const res = await resP;
+        assert.equal(res.type, 'CHANGESET_REQ');
+        assert.equal(res.data.requestID, 'requestId');
+        // Should match padId's text, not otherPadId's text.
+        assert.match(res.data.forwardsChangesets[0], /^[^$]*\$dummy text\n/);
+      } finally {
+        await otherPad.remove();
+      }
+    });
+  });
+
   describe('USER_CHANGES', function () {
     const sendUserChanges =
         async (socket, cs) => await common.sendUserChanges(socket, {baseRev: rev, changeset: cs});