Remove duplicated set-cookie header from response when expiring cookies

Closes #17192

adapters/spi/servlet-adapter-spi/src/main/java/org/keycloak/adapters/servlet/ServletHttpFacade.java

@@ -207,7 +207,7 @@ public class ServletHttpFacade implements HttpFacade {
 
         @Override
         public void setCookie(String name, String value, String path, String domain, int maxAge, boolean secure, boolean httpOnly) {
-            StringBuffer cookieBuf = new StringBuffer();
+            StringBuilder cookieBuf = new StringBuilder();
             ServerCookie.appendCookieValue(cookieBuf, 1, name, value, path, domain, null, maxAge, secure, httpOnly, null);
             String cookie = cookieBuf.toString();
             response.addHeader("Set-Cookie", cookie);

adapters/spi/tomcat-adapter-spi/src/main/java/org/keycloak/adapters/tomcat/CatalinaHttpFacade.java

@@ -218,7 +218,7 @@ public class CatalinaHttpFacade implements HttpFacade {
 
         @Override
         public void setCookie(String name, String value, String path, String domain, int maxAge, boolean secure, boolean httpOnly) {
-            StringBuffer cookieBuf = new StringBuffer();
+            StringBuilder cookieBuf = new StringBuilder();
             ServerCookie.appendCookieValue(cookieBuf, 1, name, value, path, domain, null, maxAge, secure, httpOnly, null);
             String cookie = cookieBuf.toString();
             response.addHeader("Set-Cookie", cookie);

common/src/main/java/org/keycloak/common/util/ServerCookie.java

@@ -178,7 +178,7 @@ public class ServerCookie implements Serializable {
 
 
     // TODO RFC2965 fields also need to be passed
-    public static void appendCookieValue(StringBuffer headerBuf,
+    public static void appendCookieValue(StringBuilder headerBuf,
                                          int version,
                                          String name,
                                          String value,

examples/providers/authenticator/src/main/java/org/keycloak/examples/authenticator/SecretQuestionAuthenticator.java

@@ -17,6 +17,7 @@
 
 package org.keycloak.examples.authenticator;
 
+import org.keycloak.http.HttpCookie;
 import org.keycloak.http.HttpResponse;
 import org.keycloak.authentication.AuthenticationFlowContext;
 import org.keycloak.authentication.AuthenticationFlowError;
@@ -24,7 +25,6 @@ import org.keycloak.authentication.Authenticator;
 import org.keycloak.authentication.CredentialValidator;
 import org.keycloak.authentication.RequiredActionFactory;
 import org.keycloak.authentication.RequiredActionProvider;
-import org.keycloak.common.util.ServerCookie;
 import org.keycloak.credential.CredentialProvider;
 import org.keycloak.models.AuthenticatorConfigModel;
 import org.keycloak.models.KeycloakSession;
@@ -33,7 +33,6 @@ import org.keycloak.models.UserCredentialModel;
 import org.keycloak.models.UserModel;
 
 import javax.ws.rs.core.Cookie;
-import javax.ws.rs.core.HttpHeaders;
 import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
 import java.net.URI;
@@ -97,10 +96,7 @@ public class SecretQuestionAuthenticator implements Authenticator, CredentialVal
 
     public void addCookie(AuthenticationFlowContext context, String name, String value, String path, String domain, String comment, int maxAge, boolean secure, boolean httpOnly) {
         HttpResponse response = context.getSession().getContext().getHttpResponse();
-        StringBuffer cookieBuf = new StringBuffer();
+        response.setCookieIfAbsent(new HttpCookie(1, name, value, path, domain, comment, maxAge, secure, httpOnly, null));
-        ServerCookie.appendCookieValue(cookieBuf, 1, name, value, path, domain, comment, maxAge, secure, httpOnly, null);
-        String cookie = cookieBuf.toString();
-        response.addHeader(HttpHeaders.SET_COOKIE, cookie);
     }
 
 

server-spi/src/main/java/org/keycloak/http/HttpCookie.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright 2023 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.keycloak.http;
+
+import org.keycloak.common.util.ServerCookie;
+import org.keycloak.common.util.ServerCookie.SameSiteAttributeValue;
+
+/**
+ * An extension of {@link javax.ws.rs.core.Cookie} in order to support additional
+ * fields and behavior.
+ */
+public final class HttpCookie extends javax.ws.rs.core.Cookie {
+
+    private final String comment;
+    private final int maxAge;
+    private final boolean secure;
+    private final boolean httpOnly;
+    private final SameSiteAttributeValue sameSite;
+
+    public HttpCookie(int version, String name, String value, String path, String domain, String comment, int maxAge, boolean secure, boolean httpOnly, SameSiteAttributeValue sameSite) {
+        super(name, value, path, domain, version);
+        this.comment = comment;
+        this.maxAge = maxAge;
+        this.secure = secure;
+        this.httpOnly = httpOnly;
+        this.sameSite = sameSite;
+    }
+
+    public String toHeaderValue() {
+        StringBuilder cookieBuf = new StringBuilder();
+        ServerCookie.appendCookieValue(cookieBuf, getVersion(), getName(), getValue(), getPath(), getDomain(), comment, maxAge, secure, httpOnly, sameSite);
+        return cookieBuf.toString();
+    }
+}

server-spi/src/main/java/org/keycloak/http/HttpResponse.java

@@ -16,6 +16,7 @@
  */
 
 package org.keycloak.http;
+
 /**
  * <p>Represents an out coming HTTP response.
  *
@@ -45,4 +46,20 @@ public interface HttpResponse {
      * @param value the header value
      */
     void setHeader(String name, String value);
+
+    /**
+     * Sets a new cookie only if not yet set.
+     *
+     * @param cookie the cookie
+     */
+    void setCookieIfAbsent(HttpCookie cookie);
+
+    /**
+     * Adding cookies at the end of the transaction helps when retrying a transaction might add the
+     * cookie multiple times. In some scenarios it must not be added at the end of the transaction,
+     * as at that time the response has already been sent to the caller ("committed"), so the code
+     * needs to make a choice. As retrying transactions is the exception, adding cookies at the end
+     * of the transaction is also the exception and needs to be switched on where necessary.
+     */
+    void setWriteCookiesOnTransactionComplete();
 }

services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java

@@ -49,7 +49,6 @@ import org.keycloak.services.clientpolicy.context.PreAuthorizationRequestContext
 import org.keycloak.services.messages.Messages;
 import org.keycloak.services.resources.LoginActionsService;
 import org.keycloak.services.util.CacheControlUtil;
-import org.keycloak.services.util.CookieHelper;
 import org.keycloak.sessions.AuthenticationSessionModel;
 import org.keycloak.util.TokenUtil;
 
@@ -133,7 +132,7 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase {
         return KeycloakModelUtils.runJobInRetriableTransaction(session.getKeycloakSessionFactory(), new ResponseSessionTask(session) {
             @Override
             public Response runInternal(KeycloakSession session) {
-                CookieHelper.addCookiesAtEndOfTransaction(session);
+                session.getContext().getHttpResponse().setWriteCookiesOnTransactionComplete();
                 // create another instance of the endpoint to isolate each run.
                 AuthorizationEndpoint other = new AuthorizationEndpoint(session,
                         new EventBuilder(session.getContext().getRealm(), session, clientConnection), action);

services/src/main/java/org/keycloak/services/DefaultKeycloakContext.java

@@ -176,6 +176,6 @@ public class DefaultKeycloakContext implements KeycloakContext {
     }
 
     protected HttpResponse createHttpResponse() {
-        return new HttpResponseImpl(getContextObject(org.jboss.resteasy.spi.HttpResponse.class));
+        return new HttpResponseImpl(session, getContextObject(org.jboss.resteasy.spi.HttpResponse.class));
     }
 }

services/src/main/java/org/keycloak/services/HttpResponseImpl.java

@@ -17,14 +17,26 @@
 
 package org.keycloak.services;
 
-import org.keycloak.http.HttpResponse;
+import java.util.HashSet;
+import java.util.Set;
 
-public class HttpResponseImpl implements HttpResponse {
+import javax.ws.rs.core.HttpHeaders;
+
+import org.keycloak.http.HttpCookie;
+import org.keycloak.http.HttpResponse;
+import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.KeycloakTransaction;
+
+public class HttpResponseImpl implements HttpResponse, KeycloakTransaction {
 
     private final org.jboss.resteasy.spi.HttpResponse delegate;
+    private Set<HttpCookie> cookies;
+    private boolean transactionActive;
+    private boolean writeCookiesOnTransactionComplete;
 
-    public HttpResponseImpl(org.jboss.resteasy.spi.HttpResponse delegate) {
+    public HttpResponseImpl(KeycloakSession session, org.jboss.resteasy.spi.HttpResponse delegate) {
         this.delegate = delegate;
+        session.getTransactionManager().enlistAfterCompletion(this);
     }
 
     @Override
@@ -44,6 +56,31 @@ public class HttpResponseImpl implements HttpResponse {
         delegate.getOutputHeaders().putSingle(name, value);
     }
 
+    @Override
+    public void setCookieIfAbsent(HttpCookie cookie) {
+        if (cookie == null) {
+            throw new IllegalArgumentException("Cookie is null");
+        }
+
+        if (cookies == null) {
+            cookies = new HashSet<>();
+        }
+
+        if (cookies.add(cookie)) {
+            if (writeCookiesOnTransactionComplete) {
+                // cookies are written after transaction completes
+                return;
+            }
+
+            addHeader(HttpHeaders.SET_COOKIE, cookie.toHeaderValue());
+        }
+    }
+
+    @Override
+    public void setWriteCookiesOnTransactionComplete() {
+        this.writeCookiesOnTransactionComplete = true;
+    }
+
     /**
      * Validate that the response has not been committed.
      * If the response is already committed, the headers and part of the response have been sent already.
@@ -55,4 +92,58 @@ public class HttpResponseImpl implements HttpResponse {
         }
     }
 
+    @Override
+    public void begin() {
+        transactionActive = true;
+    }
+
+    @Override
+    public void commit() {
+        if (!transactionActive) {
+            throw new IllegalStateException("Transaction not active. Response already committed or rolled back");
+        }
+
+        try {
+            addCookiesAfterTransaction();
+        } finally {
+            close();
+        }
+    }
+
+    @Override
+    public void rollback() {
+        close();
+    }
+
+    @Override
+    public void setRollbackOnly() {
+
+    }
+
+    @Override
+    public boolean getRollbackOnly() {
+        return false;
+    }
+
+    @Override
+    public boolean isActive() {
+        return transactionActive;
+    }
+
+    private void close() {
+        transactionActive = false;
+        cookies = null;
+    }
+
+    private void addCookiesAfterTransaction() {
+        if (cookies == null || !writeCookiesOnTransactionComplete) {
+            return;
+        }
+
+        // Ensure that cookies are only added when the transaction is complete, as otherwise cookies will be set for
+        // error pages, or will be added twice when running retries.
+        for (HttpCookie cookie : cookies) {
+            addHeader(HttpHeaders.SET_COOKIE, cookie.toHeaderValue());
+        }
+    }
 }

services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java

@@ -909,7 +909,6 @@ public class AuthenticationManager {
         AuthResult authResult = verifyIdentityToken(session, realm, session.getContext().getUri(), session.getContext().getConnection(), checkActive, false, null, true, tokenString, session.getContext().getRequestHeaders(), VALIDATE_IDENTITY_COOKIE);
         if (authResult == null) {
             expireIdentityCookie(realm, session.getContext().getUri(), session);
-            expireOldIdentityCookie(realm, session.getContext().getUri(), session);
             return null;
         }
         authResult.getSession().setLastSessionRefresh(Time.currentTime());

services/src/main/java/org/keycloak/services/resources/LoginActionsService.java

@@ -81,7 +81,6 @@ import org.keycloak.services.messages.Messages;
 import org.keycloak.services.util.AuthenticationFlowURLHelper;
 import org.keycloak.services.util.BrowserHistoryHelper;
 import org.keycloak.services.util.CacheControlUtil;
-import org.keycloak.services.util.CookieHelper;
 import org.keycloak.services.util.LocaleUtil;
 import org.keycloak.sessions.AuthenticationSessionCompoundId;
 import org.keycloak.sessions.AuthenticationSessionModel;
@@ -261,7 +260,7 @@ public class LoginActionsService {
             @Override
             public Response runInternal(KeycloakSession session) {
                 // create another instance of the endpoint to isolate each run.
-                CookieHelper.addCookiesAtEndOfTransaction(session);
+                session.getContext().getHttpResponse().setWriteCookiesOnTransactionComplete();
                 LoginActionsService other = new LoginActionsService(session, new EventBuilder(session.getContext().getRealm(), session, clientConnection));
                 // process the request in the created instance.
                 return other.authenticateInternal(authSessionId, code, execution, clientId, tabId);

services/src/main/java/org/keycloak/services/util/CookieHelper.java

@@ -18,18 +18,16 @@
 package org.keycloak.services.util;
 
 import org.jboss.logging.Logger;
+import org.keycloak.http.HttpCookie;
 import org.keycloak.http.HttpResponse;
 import org.jboss.resteasy.util.CookieParser;
-import org.keycloak.common.util.ServerCookie;
 import org.keycloak.models.KeycloakSession;
-import org.keycloak.models.KeycloakTransaction;
 
 import javax.ws.rs.core.Cookie;
 import javax.ws.rs.core.HttpHeaders;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 
 import static org.keycloak.common.util.ServerCookie.SameSiteAttributeValue;
@@ -44,7 +42,6 @@ public class CookieHelper {
     public static final String LEGACY_COOKIE = "_LEGACY";
 
     private static final Logger logger = Logger.getLogger(CookieHelper.class);
-    private static final String ADD_COOKIES_AT_END_OF_TRANSACTION = CookieHelper.class.getName() + "_ADD_COOKIES_AT_END_OF_TRANSACTION";
 
     /**
      * Set a response cookie.  This solely exists because JAX-RS 1.1 does not support setting HttpOnly cookies
@@ -69,14 +66,9 @@ public class CookieHelper {
         boolean secure_sameSite = sameSite == SameSiteAttributeValue.NONE || secure; // when SameSite=None, Secure attribute must be set
 
         HttpResponse response = session.getContext().getHttpResponse();
-        StringBuffer cookieBuf = new StringBuffer();
+        HttpCookie cookie = new HttpCookie(1, name, value, path, domain, comment, maxAge, secure_sameSite, httpOnly, sameSite);
-        ServerCookie.appendCookieValue(cookieBuf, 1, name, value, path, domain, comment, maxAge, secure_sameSite, httpOnly, sameSite);
+
-        String cookie = cookieBuf.toString();
+        response.setCookieIfAbsent(cookie);
-        if (shouldAddCookiesAtEndOfTransaction(session)) {
-            session.getTransactionManager().enlistAfterCompletion(new CookieTransaction(response, cookie));
-        } else {
-            response.addHeader(HttpHeaders.SET_COOKIE, cookie);
-        }
 
         // a workaround for browser in older Apple OSs – browsers ignore cookies with SameSite=None
         if (sameSiteParam == SameSiteAttributeValue.NONE) {
@@ -84,21 +76,6 @@ public class CookieHelper {
         }
     }
 
-    private static boolean shouldAddCookiesAtEndOfTransaction(KeycloakSession session) {
-        return Objects.equals(session.getAttribute(ADD_COOKIES_AT_END_OF_TRANSACTION), Boolean.TRUE);
-    }
-
-    /**
-     * Adding cookies at the end of the transaction helps when retrying a transaction might add the
-     * cookie multiple times. In some scenarios it must not be added at the end of the transaction,
-     * as at that time the response has already been sent to the caller ("committed"), so the code
-     * needs to make a choice. As retrying transactions is the exception, adding cookies at the end
-     * of the transaction is also the exception and needs to be switched on where necessary.
-     */
-    public static void addCookiesAtEndOfTransaction(KeycloakSession session) {
-        session.setAttribute(ADD_COOKIES_AT_END_OF_TRANSACTION, Boolean.TRUE);
-    }
-
     /**
      * Set a response cookie avoiding SameSite parameter
      * @param name
@@ -172,49 +149,4 @@ public class CookieHelper {
             return cookies.get(legacy);
         }
     }
-
-    /**
-     * Ensure that cookies are only added when the transaction is complete, as otherwise cookies will be set for error pages,
-     * or will be added twice when running retries.
-     */
-    private static class CookieTransaction implements KeycloakTransaction {
-        private final HttpResponse response;
-        private final String cookie;
-        private boolean transactionActive;
-
-        public CookieTransaction(HttpResponse response, String cookie) {
-            this.response = response;
-            this.cookie = cookie;
-        }
-
-        @Override
-        public void begin() {
-            transactionActive = true;
-        }
-
-        @Override
-        public void commit() {
-            response.addHeader(HttpHeaders.SET_COOKIE, cookie);
-            transactionActive = false;
-        }
-
-        @Override
-        public void rollback() {
-            transactionActive = false;
-        }
-
-        @Override
-        public void setRollbackOnly() {
-        }
-
-        @Override
-        public boolean getRollbackOnly() {
-            return false;
-        }
-
-        @Override
-        public boolean isActive() {
-            return transactionActive;
-        }
-    }
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/CookieTest.java

@@ -16,20 +16,25 @@
  */
 package org.keycloak.testsuite.cookies;
 
+import org.apache.http.Header;
+import org.apache.http.client.CookieStore;
 import org.apache.http.client.methods.CloseableHttpResponse;
 import org.apache.http.client.methods.HttpGet;
 import org.apache.http.client.protocol.HttpClientContext;
 import org.apache.http.impl.client.BasicCookieStore;
 import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
 import org.apache.http.impl.cookie.BasicClientCookie;
 import org.apache.http.protocol.BasicHttpContext;
 import org.apache.http.protocol.HttpContext;
 import org.apache.http.util.EntityUtils;
 import org.jboss.arquillian.graphene.page.Page;
-import org.junit.BeforeClass;
 import org.junit.Test;
+import org.keycloak.admin.client.resource.UsersResource;
 import org.keycloak.common.Profile;
+import org.keycloak.models.Constants;
 import org.keycloak.representations.idm.RealmRepresentation;
+import org.keycloak.representations.idm.UserRepresentation;
 import org.keycloak.testsuite.AbstractKeycloakTest;
 import org.keycloak.testsuite.arquillian.annotation.DisableFeature;
 import org.keycloak.testsuite.auth.page.AuthRealm;
@@ -40,7 +45,10 @@ import org.keycloak.testsuite.util.OAuthClient.AuthorizationEndpointResponse;
 import org.keycloak.testsuite.util.RealmBuilder;
 import org.openqa.selenium.Cookie;
 
+import java.io.IOException;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.not;
@@ -56,6 +64,8 @@ import static org.keycloak.services.util.CookieHelper.LEGACY_COOKIE;
 import static org.keycloak.testsuite.admin.AbstractAdminTest.loadJson;
 import static org.keycloak.testsuite.util.URLAssert.assertCurrentUrlStartsWithLoginUrlOf;
 
+import javax.ws.rs.core.HttpHeaders;
+
 /**
  *
  * @author hmlnarik
@@ -186,6 +196,48 @@ public class CookieTest extends AbstractKeycloakTest {
         assertSameSiteCookies(sameSiteAuthSessionIdCookie, legacyAuthSessionIdCookie);
     }
 
+    @Test
+    public void testNoDuplicationsWhenExpiringCookies() throws IOException {
+        ContainerAssume.assumeAuthServerSSL();
+
+        accountPage.navigateTo();
+        assertCurrentUrlStartsWithLoginUrlOf(accountPage);
+
+        loginPage.login("test-user@localhost", "password");
+
+        UsersResource usersResource = realmsResouce().realm(AuthRealm.TEST).users();
+        UserRepresentation user = usersResource.search("test-user@localhost").get(0);
+
+        usersResource.get(user.getId()).logout();
+
+        Cookie invalidIdentityCookie = driver.manage().getCookieNamed(KEYCLOAK_IDENTITY_COOKIE);
+        CookieStore cookieStore = new BasicCookieStore();
+
+        BasicClientCookie invalidClientIdentityCookie = new BasicClientCookie(invalidIdentityCookie.getName(), invalidIdentityCookie.getValue());
+
+        invalidClientIdentityCookie.setDomain(invalidIdentityCookie.getDomain());
+        invalidClientIdentityCookie.setPath(invalidClientIdentityCookie.getPath());
+
+        cookieStore.addCookie(invalidClientIdentityCookie);
+
+        try (CloseableHttpClient client = HttpClients.custom().setDefaultCookieStore(cookieStore).build()) {
+            HttpGet get = new HttpGet(
+                    suiteContext.getAuthServerInfo().getContextRoot() + "/auth/realms/" + AuthRealm.TEST + "/protocol/openid-connect/auth?response_type=code&client_id=" + Constants.ACCOUNT_CONSOLE_CLIENT_ID +
+                            "&redirect_uri=" + suiteContext.getAuthServerInfo().getContextRoot() + "/auth/realms/" + AuthRealm.TEST + "/account&scope=openid");
+
+            try (CloseableHttpResponse response = client.execute(get)) {
+                Header[] headers = response.getHeaders(HttpHeaders.SET_COOKIE);
+                Set<String> cookies = new HashSet<>();
+
+                for (Header header : headers) {
+                    assertTrue("Cookie '" + header.getValue() + "' is duplicated", cookies.add(header.getValue()));
+                }
+
+                assertFalse(cookies.isEmpty());
+            }
+        }
+    }
+
     private void assertSameSiteCookies(Cookie sameSiteCookie, Cookie legacyCookie) {
         assertNotNull("SameSite cookie shouldn't be null", sameSiteCookie);
         assertNotNull("Legacy cookie shouldn't be null", legacyCookie);