From 41acc2326e06e477c240fbe67ae6a0767346ddbf Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Mon, 5 Aug 2024 10:52:40 +0200 Subject: [PATCH] Merge pull request #19741 from overleaf/jpa-check-filestore-write [misc] verify file hash when downloading binary file in project-history GitOrigin-RevId: 0ef56a0753cdfd55fdea921b3555dea48036766b --- .../app/js/ProjectHistoryRedisManager.js | 1 + .../ProjectHistoryRedisManagerTests.js | 2 + .../app/js/HistoryStoreManager.js | 6 ++ .../project-history/app/js/SyncManager.js | 2 + services/project-history/app/js/types.ts | 1 + .../HistoryStoreManagerTests.js | 57 +++++++++++++++++++ .../unit/js/SyncManager/SyncManagerTests.js | 7 +++ 7 files changed, 76 insertions(+) diff --git a/services/document-updater/app/js/ProjectHistoryRedisManager.js b/services/document-updater/app/js/ProjectHistoryRedisManager.js index 6291831e27..8f16c3b5d8 100644 --- a/services/document-updater/app/js/ProjectHistoryRedisManager.js +++ b/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, } diff --git a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js index d920d2f0fa..0f5df2e29f 100644 --- a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js +++ b/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, diff --git a/services/project-history/app/js/HistoryStoreManager.js b/services/project-history/app/js/HistoryStoreManager.js index 2818710e3d..7c74538592 100644 --- a/services/project-history/app/js/HistoryStoreManager.js +++ b/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 }) } diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index 02e76faf7d..2cc7d3d674 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/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) diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index fe891c56b5..95c40d91bb 100644 --- a/services/project-history/app/js/types.ts +++ b/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 } diff --git a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js b/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js index d770d3b625..b52c7dd13d 100644 --- a/services/project-history/test/unit/js/HistoryStoreManager/HistoryStoreManagerTests.js +++ b/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 () { diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index b18e483055..48a301c165 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/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,