From 27985458ea67f6e8c6ceed20e7b2a779ef44867c Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Tue, 2 Jul 2019 12:48:57 +0200 Subject: [PATCH] Uploading an empty zipfile throws specific error (#1909) Added a counter for the total files extracted from a zip. If no files are extracted a specific error is thrown. GitOrigin-RevId: 391bb669500c86e2e7fecebd90e2201248a8afd3 --- .../src/Features/Uploads/ArchiveManager.js | 13 +++- .../acceptance/files/test_project_empty.zip | Bin 0 -> 22 bytes .../files/test_project_with_empty_folder.zip | Bin 0 -> 174 bytes .../acceptance/src/ProjectStructureTests.js | 60 +++++++++++++++ .../unit/src/Uploads/ArchiveManagerTests.js | 70 +++++++++++++++++- 5 files changed, 137 insertions(+), 6 deletions(-) create mode 100644 services/web/test/acceptance/files/test_project_empty.zip create mode 100644 services/web/test/acceptance/files/test_project_with_empty_folder.zip diff --git a/services/web/app/src/Features/Uploads/ArchiveManager.js b/services/web/app/src/Features/Uploads/ArchiveManager.js index a97e8a6d10..a600c8289c 100644 --- a/services/web/app/src/Features/Uploads/ArchiveManager.js +++ b/services/web/app/src/Features/Uploads/ArchiveManager.js @@ -61,7 +61,7 @@ const ArchiveManager = { { source, totalSizeInBytes }, 'error getting bytes of zip' ) - return callback(new Error('error getting bytes of zip')) + return callback(new Errors.InvalidError('invalid_zip_file')) } const isTooLarge = totalSizeInBytes > ONE_MEG * 300 return callback(null, isTooLarge) @@ -139,6 +139,8 @@ const ArchiveManager = { zipfile.on('error', callback) // read all the entries zipfile.readEntry() + + let entryFileCount = 0 zipfile.on('entry', function(entry) { logger.log( { source, fileName: entry.fileName }, @@ -168,6 +170,7 @@ const ArchiveManager = { zipfile.close() // bail out, stop reading file entries return callback(err) } else { + entryFileCount++ return zipfile.readEntry() } } @@ -179,7 +182,13 @@ const ArchiveManager = { }) }) // no more entries to read - return zipfile.on('end', callback) + return zipfile.on('end', () => { + if (entryFileCount > 0) { + callback() + } else { + callback(new Errors.InvalidError('empty_zip_file')) + } + }) }) }, diff --git a/services/web/test/acceptance/files/test_project_empty.zip b/services/web/test/acceptance/files/test_project_empty.zip new file mode 100644 index 0000000000000000000000000000000000000000..15cb0ecb3e219d1701294bfdf0fe3f5cb5d208e7 GIT binary patch literal 22 NcmWIWW@Tf*000g10H*)| literal 0 HcmV?d00001 diff --git a/services/web/test/acceptance/files/test_project_with_empty_folder.zip b/services/web/test/acceptance/files/test_project_with_empty_folder.zip new file mode 100644 index 0000000000000000000000000000000000000000..b728b8f634504713be9445b0984bc0c8e90ab87f GIT binary patch literal 174 zcmWIWW@h1H00HUP%YI-6l;B~IVMxs_D5*@#&q+xw(hm*cWMEch=89DZ;?fFk21b^z xj0_AcB0%*4-i%Cg%(%>vfSM@4@YWH;L^YKaVk(A-tZX2)j6fI!q}@Oq1^~)(7*PNK literal 0 HcmV?d00001 diff --git a/services/web/test/acceptance/src/ProjectStructureTests.js b/services/web/test/acceptance/src/ProjectStructureTests.js index 00577443c9..5ead09b835 100644 --- a/services/web/test/acceptance/src/ProjectStructureTests.js +++ b/services/web/test/acceptance/src/ProjectStructureTests.js @@ -342,6 +342,66 @@ describe('ProjectStructureChanges', function() { }) }) + describe('uploading an empty zipfile', function() { + let zipFile = null + + before(function(done) { + MockDocUpdaterApi.clearProjectStructureUpdates() + zipFile = fs.createReadStream( + Path.resolve(__dirname + '/../files/test_project_empty.zip') + ) + done() + }) + + it('should fail with 422 error', function(done) { + this.owner.request.post( + { + uri: 'project/new/upload', + formData: { + qqfile: zipFile + } + }, + (error, res) => { + if (error != null) { + throw error + } + expect(res.statusCode).to.equal(422) + done() + } + ) + }) + }) + + describe('uploading a zipfile containing only empty directories', function() { + let zipFile = null + + before(function(done) { + MockDocUpdaterApi.clearProjectStructureUpdates() + zipFile = fs.createReadStream( + Path.resolve(__dirname + '/../files/test_project_with_empty_folder.zip') + ) + done() + }) + + it('should fail with 422 error', function(done) { + this.owner.request.post( + { + uri: 'project/new/upload', + formData: { + qqfile: zipFile + } + }, + (error, res) => { + if (error != null) { + throw error + } + expect(res.statusCode).to.equal(422) + done() + } + ) + }) + }) + describe('uploading a project with a shared top-level folder', function() { before(function(done) { MockDocUpdaterApi.clearProjectStructureUpdates() diff --git a/services/web/test/unit/src/Uploads/ArchiveManagerTests.js b/services/web/test/unit/src/Uploads/ArchiveManagerTests.js index d9b56e3843..ee830c0288 100644 --- a/services/web/test/unit/src/Uploads/ArchiveManagerTests.js +++ b/services/web/test/unit/src/Uploads/ArchiveManagerTests.js @@ -70,11 +70,23 @@ describe('ArchiveManager', function() { describe('successfully', function() { beforeEach(function(done) { + this.readStream = new events.EventEmitter() + this.readStream.pipe = sinon.stub() + this.zipfile.openReadStream = sinon + .stub() + .callsArgWith(1, null, this.readStream) + this.writeStream = new events.EventEmitter() + this.fs.createWriteStream = sinon.stub().returns(this.writeStream) + this.fse.ensureDir = sinon.stub().callsArg(1) this.ArchiveManager.extractZipArchive( this.source, this.destination, done ) + + // entry contains a single file + this.zipfile.emit('entry', { fileName: 'testfile.txt' }) + this.readStream.emit('end') return this.zipfile.emit('end') }) @@ -93,6 +105,60 @@ describe('ArchiveManager', function() { }) }) + describe('with a zipfile containing an empty directory', function() { + beforeEach(function(done) { + this.readStream = new events.EventEmitter() + this.readStream.pipe = sinon.stub() + this.zipfile.openReadStream = sinon + .stub() + .callsArgWith(1, null, this.readStream) + this.writeStream = new events.EventEmitter() + this.fs.createWriteStream = sinon.stub().returns(this.writeStream) + this.fse.ensureDir = sinon.stub().callsArg(1) + this.ArchiveManager.extractZipArchive( + this.source, + this.destination, + error => { + this.callback(error) + done() + } + ) + + // entry contains a single, empty directory + this.zipfile.emit('entry', { fileName: 'testdir/' }) + this.readStream.emit('end') + return this.zipfile.emit('end') + }) + + it('should return the callback with an error', function() { + return sinon.assert.calledWithExactly( + this.callback, + new Errors.InvalidError('empty_zip_file') + ) + }) + }) + + describe('with an empty zipfile', function() { + beforeEach(function(done) { + this.ArchiveManager.extractZipArchive( + this.source, + this.destination, + error => { + this.callback(error) + return done() + } + ) + return this.zipfile.emit('end') + }) + + it('should return the callback with an error', function() { + return sinon.assert.calledWithExactly( + this.callback, + new Errors.InvalidError('empty_zip_file') + ) + }) + }) + describe('with an error in the zip file header', function() { beforeEach(function(done) { this.yauzl.open = sinon @@ -286,10 +352,6 @@ describe('ArchiveManager', function() { it('should not try to read the entry', function() { return this.zipfile.openReadStream.called.should.equal(false) }) - - it('should not log out a warning', function() { - return this.logger.warn.called.should.equal(false) - }) }) describe('with an error opening the file read stream', function() {