From 32925f0c4eb095e7fc1e3b29a5c6c21bf1c5654d Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 13 Mar 2019 11:04:32 +0000 Subject: [PATCH] Merge pull request #1608 from sharelatex/bg-allow-reserved-names-safely allow reserved names safely GitOrigin-RevId: d0c5f287411f5cd6d0639f57ef925409571acf8a --- .../ProjectEntityMongoUpdateHandler.coffee | 23 +++++++++++++++- .../coffee/Features/Project/SafePath.coffee | 11 +++++--- .../web/public/src/ide/directives/SafePath.js | 3 +-- .../src/ide/file-tree/FileTreeManager.js | 8 +++--- .../unit/coffee/Project/SafePathTests.coffee | 27 ++++++++++++++----- 5 files changed, 56 insertions(+), 16 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectEntityMongoUpdateHandler.coffee b/services/web/app/coffee/Features/Project/ProjectEntityMongoUpdateHandler.coffee index 106618d72f..a598715bf1 100644 --- a/services/web/app/coffee/Features/Project/ProjectEntityMongoUpdateHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectEntityMongoUpdateHandler.coffee @@ -122,6 +122,9 @@ module.exports = ProjectEntityMongoUpdateHandler = self = return callback(err) if err? ProjectLocator.findElement {project, element_id: entity_id, type: entityType}, (err, entity, entityPath)-> return callback(err) if err? + # Prevent top-level docs/files with reserved names (to match v1 behaviour) + if self._blockedFilename entityPath, entityType + return callback new Errors.InvalidNameError("blocked element name") self._checkValidMove project, entityType, entity, entityPath, destFolderId, (error) -> return callback(error) if error? ProjectEntityHandler.getAllEntitiesFromProject project, (error, oldDocs, oldFiles) -> @@ -153,10 +156,13 @@ module.exports = ProjectEntityMongoUpdateHandler = self = return callback(error) if error? ProjectLocator.findElement {project:project, element_id:entity_id, type:entityType}, (error, entity, entPath, parentFolder)=> return callback(error) if error? + endPath = path.join(path.dirname(entPath.fileSystem), newName) + # Prevent top-level docs/files with reserved names (to match v1 behaviour) + if self._blockedFilename {fileSystem: endPath}, entityType + return callback new Errors.InvalidNameError("blocked element name") # check if the new name already exists in the current folder self._checkValidElementName parentFolder, newName, (error) => return callback(error) if error? - endPath = path.join(path.dirname(entPath.fileSystem), newName) conditions = {_id:project_id} update = "$set":{}, "$inc":{} namePath = entPath.mongo+".name" @@ -257,6 +263,9 @@ module.exports = ProjectEntityMongoUpdateHandler = self = # check if the path would be too long if not SafePath.isAllowedLength newPath.fileSystem return callback new Errors.InvalidNameError("path too long") + # Prevent top-level docs/files with reserved names (to match v1 behaviour) + if self._blockedFilename newPath, type + return callback new Errors.InvalidNameError("blocked element name") self._checkValidElementName folder, element.name, (err) => return callback(err) if err? id = element._id+'' @@ -276,6 +285,18 @@ module.exports = ProjectEntityMongoUpdateHandler = self = return callback(err) callback(err, {path:newPath}, newProject) + _blockedFilename: (entityPath, entityType) -> + # check if name would be blocked in v1 + # javascript reserved names are forbidden for docs and files + # at the top-level (but folders with reserved names are allowed). + isFolder = (entityType in ['folder', 'folders']) + [dir, file] = [path.dirname(entityPath.fileSystem), path.basename(entityPath.fileSystem)] + isTopLevel = dir is '/' + if isTopLevel and !isFolder && SafePath.isBlockedFilename file + return true + else + return false + _checkValidElementName: (folder, name, callback = (err) ->) -> # check if the name is already taken by a doc, file or # folder. If so, return an error "file already exists". diff --git a/services/web/app/coffee/Features/Project/SafePath.coffee b/services/web/app/coffee/Features/Project/SafePath.coffee index 4e5a288ea9..82aef24bb1 100644 --- a/services/web/app/coffee/Features/Project/SafePath.coffee +++ b/services/web/app/coffee/Features/Project/SafePath.coffee @@ -19,7 +19,7 @@ load = () -> BADFILE_RX = /// (^\.$) # reject . as a filename - | (^\.\.$) # reject .. as a filename + | (^\.\.$) # reject .. as a filename | (^\s+) # reject leading space | (\s+$) # reject trailing space ///g @@ -67,8 +67,10 @@ load = () -> isCleanFilename: (filename) -> return SafePath.isAllowedLength(filename) && !BADCHAR_RX.test(filename) && - !BADFILE_RX.test(filename) && - !BLOCKEDFILE_RX.test(filename) + !BADFILE_RX.test(filename) + + isBlockedFilename: (filename) -> + return 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' @@ -81,6 +83,9 @@ load = () -> for element in elements return false if element.length > 0 && !SafePath.isCleanFilename element + # check for a top-level reserved name + return false if BLOCKEDFILE_RX.test(path.replace(/^\/?/,'')) # remove leading slash if present + return true isAllowedLength: (pathname) -> diff --git a/services/web/public/src/ide/directives/SafePath.js b/services/web/public/src/ide/directives/SafePath.js index 785c16ec5c..5b36408a7f 100644 --- a/services/web/public/src/ide/directives/SafePath.js +++ b/services/web/public/src/ide/directives/SafePath.js @@ -87,8 +87,7 @@ prototype\ return ( SafePath.isAllowedLength(filename) && !BADCHAR_RX.test(filename) && - !BADFILE_RX.test(filename) && - !BLOCKEDFILE_RX.test(filename) + !BADFILE_RX.test(filename) ) }, diff --git a/services/web/public/src/ide/file-tree/FileTreeManager.js b/services/web/public/src/ide/file-tree/FileTreeManager.js index 0717a9039b..116d136000 100644 --- a/services/web/public/src/ide/file-tree/FileTreeManager.js +++ b/services/web/public/src/ide/file-tree/FileTreeManager.js @@ -168,16 +168,18 @@ define([ } getMultiSelectedEntityChildNodes() { + // use pathnames with a leading slash to avoid + // problems with reserved Object properties const entities = this.getMultiSelectedEntities() const paths = {} for (var entity of Array.from(entities)) { - paths[this.getEntityPath(entity)] = entity + paths["/" + this.getEntityPath(entity)] = entity } const prefixes = {} for (var path in paths) { entity = paths[path] const parts = path.split('/') - if (parts.length <= 1) { + if (parts.length <= 2) { continue } else { // Record prefixes a/b/c.tex -> 'a' and 'a/b' @@ -186,7 +188,7 @@ define([ asc ? i <= end : i >= end; asc ? i++ : i-- ) { - prefixes[parts.slice(0, i).join('/')] = true + prefixes["/" + parts.slice(0, i).join('/')] = true } } } diff --git a/services/web/test/unit/coffee/Project/SafePathTests.coffee b/services/web/test/unit/coffee/Project/SafePathTests.coffee index afea03fdea..b726c7bf95 100644 --- a/services/web/test/unit/coffee/Project/SafePathTests.coffee +++ b/services/web/test/unit/coffee/Project/SafePathTests.coffee @@ -63,17 +63,18 @@ describe 'SafePath', -> result = @SafePath.isCleanFilename 'foo\uD800\uDFFFbar' result.should.equal false - it 'should not accept javascript property names', -> + it 'should accept javascript property names', -> result = @SafePath.isCleanFilename 'prototype' - result.should.equal false + result.should.equal true - it 'should not accept javascript property names in the prototype', -> + it 'should accept javascript property names in the prototype', -> result = @SafePath.isCleanFilename 'hasOwnProperty' - result.should.equal false + result.should.equal true - it 'should not accept javascript property names resulting from substitutions', -> - result = @SafePath.isCleanFilename ' proto ' - result.should.equal false + # this test never worked correctly because the spaces are not replaced by underscores in isCleanFilename + # it 'should not accept javascript property names resulting from substitutions', -> + # result = @SafePath.isCleanFilename ' proto ' + # result.should.equal false # it 'should not accept a trailing .', -> # result = @SafePath.isCleanFilename 'hello.' @@ -136,6 +137,18 @@ describe 'SafePath', -> result = @SafePath.isCleanPath 'foo//*/bar' result.should.equal false + it 'should not accept javascript property names', -> + result = @SafePath.isCleanPath 'prototype' + result.should.equal false + + it 'should not accept javascript property names in the prototype', -> + result = @SafePath.isCleanPath 'hasOwnProperty' + result.should.equal false + + it 'should not accept javascript property names resulting from substitutions', -> + result = @SafePath.isCleanPath ' proto ' + result.should.equal false + describe 'isAllowedLength', -> it 'should accept a valid path "main.tex"', -> result = @SafePath.isAllowedLength 'main.tex'