Merge pull request #314 from sharelatex/bg-validate-filenames-server

server side check for valid filenames

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 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)=>
@@ -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 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) ->
@@ -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 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"
@@ -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 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"
@@ -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 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
@@ -593,8 +610,10 @@ module.exports = ProjectEntityHandler =
 			return callback(e)
 		type = sanitizeTypeOfElement type
 
-		if path.resolve("/", element.name) isnt "/#{element.name}" or element.name.match("/")
-			e = new Error("invalid element name")
+		# 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 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)
 
@@ -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")

services/web/app/coffee/Features/Project/SafePath.coffee

@@ -0,0 +1,47 @@
+# 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
+
+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
+
+	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.
+
+	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)
+
+		isAllowedLength: (pathname) ->
+			return pathname.length > 0 && pathname.length <= MAX_PATH
+
+if define?
+	define [], load
+else
+	module.exports = load()

services/web/test/unit/coffee/Project/SafePathTests.coffee

@@ -0,0 +1,123 @@
+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
+	
+	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'		
+