PPT-2536: fix SAML/ADFS login + OAuth callback lost-id (UAT-breaking)#4
Merged
Conversation
…eSite) SAML/ADFS logins 401'd with "oauth state mismatch" on every real IdP response. The IdP posts the signed assertion cross-site (HTTP-POST binding) to the ACS URL, so the browser withholds our SameSite=Lax auth-flow cookie — the CSRF `state` stashed in `#initiate` is unavailable — and the IdP echoes the CSRF value back as `RelayState`, not `state`, anyway. The legacy Ruby service (omniauth-saml) never did session-state CSRF for SAML; the callback was authenticated purely by the signed assertion. Mirror that: skip the session-state check for the `adfs`/`saml` providers and let the assertion signature authenticate. The OAuth2 path keeps full state validation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… ?id= Some OAuth providers don't round-trip the `?id=` query param on the callback redirect_uri, leaving `id` nil so the CSRF state check failed (`expect_id != (id || "")`) → 401 "oauth state mismatch". The exact id was stashed on the session in `#initiate`, so recover it when the callback omits it — more precise than the legacy Ruby guess. A present-but-mismatched id is left alone so the state check still rejects it as CSRF. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two callback-state fixes for the drop-in — both currently break a login path.
1. SAML/ADFS
RelayState(every SAML login 401s)The IdP posts the signed assertion cross-site (HTTP-POST binding) to the ACS URL, so the browser withholds our
SameSite=Laxauth-flow cookie — the CSRFstatestashed in#initiateis unavailable — and the IdP echoes the CSRF value back asRelayState, notstate. The legacy Ruby service never did session-state CSRF for SAML; the callback was authenticated purely by the signed assertion. Fix: skip the session-state check for theadfs/samlproviders and let the assertion signature authenticate. OAuth2 keeps full state validation.2. OAuth callback lost-
id(login 401s if IdP drops?id=)Some OAuth providers don't round-trip the
?id=query param on the callbackredirect_uri, soidis nil and the CSRF check 401s "oauth state mismatch". Fix: recover the exact id from the session state stashed at#initiatewhen the callback omits it (a present-but-mismatched id is left alone, so CSRF is still enforced).Testing
saml/adfscallbacks but still enforced for OAuth2 (green locally).?id=omitted and asserts it still completes (validated in CI — the WebMock spec OOMs my local machine alongside the running stack).🤖 Generated with Claude Code