From dc445121fed1b5c1f2f50b961c10d6500c025ba5 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Mon, 7 Oct 2024 08:37:55 -0400 Subject: [PATCH] Merge pull request #20834 from overleaf/em-validate-tracked-deletes Sanity check for tracked changes in document-updater GitOrigin-RevId: 5f4c5cb931a6dae5257fed2f21e40777cb466309 --- .../app/js/DocumentManager.js | 7 +- .../document-updater/app/js/RangesManager.js | 72 ++++++++++++++++++- .../DocumentManager/DocumentManagerTests.js | 9 ++- .../js/RangesManager/RangesManagerTests.js | 4 ++ 4 files changed, 89 insertions(+), 3 deletions(-) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 157a99ccf6..149a63bcc9 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/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, diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index 7dfa462ce4..96cca7ff95 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/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 diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index a0c379a5be..75b7209b75 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/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) }) }) diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index 67ac6cc175..a7125219bb 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/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 )