From 7444026cc3b37a1b720e1a299befdace1a68f874 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 29 Oct 2024 10:12:01 -0400 Subject: [PATCH] Merge pull request #21310 from overleaf/em-validate-tracked-changes Reapply "Sanity check for tracked changes in document-updater" GitOrigin-RevId: e7b38d192f5202006f61bd015bba81d751af5413 --- .../app/js/DocumentManager.js | 8 +- .../document-updater/app/js/RangesManager.js | 99 ++++++++++++++++++- .../DocumentManager/DocumentManagerTests.js | 9 +- .../js/RangesManager/RangesManagerTests.js | 11 ++- 4 files changed, 122 insertions(+), 5 deletions(-) diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 157a99ccf6..2643c64a8d 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/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, diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index 7dfa462ce4..9b8a50c526 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/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 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..ec1c8703e9 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/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 ) })