Carry the SAML transaction in RelayState instead of the session

The IdP returns the assertion as a cross-site POST, and a SameSite=Lax cookie is
not sent on one - so the callback saw no session, found no saml_org_slug, and
failed every login with "SAML session mismatch". This could never have worked
against a real IdP; the specs missed it because Rack::Test carries the cookie
regardless of SameSite.

Saml::RequestStore keeps {org_slug, request_id} in Redis under a random token that
travels in RelayState, signed alongside the AuthnRequest. Claiming reads and
deletes in one operation, so a replayed assertion is rejected even inside the
InResponseTo validity window - stronger than the session binding it replaces.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: eld120

app/controllers/saml_controller.rb

@@ -20,25 +20,25 @@ def metadata
       content_type: "application/samlmetadata+xml"
   end
 
-  # SP-initiated login: redirect to the IdP with a signed AuthnRequest, remembering the
-  # request id (replay protection) and org slug (cross-tenant binding) for the callback.
+  # SP-initiated login: redirect to the IdP with a signed AuthnRequest, parking the request id
+  # (replay protection) and org slug (cross-tenant binding) in RelayState for the callback.
   def init
     settings = Saml::SettingsBuilder.build(configured_saml_configuration)
     auth_request = OneLogin::RubySaml::Authrequest.new
-    redirect_url = auth_request.create(settings)
-    session[:saml_request_id] = auth_request.request_id
-    session[:saml_org_slug] = params[:org_slug]
-    redirect_to redirect_url, allow_other_host: true
+    relay_state = Saml::RequestStore.create(request_id: auth_request.request_id,
+      org_slug: params[:org_slug])
+    redirect_to auth_request.create(settings, RelayState: relay_state), allow_other_host: true
   end
 
   # Assertion Consumer Service: validate the IdP's response and sign the user in.
   def callback
     saml_configuration = configured_saml_configuration
-    request_id = session.delete(:saml_request_id)
-    return saml_failure("SAML session mismatch") if session.delete(:saml_org_slug) != params[:org_slug]
+    saml_request = Saml::RequestStore.claim(params[:RelayState])
+    return saml_failure("this login has expired, please try again") if saml_request.blank?
+    return saml_failure("SAML session mismatch") if saml_request[:org_slug] != params[:org_slug]
 
     result = Saml::AssertionProcessor.call(saml_configuration:,
-      raw_response: params[:SAMLResponse], request_id:)
+      raw_response: params[:SAMLResponse], request_id: saml_request[:request_id])
     return saml_failure(result.error) unless result.success?
 
     sign_in_and_redirect(result.user)

app/services/saml/request_store.rb

@@ -0,0 +1,43 @@
+# Carries the in-flight SAML transaction from the AuthnRequest to the assertion.
+#
+# The session can't hold it: the IdP returns the assertion as a cross-site POST, and a
+# SameSite=Lax cookie isn't sent on one, so the callback sees no session at all. The token
+# travels in RelayState (signed alongside the AuthnRequest) and the state stays here.
+module Saml
+  module RequestStore
+    extend Functionable
+
+    # Long enough for a password prompt and an MFA challenge, short enough that an
+    # intercepted RelayState is worthless by the time it's used.
+    TTL = 10.minutes
+
+    def create(request_id:, org_slug:)
+      SecureRandom.urlsafe_base64(24).tap do |token|
+        RedisPool.conn { |r| r.set(key(token), [org_slug, request_id].join(SEPARATOR), ex: TTL.to_i) }
+      end
+    end
+
+    # Reads and deletes in one operation, so an assertion replayed against a spent token
+    # finds nothing even if it arrives inside the InResponseTo validity window.
+    def claim(token)
+      return nil if token.blank?
+
+      raw = RedisPool.conn { |r| r.getdel(key(token)) }
+      return nil if raw.blank?
+
+      org_slug, request_id = raw.split(SEPARATOR, 2)
+      {org_slug:, request_id:}
+    end
+
+    #
+    # private below here
+    #
+    SEPARATOR = "\n"
+
+    def key(token)
+      "saml_request:#{token}"
+    end
+
+    conceal :key
+  end
+end

spec/requests/saml_callback_request_spec.rb

@@ -1,7 +1,8 @@
 require "rails_helper"
 
-# End-to-end SP-initiated login: a real /init populates the session, then a signed
-# assertion (minted in-process, see spec/support/saml_helpers.rb) is POSTed to the ACS.
+# End-to-end SP-initiated login: a real /init stores the transaction and hands back a
+# RelayState, then a signed assertion (minted in-process, see spec/support/saml_helpers.rb)
+# is POSTed to the ACS along with it.
 RSpec.describe "SAML SSO login", :saml_env, type: :request do
   let(:domain) { "example.edu" }
   let(:organization) do
@@ -16,19 +17,21 @@
 
   before { saml_configuration } # ensure the config exists before /init
 
-  # Drive a real /init so the session carries saml_request_id + saml_org_slug;
-  # return the AuthnRequest id to echo back as InResponseTo.
+  # Drive a real /init; return the AuthnRequest id to echo back as InResponseTo, plus the
+  # RelayState the IdP would hand back to us.
   def initiate_login
     get "/sso/#{slug}/init"
     expect(response).to have_http_status(:found)
-    saml_request_id_from_redirect(response.headers["Location"])
+    location = response.headers["Location"]
+    [saml_request_id_from_redirect(location), Rack::Utils.parse_query(URI(location).query)["RelayState"]]
   end
 
-  def post_callback(**overrides)
-    request_id = initiate_login
+  def post_callback(relay_state: :from_init, **overrides)
+    request_id, initiated_relay_state = initiate_login
     params = {audience: settings.sp_entity_id, recipient: settings.assertion_consumer_service_url,
               in_response_to: request_id, issuer: saml_configuration.idp_entity_id, email:}.merge(overrides)
-    post "/sso/#{slug}/callback", params: {SAMLResponse: signed_saml_response(**params)}
+    post "/sso/#{slug}/callback", params: {SAMLResponse: signed_saml_response(**params),
+                                           RelayState: (relay_state == :from_init) ? initiated_relay_state : relay_state}
   end
 
   def signed_in?
@@ -155,7 +158,7 @@ def signed_in?
     end
 
     context "unsolicited response (no prior init)" do
-      it "is rejected (no session binding)" do
+      it "is rejected (no RelayState to bind it to)" do
         saml_response = signed_saml_response(audience: settings.sp_entity_id,
           recipient: settings.assertion_consumer_service_url, in_response_to: "_unsolicited",
           issuer: saml_configuration.idp_entity_id, email:)
@@ -164,5 +167,53 @@ def signed_in?
         expect(signed_in?).to be false
       end
     end
+
+    # The IdP returns the assertion as a cross-site POST, which a SameSite=Lax cookie isn't
+    # sent on - so the callback has to work with no session at all.
+    context "no session cookie on the callback" do
+      it "signs in anyway" do
+        request_id, relay_state = initiate_login
+        reset! # a fresh browser: no cookie of any kind on the POST
+        post "/sso/#{slug}/callback", params: {
+          SAMLResponse: signed_saml_response(audience: settings.sp_entity_id,
+            recipient: settings.assertion_consumer_service_url, in_response_to: request_id,
+            issuer: saml_configuration.idp_entity_id, email:),
+          RelayState: relay_state
+        }
+        expect(response).to have_http_status(:found)
+        expect(signed_in?).to be true
+      end
+    end
+
+    context "replayed assertion" do
+      it "is rejected the second time, the token being single use" do
+        request_id, relay_state = initiate_login
+        saml_response = signed_saml_response(audience: settings.sp_entity_id,
+          recipient: settings.assertion_consumer_service_url, in_response_to: request_id,
+          issuer: saml_configuration.idp_entity_id, email:)
+
+        post "/sso/#{slug}/callback", params: {SAMLResponse: saml_response, RelayState: relay_state}
+        expect(signed_in?).to be true
+
+        reset! # a fresh browser: no cookie of any kind on the POST
+        post "/sso/#{slug}/callback", params: {SAMLResponse: saml_response, RelayState: relay_state}
+        expect(response).to redirect_to(new_session_path)
+        expect(signed_in?).to be false
+      end
+    end
+
+    context "RelayState for a different organization" do
+      let(:other_organization) do
+        FactoryBot.create(:organization_with_organization_features,
+          enabled_feature_slugs: "saml_sso", user_email_domain: "other.edu")
+      end
+      it "is rejected" do
+        foreign_relay_state = Saml::RequestStore.create(request_id: "_whatever",
+          org_slug: other_organization.to_param)
+        expect { post_callback(relay_state: foreign_relay_state) }.not_to change(User, :count)
+        expect(response).to redirect_to(new_session_path)
+        expect(signed_in?).to be false
+      end
+    end
   end
 end

spec/services/saml/request_store_spec.rb

@@ -0,0 +1,42 @@
+require "rails_helper"
+
+RSpec.describe Saml::RequestStore do
+  let(:request_id) { "_abc-123" }
+  let(:org_slug) { "some-university" }
+
+  describe "create and claim" do
+    it "round trips the transaction" do
+      token = described_class.create(request_id:, org_slug:)
+      expect(token).to be_present
+      expect(described_class.claim(token)).to eq({org_slug:, request_id:})
+    end
+
+    it "only claims once" do
+      token = described_class.create(request_id:, org_slug:)
+      described_class.claim(token)
+      expect(described_class.claim(token)).to be_nil
+    end
+
+    it "issues a distinct token per request" do
+      tokens = Array.new(3) { described_class.create(request_id:, org_slug:) }
+      expect(tokens.uniq.count).to eq 3
+    end
+
+    it "expires" do
+      token = described_class.create(request_id:, org_slug:)
+      ttl = RedisPool.conn { |r| r.ttl("saml_request:#{token}") }
+      expect(ttl).to be_within(5).of(described_class::TTL.to_i)
+    end
+  end
+
+  describe "claim" do
+    it "is nil for a blank token" do
+      expect(described_class.claim(nil)).to be_nil
+      expect(described_class.claim("")).to be_nil
+    end
+
+    it "is nil for an unknown token" do
+      expect(described_class.claim("not-a-real-token")).to be_nil
+    end
+  end
+end