From 0aaeb6671e9d4550bb01f949c51ea67415b58db5 Mon Sep 17 00:00:00 2001
From: Shane Kilkelly <shane@kilkelly.me>
Date: Mon, 24 Aug 2015 11:53:33 +0100
Subject: [PATCH] Keep password reset token in session, and strip it from reset
 page url.

This fixes an issue where the reset token was leaked in the referrer header
when navigating away from the password reset page to an external site.

Now we get the token from the query string, store it in the session,
then redirect to the bare url of the password reset page, which then
uses the stored token to render the reset form.
---
 .../PasswordResetController.coffee            | 16 +++++--
 .../PasswordResetControllerTests.coffee       | 47 ++++++++++++++++++-
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee
index 06bd4ba023..d574a6838f 100644
--- a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee
+++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee
@@ -6,12 +6,12 @@ module.exports =
 
 	renderRequestResetForm: (req, res)->
 		logger.log "rendering request reset form"
-		res.render "user/passwordReset", 
+		res.render "user/passwordReset",
 			title:"reset_password"
 
 	requestReset: (req, res)->
 		email = req.body.email.trim().toLowerCase()
-		opts = 
+		opts =
 			endpointName: "password_reset_rate_limit"
 			timeInterval: 60
 			subjectName: req.ip
@@ -28,17 +28,23 @@ module.exports =
 					res.send 404, {message: req.i18n.translate("cant_find_email")}
 
 	renderSetPasswordForm: (req, res)->
-		res.render "user/setPassword", 
+		if req.query.passwordResetToken?
+			req.session.resetToken = req.query.passwordResetToken
+			return res.redirect('/user/password/set')
+		if !req.session.resetToken?
+			return res.redirect('/user/password/reset')
+		res.render "user/setPassword",
 			title:"set_password"
-			passwordResetToken:req.query.passwordResetToken
+			passwordResetToken: req.session.resetToken
 
 	setNewUserPassword: (req, res)->
 		{passwordResetToken, password} = req.body
 		if !password? or password.length == 0 or !passwordResetToken? or passwordResetToken.length == 0
 			return res.sendStatus 400
+		delete req.session.resetToken
 		PasswordResetHandler.setNewUserPassword passwordResetToken?.trim(), password?.trim(), (err, found) ->
 			return next(err) if err?
 			if found
 				res.sendStatus 200
 			else
-				res.send 404, {message: req.i18n.translate("password_reset_token_expired")}
\ No newline at end of file
+				res.send 404, {message: req.i18n.translate("password_reset_token_expired")}
diff --git a/services/web/test/UnitTests/coffee/PasswordReset/PasswordResetControllerTests.coffee b/services/web/test/UnitTests/coffee/PasswordReset/PasswordResetControllerTests.coffee
index efc4d9cbde..140bf0b9d4 100644
--- a/services/web/test/UnitTests/coffee/PasswordReset/PasswordResetControllerTests.coffee
+++ b/services/web/test/UnitTests/coffee/PasswordReset/PasswordResetControllerTests.coffee
@@ -14,7 +14,7 @@ describe "PasswordResetController", ->
 		@PasswordResetHandler =
 			generateAndEmailResetToken:sinon.stub()
 			setNewUserPassword:sinon.stub()
-		@RateLimiter = 
+		@RateLimiter =
 			addCount: sinon.stub()
 		@PasswordResetController = SandboxedModule.require modulePath, requires:
 			"settings-sharelatex":@settings
@@ -32,6 +32,8 @@ describe "PasswordResetController", ->
 				password:@password
 			i18n:
 				translate:->
+			session: {}
+			query: {}
 
 		@res = {}
 
@@ -86,6 +88,9 @@ describe "PasswordResetController", ->
 
 	describe "setNewUserPassword", ->
 
+		beforeEach ->
+			@req.session.resetToken = @token
+
 		it "should tell the user handler to reset the password", (done)->
 			@PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true)
 			@res.sendStatus = (code)=>
@@ -119,5 +124,45 @@ describe "PasswordResetController", ->
 				done()
 			@PasswordResetController.setNewUserPassword @req, @res
 
+		it "should clear the session.resetToken", (done) ->
+			@PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true)
+			@res.sendStatus = (code)=>
+				code.should.equal 200
+				@req.session.should.not.have.property 'resetToken'
+				done()
+			@PasswordResetController.setNewUserPassword @req, @res
 
+	describe "renderSetPasswordForm", ->
 
+		describe "with token in query-string", ->
+			beforeEach ->
+				@req.query.passwordResetToken = @token
+
+			it "should set session.resetToken and redirect", (done) ->
+				@req.session.should.not.have.property 'resetToken'
+				@res.redirect = (path) =>
+					path.should.equal '/user/password/set'
+					@req.session.resetToken.should.equal @token
+					done()
+				@PasswordResetController.renderSetPasswordForm(@req, @res)
+
+		describe "without a token in query-string", ->
+
+			describe "with token in session", ->
+				beforeEach ->
+					@req.session.resetToken = @token
+
+				it "should render the page, passing the reset token", (done) ->
+					@res.render = (template_path, options) =>
+						options.passwordResetToken.should.equal @req.session.resetToken
+						done()
+					@PasswordResetController.renderSetPasswordForm(@req, @res)
+
+			describe "without a token in session", ->
+
+				it "should redirect to the reset request page", (done) ->
+					@res.redirect = (path) =>
+						path.should.equal "/user/password/reset"
+						@req.session.should.not.have.property 'resetToken'
+						done()
+					@PasswordResetController.renderSetPasswordForm(@req, @res)