From 7f727d434ed93d86f429b2a51b79576de781679a Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Mon, 5 Feb 2018 12:49:15 +0000 Subject: [PATCH 1/5] server side check for valid filenames --- .../Project/ProjectEntityHandler.coffee | 27 +++++- .../coffee/Features/Project/SafePath.coffee | 31 +++++++ .../unit/coffee/Project/SafePathTests.coffee | 89 +++++++++++++++++++ 3 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 services/web/app/coffee/Features/Project/SafePath.coffee create mode 100644 services/web/test/unit/coffee/Project/SafePathTests.coffee diff --git a/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee b/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee index b67180659e..1ac12ec84e 100644 --- a/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee @@ -17,6 +17,7 @@ DocstoreManager = require "../Docstore/DocstoreManager" ProjectGetter = require "./ProjectGetter" CooldownManager = require '../Cooldown/CooldownManager' DocumentUpdaterHandler = require('../../Features/DocumentUpdater/DocumentUpdaterHandler') +SafePath = require './SafePath' module.exports = ProjectEntityHandler = getAllFolders: (project_id, callback) -> @@ -179,6 +180,9 @@ module.exports = ProjectEntityHandler = ProjectEntityHandler._addDocWithProject project, folder_id, docName, docLines, userId, callback _addDocWithProject: (project, folder_id, docName, docLines, userId, callback = (error, doc, folder_id, path) ->)=> + # check if docname is allowed + if not SafePath.isCleanFilename docName + return callback new Error("invalid element name") project_id = project._id logger.log project_id: project_id, folder_id: folder_id, doc_name: docName, "adding doc to project with project" confirmFolder project, folder_id, (folder_id)=> @@ -201,6 +205,9 @@ module.exports = ProjectEntityHandler = callback(null, doc, folder_id, result?.path?.fileSystem) restoreDoc: (project_id, doc_id, name, callback = (error, doc, folder_id) ->) -> + # check if docname is allowed (passed in from client so we check it) + if not SafePath.isCleanFilename name + return callback new Error("invalid element name") # getDoc will return the deleted doc's lines, but we don't actually remove # the deleted doc, just create a new one from its lines. ProjectEntityHandler.getDoc project_id, doc_id, include_deleted: true, (error, lines) -> @@ -208,6 +215,9 @@ module.exports = ProjectEntityHandler = ProjectEntityHandler.addDoc project_id, null, name, lines, callback addFileWithoutUpdatingHistory: (project_id, folder_id, fileName, path, userId, callback = (error, fileRef, folder_id, path, fileStoreUrl) ->)-> + # check if file name is allowed + if not SafePath.isCleanFilename fileName + return callback new Error("invalid element name") ProjectGetter.getProject project_id, {rootFolder:true, name:true}, (err, project) -> if err? logger.err project_id:project_id, err:err, "error getting project for add file" @@ -280,7 +290,8 @@ module.exports = ProjectEntityHandler = if !origonalFileRef? logger.err { project_id, folder_id, originalProject_id, origonalFileRef }, "file trying to copy is null" return callback() - fileRef = new File name : origonalFileRef.name + # convert any invalid characters in original file to '_' + fileRef = new File name : SafePath.clean(origonalFileRef.name) FileStoreHandler.copyFile originalProject_id, origonalFileRef._id, project._id, fileRef._id, (err, fileStoreUrl)-> if err? logger.err { err, project_id, folder_id, originalProject_id, origonalFileRef }, "error coping file in s3" @@ -349,6 +360,9 @@ module.exports = ProjectEntityHandler = ProjectEntityHandler.addFolderWithProject project, parentFolder_id, folderName, callback addFolderWithProject: (project, parentFolder_id, folderName, callback = (err, folder, parentFolder_id)->) -> + # check if folder name is allowed + if not SafePath.isCleanFilename folderName + return callback new Error("invalid element name") confirmFolder project, parentFolder_id, (parentFolder_id)=> folder = new Folder name: folderName logger.log project: project._id, parentFolder_id:parentFolder_id, folderName:folderName, "adding new folder" @@ -450,6 +464,9 @@ module.exports = ProjectEntityHandler = renameEntity: (project_id, entity_id, entityType, newName, userId, callback)-> + # check if name is allowed + if not SafePath.isCleanFilename newName + return callback new Error("invalid element name") logger.log(entity_id: entity_id, project_id: project_id, ('renaming '+entityType)) if !entityType? logger.err err: "No entityType set", project_id: project_id, entity_id: entity_id @@ -593,7 +610,9 @@ module.exports = ProjectEntityHandler = return callback(e) type = sanitizeTypeOfElement type - if path.resolve("/", element.name) isnt "/#{element.name}" or element.name.match("/") + # original check path.resolve("/", element.name) isnt "/#{element.name}" or element.name.match("/") + # check if name is allowed + if not SafePath.isCleanFilename element.name e = new Error("invalid element name") logger.err project_id:project._id, folder_id:folder_id, element:element, type:type, "failed trying to insert element as name was invalid" return callback(e) @@ -627,7 +646,11 @@ module.exports = ProjectEntityHandler = return callback(err) callback(err, {path:newPath}, project) + checkValidElementName: (folder, name, callback = (err) ->) -> + # check if the path would be too long + if not SafePath.isAllowedLength "#{folder}/#{name}" + return callback new Error.InvalidNameError("path too long") # check if the name is already taken by a doc, file or # folder. If so, return an error "file already exists". err = new Errors.InvalidNameError("file already exists") diff --git a/services/web/app/coffee/Features/Project/SafePath.coffee b/services/web/app/coffee/Features/Project/SafePath.coffee new file mode 100644 index 0000000000..55cb4ac0c5 --- /dev/null +++ b/services/web/app/coffee/Features/Project/SafePath.coffee @@ -0,0 +1,31 @@ +Path = require('path') + +BADCHAR_RX = /// + [ + \/ # no slashes + \* # no asterisk + \u0000-\u001F # no control characters (0-31) + \u007F # no delete + \u0080-\u009F # no unicode control characters (C1) + \uD800-\uDFFF # no unicode surrogate characters + ] + ///g + +BADFILE_RX = /// + (^\.$) # reject . as a filename + | (^\.\.$) # reject .. as a filename + | (^\s+) # reject leading space + | (\s+$) # reject trailing space + ///g + +MAX_PATH = 1024 # Maximum path length, in characters. This is fairly arbitrary. + +module.exports = SafePath = + + isCleanFilename: (filename) -> + return SafePath.isAllowedLength(filename) && + not filename.match(BADCHAR_RX) && + not filename.match(BADFILE_RX) + + 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 new file mode 100644 index 0000000000..93bd275408 --- /dev/null +++ b/services/web/test/unit/coffee/Project/SafePathTests.coffee @@ -0,0 +1,89 @@ +chai = require('chai') +assert = require('chai').assert +should = chai.should() +expect = chai.expect +sinon = require 'sinon' +modulePath = "../../../../app/js/Features/Project/SafePath" +SandboxedModule = require('sandboxed-module') + +describe 'SafePath', -> + beforeEach -> + @SafePath = SandboxedModule.require modulePath + + describe 'isCleanFilename', -> + it 'should accept a valid filename "main.tex"', -> + result = @SafePath.isCleanFilename 'main.tex' + result.should.equal true + + it 'should not accept an empty filename', -> + result = @SafePath.isCleanFilename '' + result.should.equal false + + it 'should not accept / anywhere', -> + result = @SafePath.isCleanFilename 'foo/bar' + result.should.equal false + + it 'should not accept .', -> + result = @SafePath.isCleanFilename '.' + result.should.equal false + + it 'should not accept ..', -> + result = @SafePath.isCleanFilename '..' + result.should.equal false + + it 'should not accept * anywhere', -> + result = @SafePath.isCleanFilename 'foo*bar' + result.should.equal false + + it 'should not accept leading whitespace', -> + result = @SafePath.isCleanFilename ' foobar.tex' + result.should.equal false + + it 'should not accept trailing whitespace', -> + result = @SafePath.isCleanFilename 'foobar.tex ' + result.should.equal false + + it 'should not accept leading and trailing whitespace', -> + result = @SafePath.isCleanFilename ' foobar.tex ' + result.should.equal false + + it 'should not accept control characters (0-31)', -> + result = @SafePath.isCleanFilename 'foo\u0010bar' + result.should.equal false + + it 'should not accept control characters (127, delete)', -> + result = @SafePath.isCleanFilename 'foo\u007fbar' + result.should.equal false + + it 'should not accept control characters (128-159)', -> + result = @SafePath.isCleanFilename 'foo\u0080\u0090bar' + result.should.equal false + + it 'should not accept surrogate characters (128-159)', -> + result = @SafePath.isCleanFilename 'foo\uD800\uDFFFbar' + result.should.equal false + + + + # it 'should not accept a trailing .', -> + # result = @SafePath.isCleanFilename 'hello.' + # result.should.equal false + + + # it 'should not accept \\', -> + # result = @SafePath.isCleanFilename 'foo\\bar' + # result.should.equal false + + describe 'isAllowedLength', -> + it 'should accept a valid path "main.tex"', -> + result = @SafePath.isAllowedLength 'main.tex' + result.should.equal true + + it 'should not accept an extremely long path', -> + longPath = new Array(1000).join("/subdir") + '/main.tex' + result = @SafePath.isAllowedLength longPath + result.should.equal false + + it 'should not accept an empty path', -> + result = @SafePath.isAllowedLength '' + result.should.equal false From c6f74d24f1f85a6b3aee2d642717985f28b87ee2 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 7 Feb 2018 15:08:10 +0000 Subject: [PATCH 2/5] add missing SafePath.clean function --- .../coffee/Features/Project/SafePath.coffee | 7 ++++ .../unit/coffee/Project/SafePathTests.coffee | 34 +++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/services/web/app/coffee/Features/Project/SafePath.coffee b/services/web/app/coffee/Features/Project/SafePath.coffee index 55cb4ac0c5..c1bd215777 100644 --- a/services/web/app/coffee/Features/Project/SafePath.coffee +++ b/services/web/app/coffee/Features/Project/SafePath.coffee @@ -22,6 +22,13 @@ MAX_PATH = 1024 # Maximum path length, in characters. This is fairly arbitrary. module.exports = SafePath = + clean: (filename) -> + filename = filename.replace BADCHAR_RX, '_' + # for BADFILE_RX replace any matches with an equal number of underscores + filename = filename.replace BADFILE_RX, (match) -> + return new Array(match.length + 1).join("_") + return filename + isCleanFilename: (filename) -> return SafePath.isAllowedLength(filename) && not filename.match(BADCHAR_RX) && diff --git a/services/web/test/unit/coffee/Project/SafePathTests.coffee b/services/web/test/unit/coffee/Project/SafePathTests.coffee index 93bd275408..50b44b7e2c 100644 --- a/services/web/test/unit/coffee/Project/SafePathTests.coffee +++ b/services/web/test/unit/coffee/Project/SafePathTests.coffee @@ -87,3 +87,37 @@ 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' + result.should.equal 'main.tex' + + 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 '_' + + it 'should replace ".." with "__"', -> + result = @SafePath.clean '..' + result.should.equal '__' + + it 'should replace a single trailing space with _', -> + result = @SafePath.clean 'foo ' + result.should.equal 'foo_' + + it 'should replace a multiple trailing spaces with ___', -> + result = @SafePath.clean 'foo ' + result.should.equal 'foo__' + + it 'should replace a single leading space with _', -> + result = @SafePath.clean ' foo' + result.should.equal '_foo' + + it 'should replace a multiple leading spaces with ___', -> + result = @SafePath.clean ' foo' + result.should.equal '__foo' + From 2a5ed0caf5ac66bdf7c9ec1bcd4c16f58c897a48 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 7 Feb 2018 15:22:00 +0000 Subject: [PATCH 3/5] use Errors.InvalidName instead of plain Error object --- .../Features/Project/ProjectEntityHandler.coffee | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee b/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee index 1ac12ec84e..69a518ba25 100644 --- a/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee +++ b/services/web/app/coffee/Features/Project/ProjectEntityHandler.coffee @@ -182,7 +182,7 @@ module.exports = ProjectEntityHandler = _addDocWithProject: (project, folder_id, docName, docLines, userId, callback = (error, doc, folder_id, path) ->)=> # check if docname is allowed if not SafePath.isCleanFilename docName - return callback new Error("invalid element name") + return callback new Errors.InvalidNameError("invalid element name") project_id = project._id logger.log project_id: project_id, folder_id: folder_id, doc_name: docName, "adding doc to project with project" confirmFolder project, folder_id, (folder_id)=> @@ -207,7 +207,7 @@ module.exports = ProjectEntityHandler = restoreDoc: (project_id, doc_id, name, callback = (error, doc, folder_id) ->) -> # check if docname is allowed (passed in from client so we check it) if not SafePath.isCleanFilename name - return callback new Error("invalid element name") + return callback new Errors.InvalidNameError("invalid element name") # getDoc will return the deleted doc's lines, but we don't actually remove # the deleted doc, just create a new one from its lines. ProjectEntityHandler.getDoc project_id, doc_id, include_deleted: true, (error, lines) -> @@ -217,7 +217,7 @@ module.exports = ProjectEntityHandler = addFileWithoutUpdatingHistory: (project_id, folder_id, fileName, path, userId, callback = (error, fileRef, folder_id, path, fileStoreUrl) ->)-> # check if file name is allowed if not SafePath.isCleanFilename fileName - return callback new Error("invalid element name") + return callback new Errors.InvalidNameError("invalid element name") ProjectGetter.getProject project_id, {rootFolder:true, name:true}, (err, project) -> if err? logger.err project_id:project_id, err:err, "error getting project for add file" @@ -362,7 +362,7 @@ module.exports = ProjectEntityHandler = addFolderWithProject: (project, parentFolder_id, folderName, callback = (err, folder, parentFolder_id)->) -> # check if folder name is allowed if not SafePath.isCleanFilename folderName - return callback new Error("invalid element name") + return callback new Errors.InvalidNameError("invalid element name") confirmFolder project, parentFolder_id, (parentFolder_id)=> folder = new Folder name: folderName logger.log project: project._id, parentFolder_id:parentFolder_id, folderName:folderName, "adding new folder" @@ -466,7 +466,7 @@ module.exports = ProjectEntityHandler = renameEntity: (project_id, entity_id, entityType, newName, userId, callback)-> # check if name is allowed if not SafePath.isCleanFilename newName - return callback new Error("invalid element name") + return callback new Errors.InvalidNameError("invalid element name") logger.log(entity_id: entity_id, project_id: project_id, ('renaming '+entityType)) if !entityType? logger.err err: "No entityType set", project_id: project_id, entity_id: entity_id @@ -613,7 +613,7 @@ module.exports = ProjectEntityHandler = # original check path.resolve("/", element.name) isnt "/#{element.name}" or element.name.match("/") # check if name is allowed if not SafePath.isCleanFilename element.name - e = new Error("invalid element name") + e = new Errors.InvalidNameError("invalid element name") logger.err project_id:project._id, folder_id:folder_id, element:element, type:type, "failed trying to insert element as name was invalid" return callback(e) From 57549d32be3829f81520364b8ececccc4b05e167 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 7 Feb 2018 15:28:26 +0000 Subject: [PATCH 4/5] remove unused path module --- services/web/app/coffee/Features/Project/SafePath.coffee | 2 -- 1 file changed, 2 deletions(-) diff --git a/services/web/app/coffee/Features/Project/SafePath.coffee b/services/web/app/coffee/Features/Project/SafePath.coffee index c1bd215777..12c7f012da 100644 --- a/services/web/app/coffee/Features/Project/SafePath.coffee +++ b/services/web/app/coffee/Features/Project/SafePath.coffee @@ -1,5 +1,3 @@ -Path = require('path') - BADCHAR_RX = /// [ \/ # no slashes From 9c36b38e2caf61696a4a119c47828b8dda2b06da Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Wed, 7 Feb 2018 15:43:56 +0000 Subject: [PATCH 5/5] make SafePath.coffee shareable between client and server code --- .../coffee/Features/Project/SafePath.coffee | 71 +++++++++++-------- 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/services/web/app/coffee/Features/Project/SafePath.coffee b/services/web/app/coffee/Features/Project/SafePath.coffee index 12c7f012da..f5b49cdff7 100644 --- a/services/web/app/coffee/Features/Project/SafePath.coffee +++ b/services/web/app/coffee/Features/Project/SafePath.coffee @@ -1,36 +1,47 @@ -BADCHAR_RX = /// - [ - \/ # no slashes - \* # no asterisk - \u0000-\u001F # no control characters (0-31) - \u007F # no delete - \u0080-\u009F # no unicode control characters (C1) - \uD800-\uDFFF # no unicode surrogate characters - ] - ///g +# This file is shared between the frontend and server code of web, so that +# filename validation is the same in both implementations. +# Both copies must be kept in sync: +# app/coffee/Features/Project/SafePath.coffee +# public/coffee/ide/directives/SafePath.coffee -BADFILE_RX = /// - (^\.$) # reject . as a filename - | (^\.\.$) # reject .. as a filename - | (^\s+) # reject leading space - | (\s+$) # reject trailing space - ///g +load = () -> + BADCHAR_RX = /// + [ + \/ # no slashes + \* # no asterisk + \u0000-\u001F # no control characters (0-31) + \u007F # no delete + \u0080-\u009F # no unicode control characters (C1) + \uD800-\uDFFF # no unicode surrogate characters + ] + ///g -MAX_PATH = 1024 # Maximum path length, in characters. This is fairly arbitrary. + BADFILE_RX = /// + (^\.$) # reject . as a filename + | (^\.\.$) # reject .. as a filename + | (^\s+) # reject leading space + | (\s+$) # reject trailing space + ///g -module.exports = SafePath = + MAX_PATH = 1024 # Maximum path length, in characters. This is fairly arbitrary. - clean: (filename) -> - filename = filename.replace BADCHAR_RX, '_' - # for BADFILE_RX replace any matches with an equal number of underscores - filename = filename.replace BADFILE_RX, (match) -> - return new Array(match.length + 1).join("_") - return filename + SafePath = + clean: (filename) -> + filename = filename.replace BADCHAR_RX, '_' + # for BADFILE_RX replace any matches with an equal number of underscores + filename = filename.replace BADFILE_RX, (match) -> + return new Array(match.length + 1).join("_") + return filename - isCleanFilename: (filename) -> - return SafePath.isAllowedLength(filename) && - not filename.match(BADCHAR_RX) && - not filename.match(BADFILE_RX) + isCleanFilename: (filename) -> + return SafePath.isAllowedLength(filename) && + not filename.match(BADCHAR_RX) && + not filename.match(BADFILE_RX) - isAllowedLength: (pathname) -> - return pathname.length > 0 && pathname.length <= MAX_PATH + isAllowedLength: (pathname) -> + return pathname.length > 0 && pathname.length <= MAX_PATH + +if define? + define [], load +else + module.exports = load() \ No newline at end of file