Pad: Fix `copyPadWithoutHistory` apool corruption bug

CHANGELOG.md

@@ -1,6 +1,8 @@
 # 1.9.0 (not yet released)
 
-### Notable enhancements
+### Notable enhancements and fixes
+
+* Fixed a potential attribute pool corruption bug with `copyPadWithoutHistory`.
 
 #### For plugin authors
 

src/node/db/Pad.js

@@ -492,10 +492,9 @@ Pad.prototype.copyPadWithoutHistory = async function (destinationID, force) {
 
   // initialize the pad with a new line to avoid getting the defaultText
   const newPad = await padManager.getPad(destinationID, '\n');
+  newPad.pool = sourcePad.pool.clone();
 
   const oldAText = this.atext;
-  const newPool = newPad.pool;
-  newPool.fromJsonable(sourcePad.pool.toJsonable()); // copy that sourceId pool to the new pad
 
   // based on Changeset.makeSplice
   const assem = Changeset.smartOpAssembler();

src/static/js/AttributePool.js

@@ -91,6 +91,17 @@ class AttributePool {
     this.nextNum = 0;
   }
 
+  /**
+   * @returns {AttributePool} A deep copy of this attribute pool.
+   */
+  clone() {
+    const c = new AttributePool();
+    for (const [n, a] of Object.entries(this.numToAttrib)) c.numToAttrib[n] = [a[0], a[1]];
+    Object.assign(c.attribToNum, this.attribToNum);
+    c.nextNum = this.nextNum;
+    return c;
+  }
+
   /**
    * Add an attribute to the attribute set, or query for an existing attribute identifier.
    *
@@ -164,7 +175,10 @@ class AttributePool {
 
   /**
    * @returns {Jsonable} An object that can be passed to `fromJsonable` to reconstruct this
-   * attribute pool. The returned object can be converted to JSON.
+   *     attribute pool. The returned object can be converted to JSON. WARNING: The returned object
+   *     has references to internal state (it is not a deep copy). Use the `clone()` method to copy
+   *     a pool -- do NOT do `new AttributePool().fromJsonable(pool.toJsonable())` to copy because
+   *     the resulting shared state will lead to pool corruption.
    */
   toJsonable() {
     return {
@@ -177,7 +191,10 @@ class AttributePool {
    * Replace the contents of this attribute pool with values from a previous call to `toJsonable`.
    *
    * @param {Jsonable} obj - Object returned by `toJsonable` containing the attributes and their
-   *     identifiers.
+   *     identifiers. WARNING: This function takes ownership of the object (it does not make a deep
+   *     copy). Use the `clone()` method to copy a pool -- do NOT do
+   *     `new AttributePool().fromJsonable(pool.toJsonable())` to copy because the resulting shared
+   *     state will lead to pool corruption.
    */
   fromJsonable(obj) {
     this.numToAttrib = obj.numToAttrib;

src/tests/backend/specs/api/pad.js

@@ -9,6 +9,7 @@
 
 const assert = require('assert').strict;
 const common = require('../../common');
+const padManager = require('../../../../node/db/PadManager');
 
 let agent;
 const apiKey = common.apiKey;
@@ -547,6 +548,78 @@ describe(__filename, function () {
         assert.equal(res.body.code, 0);
       });
     });
+
+    // Regression test for https://github.com/ether/etherpad-lite/issues/5296
+    it('source and destination attribute pools are independent', async function () {
+      // Strategy for this test:
+      //   1. Create a new pad without bold or italic text
+      //   2. Use copyPadWithoutHistory to copy the pad.
+      //   3. Add some bold text (but no italic text!) to the source pad. This should add a bold
+      //      attribute to the source pad's pool but not to the destination pad's pool.
+      //   4. Add some italic text (but no bold text!) to the destination pad. This should add an
+      //      italic attribute to the destination pad's pool with the same number as the newly added
+      //      bold attribute in the source pad's pool.
+      //   5. Add some more text (bold or plain) to the source pad. This will save the source pad to
+      //      the database after the destination pad has had an opportunity to corrupt the source
+      //      pad.
+      //   6. Export the source and destination pads. Make sure that <em> doesn't appear in the
+      //      source pad's HTML, and that <strong> doesn't appear int he destination pad's HTML.
+      //   7. Force the server to re-init the pads from the database.
+      //   8. Repeat step 6.
+      // If <em> appears in the source pad, or <strong> appears in the destination pad, then shared
+      // state between the two attribute pools caused corruption.
+
+      const getHtml = async (padId) => {
+        const res = await agent.get(`${endPoint('getHTML')}&padID=${padId}`)
+            .expect(200)
+            .expect('Content-Type', /json/);
+        assert.equal(res.body.code, 0);
+        return res.body.data.html;
+      };
+
+      const setBody = async (padId, bodyHtml) => {
+        await agent.post(endPoint('setHTML'))
+            .send({padID: padId, html: `<!DOCTYPE HTML><html><body>${bodyHtml}</body></html>`})
+            .expect(200)
+            .expect('Content-Type', /json/)
+            .expect((res) => assert.equal(res.body.code, 0));
+      };
+
+      const origHtml = await getHtml(sourcePadId);
+      assert.doesNotMatch(origHtml, /<strong>/);
+      assert.doesNotMatch(origHtml, /<em>/);
+      await agent.get(`${endPoint('copyPadWithoutHistory')}&sourceID=${sourcePadId}` +
+                      `&destinationID=${newPad}&force=false`)
+          .expect(200)
+          .expect('Content-Type', /json/)
+          .expect((res) => assert.equal(res.body.code, 0));
+
+      const newBodySrc = '<strong>bold</strong>';
+      const newBodyDst = '<em>italic</em>';
+      await setBody(sourcePadId, newBodySrc);
+      await setBody(newPad, newBodyDst);
+      await setBody(sourcePadId, `${newBodySrc} foo`);
+
+      let [srcHtml, dstHtml] = await Promise.all([getHtml(sourcePadId), getHtml(newPad)]);
+      assert.match(srcHtml, new RegExp(newBodySrc));
+      assert.match(dstHtml, new RegExp(newBodyDst));
+
+      // Force the server to re-read the pads from the database. This rebuilds the attribute pool
+      // objects from scratch, ensuring that an internally inconsistent attribute pool object did
+      // not cause the above tests to accidentally pass.
+      const reInitPad = async (padId) => {
+        const pad = await padManager.getPad(padId);
+        await pad.init();
+      };
+      await Promise.all([
+        reInitPad(sourcePadId),
+        reInitPad(newPad),
+      ]);
+
+      [srcHtml, dstHtml] = await Promise.all([getHtml(sourcePadId), getHtml(newPad)]);
+      assert.match(srcHtml, new RegExp(newBodySrc));
+      assert.match(dstHtml, new RegExp(newBodyDst));
+    });
   });
 });