Merge pull request #19741 from overleaf/jpa-check-filestore-write

[misc] verify file hash when downloading binary file in project-history

GitOrigin-RevId: 0ef56a0753cdfd55fdea921b3555dea48036766b

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

@@ -119,6 +119,7 @@ const ProjectHistoryRedisManager = {
         ts: new Date(),
       },
       version: projectUpdate.version,
+      hash: projectUpdate.hash,
       metadata: projectUpdate.metadata,
       projectHistoryId,
     }

services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js

@@ -188,6 +188,7 @@ describe('ProjectHistoryRedisManager', function () {
           pathname: 'foo.png',
           url,
           version: 42,
+          hash: '1337',
           metadata,
         },
         this.source
@@ -203,6 +204,7 @@ describe('ProjectHistoryRedisManager', function () {
           source: this.source,
         },
         version: 42,
+        hash: '1337',
         metadata,
         projectHistoryId: this.projectHistoryId,
         file: fileId,

services/project-history/app/js/HistoryStoreManager.js

@@ -317,6 +317,12 @@ export function createBlobForUpdate(projectId, historyId, update, callback) {
             if (err) {
               return callback(err)
             }
+            if (update.hash !== fileHash) {
+              logger.warn(
+                { projectId, fileId, webHash: update.hash, fileHash },
+                'hash mismatch between web and project-history'
+              )
+            }
             logger.debug({ fileHash }, 'created blob for file')
             callback(null, { file: fileHash })
           }

services/project-history/app/js/SyncManager.js

@@ -539,6 +539,7 @@ class SyncUpdateExpander {
       } else {
         update.file = entity.file
         update.url = entity.url
+        update.hash = entity._hash
         update.metadata = entity.metadata
       }
 
@@ -627,6 +628,7 @@ class SyncUpdateExpander {
         },
         file: entity.file,
         url: entity.url,
+        hash: entity._hash,
         metadata: entity.metadata,
       }
       this.expandedUpdates.push(addUpdate)

services/project-history/app/js/types.ts

@@ -75,6 +75,7 @@ export type AddFileUpdate = ProjectUpdateBase & {
   pathname: string
   file: string
   url: string
+  hash: string
   metadata?: LinkedFileData
 }
 

services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js

@@ -59,6 +59,11 @@ describe('HistoryStoreManager', function () {
     this.request = sinon.stub()
     this.request.get = sinon.stub()
 
+    this.logger = {
+      debug: sinon.stub(),
+      warn: sinon.stub(),
+    }
+
     this.HistoryStoreManager = await esmock(MODULE_PATH, {
       '@overleaf/fetch-utils': this.FetchUtils,
       request: this.request,
@@ -66,6 +71,7 @@ describe('HistoryStoreManager', function () {
       '../../../../app/js/LocalFileWriter.js': this.LocalFileWriter,
       '../../../../app/js/WebApiManager.js': this.WebApiManager,
       '../../../../app/js/Errors.js': Errors,
+      '@overleaf/logger': this.logger,
     })
   })
 
@@ -385,6 +391,7 @@ describe('HistoryStoreManager', function () {
         this.update = {
           file: true,
           url: `http://filestore.other.cloud.provider/project/${this.projectId}/file/${this.file_id}`,
+          hash: this.hash,
         }
         this.HistoryStoreManager.createBlobForUpdate(
           this.projectId,
@@ -400,6 +407,10 @@ describe('HistoryStoreManager', function () {
         )
       })
 
+      it('should not log any warnings', function () {
+        expect(this.logger.warn).to.not.have.been.called
+      })
+
       it('should request the file from the filestore in settings', function () {
         expect(this.FetchUtils.fetchStream).to.have.been.calledWithMatch(
           `${this.settings.apis.filestore.url}/project/${this.projectId}/file/${this.file_id}`
@@ -418,6 +429,7 @@ describe('HistoryStoreManager', function () {
         this.update = {
           file: true,
           url: `http://filestore.other.cloud.provider/project/${this.invalid_id}/file/${this.file_id}`,
+          hash: this.hash,
         }
         this.HistoryStoreManager.createBlobForUpdate(
           this.projectId,
@@ -434,6 +446,51 @@ describe('HistoryStoreManager', function () {
         expect(this.request.get).to.not.have.been.called
       })
     })
+
+    describe('when the hash mismatches', function () {
+      beforeEach(function (done) {
+        this.file_id = '012345678901234567890123'
+        this.update = {
+          file: true,
+          url: `http://filestore.other.cloud.provider/project/${this.projectId}/file/${this.file_id}`,
+          hash: 'another-hash-from-web',
+        }
+        this.HistoryStoreManager.createBlobForUpdate(
+          this.projectId,
+          this.historyId,
+          this.update,
+          (err, { file: hash }) => {
+            if (err) {
+              return done(err)
+            }
+            this.actualHash = hash
+            done()
+          }
+        )
+      })
+
+      it('should log a warning', function () {
+        expect(this.logger.warn).to.have.been.calledWith(
+          {
+            projectId: this.projectId,
+            fileId: this.file_id,
+            webHash: 'another-hash-from-web',
+            fileHash: this.hash,
+          },
+          'hash mismatch between web and project-history'
+        )
+      })
+
+      it('should request the file from the filestore in settings', function () {
+        expect(this.FetchUtils.fetchStream).to.have.been.calledWithMatch(
+          `${this.settings.apis.filestore.url}/project/${this.projectId}/file/${this.file_id}`
+        )
+      })
+
+      it('should call the callback with the blob', function () {
+        expect(this.actualHash).to.equal(this.hash)
+      })
+    })
   })
 
   describe('getProjectBlob', function () {

services/project-history/test/unit/js/SyncManager/SyncManagerTests.js

@@ -607,6 +607,7 @@ describe('SyncManager', function () {
           path: '2.png',
           file: {},
           url: 'filestore/2.png',
+          _hash: 'hash-42',
         }
         const updates = [
           resyncProjectStructureUpdate(
@@ -627,6 +628,7 @@ describe('SyncManager', function () {
             pathname: newFile.path,
             file: newFile.file,
             url: newFile.url,
+            hash: 'hash-42',
             metadata: undefined,
             meta: {
               resync: true,
@@ -647,6 +649,7 @@ describe('SyncManager', function () {
             importedAt: '2024-07-30T09:14:45.928Z',
             provider: 'references-provider',
           },
+          _hash: 'hash-42',
         }
         const updates = [
           resyncProjectStructureUpdate(
@@ -667,6 +670,7 @@ describe('SyncManager', function () {
             pathname: newFile.path,
             file: newFile.file,
             url: newFile.url,
+            hash: 'hash-42',
             metadata: {
               importedAt: '2024-07-30T09:14:45.928Z',
               provider: 'references-provider',
@@ -750,6 +754,7 @@ describe('SyncManager', function () {
             pathname: fileWichWasADoc.path,
             file: fileWichWasADoc.file,
             url: fileWichWasADoc.url,
+            hash: 'other-hash',
             metadata: undefined,
             meta: {
               resync: true,
@@ -800,6 +805,7 @@ describe('SyncManager', function () {
             pathname: fileWhichWasADoc.path,
             file: fileWhichWasADoc.file,
             url: fileWhichWasADoc.url,
+            hash: 'other-hash',
             metadata: {
               importedAt: '2024-07-30T09:14:45.928Z',
               provider: 'references-provider',
@@ -1026,6 +1032,7 @@ describe('SyncManager', function () {
             pathname: persistedFileWithNewContent.path,
             file: persistedFileWithNewContent.file,
             url: persistedFileWithNewContent.url,
+            hash: 'anotherhashvalue',
             metadata: undefined,
             meta: {
               resync: true,