From 487d9125a23083b6fdc2a9f944680429d703ba85 Mon Sep 17 00:00:00 2001
From: andrew rumble <andrew.rumble@overleaf.com>
Date: Tue, 18 Jun 2024 09:19:08 +0100
Subject: [PATCH] Improve stream error safety

GitOrigin-RevId: de4c512a62d304b3eeb2a1521aac173fa93d8411
---
 services/clsi/app/js/OutputController.js      |  3 ++-
 .../clsi/app/js/OutputFileArchiveManager.js   | 15 ++++++++++++---
 .../test/unit/js/OutputControllerTests.js     | 19 ++++++++++++-------
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/services/clsi/app/js/OutputController.js b/services/clsi/app/js/OutputController.js
index 8ac6fd7a28..e5048c4c25 100644
--- a/services/clsi/app/js/OutputController.js
+++ b/services/clsi/app/js/OutputController.js
@@ -1,5 +1,6 @@
 const OutputFileArchiveManager = require('./OutputFileArchiveManager')
 const { expressify } = require('@overleaf/promise-utils')
+const { pipeline } = require('node:stream/promises')
 
 async function createOutputZip(req, res) {
   const {
@@ -16,7 +17,7 @@ async function createOutputZip(req, res) {
 
   res.attachment('output.zip')
   res.setHeader('X-Content-Type-Options', 'nosniff')
-  archive.pipe(res)
+  await pipeline(archive, res)
 }
 
 module.exports = { createOutputZip: expressify(createOutputZip) }
diff --git a/services/clsi/app/js/OutputFileArchiveManager.js b/services/clsi/app/js/OutputFileArchiveManager.js
index 98d6b8dddd..5c7902a468 100644
--- a/services/clsi/app/js/OutputFileArchiveManager.js
+++ b/services/clsi/app/js/OutputFileArchiveManager.js
@@ -34,13 +34,22 @@ module.exports = {
     const missingFiles = []
 
     for (const file of validFiles) {
+      let fileHandle
+
       try {
-        const fileHandle = await open(file, 'r')
-        const fileStream = fileHandle.createReadStream()
-        archive.append(fileStream, { name: path.basename(file) })
+        fileHandle = await open(file, 'r')
       } catch (error) {
+        logger.warn(
+          { file, error },
+          'error opening file to add to output files archive'
+        )
         missingFiles.push(file)
+        continue
       }
+      const fileStream = fileHandle.createReadStream()
+      archive.append(fileStream, {
+        name: path.basename(file),
+      })
     }
 
     if (missingFiles.length > 0) {
diff --git a/services/clsi/test/unit/js/OutputControllerTests.js b/services/clsi/test/unit/js/OutputControllerTests.js
index 49fa1fe5f3..33367af26a 100644
--- a/services/clsi/test/unit/js/OutputControllerTests.js
+++ b/services/clsi/test/unit/js/OutputControllerTests.js
@@ -8,10 +8,9 @@ const MODULE_PATH = require('path').join(
 describe('OutputController', function () {
   describe('createOutputZip', function () {
     beforeEach(function () {
-      this.archive = {
-        on: sinon.stub(),
-        pipe: sinon.stub(),
-      }
+      this.archive = {}
+
+      this.pipeline = sinon.stub().resolves()
 
       this.archiveFilesForBuild = sinon.stub().resolves(this.archive)
 
@@ -20,6 +19,9 @@ describe('OutputController', function () {
           './OutputFileArchiveManager': {
             archiveFilesForBuild: this.archiveFilesForBuild,
           },
+          'node:stream/promises': {
+            pipeline: this.pipeline,
+          },
         },
       })
     })
@@ -40,12 +42,15 @@ describe('OutputController', function () {
             files: ['output.tex'],
           },
         }
-        this.archive.pipe.callsFake(() => done())
+        this.pipeline.callsFake(() => {
+          done()
+          return Promise.resolve()
+        })
         this.OutputController.createOutputZip(this.req, this.res)
       })
 
-      it('pipes the archive to the response', function () {
-        sinon.assert.calledWith(this.archive.pipe, this.res)
+      it('creates a pipeline from the archive to the response', function () {
+        sinon.assert.calledWith(this.pipeline, this.archive, this.res)
       })
 
       it('calls the express convenience method to set attachment headers', function () {