fix(api): SAML AuthnStatement + Redirect-binding SSO endpoint - #174
Merged
Conversation
themightychris
added a commit
that referenced
this pull request
Sep 10, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jmpurQpBHXD9yLFEGUriR
The SAML assertion spec described Subject, Conditions and the attribute set but never an AuthnStatement, and the implementation followed it. The Web Browser SSO profile requires at least one, and Slack's SP config sends a RequestedAuthnContext, so an assertion without one is the most likely rejection when we test against the real workspace. Spec the statement: AuthnInstant = assertion issue time, a fresh SessionIndex, and a fixed AuthnContextClassRef of ...:ac:classes:Password. That class matches what the legacy Emergence SAML2 connector emitted (setAuthnContext(AC_PASSWORD)) against this same workspace, which is worth more than the marginally more precise PasswordProtectedTransport. Slack's live "Test configuration" also failed with a 404: it sends the AuthnRequest over HTTP-Redirect (GET with a DEFLATEd SAMLRequest in the query), and although our metadata advertises that binding at the /sso Location, the spec only defined POST. Add GET to the endpoints table and describe both bindings as one flow that differs only in transport. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jmpurQpBHXD9yLFEGUriR
Two gaps found while preparing to test the IdP against the live Slack workspace, both in the SP-initiated path. The assertion carried Subject, Conditions and AttributeStatement but no AuthnStatement, which the Web Browser SSO profile requires and which Slack's RequestedAuthnContext gives it a reason to check. Add one to the login response template between Conditions and AttributeStatement, with AuthnInstant = the assertion issue time, a fresh SessionIndex from the same id source as the Response/Assertion IDs, and the fixed class urn:oasis:names:tc:SAML:2.0:ac:classes:Password. That class is what the legacy Emergence SAML2 connector asserted against this workspace (setAuthnContext(AC_PASSWORD)), so existing Slack accounts see the same context they were established under. The placeholders ride the existing customTagReplacement path; samlify signs after that callback returns, so the statement lands inside the signed subtree. Slack's "Test configuration" then 404ed: Slack sends the AuthnRequest over HTTP-Redirect (GET with a DEFLATEd SAMLRequest in the query), and while the metadata advertised that binding at /sso, only POST was registered. Add GET /api/saml/slack/sso. Rather than call samlify's 'redirect' parser and teach the resume cookie about bindings, inflate at the edge back to the plain-base64 form the POST binding carries and run both through one handleSpInitiatedSso; samlify's redirect flow is that same inflate followed by the same parser, so nothing is lost and the cookie's samlRequest claim keeps a single shape. Fold spaces back to '+' in the query value, since a sender that leaves base64 '+' unescaped has it URL-decoded to a space. Tests cover the AuthnStatement (position, ClassRef, AuthnInstant <= IssueInstant, SessionIndex distinct from the assertion ID), verify the signature cryptographically against the metadata endpoint's cert and show that tampering with the ClassRef breaks it, and exercise the Redirect binding for signed-in, anonymous (through /sso/resume), bad ACS and non-DEFLATEd payloads. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jmpurQpBHXD9yLFEGUriR
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015jmpurQpBHXD9yLFEGUriR
themightychris
force-pushed
the
fix/saml-authn-statement
branch
from
September 10, 2026 06:36
983276b to
78ab9be
Compare
Merged
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.
Summary
Two conformance gaps in the Slack SAML IdP, both found while preparing to test against the live workspace.
1. Assertion had no
AuthnStatement. The Web Browser SSO profile requires at least one, and Slack sends aRequestedAuthnContext, so this was the most likely first rejection. The assertion now carries one betweenConditionsandAttributeStatement:AuthnInstant= the assertion'sIssueInstantSessionIndex= a fresh opaque id (distinct from the assertion ID)AuthnContextClassRef=urn:oasis:names:tc:SAML:2.0:ac:classes:Password, fixed, never echoed from the requestSessionNotOnOrAfteromittedWhy
Passwordand notPasswordProtectedTransport: the legacy Emergence SAML2 connector laddr used (emergence-saml2→Emergence\SAML2\Connector::getSAMLAssertion) calledsetAuthnContext(SAML2_Constants::AC_PASSWORD)andsetSessionIndex(generateId()), with simplesamlphp defaultingAuthnInstantto construction time and leavingSessionNotOnOrAfternull.AC_PASSWORDis...:ac:classes:Password(confirmed in simplesamlphp/saml2Constants.php). That class was accepted by this same workspace for years, so v1 matches it. Slack's default request asks forPasswordProtectedTransport, but the legacy evidence says the check is lenient; if the live test rejects on context it's a one-line constant + spec edit.2.
GET /api/saml/slack/ssowas missing. Slack's "Test configuration" 404ed because Slack sends the AuthnRequest over HTTP-Redirect (GET ...?SAMLRequest=<deflate+base64>), which our metadata advertises at that Location but onlyPOSTwas registered. The GET handler inflates the payload back to the plain-base64 form the POST binding carries, and both methods share onehandleSpInitiatedSso(ACS allow-list, signed-in → auto-submit form, anonymous → resume cookie →/sso/resume). That keeps the resume cookie binding-agnostic; samlify's own'redirect'parser is the same inflate followed by the same parse.RelayStateis preserved.Changes
specs/api/saml.md— new### Authentication statementsection; endpoints table listsGETandPOST /sso; SP-initiated section describes both bindings as one flow (spec-first commit)apps/api/src/saml/config.ts—SLACK_AUTHN_CONTEXT_CLASS_REF;AuthnStatementin the template;AuthnInstant/SessionIndex/AuthnContextClassRefsubstitutionsapps/api/src/routes/saml.ts—GET /api/saml/slack/sso;inflateRedirectBindingRequest; POST body extracted into sharedhandleSpInitiatedSso; SessionIndex minted alongside the Response/Assertion IDsapps/api/tests/saml.test.ts— AuthnStatement shape + ordering; cryptographic signature verification against the metadata cert (viasamlify.SamlLib.verifySignature) plus a tamper case; Redirect binding: signed-in, anonymous → resume, bad ACS, non-DEFLATEd payloadplans/saml-authn-statement.md— plan, closed outTest plan
npm run type-checkcleannpm run lintcleanapps/api/tests/saml.test.ts18/18npm test🤖 Generated with Claude Code
https://claude.ai/code/session_015jmpurQpBHXD9yLFEGUriR