Improve error reporting and show 404 when project ids are malformed

services/web/app.coffee

@@ -18,18 +18,6 @@ argv = require("optimist")
 	.usage("Usage: $0")
 	.argv
 
-Server.app.use (error, req, res, next) ->
-	if error?.code is 'EBADCSRFTOKEN'
-		logger.log err: error,url:req.url, method:req.method, user:req?.sesson?.user, "invalid csrf"
-		res.sendStatus(403)
-		return
-	logger.error err: error, url:req.url, method:req.method, user:req?.sesson?.user, "error passed to top level next middlewear"
-	res.statusCode = error.status or 500
-	if res.statusCode == 500
-		res.end("Oops, something went wrong with your request, sorry. If this continues, please contact us at #{Settings.adminEmail}")
-	else
-		res.end()
-
 if Settings.catchErrors
 	process.removeAllListeners "uncaughtException"
 	process.on "uncaughtException", (error) ->

services/web/app/coffee/Features/Authorization/AuthorizationManager.coffee

@@ -3,6 +3,7 @@ Project = require("../../models/Project").Project
 User = require("../../models/User").User
 PrivilegeLevels = require("./PrivilegeLevels")
 PublicAccessLevels = require("./PublicAccessLevels")
+Errors = require("../Errors/Errors")
 
 module.exports = AuthorizationManager =
 	# Get the privilege level that the user has for the project
@@ -14,8 +15,10 @@ module.exports = AuthorizationManager =
 		getPublicAccessLevel = () ->
 			Project.findOne { _id: project_id }, { publicAccesLevel: 1 }, (error, project) ->
 				return callback(error) if error?
+				if !project?
+					return callback new Errors.NotFoundError("no project found with id #{project_id}")
 				if project.publicAccesLevel == PublicAccessLevels.READ_ONLY
-					return callback null, PrivilegeLevels.READ_ONLY
+					return callback null, PrivilegeLevels.READ_ONLY, true
 				else if project.publicAccesLevel == PublicAccessLevels.READ_AND_WRITE
 					return callback null, PrivilegeLevels.READ_AND_WRITE, true
 				else

services/web/app/coffee/Features/Authorization/AuthorizationMiddlewear.coffee

@@ -1,6 +1,8 @@
 AuthorizationManager = require("./AuthorizationManager")
 async = require "async"
 logger = require "logger-sharelatex"
+ObjectId = require("mongojs").ObjectId
+Errors = require "../Errors/Errors"
 
 module.exports = AuthorizationMiddlewear =
 	ensureUserCanReadMultipleProjects: (req, res, next) ->
@@ -83,6 +85,8 @@ module.exports = AuthorizationMiddlewear =
 		project_id = req.params?.project_id or req.params?.Project_id
 		if !project_id?
 			return callback(new Error("Expected project_id in request parameters"))
+		if !ObjectId.isValid(project_id)
+			return callback(new Errors.NotFoundError("invalid project_id: #{project_id}"))
 		AuthorizationMiddlewear._getUserId req, (error, user_id) ->
 			return callback(error) if error?
 			callback(null, user_id, project_id)

services/web/app/coffee/Features/Collaborators/CollaboratorsHandler.coffee

@@ -7,12 +7,13 @@ ContactManager = require "../Contacts/ContactManager"
 CollaboratorsEmailHandler = require "./CollaboratorsEmailHandler"
 async = require "async"
 PrivilegeLevels = require "../Authorization/PrivilegeLevels"
+Errors = require "../Errors/Errors"
 
 module.exports = CollaboratorsHandler =
 	getMemberIdsWithPrivilegeLevels: (project_id, callback = (error, members) ->) ->
 		Project.findOne { _id: project_id }, { owner_ref: 1, collaberator_refs: 1, readOnly_refs: 1 }, (error, project) ->
 			return callback(error) if error?
-			return callback null, null if !project?
+			return callback new Errors.NotFoundError("no project found with id #{project_id}") if !project?
 			members = []
 			members.push { id: project.owner_ref.toString(), privilegeLevel: PrivilegeLevels.OWNER }
 			for member_id in project.readOnly_refs or []

services/web/app/coffee/Features/Errors/ErrorController.coffee

@@ -1,5 +1,24 @@
+Errors = require "./Errors"
+logger = require "logger-sharelatex"
+
 module.exports = ErrorController =
 	notFound: (req, res)->
-		res.statusCode = 404
+		res.status(404)
 		res.render 'general/404',
-			title: "page_not_found"
+			title: "page_not_found"
+
+	serverError: (req, res)->
+		res.status(500)
+		res.render 'general/500',
+			title: "Server Error"
+	
+	handleError: (error, req, res, next) ->
+		if error?.code is 'EBADCSRFTOKEN'
+			logger.log err: error,url:req.url, method:req.method, user:req?.sesson?.user, "invalid csrf"
+			res.sendStatus(403)
+			return
+		logger.error err: error, url:req.url, method:req.method, user:req?.sesson?.user, "error passed to top level next middlewear"
+		if error instanceof Errors.NotFoundError
+			ErrorController.notFound req, res
+		else
+			ErrorController.serverError req, res

services/web/app/coffee/Features/Errors/Errors.coffee

@@ -0,0 +1,9 @@
+NotFoundError = (message) ->
+	error = new Error(message)
+	error.name = "NotFoundError"
+	error.__proto__ = NotFoundError.prototype
+	return error
+NotFoundError.prototype.__proto__ = Error.prototype
+
+module.exports = Errors =
+	NotFoundError: NotFoundError

services/web/app/coffee/infrastructure/Server.coffee

@@ -30,6 +30,8 @@ OldAssetProxy = require("./OldAssetProxy")
 translations = require("translations-sharelatex").setup(Settings.i18n)
 Modules = require "./Modules"
 
+ErrorController = require "../Features/Errors/ErrorController"
+
 metrics.mongodb.monitor(Path.resolve(__dirname + "/../../../node_modules/mongojs/node_modules/mongodb"), logger)
 metrics.mongodb.monitor(Path.resolve(__dirname + "/../../../node_modules/mongoose/node_modules/mongodb"), logger)
 
@@ -136,6 +138,8 @@ app.use(webRouter)
 
 router = new Router(webRouter, apiRouter)
 
+app.use ErrorController.handleError
+
 module.exports =
 	app: app
 	server: server

services/web/app/views/general/500.jade

@@ -0,0 +1,16 @@
+extends ../layout
+
+block content
+	.content
+		.container
+			.row
+				.col-md-8.col-md-offset-2.text-center
+					.page-header
+						h2 Oh dear, something went wrong.
+					p: img(src="/img/lion-sad-128.png", alt="Sad Lion")
+					p
+						| Something went wrong with your request, sorry. Our staff are probably looking into this, but it continues, please contact us at #{settings.adminEmail}
+					p
+						a(href="/")
+							i.fa.fa-arrow-circle-o-left
+							|  #{translate("take_me_home")}

services/web/test/UnitTests/coffee/Authorization/AuthorizationManagerTests.coffee

@@ -4,6 +4,7 @@ should = chai.should()
 expect = chai.expect
 modulePath = "../../../../app/js/Features/Authorization/AuthorizationManager.js"
 SandboxedModule = require('sandboxed-module')
+Errors = require "../../../../app/js/Features/Errors/Errors.js"
 
 describe "AuthorizationManager", ->
 	beforeEach ->
@@ -11,6 +12,7 @@ describe "AuthorizationManager", ->
 			"../Collaborators/CollaboratorsHandler": @CollaboratorsHandler = {}
 			"../../models/Project": Project: @Project = {}
 			"../../models/User": User: @User = {}
+			"../Errors/Errors": Errors
 		@user_id = "user-id-1"
 		@project_id = "project-id-1"
 		@callback = sinon.stub()
@@ -91,6 +93,16 @@ describe "AuthorizationManager", ->
 				
 				it "should return the public privilege level", ->
 					@callback.calledWith(null, "readAndWrite", true).should.equal true
+		
+		describe "when the project doesn't exist", ->
+			beforeEach ->
+				@Project.findOne
+					.withArgs({ _id: @project_id }, { publicAccesLevel: 1 })
+					.yields(null, null)
+				
+			it "should return a NotFoundError", ->
+				@AuthorizationManager.getPrivilegeLevelForProject @user_id, @project_id, (error) ->
+					error.should.be.instanceof Errors.NotFoundError
 	
 	describe "canUserReadProject", ->
 		beforeEach ->

services/web/test/UnitTests/coffee/Authorization/AuthorizationMiddlewearTests.coffee

@@ -4,16 +4,21 @@ should = chai.should()
 expect = chai.expect
 modulePath = "../../../../app/js/Features/Authorization/AuthorizationMiddlewear.js"
 SandboxedModule = require('sandboxed-module')
+Errors = require "../../../../app/js/Features/Errors/Errors.js"
 
 describe "AuthorizationMiddlewear", ->
 	beforeEach ->
 		@AuthorizationMiddlewear = SandboxedModule.require modulePath, requires:
 			"./AuthorizationManager": @AuthorizationManager = {}
 			"logger-sharelatex": {log: () ->}
+			"mongojs": ObjectId: @ObjectId = {}
+			"../Errors/Errors": Errors
 		@user_id = "user-id-123"
 		@project_id = "project-id-123"
 		@req = {}
 		@res = {}
+		@ObjectId.isValid = sinon.stub()
+		@ObjectId.isValid.withArgs(@project_id).returns true
 		@next = sinon.stub()
 	
 	METHODS_TO_TEST = {
@@ -90,6 +95,17 @@ describe "AuthorizationMiddlewear", ->
 							@AuthorizationMiddlewear.redirectToRestricted
 								.calledWith(@req, @res, @next)
 								.should.equal true
+				
+				describe "with malformed project id", ->
+					beforeEach ->
+						@req.params =
+							project_id: "blah"
+						@ObjectId.isValid = sinon.stub().returns false
+					
+					it "should return a not found error", (done) ->
+						@AuthorizationMiddlewear[middlewearMethod] @req, @res, (error) ->
+							error.should.be.instanceof Errors.NotFoundError
+							done()
 	
 	describe "ensureUserIsSiteAdmin", ->
 		beforeEach ->

services/web/test/UnitTests/coffee/Collaborators/CollaboratorsHandlerTests.coffee

@@ -5,6 +5,7 @@ path = require('path')
 sinon = require('sinon')
 modulePath = path.join __dirname, "../../../../app/js/Features/Collaborators/CollaboratorsHandler"
 expect = require("chai").expect
+Errors = require "../../../../app/js/Features/Errors/Errors.js"
 
 describe "CollaboratorsHandler", ->
 	beforeEach ->
@@ -16,6 +17,7 @@ describe "CollaboratorsHandler", ->
 			"../../models/Project": Project: @Project = {}
 			"../Project/ProjectEntityHandler": @ProjectEntityHandler = {}
 			"./CollaboratorsEmailHandler": @CollaboratorsEmailHandler = {}
+			"../Errors/Errors": Errors
 
 		@project_id = "mock-project-id"
 		@user_id = "mock-user-id"
@@ -24,25 +26,35 @@ describe "CollaboratorsHandler", ->
 		@callback = sinon.stub()
 	
 	describe "getMemberIdsWithPrivilegeLevels", ->
-		beforeEach ->
-			@Project.findOne = sinon.stub()
-			@Project.findOne.withArgs({_id: @project_id}, {owner_ref: 1, collaberator_refs: 1, readOnly_refs: 1}).yields(null, @project = {
-				owner_ref: [ "owner-ref" ]
-				readOnly_refs: [ "read-only-ref-1", "read-only-ref-2" ]
-				collaberator_refs: [ "read-write-ref-1", "read-write-ref-2" ]
-			})
-			@CollaboratorHandler.getMemberIdsWithPrivilegeLevels @project_id, @callback
+		describe "with project", ->
+			beforeEach ->
+				@Project.findOne = sinon.stub()
+				@Project.findOne.withArgs({_id: @project_id}, {owner_ref: 1, collaberator_refs: 1, readOnly_refs: 1}).yields(null, @project = {
+					owner_ref: [ "owner-ref" ]
+					readOnly_refs: [ "read-only-ref-1", "read-only-ref-2" ]
+					collaberator_refs: [ "read-write-ref-1", "read-write-ref-2" ]
+				})
+				@CollaboratorHandler.getMemberIdsWithPrivilegeLevels @project_id, @callback
 
-		it "should return an array of member ids with their privilege levels", ->
-			@callback
-				.calledWith(null, [
-					{ id: "owner-ref", privilegeLevel: "owner" }
-					{ id: "read-only-ref-1", privilegeLevel: "readOnly" }
-					{ id: "read-only-ref-2", privilegeLevel: "readOnly" }
-					{ id: "read-write-ref-1", privilegeLevel: "readAndWrite" }
-					{ id: "read-write-ref-2", privilegeLevel: "readAndWrite" }
-				])
-				.should.equal true
+			it "should return an array of member ids with their privilege levels", ->
+				@callback
+					.calledWith(null, [
+						{ id: "owner-ref", privilegeLevel: "owner" }
+						{ id: "read-only-ref-1", privilegeLevel: "readOnly" }
+						{ id: "read-only-ref-2", privilegeLevel: "readOnly" }
+						{ id: "read-write-ref-1", privilegeLevel: "readAndWrite" }
+						{ id: "read-write-ref-2", privilegeLevel: "readAndWrite" }
+					])
+					.should.equal true
+		
+		describe "with a missing project", ->
+			beforeEach ->
+				@Project.findOne = sinon.stub().yields(null, null)
+			
+			it "should return a NotFoundError", (done) ->
+				@CollaboratorHandler.getMemberIdsWithPrivilegeLevels @project_id, (error) ->
+					error.should.be.instanceof Errors.NotFoundError
+					done()
 	
 	describe "getMemberIds", ->
 		beforeEach ->

services/web/test/acceptance/coffee/AuthorizationTests.coffee

@@ -1,83 +1,8 @@
-request = require("request")
 expect = require("chai").expect
-async = require "async"
-settings = require("settings-sharelatex")
-{db} = require("../../../app/js/infrastructure/mongojs")
-
-count = 0
-BASE_URL = "http://localhost:3000"
-
-request = request.defaults({
-	baseUrl: BASE_URL,
-	followRedirect: false
-})
-
-class User
-	constructor: (options = {}) ->
-		@email = "acceptance-test-#{count}@example.com"
-		@password = "acceptance-test-#{count}-password"
-		count++
-		@jar = request.jar()
-		@request = request.defaults({
-			jar: @jar
-		})
-	
-	login: (callback = (error) ->) ->
-		@getCsrfToken (error) =>
-			return callback(error) if error?
-			@request.post {
-				url: "/register" # Register will log in, but also ensure user exists
-				json:
-					email: @email
-					password: @password
-			}, (error, response, body) =>
-				return callback(error) if error?
-				db.users.findOne {email: @email}, (error, user) =>
-					return callback(error) if error?
-					@id = user?._id?.toString()
-					callback()
-	
-	createProject: (name, callback = (error, project_id) ->) ->
-		@request.post {
-			url: "/project/new",
-			json:
-				projectName: name
-		}, (error, response, body) ->
-			return callback(error) if error?
-			if !body?.project_id?
-				console.error "SOMETHING WENT WRONG CREATING PROJECT", response.statusCode, response.headers["location"], body
-			callback(null, body.project_id)
-	
-	addUserToProject: (project_id, email, privileges, callback = (error, user) ->) ->
-		@request.post {
-			url: "/project/#{project_id}/users",
-			json: {email, privileges}
-		}, (error, response, body) ->
-			return callback(error) if error?
-			callback(null, body.user)
-	
-	makePublic: (project_id, level, callback = (error) ->) ->
-		@request.post {
-			url: "/project/#{project_id}/settings/admin",
-			json:
-				publicAccessLevel: level
-		}, (error, response, body) ->
-			return callback(error) if error?
-			callback(null)
-
-	getCsrfToken: (callback = (error) ->) ->
-		@request.get {
-			url: "/register"
-		}, (err, response, body) =>
-			return callback(error) if error?
-			csrfMatches = body.match("window.csrfToken = \"(.*?)\";")
-			if !csrfMatches?
-				return callback(new Error("no csrf token found"))
-			@request = @request.defaults({
-				headers:
-					"x-csrf-token": csrfMatches[1]
-			})
-			callback()
+async = require("async")
+User = require "./helpers/User"
+request = require "./helpers/request"
+settings = require "settings-sharelatex"
 
 try_read_access = (user, project_id, test, callback) ->
 	async.series [

services/web/test/acceptance/coffee/ProjectCRUDTests.coffee

@@ -0,0 +1,21 @@
+expect = require("chai").expect
+async = require("async")
+User = require "./helpers/User"
+
+describe "Project CRUD", ->
+	before (done) ->
+		@user = new User()
+		@user.login done
+
+	describe "when project doesn't exist", ->
+		it "should return 404", (done) ->
+			@user.request.get "/project/aaaaaaaaaaaaaaaaaaaaaaaa", (err, res, body) ->
+				expect(res.statusCode).to.equal 404
+				done()
+		
+	describe "when project has malformed id", ->
+		it "should return 404", (done) ->
+			@user.request.get "/project/blah", (err, res, body) ->
+				expect(res.statusCode).to.equal 404
+				done()
+		

services/web/test/acceptance/coffee/helpers/User.coffee

@@ -0,0 +1,74 @@
+request = require("./request")
+settings = require("settings-sharelatex")
+{db} = require("../../../../app/js/infrastructure/mongojs")
+
+count = 0
+
+class User
+	constructor: (options = {}) ->
+		@email = "acceptance-test-#{count}@example.com"
+		@password = "acceptance-test-#{count}-password"
+		count++
+		@jar = request.jar()
+		@request = request.defaults({
+			jar: @jar
+		})
+	
+	login: (callback = (error) ->) ->
+		@getCsrfToken (error) =>
+			return callback(error) if error?
+			@request.post {
+				url: "/register" # Register will log in, but also ensure user exists
+				json:
+					email: @email
+					password: @password
+			}, (error, response, body) =>
+				return callback(error) if error?
+				db.users.findOne {email: @email}, (error, user) =>
+					return callback(error) if error?
+					@id = user?._id?.toString()
+					callback()
+	
+	createProject: (name, callback = (error, project_id) ->) ->
+		@request.post {
+			url: "/project/new",
+			json:
+				projectName: name
+		}, (error, response, body) ->
+			return callback(error) if error?
+			if !body?.project_id?
+				console.error "SOMETHING WENT WRONG CREATING PROJECT", response.statusCode, response.headers["location"], body
+			callback(null, body.project_id)
+	
+	addUserToProject: (project_id, email, privileges, callback = (error, user) ->) ->
+		@request.post {
+			url: "/project/#{project_id}/users",
+			json: {email, privileges}
+		}, (error, response, body) ->
+			return callback(error) if error?
+			callback(null, body.user)
+	
+	makePublic: (project_id, level, callback = (error) ->) ->
+		@request.post {
+			url: "/project/#{project_id}/settings/admin",
+			json:
+				publicAccessLevel: level
+		}, (error, response, body) ->
+			return callback(error) if error?
+			callback(null)
+
+	getCsrfToken: (callback = (error) ->) ->
+		@request.get {
+			url: "/register"
+		}, (err, response, body) =>
+			return callback(error) if error?
+			csrfMatches = body.match("window.csrfToken = \"(.*?)\";")
+			if !csrfMatches?
+				return callback(new Error("no csrf token found"))
+			@request = @request.defaults({
+				headers:
+					"x-csrf-token": csrfMatches[1]
+			})
+			callback()
+
+module.exports = User

services/web/test/acceptance/coffee/helpers/request.coffee

@@ -0,0 +1,5 @@
+BASE_URL = "http://localhost:3000"
+module.exports = require("request").defaults({
+	baseUrl: BASE_URL,
+	followRedirect: false
+})