Fix authentication bypass with AllowIDPInitiated

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
 				}

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) {