diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 9a440fad8e..cf162d2581 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/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 diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee index 3ffd08e08b..434c80de61 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/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"}}