From aa5eeb0903222116238b66219de8177422531327 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 15 Sep 2017 13:41:56 +0100 Subject: [PATCH 1/6] fallback check for missing files dot files are not examined by OutputFileFinder, so do an extra check to make sure those exist also check for any relative paths in the resources --- app/coffee/ResourceStateManager.coffee | 30 ++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/app/coffee/ResourceStateManager.coffee b/app/coffee/ResourceStateManager.coffee index b894701..e250e64 100644 --- a/app/coffee/ResourceStateManager.coffee +++ b/app/coffee/ResourceStateManager.coffee @@ -4,6 +4,7 @@ logger = require "logger-sharelatex" settings = require("settings-sharelatex") Errors = require "./Errors" SafeReader = require "./SafeReader" +async = require "async" module.exports = ResourceStateManager = @@ -50,14 +51,31 @@ module.exports = ResourceStateManager = resources = ({path: path} for path in resourceList) callback(null, resources) - checkResourceFiles: (resources, allFiles, directory, callback = (error) ->) -> + checkResourceFiles: (resources, allFiles, basePath, callback = (error) ->) -> + # check the paths are all relative to current directory + for file in resources or [] + for dir in file?.path?.split('/') + if dir == '..' + return callback new Error("relative path in resource file list") # check if any of the input files are not present in list of files seenFile = {} for file in allFiles seenFile[file] = true - missingFiles = (resource.path for resource in resources when not seenFile[resource.path]) - if missingFiles.length > 0 - logger.err missingFiles:missingFiles, dir:directory, allFiles:allFiles, resources:resources, "missing input files for project" - return callback new Errors.FilesOutOfSyncError("resource files missing in incremental update") + missingFileCandidates = (resource.path for resource in resources when not seenFile[resource.path]) + # now check if they are really missing + ResourceStateManager._checkMissingFiles missingFileCandidates, basePath, (missingFiles) -> + if missingFiles?.length > 0 + logger.err missingFiles:missingFiles, basePath:basePath, allFiles:allFiles, resources:resources, "missing input files for project" + return callback new Errors.FilesOutOfSyncError("resource files missing in incremental update") + else + callback() + + _checkMissingFiles: (missingFileCandidates, basePath, callback = (missingFiles) ->) -> + if missingFileCandidates.length > 0 + fileDoesNotExist = (file, cb) -> + fs.stat Path.join(basePath, file), (err) -> + logger.log file:file, err:err, result: err?, "stating potential missing file" + cb(err?) + async.filterSeries missingFileCandidates, fileDoesNotExist, callback else - callback() + callback([]) From 8305268848b5316dcbc2622a4c69a2811d7d595c Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 15 Sep 2017 13:42:57 +0100 Subject: [PATCH 2/6] unit tests for ResourceStateManager --- .../coffee/ResourceStateManagerTests.coffee | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 test/unit/coffee/ResourceStateManagerTests.coffee diff --git a/test/unit/coffee/ResourceStateManagerTests.coffee b/test/unit/coffee/ResourceStateManagerTests.coffee new file mode 100644 index 0000000..6c55a1d --- /dev/null +++ b/test/unit/coffee/ResourceStateManagerTests.coffee @@ -0,0 +1,124 @@ +SandboxedModule = require('sandboxed-module') +sinon = require('sinon') +should = require('chai').should() +modulePath = require('path').join __dirname, '../../../app/js/ResourceStateManager' +Path = require "path" +Errors = require "../../../app/js/Errors" + +describe "ResourceStateManager", -> + beforeEach -> + @ResourceStateManager = SandboxedModule.require modulePath, requires: + "fs": @fs = {} + "logger-sharelatex": {log: sinon.stub(), err: sinon.stub()} + "./SafeReader": @SafeReader = {} + @basePath = "/path/to/write/files/to" + @resources = [ + {path: "resource-1-mock"} + {path: "resource-2-mock"} + {path: "resource-3-mock"} + ] + @state = "1234567890" + @resourceFileName = "#{@basePath}/.project-sync-state" + @resourceFileContents = "#{@resources[0].path}\n#{@resources[1].path}\n#{@resources[2].path}\nstateHash:#{@state}" + @callback = sinon.stub() + + describe "saveProjectState", -> + beforeEach -> + @fs.writeFile = sinon.stub().callsArg(2) + + describe "when the state is specified", -> + beforeEach -> + @ResourceStateManager.saveProjectState(@state, @resources, @basePath, @callback) + + it "should write the resource list to disk", -> + @fs.writeFile + .calledWith(@resourceFileName, @resourceFileContents) + .should.equal true + + it "should call the callback", -> + @callback.called.should.equal true + + describe "when the state is undefined", -> + beforeEach -> + @state = undefined + @fs.unlink = sinon.stub().callsArg(1) + @ResourceStateManager.saveProjectState(@state, @resources, @basePath, @callback) + + it "should unlink the resource file", -> + @fs.unlink + .calledWith(@resourceFileName) + .should.equal true + + it "should not write the resource list to disk", -> + @fs.writeFile.called.should.equal false + + it "should call the callback", -> + @callback.called.should.equal true + + describe "checkProjectStateMatches", -> + + describe "when the state matches", -> + beforeEach -> + @SafeReader.readFile = sinon.stub().callsArgWith(3, null, @resourceFileContents) + @ResourceStateManager.checkProjectStateMatches(@state, @basePath, @callback) + + it "should read the resource file", -> + @SafeReader.readFile + .calledWith(@resourceFileName) + .should.equal true + + it "should call the callback with the results", -> + @callback.calledWithMatch(null, @resources).should.equal true + + describe "when the state does not match", -> + beforeEach -> + @SafeReader.readFile = sinon.stub().callsArgWith(3, null, @resourceFileContents) + @ResourceStateManager.checkProjectStateMatches("not-the-original-state", @basePath, @callback) + + it "should call the callback with an error", -> + error = new Errors.FilesOutOfSyncError("invalid state for incremental update") + @callback.calledWith(error).should.equal true + + describe "checkResourceFiles", -> + describe "when all the files are present", -> + beforeEach -> + @allFiles = [ @resources[0].path, @resources[1].path, @resources[2].path] + @ResourceStateManager.checkResourceFiles(@resources, @allFiles, @basePath, @callback) + + it "should call the callback", -> + @callback.calledWithExactly().should.equal true + + describe "when there is a file missing from the outputFileFinder but present on disk", -> + beforeEach -> + @allFiles = [ @resources[0].path, @resources[1].path] + @fs.stat = sinon.stub().callsArg(1) + @ResourceStateManager.checkResourceFiles(@resources, @allFiles, @basePath, @callback) + + it "should stat the file to see if it is present", -> + @fs.stat.called.should.equal true + + it "should call the callback", -> + @callback.calledWithExactly().should.equal true + + describe "when there is a missing file", -> + beforeEach -> + @allFiles = [ @resources[0].path, @resources[1].path] + @fs.stat = sinon.stub().callsArgWith(1, new Error()) + @ResourceStateManager.checkResourceFiles(@resources, @allFiles, @basePath, @callback) + + it "should stat the file to see if it is present", -> + @fs.stat.called.should.equal true + + it "should call the callback with an error", -> + error = new Errors.FilesOutOfSyncError("resource files missing in incremental update") + @callback.calledWith(error).should.equal true + + describe "when a resource contains a relative path", -> + beforeEach -> + @resources[0].path = "../foo/bar.tex" + @allFiles = [ @resources[0].path, @resources[1].path, @resources[2].path] + @ResourceStateManager.checkResourceFiles(@resources, @allFiles, @basePath, @callback) + + it "should call the callback with an error", -> + @callback.calledWith(new Error("relative path in resource file list")).should.equal true + From 0930b1cd8f4f2a8c3dca70fe406c419746a45648 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 26 Sep 2017 09:47:29 +0100 Subject: [PATCH 3/6] only exclude clsi-specific files from output list --- app/coffee/OutputFileFinder.coffee | 6 ++++-- app/coffee/ResourceWriter.coffee | 2 -- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/coffee/OutputFileFinder.coffee b/app/coffee/OutputFileFinder.coffee index 80bd07f..4b07f6e 100644 --- a/app/coffee/OutputFileFinder.coffee +++ b/app/coffee/OutputFileFinder.coffee @@ -29,8 +29,10 @@ module.exports = OutputFileFinder = callback = (error, fileList) -> _callback(error, fileList) _callback = () -> - - args = [directory, "-name", ".*", "-prune", "-o", "-type", "f", "-print"] + + # don't include clsi-specific files/directories in the output list + EXCLUDE_DIRS = ["-name", ".cache", "-o", "-name", ".archive","-o", "-name", ".project-*"] + args = [directory, "(", EXCLUDE_DIRS..., ")", "-prune", "-o", "-type", "f", "-print"] logger.log args: args, "running find command" proc = spawn("find", args) diff --git a/app/coffee/ResourceWriter.coffee b/app/coffee/ResourceWriter.coffee index f9e90b0..55970ee 100644 --- a/app/coffee/ResourceWriter.coffee +++ b/app/coffee/ResourceWriter.coffee @@ -78,8 +78,6 @@ module.exports = ResourceWriter = should_delete = true if path.match(/^output\./) or path.match(/\.aux$/) or path.match(/^cache\//) # knitr cache should_delete = false - if path == '.project-sync-state' - should_delete = false if path == "output.pdf" or path == "output.dvi" or path == "output.log" or path == "output.xdv" should_delete = true if path == "output.tex" # created by TikzManager if present in output files From f11468b5955dd16901954c5fff1591a564e4b636 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 26 Sep 2017 09:48:09 +0100 Subject: [PATCH 4/6] remove stat test for missing files --- app/coffee/ResourceStateManager.coffee | 23 ++++--------------- .../coffee/ResourceStateManagerTests.coffee | 15 ------------ 2 files changed, 5 insertions(+), 33 deletions(-) diff --git a/app/coffee/ResourceStateManager.coffee b/app/coffee/ResourceStateManager.coffee index e250e64..fbd4c67 100644 --- a/app/coffee/ResourceStateManager.coffee +++ b/app/coffee/ResourceStateManager.coffee @@ -4,7 +4,6 @@ logger = require "logger-sharelatex" settings = require("settings-sharelatex") Errors = require "./Errors" SafeReader = require "./SafeReader" -async = require "async" module.exports = ResourceStateManager = @@ -61,21 +60,9 @@ module.exports = ResourceStateManager = seenFile = {} for file in allFiles seenFile[file] = true - missingFileCandidates = (resource.path for resource in resources when not seenFile[resource.path]) - # now check if they are really missing - ResourceStateManager._checkMissingFiles missingFileCandidates, basePath, (missingFiles) -> - if missingFiles?.length > 0 - logger.err missingFiles:missingFiles, basePath:basePath, allFiles:allFiles, resources:resources, "missing input files for project" - return callback new Errors.FilesOutOfSyncError("resource files missing in incremental update") - else - callback() - - _checkMissingFiles: (missingFileCandidates, basePath, callback = (missingFiles) ->) -> - if missingFileCandidates.length > 0 - fileDoesNotExist = (file, cb) -> - fs.stat Path.join(basePath, file), (err) -> - logger.log file:file, err:err, result: err?, "stating potential missing file" - cb(err?) - async.filterSeries missingFileCandidates, fileDoesNotExist, callback + missingFiles = (resource.path for resource in resources when not seenFile[resource.path]) + if missingFiles?.length > 0 + logger.err missingFiles:missingFiles, basePath:basePath, allFiles:allFiles, resources:resources, "missing input files for project" + return callback new Errors.FilesOutOfSyncError("resource files missing in incremental update") else - callback([]) + callback() diff --git a/test/unit/coffee/ResourceStateManagerTests.coffee b/test/unit/coffee/ResourceStateManagerTests.coffee index 6c55a1d..e5e1c13 100644 --- a/test/unit/coffee/ResourceStateManagerTests.coffee +++ b/test/unit/coffee/ResourceStateManagerTests.coffee @@ -88,27 +88,12 @@ describe "ResourceStateManager", -> it "should call the callback", -> @callback.calledWithExactly().should.equal true - describe "when there is a file missing from the outputFileFinder but present on disk", -> - beforeEach -> - @allFiles = [ @resources[0].path, @resources[1].path] - @fs.stat = sinon.stub().callsArg(1) - @ResourceStateManager.checkResourceFiles(@resources, @allFiles, @basePath, @callback) - - it "should stat the file to see if it is present", -> - @fs.stat.called.should.equal true - - it "should call the callback", -> - @callback.calledWithExactly().should.equal true - describe "when there is a missing file", -> beforeEach -> @allFiles = [ @resources[0].path, @resources[1].path] @fs.stat = sinon.stub().callsArgWith(1, new Error()) @ResourceStateManager.checkResourceFiles(@resources, @allFiles, @basePath, @callback) - it "should stat the file to see if it is present", -> - @fs.stat.called.should.equal true - it "should call the callback with an error", -> error = new Errors.FilesOutOfSyncError("resource files missing in incremental update") @callback.calledWith(error).should.equal true From dbeff9a7b8b960801d3dd3b575fbca63df3a74de Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 26 Sep 2017 10:42:59 +0100 Subject: [PATCH 5/6] exclude hidden files from output express static server doesn't serve them and rejects with 404 --- app/coffee/OutputCacheManager.coffee | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/coffee/OutputCacheManager.coffee b/app/coffee/OutputCacheManager.coffee index 465b043..41786af 100644 --- a/app/coffee/OutputCacheManager.coffee +++ b/app/coffee/OutputCacheManager.coffee @@ -63,6 +63,11 @@ module.exports = OutputCacheManager = # copy all the output files into the new cache directory results = [] async.mapSeries outputFiles, (file, cb) -> + # don't send dot files as output, express doesn't serve them + if file?.path?.match(/^\.|\/./) + logger.warn compileDir: compileDir, path: file.path, "ignoring dotfile in output" + return cb() + # copy other files into cache directory if valid newFile = _.clone(file) [src, dst] = [Path.join(compileDir, file.path), Path.join(cacheDir, file.path)] OutputCacheManager._checkFileIsSafe src, (err, isSafe) -> From 23fec681113682e339b52b6bebb8729166778893 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Tue, 26 Sep 2017 11:03:20 +0100 Subject: [PATCH 6/6] use a separate function for hidden file check --- app/coffee/OutputCacheManager.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/coffee/OutputCacheManager.coffee b/app/coffee/OutputCacheManager.coffee index 41786af..23e179c 100644 --- a/app/coffee/OutputCacheManager.coffee +++ b/app/coffee/OutputCacheManager.coffee @@ -64,7 +64,7 @@ module.exports = OutputCacheManager = results = [] async.mapSeries outputFiles, (file, cb) -> # don't send dot files as output, express doesn't serve them - if file?.path?.match(/^\.|\/./) + if OutputCacheManager._fileIsHidden(file.path) logger.warn compileDir: compileDir, path: file.path, "ignoring dotfile in output" return cb() # copy other files into cache directory if valid @@ -149,6 +149,9 @@ module.exports = OutputCacheManager = removeDir dir, cb , callback + _fileIsHidden: (path) -> + return path?.match(/^\.|\/./)? + _checkFileIsSafe: (src, callback = (error, isSafe) ->) -> # check if we have a valid file to copy into the cache fs.stat src, (err, stats) ->