From 40ac895ef72db6d253eeea30588f8a788ba9e4af Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Mon, 25 Mar 2019 09:02:42 +1100 Subject: [PATCH] SECURITY: properly validate return URL for SSO Previously carefully crafted URLs could redirect off site --- app/controllers/session_controller.rb | 8 +++++--- spec/requests/session_controller_spec.rb | 11 +++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 74ba5ab2047..fa46aad1af6 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -174,7 +174,7 @@ class SessionController < ApplicationController begin uri = URI(return_path) if (uri.hostname == Discourse.current_hostname) - return_path = uri.request_uri + return_path = uri.to_s elsif !SiteSetting.sso_allows_all_return_paths return_path = path("/") end @@ -183,8 +183,10 @@ class SessionController < ApplicationController end end - # never redirects back to sso in an sso loop - if return_path.start_with?(path("/session/sso")) + # this can be done more surgically with a regex + # but it the edge case of never supporting redirects back to + # any url with `/session/sso` in it anywhere is reasonable + if return_path.include?(path("/session/sso")) return_path = path("/") end diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index afc5a1e1de0..443c1e801bf 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -403,6 +403,17 @@ RSpec.describe SessionController do expect(logged_on_user.admin).to eq(true) end + it 'does not redirect offsite' do + sso = get_sso("#{Discourse.base_url}//site.com/xyz") + sso.external_id = '666' + sso.email = 'bob@bob.com' + sso.name = 'Sam Saffron' + sso.username = 'sam' + + get "/session/sso_login", params: Rack::Utils.parse_query(sso.payload), headers: headers + expect(response).to redirect_to("#{Discourse.base_url}//site.com/xyz") + end + it 'redirects to a non-relative url' do sso = get_sso("#{Discourse.base_url}/b/") sso.external_id = '666' # the number of the beast