From 29b9da4a5ac2ff3d2b62f8ca50de94880c2c792b Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 12 Nov 2025 13:27:00 +0100 Subject: [PATCH 1/3] Add configurable SLO RelayState validator * Provide a default validator that accepts only relative paths (starting with /) and rejects absolute URLs, protocol-relative, invalid URIs, and other schemes. * Add :slo_relay_state_validator option to override or change acceptance semantics. * Update documentation. --- README.md | 11 ++ lib/omniauth/strategies/saml.rb | 60 +++++++-- spec/omniauth/strategies/saml_spec.rb | 178 ++++++++++++++++++++++++-- 3 files changed, 227 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 7520049..55bc09d 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,17 @@ Note that when [integrating with Devise](#devise-integration), the URL path will * `:slo_enabled` - Enables or disables Single Logout (SLO). Set to `false` to disable SLO. Defaults to `true`. Optional. +* `: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 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 + 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 mapping. So for example, a param `original_request_param` with value `original_param_value`, diff --git a/lib/omniauth/strategies/saml.rb b/lib/omniauth/strategies/saml.rb index ceeb292..dfee217 100644 --- a/lib/omniauth/strategies/saml.rb +++ b/lib/omniauth/strategies/saml.rb @@ -1,5 +1,6 @@ require 'omniauth' require 'ruby-saml' +require 'uri' module OmniAuth module Strategies @@ -27,8 +28,26 @@ def self.inherited(subclass) first_name: ["first_name", "firstname", "firstName"], last_name: ["last_name", "lastname", "lastName"] } + 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 + + return false unless uri.relative? + + path = uri.path + path && path.start_with?("/") + end + option :slo_default_relay_state option :slo_enabled, true + option :slo_relay_state_validator, DEFAULT_SLO_RELAY_STATE_VALIDATOR option :uid_attribute option :idp_slo_session_destroy, proc { |_env, session| session.clear } @@ -148,19 +167,36 @@ def handle_response(raw_response, opts, settings) def slo_relay_state if request.params.has_key?("RelayState") && request.params["RelayState"] != "" - request.params["RelayState"] - else - slo_default_relay_state = options.slo_default_relay_state - if slo_default_relay_state.respond_to?(:call) - if slo_default_relay_state.arity == 1 - slo_default_relay_state.call(request) - else - slo_default_relay_state.call - end - else - slo_default_relay_state - end + relay_state = request.params["RelayState"] + + return relay_state if valid_slo_relay_state?(relay_state) end + + default_slo_relay_state + end + + def valid_slo_relay_state?(relay_state) + validator = options.slo_relay_state_validator + + 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) + arity = validator.arity + + return validator.call if validator.arity.zero? + return validator.call(relay_state) if validator.arity == 1 + validator.call(relay_state, request) + end + + def default_slo_relay_state + slo_default_relay_state = options.slo_default_relay_state + + return slo_default_relay_state unless slo_default_relay_state.respond_to?(:call) + return slo_default_relay_state.call if slo_default_relay_state.arity.zero? + slo_default_relay_state.call(request) end def handle_logout_response(raw_response, settings) diff --git a/spec/omniauth/strategies/saml_spec.rb b/spec/omniauth/strategies/saml_spec.rb index a488922..51cd090 100644 --- a/spec/omniauth/strategies/saml_spec.rb +++ b/spec/omniauth/strategies/saml_spec.rb @@ -30,6 +30,55 @@ end let(:strategy) { [OmniAuth::Strategies::SAML, saml_options] } + shared_examples 'validating RelayState param' do + context 'when slo_relay_state_validator is not defined and default' do + [ + ['/signed-out', '//attacker.test', '%2Fsigned-out'], + ['/signed-out', 'javascript:alert(1)', '%2Fsigned-out'], + ['/signed-out', 'https://example.com/logout', '%2Fsigned-out'], + ['/signed-out', 'https://example.com/logout?param=1&two=two', '%2Fsigned-out'], + ['/signed-out', '/', '%2F'], + ['', '//attacker.test', ''], + ['', '/team/logout', '%2Fteam%2Flogout'], + ].each do |slo_default_relay_state, relay_state_param, expected_relay_state| + context "when slo_default_relay_state: #{slo_default_relay_state.inspect}, relay_state_param: #{relay_state_param.inspect}" do + let(:saml_options) { super().merge(slo_default_relay_state: slo_default_relay_state) } + let(:params) { super().merge('RelayState' => relay_state_param) } + + it { is_expected.to be_redirect.and have_attributes(location: a_string_including("RelayState=#{expected_relay_state}")) } + end + end + end + + context 'when slo_relay_state_validator is overridden' do + [ + ['/signed-out', proc { |state| state.start_with?('https://trusted.example.com') }, 'https://trusted.example.com/logout', 'https%3A%2F%2Ftrusted.example.com%2Flogout'], + ['/signed-out', proc { |state| state.start_with?('https://trusted.example.com') }, 'https://attacker.test/logout', '%2Fsigned-out'], + ['/signed-out', proc { |state| state.start_with?('https://trusted.example.com') }, '/safe/path', '%2Fsigned-out'], + ['/signed-out', proc { |state, req| state == req.params['RelayState'] }, '/team/logout', '%2Fteam%2Flogout'], + ['/signed-out', nil, '//attacker.test', '%2Fsigned-out'], + ['/signed-out', false, '//attacker.test', '%2Fsigned-out'], + ['/signed-out', proc { |_| false }, '//attacker.test', '%2Fsigned-out'], + ['/signed-out', proc { |_| true }, 'javascript:alert(1)', 'javascript%3Aalert%281%29'], + [nil, true, 'https://example.com/logout', 'https%3A%2F%2Fexample.com%2Flogout'], + [nil, true, 'javascript:alert(1)', 'javascript%3Aalert%281%29'], + [nil, true, '/', '%2F'], + ].each do |slo_default_relay_state, slo_relay_state_validator, relay_state_param, expected_relay_state| + context "when slo_default_relay_state: #{slo_default_relay_state.inspect}, slo_relay_state_validator: #{slo_relay_state_validator.inspect}, relay_state_param: #{relay_state_param.inspect}" do + let(:saml_options) do + super().merge( + slo_default_relay_state: slo_default_relay_state, + slo_relay_state_validator: slo_relay_state_validator, + ) + end + let(:params) { super().merge('RelayState' => relay_state_param) } + + it { is_expected.to be_redirect.and have_attributes(location: a_string_including("RelayState=#{expected_relay_state}")) } + end + end + end + end + describe 'POST /auth/saml' do context 'without idp runtime params present' do before do @@ -274,37 +323,128 @@ end context "when response is a logout response" do - before :each do - post "/auth/saml/slo", { - SAMLResponse: load_xml(:example_logout_response), - RelayState: "https://example.com/", - }, "rack.session" => {"saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9"} + let(:opts) do + { "rack.session" => { "saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9" } } end - it "should redirect to relaystate" do - expect(last_response).to be_redirect - expect(last_response.location).to match /https:\/\/example.com\// + let(:params) { { SAMLResponse: load_xml(:example_logout_response) } } + + subject(:post_slo_response) { post "/auth/saml/slo", params, opts } + + context "when relay state is relative" do + let(:params) {super().merge(RelayState: "/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(:params) {super().merge(RelayState: "https://example.com/")} + + it "redirects without a location header" do + post_slo_response + + expect(last_response).to be_redirect + expect(last_response.headers.fetch("Location")).to be_nil + 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 valid" do + let(:params) {super().merge(RelayState: "/safe/logout")} + + it {is_expected.to be_redirect.and have_attributes(location: '/safe/logout') } + end + + context "when response relay state is invalid" do + let(:params) {super().merge(RelayState: "javascript:alert(1)")} + + it {is_expected.to be_redirect.and have_attributes(location: '/signed-out') } + 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 valid" do + let(:params) {super().merge(RelayState: "/safe/logout")} + + it {is_expected.to be_redirect.and have_attributes(location: '/safe/logout') } + end + + context "when response relay state is invalid" do + let(:params) {super().merge(RelayState: "javascript:alert(1)")} + + it {is_expected.to be_redirect.and have_attributes(location: nil) } + end end end context "when request is a logout request" do subject { post "/auth/saml/slo", params, "rack.session" => { "saml_uid" => "username@example.com" } } + 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 + + it_behaves_like 'validating RelayState param' + + context 'when slo_default_relay_state is blank' do + let(:saml_options) { super().merge(slo_default_relay_state: nil) } + + context "when request relay state is invalid" do + let(:params) do + { + "SAMLRequest" => load_xml(:example_logout_request), + "RelayState" => "javascript:alert(1)", + } + end + + it "redirects without including a RelayState parameter" do + subject + + expect(last_response).to be_redirect + expect(last_response.location).to match %r{https://idp\.sso\.example\.com/signoff/29490} + expect(last_response.location).not_to match(/RelayState=/) + end + end + end + + context "with a custom relay state validator" do + let(:saml_options) do + super().merge( + slo_relay_state_validator: proc do |relay_state, rack_request| + expect(rack_request).to respond_to(:params) + relay_state == "custom-state" + end, + ) end + let(:params) { super().merge("RelayState" => "custom-state") } + + it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=custom-state/)) } end context "when request is an invalid logout request" do @@ -345,6 +485,9 @@ end describe 'POST /auth/saml/spslo' do + let(:params) { {} } + subject { post "/auth/saml/spslo", params } + def test_default_relay_state(static_default_relay_state = nil, &block_default_relay_state) saml_options["slo_default_relay_state"] = static_default_relay_state || block_default_relay_state post "/auth/saml/spslo" @@ -370,6 +513,21 @@ def test_default_relay_state(static_default_relay_state = nil, &block_default_re end end + it_behaves_like 'validating RelayState param' + + context 'when slo_default_relay_state is blank' do + let(:saml_options) { super().merge(slo_default_relay_state: nil) } + let(:params) { { RelayState: "//example.com" } } + + it "redirects without including a RelayState parameter" do + subject + + expect(last_response).to be_redirect + expect(last_response.location).to match %r{https://idp\.sso\.example\.com/signoff/29490} + expect(last_response.location).not_to match(/RelayState=/) + end + end + it "should give not implemented without an idp_slo_service_url" do saml_options.delete(:idp_slo_service_url) post "/auth/saml/spslo" From be0f3be2ae9f38fa311b15ca77f75da6e43f876a Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Wed, 12 Nov 2025 13:27:45 +0100 Subject: [PATCH 2/3] refactor: Apply suggestion from @copilot - https://github.com/omniauth/omniauth-saml/pull/245#discussion_r2514433946 - https://github.com/omniauth/omniauth-saml/pull/245#discussion_r2518160229 --- lib/omniauth/strategies/saml.rb | 2 -- spec/omniauth/strategies/saml_spec.rb | 20 ++++++++++---------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/lib/omniauth/strategies/saml.rb b/lib/omniauth/strategies/saml.rb index dfee217..41c7c44 100644 --- a/lib/omniauth/strategies/saml.rb +++ b/lib/omniauth/strategies/saml.rb @@ -184,8 +184,6 @@ def valid_slo_relay_state?(relay_state) end def call_slo_relay_state_validator(validator, relay_state) - arity = validator.arity - return validator.call if validator.arity.zero? return validator.call(relay_state) if validator.arity == 1 validator.call(relay_state, request) diff --git a/spec/omniauth/strategies/saml_spec.rb b/spec/omniauth/strategies/saml_spec.rb index 51cd090..756077b 100644 --- a/spec/omniauth/strategies/saml_spec.rb +++ b/spec/omniauth/strategies/saml_spec.rb @@ -332,7 +332,7 @@ subject(:post_slo_response) { post "/auth/saml/slo", params, opts } context "when relay state is relative" do - let(:params) {super().merge(RelayState: "/signed-out")} + let(:params) { super().merge(RelayState: "/signed-out") } it "redirects to the relaystate" do post_slo_response @@ -343,7 +343,7 @@ end context "when relay state is an absolute https URL" do - let(:params) {super().merge(RelayState: "https://example.com/")} + let(:params) { super().merge(RelayState: "https://example.com/") } it "redirects without a location header" do post_slo_response @@ -357,15 +357,15 @@ let(:saml_options) { super().merge(slo_default_relay_state: '/signed-out') } context "when response relay state is valid" do - let(:params) {super().merge(RelayState: "/safe/logout")} + let(:params) { super().merge(RelayState: "/safe/logout") } - it {is_expected.to be_redirect.and have_attributes(location: '/safe/logout') } + it { is_expected.to be_redirect.and have_attributes(location: '/safe/logout') } end context "when response relay state is invalid" do - let(:params) {super().merge(RelayState: "javascript:alert(1)")} + let(:params) { super().merge(RelayState: "javascript:alert(1)") } - it {is_expected.to be_redirect.and have_attributes(location: '/signed-out') } + it { is_expected.to be_redirect.and have_attributes(location: '/signed-out') } end end @@ -373,15 +373,15 @@ let(:saml_options) { super().merge(slo_default_relay_state: nil) } context "when response relay state is valid" do - let(:params) {super().merge(RelayState: "/safe/logout")} + let(:params) { super().merge(RelayState: "/safe/logout") } - it {is_expected.to be_redirect.and have_attributes(location: '/safe/logout') } + it { is_expected.to be_redirect.and have_attributes(location: '/safe/logout') } end context "when response relay state is invalid" do - let(:params) {super().merge(RelayState: "javascript:alert(1)")} + let(:params) { super().merge(RelayState: "javascript:alert(1)") } - it {is_expected.to be_redirect.and have_attributes(location: nil) } + it { is_expected.to be_redirect.and have_attributes(location: nil) } end end end From c01f6a4d3cb0e3103768bd5a0d75e87dd941befd Mon Sep 17 00:00:00 2001 From: Gerardo Navarro Date: Thu, 13 Nov 2025 11:30:55 +0100 Subject: [PATCH 3/3] Update README.md Simplified documentation for the `slo_relay_state_validator`. --- README.md | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 55bc09d..fb25b09 100644 --- a/README.md +++ b/README.md @@ -101,20 +101,18 @@ Note that when [integrating with Devise](#devise-integration), the URL path will * `:slo_default_relay_state` - The value to use as default `RelayState` for single log outs. The value can be a string, or a `Proc` (or other object responding to `call`). The `request` instance will be passed to this callable if it has an arity of 1. If the value is a string, - the string will be returned, when the `RelayState` is called. Optional. + the string will be returned, when the `RelayState` is called. + The value is assumed to be safe and is not validated by `:slo_relay_state_validator`. + Optional. * `:slo_enabled` - Enables or disables Single Logout (SLO). Set to `false` to disable SLO. Defaults to `true`. Optional. -* `: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 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 - examples. +* `:slo_relay_state_validator` - A callable used to validate any `RelayState` before performing the redirect + in Single Logout flows. The callable receives the RelayState value and the current Rack request. + If unset, the default validator is used. The default validator allows only relative paths beginning + with `/` and rejects absolute URLs, invalid URIs, protocol-relative URLs, and other schemes. + If the given `RelayState` is considered invalid then the `slo_default_relay_state` value is used for the SLO redirect. + Optional. * `: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