[web] do not send filestore URLs when feature is disabled (#23095)

* [web] do not return createdBlob=true from error path

Defense in-depth, currently the only call-site bails out on error.

* [web] do not send filestore URLs when feature is disabled

GitOrigin-RevId: 7e90cf4c4babffeec337702502234bac73c1f116

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

@@ -210,7 +210,7 @@ export type Doc = {
 
 export type File = {
   file: string
-  url: string
+  url?: string
   path: string
   _hash: string
   metadata?: LinkedFileData

services/web/app/src/Features/DocumentUpdater/DocumentUpdaterHandler.js

@@ -9,6 +9,7 @@ const { promisify } = require('util')
 const { promisifyMultiResult } = require('@overleaf/promise-utils')
 const ProjectGetter = require('../Project/ProjectGetter')
 const FileStoreHandler = require('../FileStore/FileStoreHandler')
+const Features = require('../../infrastructure/Features')
 
 /**
  * @param {string} projectId
@@ -275,11 +276,25 @@ function resyncProjectHistory(
     doc: doc.doc._id,
     path: doc.path,
   }))
+  const hasFilestore = Features.hasFeature('filestore')
+  if (!hasFilestore) {
+    // Files without a hash likely do not have a blob. Abort.
+    for (const { file } of files) {
+      if (!file.hash) {
+        return callback(
+          new OError('found file with missing hash', { projectId, file })
+        )
+      }
+    }
+  }
   files = files.map(file => ({
     file: file.file._id,
     path: file.path,
-    url: FileStoreHandler._buildUrl(projectId, file.file._id),
+    url: hasFilestore
+      ? FileStoreHandler._buildUrl(projectId, file.file._id)
+      : undefined,
     _hash: file.file.hash,
+    createdBlob: !hasFilestore,
     metadata: buildFileMetadataForHistory(file.file),
   }))
 
@@ -377,6 +392,17 @@ function updateProjectStructure(
         changes.newDocs,
         historyRangesSupport
       )
+      const hasFilestore = Features.hasFeature('filestore')
+      if (!hasFilestore) {
+        for (const newEntity of changes.newFiles || []) {
+          if (!newEntity.file.hash) {
+            // Files without a hash likely do not have a blob. Abort.
+            return callback(
+              new OError('found file with missing hash', { newEntity })
+            )
+          }
+        }
+      }
       const {
         deletes: fileDeletes,
         adds: fileAdds,
@@ -507,6 +533,7 @@ function _getUpdates(
       })
     }
   }
+  const hasFilestore = Features.hasFeature('filestore')
 
   for (const id in newEntitiesHash) {
     const newEntity = newEntitiesHash[id]
@@ -521,10 +548,10 @@ function _getUpdates(
         docLines: newEntity.docLines,
         ranges: newEntity.ranges,
         historyRangesSupport,
-        url: newEntity.url,
+        url: newEntity.file != null && hasFilestore ? newEntity.url : undefined,
         hash: newEntity.file != null ? newEntity.file.hash : undefined,
         metadata: buildFileMetadataForHistory(newEntity.file),
-        createdBlob: newEntity.createdBlob ?? false,
+        createdBlob: (newEntity.createdBlob || !hasFilestore) ?? false,
       })
     } else if (newEntity.path !== oldEntity.path) {
       // entity renamed

services/web/app/src/Features/FileStore/FileStoreHandler.js

@@ -45,7 +45,10 @@ const FileStoreHandler = {
         FileStoreHandler.RETRY_ATTEMPTS,
         cb =>
           HistoryManager.uploadBlobFromDisk(historyId, hash, size, fsPath, cb),
-        error => callback(error, true)
+        error => {
+          if (error) return callback(error, false)
+          callback(null, true)
+        }
       )
     } else {
       callback(null, false)

services/web/test/unit/src/DocumentUpdater/DocumentUpdaterHandlerTests.js

@@ -28,6 +28,7 @@ describe('DocumentUpdaterHandler', function () {
           url: 'http://project_history.example.com',
         },
       },
+      moduleImportSequence: [],
     }
     this.source = 'dropbox'
 
@@ -1510,6 +1511,89 @@ describe('DocumentUpdaterHandler', function () {
           )
         })
       })
+
+      describe('with filestore disabled', function () {
+        beforeEach(function () {
+          this.settings.disableFilestore = true
+        })
+        it('should add files without URL and with createdBlob', function (done) {
+          this.fileId = new ObjectId()
+          this.changes = {
+            newFiles: [
+              {
+                path: '/bar',
+                url: 'filestore.example.com/file',
+                file: { _id: this.fileId, hash: '12345' },
+              },
+            ],
+            newProject: { version: this.version },
+          }
+
+          const updates = [
+            {
+              type: 'add-file',
+              id: this.fileId.toString(),
+              pathname: '/bar',
+              docLines: undefined,
+              historyRangesSupport: false,
+              url: undefined,
+              hash: '12345',
+              ranges: undefined,
+              createdBlob: true,
+              metadata: undefined,
+            },
+          ]
+
+          this.handler.updateProjectStructure(
+            this.project_id,
+            this.projectHistoryId,
+            this.user_id,
+            this.changes,
+            this.source,
+            () => {
+              this.request.should.have.been.calledWith({
+                url: this.url,
+                method: 'POST',
+                json: {
+                  updates,
+                  userId: this.user_id,
+                  version: this.version,
+                  projectHistoryId: this.projectHistoryId,
+                  source: this.source,
+                },
+                timeout: 30 * 1000,
+              })
+              done()
+            }
+          )
+        })
+        it('should flag files without hash', function (done) {
+          this.fileId = new ObjectId()
+          this.changes = {
+            newFiles: [
+              {
+                path: '/bar',
+                url: 'filestore.example.com/file',
+                file: { _id: this.fileId },
+              },
+            ],
+            newProject: { version: this.version },
+          }
+
+          this.handler.updateProjectStructure(
+            this.project_id,
+            this.projectHistoryId,
+            this.user_id,
+            this.changes,
+            this.source,
+            err => {
+              err.should.match(/found file with missing hash/)
+              this.request.should.not.have.been.called
+              done()
+            }
+          )
+        })
+      })
     })
   })
 
@@ -1607,6 +1691,7 @@ describe('DocumentUpdaterHandler', function () {
                   _hash: '42',
                   path: '1.png',
                   url: `http://filestore/project/${projectId}/file/${fileId1}`,
+                  createdBlob: false,
                   metadata: undefined,
                 },
                 {
@@ -1614,6 +1699,7 @@ describe('DocumentUpdaterHandler', function () {
                   _hash: '1337',
                   path: '1.bib',
                   url: `http://filestore/project/${projectId}/file/${fileId2}`,
+                  createdBlob: false,
                   metadata: {
                     importedAt: fileCreated2,
                     provider: 'references-provider',
@@ -1624,6 +1710,7 @@ describe('DocumentUpdaterHandler', function () {
                   _hash: '21',
                   path: 'bar.txt',
                   url: `http://filestore/project/${projectId}/file/${fileId3}`,
+                  createdBlob: false,
                   metadata: {
                     importedAt: fileCreated3,
                     provider: 'project_output_file',
@@ -1641,6 +1728,158 @@ describe('DocumentUpdaterHandler', function () {
         }
       )
     })
+    describe('with filestore disabled', function () {
+      beforeEach(function () {
+        this.settings.disableFilestore = true
+      })
+      it('should add files without URL', function (done) {
+        const fileId1 = new ObjectId()
+        const fileId2 = new ObjectId()
+        const fileId3 = new ObjectId()
+        const fileCreated2 = new Date()
+        const fileCreated3 = new Date()
+        const otherProjectId = new ObjectId().toString()
+        const files = [
+          { file: { _id: fileId1, hash: '42' }, path: '1.png' },
+          {
+            file: {
+              _id: fileId2,
+              hash: '1337',
+              created: fileCreated2,
+              linkedFileData: {
+                provider: 'references-provider',
+              },
+            },
+            path: '1.bib',
+          },
+          {
+            file: {
+              _id: fileId3,
+              hash: '21',
+              created: fileCreated3,
+              linkedFileData: {
+                provider: 'project_output_file',
+                build_id: '1234-abc',
+                clsiServerId: 'server-1',
+                source_project_id: otherProjectId,
+                source_output_file_path: 'foo/bar.txt',
+              },
+            },
+            path: 'bar.txt',
+          },
+        ]
+        const docs = []
+        this.request.yields(null, { statusCode: 200 })
+        const projectId = new ObjectId()
+        const projectHistoryId = 99
+        this.handler.resyncProjectHistory(
+          projectId,
+          projectHistoryId,
+          docs,
+          files,
+          {},
+          () => {
+            this.request.should.have.been.calledWith({
+              url: `${this.settings.apis.documentupdater.url}/project/${projectId}/history/resync`,
+              method: 'POST',
+              json: {
+                docs: [],
+                files: [
+                  {
+                    file: fileId1,
+                    _hash: '42',
+                    path: '1.png',
+                    url: undefined,
+                    createdBlob: true,
+                    metadata: undefined,
+                  },
+                  {
+                    file: fileId2,
+                    _hash: '1337',
+                    path: '1.bib',
+                    url: undefined,
+                    createdBlob: true,
+                    metadata: {
+                      importedAt: fileCreated2,
+                      provider: 'references-provider',
+                    },
+                  },
+                  {
+                    file: fileId3,
+                    _hash: '21',
+                    path: 'bar.txt',
+                    url: undefined,
+                    createdBlob: true,
+                    metadata: {
+                      importedAt: fileCreated3,
+                      provider: 'project_output_file',
+                      source_project_id: otherProjectId,
+                      source_output_file_path: 'foo/bar.txt',
+                      // build_id and clsiServerId are omitted
+                    },
+                  },
+                ],
+                projectHistoryId,
+              },
+              timeout: 6 * 60 * 1000,
+            })
+            done()
+          }
+        )
+      })
+      it('should flag files with missing hashes', function (done) {
+        const fileId1 = new ObjectId()
+        const fileId2 = new ObjectId()
+        const fileId3 = new ObjectId()
+        const fileCreated2 = new Date()
+        const fileCreated3 = new Date()
+        const otherProjectId = new ObjectId().toString()
+        const files = [
+          { file: { _id: fileId1, hash: '42' }, path: '1.png' },
+          {
+            file: {
+              _id: fileId2,
+              created: fileCreated2,
+              linkedFileData: {
+                provider: 'references-provider',
+              },
+            },
+            path: '1.bib',
+          },
+          {
+            file: {
+              _id: fileId3,
+              hash: '21',
+              created: fileCreated3,
+              linkedFileData: {
+                provider: 'project_output_file',
+                build_id: '1234-abc',
+                clsiServerId: 'server-1',
+                source_project_id: otherProjectId,
+                source_output_file_path: 'foo/bar.txt',
+              },
+            },
+            path: 'bar.txt',
+          },
+        ]
+        const docs = []
+        this.request.yields(null, { statusCode: 200 })
+        const projectId = new ObjectId()
+        const projectHistoryId = 99
+        this.handler.resyncProjectHistory(
+          projectId,
+          projectHistoryId,
+          docs,
+          files,
+          {},
+          err => {
+            err.should.match(/found file with missing hash/)
+            this.request.should.not.have.been.called
+            done()
+          }
+        )
+      })
+    })
   })
 
   describe('appendToDocument', function () {