Merge pull request #1499 from sharelatex/ta-open-redirect-fix

Prevent Open Redirects

GitOrigin-RevId: 8cd2ead74de60f47b728ac227c21440281b111a5

services/web/app/coffee/Features/Authentication/AuthenticationController.coffee

@@ -15,6 +15,7 @@ NotificationsBuilder = require("../Notifications/NotificationsBuilder")
 SudoModeHandler = require '../SudoMode/SudoModeHandler'
 V1Api = require "../V1/V1Api"
 {User} = require "../../models/User"
+{ URL } = require('url')
 
 module.exports = AuthenticationController =
 
@@ -225,7 +226,8 @@ module.exports = AuthenticationController =
 				!/^\/(socket.io|js|stylesheets|img)\/.*$/.test(value) &&
 				!/^.*\.(png|jpeg|svg)$/.test(value)
 		)
-			req.session.postLoginRedirect = value
+			safePath = AuthenticationController._getSafeRedirectPath(value)
+			req.session.postLoginRedirect = safePath
 
 	_redirectToLoginOrRegisterPage: (req, res)->
 		if (req.query.zipUrl? or req.query.project_name? or req.path == '/user/subscription/new')
@@ -261,8 +263,17 @@ module.exports = AuthenticationController =
 		callback()
 
 	_getRedirectFromSession: (req) ->
-		return req?.session?.postLoginRedirect || null
+		value = req?.session?.postLoginRedirect
+		safePath = AuthenticationController._getSafeRedirectPath(value) if value
+		return safePath || null
 
 	_clearRedirectFromSession: (req) ->
 		if req.session?
 			delete req.session.postLoginRedirect
+
+	_getSafeRedirectPath: (value) ->
+		baseURL = Settings.siteUrl # base URL is required to construct URL from path
+		url = new URL(value, baseURL)
+		safePath = "#{url.pathname}#{url.search}#{url.hash}"
+		safePath = undefined if safePath == '/'
+		safePath

services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee

@@ -24,7 +24,7 @@ describe "AuthenticationController", ->
 			"../User/UserHandler": @UserHandler = {setupLoginData:sinon.stub()}
 			"../Analytics/AnalyticsManager": @AnalyticsManager = { recordEvent: sinon.stub() }
 			"logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub(), err: sinon.stub() }
-			"settings-sharelatex": {}
+			"settings-sharelatex": { siteUrl:  'http://www.foo.bar' }
 			"passport": @passport =
 				authenticate: sinon.stub().returns(sinon.stub())
 			"../User/UserSessionsManager": @UserSessionsManager =
@@ -654,6 +654,10 @@ describe "AuthenticationController", ->
 			@AuthenticationController.setRedirectInSession(@req, '/somewhere/specific')
 			expect(@req.session.postLoginRedirect).to.equal "/somewhere/specific"
 
+		it 'should not allow open redirects', ->
+			@AuthenticationController.setRedirectInSession(@req, 'https://evil.com')
+			expect(@req.session.postLoginRedirect).to.be.undefined
+
 		describe 'with a png', ->
 			beforeEach ->
 				@req = {session: {}}
@@ -672,12 +676,44 @@ describe "AuthenticationController", ->
 				expect(@req.session.postLoginRedirect).to.equal undefined
 
 	describe '_getRedirectFromSession', ->
-		beforeEach ->
-			@req = {session: {postLoginRedirect: "/a?b=c"}}
-
 		it 'should get redirect property from session', ->
+			@req = session: { postLoginRedirect: '/a?b=c' }
 			expect(@AuthenticationController._getRedirectFromSession(@req)).to.equal "/a?b=c"
 
+		it 'should not allow open redirects', ->
+			@req = session: { postLoginRedirect: 'https://evil.com' }
+			expect(@AuthenticationController._getRedirectFromSession(@req)).to.be.null
+
+		it 'handle null values', ->
+			@req = session: {}
+			expect(@AuthenticationController._getRedirectFromSession(@req)).to.be.null
+
+	describe '_getSafeRedirectPath', ->
+		it 'sanitize redirect path to prevent open redirects', ->
+			expect(
+				@AuthenticationController._getSafeRedirectPath('https://evil.com')
+			).to.be.undefined
+
+			expect(
+				@AuthenticationController._getSafeRedirectPath('//evil.com')
+			).to.be.undefined
+
+			expect(
+				@AuthenticationController._getSafeRedirectPath('//ol.com/evil')
+			).to.equal '/evil'
+
+			expect(
+				@AuthenticationController._getSafeRedirectPath('////evil.com')
+			).to.be.undefined
+
+			expect(
+				@AuthenticationController._getSafeRedirectPath('%2F%2Fevil.com')
+			).to.equal '/%2F%2Fevil.com'
+
+			expect(
+				@AuthenticationController._getSafeRedirectPath('.evil.com')
+			).to.equal '/.evil.com'
+
 	describe '_clearRedirectFromSession', ->
 		beforeEach ->
 			@req = {session: {postLoginRedirect: "/a?b=c"}}