Merge pull request #14 from gerardo-navarro/gerardo-navarro-restrict-default-to-relative-paths

Move protocol-relative RelayState guard before URI parsing

Author: gerardo-navarro

README.md

@@ -107,10 +107,10 @@ Note that when [integrating with Devise](#devise-integration), the URL path will
 
 * `:slo_relay_state_validator` - A callable used to validate any RelayState before OmniAuth uses it for
   redirects in Single Logout flows. The callable receives the RelayState value and, if it accepts a
-  second argument, the current Rack request. The default validator allows relative paths beginning
-  with `/` and absolute `http` or `https` URLs, and rejects invalid URIs, protocol-relative URLs, and
-  other schemes. Defaults generated via `:slo_default_relay_state` are assumed to be safe and skipped by
-  this validation step. Optional. When set to `true`, every RelayState value is accepted. When set to
+  second argument, the current Rack request. The default validator allows only relative paths beginning
+  with `/` and rejects absolute URLs, invalid URIs, protocol-relative URLs, and other schemes. Defaults
+  generated via `:slo_default_relay_state` are assumed to be safe and skipped by this validation step.
+  Optional. When set to `true`, every RelayState value is accepted. When set to
   `false` (or any other falsy value), every provided RelayState is rejected and the strategy falls back
   to the default RelayState. See the SLO relay state validator specs in
   [`spec/omniauth/strategies/saml_spec.rb`](spec/omniauth/strategies/saml_spec.rb) for additional

lib/omniauth/strategies/saml.rb

@@ -31,20 +31,18 @@ def self.inherited(subclass)
       DEFAULT_SLO_RELAY_STATE_VALIDATOR = lambda do |relay_state, _request|
         return true if relay_state.nil? || relay_state == ""
 
+        return false if relay_state.start_with?("//")
+
         begin
           uri = URI.parse(relay_state)
         rescue URI::Error
           return false
         end
 
-        if uri.scheme.nil?
-          return false if relay_state.start_with?("//")
+        return false unless uri.relative?
 
-          path = uri.path
-          path && path.start_with?("/")
-        else
-          %w[http https].include?(uri.scheme) && !uri.host.nil?
-        end
+        path = uri.path
+        path && path.start_with?("/")
       end
 
       option :slo_default_relay_state

spec/omniauth/strategies/saml_spec.rb

@@ -276,70 +276,64 @@ def post_xml(xml = :example_response, opts = {})
     end
 
     context "when response is a logout response" do
-      before :each do
-        post "/auth/saml/slo", {
+      let(:opts) do
+        { "rack.session" => { "saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9" } }
+      end
+
+      let(:params) do
+        {
           SAMLResponse: load_xml(:example_logout_response),
-          RelayState: "https://example.com/",
-        }, "rack.session" => {"saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9"}
+          RelayState: relay_state,
+        }
       end
 
-      it "should redirect to relaystate" do
-        expect(last_response).to be_redirect
-        expect(last_response.location).to match /https:\/\/example.com\//
+      subject(:post_slo_response) { post "/auth/saml/slo", params, opts }
+
+      context "when relay state is relative" do
+        let(:relay_state) { "/signed-out" }
+
+        it "redirects to the relaystate" do
+          post_slo_response
+
+          expect(last_response).to be_redirect
+          expect(last_response.location).to eq "/signed-out"
+        end
+      end
+
+      context "when relay state is an absolute https URL" do
+        let(:relay_state) { "https://example.com/" }
+
+        it "raises an invalid relay state error" do
+          expect { post_slo_response }.
+            to raise_error(OmniAuth::Strategies::SAML::ValidationError, "Invalid RelayState")
+        end
       end
 
       context 'when slo_default_relay_state is present' do
         let(:saml_options) { super().merge(slo_default_relay_state: '/signed-out') }
 
         context "when response relay state is invalid" do
-          let(:params) do
-            {
-              SAMLResponse: load_xml(:example_logout_response),
-              RelayState: "https://example.com/",
-            }
-          end
-
-          let(:opts) do
-            { "rack.session" => { "saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9" } }
-          end
-
-          subject(:post_slo_response) { post "/auth/saml/slo", params, opts }
+          let(:relay_state) { "https://example.com/" }
 
           [
             "//attacker.test",
             "javascript:alert(1)",
+            "https://example.com/logout",
           ].each do |unsafe_relay_state|
             context "#{unsafe_relay_state}" do
-              let(:params) { super().merge(RelayState: unsafe_relay_state) }
+              let(:relay_state) { unsafe_relay_state }
 
               it { is_expected.to be_redirect.and have_attributes(location: "/signed-out") }
             end
           end
-
-          context 'when absolute https relay state' do
-            let(:params) { super().merge(RelayState: "https://example.com/logout") }
-
-            it { is_expected.to be_redirect.and have_attributes(location: "https://example.com/logout") }
-          end
         end
       end
 
       context 'when slo_default_relay_state is blank' do
         let(:saml_options) { super().merge(slo_default_relay_state: nil) }
 
         context "when response relay state is invalid" do
-          let(:params) do
-            {
-              SAMLResponse: load_xml(:example_logout_response),
-              RelayState: "javascript:alert(1)",
-            }
-          end
-
-          let(:opts) do
-            { "rack.session" => { "saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9" } }
-          end
-
-          subject(:post_slo_response) { post "/auth/saml/slo", params, opts }
+          let(:relay_state) { "javascript:alert(1)" }
 
           it { expect { post_slo_response }.to raise_error(OmniAuth::Strategies::SAML::ValidationError, "Invalid RelayState") }
         end
@@ -349,27 +343,33 @@ def post_xml(xml = :example_response, opts = {})
     context "when request is a logout request" do
       subject { post "/auth/saml/slo", params, "rack.session" => { "saml_uid" => "[email protected]" } }
 
+      let(:relay_state) { "https://example.com/" }
+
       let(:params) do
         {
           "SAMLRequest" => load_xml(:example_logout_request),
-          "RelayState" => "https://example.com/",
+          "RelayState" => relay_state,
         }
       end
 
       context "when logout request is valid" do
+        let(:relay_state) { "/logout" }
+
         before { subject }
 
         it "should redirect to logout response" do
           expect(last_response).to be_redirect
           expect(last_response.location).to match /https:\/\/idp.sso.example.com\/signoff\/29490/
-          expect(last_response.location).to match /RelayState=https%3A%2F%2Fexample.com%2F/
+          expect(last_response.location).to match /RelayState=%2Flogout/
         end
       end
 
       context 'when slo_default_relay_state is present' do
         let(:saml_options) { super().merge(slo_default_relay_state: '/signed-out') }
 
         context "when request relay state is invalid" do
+          let(:relay_state) { "https://example.com/" }
+
           let(:params) do
             {
               "SAMLRequest" => load_xml(:example_logout_request),
@@ -380,6 +380,7 @@ def post_xml(xml = :example_response, opts = {})
           [
             "//attacker.test",
             "javascript:alert(1)",
+            "https://example.com/logout",
           ].each do |unsafe_relay_state|
             context "when the relay state is #{unsafe_relay_state}" do
               let(:relay_state) { unsafe_relay_state }
@@ -397,11 +398,6 @@ def post_xml(xml = :example_response, opts = {})
             it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
           end
 
-          context "when the relay state is an https URL" do
-            let(:relay_state) { "https://example.com/logout" }
-
-            it { is_expected.to have_attributes(location: a_string_matching(/RelayState=https%3A%2F%2Fexample.com%2Flogout/)) }
-          end
         end
 
         context "with validators that resolve to falsy" do
@@ -540,15 +536,15 @@ def test_default_relay_state(static_default_relay_state = nil, &block_default_re
       end
     end
 
-    context 'when slo_default_relay_state is present' do
-      let(:saml_options) { super().merge(slo_default_relay_state: '/signed-out') }
-      let(:params) { {} }
-      subject { post "/auth/saml/spslo", params }
+      context 'when slo_default_relay_state is present' do
+        let(:saml_options) { super().merge(slo_default_relay_state: '/signed-out') }
+        let(:params) { {} }
+        subject { post "/auth/saml/spslo", params }
 
       context 'with an https relay state' do
         let(:params) { { RelayState: "https://example.com/logout" } }
 
-        it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=https%3A%2F%2Fexample.com%2Flogout/)) }
+        it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
       end
 
       context 'with a protocol relative relay state' do