From 9d93eee3e8bfeb0dbd32bbc458b674504e49b811 Mon Sep 17 00:00:00 2001
From: Brian Gough <brian.gough@overleaf.com>
Date: Wed, 9 Jan 2019 10:31:59 +0000
Subject: [PATCH] return a 404 error (instead of a 500) when copying a missing
 file

---
 services/filestore/app/coffee/FileController.coffee    |  7 +++++--
 .../filestore/app/coffee/S3PersistorManager.coffee     | 10 ++++++++--
 .../test/unit/coffee/FileControllerTests.coffee        |  9 +++++++++
 .../test/unit/coffee/S3PersistorManagerTests.coffee    |  7 +++++++
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/services/filestore/app/coffee/FileController.coffee b/services/filestore/app/coffee/FileController.coffee
index 24fd5229de..60dcc207b7 100644
--- a/services/filestore/app/coffee/FileController.coffee
+++ b/services/filestore/app/coffee/FileController.coffee
@@ -60,8 +60,11 @@ module.exports = FileController =
 		logger.log key:key, bucket:bucket, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "reciving request to copy file"
 		PersistorManager.copyFile bucket, "#{oldProject_id}/#{oldFile_id}", key, (err)->
 			if err?
-				logger.log err:err, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "something went wrong copying file"
-				res.send 500
+				if err instanceof Errors.NotFoundError
+					res.send 404
+				else
+					logger.log err:err, oldProject_id:oldProject_id, oldFile_id:oldFile_id, "something went wrong copying file"
+					res.send 500
 			else
 				res.send 200
 
diff --git a/services/filestore/app/coffee/S3PersistorManager.coffee b/services/filestore/app/coffee/S3PersistorManager.coffee
index 8debdaad4f..dd9aae3bf7 100644
--- a/services/filestore/app/coffee/S3PersistorManager.coffee
+++ b/services/filestore/app/coffee/S3PersistorManager.coffee
@@ -102,8 +102,14 @@ module.exports =
 		# use the AWS SDK instead of knox due to problems with error handling (https://github.com/Automattic/knox/issues/114)
 		s3.copyObject {Bucket: bucketName, Key: destKey, CopySource: source}, (err) ->
 			if err?
-				logger.err err:err, bucketName:bucketName, sourceKey:sourceKey, destKey:destKey, "something went wrong copying file in aws"
-			callback(err)
+				if err.code is 'NoSuchKey'
+					logger.err bucketName:bucketName, sourceKey:sourceKey, "original file not found in s3 when copying"
+					callback(new Errors.NotFoundError("original file not found in S3 when copying"))
+				else
+					logger.err err:err, bucketName:bucketName, sourceKey:sourceKey, destKey:destKey, "something went wrong copying file in aws"
+					callback(err)
+			else
+				callback()
 
 	deleteFile: (bucketName, key, callback)->
 		logger.log bucketName:bucketName, key:key, "delete file in s3"
diff --git a/services/filestore/test/unit/coffee/FileControllerTests.coffee b/services/filestore/test/unit/coffee/FileControllerTests.coffee
index 591644de60..0645aff27c 100644
--- a/services/filestore/test/unit/coffee/FileControllerTests.coffee
+++ b/services/filestore/test/unit/coffee/FileControllerTests.coffee
@@ -28,6 +28,8 @@ describe "FileController", ->
 			"./LocalFileWriter":@LocalFileWriter
 			"./FileHandler": @FileHandler
 			"./PersistorManager":@PersistorManager
+			"./Errors": @Errors =
+				NotFoundError: sinon.stub()
 			"settings-sharelatex": @settings
 			"logger-sharelatex":
 				log:->
@@ -111,6 +113,13 @@ describe "FileController", ->
 				done()
 			@controller.copyFile @req, @res
 
+		it "should send a 404 if the original file was not found", (done) ->
+			@PersistorManager.copyFile.callsArgWith(3, new @Errors.NotFoundError())
+			@res.send = (code)=>
+				code.should.equal 404
+				done()
+			@controller.copyFile @req, @res
+
 		it "should send a 500 if there was an error", (done)->
 			@PersistorManager.copyFile.callsArgWith(3, "error")
 			@res.send = (code)=>
diff --git a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
index 0860514180..7fc70c5065 100644
--- a/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
+++ b/services/filestore/test/unit/coffee/S3PersistorManagerTests.coffee
@@ -218,6 +218,13 @@ describe "S3PersistorManagerTests", ->
 				@stubbedS3Client.copyObject.calledWith({Bucket: @bucketName, Key: @destKey, CopySource: @bucketName + '/' + @key}).should.equal true
 				done()
 
+		it "should return a NotFoundError object if the original file does not exist", (done)->
+			NoSuchKeyError = {code: "NoSuchKey"}
+			@stubbedS3Client.copyObject.callsArgWith(1, NoSuchKeyError)
+			@S3PersistorManager.copyFile @bucketName, @sourceKey, @destKey, (err)=>
+				expect(err instanceof @Errors.NotFoundError).to.equal true
+				done()
+
 	describe "deleteDirectory", ->
 
 		beforeEach ->