From 81e824382764fb1307102e4fcf17a5e7033bb4c8 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/services/clsi/app/coffee/ResourceStateManager.coffee b/services/clsi/app/coffee/ResourceStateManager.coffee index b894701a6a..e250e6447c 100644 --- a/services/clsi/app/coffee/ResourceStateManager.coffee +++ b/services/clsi/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 b03271edee0e2aac188a28ccf6c042f1b363b229 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 services/clsi/test/unit/coffee/ResourceStateManagerTests.coffee diff --git a/services/clsi/test/unit/coffee/ResourceStateManagerTests.coffee b/services/clsi/test/unit/coffee/ResourceStateManagerTests.coffee new file mode 100644 index 0000000000..6c55a1d0be --- /dev/null +++ b/services/clsi/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 d46943a7bb9fe3add4694b004f7cf4e87c5c33c6 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 --- services/clsi/app/coffee/OutputFileFinder.coffee | 6 ++++-- services/clsi/app/coffee/ResourceWriter.coffee | 2 -- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/services/clsi/app/coffee/OutputFileFinder.coffee b/services/clsi/app/coffee/OutputFileFinder.coffee index 80bd07f2cb..4b07f6e13a 100644 --- a/services/clsi/app/coffee/OutputFileFinder.coffee +++ b/services/clsi/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/services/clsi/app/coffee/ResourceWriter.coffee b/services/clsi/app/coffee/ResourceWriter.coffee index f9e90b036a..55970ee850 100644 --- a/services/clsi/app/coffee/ResourceWriter.coffee +++ b/services/clsi/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 2a23082c4e899dfa89ffd0e5f82893d644838c4f 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/services/clsi/app/coffee/ResourceStateManager.coffee b/services/clsi/app/coffee/ResourceStateManager.coffee index e250e6447c..fbd4c67736 100644 --- a/services/clsi/app/coffee/ResourceStateManager.coffee +++ b/services/clsi/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/services/clsi/test/unit/coffee/ResourceStateManagerTests.coffee b/services/clsi/test/unit/coffee/ResourceStateManagerTests.coffee index 6c55a1d0be..e5e1c13011 100644 --- a/services/clsi/test/unit/coffee/ResourceStateManagerTests.coffee +++ b/services/clsi/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 ef0db811e195f269af33f0b83b5159319026dff4 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 --- services/clsi/app/coffee/OutputCacheManager.coffee | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/services/clsi/app/coffee/OutputCacheManager.coffee b/services/clsi/app/coffee/OutputCacheManager.coffee index 465b0431d9..41786af905 100644 --- a/services/clsi/app/coffee/OutputCacheManager.coffee +++ b/services/clsi/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 a7cb7e6e4c46d4b0974fd728c0fa1572279adcfa 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 --- services/clsi/app/coffee/OutputCacheManager.coffee | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/clsi/app/coffee/OutputCacheManager.coffee b/services/clsi/app/coffee/OutputCacheManager.coffee index 41786af905..23e179c425 100644 --- a/services/clsi/app/coffee/OutputCacheManager.coffee +++ b/services/clsi/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) ->