Merge pull request #13 from gerardo-navarro/gerardo-navarro-adjust-relay_state_validator-behavior

Clarify SLO relay state validator boolean handling

Author: gerardo-navarro

README.md

@@ -110,13 +110,11 @@ Note that when [integrating with Devise](#devise-integration), the URL path will
   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.
-
-  ```ruby
-  config.omniauth :saml, slo_relay_state_validator: lambda { |relay_state|
-    relay_state&.start_with?("/")
-  }
-  ```
+  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
+  examples.
 
 * `:idp_sso_service_url_runtime_params` - A dynamic mapping of request params that exist
   during the request phase of OmniAuth that should to be sent to the IdP after a specific

lib/omniauth/strategies/saml.rb

@@ -187,9 +187,10 @@ def slo_relay_state
 
       def valid_slo_relay_state?(relay_state)
         validator = options.slo_relay_state_validator
-        return true unless validator
 
-        !!call_slo_relay_state_validator(validator, relay_state)
+        return !!call_slo_relay_state_validator(validator, relay_state) if validator.respond_to?(:call)
+
+        !!validator
       end
 
       def call_slo_relay_state_validator(validator, relay_state)

spec/omniauth/strategies/saml_spec.rb

@@ -410,12 +410,18 @@ def post_xml(xml = :example_response, opts = {})
           context "when the validator option is nil" do
             let(:saml_options) { super().merge(slo_relay_state_validator: nil) }
 
-            it { is_expected.to have_attributes(location: a_string_matching(/RelayState=javascript%3Aalert%281%29/)) }
+            it { is_expected.to have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
           end
 
           context "when the validator option is false" do
             let(:saml_options) { super().merge(slo_relay_state_validator: false) }
 
+            it { is_expected.to have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
+          end
+
+          context "when the validator option is true" do
+            let(:saml_options) { super().merge(slo_relay_state_validator: true) }
+
             it { is_expected.to have_attributes(location: a_string_matching(/RelayState=javascript%3Aalert%281%29/)) }
           end
 
@@ -551,34 +557,40 @@ def test_default_relay_state(static_default_relay_state = nil, &block_default_re
         it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
       end
 
-      context 'with a javascript relay state' do
-        let(:params) { { RelayState: "javascript:alert(1)" } }
+        context 'with a javascript relay state' do
+          let(:params) { { RelayState: "javascript:alert(1)" } }
 
-        it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
+          it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
 
-        context 'when the validator would reject the default' do
-          let(:saml_options) do
-            super().merge(slo_relay_state_validator: proc { |value| value.start_with?("https://") })
+          context 'when the validator would reject the default' do
+            let(:saml_options) do
+              super().merge(slo_relay_state_validator: proc { |value| value.start_with?("https://") })
+            end
+
+            it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
           end
 
-          it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
-        end
+          context 'when the validator is nil' do
+            let(:saml_options) { super().merge(slo_relay_state_validator: nil) }
 
-        context 'when the validator is nil' do
-          let(:saml_options) { super().merge(slo_relay_state_validator: nil) }
+            it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
+          end
 
-          it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=javascript%3Aalert%281%29/)) }
-        end
+          context 'when the validator is false' do
+            let(:saml_options) { super().merge(slo_relay_state_validator: false) }
 
-        context 'when the validator is false' do
-          let(:saml_options) { super().merge(slo_relay_state_validator: false) }
+            it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }
+          end
 
-          it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=javascript%3Aalert%281%29/)) }
-        end
+          context 'when the validator is true' do
+            let(:saml_options) { super().merge(slo_relay_state_validator: true) }
 
-        context 'when the validator returns false' do
-          let(:saml_options) do
-            super().merge(slo_relay_state_validator: proc { |state| state == "/signed-out" })
+            it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=javascript%3Aalert%281%29/)) }
+          end
+
+          context 'when the validator returns false' do
+            let(:saml_options) do
+              super().merge(slo_relay_state_validator: proc { |state| state == "/signed-out" })
           end
 
           it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=%2Fsigned-out/)) }