Skip to content

Commit 469572c

Browse files
committed
Fix XSS on error messages in flash messages from external providers.
1 parent 00fa8ef commit 469572c

File tree

3 files changed

+139
-1
lines changed

3 files changed

+139
-1
lines changed

src/api-umbrella/web-app/app/controllers/admin/admins/omniauth_callbacks_controller.rb

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ def login
9898
ActionController::Base.helpers.content_tag(:a, "contact us", :href => ApiUmbrellaConfig[:contact_url]),
9999
" for further assistance.",
100100
])
101+
flash[:html_safe] = true
101102

102103
redirect_to new_admin_session_path
103104
end
@@ -111,6 +112,7 @@ def email_unverified_error
111112
ActionController::Base.helpers.content_tag(:a, "contact us", :href => ApiUmbrellaConfig[:contact_url]),
112113
" for further assistance.",
113114
])
115+
flash[:html_safe] = true
114116

115117
redirect_to new_admin_session_path
116118
end
@@ -121,6 +123,7 @@ def mfa_required_error
121123
ActionController::Base.helpers.content_tag(:a, "contact us", :href => ApiUmbrellaConfig[:contact_url]),
122124
" for further assistance.",
123125
])
126+
flash[:html_safe] = true
124127

125128
redirect_to new_admin_session_path
126129
end

src/api-umbrella/web-app/app/views/layouts/application.html.erb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
<% flash.each do |flash_type, message| %>
1313
<% next unless(message.kind_of?(String)) %>
1414
<div class="alert <%= bootstrap_class_for(flash_type) %>">
15-
<%= message.html_safe %>
15+
<%= if(flash[:html_safe]) then message.html_safe else message end %>
1616
</div>
1717
<% end %>
1818

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
require_relative "../test_helper"
2+
3+
class Test::AdminUi::TestFlashMessagesHtmlSafety < Minitest::Capybara::Test
4+
include Capybara::Screenshot::MiniTestPlugin
5+
include ApiUmbrellaTestHelpers::Setup
6+
include ApiUmbrellaTestHelpers::AdminAuth
7+
include ApiUmbrellaTestHelpers::AdminUiLogin
8+
include Minitest::Hooks
9+
10+
def setup
11+
super
12+
setup_server
13+
once_per_class_setup do
14+
override_config_set({
15+
# While the contact URL should be trusted, since it's configured by an
16+
# admin, still test with special character to ensure it's escaped
17+
# properly.
18+
"contact_url" => "https://example.com/contact/?q='\"><script>alert('hello')</script>",
19+
"web" => {
20+
"admin" => {
21+
"auth_strategies" => {
22+
"enabled" => [
23+
"google",
24+
"max.gov",
25+
],
26+
"max.gov" => {
27+
"require_mfa" => true,
28+
},
29+
},
30+
},
31+
},
32+
}, ["--router", "--web"])
33+
end
34+
end
35+
36+
def after_all
37+
super
38+
override_config_reset(["--router", "--web"])
39+
end
40+
41+
def test_unverified_html_message
42+
omniauth_data = {
43+
"provider" => "google_oauth2",
44+
"info" => {
45+
"email" => "[email protected]",
46+
},
47+
"extra" => {
48+
"raw_info" => {
49+
"email_verified" => false,
50+
},
51+
},
52+
}
53+
54+
mock_omniauth(omniauth_data) do
55+
assert_login_forbidden("Sign in with Google", "not verified")
56+
assert_match("The email address '[email protected]' is not verified. Please <a href=\"https://example.com/contact/?q='&quot;&gt;&lt;script&gt;alert('hello')&lt;/script&gt;\">contact us</a> for further assistance.", page.body)
57+
end
58+
end
59+
60+
def test_unverified_html_message_with_xss_email
61+
omniauth_data = {
62+
"provider" => "google_oauth2",
63+
"info" => {
64+
"email" => "'\"><script>alert('hello')</script>",
65+
},
66+
"extra" => {
67+
"raw_info" => {
68+
"email_verified" => false,
69+
},
70+
},
71+
}
72+
73+
mock_omniauth(omniauth_data) do
74+
assert_login_forbidden("Sign in with Google", "not verified")
75+
assert_match("The email address ''\"&gt;&lt;script&gt;alert('hello')&lt;/script&gt;' is not verified. Please <a href=\"https://example.com/contact/?q='&quot;&gt;&lt;script&gt;alert('hello')&lt;/script&gt;\">contact us</a> for further assistance.", page.body)
76+
end
77+
end
78+
79+
def test_nonexistent_html_message
80+
omniauth_data = {
81+
"provider" => "google_oauth2",
82+
"info" => {
83+
"email" => "[email protected]",
84+
},
85+
"extra" => {
86+
"raw_info" => {
87+
"email_verified" => true,
88+
},
89+
},
90+
}
91+
92+
mock_omniauth(omniauth_data) do
93+
assert_login_forbidden("Sign in with Google", "not authorized")
94+
assert_match("The account for '[email protected]' is not authorized to access the admin. Please <a href=\"https://example.com/contact/?q='&quot;&gt;&lt;script&gt;alert('hello')&lt;/script&gt;\">contact us</a> for further assistance.", page.body)
95+
end
96+
end
97+
98+
def test_nonexistent_html_message_with_xss_email
99+
omniauth_data = {
100+
"provider" => "google_oauth2",
101+
"info" => {
102+
"email" => "'\"><script>alert('hello')</script>",
103+
},
104+
"extra" => {
105+
"raw_info" => {
106+
"email_verified" => true,
107+
},
108+
},
109+
}
110+
111+
mock_omniauth(omniauth_data) do
112+
assert_login_forbidden("Sign in with Google", "not authorized")
113+
assert_match("The account for ''\"&gt;&lt;script&gt;alert('hello')&lt;/script&gt;' is not authorized to access the admin. Please <a href=\"https://example.com/contact/?q='&quot;&gt;&lt;script&gt;alert('hello')&lt;/script&gt;\">contact us</a> for further assistance.", page.body)
114+
end
115+
end
116+
117+
def test_mfa_required_html_message
118+
omniauth_data = {
119+
"provider" => "cas",
120+
"info" => {
121+
"email" => "[email protected]",
122+
},
123+
}
124+
125+
mock_omniauth(omniauth_data) do
126+
assert_login_forbidden("Sign in with MAX.gov", "must use multi-factor")
127+
assert_match("You must use multi-factor authentication to sign in. Please try again, or <a href=\"https://example.com/contact/?q='&quot;&gt;&lt;script&gt;alert('hello')&lt;/script&gt;\">contact us</a> for further assistance.", page.body)
128+
end
129+
end
130+
131+
def test_error_message_from_external_provider
132+
visit "/admins/auth/google_oauth2/callback?error='\"><script>confirm(document.domain)</script>"
133+
assert_match("Could not authenticate you from GoogleOauth2 because \"'\"&gt;&lt;script&gt;confirm(document.domain)&lt;/script&gt;\".", page.body)
134+
end
135+
end

0 commit comments

Comments
 (0)