From 4bfa14c5be60a4925f5ee269d2da36747fc5bc88 Mon Sep 17 00:00:00 2001 From: docJerem Date: Tue, 19 May 2026 13:38:38 +0200 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=90=9B=20Fix=20/=20IdP-initiated=20fl?= =?UTF-8?q?ow=20never=20completes=20=E2=80=94=20validate=5Fauthresp=20retu?= =?UTF-8?q?rns=20bare=20:ok?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The IdP-initiated success branch of `validate_authresp/4` returned the bare atom `:ok`, but the caller pattern-matched `{:ok, nonce}` inside `with`. The mismatch fell through to the `_ -> {:error, :access_denied}` catch-all, making the entire IdP-initiated success path effectively dead code: any IdP posting a Response with an empty `InResponseTo` was rejected with `access_denied`, even when `allow_idp_initiated_flow: true`. Changes: * `validate_authresp/4` now returns `{:ok, flow, nonce}` on success, where `flow` is `:idp_initiated` or `:sp_initiated` and `nonce` is `nil` for IdP-initiated (no AuthnRequest exists to bind a nonce to). * `consume_signin_response/2` propagates a new `flow:` field in its success map, so consumers do not have to deduce the flow type from `nonce == nil` or `assertion.subject.in_response_to == ""`. * Error atom `:idp_first_flow_not_allowed` renamed to `:idp_initiated_not_allowed` for alignment with standard SAML terminology used elsewhere in the module. * Added regression tests covering all four `validate_authresp/4` paths (previously uncovered). Closes #24 --- lib/ex_saml/sp_handler.ex | 38 ++++++--- test/ex_saml/sp_handler_test.exs | 138 ++++++++++++++++++++++++++++++- 2 files changed, 161 insertions(+), 15 deletions(-) diff --git a/lib/ex_saml/sp_handler.ex b/lib/ex_saml/sp_handler.ex index f6064b5..f3dd2e9 100644 --- a/lib/ex_saml/sp_handler.ex +++ b/lib/ex_saml/sp_handler.ex @@ -57,8 +57,21 @@ defmodule ExSaml.SPHandler do @doc """ Processes the IdP sign-in response and extracts the SAML assertion. - Returns `{:ok, %{assertion: assertion, nonce: nonce, user_token: token, redirect_uri: uri}}` - on success, or `{:error, reason}` on failure. + On success returns + `{:ok, %{flow: flow, assertion: assertion, nonce: nonce, user_token: token, redirect_uri: uri}}` + where: + + * `flow` is `:idp_initiated` or `:sp_initiated` and reflects which SAML flow + produced the response (deduced from the assertion's `SubjectConfirmationData` + `InResponseTo` — empty means IdP-initiated). + * `nonce` is the AuthnRequest-bound SAML nonce for SP-initiated flows, and + `nil` for IdP-initiated flows (no AuthnRequest exists in that case, so no + nonce is generated; downstream consumers must accept `nil` for the + IdP-initiated case). + + On failure returns `{:error, reason}`. Possible reasons include + `:idp_initiated_not_allowed`, `:invalid_target_url`, `:invalid_relay_state`, + `:invalid_idp_id`, and `:access_denied`. """ # Router-facing clause: matches when the SP router dispatched here with # `idp_id` in path params. Performs the full SAML flow AND handles the @@ -112,9 +125,10 @@ defmodule ExSaml.SPHandler do redirect_uri = RelayStateCache.get(relay_state)[:redirect_uri] with {:ok, assertion} <- Helper.decode_idp_auth_resp(sp, saml_encoding, saml_response), - {:ok, nonce} <- validate_authresp(conn, idp_data, assertion, relay_state) do + {:ok, flow, nonce} <- validate_authresp(conn, idp_data, assertion, relay_state) do {:ok, %{ + flow: flow, assertion: %Assertion{assertion | idp_id: idp_id}, nonce: nonce, user_token: user_token, @@ -169,22 +183,24 @@ defmodule ExSaml.SPHandler do get_session(conn, "target_url") || RelayStateCache.get(relay_state)[:target_url] || "/" end - # IDP-initiated flow auth response - defp validate_authresp(_conn, idp_data, %{subject: %{in_response_to: ""}}, relay_state) do + # IdP-initiated flow auth response. Tagged as `:idp_initiated` with a `nil` + # nonce since there is no AuthnRequest to bind a nonce to in this flow. + @doc false + def validate_authresp(_conn, idp_data, %{subject: %{in_response_to: ""}}, relay_state) do cond do !idp_data.allow_idp_initiated_flow -> - {:error, :idp_first_flow_not_allowed} + {:error, :idp_initiated_not_allowed} idp_data.allowed_target_urls && relay_state not in idp_data.allowed_target_urls -> {:error, :invalid_target_url} true -> - :ok + {:ok, :idp_initiated, nil} end end - # SP-initiated flow auth response - defp validate_authresp(conn, %IdpData{id: idp_id}, _assertion, relay_state) do + # SP-initiated flow auth response. + def validate_authresp(conn, %IdpData{id: idp_id}, _assertion, relay_state) do rs_in_session = get_session(conn, "relay_state") || RelayStateCache.get(relay_state)[:relay_state] @@ -193,8 +209,6 @@ defmodule ExSaml.SPHandler do saml_nonce_in_session = get_session(conn, "saml_nonce") || RelayStateCache.get(relay_state)[:saml_nonce] - # RelayStateCache.delete(relay_state) - cond do rs_in_session == nil || rs_in_session != relay_state -> {:error, :invalid_relay_state} @@ -203,7 +217,7 @@ defmodule ExSaml.SPHandler do {:error, :invalid_idp_id} true -> - {:ok, saml_nonce_in_session} + {:ok, :sp_initiated, saml_nonce_in_session} end end diff --git a/test/ex_saml/sp_handler_test.exs b/test/ex_saml/sp_handler_test.exs index 887aad6..46f1c2f 100644 --- a/test/ex_saml/sp_handler_test.exs +++ b/test/ex_saml/sp_handler_test.exs @@ -1,13 +1,14 @@ defmodule ExSaml.SPHandlerTest do use ExUnit.Case, async: false - import Plug.Test + import Plug.Conn + import Plug.Test - alias ExSaml.SPHandler + alias ExSaml.{IdpData, SPHandler, Subject} # Minimal in-process stub for the cache configured via # `config :ex_saml, cache: …`. The real cache is a Nebulex backend, but for - # unit-testing `target_url/2` we only need `get/1`. The returned value is + # unit-testing the `SPHandler` we only need `get/1`. The returned value is # read from the process dictionary so each test can drive its own scenario. defmodule StubRelayCache do def get(_key), do: Process.get(:stub_relay_cache_value) @@ -55,6 +56,39 @@ defmodule ExSaml.SPHandlerTest do {:ok, conn: conn} end + # Builds a fresh conn with the given session payload populated. Used by tests + # that need to drive `validate_authresp/4`'s SP-initiated branch from + # session-stored values (relay_state, idp_id, saml_nonce). Independent of the + # default `:conn` provided by `setup` because each test needs its own session. + defp build_conn(session) do + secret = String.duplicate("a", 64) + + opts = + Plug.Session.init( + store: :cookie, + key: "_test_session", + signing_salt: "salt", + encryption_salt: "esalt" + ) + + conn = + :get + |> conn("/") + |> init_test_session(session) + |> Map.put(:secret_key_base, secret) + |> Plug.Session.call(opts) + |> fetch_session() + + Enum.reduce(session, conn, fn {k, v}, acc -> put_session(acc, k, v) end) + end + + defp idp_initiated_assertion, do: %{subject: %Subject{in_response_to: ""}} + defp sp_initiated_assertion, do: %{subject: %Subject{in_response_to: "request-id-42"}} + + # --------------------------------------------------------------------------- + # target_url/2 + # --------------------------------------------------------------------------- + describe "target_url/2" do test "returns the session target_url when set", %{conn: conn} do Process.put(:stub_relay_cache_value, %{target_url: "/from-cache"}) @@ -86,4 +120,102 @@ defmodule ExSaml.SPHandlerTest do assert SPHandler.target_url(conn, "rls") == "/sign-in" end end + + # --------------------------------------------------------------------------- + # validate_authresp/4 — IdP-initiated + # --------------------------------------------------------------------------- + + describe "validate_authresp/4 — IdP-initiated" do + test "returns {:ok, :idp_initiated, nil} on success — regression test for #24" do + idp = %IdpData{allow_idp_initiated_flow: true, allowed_target_urls: nil} + + assert {:ok, :idp_initiated, nil} = + SPHandler.validate_authresp(build_conn(%{}), idp, idp_initiated_assertion(), "") + end + + test "succeeds when relay_state is one of allowed_target_urls" do + idp = %IdpData{ + allow_idp_initiated_flow: true, + allowed_target_urls: ["https://app.example.com/dashboard"] + } + + assert {:ok, :idp_initiated, nil} = + SPHandler.validate_authresp( + build_conn(%{}), + idp, + idp_initiated_assertion(), + "https://app.example.com/dashboard" + ) + end + + test "returns :idp_initiated_not_allowed when allow_idp_initiated_flow is false" do + idp = %IdpData{allow_idp_initiated_flow: false} + + assert {:error, :idp_initiated_not_allowed} = + SPHandler.validate_authresp(build_conn(%{}), idp, idp_initiated_assertion(), "") + end + + test "returns :invalid_target_url when relay_state not in allowed_target_urls" do + idp = %IdpData{ + allow_idp_initiated_flow: true, + allowed_target_urls: ["https://app.example.com/dashboard"] + } + + assert {:error, :invalid_target_url} = + SPHandler.validate_authresp( + build_conn(%{}), + idp, + idp_initiated_assertion(), + "https://elsewhere.example.com/" + ) + end + end + + # --------------------------------------------------------------------------- + # validate_authresp/4 — SP-initiated + # --------------------------------------------------------------------------- + + describe "validate_authresp/4 — SP-initiated" do + test "returns {:ok, :sp_initiated, nonce} on success" do + idp = %IdpData{id: "idp-1"} + + conn = + build_conn(%{ + "relay_state" => "rs-abc", + "idp_id" => "idp-1", + "saml_nonce" => "nonce-xyz" + }) + + assert {:ok, :sp_initiated, "nonce-xyz"} = + SPHandler.validate_authresp(conn, idp, sp_initiated_assertion(), "rs-abc") + end + + test "returns :invalid_relay_state when session relay_state does not match" do + idp = %IdpData{id: "idp-1"} + + conn = + build_conn(%{ + "relay_state" => "rs-different", + "idp_id" => "idp-1", + "saml_nonce" => "nonce-xyz" + }) + + assert {:error, :invalid_relay_state} = + SPHandler.validate_authresp(conn, idp, sp_initiated_assertion(), "rs-abc") + end + + test "returns :invalid_idp_id when session idp_id does not match" do + idp = %IdpData{id: "idp-1"} + + conn = + build_conn(%{ + "relay_state" => "rs-abc", + "idp_id" => "idp-other", + "saml_nonce" => "nonce-xyz" + }) + + assert {:error, :invalid_idp_id} = + SPHandler.validate_authresp(conn, idp, sp_initiated_assertion(), "rs-abc") + end + end end From b0245afecae0e4a3cf0b03156547b3b45b8646c2 Mon Sep 17 00:00:00 2001 From: docJerem Date: Tue, 19 May 2026 14:15:17 +0200 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=94=A7=20CI=20/=20Realign=20dialyzer?= =?UTF-8?q?=20ignores=20after=20sp=5Fhandler=20line=20shift?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tightened return type of `validate_authresp/4` (now strictly `{:ok, flow, nonce} | {:error, atom}`) makes the defensive catch-all in the library-facing clause of `consume_signin_response/2` provably unreachable too — same fail-closed rationale as the router-facing clause. Updates: * Document the library-facing catch-all with the same inline rationale as the router-facing one * `.dialyzer_ignore.exs`: shift the existing ignore from `:95` → `:108` (line moved due to the expanded `@doc` block) and add `:144` for the new defensive catch-all --- .dialyzer_ignore.exs | 10 ++++++---- lib/ex_saml/sp_handler.ex | 5 +++++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs index fac1bc3..4b0280e 100644 --- a/.dialyzer_ignore.exs +++ b/.dialyzer_ignore.exs @@ -1,7 +1,9 @@ [ - # Defensive catch-all in `consume_signin_response/1`'s `with` else block. - # Provably unreachable today, kept intentionally so the auth endpoint fails - # closed (HTTP 403) if a future change introduces a new return shape. + # Defensive catch-alls in `consume_signin_response/1` and + # `consume_signin_response/2` `with` else blocks. Provably unreachable today, + # kept intentionally so the auth endpoint fails closed (HTTP 403 / error + # tuple) if a future change introduces a new return shape. # See lib/ex_saml/sp_handler.ex for the inline rationale. - ~r/lib\/ex_saml\/sp_handler\.ex:95:.*pattern_match_cov/ + ~r/lib\/ex_saml\/sp_handler\.ex:108:.*pattern_match_cov/, + ~r/lib\/ex_saml\/sp_handler\.ex:144:.*pattern_match_cov/ ] diff --git a/lib/ex_saml/sp_handler.ex b/lib/ex_saml/sp_handler.ex index f3dd2e9..d4831c6 100644 --- a/lib/ex_saml/sp_handler.ex +++ b/lib/ex_saml/sp_handler.ex @@ -136,6 +136,11 @@ defmodule ExSaml.SPHandler do }} else {:error, error} -> {:error, error} + # Defensive fallback: unreachable today (Dialyzer flags it as such), but + # kept so that any future change introducing a new return shape from the + # `with` chain — e.g. a new validator step that forgets the + # `{:ok, _, _}` shape — fails closed with `{:error, :access_denied}` + # rather than crashing the caller with a `WithClauseError`. _ -> {:error, :access_denied} end end From 41bfb06342eb2ae51fee6d77df5fc6f590f60a6f Mon Sep 17 00:00:00 2001 From: docJerem Date: Tue, 19 May 2026 14:28:08 +0200 Subject: [PATCH 3/3] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Refacto=20/=20Drop=20u?= =?UTF-8?q?njustified=20library-facing=20catch-all=20in=20consume=5Fsignin?= =?UTF-8?q?=5Fresponse/2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `_ -> {:error, :access_denied}` catch-all in the library-facing `consume_signin_response/2` had no real fail-closed justification (unlike its router-facing sibling which returns an HTTP 403 to safely close out an auth endpoint). Here it just hid a `WithClauseError` behind a generic error tuple, swallowing the diagnostic signal Dialyzer was emitting. Removing it means a future change introducing an unexpected return shape from the `with` chain will surface as a `WithClauseError` with the actual mismatched value in the stack trace — strictly more informative than masking it as `:access_denied`. The router-facing 403 fallback is kept intact (HTTP endpoints must fail closed). Also drops the matching `.dialyzer_ignore.exs` entry for line 144; the ignore for line 108 is preserved. --- .dialyzer_ignore.exs | 10 ++++------ lib/ex_saml/sp_handler.ex | 6 ------ 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs index 4b0280e..d85540b 100644 --- a/.dialyzer_ignore.exs +++ b/.dialyzer_ignore.exs @@ -1,9 +1,7 @@ [ - # Defensive catch-alls in `consume_signin_response/1` and - # `consume_signin_response/2` `with` else blocks. Provably unreachable today, - # kept intentionally so the auth endpoint fails closed (HTTP 403 / error - # tuple) if a future change introduces a new return shape. + # Defensive catch-all in `consume_signin_response/1`'s `with` else block. + # Provably unreachable today, kept intentionally so the auth endpoint fails + # closed (HTTP 403) if a future change introduces a new return shape. # See lib/ex_saml/sp_handler.ex for the inline rationale. - ~r/lib\/ex_saml\/sp_handler\.ex:108:.*pattern_match_cov/, - ~r/lib\/ex_saml\/sp_handler\.ex:144:.*pattern_match_cov/ + ~r/lib\/ex_saml\/sp_handler\.ex:108:.*pattern_match_cov/ ] diff --git a/lib/ex_saml/sp_handler.ex b/lib/ex_saml/sp_handler.ex index d4831c6..be223b5 100644 --- a/lib/ex_saml/sp_handler.ex +++ b/lib/ex_saml/sp_handler.ex @@ -136,12 +136,6 @@ defmodule ExSaml.SPHandler do }} else {:error, error} -> {:error, error} - # Defensive fallback: unreachable today (Dialyzer flags it as such), but - # kept so that any future change introducing a new return shape from the - # `with` chain — e.g. a new validator step that forgets the - # `{:ok, _, _}` shape — fails closed with `{:error, :access_denied}` - # rather than crashing the caller with a `WithClauseError`. - _ -> {:error, :access_denied} end end