From da242d90e6da913bb78eb6e4c1fa675927fc42fd Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Sat, 12 Mar 2016 12:01:36 +0000 Subject: [PATCH 1/4] added extra isSymLink checks into reading files --- .../FileStore/FileStoreHandler.coffee | 39 +++-- .../Uploads/FileSystemImportManager.coffee | 146 +++++++++++------- .../FileStore/FileStoreHandlerTests.coffee | 9 ++ .../FileSystemImportManagerTests.coffee | 65 +++++++- 4 files changed, 178 insertions(+), 81 deletions(-) diff --git a/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee b/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee index 34ab6f8ece..20af15676f 100644 --- a/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee +++ b/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee @@ -6,24 +6,31 @@ settings = require("settings-sharelatex") oneMinInMs = 60 * 1000 fiveMinsInMs = oneMinInMs * 5 -module.exports = +module.exports = FileStoreHandler = uploadFileFromDisk: (project_id, file_id, fsPath, callback)-> - logger.log project_id:project_id, file_id:file_id, fsPath:fsPath, "uploading file from disk" - readStream = fs.createReadStream(fsPath) - opts = - method: "post" - uri: @_buildUrl(project_id, file_id) - timeout:fiveMinsInMs - writeStream = request(opts) - readStream.pipe writeStream - writeStream.on "end", callback - readStream.on "error", (err)-> - logger.err err:err, project_id:project_id, file_id:file_id, fsPath:fsPath, "something went wrong on the read stream of uploadFileFromDisk" - callback err - writeStream.on "error", (err)-> - logger.err err:err, project_id:project_id, file_id:file_id, fsPath:fsPath, "something went wrong on the write stream of uploadFileFromDisk" - callback err + fs.lstat fsPath, (err, stat)-> + if err? + logger.err err:err, "error with path symlink check" + return callback(err) + if stat.isSymbolicLink() + logger.log project_id:project_id, file_id:file_id, fsPath:fsPath, "error uploading file from disk, file path is symlink" + return callback('file is from symlink') + logger.log project_id:project_id, file_id:file_id, fsPath:fsPath, "uploading file from disk" + readStream = fs.createReadStream(fsPath) + opts = + method: "post" + uri: FileStoreHandler._buildUrl(project_id, file_id) + timeout:fiveMinsInMs + writeStream = request(opts) + readStream.pipe writeStream + writeStream.on "end", callback + readStream.on "error", (err)-> + logger.err err:err, project_id:project_id, file_id:file_id, fsPath:fsPath, "something went wrong on the read stream of uploadFileFromDisk" + callback err + writeStream.on "error", (err)-> + logger.err err:err, project_id:project_id, file_id:file_id, fsPath:fsPath, "something went wrong on the write stream of uploadFileFromDisk" + callback err getFileStream: (project_id, file_id, query, callback)-> logger.log project_id:project_id, file_id:file_id, query:query, "getting file stream from file store" diff --git a/services/web/app/coffee/Features/Uploads/FileSystemImportManager.coffee b/services/web/app/coffee/Features/Uploads/FileSystemImportManager.coffee index c7e4fbe03a..a8b9f639fe 100644 --- a/services/web/app/coffee/Features/Uploads/FileSystemImportManager.coffee +++ b/services/web/app/coffee/Features/Uploads/FileSystemImportManager.coffee @@ -4,76 +4,108 @@ _ = require "underscore" FileTypeManager = require "./FileTypeManager" EditorController = require "../Editor/EditorController" ProjectLocator = require "../Project/ProjectLocator" +logger = require("logger-sharelatex") module.exports = FileSystemImportManager = addDoc: (user_id, project_id, folder_id, name, path, replace, callback = (error, doc)-> )-> - fs.readFile path, "utf8", (error, content = "") -> - return callback(error) if error? - content = content.replace(/\r/g, "") - lines = content.split("\n") - if replace + FileSystemImportManager._isSymLink path, (err, isSymLink)-> + if isSymLink + logger.log user_id:user_id, project_id:project_id, folder_id:folder_id, name:name, path:path, "add doc is from symlink, stopping process" + return callback("path is symlink") + fs.readFile path, "utf8", (error, content = "") -> + return callback(error) if error? + content = content.replace(/\r/g, "") + lines = content.split("\n") + if replace + ProjectLocator.findElement project_id: project_id, element_id: folder_id, type: "folder", (error, folder) -> + return callback(error) if error? + return callback(new Error("Couldn't find folder")) if !folder? + existingDoc = null + for doc in folder.docs + if doc.name == name + existingDoc = doc + break + if existingDoc? + EditorController.setDoc project_id, existingDoc._id, user_id, lines, "upload", callback + else + EditorController.addDocWithoutLock project_id, folder_id, name, lines, "upload", callback + else + EditorController.addDocWithoutLock project_id, folder_id, name, lines, "upload", callback + + addFile: (user_id, project_id, folder_id, name, path, replace, callback = (error, file)-> )-> + FileSystemImportManager._isSymLink path, (err, isSymLink)-> + if isSymLink + logger.log user_id:user_id, project_id:project_id, folder_id:folder_id, name:name, path:path, "add file is from symlink, stopping insert" + return callback("path is symlink") + + if !replace + EditorController.addFileWithoutLock project_id, folder_id, name, path, "upload", callback + else ProjectLocator.findElement project_id: project_id, element_id: folder_id, type: "folder", (error, folder) -> return callback(error) if error? return callback(new Error("Couldn't find folder")) if !folder? - existingDoc = null - for doc in folder.docs - if doc.name == name - existingDoc = doc + existingFile = null + for fileRef in folder.fileRefs + if fileRef.name == name + existingFile = fileRef break - if existingDoc? - EditorController.setDoc project_id, existingDoc._id, user_id, lines, "upload", callback + if existingFile? + EditorController.replaceFile project_id, existingFile._id, path, "upload", callback else - EditorController.addDocWithoutLock project_id, folder_id, name, lines, "upload", callback - else - EditorController.addDocWithoutLock project_id, folder_id, name, lines, "upload", callback - - addFile: (user_id, project_id, folder_id, name, path, replace, callback = (error, file)-> )-> - if replace - ProjectLocator.findElement project_id: project_id, element_id: folder_id, type: "folder", (error, folder) -> - return callback(error) if error? - return callback(new Error("Couldn't find folder")) if !folder? - existingFile = null - for fileRef in folder.fileRefs - if fileRef.name == name - existingFile = fileRef - break - if existingFile? - EditorController.replaceFile project_id, existingFile._id, path, "upload", callback - else - EditorController.addFileWithoutLock project_id, folder_id, name, path, "upload", callback - else - EditorController.addFileWithoutLock project_id, folder_id, name, path, "upload", callback + EditorController.addFileWithoutLock project_id, folder_id, name, path, "upload", callback addFolder: (user_id, project_id, folder_id, name, path, replace, callback = (error)-> ) -> - EditorController.addFolderWithoutLock project_id, folder_id, name, "upload", (error, new_folder) => - return callback(error) if error? - @addFolderContents user_id, project_id, new_folder._id, path, replace, (error) -> + FileSystemImportManager._isSymLink path, (err, isSymLink)-> + if isSymLink + logger.log user_id:user_id, project_id:project_id, folder_id:folder_id, path:path, "add folder is from symlink, stopping insert" + return callback("path is symlink") + EditorController.addFolderWithoutLock project_id, folder_id, name, "upload", (error, new_folder) => return callback(error) if error? - callback null, new_folder + FileSystemImportManager.addFolderContents user_id, project_id, new_folder._id, path, replace, (error) -> + return callback(error) if error? + callback null, new_folder addFolderContents: (user_id, project_id, parent_folder_id, folderPath, replace, callback = (error)-> ) -> - fs.readdir folderPath, (error, entries = []) => - return callback(error) if error? - jobs = _.map entries, (entry) => - (callback) => - FileTypeManager.shouldIgnore entry, (error, ignore) => - return callback(error) if error? - if !ignore - @addEntity user_id, project_id, parent_folder_id, entry, "#{folderPath}/#{entry}", replace, callback - else - callback() - async.parallelLimit jobs, 5, callback + FileSystemImportManager._isSymLink folderPath, (err, isSymLink)-> + if isSymLink + logger.log user_id:user_id, project_id:project_id, parent_folder_id:parent_folder_id, folderPath:folderPath, "add folder contents is from symlink, stopping insert" + return callback("path is symlink") + fs.readdir folderPath, (error, entries = []) => + return callback(error) if error? + jobs = _.map entries, (entry) => + (callback) => + FileTypeManager.shouldIgnore entry, (error, ignore) => + return callback(error) if error? + if !ignore + FileSystemImportManager.addEntity user_id, project_id, parent_folder_id, entry, "#{folderPath}/#{entry}", replace, callback + else + callback() + async.parallelLimit jobs, 5, callback addEntity: (user_id, project_id, folder_id, name, path, replace, callback = (error, entity)-> ) -> - FileTypeManager.isDirectory path, (error, isDirectory) => - return callback(error) if error? - if isDirectory - @addFolder user_id, project_id, folder_id, name, path, replace, callback - else - FileTypeManager.isBinary name, path, (error, isBinary) => - return callback(error) if error? - if isBinary - @addFile user_id, project_id, folder_id, name, path, replace, callback - else - @addDoc user_id, project_id, folder_id, name, path, replace, callback + FileSystemImportManager._isSymLink path, (err, isSymLink)-> + if isSymLink + logger.log user_id:user_id, project_id:project_id, folder_id:folder_id, path:path, "add entry is from symlink, stopping insert" + return callback("path is symlink") + + FileTypeManager.isDirectory path, (error, isDirectory) => + return callback(error) if error? + if isDirectory + FileSystemImportManager.addFolder user_id, project_id, folder_id, name, path, replace, callback + else + FileTypeManager.isBinary name, path, (error, isBinary) => + return callback(error) if error? + if isBinary + FileSystemImportManager.addFile user_id, project_id, folder_id, name, path, replace, callback + else + FileSystemImportManager.addDoc user_id, project_id, folder_id, name, path, replace, callback + + + _isSymLink: (path, callback = (err, isSymLink)->)-> + fs.lstat path, (err, stat)-> + if err? + logger.err err:err, "error with path symlink check" + return callback(err) + isSymLink = stat.isSymbolicLink() + callback(err, isSymLink) diff --git a/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee b/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee index 2a82ba944f..c35eefcf87 100644 --- a/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee @@ -10,6 +10,7 @@ describe "FileStoreHandler", -> beforeEach -> @fs = createReadStream : sinon.stub() + lstat: sinon.stub().callsArgWith(1, null, {isSymbolicLink:=>@isSymlink}) @writeStream = my:"writeStream" on: (type, cb)-> @@ -31,6 +32,7 @@ describe "FileStoreHandler", -> describe "uploadFileFromDisk", -> beforeEach -> @request.returns(@writeStream) + @isSymlink = false it "should create read stream", (done)-> @fs.createReadStream.returns @@ -74,6 +76,13 @@ describe "FileStoreHandler", -> @handler._buildUrl.calledWith(@project_id, @file_id).should.equal true done() + describe "symlink", -> + it "should not read file if it is symlink", (done)-> + @isSymlink = true + @handler.uploadFileFromDisk @project_id, @file_id, @fsPath, => + @fs.createReadStream.called.should.equal false + done() + describe "deleteFile", -> it "should send a delete request to filestore api", (done)-> diff --git a/services/web/test/UnitTests/coffee/Uploads/FileSystemImportManagerTests.coffee b/services/web/test/UnitTests/coffee/Uploads/FileSystemImportManagerTests.coffee index 441dd8163d..4fbea218bb 100644 --- a/services/web/test/UnitTests/coffee/Uploads/FileSystemImportManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Uploads/FileSystemImportManagerTests.coffee @@ -18,12 +18,29 @@ describe "FileSystemImportManager", -> "../Editor/EditorController": @EditorController = {} "./FileTypeManager": @FileTypeManager = {} "../Project/ProjectLocator": @ProjectLocator = {} + "logger-sharelatex": + log:-> + err:-> describe "addDoc", -> beforeEach -> @docContent = "one\ntwo\nthree" @docLines = @docContent.split("\n") @fs.readFile = sinon.stub().callsArgWith(2, null, @docContent) + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + + + describe "when path is symlink", -> + beforeEach -> + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, true) + @EditorController.addDocWithoutLock = sinon.stub() + @FileSystemImportManager.addDoc @user_id, @project_id, @folder_id, @name, @path_on_disk, false, @callback + + it "should not read the file from disk", -> + @fs.readFile.called.should.equal false + + it "should not insert the doc", -> + @EditorController.addDocWithoutLock.called.should.equal false describe "with replace set to false", -> beforeEach -> @@ -98,12 +115,24 @@ describe "FileSystemImportManager", -> describe "addFile with replace set to false", -> beforeEach -> @EditorController.addFileWithoutLock = sinon.stub().callsArg(5) + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) @FileSystemImportManager.addFile @user_id, @project_id, @folder_id, @name, @path_on_disk, false, @callback it "should add the file", -> @EditorController.addFileWithoutLock.calledWith(@project_id, @folder_id, @name, @path_on_disk, "upload") .should.equal true + describe "addFile with symlink", -> + beforeEach -> + @EditorController.addFileWithoutLock = sinon.stub() + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, true) + @EditorController.replaceFile = sinon.stub() + @FileSystemImportManager.addFile @user_id, @project_id, @folder_id, @name, @path_on_disk, false, @callback + + it "should node add the file", -> + @EditorController.addFileWithoutLock.called.should.equal false + @EditorController.replaceFile.called.should.equal false + describe "addFile with replace set to true", -> describe "when the file doesn't exist", -> beforeEach -> @@ -113,6 +142,7 @@ describe "FileSystemImportManager", -> name: "not-the-right-file.tex" }] } + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) @ProjectLocator.findElement = sinon.stub().callsArgWith(1, null, @folder) @EditorController.addFileWithoutLock = sinon.stub().callsArg(5) @FileSystemImportManager.addFile @user_id, @project_id, @folder_id, @name, @path_on_disk, true, @callback @@ -137,6 +167,7 @@ describe "FileSystemImportManager", -> name: "not-the-right-file.tex" }] } + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) @ProjectLocator.findElement = sinon.stub().callsArgWith(1, null, @folder) @EditorController.replaceFile = sinon.stub().callsArg(4) @FileSystemImportManager.addFile @user_id, @project_id, @folder_id, @name, @path_on_disk, true, @callback @@ -151,20 +182,34 @@ describe "FileSystemImportManager", -> .should.equal true describe "addFolder", -> + beforeEach -> @new_folder_id = "new-folder-id" @EditorController.addFolderWithoutLock = sinon.stub().callsArgWith(4, null, _id: @new_folder_id) @FileSystemImportManager.addFolderContents = sinon.stub().callsArg(5) - @FileSystemImportManager.addFolder @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback - it "should add a folder to the project", -> - @EditorController.addFolderWithoutLock.calledWith(@project_id, @folder_id, @name, "upload") - .should.equal true + describe "successfully", -> + beforeEach -> + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager.addFolder @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback - it "should add the folders contents", -> - @FileSystemImportManager.addFolderContents.calledWith(@user_id, @project_id, @new_folder_id, @path_on_disk, @replace) - .should.equal true + it "should add a folder to the project", -> + @EditorController.addFolderWithoutLock.calledWith(@project_id, @folder_id, @name, "upload") + .should.equal true + it "should add the folders contents", -> + @FileSystemImportManager.addFolderContents.calledWith(@user_id, @project_id, @new_folder_id, @path_on_disk, @replace) + .should.equal true + + describe "with symlink", -> + beforeEach -> + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, true) + @FileSystemImportManager.addFolder @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback + + it "should not add a folder to the project", -> + @EditorController.addFolderWithoutLock.called.should.equal false + @FileSystemImportManager.addFolderContents.called.should.equal false + describe "addFolderContents", -> beforeEach -> @folderEntries = ["path1", "path2", "path3"] @@ -173,6 +218,7 @@ describe "FileSystemImportManager", -> @FileSystemImportManager.addEntity = sinon.stub().callsArg(6) @FileTypeManager.shouldIgnore = (path, callback) => callback null, @ignoredEntries.indexOf(require("path").basename(path)) != -1 + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) @FileSystemImportManager.addFolderContents @user_id, @project_id, @folder_id, @path_on_disk, @replace, @callback it "should call addEntity for each file in the folder which is not ignored", -> @@ -188,11 +234,12 @@ describe "FileSystemImportManager", -> it "should look in the correct directory", -> @fs.readdir.calledWith(@path_on_disk).should.equal true - describe "addEntity", -> + describe "addEntity dddddddd", -> describe "with directory", -> beforeEach -> @FileTypeManager.isDirectory = sinon.stub().callsArgWith(1, null, true) @FileSystemImportManager.addFolder = sinon.stub().callsArg(6) + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) @FileSystemImportManager.addEntity @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback it "should call addFolder", -> @@ -203,6 +250,7 @@ describe "FileSystemImportManager", -> beforeEach -> @FileTypeManager.isDirectory = sinon.stub().callsArgWith(1, null, false) @FileTypeManager.isBinary = sinon.stub().callsArgWith(2, null, true) + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) @FileSystemImportManager.addFile = sinon.stub().callsArg(6) @FileSystemImportManager.addEntity @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback @@ -215,6 +263,7 @@ describe "FileSystemImportManager", -> @FileTypeManager.isDirectory = sinon.stub().callsArgWith(1, null, false) @FileTypeManager.isBinary = sinon.stub().callsArgWith(2, null, false) @FileSystemImportManager.addDoc = sinon.stub().callsArg(6) + @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) @FileSystemImportManager.addEntity @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback it "should call addFile", -> From 6664b67fba4d6e92b4d77bac044edaadaac57236 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Sat, 12 Mar 2016 12:38:21 +0000 Subject: [PATCH 2/4] check size of zip files --- .../Features/Uploads/ArchiveManager.coffee | 61 ++++++++++++------- .../coffee/Uploads/ArchiveManagerTests.coffee | 14 +++++ 2 files changed, 54 insertions(+), 21 deletions(-) diff --git a/services/web/app/coffee/Features/Uploads/ArchiveManager.coffee b/services/web/app/coffee/Features/Uploads/ArchiveManager.coffee index 325df9369f..74f6108ba0 100644 --- a/services/web/app/coffee/Features/Uploads/ArchiveManager.coffee +++ b/services/web/app/coffee/Features/Uploads/ArchiveManager.coffee @@ -4,38 +4,57 @@ metrics = require "../../infrastructure/Metrics" fs = require "fs" Path = require "path" +ONE_MEG = 1024 * 1024 + module.exports = ArchiveManager = extractZipArchive: (source, destination, _callback = (err) ->) -> callback = (args...) -> _callback(args...) _callback = () -> - timer = new metrics.Timer("unzipDirectory") - logger.log source: source, destination: destination, "unzipping file" + child.exec "unzip -l #{source} | tail -n 1", (err, result)-> + if err? + logger.err err:err, "error checking size of zip file" + return callback(err) - unzip = child.spawn("unzip", [source, "-d", destination]) + totalSizeInBytes = result.trim()?.split(" ")?[0] + + if !totalSizeInBytes? + logger.err source:source, "error getting bytes of zip" + return callback(new Error("something went wrong")) - # don't remove this line, some zips need - # us to listen on this for some unknow reason - unzip.stdout.on "data", (d)-> + if totalSizeInBytes > ONE_MEG * 300 + logger.log source:source, totalSizeInBytes:totalSizeInBytes, "zip file too large" + return callback(new Error("zip_too_large")) + - error = null - unzip.stderr.on "data", (chunk) -> - error ||= "" - error += chunk - unzip.on "error", (err) -> - logger.error {err, source, destination}, "unzip failed" - if err.code == "ENOENT" - logger.error "unzip command not found. Please check the unzip command is installed" - callback(err) + timer = new metrics.Timer("unzipDirectory") + logger.log source: source, destination: destination, "unzipping file" - unzip.on "exit", () -> - timer.done() - if error? - error = new Error(error) - logger.error err:error, source: source, destination: destination, "error unzipping file" - callback(error) + unzip = child.spawn("unzip", [source, "-d", destination]) + + # don't remove this line, some zips need + # us to listen on this for some unknow reason + unzip.stdout.on "data", (d)-> + + error = null + unzip.stderr.on "data", (chunk) -> + error ||= "" + error += chunk + + unzip.on "error", (err) -> + logger.error {err, source, destination}, "unzip failed" + if err.code == "ENOENT" + logger.error "unzip command not found. Please check the unzip command is installed" + callback(err) + + unzip.on "exit", () -> + timer.done() + if error? + error = new Error(error) + logger.error err:error, source: source, destination: destination, "error unzipping file" + callback(error) findTopLevelDirectory: (directory, callback = (error, topLevelDir) ->) -> fs.readdir directory, (error, files) -> diff --git a/services/web/test/UnitTests/coffee/Uploads/ArchiveManagerTests.coffee b/services/web/test/UnitTests/coffee/Uploads/ArchiveManagerTests.coffee index 079ab6d099..f1547d0413 100644 --- a/services/web/test/UnitTests/coffee/Uploads/ArchiveManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Uploads/ArchiveManagerTests.coffee @@ -15,6 +15,7 @@ describe "ArchiveManager", -> @process.stderr = new events.EventEmitter @child = spawn: sinon.stub().returns(@process) + exec: sinon.stub().callsArgWith(1, null, " 109042 2 files") @metrics = Timer: class Timer done: sinon.stub() @@ -58,6 +59,19 @@ describe "ArchiveManager", -> it "should log out the error", -> @logger.error.called.should.equal true + describe "with a zip that is too large", -> + beforeEach (done) -> + @child.exec = sinon.stub().callsArgWith(1, null, " 10000000000009042 2 files") + @ArchiveManager.extractZipArchive @source, @destination, (error) => + @callback(error) + done() + + it "should return the callback with an error", -> + @callback.calledWithExactly(new Error("zip_too_large")).should.equal true + + it "should not call spawn", -> + @child.spawn.called.should.equal false + describe "with an error on the process", -> beforeEach (done) -> @ArchiveManager.extractZipArchive @source, @destination, (error) => From f11ba9738935efc9606e534c0fd418079477864a Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Sat, 12 Mar 2016 15:05:29 +0000 Subject: [PATCH 3/4] check stat.isFile and isDirectory rather then symlink --- .../FileStore/FileStoreHandler.coffee | 11 ++++---- .../Uploads/FileSystemImportManager.coffee | 26 +++++++++---------- .../FileStore/FileStoreHandlerTests.coffee | 9 ++++--- .../FileSystemImportManagerTests.coffee | 26 +++++++++---------- 4 files changed, 38 insertions(+), 34 deletions(-) diff --git a/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee b/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee index 20af15676f..09790cf99f 100644 --- a/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee +++ b/services/web/app/coffee/Features/FileStore/FileStoreHandler.coffee @@ -11,11 +11,12 @@ module.exports = FileStoreHandler = uploadFileFromDisk: (project_id, file_id, fsPath, callback)-> fs.lstat fsPath, (err, stat)-> if err? - logger.err err:err, "error with path symlink check" - return callback(err) - if stat.isSymbolicLink() - logger.log project_id:project_id, file_id:file_id, fsPath:fsPath, "error uploading file from disk, file path is symlink" - return callback('file is from symlink') + logger.err err:err, project_id:project_id, file_id:file_id, fsPath:fsPath, "error stating file" + callback(err) + if !stat.isFile() + logger.log project_id:project_id, file_id:file_id, fsPath:fsPath, "tried to upload symlink, not contining" + return callback(new Error("can not upload symlink")) + logger.log project_id:project_id, file_id:file_id, fsPath:fsPath, "uploading file from disk" readStream = fs.createReadStream(fsPath) opts = diff --git a/services/web/app/coffee/Features/Uploads/FileSystemImportManager.coffee b/services/web/app/coffee/Features/Uploads/FileSystemImportManager.coffee index a8b9f639fe..04c7c3bcc7 100644 --- a/services/web/app/coffee/Features/Uploads/FileSystemImportManager.coffee +++ b/services/web/app/coffee/Features/Uploads/FileSystemImportManager.coffee @@ -8,8 +8,8 @@ logger = require("logger-sharelatex") module.exports = FileSystemImportManager = addDoc: (user_id, project_id, folder_id, name, path, replace, callback = (error, doc)-> )-> - FileSystemImportManager._isSymLink path, (err, isSymLink)-> - if isSymLink + FileSystemImportManager._isSafeOnFileSystem path, (err, isSafe)-> + if !isSafe logger.log user_id:user_id, project_id:project_id, folder_id:folder_id, name:name, path:path, "add doc is from symlink, stopping process" return callback("path is symlink") fs.readFile path, "utf8", (error, content = "") -> @@ -33,8 +33,8 @@ module.exports = FileSystemImportManager = EditorController.addDocWithoutLock project_id, folder_id, name, lines, "upload", callback addFile: (user_id, project_id, folder_id, name, path, replace, callback = (error, file)-> )-> - FileSystemImportManager._isSymLink path, (err, isSymLink)-> - if isSymLink + FileSystemImportManager._isSafeOnFileSystem path, (err, isSafe)-> + if !isSafe logger.log user_id:user_id, project_id:project_id, folder_id:folder_id, name:name, path:path, "add file is from symlink, stopping insert" return callback("path is symlink") @@ -55,8 +55,8 @@ module.exports = FileSystemImportManager = EditorController.addFileWithoutLock project_id, folder_id, name, path, "upload", callback addFolder: (user_id, project_id, folder_id, name, path, replace, callback = (error)-> ) -> - FileSystemImportManager._isSymLink path, (err, isSymLink)-> - if isSymLink + FileSystemImportManager._isSafeOnFileSystem path, (err, isSafe)-> + if !isSafe logger.log user_id:user_id, project_id:project_id, folder_id:folder_id, path:path, "add folder is from symlink, stopping insert" return callback("path is symlink") EditorController.addFolderWithoutLock project_id, folder_id, name, "upload", (error, new_folder) => @@ -66,8 +66,8 @@ module.exports = FileSystemImportManager = callback null, new_folder addFolderContents: (user_id, project_id, parent_folder_id, folderPath, replace, callback = (error)-> ) -> - FileSystemImportManager._isSymLink folderPath, (err, isSymLink)-> - if isSymLink + FileSystemImportManager._isSafeOnFileSystem folderPath, (err, isSafe)-> + if !isSafe logger.log user_id:user_id, project_id:project_id, parent_folder_id:parent_folder_id, folderPath:folderPath, "add folder contents is from symlink, stopping insert" return callback("path is symlink") fs.readdir folderPath, (error, entries = []) => @@ -83,8 +83,8 @@ module.exports = FileSystemImportManager = async.parallelLimit jobs, 5, callback addEntity: (user_id, project_id, folder_id, name, path, replace, callback = (error, entity)-> ) -> - FileSystemImportManager._isSymLink path, (err, isSymLink)-> - if isSymLink + FileSystemImportManager._isSafeOnFileSystem path, (err, isSafe)-> + if !isSafe logger.log user_id:user_id, project_id:project_id, folder_id:folder_id, path:path, "add entry is from symlink, stopping insert" return callback("path is symlink") @@ -101,11 +101,11 @@ module.exports = FileSystemImportManager = FileSystemImportManager.addDoc user_id, project_id, folder_id, name, path, replace, callback - _isSymLink: (path, callback = (err, isSymLink)->)-> + _isSafeOnFileSystem: (path, callback = (err, isSafe)->)-> fs.lstat path, (err, stat)-> if err? logger.err err:err, "error with path symlink check" return callback(err) - isSymLink = stat.isSymbolicLink() - callback(err, isSymLink) + isSafe = stat.isFile() or stat.isDirectory() + callback(err, isSafe) diff --git a/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee b/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee index c35eefcf87..8f9d8a555e 100644 --- a/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/FileStore/FileStoreHandlerTests.coffee @@ -10,7 +10,10 @@ describe "FileStoreHandler", -> beforeEach -> @fs = createReadStream : sinon.stub() - lstat: sinon.stub().callsArgWith(1, null, {isSymbolicLink:=>@isSymlink}) + lstat: sinon.stub().callsArgWith(1, null, { + isFile:=> @isSafeOnFileSystem + isDirectory:-> return false + }) @writeStream = my:"writeStream" on: (type, cb)-> @@ -32,7 +35,7 @@ describe "FileStoreHandler", -> describe "uploadFileFromDisk", -> beforeEach -> @request.returns(@writeStream) - @isSymlink = false + @isSafeOnFileSystem = true it "should create read stream", (done)-> @fs.createReadStream.returns @@ -78,7 +81,7 @@ describe "FileStoreHandler", -> describe "symlink", -> it "should not read file if it is symlink", (done)-> - @isSymlink = true + @isSafeOnFileSystem = false @handler.uploadFileFromDisk @project_id, @file_id, @fsPath, => @fs.createReadStream.called.should.equal false done() diff --git a/services/web/test/UnitTests/coffee/Uploads/FileSystemImportManagerTests.coffee b/services/web/test/UnitTests/coffee/Uploads/FileSystemImportManagerTests.coffee index 4fbea218bb..33013be91b 100644 --- a/services/web/test/UnitTests/coffee/Uploads/FileSystemImportManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Uploads/FileSystemImportManagerTests.coffee @@ -27,12 +27,12 @@ describe "FileSystemImportManager", -> @docContent = "one\ntwo\nthree" @docLines = @docContent.split("\n") @fs.readFile = sinon.stub().callsArgWith(2, null, @docContent) - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) describe "when path is symlink", -> beforeEach -> - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, true) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, false) @EditorController.addDocWithoutLock = sinon.stub() @FileSystemImportManager.addDoc @user_id, @project_id, @folder_id, @name, @path_on_disk, false, @callback @@ -115,7 +115,7 @@ describe "FileSystemImportManager", -> describe "addFile with replace set to false", -> beforeEach -> @EditorController.addFileWithoutLock = sinon.stub().callsArg(5) - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) @FileSystemImportManager.addFile @user_id, @project_id, @folder_id, @name, @path_on_disk, false, @callback it "should add the file", -> @@ -125,7 +125,7 @@ describe "FileSystemImportManager", -> describe "addFile with symlink", -> beforeEach -> @EditorController.addFileWithoutLock = sinon.stub() - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, true) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, false) @EditorController.replaceFile = sinon.stub() @FileSystemImportManager.addFile @user_id, @project_id, @folder_id, @name, @path_on_disk, false, @callback @@ -142,7 +142,7 @@ describe "FileSystemImportManager", -> name: "not-the-right-file.tex" }] } - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) @ProjectLocator.findElement = sinon.stub().callsArgWith(1, null, @folder) @EditorController.addFileWithoutLock = sinon.stub().callsArg(5) @FileSystemImportManager.addFile @user_id, @project_id, @folder_id, @name, @path_on_disk, true, @callback @@ -167,7 +167,7 @@ describe "FileSystemImportManager", -> name: "not-the-right-file.tex" }] } - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) @ProjectLocator.findElement = sinon.stub().callsArgWith(1, null, @folder) @EditorController.replaceFile = sinon.stub().callsArg(4) @FileSystemImportManager.addFile @user_id, @project_id, @folder_id, @name, @path_on_disk, true, @callback @@ -190,7 +190,7 @@ describe "FileSystemImportManager", -> describe "successfully", -> beforeEach -> - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) @FileSystemImportManager.addFolder @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback it "should add a folder to the project", -> @@ -203,7 +203,7 @@ describe "FileSystemImportManager", -> describe "with symlink", -> beforeEach -> - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, true) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, false) @FileSystemImportManager.addFolder @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback it "should not add a folder to the project", -> @@ -218,7 +218,7 @@ describe "FileSystemImportManager", -> @FileSystemImportManager.addEntity = sinon.stub().callsArg(6) @FileTypeManager.shouldIgnore = (path, callback) => callback null, @ignoredEntries.indexOf(require("path").basename(path)) != -1 - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) @FileSystemImportManager.addFolderContents @user_id, @project_id, @folder_id, @path_on_disk, @replace, @callback it "should call addEntity for each file in the folder which is not ignored", -> @@ -234,12 +234,12 @@ describe "FileSystemImportManager", -> it "should look in the correct directory", -> @fs.readdir.calledWith(@path_on_disk).should.equal true - describe "addEntity dddddddd", -> + describe "addEntity", -> describe "with directory", -> beforeEach -> @FileTypeManager.isDirectory = sinon.stub().callsArgWith(1, null, true) @FileSystemImportManager.addFolder = sinon.stub().callsArg(6) - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) @FileSystemImportManager.addEntity @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback it "should call addFolder", -> @@ -250,7 +250,7 @@ describe "FileSystemImportManager", -> beforeEach -> @FileTypeManager.isDirectory = sinon.stub().callsArgWith(1, null, false) @FileTypeManager.isBinary = sinon.stub().callsArgWith(2, null, true) - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) @FileSystemImportManager.addFile = sinon.stub().callsArg(6) @FileSystemImportManager.addEntity @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback @@ -263,7 +263,7 @@ describe "FileSystemImportManager", -> @FileTypeManager.isDirectory = sinon.stub().callsArgWith(1, null, false) @FileTypeManager.isBinary = sinon.stub().callsArgWith(2, null, false) @FileSystemImportManager.addDoc = sinon.stub().callsArg(6) - @FileSystemImportManager._isSymLink = sinon.stub().callsArgWith(1, null, false) + @FileSystemImportManager._isSafeOnFileSystem = sinon.stub().callsArgWith(1, null, true) @FileSystemImportManager.addEntity @user_id, @project_id, @folder_id, @name, @path_on_disk, @replace, @callback it "should call addFile", -> From 8812ff445e385694c10166371988f2e58fa94e76 Mon Sep 17 00:00:00 2001 From: Henry Oswald Date: Sat, 12 Mar 2016 15:43:16 +0000 Subject: [PATCH 4/4] change zip size check to spawn --- .../Features/Uploads/ArchiveManager.coffee | 60 +++++++++++++++---- .../coffee/Uploads/ArchiveManagerTests.coffee | 48 ++++++++++++++- 2 files changed, 95 insertions(+), 13 deletions(-) diff --git a/services/web/app/coffee/Features/Uploads/ArchiveManager.coffee b/services/web/app/coffee/Features/Uploads/ArchiveManager.coffee index 74f6108ba0..a615810fe6 100644 --- a/services/web/app/coffee/Features/Uploads/ArchiveManager.coffee +++ b/services/web/app/coffee/Features/Uploads/ArchiveManager.coffee @@ -3,30 +3,68 @@ logger = require "logger-sharelatex" metrics = require "../../infrastructure/Metrics" fs = require "fs" Path = require "path" +_ = require("underscore") ONE_MEG = 1024 * 1024 module.exports = ArchiveManager = + + + _isZipTooLarge: (source, callback = (err, isTooLarge)->)-> + callback = _.once callback + + unzip = child.spawn("unzip", ["-l", source]) + + output = "" + unzip.stdout.on "data", (d)-> + output += d + + error = null + unzip.stderr.on "data", (chunk) -> + error ||= "" + error += chunk + + unzip.on "error", (err) -> + logger.error {err, source, destination}, "unzip failed" + if err.code == "ENOENT" + logger.error "unzip command not found. Please check the unzip command is installed" + callback(err) + + unzip.on "exit", () -> + if error? + error = new Error(error) + logger.error err:error, source: source, destination: destination, "error checking zip size" + + lines = output.split("\n") + lastLine = lines[lines.length - 2]?.trim() + totalSizeInBytes = lastLine?.split(" ")?[0] + + totalSizeInBytes = parseInt(totalSizeInBytes) + + if !totalSizeInBytes? or isNaN(totalSizeInBytes) + logger.err source:source, "error getting bytes of zip" + return callback(new Error("something went wrong")) + + isTooLarge = totalSizeInBytes > (ONE_MEG * 300) + + callback(error, isTooLarge) + + + + + extractZipArchive: (source, destination, _callback = (err) ->) -> callback = (args...) -> _callback(args...) _callback = () -> - child.exec "unzip -l #{source} | tail -n 1", (err, result)-> + ArchiveManager._isZipTooLarge source, (err, isTooLarge)-> if err? logger.err err:err, "error checking size of zip file" return callback(err) - totalSizeInBytes = result.trim()?.split(" ")?[0] - - if !totalSizeInBytes? - logger.err source:source, "error getting bytes of zip" - return callback(new Error("something went wrong")) - - if totalSizeInBytes > ONE_MEG * 300 - logger.log source:source, totalSizeInBytes:totalSizeInBytes, "zip file too large" - return callback(new Error("zip_too_large")) - + if isTooLarge + return callback(new Error("zip_too_large")) timer = new metrics.Timer("unzipDirectory") diff --git a/services/web/test/UnitTests/coffee/Uploads/ArchiveManagerTests.coffee b/services/web/test/UnitTests/coffee/Uploads/ArchiveManagerTests.coffee index f1547d0413..89b14c78c9 100644 --- a/services/web/test/UnitTests/coffee/Uploads/ArchiveManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Uploads/ArchiveManagerTests.coffee @@ -1,4 +1,5 @@ sinon = require('sinon') +expect = require("chai").expect chai = require('chai') should = chai.should() modulePath = "../../../../app/js/Features/Uploads/ArchiveManager.js" @@ -9,13 +10,16 @@ describe "ArchiveManager", -> beforeEach -> @logger = error: sinon.stub() + err:-> log: sinon.stub() @process = new events.EventEmitter @process.stdout = new events.EventEmitter @process.stderr = new events.EventEmitter + @child = spawn: sinon.stub().returns(@process) - exec: sinon.stub().callsArgWith(1, null, " 109042 2 files") + + @metrics = Timer: class Timer done: sinon.stub() @@ -30,6 +34,7 @@ describe "ArchiveManager", -> @source = "/path/to/zip/source.zip" @destination = "/path/to/zip/destination" @callback = sinon.stub() + @ArchiveManager._isZipTooLarge = sinon.stub().callsArgWith(1, null, false) describe "successfully", -> beforeEach (done) -> @@ -61,7 +66,7 @@ describe "ArchiveManager", -> describe "with a zip that is too large", -> beforeEach (done) -> - @child.exec = sinon.stub().callsArgWith(1, null, " 10000000000009042 2 files") + @ArchiveManager._isZipTooLarge = sinon.stub().callsArgWith(1, null, true) @ArchiveManager.extractZipArchive @source, @destination, (error) => @callback(error) done() @@ -85,6 +90,45 @@ describe "ArchiveManager", -> it "should log out the error", -> @logger.error.called.should.equal true + describe "_isZipTooLarge", -> + beforeEach -> + @output = (totalSize)->" Length Date Time Name \n-------- ---- ---- ---- \n241 03-12-16 12:20 main.tex \n108801 03-12-16 12:20 ddd/x1J5kHh.jpg \n-------- ------- \n#{totalSize} 2 files\n" + + it "should return false with small output", (done)-> + @ArchiveManager._isZipTooLarge @source, (error, isTooLarge) => + isTooLarge.should.equal false + done() + @process.stdout.emit "data", @output("109042") + @process.emit "exit" + + it "should return true with large bytes", (done)-> + @ArchiveManager._isZipTooLarge @source, (error, isTooLarge) => + isTooLarge.should.equal true + done() + @process.stdout.emit "data", @output("1090000000000000042") + @process.emit "exit" + + it "should return error on no data", (done)-> + @ArchiveManager._isZipTooLarge @source, (error, isTooLarge) => + expect(error).to.exist + done() + @process.stdout.emit "data", "" + @process.emit "exit" + + it "should return error if it didn't get a number", (done)-> + @ArchiveManager._isZipTooLarge @source, (error, isTooLarge) => + expect(error).to.exist + done() + @process.stdout.emit "data", @output("total_size_string") + @process.emit "exit" + + it "should return error if the is only a bit of data", (done)-> + @ArchiveManager._isZipTooLarge @source, (error, isTooLarge) => + expect(error).to.exist + done() + @process.stdout.emit "data", " Length Date Time Name \n--------" + @process.emit "exit" + describe "findTopLevelDirectory", -> beforeEach -> @fs.readdir = sinon.stub()