Skip to content

Commit ee336d2

Browse files
committed
Fix open redirect vulnerability for certain URLs
This fixes an open redirect vulnerability where we consider schema-relative URLs to be valid, meaning that an attacked could potentially get access to the token used to authenticate with the reporting API. I've reworked the code to rely on the Rails `url_from` method which is what `redirect_to` uses to ensure safe redirects. Jira-Issue: MAV-6742
1 parent be6fad3 commit ee336d2

3 files changed

Lines changed: 17 additions & 10 deletions

File tree

app/controllers/concerns/authentication_concern.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ module AuthenticationConcern
99
# only return true if the given URL is part of this domain (relative or absolute)
1010
# or part of the mavis reporting app, or localhost (to support local dev)
1111
def is_valid_redirect?(url)
12-
url.start_with?("/") || url.start_with?(request.base_url) ||
12+
!url_from(url).nil? ||
1313
url.start_with?(
1414
Settings.reporting_api.client_app.root_url || "http://localhost"
1515
)

spec/controllers/concerns/authentication_concern_spec.rb

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ def current_user
2929
.new(request: mock_request)
3030
end
3131

32-
describe "set_user_cis2_info" do
32+
describe "#set_user_cis2_info" do
3333
let(:user) { build(:user, cis2_info: nil) }
3434

3535
context "when cis2 is disabled" do
@@ -53,9 +53,6 @@ def current_user
5353
end
5454
end
5555

56-
# The commented out code block you provided is a pending RSpec example group for a method called
57-
# `#add_auth_code_to`. This method is expected to find or generate a `OneTimeToken` for a given user with
58-
# the `cis2_info` from the current session. The example group contains two contexts:
5956
describe "#add_auth_code_to" do
6057
let(:user) { create(:user) }
6158
let(:token) do
@@ -100,7 +97,7 @@ def current_user
10097
end
10198
end
10299

103-
describe "reporting_app_redirect_uri_with_auth_code_for" do
100+
describe "#reporting_app_redirect_uri_with_auth_code_for" do
104101
let(:session_cis2_info) { { "team_workgroup" => "r1l" } }
105102
let(:token) do
106103
build(:reporting_api_one_time_token, user: user, token: "mytoken")

spec/features/user_cis2_authentication_from_redirect_spec.rb

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,22 @@
2222
and_the_return_url_has_a_token_param_added_to_it
2323
end
2424

25-
scenario "someone has supplied their own external redirect url" do
25+
scenario "someone has supplied their own absolute redirect url" do
2626
given_a_test_team_is_setup_in_mavis_and_cis2
2727
when_i_go_to_the_start_page_with_a_redirect_uri_param_that_does_not_match_the_reporting_app
2828

2929
when_i_click_the_cis2_login_button
3030
then_i_see_the_dashboard
3131
end
3232

33+
scenario "someone has supplied their own schema-relative redirect url" do
34+
given_a_test_team_is_setup_in_mavis_and_cis2
35+
when_i_go_to_the_start_page_with_a_redirect_uri_param_that_is_schema_relative
36+
37+
when_i_click_the_cis2_login_button
38+
then_i_see_the_dashboard
39+
end
40+
3341
def given_a_test_team_is_setup_in_mavis_and_cis2
3442
@user = create(:user, uid: "123")
3543
@team = create(:team, users: [@user])
@@ -61,12 +69,14 @@ def when_i_go_to_the_start_page_with_a_redirect_uri_param_that_matches_the_repor
6169
visit [start_path, "redirect_uri=#{uri}"].join("?")
6270
end
6371

64-
def redirect_elsewhere_url
65-
"https://some.example.com/redirect/elsewhere"
72+
def when_i_go_to_the_start_page_with_a_redirect_uri_param_that_is_schema_relative
73+
uri =
74+
URI.encode_uri_component("https://some.example.com/redirect/elsewhere")
75+
visit [start_path, "redirect_uri=#{uri}"].join("?")
6676
end
6777

6878
def when_i_go_to_the_start_page_with_a_redirect_uri_param_that_does_not_match_the_reporting_app
69-
uri = URI.encode_uri_component(redirect_elsewhere_url)
79+
uri = URI.encode_uri_component("//some.example.com/redirect/elsewhere")
7080
visit [start_path, "redirect_uri=#{uri}"].join("?")
7181
end
7282

0 commit comments

Comments
 (0)