From e4efe121daf7290ae32e2b1df31c5a58caaf972e Mon Sep 17 00:00:00 2001
From: Jakob Ackermann <jakob.ackermann@overleaf.com>
Date: Tue, 21 Mar 2023 13:18:16 +0000
Subject: [PATCH] Merge pull request #12229 from
 overleaf/jpa-reject-decrement-doc-version

[docstore] reject doc updates that decrement the doc version

GitOrigin-RevId: 533cc5ece8d5684c85b2f63fa2a093c68f6b5877
---
 services/docstore/app.js                      |  2 +
 services/docstore/app/js/DocManager.js        | 10 +++++
 services/docstore/app/js/Errors.js            |  3 ++
 .../test/acceptance/js/UpdatingDocsTests.js   | 37 +++++++++++++++++++
 .../docstore/test/unit/js/DocManagerTests.js  | 20 ++++++++++
 5 files changed, 72 insertions(+)

diff --git a/services/docstore/app.js b/services/docstore/app.js
index b9aa78908a..8cde97ca27 100644
--- a/services/docstore/app.js
+++ b/services/docstore/app.js
@@ -91,6 +91,8 @@ app.use(function (error, req, res, next) {
     return res.sendStatus(404)
   } else if (error instanceof Errors.DocModifiedError) {
     return res.sendStatus(409)
+  } else if (error instanceof Errors.DocVersionDecrementedError) {
+    return res.sendStatus(409)
   } else {
     return res.status(500).send('Oops, something went wrong')
   }
diff --git a/services/docstore/app/js/DocManager.js b/services/docstore/app/js/DocManager.js
index cdf09cd733..8c28a10ae8 100644
--- a/services/docstore/app/js/DocManager.js
+++ b/services/docstore/app/js/DocManager.js
@@ -263,6 +263,16 @@ module.exports = DocManager = {
           updateVersion = true
           updateRanges = true
         } else {
+          if (doc.version > version) {
+            // Reject update when the version was decremented.
+            // Potential reasons: racing flush, broken history.
+            return callback(
+              new Errors.DocVersionDecrementedError('rejecting stale update', {
+                updateVersion: version,
+                flushedVersion: doc.version,
+              })
+            )
+          }
           updateLines = !_.isEqual(doc.lines, lines)
           updateVersion = doc.version !== version
           updateRanges = RangeManager.shouldUpdateRanges(doc.ranges, ranges)
diff --git a/services/docstore/app/js/Errors.js b/services/docstore/app/js/Errors.js
index 580ce9df5d..bbdbe75c08 100644
--- a/services/docstore/app/js/Errors.js
+++ b/services/docstore/app/js/Errors.js
@@ -8,9 +8,12 @@ class DocModifiedError extends OError {}
 
 class DocRevValueError extends OError {}
 
+class DocVersionDecrementedError extends OError {}
+
 module.exports = {
   Md5MismatchError,
   DocModifiedError,
   DocRevValueError,
+  DocVersionDecrementedError,
   ...Errors,
 }
diff --git a/services/docstore/test/acceptance/js/UpdatingDocsTests.js b/services/docstore/test/acceptance/js/UpdatingDocsTests.js
index 6ffc564a7d..db631db83b 100644
--- a/services/docstore/test/acceptance/js/UpdatingDocsTests.js
+++ b/services/docstore/test/acceptance/js/UpdatingDocsTests.js
@@ -179,6 +179,43 @@ describe('Applying updates to a doc', function () {
     })
   })
 
+  describe('when the version was decremented', function () {
+    beforeEach(function (done) {
+      DocstoreClient.updateDoc(
+        this.project_id,
+        this.doc_id,
+        this.newLines,
+        this.version - 1,
+        this.newRanges,
+        (error, res, body) => {
+          if (error) return done(error)
+          this.res = res
+          this.body = body
+          done()
+        }
+      )
+    })
+
+    it('should return 409', function () {
+      this.res.statusCode.should.equal(409)
+    })
+
+    it('should not update the doc in the API', function (done) {
+      DocstoreClient.getDoc(
+        this.project_id,
+        this.doc_id,
+        {},
+        (error, res, doc) => {
+          if (error) return done(error)
+          doc.lines.should.deep.equal(this.originalLines)
+          doc.version.should.equal(this.version)
+          doc.ranges.should.deep.equal(this.originalRanges)
+          done()
+        }
+      )
+    })
+  })
+
   describe('when the ranges have changed', function () {
     beforeEach(function (done) {
       return DocstoreClient.updateDoc(
diff --git a/services/docstore/test/unit/js/DocManagerTests.js b/services/docstore/test/unit/js/DocManagerTests.js
index ac85ea9593..abf8afb03c 100644
--- a/services/docstore/test/unit/js/DocManagerTests.js
+++ b/services/docstore/test/unit/js/DocManagerTests.js
@@ -796,6 +796,26 @@ describe('DocManager', function () {
       })
     })
 
+    describe('when the version was decremented', function () {
+      beforeEach(function () {
+        this.DocManager._getDoc = sinon.stub().yields(null, this.doc)
+        this.DocManager.updateDoc(
+          this.project_id,
+          this.doc_id,
+          this.newDocLines,
+          this.version - 1,
+          this.originalRanges,
+          this.callback
+        )
+      })
+
+      it('should return an error', function () {
+        this.callback.should.have.been.calledWith(
+          sinon.match.instanceOf(Errors.DocVersionDecrementedError)
+        )
+      })
+    })
+
     describe('when the doc lines have not changed', function () {
       beforeEach(function () {
         this.DocManager._getDoc = sinon.stub().callsArgWith(3, null, this.doc)