fix: make the SAML IdP work and verify request signatures

[jaspermayone]

The SAML IdP could never issue an assertion. Fixing the first fault exposed five more behind it, none of which were reachable before. There was no SAML test coverage at all, so all of it is now covered.

The root fault

The controller replaced saml_idp's validate_saml_request with a presence check:

def validate_saml_request
  return if saml_request.present? || session[:saml_request_params].present?
  render plain: "Missing SAML request", status: :bad_request
end

saml_idp's default saml_request is a stub Struct, so it is always present. The check always passed, decode_request was never called, and the stub's issuer is nil. Every authentication therefore ended at find_service_provider with 403 Unknown or disabled service provider.

flowchart TD
    A[AuthnRequest arrives] --> B[validate_saml_request]
    B --> C{"saml_request.present?"}
    C -->|"always true, it is a stub"| D[return early]
    D --> E[decode_request never runs]
    E --> F["saml_request.issuer is nil"]
    F --> G[403 Unknown service provider]

Loading

What was behind it

Fault	Effect
SamlResponse.new called with keyword arguments	Takes 9 required positional args. ArgumentError on Ruby 3.4
response.add_attribute(...)	No such method. NoMethodError
session_expiry: 1.hour.from_now	A Time where the gem does .zero? and now + expiry
attributes_for returns { name => value }	Builder wants { name => { getter: } }
name_id_format: passed as a URN string	Gem builds the URN from { version => { key => getter } }, so the setting was ignored and emailAddress came out as 2.0 instead of 1.1
View wrapped the response in Base64.strict_encode64	It was already base64. No SP could read it
View read params[:RelayState]	Empty after the login redirect
store_saml_request wrote session[:saml_return_to]	AuthController reads return_to. Signing in abandoned the SAML flow and landed on the dashboard

The response building is no longer hand rolled. It now goes through the gem's own encode_authn_response, which handles the positional wiring.

Request signatures

AuthnRequest signatures were never verified. saml_idp 1.0 can check them, so this wires that up:

• New want_authn_requests_signed column, default false so every existing integration keeps working. Turn it on per provider.
• The service provider finder now supplies sign_authn_request, validate_signature, and the certificate fingerprint. saml_idp refuses to check a signature unless both the certificate and its fingerprint are present, so certificate_fingerprint computes the latter.
• acs_url stays the stored URL rather than the one the request carries, so a caller cannot redirect an assertion elsewhere.

Exposed in the admin form as "Require signed AuthnRequests".

Also fixed

config.x.app_host was set in development and production but not test, so the SAML issuer and entity ID came out as a bare /saml under test.

Verification

25 integration tests, covering the feature flag, metadata, request decoding, provider resolution, assertion contents (audience, destination, NameID, NameID format, attributes, InResponseTo, signature), the sign in round trip, authentication logging, and signature acceptance and rejection.

They are real regression tests, not confirmation of current behaviour. Against the previous controller and view they produce 11 failures and 7 errors of 24. I reverted the fix and ran them to confirm this rather than trusting a green first run.

• bin/rails test: 392 runs, 1044 assertions, 0 failures, 0 errors, 0 skips
• bin/rubocop, bin/brakeman (0 warnings), bin/bundler-audit (no vulnerabilities): clean

Note

Single Logout is repaired to the extent that it now decodes and validates the request, uses request_id rather than the non-existent .id, and sends the response to the provider's own logout endpoint. It has no test coverage here, because exercising it needs a signed LogoutRequest fixture and the endpoint appears unused. Worth a follow up before anyone relies on it.

https://claude.ai/code/session_018gopCvu5KgRjeWBM4Zw6pm

[coderabbitai]

Warning

Review limit reached

Next included review available in 33 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: c2186729-dcca-45e5-83cd-828fd4800c56

📥 Commits

Reviewing files that changed from the base of the PR and between c27c17a and 50bde9e.

📒 Files selected for processing (10)

• app/controllers/admin/saml/service_providers_controller.rb
• app/controllers/saml/idp_controller.rb
• app/models/saml/service_provider.rb
• app/views/admin/saml/service_providers/_form.html.erb
• app/views/saml/idp/create.html.erb
• config/environments/test.rb
• config/initializers/saml_idp.rb
• db/migrate/20260907150500_add_want_authn_requests_signed_to_saml_service_providers.rb
• db/schema.rb
• test/integration/saml_idp_test.rb

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}