From 128821093b0c9d88150aa9fdc9072a272bbacbe3 Mon Sep 17 00:00:00 2001 From: Shinsuke Sugaya Date: Mon, 10 Aug 2026 21:10:54 +0900 Subject: [PATCH] fix(sso): stop bouncing an unmatched SAML response back to the IdP `getLoginCredential()` decided whether a request was an assertion consumer service callback by looking for `SAML_STATE` in the session. That is the one piece of state that goes missing when the session cookie is not returned, so a callback without it was mistaken for a fresh visit and answered with a new AuthnRequest -- which the IdP answers with another assertion, in the same state, forever. The assertion arrives as a cross-site POST, and a `SameSite=Lax` cookie is not sent on one. Fess ships `tomcat.sameSiteCookies = lax`, and because the attribute is set explicitly the "Lax + POST" grace period does not apply either. SAML SSO therefore loops on a default install unless the deployment sets `tomcat.sameSiteCookies = none`. This is the same failure #3215 fixed for Entra ID. SAML cannot take the same route, because the HTTP-POST binding is not optional for the assertion consumer service, so the loop itself is broken instead: - a request is treated as a callback when it carries `SAMLResponse`, which does not depend on the session - a callback with no matching AuthnRequest ID logs a warning naming the likely cause and returns null, so `SsoAction` shows the SSO login error once A plain visit to `/sso/` while a login is in flight also no longer consumes the pending AuthnRequest ID, so the assertion that follows is still matched. Note that an IdP-initiated (unsolicited) response is now rejected rather than answered with a fresh AuthnRequest. It previously succeeded only via that extra round trip, and only where the session cookie survived; it is not a documented or tested flow, and Fess binds every response to an AuthnRequest ID it sent, so there is nothing for an unsolicited response to match against. The required cookie setting is documented on the class. Tests: 3 added to SamlAuthenticatorTest; the two behavioural ones fail before this change (the callback returned an ActionResponseCredential, and the pending ID was consumed). `org.codelibs.fess.sso` package: 166 tests, 0 failures. --- .../fess/sso/saml/SamlAuthenticator.java | 43 ++++++++- .../fess/sso/saml/SamlAuthenticatorTest.java | 93 +++++++++++++++++++ 2 files changed, 133 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/codelibs/fess/sso/saml/SamlAuthenticator.java b/src/main/java/org/codelibs/fess/sso/saml/SamlAuthenticator.java index 631ff7ad1..1980d2c64 100644 --- a/src/main/java/org/codelibs/fess/sso/saml/SamlAuthenticator.java +++ b/src/main/java/org/codelibs/fess/sso/saml/SamlAuthenticator.java @@ -133,6 +133,16 @@ * saml.security.want_assertions_signed=true * * + *

Session Cookie Settings (Required)

+ *

The IdP returns the assertion as a cross-site POST to the assertion consumer service. + * A {@code SameSite=Lax} cookie is not sent on such a request, so the shipped default in + * {@code tomcat_config.properties} has to be changed for SAML:

+ * 
+ * tomcat.sameSiteCookies = none
+ *
+ *

{@code none} is only accepted by browsers on a {@code Secure} cookie, so Fess must be + * served over HTTPS.

+ * * @see Fess Documentation */ public class SamlAuthenticator implements SsoAuthenticator { @@ -311,9 +321,9 @@ public LoginCredential getLoginCredential() { final HttpServletResponse response = LaResponseUtil.getResponse(); - final HttpSession session = request.getSession(false); - if (session != null) { - final String requestId = (String) session.getAttribute(SAML_STATE); + if (containsSamlResponse(request)) { + final HttpSession session = request.getSession(false); + final String requestId = session == null ? null : (String) session.getAttribute(SAML_STATE); if (StringUtil.isNotBlank(requestId)) { session.removeAttribute(SAML_STATE); try { @@ -336,6 +346,14 @@ public LoginCredential getLoginCredential() { return null; } } + // The assertion arrived but the matching AuthnRequest ID is unreachable. Sending + // another AuthnRequest would come straight back here in the same state, looping + // forever, so fail once instead. + logger.warn("Received a SAML response with no matching AuthnRequest ID in the session." + + " The assertion consumer service is a cross-site POST, which does not carry a SameSite=Lax cookie;" + + " see tomcat.sameSiteCookies in tomcat_config.properties." + + " An IdP-initiated (unsolicited) response is rejected for the same reason."); + return null; } try { @@ -351,6 +369,25 @@ public LoginCredential getLoginCredential() { }).orElse(null); } + /** + * Returns whether the request carries a SAML response, which is what the IdP posts to the + * assertion consumer service. + * + *

The session is deliberately not consulted here: it is the session cookie that goes + * missing when the browser refuses to send it on the cross-site POST, and a callback that is + * mistaken for a fresh visit is redirected back to the IdP forever.

+ * + *

Only solicited responses are accepted. Fess binds every response to the ID of the + * AuthnRequest it sent, so an unsolicited (IdP-initiated) response has nothing to match + * against and is rejected rather than answered with a fresh AuthnRequest.

+ * + * @param request The HTTP request. + * @return true if the request carries a SAML response. + */ + protected boolean containsSamlResponse(final HttpServletRequest request) { + return StringUtil.isNotBlank(request.getParameter("SAMLResponse")); + } + /** * Creates a login credential. * @param request The HTTP request. diff --git a/src/test/java/org/codelibs/fess/sso/saml/SamlAuthenticatorTest.java b/src/test/java/org/codelibs/fess/sso/saml/SamlAuthenticatorTest.java index c02fce2bd..3613313bf 100644 --- a/src/test/java/org/codelibs/fess/sso/saml/SamlAuthenticatorTest.java +++ b/src/test/java/org/codelibs/fess/sso/saml/SamlAuthenticatorTest.java @@ -30,12 +30,15 @@ import org.codelibs.core.lang.StringUtil; import org.codelibs.core.misc.DynamicProperties; +import org.codelibs.fess.app.web.base.login.ActionResponseCredential; import org.codelibs.fess.exception.SsoMessageException; import org.codelibs.fess.sso.SsoResponseType; import org.codelibs.fess.unit.UnitFessTestCase; import org.codelibs.fess.util.ComponentUtil; import org.codelibs.saml2.core.settings.Saml2Settings; +import org.dbflute.utflute.mocklet.MockletHttpServletRequest; import org.junit.jupiter.api.Test; +import org.lastaflute.web.login.credential.LoginCredential; public class SamlAuthenticatorTest extends UnitFessTestCase { @@ -195,6 +198,96 @@ public void test_getLogoutResponse_withoutIdpSingleLogoutServiceUrl() throws Exc } } + // =================================================================================== + // Assertion Consumer Service + // ========================== + + /** Minimal IdP settings, so that an AuthnRequest can actually be built. */ + private void setUpIdp(final DynamicProperties systemProperties) { + systemProperties.setProperty("saml.idp.entityid", "https://idp.example.com/metadata"); + systemProperties.setProperty("saml.idp.single_sign_on_service.url", "https://idp.example.com/sso"); + systemProperties.setProperty("saml.idp.certfingerprint", "afe71c28ef740bc87425be13a2263d37971da1f9"); + } + + private void tearDownIdp(final DynamicProperties systemProperties) { + systemProperties.remove("saml.idp.entityid"); + systemProperties.remove("saml.idp.single_sign_on_service.url"); + systemProperties.remove("saml.idp.certfingerprint"); + } + + @Test + public void test_containsSamlResponse() throws Exception { + final SamlAuthenticator authenticator = new SamlAuthenticator(); + + assertFalse(authenticator.containsSamlResponse(getMockRequest())); + + final MockletHttpServletRequest blank = getMockRequest(); + blank.setParameter("SAMLResponse", " "); + assertFalse(authenticator.containsSamlResponse(blank)); + + final MockletHttpServletRequest posted = getMockRequest(); + posted.setParameter("SAMLResponse", "PHNhbWxwOlJlc3BvbnNlIC8+"); + assertTrue(authenticator.containsSamlResponse(posted)); + } + + @Test + public void test_getLoginCredential_unmatchedResponseFailsInsteadOfRedirecting() throws Exception { + final SamlAuthenticator authenticator = createAuthenticator(); + final DynamicProperties systemProperties = ComponentUtil.getSystemProperties(); + final LogCapturingAppender appender = LogCapturingAppender.attach(SamlAuthenticator.class); + try { + setUpIdp(systemProperties); + // the IdP posts the assertion cross-site, so a SameSite=Lax cookie is not sent back + // and the session holding the AuthnRequest ID is unreachable + final MockletHttpServletRequest request = getMockRequest(); + request.setMethod("POST"); + request.setParameter("SAMLResponse", "PHNhbWxwOlJlc3BvbnNlIC8+"); + + // redirecting to the IdP again would come straight back in the same state + assertNull(authenticator.getLoginCredential()); + assertEquals(1, appender.warnings().size()); + assertTrue(appender.warnings().get(0), appender.warnings().get(0).contains("no matching AuthnRequest ID")); + } finally { + tearDownIdp(systemProperties); + appender.detach(); + } + } + + @Test + public void test_getLoginCredential_requestWithoutResponseStartsLogin() throws Exception { + final SamlAuthenticator authenticator = createAuthenticator(); + final DynamicProperties systemProperties = ComponentUtil.getSystemProperties(); + try { + setUpIdp(systemProperties); + final MockletHttpServletRequest request = getMockRequest(); + + final LoginCredential credential = authenticator.getLoginCredential(); + + assertTrue(String.valueOf(credential), credential instanceof ActionResponseCredential); + assertNotNull(request.getSession(false).getAttribute("SAML_STATE")); + } finally { + tearDownIdp(systemProperties); + } + } + + @Test + public void test_getLoginCredential_requestWithoutResponseKeepsPendingRequestId() throws Exception { + final SamlAuthenticator authenticator = createAuthenticator(); + final DynamicProperties systemProperties = ComponentUtil.getSystemProperties(); + try { + setUpIdp(systemProperties); + // a plain visit to /sso/ while a login is in flight must not consume the pending ID + final MockletHttpServletRequest request = getMockRequest(); + request.getSession().setAttribute("SAML_STATE", "ONELOGIN_pending"); + + authenticator.getLoginCredential(); + + assertNotNull(request.getSession(false).getAttribute("SAML_STATE")); + } finally { + tearDownIdp(systemProperties); + } + } + @Test public void test_buildDefaultUrl_withDefaultBaseUrl() throws Exception { assertEquals("http://localhost:8080/sso/metadata", new SamlAuthenticator().buildDefaultUrl("/sso/metadata"));