Merge pull request #20834 from overleaf/em-validate-tracked-deletes

Sanity check for tracked changes in document-updater

GitOrigin-RevId: 5f4c5cb931a6dae5257fed2f21e40777cb466309

services/document-updater/app/js/DocumentManager.js

@@ -264,7 +264,12 @@ const DocumentManager = {
       throw new Errors.NotFoundError(`document not found: ${docId}`)
     }
 
-    const newRanges = RangesManager.acceptChanges(changeIds, ranges)
+    const newRanges = RangesManager.acceptChanges(
+      projectId,
+      docId,
+      changeIds,
+      ranges
+    )
 
     await RedisManager.promises.updateDocument(
       projectId,

services/document-updater/app/js/RangesManager.js

@@ -80,6 +80,8 @@ const RangesManager = {
       }
     }
 
+    sanityCheckTrackedChanges(projectId, docId, rangesTracker.changes)
+
     if (
       rangesTracker.changes?.length > RangesManager.MAX_CHANGES ||
       rangesTracker.comments?.length > RangesManager.MAX_COMMENTS
@@ -127,11 +129,12 @@ const RangesManager = {
     return { newRanges, rangesWereCollapsed, historyUpdates }
   },
 
-  acceptChanges(changeIds, ranges) {
+  acceptChanges(projectId, docId, changeIds, ranges) {
     const { changes, comments } = ranges
     logger.debug(`accepting ${changeIds.length} changes in ranges`)
     const rangesTracker = new RangesTracker(changes, comments)
     rangesTracker.removeChangeIds(changeIds)
+    sanityCheckTrackedChanges(projectId, docId, rangesTracker.changes)
     const newRanges = RangesManager._getRanges(rangesTracker)
     return newRanges
   },
@@ -557,4 +560,71 @@ function getCroppedCommentOps(op, comments) {
   return historyCommentOps
 }
 
+/**
+ * Check some tracked changes assumptions:
+ *
+ * - Tracked changes can't be empty
+ * - Tracked inserts can't overlap with another tracked change
+ * - There can't be two tracked deletes at the same position
+ * - Ranges should be ordered by position, deletes before inserts
+ *
+ * If any assumption isn't upheld, log a warning.
+ *
+ * @param {string} projectId
+ * @param {string} docId
+ * @param {TrackedChange[]} changes
+ */
+function sanityCheckTrackedChanges(projectId, docId, changes) {
+  let lastDeletePos = -1 // allow a tracked delete at position 0
+  let lastInsertEnd = 0
+  let ok = true
+  let badChangeIndex
+  for (let i = 0; i < changes.length; i++) {
+    const change = changes[i]
+
+    const op = change.op
+    if ('i' in op) {
+      if (op.i.length === 0 || op.p < lastDeletePos || op.p < lastInsertEnd) {
+        ok = false
+        badChangeIndex = i
+        break
+      }
+      lastInsertEnd = op.p + op.i.length
+    } else if ('d' in op) {
+      if (op.d.length === 0 || op.p <= lastDeletePos || op.p < lastInsertEnd) {
+        ok = false
+        badChangeIndex = i
+        break
+      }
+      lastDeletePos = op.p
+    }
+  }
+
+  if (ok) {
+    return
+  }
+
+  const changeRanges = []
+  for (const change of changes) {
+    if ('i' in change.op) {
+      changeRanges.push({
+        id: change.id,
+        p: change.op.p,
+        i: change.op.i.length,
+      })
+    } else if ('d' in change.op) {
+      changeRanges.push({
+        id: change.id,
+        p: change.op.p,
+        d: change.op.d.length,
+      })
+    }
+  }
+
+  logger.warn(
+    { projectId, docId, changes: changeRanges, badChangeIndex },
+    'Malformed tracked changes detected'
+  )
+}
+
 module.exports = RangesManager

services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js

@@ -691,6 +691,8 @@ describe('DocumentManager', function () {
 
       it('should apply the accept change to the ranges', function () {
         this.RangesManager.acceptChanges.should.have.been.calledWith(
+          this.project_id,
+          this.doc_id,
           [this.change_id],
           this.ranges
         )
@@ -722,7 +724,12 @@ describe('DocumentManager', function () {
 
       it('should apply the accept change to the ranges', function () {
         this.RangesManager.acceptChanges
-          .calledWith(this.change_ids, this.ranges)
+          .calledWith(
+            this.project_id,
+            this.doc_id,
+            this.change_ids,
+            this.ranges
+          )
           .should.equal(true)
       })
     })

services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js

@@ -668,6 +668,8 @@ describe('RangesManager', function () {
       beforeEach(function () {
         this.change_ids = [this.ranges.changes[1].id]
         this.result = this.RangesManager.acceptChanges(
+          this.project_id,
+          this.doc_id,
           this.change_ids,
           this.ranges
         )
@@ -714,6 +716,8 @@ describe('RangesManager', function () {
           this.ranges.changes[4].id,
         ]
         this.result = this.RangesManager.acceptChanges(
+          this.project_id,
+          this.doc_id,
           this.change_ids,
           this.ranges
         )