From e66210d2af973ae88b7bbde91a8c2bf15e5f650e Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Sat, 6 Oct 2018 18:09:41 +0100 Subject: [PATCH 1/3] Add method to sanitize full paths For convenience, add a method to SafePath to break a path into components and verify the status of each one. bug: overleaf/sharelatex#908 Signed-off-by: Simon Detheridge --- .../coffee/Features/Project/SafePath.coffee | 9 +++ .../unit/coffee/Project/SafePathTests.coffee | 59 ++++++++++++++++++- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/services/web/app/coffee/Features/Project/SafePath.coffee b/services/web/app/coffee/Features/Project/SafePath.coffee index a77f83ce2b..8684817df3 100644 --- a/services/web/app/coffee/Features/Project/SafePath.coffee +++ b/services/web/app/coffee/Features/Project/SafePath.coffee @@ -67,6 +67,15 @@ load = () -> !BADFILE_RX.test(filename) && !BLOCKEDFILE_RX.test(filename) + isCleanPath: (path) -> + elements = path.split('/') + return false if elements[elements.length - 1].length == 0 + + for element in elements + return false if element.length > 0 && !SafePath.isCleanFilename element + + return true + isAllowedLength: (pathname) -> return pathname.length > 0 && pathname.length <= MAX_PATH diff --git a/services/web/test/unit/coffee/Project/SafePathTests.coffee b/services/web/test/unit/coffee/Project/SafePathTests.coffee index 6e0c55a5bc..afea03fdea 100644 --- a/services/web/test/unit/coffee/Project/SafePathTests.coffee +++ b/services/web/test/unit/coffee/Project/SafePathTests.coffee @@ -83,6 +83,59 @@ describe 'SafePath', -> result = @SafePath.isCleanFilename 'foo\\bar' result.should.equal false + describe 'isCleanPath', -> + it 'should accept a valid filename "main.tex"', -> + result = @SafePath.isCleanPath 'main.tex' + result.should.equal true + + it 'should accept a valid path "foo/main.tex"', -> + result = @SafePath.isCleanPath 'foo/main.tex' + result.should.equal true + + it 'should accept empty path elements', -> + result = @SafePath.isCleanPath 'foo//main.tex' + result.should.equal true + + it 'should not accept an empty filename', -> + result = @SafePath.isCleanPath 'foo/bar/' + result.should.equal false + + it 'should accept a path that starts with a slash', -> + result = @SafePath.isCleanPath '/etc/passwd' + result.should.equal true + + it 'should not accept a path that has an asterisk as the 0th element', -> + result = @SafePath.isCleanPath '*/foo/bar' + result.should.equal false + + it 'should not accept a path that has an asterisk as a middle element', -> + result = @SafePath.isCleanPath 'foo/*/bar' + result.should.equal false + + it 'should not accept a path that has an asterisk as the filename', -> + result = @SafePath.isCleanPath 'foo/bar/*' + result.should.equal false + + it 'should not accept a path that contains an asterisk in the 0th element', -> + result = @SafePath.isCleanPath 'f*o/bar/baz' + result.should.equal false + + it 'should not accept a path that contains an asterisk in a middle element', -> + result = @SafePath.isCleanPath 'foo/b*r/baz' + result.should.equal false + + it 'should not accept a path that contains an asterisk in the filename', -> + result = @SafePath.isCleanPath 'foo/bar/b*z' + result.should.equal false + + it 'should not accept multiple problematic elements', -> + result = @SafePath.isCleanPath 'f*o/b*r/b*z' + result.should.equal false + + it 'should not accept a problematic path with an empty element', -> + result = @SafePath.isCleanPath 'foo//*/bar' + result.should.equal false + describe 'isAllowedLength', -> it 'should accept a valid path "main.tex"', -> result = @SafePath.isAllowedLength 'main.tex' @@ -96,7 +149,7 @@ describe 'SafePath', -> it 'should not accept an empty path', -> result = @SafePath.isAllowedLength '' result.should.equal false - + describe 'clean', -> it 'should not modify a valid filename', -> result = @SafePath.clean 'main.tex' @@ -105,7 +158,7 @@ describe 'SafePath', -> it 'should replace invalid characters with _', -> result = @SafePath.clean 'foo/bar*/main.tex' result.should.equal 'foo_bar__main.tex' - + it 'should replace "." with "_"', -> result = @SafePath.clean '.' result.should.equal '_' @@ -133,7 +186,7 @@ describe 'SafePath', -> it 'should prefix javascript property names with @', -> result = @SafePath.clean 'prototype' result.should.equal '@prototype' - + it 'should prefix javascript property names in the prototype with @', -> result = @SafePath.clean 'hasOwnProperty' result.should.equal '@hasOwnProperty' From 56dcbefb5b4dad0e2aaa6e4ab8311d8acdba004b Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Sat, 6 Oct 2018 18:12:33 +0100 Subject: [PATCH 2/3] Check for safe paths in all ProjectEntityHandler methods Some import mechanisms (for example, Github project import) call methods such as 'upsert*' directly, bypassing existing filename checks. Added checks to all methods in ProjectEntityHandler that can create or rename a file. bug: overleaf/sharelatex#908 Signed-off-by: Simon Detheridge --- .../Project/ProjectEntityUpdateHandler.coffee | 17 + .../coffee/Features/Project/SafePath.coffee | 9 +- .../ProjectEntityUpdateHandlerTests.coffee | 594 ++++++++++++------ 3 files changed, 423 insertions(+), 197 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectEntityUpdateHandler.coffee b/services/web/app/coffee/Features/Project/ProjectEntityUpdateHandler.coffee index b7be80cb47..079065a948 100644 --- a/services/web/app/coffee/Features/Project/ProjectEntityUpdateHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectEntityUpdateHandler.coffee @@ -129,6 +129,8 @@ module.exports = ProjectEntityUpdateHandler = self = Project.update {_id:project_id}, {$unset: {rootDoc_id: true}}, {}, callback addDoc: wrapWithLock (project_id, folder_id, docName, docLines, userId, callback = (error, doc, folder_id) ->)=> + if not SafePath.isCleanFilename docName + return callback new Errors.InvalidNameError("invalid element name") self.addDocWithoutUpdatingHistory.withoutLock project_id, folder_id, docName, docLines, userId, (error, doc, folder_id, path, project) -> return callback(error) if error? projectHistoryId = project.overleaf?.history?.id @@ -166,6 +168,8 @@ module.exports = ProjectEntityUpdateHandler = self = addFile: wrapWithLock beforeLock: (next) -> (project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback) -> + if not SafePath.isCleanFilename fileName + return callback new Errors.InvalidNameError("invalid element name") ProjectEntityUpdateHandler._uploadFile project_id, folder_id, fileName, fsPath, linkedFileData, userId, (error, fileRef, fileStoreUrl) -> return callback(error) if error? next(project_id, folder_id, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback) @@ -241,6 +245,8 @@ module.exports = ProjectEntityUpdateHandler = self = # the history unless you are making sure it is updated in some other way. beforeLock: (next) -> (project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback) -> + if not SafePath.isCleanFilename fileName + return callback(new Errors.InvalidNameError("invalid element name")) ProjectEntityUpdateHandler._uploadFile project_id, folder_id, fileName, fsPath, linkedFileData, userId, (error, fileRef, fileStoreUrl) -> return callback(error) if error? next(project_id, folder_id, fileName, fsPath, linkedFileData, userId, fileRef, fileStoreUrl, callback) @@ -250,6 +256,8 @@ module.exports = ProjectEntityUpdateHandler = self = callback(null, fileRef, folder_id, result?.path?.fileSystem, fileStoreUrl) upsertDoc: wrapWithLock (project_id, folder_id, docName, docLines, source, userId, callback = (err, doc, folder_id, isNewDoc)->)-> + if not SafePath.isCleanFilename docName + return callback new Errors.InvalidNameError("invalid element name") 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? @@ -272,6 +280,8 @@ module.exports = ProjectEntityUpdateHandler = self = upsertFile: wrapWithLock beforeLock: (next) -> (project_id, folder_id, fileName, fsPath, linkedFileData, userId, callback)-> + if not SafePath.isCleanFilename fileName + return callback new Errors.InvalidNameError("invalid element name") # create a new file fileRef = new File( name: fileName @@ -301,6 +311,8 @@ module.exports = ProjectEntityUpdateHandler = self = callback null, newFileRef, !existingFile?, existingFile upsertDocWithPath: wrapWithLock (project_id, elementPath, docLines, source, userId, callback) -> + if not SafePath.isCleanPath elementPath + return callback new Errors.InvalidNameError("invalid element name") docName = path.basename(elementPath) folderPath = path.dirname(elementPath) self.mkdirp.withoutLock project_id, folderPath, (err, newFolders, folder) -> @@ -312,6 +324,8 @@ module.exports = ProjectEntityUpdateHandler = self = upsertFileWithPath: wrapWithLock beforeLock: (next) -> (project_id, elementPath, fsPath, linkedFileData, userId, callback)-> + if not SafePath.isCleanPath elementPath + return callback new Errors.InvalidNameError("invalid element name") fileName = path.basename(elementPath) folderPath = path.dirname(elementPath) # create a new file @@ -351,6 +365,9 @@ module.exports = ProjectEntityUpdateHandler = self = self.deleteEntity.withoutLock project_id, element._id, type, userId, callback mkdirp: wrapWithLock (project_id, path, callback = (err, newlyCreatedFolders, lastFolderInPath)->)-> + for folder in path.split('/') + if folder.length > 0 and not SafePath.isCleanFilename folder + return callback new Errors.InvalidNameError("invalid element name") ProjectEntityMongoUpdateHandler.mkdirp project_id, path, callback addFolder: wrapWithLock (project_id, parentFolder_id, folderName, callback) -> diff --git a/services/web/app/coffee/Features/Project/SafePath.coffee b/services/web/app/coffee/Features/Project/SafePath.coffee index 8684817df3..4e5a288ea9 100644 --- a/services/web/app/coffee/Features/Project/SafePath.coffee +++ b/services/web/app/coffee/Features/Project/SafePath.coffee @@ -52,6 +52,7 @@ load = () -> MAX_PATH = 1024 # Maximum path length, in characters. This is fairly arbitrary. SafePath = + # convert any invalid characters to underscores in the given filename clean: (filename) -> filename = filename.replace BADCHAR_RX, '_' # for BADFILE_RX replace any matches with an equal number of underscores @@ -61,15 +62,21 @@ load = () -> filename = filename.replace BLOCKEDFILE_RX, "@$1" return filename + # returns whether the filename is 'clean' (does not contain any invalid + # characters or reserved words) isCleanFilename: (filename) -> return SafePath.isAllowedLength(filename) && !BADCHAR_RX.test(filename) && !BADFILE_RX.test(filename) && !BLOCKEDFILE_RX.test(filename) + # returns whether a full path is 'clean' - e.g. is a full or relative path + # that points to a file, and each element passes the rules in 'isCleanFilename' isCleanPath: (path) -> elements = path.split('/') - return false if elements[elements.length - 1].length == 0 + + lastElementIsEmpty = elements[elements.length - 1].length == 0 + return false if lastElementIsEmpty for element in elements return false if element.length > 0 && !SafePath.isCleanFilename element diff --git a/services/web/test/unit/coffee/Project/ProjectEntityUpdateHandlerTests.coffee b/services/web/test/unit/coffee/Project/ProjectEntityUpdateHandlerTests.coffee index 3de4546e6d..5c31113552 100644 --- a/services/web/test/unit/coffee/Project/ProjectEntityUpdateHandlerTests.coffee +++ b/services/web/test/unit/coffee/Project/ProjectEntityUpdateHandlerTests.coffee @@ -289,71 +289,101 @@ describe 'ProjectEntityUpdateHandler', -> .should.equal true describe 'addDoc', -> - beforeEach -> - @path = "/path/to/doc" + describe 'adding a doc', -> + beforeEach -> + @path = "/path/to/doc" - @newDoc = _id: doc_id - @ProjectEntityUpdateHandler.addDocWithoutUpdatingHistory = - withoutLock: sinon.stub().yields(null, @newDoc, folder_id, @path, @project) - @ProjectEntityUpdateHandler.addDoc project_id, folder_id, @docName, @docLines, userId, @callback + @newDoc = _id: doc_id + @ProjectEntityUpdateHandler.addDocWithoutUpdatingHistory = + withoutLock: sinon.stub().yields(null, @newDoc, folder_id, @path, @project) + @ProjectEntityUpdateHandler.addDoc project_id, folder_id, @docName, @docLines, userId, @callback - it "creates the doc without history", () -> - @ProjectEntityUpdateHandler.addDocWithoutUpdatingHistory.withoutLock - .calledWith(project_id, folder_id, @docName, @docLines, userId) - .should.equal true + it "creates the doc without history", () -> + @ProjectEntityUpdateHandler.addDocWithoutUpdatingHistory.withoutLock + .calledWith(project_id, folder_id, @docName, @docLines, userId) + .should.equal true - it "sends the change in project structure to the doc updater", () -> - newDocs = [ - doc: @newDoc - path: @path - docLines: @docLines.join('\n') - ] - @DocumentUpdaterHandler.updateProjectStructure - .calledWith(project_id, projectHistoryId, userId, {newDocs}) - .should.equal true + it "sends the change in project structure to the doc updater", () -> + newDocs = [ + doc: @newDoc + path: @path + docLines: @docLines.join('\n') + ] + @DocumentUpdaterHandler.updateProjectStructure + .calledWith(project_id, projectHistoryId, userId, {newDocs}) + .should.equal true + + describe 'adding a doc with an invalid name', -> + beforeEach -> + @path = "/path/to/doc" + + @newDoc = _id: doc_id + @ProjectEntityUpdateHandler.addDocWithoutUpdatingHistory = + withoutLock: sinon.stub().yields(null, @newDoc, folder_id, @path, @project) + @ProjectEntityUpdateHandler.addDoc project_id, folder_id, "*" + @docName, @docLines, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true describe 'addFile', -> - beforeEach -> - @path = "/path/to/file" + describe 'adding a file', -> + beforeEach -> + @path = "/path/to/file" - @newFile = {_id: file_id, rev: 0, name: @fileName, linkedFileData: @linkedFileData} - @TpdsUpdateSender.addFile = sinon.stub().yields() - @ProjectEntityMongoUpdateHandler.addFile = sinon.stub().yields(null, {path: fileSystem: @path}, @project) - @ProjectEntityUpdateHandler.addFile project_id, folder_id, @fileName, @fileSystemPath, @linkedFileData, userId, @callback + @newFile = {_id: file_id, rev: 0, name: @fileName, linkedFileData: @linkedFileData} + @TpdsUpdateSender.addFile = sinon.stub().yields() + @ProjectEntityMongoUpdateHandler.addFile = sinon.stub().yields(null, {path: fileSystem: @path}, @project) + @ProjectEntityUpdateHandler.addFile project_id, folder_id, @fileName, @fileSystemPath, @linkedFileData, userId, @callback - it "updates the file in the filestore", () -> - @FileStoreHandler.uploadFileFromDisk - .calledWith(project_id, file_id, @fileSystemPath) - .should.equal true + it "updates the file in the filestore", () -> + @FileStoreHandler.uploadFileFromDisk + .calledWith(project_id, file_id, @fileSystemPath) + .should.equal true - it "updates the file in mongo", () -> - fileMatcher = sinon.match (file) => - file.name == @fileName + it "updates the file in mongo", () -> + fileMatcher = sinon.match (file) => + file.name == @fileName - @ProjectEntityMongoUpdateHandler.addFile - .calledWithMatch(project_id, folder_id, fileMatcher) - .should.equal true + @ProjectEntityMongoUpdateHandler.addFile + .calledWithMatch(project_id, folder_id, fileMatcher) + .should.equal true - it "notifies the tpds", () -> - @TpdsUpdateSender.addFile - .calledWith({ - project_id: project_id - project_name: @project.name - file_id: file_id - rev: 0 + it "notifies the tpds", () -> + @TpdsUpdateSender.addFile + .calledWith({ + project_id: project_id + project_name: @project.name + file_id: file_id + rev: 0 + path: @path + }) + .should.equal true + + it "sends the change in project structure to the doc updater", () -> + newFiles = [ + file: @newFile path: @path - }) - .should.equal true + url: @fileUrl + ] + @DocumentUpdaterHandler.updateProjectStructure + .calledWith(project_id, projectHistoryId, userId, {newFiles}) + .should.equal true - it "sends the change in project structure to the doc updater", () -> - newFiles = [ - file: @newFile - path: @path - url: @fileUrl - ] - @DocumentUpdaterHandler.updateProjectStructure - .calledWith(project_id, projectHistoryId, userId, {newFiles}) - .should.equal true + describe 'adding a file with an invalid name', -> + beforeEach -> + @path = "/path/to/file" + + @newFile = {_id: file_id, rev: 0, name: @fileName, linkedFileData: @linkedFileData} + @TpdsUpdateSender.addFile = sinon.stub().yields() + @ProjectEntityMongoUpdateHandler.addFile = sinon.stub().yields(null, {path: fileSystem: @path}, @project) + @ProjectEntityUpdateHandler.addFile project_id, folder_id, "*" + @fileName, @fileSystemPath, @linkedFileData, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true describe 'replaceFile', -> beforeEach -> @@ -404,83 +434,116 @@ describe 'ProjectEntityUpdateHandler', -> .should.equal true describe 'addDocWithoutUpdatingHistory', -> - beforeEach -> - @path = "/path/to/doc" + describe 'adding a doc', -> + beforeEach -> + @path = "/path/to/doc" - @project = _id: project_id, name: 'some project' + @project = _id: project_id, name: 'some project' - @TpdsUpdateSender.addDoc = sinon.stub().yields() - @DocstoreManager.updateDoc = sinon.stub().yields(null, false, @rev = 5) - @ProjectEntityMongoUpdateHandler.addDoc = sinon.stub().yields(null, {path: fileSystem: @path}, @project) - @ProjectEntityUpdateHandler.addDocWithoutUpdatingHistory project_id, folder_id, @docName, @docLines, userId, @callback + @TpdsUpdateSender.addDoc = sinon.stub().yields() + @DocstoreManager.updateDoc = sinon.stub().yields(null, false, @rev = 5) + @ProjectEntityMongoUpdateHandler.addDoc = sinon.stub().yields(null, {path: fileSystem: @path}, @project) + @ProjectEntityUpdateHandler.addDocWithoutUpdatingHistory project_id, folder_id, @docName, @docLines, userId, @callback - it "updates the doc in the docstore", () -> - @DocstoreManager.updateDoc - .calledWith(project_id, doc_id, @docLines, 0, {}) - .should.equal true + it "updates the doc in the docstore", () -> + @DocstoreManager.updateDoc + .calledWith(project_id, doc_id, @docLines, 0, {}) + .should.equal true - it "updates the doc in mongo", () -> - docMatcher = sinon.match (doc) => - doc.name == @docName + it "updates the doc in mongo", () -> + docMatcher = sinon.match (doc) => + doc.name == @docName - @ProjectEntityMongoUpdateHandler.addDoc - .calledWithMatch(project_id, folder_id, docMatcher) - .should.equal true + @ProjectEntityMongoUpdateHandler.addDoc + .calledWithMatch(project_id, folder_id, docMatcher) + .should.equal true - it "notifies the tpds", () -> - @TpdsUpdateSender.addDoc - .calledWith({ - project_id: project_id - project_name: @project.name - doc_id: doc_id - rev: 0 - path: @path - }) - .should.equal true + it "notifies the tpds", () -> + @TpdsUpdateSender.addDoc + .calledWith({ + project_id: project_id + project_name: @project.name + doc_id: doc_id + rev: 0 + path: @path + }) + .should.equal true - it "should not should send the change in project structure to the doc updater", () -> - @DocumentUpdaterHandler.updateProjectStructure - .called - .should.equal false + it "should not should send the change in project structure to the doc updater", () -> + @DocumentUpdaterHandler.updateProjectStructure + .called + .should.equal false + + describe 'adding a doc with an invalid name', -> + beforeEach -> + @path = "/path/to/doc" + + @project = _id: project_id, name: 'some project' + + @TpdsUpdateSender.addDoc = sinon.stub().yields() + @DocstoreManager.updateDoc = sinon.stub().yields(null, false, @rev = 5) + @ProjectEntityMongoUpdateHandler.addDoc = sinon.stub().yields(null, {path: fileSystem: @path}, @project) + @ProjectEntityUpdateHandler.addDocWithoutUpdatingHistory project_id, folder_id, "*" + @docName, @docLines, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true describe 'addFileWithoutUpdatingHistory', -> - beforeEach -> - @path = "/path/to/file" + describe 'adding a file', -> + beforeEach -> + @path = "/path/to/file" - @project = _id: project_id, name: 'some project' + @project = _id: project_id, name: 'some project' - @TpdsUpdateSender.addFile = sinon.stub().yields() - @ProjectEntityMongoUpdateHandler.addFile = sinon.stub().yields(null, {path: fileSystem: @path}, @project) - @ProjectEntityUpdateHandler.addFileWithoutUpdatingHistory project_id, folder_id, @fileName, @fileSystemPath, userId, @callback + @TpdsUpdateSender.addFile = sinon.stub().yields() + @ProjectEntityMongoUpdateHandler.addFile = sinon.stub().yields(null, {path: fileSystem: @path}, @project) + @ProjectEntityUpdateHandler.addFileWithoutUpdatingHistory project_id, folder_id, @fileName, @fileSystemPath, @linkedFileData, userId, @callback - it "updates the file in the filestore", () -> - @FileStoreHandler.uploadFileFromDisk - .calledWith(project_id, file_id, @fileSystemPath) - .should.equal true + it "updates the file in the filestore", () -> + @FileStoreHandler.uploadFileFromDisk + .calledWith(project_id, file_id, @fileSystemPath) + .should.equal true - it "updates the file in mongo", () -> - fileMatcher = sinon.match (file) => - file.name == @fileName + it "updates the file in mongo", () -> + fileMatcher = sinon.match (file) => + file.name == @fileName - @ProjectEntityMongoUpdateHandler.addFile - .calledWithMatch(project_id, folder_id, fileMatcher) - .should.equal true + @ProjectEntityMongoUpdateHandler.addFile + .calledWithMatch(project_id, folder_id, fileMatcher) + .should.equal true - it "notifies the tpds", () -> - @TpdsUpdateSender.addFile - .calledWith({ - project_id: project_id - project_name: @project.name - file_id: file_id - rev: 0 - path: @path - }) - .should.equal true + it "notifies the tpds", () -> + @TpdsUpdateSender.addFile + .calledWith({ + project_id: project_id + project_name: @project.name + file_id: file_id + rev: 0 + path: @path + }) + .should.equal true - it "should not should send the change in project structure to the doc updater", () -> - @DocumentUpdaterHandler.updateProjectStructure - .called - .should.equal false + it "should not should send the change in project structure to the doc updater", () -> + @DocumentUpdaterHandler.updateProjectStructure + .called + .should.equal false + + describe 'adding a file with an invalid name', -> + beforeEach -> + @path = "/path/to/file" + + @project = _id: project_id, name: 'some project' + + @TpdsUpdateSender.addFile = sinon.stub().yields() + @ProjectEntityMongoUpdateHandler.addFile = sinon.stub().yields(null, {path: fileSystem: @path}, @project) + @ProjectEntityUpdateHandler.addFileWithoutUpdatingHistory project_id, folder_id, "*" + @fileName, @fileSystemPath, @linkedFileData, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true describe 'upsertDoc', -> describe 'upserting into an invalid folder', -> @@ -543,6 +606,20 @@ describe 'ProjectEntityUpdateHandler', -> it 'returns the doc', -> @callback.calledWith(null, @newDoc, true) + describe 'upserting a new doc with an invalid name', -> + beforeEach -> + @folder = _id: folder_id, docs: [] + @newDoc = _id: doc_id + @ProjectLocator.findElement = sinon.stub().yields(null, @folder) + @ProjectEntityUpdateHandler.addDoc = withoutLock: sinon.stub().yields(null, @newDoc) + + @ProjectEntityUpdateHandler.upsertDoc project_id, folder_id, "*" + @docName, @docLines, @source, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true + describe 'upsertFile', -> describe 'upserting into an invalid folder', -> beforeEach -> @@ -593,63 +670,155 @@ describe 'ProjectEntityUpdateHandler', -> it 'returns the file', -> @callback.calledWith(null, @newFile, true) + describe 'upserting a new file with an invalid name', -> + beforeEach -> + @folder = _id: folder_id, fileRefs: [] + @newFile = _id: file_id + @ProjectLocator.findElement = sinon.stub().yields(null, @folder) + @ProjectEntityUpdateHandler.addFile = mainTask: sinon.stub().yields(null, @newFile) + + @ProjectEntityUpdateHandler.upsertFile project_id, folder_id, '*' + @fileName, @fileSystemPath, @linkedFileData, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true + describe 'upsertDocWithPath', -> - beforeEach -> - @path = "/folder/doc.tex" - @newFolders = [ 'mock-a', 'mock-b' ] - @folder = _id: folder_id - @doc = _id: doc_id - @isNewDoc = true - @ProjectEntityUpdateHandler.mkdirp = - withoutLock: sinon.stub().yields(null, @newFolders, @folder) - @ProjectEntityUpdateHandler.upsertDoc = - withoutLock: sinon.stub().yields(null, @doc, @isNewDoc) + describe 'upserting a doc', -> + beforeEach -> + @path = "/folder/doc.tex" + @newFolders = [ 'mock-a', 'mock-b' ] + @folder = _id: folder_id + @doc = _id: doc_id + @isNewDoc = true + @ProjectEntityUpdateHandler.mkdirp = + withoutLock: sinon.stub().yields(null, @newFolders, @folder) + @ProjectEntityUpdateHandler.upsertDoc = + withoutLock: sinon.stub().yields(null, @doc, @isNewDoc) - @ProjectEntityUpdateHandler.upsertDocWithPath project_id, @path, @docLines, @source, userId, @callback + @ProjectEntityUpdateHandler.upsertDocWithPath project_id, @path, @docLines, @source, userId, @callback - it 'creates any necessary folders', -> - @ProjectEntityUpdateHandler.mkdirp.withoutLock - .calledWith(project_id, '/folder') - .should.equal true + it 'creates any necessary folders', -> + @ProjectEntityUpdateHandler.mkdirp.withoutLock + .calledWith(project_id, '/folder') + .should.equal true - it 'upserts the doc', -> - @ProjectEntityUpdateHandler.upsertDoc.withoutLock - .calledWith(project_id, @folder._id, 'doc.tex', @docLines, @source, userId) - .should.equal true + it 'upserts the doc', -> + @ProjectEntityUpdateHandler.upsertDoc.withoutLock + .calledWith(project_id, @folder._id, 'doc.tex', @docLines, @source, userId) + .should.equal true - it 'calls the callback', -> - @callback - .calledWith(null, @doc, @isNewDoc, @newFolders, @folder) - .should.equal true + it 'calls the callback', -> + @callback + .calledWith(null, @doc, @isNewDoc, @newFolders, @folder) + .should.equal true + + describe 'upserting a doc with an invalid path', -> + beforeEach -> + @path = "/*folder/doc.tex" + @newFolders = [ 'mock-a', 'mock-b' ] + @folder = _id: folder_id + @doc = _id: doc_id + @isNewDoc = true + @ProjectEntityUpdateHandler.mkdirp = + withoutLock: sinon.stub().yields(null, @newFolders, @folder) + @ProjectEntityUpdateHandler.upsertDoc = + withoutLock: sinon.stub().yields(null, @doc, @isNewDoc) + + @ProjectEntityUpdateHandler.upsertDocWithPath project_id, @path, @docLines, @source, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true + + describe 'upserting a doc with an invalid name', -> + beforeEach -> + @path = "/folder/*doc.tex" + @newFolders = [ 'mock-a', 'mock-b' ] + @folder = _id: folder_id + @doc = _id: doc_id + @isNewDoc = true + @ProjectEntityUpdateHandler.mkdirp = + withoutLock: sinon.stub().yields(null, @newFolders, @folder) + @ProjectEntityUpdateHandler.upsertDoc = + withoutLock: sinon.stub().yields(null, @doc, @isNewDoc) + + @ProjectEntityUpdateHandler.upsertDocWithPath project_id, @path, @docLines, @source, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true describe 'upsertFileWithPath', -> - beforeEach -> - @path = "/folder/file.png" - @newFolders = [ 'mock-a', 'mock-b' ] - @folder = _id: folder_id - @file = _id: file_id - @isNewFile = true - @ProjectEntityUpdateHandler.mkdirp = - withoutLock: sinon.stub().yields(null, @newFolders, @folder) - @ProjectEntityUpdateHandler.upsertFile = - mainTask: sinon.stub().yields(null, @file, @isNewFile) + describe 'upserting a file', -> + beforeEach -> + @path = "/folder/file.png" + @newFolders = [ 'mock-a', 'mock-b' ] + @folder = _id: folder_id + @file = _id: file_id + @isNewFile = true + @ProjectEntityUpdateHandler.mkdirp = + withoutLock: sinon.stub().yields(null, @newFolders, @folder) + @ProjectEntityUpdateHandler.upsertFile = + mainTask: sinon.stub().yields(null, @file, @isNewFile) - @ProjectEntityUpdateHandler.upsertFileWithPath project_id, @path, @fileSystemPath, @linkedFileData, userId, @callback + @ProjectEntityUpdateHandler.upsertFileWithPath project_id, @path, @fileSystemPath, @linkedFileData, userId, @callback - it 'creates any necessary folders', -> - @ProjectEntityUpdateHandler.mkdirp.withoutLock - .calledWith(project_id, '/folder') - .should.equal true + it 'creates any necessary folders', -> + @ProjectEntityUpdateHandler.mkdirp.withoutLock + .calledWith(project_id, '/folder') + .should.equal true - it 'upserts the file', -> - @ProjectEntityUpdateHandler.upsertFile.mainTask - .calledWith(project_id, @folder._id, 'file.png', @fileSystemPath, @linkedFileData, userId) - .should.equal true + it 'upserts the file', -> + @ProjectEntityUpdateHandler.upsertFile.mainTask + .calledWith(project_id, @folder._id, 'file.png', @fileSystemPath, @linkedFileData, userId) + .should.equal true - it 'calls the callback', -> - @callback - .calledWith(null, @file, @isNewFile, undefined, @newFolders, @folder) - .should.equal true + it 'calls the callback', -> + @callback + .calledWith(null, @file, @isNewFile, undefined, @newFolders, @folder) + .should.equal true + + describe 'upserting a file with an invalid path', -> + beforeEach -> + @path = "/*folder/file.png" + @newFolders = [ 'mock-a', 'mock-b' ] + @folder = _id: folder_id + @file = _id: file_id + @isNewFile = true + @ProjectEntityUpdateHandler.mkdirp = + withoutLock: sinon.stub().yields(null, @newFolders, @folder) + @ProjectEntityUpdateHandler.upsertFile = + mainTask: sinon.stub().yields(null, @file, @isNewFile) + + @ProjectEntityUpdateHandler.upsertFileWithPath project_id, @path, @fileSystemPath, @linkedFileData, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true + + describe 'upserting a file with an invalid name', -> + beforeEach -> + @path = "/folder/*file.png" + @newFolders = [ 'mock-a', 'mock-b' ] + @folder = _id: folder_id + @file = _id: file_id + @isNewFile = true + @ProjectEntityUpdateHandler.mkdirp = + withoutLock: sinon.stub().yields(null, @newFolders, @folder) + @ProjectEntityUpdateHandler.upsertFile = + mainTask: sinon.stub().yields(null, @file, @isNewFile) + + @ProjectEntityUpdateHandler.upsertFileWithPath project_id, @path, @fileSystemPath, @linkedFileData, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true describe 'deleteEntity', -> beforeEach -> @@ -721,16 +890,29 @@ describe 'ProjectEntityUpdateHandler', -> .should.equal true describe 'addFolder', -> - beforeEach -> - @parentFolder_id = '123asdf' - @folderName = 'new-folder' - @ProjectEntityMongoUpdateHandler.addFolder = sinon.stub().yields() - @ProjectEntityUpdateHandler.addFolder project_id, @parentFolder_id, @folderName, @callback + describe 'adding a folder', -> + beforeEach -> + @parentFolder_id = '123asdf' + @folderName = 'new-folder' + @ProjectEntityMongoUpdateHandler.addFolder = sinon.stub().yields() + @ProjectEntityUpdateHandler.addFolder project_id, @parentFolder_id, @folderName, @callback - it 'calls ProjectEntityMongoUpdateHandler', -> - @ProjectEntityMongoUpdateHandler.addFolder - .calledWith(project_id, @parentFolder_id, @folderName) - .should.equal true + it 'calls ProjectEntityMongoUpdateHandler', -> + @ProjectEntityMongoUpdateHandler.addFolder + .calledWith(project_id, @parentFolder_id, @folderName) + .should.equal true + + describe 'adding a folder with an invalid name', -> + beforeEach -> + @parentFolder_id = '123asdf' + @folderName = '*new-folder' + @ProjectEntityMongoUpdateHandler.addFolder = sinon.stub().yields() + @ProjectEntityUpdateHandler.addFolder project_id, @parentFolder_id, @folderName, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true describe 'moveEntity', -> beforeEach -> @@ -763,35 +945,57 @@ describe 'ProjectEntityUpdateHandler', -> .should.equal true describe "renameEntity", -> - beforeEach -> - @project_name = 'project name' - @startPath = '/folder/a.tex' - @endPath = '/folder/b.tex' - @rev = 2 - @changes = newDocs: ['old-doc'], newFiles: ['old-file'] - @newDocName = 'b.tex' - @ProjectEntityMongoUpdateHandler.renameEntity = sinon.stub().yields( - null, @project, @startPath, @endPath, @rev, @changes - ) - @TpdsUpdateSender.moveEntity = sinon.stub() - @DocumentUpdaterHandler.updateProjectStructure = sinon.stub() + describe 'renaming an entity', -> + beforeEach -> + @project_name = 'project name' + @startPath = '/folder/a.tex' + @endPath = '/folder/b.tex' + @rev = 2 + @changes = newDocs: ['old-doc'], newFiles: ['old-file'] + @newDocName = 'b.tex' + @ProjectEntityMongoUpdateHandler.renameEntity = sinon.stub().yields( + null, @project, @startPath, @endPath, @rev, @changes + ) + @TpdsUpdateSender.moveEntity = sinon.stub() + @DocumentUpdaterHandler.updateProjectStructure = sinon.stub() - @ProjectEntityUpdateHandler.renameEntity project_id, doc_id, 'doc', @newDocName, userId, @callback + @ProjectEntityUpdateHandler.renameEntity project_id, doc_id, 'doc', @newDocName, userId, @callback - it 'moves the entity in mongo', -> - @ProjectEntityMongoUpdateHandler.renameEntity - .calledWith(project_id, doc_id, 'doc', @newDocName) - .should.equal true + it 'moves the entity in mongo', -> + @ProjectEntityMongoUpdateHandler.renameEntity + .calledWith(project_id, doc_id, 'doc', @newDocName) + .should.equal true - it 'notifies tpds', -> - @TpdsUpdateSender.moveEntity - .calledWith({project_id, @project_name, @startPath, @endPath, @rev}) - .should.equal true + it 'notifies tpds', -> + @TpdsUpdateSender.moveEntity + .calledWith({project_id, @project_name, @startPath, @endPath, @rev}) + .should.equal true - it 'sends the changes in project structure to the doc updater', -> - @DocumentUpdaterHandler.updateProjectStructure - .calledWith(project_id, projectHistoryId, userId, @changes, @callback) - .should.equal true + it 'sends the changes in project structure to the doc updater', -> + @DocumentUpdaterHandler.updateProjectStructure + .calledWith(project_id, projectHistoryId, userId, @changes, @callback) + .should.equal true + + describe 'renaming an entity to an invalid name', -> + beforeEach -> + @project_name = 'project name' + @startPath = '/folder/a.tex' + @endPath = '/folder/b.tex' + @rev = 2 + @changes = newDocs: ['old-doc'], newFiles: ['old-file'] + @newDocName = '*b.tex' + @ProjectEntityMongoUpdateHandler.renameEntity = sinon.stub().yields( + null, @project, @startPath, @endPath, @rev, @changes + ) + @TpdsUpdateSender.moveEntity = sinon.stub() + @DocumentUpdaterHandler.updateProjectStructure = sinon.stub() + + @ProjectEntityUpdateHandler.renameEntity project_id, doc_id, 'doc', @newDocName, userId, @callback + + it 'returns an error', -> + errorMatcher = sinon.match.instanceOf(Errors.InvalidNameError) + @callback.calledWithMatch(errorMatcher) + .should.equal true describe "resyncProjectHistory", -> describe "a deleted project", -> @@ -998,5 +1202,3 @@ describe 'ProjectEntityUpdateHandler', -> it "should call the callback", -> @callback.called.should.equal true - - From 32149e652ff1a46e057bd9e3dae2da1f3494422e Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Tue, 9 Oct 2018 17:09:49 +0100 Subject: [PATCH 3/3] Handle 'invalid element name' error in project list ui When invalid filenames are found during project-copy, the somewhat obscure (and non-localised) 'invalid element name' error is returned. Add a special case to handle this particular error and display something more descriptive to the user. Added a modal error handler for when this error is generated by clicking the 'copy' icon in the project list, instead of using the 'more' dropdown which opens a modal copy dialog bug: overleaf/sharelatex#908 Signed-off-by: Simon Detheridge --- .../web/app/views/project/list/modals.pug | 20 ++++++++++++--- .../project-list/modal-controllers.coffee | 14 +++++++---- .../main/project-list/project-list.coffee | 25 +++++++++++++------ 3 files changed, 43 insertions(+), 16 deletions(-) diff --git a/services/web/app/views/project/list/modals.pug b/services/web/app/views/project/list/modals.pug index 50d7f8d2ef..11e8da29ea 100644 --- a/services/web/app/views/project/list/modals.pug +++ b/services/web/app/views/project/list/modals.pug @@ -98,7 +98,7 @@ script(type='text/ng-template', id='renameProjectModalTemplate') ) × h3 #{translate("rename_project")} .modal-body - .alert.alert-danger(ng-show="state.error.message") {{ state.error.message}} + .alert.alert-danger(ng-show="state.error.message") {{state.error.message}} .alert.alert-danger(ng-show="state.error && !state.error.message") #{translate("generic_something_went_wrong")} form(name="renameProjectForm", novalidate) input.form-control( @@ -127,7 +127,7 @@ script(type='text/ng-template', id='cloneProjectModalTemplate') ) × h3 #{translate("copy_project")} .modal-body - .alert.alert-danger(ng-show="state.error.message") {{ state.error.message}} + .alert.alert-danger(ng-show="state.error.message") {{state.error.message === "invalid element name" ? translate("invalid_element_name") : state.error.message}} .alert.alert-danger(ng-show="state.error && !state.error.message") #{translate("generic_something_went_wrong")} form(name="cloneProjectForm", novalidate) .form-group @@ -161,7 +161,7 @@ script(type='text/ng-template', id='newProjectModalTemplate') ) × h3 #{translate("new_project")} .modal-body - .alert.alert-danger(ng-show="state.error.message") {{ state.error.message}} + .alert.alert-danger(ng-show="state.error.message") {{state.error.message}} .alert.alert-danger(ng-show="state.error && !state.error.message") #{translate("generic_something_went_wrong")} form(novalidate, name="newProjectForm") input.form-control( @@ -262,6 +262,20 @@ script(type="text/ng-template", id="uploadProjectModalTemplate") .modal-footer button.btn.btn-default(ng-click="cancel()") #{translate("cancel")} +script(type="text/ng-template", id="showErrorModalTemplate") + .modal-header + button.close( + type="button" + data-dismiss="modal" + ng-click="cancel()" + ) × + h3 #{translate("generic_something_went_wrong")} + .modal-body + .alert.alert-danger(ng-show="error.message") {{error.message === "invalid element name" ? translate("invalid_element_name") : error.message}} + .alert.alert-danger(ng-show="error && !error.message") #{translate("generic_something_went_wrong")} + .modal-footer + button.btn.btn-default(ng-click="cancel()") #{translate("cancel")} + script(type="text/ng-template", id="userProfileModalTemplate") .modal-header button.close( diff --git a/services/web/public/coffee/main/project-list/modal-controllers.coffee b/services/web/public/coffee/main/project-list/modal-controllers.coffee index 27124e0951..3d15797940 100644 --- a/services/web/public/coffee/main/project-list/modal-controllers.coffee +++ b/services/web/public/coffee/main/project-list/modal-controllers.coffee @@ -2,9 +2,9 @@ define [ "base" ], (App) -> App.controller 'RenameProjectModalController', ($scope, $modalInstance, $timeout, project, queuedHttp) -> - $scope.inputs = + $scope.inputs = projectName: project.name - + $scope.state = inflight: false error: false @@ -35,7 +35,7 @@ define [ $modalInstance.dismiss('cancel') App.controller 'CloneProjectModalController', ($scope, $modalInstance, $timeout, project) -> - $scope.inputs = + $scope.inputs = projectName: project.name + " (Copy)" $scope.state = inflight: false @@ -66,7 +66,7 @@ define [ $modalInstance.dismiss('cancel') App.controller 'NewProjectModalController', ($scope, $modalInstance, $timeout, template) -> - $scope.inputs = + $scope.inputs = projectName: "" $scope.state = inflight: false @@ -123,7 +123,6 @@ define [ $scope.cancel = () -> $modalInstance.dismiss('cancel') - App.controller 'UploadProjectModalController', ($scope, $modalInstance, $timeout) -> $scope.cancel = () -> $modalInstance.dismiss('cancel') @@ -137,3 +136,8 @@ define [ $scope.dismiss = () -> $modalInstance.dismiss('cancel') + + App.controller 'ShowErrorModalController', ($scope, $modalInstance, error) -> + $scope.error = error + $scope.cancel = () -> + $modalInstance.dismiss('cancel') diff --git a/services/web/public/coffee/main/project-list/project-list.coffee b/services/web/public/coffee/main/project-list/project-list.coffee index 9a8d7f7a73..8bdec6f42a 100644 --- a/services/web/public/coffee/main/project-list/project-list.coffee +++ b/services/web/public/coffee/main/project-list/project-list.coffee @@ -13,7 +13,7 @@ define [ $scope.predicate = "lastUpdated" $scope.nUntagged = 0 $scope.reverse = true - $scope.searchText = + $scope.searchText = value : "" $timeout () -> @@ -37,7 +37,7 @@ define [ angular.element($window).bind "resize", () -> recalculateProjectListHeight() $scope.$apply() - + # Allow tags to be accessed on projects as well projectsById = {} for project in $scope.projects @@ -56,7 +56,7 @@ define [ tag.selected = true else tag.selected = false - + $scope.changePredicate = (newPredicate)-> if $scope.predicate == newPredicate $scope.reverse = !$scope.reverse @@ -145,7 +145,7 @@ define [ # We don't want hidden selections project.selected = false - localStorage("project_list", JSON.stringify({ + localStorage("project_list", JSON.stringify({ filter: $scope.filter, selectedTagId: selectedTag?._id })) @@ -461,7 +461,7 @@ define [ resolve: project: () -> project ) - + if storedUIOpts?.filter? if storedUIOpts.filter == "tag" and storedUIOpts.selectedTagId? markTagAsSelected(storedUIOpts.selectedTagId) @@ -505,7 +505,16 @@ define [ $scope.project.isTableActionInflight = true $scope.cloneProject($scope.project, "#{$scope.project.name} (Copy)") .then () -> $scope.project.isTableActionInflight = false - .catch () -> $scope.project.isTableActionInflight = false + .catch (response) -> + { data, status } = response + error = if status == 400 then message: data else true + modalInstance = $modal.open( + templateUrl: "showErrorModalTemplate" + controller: "ShowErrorModalController" + resolve: + error: () -> error + ) + $scope.project.isTableActionInflight = false $scope.download = (e) -> e.stopPropagation() @@ -535,11 +544,11 @@ define [ url: "/project/#{$scope.project.id}?forever=true" headers: "X-CSRF-Token": window.csrfToken - }).then () -> + }).then () -> $scope.project.isTableActionInflight = false $scope._removeProjectFromList $scope.project for tag in $scope.tags $scope._removeProjectIdsFromTagArray(tag, [ $scope.project.id ]) $scope.updateVisibleProjects() - .catch () -> + .catch () -> $scope.project.isTableActionInflight = false