Merge pull request #21310 from overleaf/em-validate-tracked-changes

Reapply "Sanity check for tracked changes in document-updater"

GitOrigin-RevId: e7b38d192f5202006f61bd015bba81d751af5413

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

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

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

@@ -80,6 +80,13 @@ const RangesManager = {
       }
     }
 
+    sanityCheckTrackedChanges(
+      projectId,
+      docId,
+      rangesTracker.changes,
+      getDocLength(newDocLines)
+    )
+
     if (
       rangesTracker.changes?.length > RangesManager.MAX_CHANGES ||
       rangesTracker.comments?.length > RangesManager.MAX_COMMENTS
@@ -127,11 +134,17 @@ const RangesManager = {
     return { newRanges, rangesWereCollapsed, historyUpdates }
   },
 
-  acceptChanges(changeIds, ranges) {
+  acceptChanges(projectId, docId, changeIds, ranges, lines) {
     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,
+      getDocLength(lines)
+    )
     const newRanges = RangesManager._getRanges(rangesTracker)
     return newRanges
   },
@@ -557,4 +570,88 @@ 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
+ * @param {number} docLength
+ */
+function sanityCheckTrackedChanges(projectId, docId, changes, docLength) {
+  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 ||
+        op.p < 0 ||
+        op.p + op.i.length > docLength
+      ) {
+        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 ||
+        op.p < 0 ||
+        op.p > docLength
+      ) {
+        ok = false
+        badChangeIndex = i
+        break
+      }
+      lastDeletePos = op.p
+      if (lastDeletePos >= docLength) {
+        badChangeIndex = i
+        break
+      }
+    }
+  }
+
+  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

@@ -658,6 +658,7 @@ describe('RangesManager', function () {
           { i: 'amet', p: 40 },
         ]),
       }
+      this.lines = ['lorem xxx', 'ipsum yyy', 'dolor zzz', 'sit wwwww', 'amet']
       this.removeChangeIdsSpy = sinon.spy(
         this.RangesTracker.prototype,
         'removeChangeIds'
@@ -668,8 +669,11 @@ 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
+          this.ranges,
+          this.lines
         )
       })
 
@@ -714,8 +718,11 @@ describe('RangesManager', function () {
           this.ranges.changes[4].id,
         ]
         this.result = this.RangesManager.acceptChanges(
+          this.project_id,
+          this.doc_id,
           this.change_ids,
-          this.ranges
+          this.ranges,
+          this.lines
         )
       })