Merge pull request #32835 from overleaf/bg-fix-potential-race-condition-in-archive-manager

fix potential race condition in extractZipArchive

GitOrigin-RevId: 6dc77443e8a58172825d2b03645da05a9887e468

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

@@ -18,6 +18,7 @@ import OError from '@overleaf/o-error'
 import metrics from '@overleaf/metrics'
 import fs from 'node:fs'
 import Path from 'node:path'
+import { pipeline } from 'node:stream'
 import yauzl from 'yauzl'
 import Settings from '@overleaf/settings'
 import {
@@ -108,27 +109,16 @@ const ArchiveManager = {
     }
     callback = _.once(callback)
 
-    return zipfile.openReadStream(entry, function (err, readStream) {
+    fs.mkdir(Path.dirname(destFile), { recursive: true }, function (err) {
       if (err != null) {
         return callback(err)
       }
-      readStream.on('error', callback)
-      readStream.on('end', callback)
-
-      const errorHandler = function (err) {
-        // clean up before calling callback
-        readStream.unpipe()
-        readStream.destroy()
-        return callback(err)
-      }
-
-      fs.mkdir(Path.dirname(destFile), { recursive: true }, function (err) {
+      zipfile.openReadStream(entry, function (err, readStream) {
         if (err != null) {
-          return errorHandler(err)
+          return callback(err)
         }
         const writeStream = fs.createWriteStream(destFile)
-        writeStream.on('error', errorHandler)
-        return readStream.pipe(writeStream)
+        pipeline(readStream, writeStream, callback)
       })
     })
   },

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

@@ -10,6 +10,7 @@ import { vi, expect } from 'vitest'
 import sinon from 'sinon'
 import ArchiveErrors from '../../../../app/src/Features/Uploads/ArchiveErrors.mjs'
 import events from 'node:events'
+import { PassThrough } from 'node:stream'
 
 vi.mock('../../../../app/src/Features/Uploads/ArchiveErrors.js', () =>
   vi.importActual('../../../../app/src/Features/Uploads/ArchiveErrors.js')
@@ -74,13 +75,13 @@ describe('ArchiveManager', function () {
     describe('successfully', function () {
       beforeEach(async function (ctx) {
         await new Promise(resolve => {
-          ctx.readStream = new events.EventEmitter()
-          ctx.readStream.pipe = sinon.stub()
+          ctx.readStream = new PassThrough()
           ctx.zipfile.openReadStream = sinon
             .stub()
             .callsArgWith(1, null, ctx.readStream)
-          ctx.writeStream = new events.EventEmitter()
+          ctx.writeStream = new PassThrough()
           ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream)
+          sinon.spy(ctx.writeStream, 'destroy')
           ctx.ArchiveManager.extractZipArchive(
             ctx.source,
             ctx.destination,
@@ -89,7 +90,7 @@ describe('ArchiveManager', function () {
 
           // entry contains a single file
           ctx.zipfile.emit('entry', { fileName: 'testfile.txt' })
-          ctx.readStream.emit('end')
+          ctx.readStream.end()
           ctx.zipfile.emit('end')
         })
       })
@@ -106,13 +107,13 @@ describe('ArchiveManager', function () {
     describe('with a zipfile containing an empty directory', function () {
       beforeEach(async function (ctx) {
         await new Promise(resolve => {
-          ctx.readStream = new events.EventEmitter()
-          ctx.readStream.pipe = sinon.stub()
+          ctx.readStream = new PassThrough()
           ctx.zipfile.openReadStream = sinon
             .stub()
             .callsArgWith(1, null, ctx.readStream)
-          ctx.writeStream = new events.EventEmitter()
+          ctx.writeStream = new PassThrough()
           ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream)
+          sinon.spy(ctx.writeStream, 'destroy')
           ctx.ArchiveManager.extractZipArchive(
             ctx.source,
             ctx.destination,
@@ -124,7 +125,7 @@ describe('ArchiveManager', function () {
 
           // entry contains a single, empty directory
           ctx.zipfile.emit('entry', { fileName: 'testdir/' })
-          ctx.readStream.emit('end')
+          ctx.readStream.end()
           ctx.zipfile.emit('end')
         })
       })
@@ -285,10 +286,10 @@ describe('ArchiveManager', function () {
     describe('with backslashes in the path', function () {
       beforeEach(async function (ctx) {
         await new Promise(resolve => {
-          ctx.readStream = new events.EventEmitter()
-          ctx.readStream.pipe = sinon.stub()
-          ctx.writeStream = new events.EventEmitter()
+          ctx.readStream = new PassThrough()
+          ctx.writeStream = new PassThrough()
           ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream)
+          sinon.spy(ctx.writeStream, 'destroy')
           ctx.zipfile.openReadStream = sinon
             .stub()
             .callsArgWith(1, null, ctx.readStream)
@@ -301,7 +302,9 @@ describe('ArchiveManager', function () {
             }
           )
           ctx.zipfile.emit('entry', { fileName: 'wombat\\foo.tex' })
+          ctx.readStream.end()
           ctx.zipfile.emit('entry', { fileName: 'potato\\bar.tex' })
+          ctx.readStream.end()
           return ctx.zipfile.emit('end')
         })
       })
@@ -362,7 +365,7 @@ describe('ArchiveManager', function () {
           ctx.zipfile.openReadStream = sinon
             .stub()
             .callsArgWith(1, new Error('Something went wrong'))
-          ctx.writeStream = new events.EventEmitter()
+          ctx.writeStream = new PassThrough()
           ctx.ArchiveManager.extractZipArchive(
             ctx.source,
             ctx.destination,
@@ -392,13 +395,13 @@ describe('ArchiveManager', function () {
     describe('with an error in the file read stream', function () {
       beforeEach(async function (ctx) {
         await new Promise(resolve => {
-          ctx.readStream = new events.EventEmitter()
-          ctx.readStream.pipe = sinon.stub()
+          ctx.readStream = new PassThrough()
           ctx.zipfile.openReadStream = sinon
             .stub()
             .callsArgWith(1, null, ctx.readStream)
-          ctx.writeStream = new events.EventEmitter()
+          ctx.writeStream = new PassThrough()
           ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream)
+          sinon.spy(ctx.writeStream, 'destroy')
           ctx.ArchiveManager.extractZipArchive(
             ctx.source,
             ctx.destination,
@@ -409,7 +412,6 @@ describe('ArchiveManager', function () {
           )
           ctx.zipfile.emit('entry', { fileName: 'testfile.txt' })
           ctx.readStream.emit('error', new Error('Something went wrong'))
-          ctx.zipfile.emit('end')
         })
       })
 
@@ -429,15 +431,14 @@ describe('ArchiveManager', function () {
     describe('with an error in the file write stream', function () {
       beforeEach(async function (ctx) {
         await new Promise(resolve => {
-          ctx.readStream = new events.EventEmitter()
-          ctx.readStream.pipe = sinon.stub()
-          ctx.readStream.unpipe = sinon.stub()
-          ctx.readStream.destroy = sinon.stub()
+          ctx.readStream = new PassThrough()
+          sinon.spy(ctx.readStream, 'destroy')
           ctx.zipfile.openReadStream = sinon
             .stub()
             .callsArgWith(1, null, ctx.readStream)
-          ctx.writeStream = new events.EventEmitter()
+          ctx.writeStream = new PassThrough()
           ctx.fs.createWriteStream = sinon.stub().returns(ctx.writeStream)
+          sinon.spy(ctx.writeStream, 'destroy')
           ctx.ArchiveManager.extractZipArchive(
             ctx.source,
             ctx.destination,
@@ -448,7 +449,6 @@ describe('ArchiveManager', function () {
           )
           ctx.zipfile.emit('entry', { fileName: 'testfile.txt' })
           ctx.writeStream.emit('error', new Error('Something went wrong'))
-          ctx.zipfile.emit('end')
         })
       })
 
@@ -459,11 +459,6 @@ describe('ArchiveManager', function () {
             .and(sinon.match.has('message', 'Something went wrong'))
         )
       })
-
-      it('should unpipe from the readstream', function (ctx) {
-        ctx.readStream.unpipe.called.should.equal(true)
-      })
-
       it('should destroy the readstream', function (ctx) {
         ctx.readStream.destroy.called.should.equal(true)
       })