From 3f6f6a1dc805808850f1b51c546426c193b9f58b Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 29 Sep 2026 12:46:41 +0300 Subject: [PATCH 1/2] Keep the IdP discovery cookie within RFC 6265 Tomcat's cookie processor refuses a cookie domain with a leading dot and a space in a cookie value. The standalone IdP discovery WAR can be configured to produce both, so /saml2writer answered 500 on the response.addCookie path; and an unencoded list of two or more preferred IdPs, when written by hand, was dropped by the container on the way back in, so the list was never read. - newCookie sets ".example.com" as "example.com" (RFC 6265 5.2.3). - With cookie encoding off, the list separator is written as %20 and read back as a space; a base64 IdP never contains '%', so values already in browsers read as before. - CookieWriterServlet logs a cookie the container still refuses and goes on to the RelayState instead of failing with 500. Fixes #1145 --- openam-federation/openam-idpdiscovery/pom.xml | 23 +++++ .../saml2/idpdiscovery/CookieUtils.java | 20 +++- .../idpdiscovery/CookieWriterServlet.java | 10 +- .../saml2/idpdiscovery/CookieUtilsTest.java | 93 ++++++++++++++++++- .../idpdiscovery/CookieWriterServletTest.java | 67 +++++++++++++ 5 files changed, 210 insertions(+), 3 deletions(-) create mode 100644 openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieWriterServletTest.java diff --git a/openam-federation/openam-idpdiscovery/pom.xml b/openam-federation/openam-idpdiscovery/pom.xml index e25866f1ba..a8a8038d26 100644 --- a/openam-federation/openam-idpdiscovery/pom.xml +++ b/openam-federation/openam-idpdiscovery/pom.xml @@ -51,6 +51,29 @@ mockito-core test + + + org.apache.tomcat.embed + tomcat-embed-core + 10.1.48 + test + + + + + + org.apache.maven.plugins + maven-surefire-plugin + + + + jakarta.servlet:jakarta.servlet-api + + + + + diff --git a/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java b/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java index a20c74b0c0..5eac6e4f98 100644 --- a/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java +++ b/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java @@ -78,6 +78,10 @@ public class CookieUtils { SystemProperties.get(IDPDiscoveryConstants.AM_COOKIE_ENCODE). equalsIgnoreCase("true")); + // The preferred IdP list separator as it is written in an unencoded cookie. + // A base64 IdP never contains '%', so it cannot be mistaken for part of one. + private static final String ESCAPED_SEPARATOR = "%20"; + private static int defAge = -1; public static Debug debug = Debug.getInstance("libIDPDiscovery"); // IDP Discovery Resource bundle @@ -219,6 +223,9 @@ public static String getCookieValueFromReq( // Bea, IBM if (cookieEncoding && (cookieValue != null)) { cookieValue= URLEncDec.decode(cookieValue); + } else if (cookieValue != null) { + cookieValue = cookieValue.replace(ESCAPED_SEPARATOR, + IDPDiscoveryConstants.PREFERRED_COOKIE_SEPERATOR); } } else { debug.message("No Cookie is in the request"); @@ -353,7 +360,13 @@ public static Cookie newCookie(String name, String value, String path) { if (cookieEncoding) { cookie = new Cookie(name, URLEncDec.encode(value)); } else { - cookie = new Cookie(name, value); + // RFC 6265 allows no space in a cookie value, and the container + // refuses to write or to read one. The separator of the preferred + // IdP list is escaped: it is the list's only character outside the + // cookie octets, since the IdPs themselves are base64. + cookie = new Cookie(name, value == null ? null : value.replace( + IDPDiscoveryConstants.PREFERRED_COOKIE_SEPERATOR, + ESCAPED_SEPARATOR)); } cookie.setMaxAge(maxAge); @@ -364,6 +377,11 @@ public static Cookie newCookie(String name, String value, String path) { cookie.setPath("/"); } + // A leading dot is ignored by RFC 6265 (5.2.3) and refused by the + // container, so ".example.com" is set as "example.com". + if ((domain != null) && domain.startsWith(".")) { + domain = domain.substring(1); + } if ((domain != null) && (domain.length() > 0)) { cookie.setDomain(domain); } diff --git a/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieWriterServlet.java b/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieWriterServlet.java index e49d12a78b..1fa69373fb 100644 --- a/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieWriterServlet.java +++ b/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieWriterServlet.java @@ -278,7 +278,15 @@ private void doGetPost( HttpServletRequest request, "/", domain ); - CookieUtils.addCookieToResponse(response,idpListCookie); + try { + CookieUtils.addCookieToResponse(response, idpListCookie); + } catch (IllegalArgumentException e) { + // The container refused the cookie, e.g. for a domain it + // does not accept. Setting the preferred IdP is done behind + // the screens, so the user still goes on to the RelayState. + CookieUtils.debug.error(classMethod + + "The preferred IDP cookie could not be set.", e); + } if(isValidReturn) { if (CookieUtils.debug.messageEnabled()) { CookieUtils.debug.message( diff --git a/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java b/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java index 3b98f3bb1b..e73a50734d 100644 --- a/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java +++ b/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java @@ -22,19 +22,24 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; import jakarta.servlet.http.Cookie; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.apache.tomcat.util.http.MimeHeaders; +import org.apache.tomcat.util.http.Rfc6265CookieProcessor; +import org.apache.tomcat.util.http.ServerCookies; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; /** * Verifies that {@link CookieUtils#isRedirectUrlValid} blocks the open redirect * described in GHSA-2pf8-52jh-5x3m while still allowing legitimate same-origin - * and relative RelayState redirects. + * and relative RelayState redirects, and that the preferred-IdP cookie gets + * through the container's RFC 6265 cookie processor on the way out and back in. */ public class CookieUtilsTest { @@ -177,4 +182,90 @@ public void addCookieToResponseWritesTheHeaderItselfWhenSameSiteIsConfigured() { verify(response).addHeader(eq("SET-COOKIE"), eq("_saml_idp=aWRw;path=/;secure;httponly;SameSite=Lax")); }); } + + @Test + public void aLeadingDotDomainIsAcceptedByTheContainer() { + Cookie cookie = CookieUtils.newCookie("_saml_idp", "aWRw", -1, "/", ".example.com"); + + String header = new Rfc6265CookieProcessor().generateHeader(cookie, null); + + assertTrue(header.contains("Domain=example.com"), header); + } + + @Test + public void anUnencodedListOfTwoIdpsIsAcceptedByTheContainer() { + withCookieEncoding(false, () -> { + Cookie cookie = CookieUtils.newCookie("_saml_idp", "aWRw aWRwMg==", -1, "/", null); + + new Rfc6265CookieProcessor().generateHeader(cookie, null); + }); + } + + @Test + public void anUnencodedListOfTwoIdpsIsReadBackFromTheContainer() { + withCookieEncoding(false, () -> { + Cookie cookie = CookieUtils.newCookie("_saml_idp", "aWRw aWRwMg==", -1, "/", null); + + assertEquals(readBack(cookie), "aWRw aWRwMg=="); + }); + } + + @Test + public void anUnencodedIdpAlreadyInTheBrowserIsReadAsItIs() { + withCookieEncoding(false, () -> { + // base64 of "https://idp/?>>?": '+', '/' and '=' are all cookie octets + Cookie cookie = new Cookie("_saml_idp", "aHR0cHM6Ly9pZHAvPz4+Pw=="); + + assertEquals(readBack(cookie), "aHR0cHM6Ly9pZHAvPz4+Pw=="); + }); + } + + @Test + public void anEncodedListOfTwoIdpsIsReadBackFromTheContainer() { + withCookieEncoding(true, () -> { + Cookie cookie = CookieUtils.newCookie("_saml_idp", "aWRw aWRwMg==", -1, "/", null); + + assertEquals(readBack(cookie), "aWRw aWRwMg=="); + }); + } + + private static void withCookieEncoding(boolean encode, Runnable body) { + boolean saved = CookieUtils.cookieEncoding; + CookieUtils.cookieEncoding = encode; + try { + body.run(); + } finally { + CookieUtils.cookieEncoding = saved; + } + } + + /** + * Returns the preferred-IdP list the reader gets from the cookie once it has been + * through the container both ways. + */ + private String readBack(Cookie cookie) { + Cookie[] cookies = sentBackThroughTheContainer(cookie); + when(request.getRequestURI()).thenReturn("/idpdiscovery/saml2reader"); + when(request.getCookies()).thenReturn(cookies); + return CookieUtils.getCookieValueFromReq(request, cookie.getName()); + } + + /** + * Writes the cookie with the container's cookie processor and returns what the container + * makes of the {@code Cookie} header a browser sends back with it. + */ + private static Cookie[] sentBackThroughTheContainer(Cookie cookie) { + Rfc6265CookieProcessor processor = new Rfc6265CookieProcessor(); + processor.generateHeader(cookie, null); + MimeHeaders headers = new MimeHeaders(); + headers.addValue("Cookie").setString(cookie.getName() + "=" + cookie.getValue()); + ServerCookies parsed = new ServerCookies(4); + processor.parseCookieHeader(headers, parsed); + Cookie[] cookies = new Cookie[parsed.getCookieCount()]; + for (int i = 0; i < cookies.length; i++) { + cookies[i] = new Cookie(parsed.getCookie(i).getName().toString(), + parsed.getCookie(i).getValue().toString()); + } + return cookies; + } } diff --git a/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieWriterServletTest.java b/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieWriterServletTest.java new file mode 100644 index 0000000000..95ba554995 --- /dev/null +++ b/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieWriterServletTest.java @@ -0,0 +1,67 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC. + */ +package com.sun.identity.saml2.idpdiscovery; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import jakarta.servlet.http.Cookie; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +public class CookieWriterServletTest { + + private boolean savedHttpOnly; + private String savedSameSite; + + @BeforeMethod + public void routeTheCookieThroughTheContainer() { + savedHttpOnly = CookieUtils.cookieHttpOnly; + savedSameSite = CookieUtils.cookieSameSite; + CookieUtils.cookieHttpOnly = false; + CookieUtils.cookieSameSite = null; + } + + @AfterMethod + public void restoreTheCookieSettings() { + CookieUtils.cookieHttpOnly = savedHttpOnly; + CookieUtils.cookieSameSite = savedSameSite; + } + + @Test + public void goesOnToTheRelayStateWhenTheContainerRefusesTheCookie() throws Exception { + HttpServletRequest request = mock(HttpServletRequest.class); + when(request.getRequestURI()).thenReturn("/idpdiscovery/saml2writer"); + when(request.getScheme()).thenReturn("https"); + when(request.getServerName()).thenReturn("cdc.example.com"); + when(request.getServerPort()).thenReturn(443); + when(request.getParameter("_saml_idp")).thenReturn("https://idp.example.com"); + when(request.getParameter("RelayState")).thenReturn("/sp/return"); + HttpServletResponse response = mock(HttpServletResponse.class); + doThrow(new IllegalArgumentException("An invalid domain [example.com:8443] was specified")) + .when(response).addCookie(any(Cookie.class)); + + new CookieWriterServlet().doGet(request, response); + + verify(response).sendRedirect("/sp/return"); + } +} From 20b03a0f23ccec2e1c1b3c18deb701398f9bbb34 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Tue, 29 Sep 2026 15:17:45 +0300 Subject: [PATCH 2/2] Pin that only a leading dot is dropped from the cookie domain - aDomainWithoutALeadingDotIsSetAsItIs: a configured "example.com" is set as it is; stripping the first character of any domain now fails with "Domain=xample.com". - The addCookieToResponse comment and the #1136 HttpOnly test no longer give the leading-dot domain or the unencoded list as the reason for the hand-built header, since newCookie produces neither; they name a domain the container still refuses, "example.com:8443". --- .../identity/saml2/idpdiscovery/CookieUtils.java | 5 ++--- .../saml2/idpdiscovery/CookieUtilsTest.java | 15 ++++++++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java b/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java index 5eac6e4f98..b30bf58c5a 100644 --- a/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java +++ b/openam-federation/openam-idpdiscovery/src/main/java/com/sun/identity/saml2/idpdiscovery/CookieUtils.java @@ -517,9 +517,8 @@ public static void addCookieToResponse(HttpServletResponse response, // The servlet Cookie has no SameSite attribute, and an HttpOnly cookie // keeps the hand-built header it has always had: the container's cookie - // processor rejects a domain with a leading dot (".example.com") and the - // space-separated value of an unencoded preferred-IdP list, both of which - // this WAR can be configured to produce. + // processor rejects cookie domains this WAR can be configured with, such + // as one with a port ("example.com:8443"). StringBuffer sb = new StringBuffer(150); sb.append(cookie.getName()).append("=").append(cookie.getValue()); String path = cookie.getPath(); diff --git a/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java b/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java index e73a50734d..6de8d18b69 100644 --- a/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java +++ b/openam-federation/openam-idpdiscovery/src/test/java/com/sun/identity/saml2/idpdiscovery/CookieUtilsTest.java @@ -151,7 +151,7 @@ public void addCookieToResponseLeavesHttpOnlyAloneWhenNotConfigured() { /** * HttpOnly without SameSite still goes out as a hand-built header: the container's cookie - * processor would reject a configured domain such as ".example.com". + * processor would reject a configured domain such as "example.com:8443". */ @Test public void addCookieToResponseWritesTheHeaderItselfWhenHttpOnlyIsConfigured() { @@ -159,12 +159,12 @@ public void addCookieToResponseWritesTheHeaderItselfWhenHttpOnlyIsConfigured() { HttpServletResponse response = mock(HttpServletResponse.class); Cookie cookie = new Cookie("_saml_idp", "aWRw"); cookie.setPath("/"); - cookie.setDomain(".example.com"); + cookie.setDomain("example.com:8443"); CookieUtils.addCookieToResponse(response, cookie); verify(response, never()).addCookie(any(Cookie.class)); - verify(response).addHeader(eq("SET-COOKIE"), eq("_saml_idp=aWRw;path=/;domain=.example.com;httponly")); + verify(response).addHeader(eq("SET-COOKIE"), eq("_saml_idp=aWRw;path=/;domain=example.com:8443;httponly")); }); } @@ -192,6 +192,15 @@ public void aLeadingDotDomainIsAcceptedByTheContainer() { assertTrue(header.contains("Domain=example.com"), header); } + @Test + public void aDomainWithoutALeadingDotIsSetAsItIs() { + Cookie cookie = CookieUtils.newCookie("_saml_idp", "aWRw", -1, "/", "example.com"); + + String header = new Rfc6265CookieProcessor().generateHeader(cookie, null); + + assertTrue(header.contains("Domain=example.com"), header); + } + @Test public void anUnencodedListOfTwoIdpsIsAcceptedByTheContainer() { withCookieEncoding(false, () -> {