From 7eed283b11c8b60cd8ca45514a2e7d3059bea3cf Mon Sep 17 00:00:00 2001 From: Alf Eaton Date: Tue, 19 May 2026 12:28:06 +0100 Subject: [PATCH] Ignore entries in __MACOSX folder when importing zip archive (#33147) GitOrigin-RevId: e990d593d96085e13a209d4155823097b0814276 --- .../src/Features/Uploads/ArchiveManager.mjs | 40 ++++-- .../unit/src/Uploads/ArchiveManager.test.mjs | 118 +++++++++++++++++- 2 files changed, 148 insertions(+), 10 deletions(-) diff --git a/services/web/app/src/Features/Uploads/ArchiveManager.mjs b/services/web/app/src/Features/Uploads/ArchiveManager.mjs index c9f5d6fa9f..644c65ced6 100644 --- a/services/web/app/src/Features/Uploads/ArchiveManager.mjs +++ b/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) diff --git a/services/web/test/unit/src/Uploads/ArchiveManager.test.mjs b/services/web/test/unit/src/Uploads/ArchiveManager.test.mjs index 59ab988e05..03f807a365 100644 --- a/services/web/test/unit/src/Uploads/ArchiveManager.test.mjs +++ b/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 () {