From 3e082c10e6d370e30ad05c7120f968e168555741 Mon Sep 17 00:00:00 2001 From: aashh Date: Sat, 7 Feb 2026 17:54:14 -0800 Subject: [PATCH] fix: validate InResponseTo when present, even with AllowIDPInitiated Previously, when AllowIDPInitiated was true, InResponseTo was completely skipped at both the Response and SubjectConfirmation levels. This allowed replay of assertions with arbitrary InResponseTo values. Now, InResponseTo is only skipped when it is empty AND AllowIDPInitiated is true (a genuine IDP-initiated flow). When InResponseTo is present and non-empty, it is validated against tracked request IDs regardless of AllowIDPInitiated. IDPs that set InResponseTo to arbitrary values in IDP-initiated flows (e.g. Rippling) can use the existing ValidateRequestID hook to customize this behavior. Fixes #3 --- service_provider.go | 39 +++++++++++++++++++------------------- service_provider_test.go | 41 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 20 deletions(-) diff --git a/service_provider.go b/service_provider.go index c97886d0..1b29ef58 100644 --- a/service_provider.go +++ b/service_provider.go @@ -1098,7 +1098,17 @@ func (sp *ServiceProvider) validateRequestID(response Response, possibleRequestI } requestIDvalid := false - if sp.AllowIDPInitiated { + + // When AllowIDPInitiated is set and InResponseTo is empty, this is + // a true IDP-initiated flow — accept it without matching a request. + // When InResponseTo IS present, always validate it against the tracked + // request IDs, even if AllowIDPInitiated is set. This prevents replay + // of assertions bound to a specific AuthnRequest. + // + // Note: some IDPs (e.g. Rippling) set InResponseTo to a non-empty value + // even in IDP-initiated flows. Users of such IDPs should override + // ValidateRequestID to customize this behavior. + if sp.AllowIDPInitiated && response.InResponseTo == "" { requestIDvalid = true } else { for _, possibleRequestID := range possibleRequestIDs { @@ -1186,28 +1196,17 @@ func (sp *ServiceProvider) validateAssertion(assertion *Assertion, possibleReque return fmt.Errorf("issuer is not %q", sp.IDPMetadata.EntityID) } for _, subjectConfirmation := range assertion.Subject.SubjectConfirmations { + inResponseTo := subjectConfirmation.SubjectConfirmationData.InResponseTo requestIDvalid := false - // We *DO NOT* validate InResponseTo when AllowIDPInitiated is set. Here's why: - // - // The SAML specification does not provide clear guidance for handling InResponseTo for IDP-initiated - // requests where there is no request to be in response to. The specification says: - // - // InResponseTo [Optional] - // The ID of a SAML protocol message in response to which an attesting entity can present the - // assertion. For example, this attribute might be used to correlate the assertion to a SAML - // request that resulted in its presentation. - // - // The initial thought was that we should specify a single empty string in possibleRequestIDs for IDP-initiated - // requests so that we would ensure that an InResponseTo was *not* provided in those cases where it wasn't - // expected. Even that turns out to be frustrating for users. And in practice some IDPs (e.g. Rippling) - // set a specific non-empty value for InResponseTo in IDP-initiated requests. - // - // Finally, it is unclear that there is significant security value in checking InResponseTo when we allow - // IDP initiated assertions. - if !sp.AllowIDPInitiated { + // When AllowIDPInitiated is set and InResponseTo is empty, accept + // (true IDP-initiated flow). When InResponseTo IS present, validate + // it even if AllowIDPInitiated is set to prevent assertion replay. + if sp.AllowIDPInitiated && inResponseTo == "" { + requestIDvalid = true + } else { for _, possibleRequestID := range possibleRequestIDs { - if subjectConfirmation.SubjectConfirmationData.InResponseTo == possibleRequestID { + if inResponseTo == possibleRequestID { requestIDvalid = true break } diff --git a/service_provider_test.go b/service_provider_test.go index 35103d6b..32368aae 100644 --- a/service_provider_test.go +++ b/service_provider_test.go @@ -1234,6 +1234,47 @@ func TestSPInvalidAssertions(t *testing.T) { assertion.Conditions.AudienceRestrictions = []AudienceRestriction{} err = s.validateAssertion(&assertion, []string{"id-9e61753d64e928af5a7a341a97f420c9"}, TimeNow()) assert.Check(t, err) + assertion = Assertion{} + assert.Check(t, xml.Unmarshal(assertionBuf, &assertion)) + + // InResponseTo validation when AllowIDPInitiated is set: + // with AllowIDPInitiated, empty InResponseTo should be accepted + s.AllowIDPInitiated = true + assertion.Subject.SubjectConfirmations[0].SubjectConfirmationData.InResponseTo = "" + err = s.validateAssertion(&assertion, []string{""}, TimeNow()) + assert.Check(t, err) + assertion = Assertion{} + assert.Check(t, xml.Unmarshal(assertionBuf, &assertion)) + + // with AllowIDPInitiated, non-empty InResponseTo that doesn't match should be rejected + s.AllowIDPInitiated = true + assertion.Subject.SubjectConfirmations[0].SubjectConfirmationData.InResponseTo = "wrong-request-id" + err = s.validateAssertion(&assertion, []string{"", "id-9e61753d64e928af5a7a341a97f420c9"}, TimeNow()) + assert.Check(t, is.Error(err, "assertion SubjectConfirmation one of the possible request IDs ([ id-9e61753d64e928af5a7a341a97f420c9])")) + assertion = Assertion{} + assert.Check(t, xml.Unmarshal(assertionBuf, &assertion)) + + // with AllowIDPInitiated, matching InResponseTo should still be accepted + s.AllowIDPInitiated = true + err = s.validateAssertion(&assertion, []string{"", "id-9e61753d64e928af5a7a341a97f420c9"}, TimeNow()) + assert.Check(t, err) + s.AllowIDPInitiated = false +} + +func TestSPValidateRequestIDWithAllowIDPInitiated(t *testing.T) { + s := ServiceProvider{AllowIDPInitiated: true} + + // Empty InResponseTo should be accepted (true IDP-initiated flow) + err := s.validateRequestID(Response{InResponseTo: ""}, []string{""}) + assert.Check(t, err) + + // Non-empty InResponseTo that doesn't match should be rejected + err = s.validateRequestID(Response{InResponseTo: "wrong-id"}, []string{"", "id-expected"}) + assert.Check(t, is.Error(err, "`InResponseTo` does not match any of the possible request IDs (expected [ id-expected])")) + + // Matching InResponseTo should be accepted + err = s.validateRequestID(Response{InResponseTo: "id-expected"}, []string{"", "id-expected"}) + assert.Check(t, err) } func TestXswPermutationOneIsRejected(t *testing.T) {