Merge pull request #24390 from overleaf/em-enforce-content-hash-validation

Enforce content hash validation in history

GitOrigin-RevId: 90de21ea86ddc6548001059c41139a2af5b27060

services/history-v1/api/controllers/project_import.js

@@ -22,6 +22,7 @@ const BlobStore = storage.BlobStore
 const chunkStore = storage.chunkStore
 const HashCheckBlobStore = storage.HashCheckBlobStore
 const persistChanges = storage.persistChanges
+const InvalidChangeError = storage.InvalidChangeError
 
 const render = require('./render')
 
@@ -113,7 +114,8 @@ async function importChanges(req, res, next) {
       err instanceof File.NotEditableError ||
       err instanceof FileMap.PathnameError ||
       err instanceof Snapshot.EditMissingFileError ||
-      err instanceof chunkStore.ChunkVersionConflictError
+      err instanceof chunkStore.ChunkVersionConflictError ||
+      err instanceof InvalidChangeError
     ) {
       // If we failed to apply operations, that's probably because they were
       // invalid.

services/history-v1/storage/index.js

@@ -15,3 +15,6 @@ exports.zipStore = require('./lib/zip_store')
 const { BlobStore, loadGlobalBlobs } = require('./lib/blob_store')
 exports.BlobStore = BlobStore
 exports.loadGlobalBlobs = loadGlobalBlobs
+
+const { InvalidChangeError } = require('./lib/errors')
+exports.InvalidChangeError = InvalidChangeError

services/history-v1/storage/lib/persist_changes.js

@@ -81,12 +81,6 @@ async function persistChanges(projectId, allChanges, limits, clientEndVersion) {
   let originalEndVersion
   let changesToPersist
 
-  /**
-   * It's only useful to log validation errors once per flush. When we enforce
-   * content hash validation, it will stop the flush right away anyway.
-   */
-  let validationErrorLogged = false
-
   limits = limits || {}
   _.defaults(limits, {
     changeBucketMinutes: 60,
@@ -131,22 +125,7 @@ async function persistChanges(projectId, allChanges, limits, clientEndVersion) {
       for (const operation of change.iterativelyApplyTo(currentSnapshot, {
         strict: true,
       })) {
-        try {
-          await validateContentHash(operation)
-        } catch (err) {
-          // Temporary: skip validation errors
-          if (err instanceof InvalidChangeError) {
-            if (!validationErrorLogged) {
-              logger.warn(
-                { err, projectId },
-                'content snapshot mismatch (ignored)'
-              )
-              validationErrorLogged = true
-            }
-          } else {
-            throw err
-          }
-        }
+        await validateContentHash(operation)
       }
 
       chunk.pushChanges([change])

services/history-v1/test/acceptance/js/storage/persist_changes.test.js

@@ -213,7 +213,7 @@ describe('persistChanges', function () {
       expect(result.numberOfChangesPersisted).to.equal(1)
     })
 
-    it('acccepts a change with an invalid hash (only logs for now)', async function () {
+    it('rejects a change with an invalid hash', async function () {
       const limitsToPersistImmediately = {
         minChangeTimestamp: farFuture,
         maxChangeTimestamp: farFuture,
@@ -235,13 +235,9 @@ describe('persistChanges', function () {
       )
       const changes = [change]
 
-      const result = await persistChanges(
-        projectId,
-        changes,
-        limitsToPersistImmediately,
-        0
-      )
-      expect(result.numberOfChangesPersisted).to.equal(1)
+      await expect(
+        persistChanges(projectId, changes, limitsToPersistImmediately, 0)
+      ).to.be.rejectedWith(storage.InvalidChangeError)
     })
   })
 })