[#1145] Keep the IdP discovery cookie within RFC 6265

openam-federation/openam-idpdiscovery/pom.xml

@@ -51,6 +51,29 @@
             <artifactId>mockito-core</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <!-- The container's RFC 6265 cookie processor, to check the preferred-IdP cookie against it -->
+            <groupId>org.apache.tomcat.embed</groupId>
+            <artifactId>tomcat-embed-core</artifactId>
+            <version>10.1.48</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-surefire-plugin</artifactId>
+                <configuration>
+                    <!-- tomcat-embed-core carries the Servlet 6.0 API its cookie processor calls;
+                         the Servlet 5.0 API this module compiles against would shadow it -->
+                    <classpathDependencyExcludes>
+                        <classpathDependencyExclude>jakarta.servlet:jakarta.servlet-api</classpathDependencyExclude>
+                    </classpathDependencyExcludes>
+                </configuration>
+            </plugin>
+        </plugins>
+    </build>
 </project>
 

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);
         }
@@ -499,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();

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(

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 {
 
@@ -146,20 +151,20 @@ 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() {
         withCookieSettings(false, true, null, () -> {
             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"));
         });
     }
 
@@ -177,4 +182,99 @@ 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 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, () -> {
+            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;
+    }
 }

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");
+    }
+}