Ignore entries in __MACOSX folder when importing zip archive (#33147)

GitOrigin-RevId: e990d593d96085e13a209d4155823097b0814276

services/web/app/src/Features/Uploads/ArchiveManager.mjs

@@ -13,8 +13,9 @@ import {
   EmptyZipFileError,
   ZipContentsTooLargeError,
 } from './ArchiveErrors.mjs'
+import FileTypeManager from './FileTypeManager.mjs'
 
-const ONE_MEG = 1024 * 1024
+const MAX_UNCOMPRESSED_BYTES = 1024 * 1024 * 300 // 300MB
 
 /**
  * Check if a zip entry's file path is safe and return the destination path.
@@ -69,34 +70,55 @@ function _isZipTooLarge(source) {
         return done(new InvalidZipFileError().withCause(err))
       }
 
+      // Initial rejection of pathological zips without enumerating the central directory.
+      // The per-entry check below applies the real limit after filtering out ignored files.
       if (
-        Settings.maxEntitiesPerProject != null &&
-        zipfile.entryCount > Settings.maxEntitiesPerProject
+        Number.isFinite(Settings.maxEntitiesPerProject) &&
+        zipfile.entryCount > Settings.maxEntitiesPerProject * 10
       ) {
         zipfile.close()
-        return done(null, true) // too many files in zip file
+        return done(null, true)
       }
 
       let totalSizeInBytes = null
+      let projectEntryCount = 0
       zipfile.on('error', err => done(err))
 
       // read all the entries
       zipfile.readEntry()
       zipfile.on('entry', entry => {
-        totalSizeInBytes += entry.uncompressedSize
+        if (!FileTypeManager.shouldIgnore(entry.fileName)) {
+          totalSizeInBytes += entry.uncompressedSize
+
+          if (totalSizeInBytes > MAX_UNCOMPRESSED_BYTES) {
+            zipfile.close()
+            return done(null, true) // total uncompressed size too large
+          }
+
+          projectEntryCount++
+
+          if (
+            Number.isFinite(Settings.maxEntitiesPerProject) &&
+            projectEntryCount > Settings.maxEntitiesPerProject
+          ) {
+            zipfile.close()
+            return done(null, true) // too many files in zip file
+          }
+        }
+
         zipfile.readEntry()
       })
 
       // no more entries to read
       zipfile.on('end', () => {
-        if (totalSizeInBytes == null || isNaN(totalSizeInBytes)) {
+        if (!Number.isFinite(totalSizeInBytes)) {
           logger.warn(
             { source, totalSizeInBytes },
             'error getting bytes of zip'
           )
           return done(new InvalidZipFileError({ info: { totalSizeInBytes } }))
         }
-        done(null, totalSizeInBytes > ONE_MEG * 300)
+        done(null, false)
       })
     })
   })
@@ -121,6 +143,10 @@ function _extractZipFiles(source, destination) {
 
       let entryFileCount = 0
       zipfile.on('entry', entry => {
+        if (FileTypeManager.shouldIgnore(entry.fileName)) {
+          return zipfile.readEntry()
+        }
+
         let destFile
         try {
           destFile = _checkFilePath(entry, destination)

services/web/test/unit/src/Uploads/ArchiveManager.test.mjs

@@ -52,6 +52,14 @@ describe('ArchiveManager', function () {
       () => ArchiveErrors
     )
 
+    ctx.FileTypeManager = {
+      shouldIgnore: sinon.stub().returns(false),
+    }
+    vi.doMock(
+      '../../../../app/src/Features/Uploads/FileTypeManager.mjs',
+      () => ({ default: ctx.FileTypeManager })
+    )
+
     ctx.ArchiveManager = (await import(modulePath)).default
   })
 
@@ -348,6 +356,43 @@ describe('ArchiveManager', function () {
       })
     })
 
+    describe('with an entry that FileTypeManager wants ignored', function () {
+      beforeEach(async function (ctx) {
+        ctx.FileTypeManager.shouldIgnore
+          .withArgs('__MACOSX/project/._file.tex')
+          .returns(true)
+
+        ctx.readStream = new PassThrough()
+        ctx.zipfile.openReadStream = sinon
+          .stub()
+          .callsArgWith(1, null, ctx.readStream)
+        ctx.writeStream = new PassThrough()
+        ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream)
+
+        const promise = ctx.ArchiveManager.promises.extractZipArchive(
+          ctx.source,
+          ctx.destination
+        )
+        await Promise.resolve()
+
+        ctx.zipfile.emit('entry', { fileName: 'project/file.tex' })
+        ctx.readStream.end()
+        await new Promise(resolve => process.nextTick(resolve))
+        ctx.zipfile.emit('entry', {
+          fileName: '__MACOSX/project/._file.tex',
+        })
+        ctx.zipfile.emit('end')
+        await promise
+      })
+
+      it('should not open a read stream for the ignored entry', function (ctx) {
+        ctx.zipfile.openReadStream.callCount.should.equal(1)
+        ctx.zipfile.openReadStream.firstCall.args[0].should.deep.equal({
+          fileName: 'project/file.tex',
+        })
+      })
+    })
+
     describe('with an error opening the file read stream', function () {
       beforeEach(async function (ctx) {
         ctx.zipfile.openReadStream = sinon
@@ -504,13 +549,80 @@ describe('ArchiveManager', function () {
       )
     })
 
-    it('should return true and close zipfile when entryCount exceeds maxEntitiesPerProject', async function (ctx) {
-      ctx.zipfile.entryCount = 100
-      ctx.Settings.maxEntitiesPerProject = 50
+    it('should reject upfront when entryCount far exceeds the limit', async function (ctx) {
+      ctx.Settings.maxEntitiesPerProject = 10
+      ctx.zipfile.entryCount = 1_000_000_000
       const isTooLarge = await ctx.ArchiveManager._isZipTooLarge(ctx.source)
       isTooLarge.should.equal(true)
+      ctx.zipfile.readEntry.called.should.equal(false)
       ctx.zipfile.close.called.should.equal(true)
     })
+
+    it('should not reject a __MACOSX-padded zip whose real entry count is within the limit', async function (ctx) {
+      ctx.Settings.maxEntitiesPerProject = 10
+      ctx.FileTypeManager.shouldIgnore.callsFake(name =>
+        name.startsWith('__MACOSX/')
+      )
+      const promise = ctx.ArchiveManager._isZipTooLarge(ctx.source)
+      for (let i = 0; i < 6; i++) {
+        ctx.zipfile.emit('entry', {
+          fileName: `project/file-${i}.tex`,
+          uncompressedSize: 100,
+        })
+      }
+      ctx.zipfile.emit('entry', {
+        fileName: '__MACOSX/',
+        uncompressedSize: 0,
+      })
+      ctx.zipfile.emit('entry', {
+        fileName: '__MACOSX/project/',
+        uncompressedSize: 0,
+      })
+      for (let i = 0; i < 6; i++) {
+        ctx.zipfile.emit('entry', {
+          fileName: `__MACOSX/project/._file-${i}.tex`,
+          uncompressedSize: 200,
+        })
+      }
+      ctx.zipfile.emit('end')
+      const isTooLarge = await promise
+      isTooLarge.should.equal(false)
+    })
+
+    it('should reject and stop iterating once the limit is exceeded', async function (ctx) {
+      ctx.Settings.maxEntitiesPerProject = 3
+      const promise = ctx.ArchiveManager._isZipTooLarge(ctx.source)
+      for (let i = 0; i < 4; i++) {
+        ctx.zipfile.emit('entry', {
+          fileName: `file-${i}.tex`,
+          uncompressedSize: 100,
+        })
+      }
+      const isTooLarge = await promise
+      isTooLarge.should.equal(true)
+      ctx.zipfile.close.called.should.equal(true)
+      // 3 readEntry()s for the first three entries, none after the 4th bails
+      ctx.zipfile.readEntry.callCount.should.equal(4)
+    })
+
+    it('should ignore __MACOSX bytes when computing total size', async function (ctx) {
+      ctx.FileTypeManager.shouldIgnore
+        .withArgs('__MACOSX/._file.tex')
+        .returns(true)
+      const promise = ctx.ArchiveManager._isZipTooLarge(ctx.source)
+      ctx.zipfile.emit('entry', {
+        fileName: 'file.tex',
+        uncompressedSize: 100,
+      })
+      // Huge AppleDouble entry that would otherwise trip the size check.
+      ctx.zipfile.emit('entry', {
+        fileName: '__MACOSX/._file.tex',
+        uncompressedSize: 109e16,
+      })
+      ctx.zipfile.emit('end')
+      const isTooLarge = await promise
+      isTooLarge.should.equal(false)
+    })
   })
 
   describe('findTopLevelDirectory', function () {