From c4b1fd092a14f99961c88013cf051bf2089c00bb Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Tue, 6 Feb 2024 15:14:04 +0100 Subject: [PATCH] Use code from RestEasy to create and set cookies (#26558) Closes #26557 Signed-off-by: stianst --- .../keycloak/common/util/ServerCookie.java | 4 + .../topics/keycloak/changes-24_0_0.adoc | 8 +- pom.xml | 2 +- .../resteasy/QuarkusHttpResponse.java | 21 +-- .../java/org/keycloak/cookie/CookieScope.java | 14 +- .../java/org/keycloak/http/HttpCookie.java | 37 +++-- .../java/org/keycloak/http/HttpResponse.java | 15 +- .../keycloak/cookie/CookiePathResolver.java | 34 ++++ .../keycloak/cookie/CookieSecureResolver.java | 39 +++++ .../cookie/DefaultCookieProvider.java | 145 ++++++++++-------- .../cookie/DefaultCookieProviderFactory.java | 22 ++- .../freemarker/AuthenticationStateCookie.java | 3 +- .../keycloak/services/HttpResponseImpl.java | 17 +- .../AuthenticationSessionManager.java | 2 - .../oidc/endpoints/login-status-iframe.html | 9 +- .../cookies/DefaultCookieProviderTest.java | 60 +++++++- .../base/login/resources/js/authChecker.js | 11 +- 17 files changed, 304 insertions(+), 139 deletions(-) create mode 100644 services/src/main/java/org/keycloak/cookie/CookiePathResolver.java create mode 100644 services/src/main/java/org/keycloak/cookie/CookieSecureResolver.java diff --git a/common/src/main/java/org/keycloak/common/util/ServerCookie.java b/common/src/main/java/org/keycloak/common/util/ServerCookie.java index ee79f654fa..af410cbad7 100755 --- a/common/src/main/java/org/keycloak/common/util/ServerCookie.java +++ b/common/src/main/java/org/keycloak/common/util/ServerCookie.java @@ -27,7 +27,11 @@ import java.util.TimeZone; /** * Server-side cookie representation. borrowed from Tomcat. + * + * @deprecated Should not be used on the Keycloak server-side, or in extensions. Will be removed when no longer used by + * adapters */ +@Deprecated public class ServerCookie implements Serializable { private static final String tspecials = ",; "; private static final String tspecials2 = "()<>@,;:\\\"/[]?={} \t"; diff --git a/docs/documentation/upgrading/topics/keycloak/changes-24_0_0.adoc b/docs/documentation/upgrading/topics/keycloak/changes-24_0_0.adoc index f2cefe4347..acd47d512c 100644 --- a/docs/documentation/upgrading/topics/keycloak/changes-24_0_0.adoc +++ b/docs/documentation/upgrading/topics/keycloak/changes-24_0_0.adoc @@ -306,9 +306,11 @@ and `org.keycloak:keycloak-model-legacy` module was deprecated and will be remov As part of refactoring cookie handling in Keycloak there are some changes to how cookies are set: * All cookies will now have the secure attribute set if the request is through a secure context -* KEYCLOAK_LOCALE and WELCOME_STATE_CHECKER cookies now set SameSite=Strict +* `WELCOME_STATE_CHECKER` cookies now set `SameSite=Strict` For custom extensions there may be some changes needed: -* LocaleSelectorProvider.KEYCLOAK_LOCALE is deprecated as cookies are now managed through the CookieProvider -* HttpResponse.setWriteCookiesOnTransactionComplete has been removed +* `LocaleSelectorProvider.KEYCLOAK_LOCALE` is deprecated as cookies are now managed through the CookieProvider +* `HttpResponse.setWriteCookiesOnTransactionComplete` has been removed +* `HttpCookie` is deprecated, please use `NewCookie.Builder` instead +* `ServerCookie` is deprecated, please use `NewCookie.Builder` instead diff --git a/pom.xml b/pom.xml index 4c04945257..a820ee2f3e 100644 --- a/pom.xml +++ b/pom.xml @@ -112,7 +112,7 @@ 2.0.0.Final 1.2.17 4.7.7.Final - 6.2.4.Final + 6.2.7.Final ${resteasy.version} 20220608.1 2.0.6 diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusHttpResponse.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusHttpResponse.java index d469f9593b..7216f39922 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusHttpResponse.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/integration/resteasy/QuarkusHttpResponse.java @@ -18,9 +18,10 @@ package org.keycloak.quarkus.runtime.integration.resteasy; import jakarta.ws.rs.core.HttpHeaders; +import jakarta.ws.rs.core.NewCookie; +import jakarta.ws.rs.ext.RuntimeDelegate; import org.jboss.resteasy.reactive.server.core.ResteasyReactiveRequestContext; import org.jboss.resteasy.reactive.server.vertx.VertxResteasyReactiveRequestContext; -import org.keycloak.http.HttpCookie; import org.keycloak.http.HttpResponse; import java.util.HashSet; @@ -28,9 +29,11 @@ import java.util.Set; public final class QuarkusHttpResponse implements HttpResponse { + private static final RuntimeDelegate.HeaderDelegate NEW_COOKIE_HEADER_DELEGATE = RuntimeDelegate.getInstance().createHeaderDelegate(NewCookie.class); + private final ResteasyReactiveRequestContext requestContext; - private Set cookies; + private Set newCookies; public QuarkusHttpResponse(ResteasyReactiveRequestContext requestContext) { this.requestContext = requestContext; @@ -58,18 +61,18 @@ public final class QuarkusHttpResponse implements HttpResponse { } @Override - public void setCookieIfAbsent(HttpCookie cookie) { - if (cookie == null) { + public void setCookieIfAbsent(NewCookie newCookie) { + if (newCookie == null) { throw new IllegalArgumentException("Cookie is null"); } - if (cookies == null) { - cookies = new HashSet<>(); + if (newCookies == null) { + newCookies = new HashSet<>(); } - if (cookies.add(cookie)) { - addHeader(HttpHeaders.SET_COOKIE, cookie.toHeaderValue()); + if (newCookies.add(newCookie)) { + String headerValue = NEW_COOKIE_HEADER_DELEGATE.toString(newCookie); + addHeader(HttpHeaders.SET_COOKIE, headerValue); } } - } diff --git a/server-spi-private/src/main/java/org/keycloak/cookie/CookieScope.java b/server-spi-private/src/main/java/org/keycloak/cookie/CookieScope.java index 79ce56fb3b..fa8f16853c 100644 --- a/server-spi-private/src/main/java/org/keycloak/cookie/CookieScope.java +++ b/server-spi-private/src/main/java/org/keycloak/cookie/CookieScope.java @@ -1,17 +1,17 @@ package org.keycloak.cookie; -import org.keycloak.common.util.ServerCookie; +import jakarta.ws.rs.core.NewCookie; public enum CookieScope { // Internal cookies are only available for direct requests to Keycloak - INTERNAL(ServerCookie.SameSiteAttributeValue.STRICT, true), + INTERNAL(NewCookie.SameSite.STRICT, true), // Federation cookies are available after redirect from applications, and are also available in an iframe context // unless the browser blocks third-party cookies - FEDERATION(ServerCookie.SameSiteAttributeValue.NONE, true), + FEDERATION(NewCookie.SameSite.NONE, true), // Federation cookies that are also available from JavaScript - FEDERATION_JS(ServerCookie.SameSiteAttributeValue.NONE, false), + FEDERATION_JS(NewCookie.SameSite.NONE, false), // Legacy cookies do not set the SameSite attribute and will default to SameSite=Lax in modern browsers @Deprecated @@ -21,15 +21,15 @@ public enum CookieScope { @Deprecated LEGACY_JS(null, false); - private final ServerCookie.SameSiteAttributeValue sameSite; + private final NewCookie.SameSite sameSite; private final boolean httpOnly; - CookieScope(ServerCookie.SameSiteAttributeValue sameSite, boolean httpOnly) { + CookieScope(NewCookie.SameSite sameSite, boolean httpOnly) { this.sameSite = sameSite; this.httpOnly = httpOnly; } - public ServerCookie.SameSiteAttributeValue getSameSite() { + public NewCookie.SameSite getSameSite() { return sameSite; } diff --git a/server-spi/src/main/java/org/keycloak/http/HttpCookie.java b/server-spi/src/main/java/org/keycloak/http/HttpCookie.java index 4ee3b8c238..a142ab29ca 100644 --- a/server-spi/src/main/java/org/keycloak/http/HttpCookie.java +++ b/server-spi/src/main/java/org/keycloak/http/HttpCookie.java @@ -17,33 +17,36 @@ package org.keycloak.http; -import org.keycloak.common.util.ServerCookie; +import jakarta.ws.rs.core.NewCookie; +import jakarta.ws.rs.ext.RuntimeDelegate; import org.keycloak.common.util.ServerCookie.SameSiteAttributeValue; /** * An extension of {@link javax.ws.rs.core.Cookie} in order to support additional * fields and behavior. + * + * @deprecated This class will be removed in the future. Please use {@link jakarta.ws.rs.core.NewCookie.Builder} */ -public final class HttpCookie extends jakarta.ws.rs.core.Cookie { - - private final String comment; - private final int maxAge; - private final boolean secure; - private final boolean httpOnly; - private final SameSiteAttributeValue sameSite; +@Deprecated(since = "24.0.0", forRemoval = true) +public final class HttpCookie extends NewCookie { 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; + super(name, value, path, domain, version, comment, maxAge, null, secure, httpOnly, convertSameSite(sameSite)); + } + + private static SameSite convertSameSite(SameSiteAttributeValue sameSiteAttributeValue) { + if (sameSiteAttributeValue == null) { + return null; + } + switch (sameSiteAttributeValue) { + case NONE: return SameSite.NONE; + case LAX: return SameSite.LAX; + case STRICT: return SameSite.STRICT; + } + throw new IllegalArgumentException("Unknown SameSite value " + sameSiteAttributeValue); } public String toHeaderValue() { - StringBuilder cookieBuf = new StringBuilder(); - ServerCookie.appendCookieValue(cookieBuf, getVersion(), getName(), getValue(), getPath(), getDomain(), comment, maxAge, secure, httpOnly, sameSite); - return cookieBuf.toString(); + return RuntimeDelegate.getInstance().createHeaderDelegate(NewCookie.class).toString(this); } } diff --git a/server-spi/src/main/java/org/keycloak/http/HttpResponse.java b/server-spi/src/main/java/org/keycloak/http/HttpResponse.java index 85c0da03cf..359de398aa 100644 --- a/server-spi/src/main/java/org/keycloak/http/HttpResponse.java +++ b/server-spi/src/main/java/org/keycloak/http/HttpResponse.java @@ -17,6 +17,8 @@ package org.keycloak.http; +import jakarta.ws.rs.core.NewCookie; + /** *

Represents an out coming HTTP response. * @@ -56,6 +58,17 @@ public interface HttpResponse { * * @param cookie the cookie */ - void setCookieIfAbsent(HttpCookie cookie); + void setCookieIfAbsent(NewCookie cookie); + + /** + * Sets a new cookie only if not yet set. + * @deprecated This method will be removed in the future. Please use {@link jakarta.ws.rs.core.NewCookie.Builder} + * + * @param cookie the cookie + */ + @Deprecated(since = "24.0.0", forRemoval = true) + default void setCookieIfAbsent(HttpCookie cookie) { + setCookieIfAbsent((NewCookie) cookie); + } } diff --git a/services/src/main/java/org/keycloak/cookie/CookiePathResolver.java b/services/src/main/java/org/keycloak/cookie/CookiePathResolver.java new file mode 100644 index 0000000000..f161552fc2 --- /dev/null +++ b/services/src/main/java/org/keycloak/cookie/CookiePathResolver.java @@ -0,0 +1,34 @@ +package org.keycloak.cookie; + +import org.keycloak.models.KeycloakContext; +import org.keycloak.services.resources.RealmsResource; + +class CookiePathResolver { + + private final KeycloakContext context; + private String realmPath; + + private String requestPath; + + CookiePathResolver(KeycloakContext context) { + this.context = context; + } + + String resolvePath(CookieType cookieType) { + switch (cookieType.getPath()) { + case REALM: + if (realmPath == null) { + realmPath = RealmsResource.realmBaseUrl(context.getUri()).path("/").build(context.getRealm().getName()).getRawPath(); + } + return realmPath; + case REQUEST: + if (requestPath == null) { + requestPath = context.getUri().getRequestUri().getRawPath(); + } + return requestPath; + default: + throw new IllegalArgumentException("Unsupported enum value " + cookieType.getPath().name()); + } + } + +} diff --git a/services/src/main/java/org/keycloak/cookie/CookieSecureResolver.java b/services/src/main/java/org/keycloak/cookie/CookieSecureResolver.java new file mode 100644 index 0000000000..d0c732f95d --- /dev/null +++ b/services/src/main/java/org/keycloak/cookie/CookieSecureResolver.java @@ -0,0 +1,39 @@ +package org.keycloak.cookie; + +import jakarta.ws.rs.core.NewCookie; +import org.keycloak.models.KeycloakContext; +import org.keycloak.models.RealmModel; + +import java.net.URI; + +class CookieSecureResolver { + + private final KeycloakContext context; + private final boolean sameSiteLegacyEnabled; + + CookieSecureResolver(KeycloakContext context, boolean sameSiteLegacyEnabled) { + this.context = context; + this.sameSiteLegacyEnabled = sameSiteLegacyEnabled; + } + + boolean resolveSecure(NewCookie.SameSite sameSite) { + // Due to cookies with SameSite=none secure context is always required when same-site legacy cookies are disabled + if (!sameSiteLegacyEnabled) { + return true; + } else { + // SameSite=none requires secure context + if (NewCookie.SameSite.NONE.equals(sameSite)) { + return true; + } + + URI requestUri = context.getUri().getRequestUri(); + RealmModel realm = context.getRealm(); + if (realm != null && realm.getSslRequired().isRequired(requestUri.getHost())) { + return true; + } + + return false; + } + } + +} diff --git a/services/src/main/java/org/keycloak/cookie/DefaultCookieProvider.java b/services/src/main/java/org/keycloak/cookie/DefaultCookieProvider.java index e8fc33f12d..6ba907473a 100644 --- a/services/src/main/java/org/keycloak/cookie/DefaultCookieProvider.java +++ b/services/src/main/java/org/keycloak/cookie/DefaultCookieProvider.java @@ -1,31 +1,36 @@ package org.keycloak.cookie; import jakarta.ws.rs.core.Cookie; +import jakarta.ws.rs.core.NewCookie; import org.jboss.logging.Logger; -import org.keycloak.common.util.ServerCookie; -import org.keycloak.http.HttpCookie; import org.keycloak.models.KeycloakContext; -import org.keycloak.models.RealmModel; -import org.keycloak.services.resources.RealmsResource; -import org.keycloak.urls.UrlType; -import java.net.URI; import java.util.Map; - public class DefaultCookieProvider implements CookieProvider { private static final Logger logger = Logger.getLogger(DefaultCookieProvider.class); private final KeycloakContext context; + private CookiePathResolver pathResolver; + + private CookieSecureResolver secureResolver; + private final Map cookies; - private final boolean legacyCookiesEnabled; + private final boolean sameSiteLegacyEnabled; - public DefaultCookieProvider(KeycloakContext context, boolean legacyCookiesEnabled) { + public DefaultCookieProvider(KeycloakContext context, boolean sameSiteLegacyEnabled) { this.context = context; this.cookies = context.getRequestHeaders().getCookies(); - this.legacyCookiesEnabled = legacyCookiesEnabled; + this.pathResolver = new CookiePathResolver(context); + this.secureResolver = new CookieSecureResolver(context, sameSiteLegacyEnabled); + this.sameSiteLegacyEnabled = sameSiteLegacyEnabled; + + if (logger.isTraceEnabled()) { + String cookieNames = String.join(", ", this.cookies.keySet()); + logger.tracef("Path: %s, cookies: %s", context.getUri().getRequestUri().getRawPath(), cookieNames); + } } @Override @@ -40,59 +45,90 @@ public class DefaultCookieProvider implements CookieProvider { @Override public void set(CookieType cookieType, String value, int maxAge) { String name = cookieType.getName(); - ServerCookie.SameSiteAttributeValue sameSite = cookieType.getScope().getSameSite(); - boolean secure = resolveSecure(sameSite); - String path = resolvePath(cookieType); + NewCookie.SameSite sameSite = cookieType.getScope().getSameSite(); + boolean secure = secureResolver.resolveSecure(sameSite); + String path = pathResolver.resolvePath(cookieType); boolean httpOnly = cookieType.getScope().isHttpOnly(); - HttpCookie newCookie = new HttpCookie(1, name, value, path, null, null, maxAge, secure, httpOnly, sameSite); + NewCookie newCookie = new NewCookie.Builder(name) + .version(1) + .value(value) + .path(path) + .maxAge(maxAge) + .secure(secure) + .httpOnly(httpOnly) + .sameSite(sameSite) + .build(); + context.getHttpResponse().setCookieIfAbsent(newCookie); logger.tracef("Setting cookie: name: %s, path: %s, same-site: %s, secure: %s, http-only: %s, max-age: %d", name, path, sameSite, secure, httpOnly, maxAge); - if (legacyCookiesEnabled && cookieType.supportsSameSiteLegacy()) { - if (ServerCookie.SameSiteAttributeValue.NONE.equals(sameSite)) { - secure = resolveSecure(null); - String legacyName = cookieType.getSameSiteLegacyName(); - HttpCookie legacyCookie = new HttpCookie(1, legacyName, value, path, null, null, maxAge, secure, httpOnly, null); - context.getHttpResponse().setCookieIfAbsent(legacyCookie); + setSameSiteLegacy(cookieType, value, maxAge); + } - logger.tracef("Setting legacy cookie: name: %s, path: %s, same-site: %s, secure: %s, http-only: %s, max-age: %d", legacyName, path, sameSite, secure, httpOnly, maxAge); - } - } else { - expireLegacy(cookieType); + private void setSameSiteLegacy(CookieType cookieType, String value, int maxAge) { + if (sameSiteLegacyEnabled && cookieType.supportsSameSiteLegacy()) { + String legacyName = cookieType.getSameSiteLegacyName(); + boolean legacySecure = secureResolver.resolveSecure(null); + String path = pathResolver.resolvePath(cookieType); + boolean httpOnly = cookieType.getScope().isHttpOnly(); + + NewCookie legacyCookie = new NewCookie.Builder(legacyName) + .version(1) + .value(value) + .maxAge(maxAge) + .path(path) + .secure(legacySecure) + .httpOnly(httpOnly) + .build(); + context.getHttpResponse().setCookieIfAbsent(legacyCookie); + + logger.tracef("Setting legacy cookie: name: %s, path: %s, same-site: %s, secure: %s, http-only: %s, max-age: %d", legacyName, path, null, legacySecure, httpOnly, maxAge); + } else if (cookieType.supportsSameSiteLegacy()) { + expireSameSiteLegacy(cookieType); } } @Override public String get(CookieType cookieType) { Cookie cookie = cookies.get(cookieType.getName()); - if (cookie == null && cookieType.supportsSameSiteLegacy()) { - cookie = cookies.get(cookieType.getSameSiteLegacyName()); + if (cookie == null) { + cookie = getSameSiteLegacyCookie(cookieType); } return cookie != null ? cookie.getValue() : null; } - @Override - public void expire(CookieType cookieType) { - Cookie cookie = cookies.get(cookieType.getName()); - expire(cookie, cookieType); - - expireLegacy(cookieType); - } - - private void expireLegacy(CookieType cookieType) { + private Cookie getSameSiteLegacyCookie(CookieType cookieType) { if (cookieType.supportsSameSiteLegacy()) { - String legacyName = cookieType.getSameSiteLegacyName(); - Cookie legacyCookie = cookies.get(legacyName); - expire(legacyCookie, cookieType); + return cookies.get(cookieType.getSameSiteLegacyName()); + } else { + return null; } } - private void expire(Cookie cookie, CookieType cookieType) { + @Override + public void expire(CookieType cookieType) { + expire(cookieType.getName(), cookieType); + expireSameSiteLegacy(cookieType); + } + + private void expireSameSiteLegacy(CookieType cookieType) { + if (cookieType.supportsSameSiteLegacy()) { + expire(cookieType.getSameSiteLegacyName(), cookieType); + } + } + + private void expire(String cookieName, CookieType cookieType) { + Cookie cookie = cookies.get(cookieName); if (cookie != null) { - String path = resolvePath(cookieType); - HttpCookie newCookie = new HttpCookie(1, cookie.getName(), "", path, null, null, CookieMaxAge.EXPIRED, false, false, null); + String path = pathResolver.resolvePath(cookieType); + NewCookie newCookie = new NewCookie.Builder(cookieName) + .version(1) + .path(path) + .maxAge(CookieMaxAge.EXPIRED) + .build(); + context.getHttpResponse().setCookieIfAbsent(newCookie); logger.tracef("Expiring cookie: name: %s, path: %s", cookie.getName(), path); @@ -103,31 +139,4 @@ public class DefaultCookieProvider implements CookieProvider { public void close() { } - private String resolvePath(CookieType cookieType) { - switch (cookieType.getPath()) { - case REALM: - return RealmsResource.realmBaseUrl(context.getUri()).path("/").build(context.getRealm().getName()).getRawPath(); - case REQUEST: - return context.getUri().getRequestUri().getRawPath(); - default: - throw new IllegalArgumentException("Unsupported enum value " + cookieType.getPath().name()); - } - } - - private boolean resolveSecure(ServerCookie.SameSiteAttributeValue sameSite) { - URI requestUri = context.getUri().getRequestUri(); - - // SameSite=none requires secure context - if (ServerCookie.SameSiteAttributeValue.NONE.equals(sameSite)) { - return true; - } - - RealmModel realm = context.getRealm(); - if (realm != null && realm.getSslRequired().isRequired(requestUri.getHost())) { - return true; - } - - return false; - } - } diff --git a/services/src/main/java/org/keycloak/cookie/DefaultCookieProviderFactory.java b/services/src/main/java/org/keycloak/cookie/DefaultCookieProviderFactory.java index 31b3f1e42e..e7ec2c10c8 100644 --- a/services/src/main/java/org/keycloak/cookie/DefaultCookieProviderFactory.java +++ b/services/src/main/java/org/keycloak/cookie/DefaultCookieProviderFactory.java @@ -3,19 +3,24 @@ package org.keycloak.cookie; import org.keycloak.Config; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; +import org.keycloak.provider.ProviderConfigProperty; +import org.keycloak.provider.ProviderConfigurationBuilder; + +import java.util.List; public class DefaultCookieProviderFactory implements CookieProviderFactory { - private boolean legacyCookies; + private static final String SAME_SITE_LEGACY_KEY = "sameSiteLegacy"; + private boolean sameSiteLegacyEnabled; @Override public CookieProvider create(KeycloakSession session) { - return new DefaultCookieProvider(session.getContext(), legacyCookies); + return new DefaultCookieProvider(session.getContext(), sameSiteLegacyEnabled); } @Override public void init(Config.Scope config) { - legacyCookies = config.getBoolean("legacyCookies", true); + sameSiteLegacyEnabled = config.getBoolean(SAME_SITE_LEGACY_KEY, true); } @Override @@ -31,4 +36,15 @@ public class DefaultCookieProviderFactory implements CookieProviderFactory { return "default"; } + @Override + public List getConfigMetadata() { + return ProviderConfigurationBuilder.create() + .property() + .name(SAME_SITE_LEGACY_KEY) + .type("boolean") + .helpText("Adds legacy cookies without SameSite parameter") + .defaultValue(true) + .add() + .build(); + } } diff --git a/services/src/main/java/org/keycloak/forms/login/freemarker/AuthenticationStateCookie.java b/services/src/main/java/org/keycloak/forms/login/freemarker/AuthenticationStateCookie.java index 1cfce9d05d..acaaa6aecd 100644 --- a/services/src/main/java/org/keycloak/forms/login/freemarker/AuthenticationStateCookie.java +++ b/services/src/main/java/org/keycloak/forms/login/freemarker/AuthenticationStateCookie.java @@ -24,6 +24,7 @@ import java.util.Set; import com.fasterxml.jackson.annotation.JsonProperty; import org.jboss.logging.Logger; +import org.keycloak.common.util.Encode; import org.keycloak.cookie.CookieProvider; import org.keycloak.cookie.CookieType; import org.keycloak.models.KeycloakSession; @@ -69,7 +70,7 @@ public class AuthenticationStateCookie { cookie.setRemainingTabs(rootAuthSession.getAuthenticationSessions().keySet()); try { - String encoded = JsonSerialization.writeValueAsString(cookie); + String encoded = Encode.urlEncode(JsonSerialization.writeValueAsString(cookie)); session.getProvider(CookieProvider.class).set(CookieType.AUTH_STATE, encoded, cookieMaxAge); } catch (IOException ioe) { throw new IllegalStateException("Exception thrown when encoding cookie", ioe); diff --git a/services/src/main/java/org/keycloak/services/HttpResponseImpl.java b/services/src/main/java/org/keycloak/services/HttpResponseImpl.java index f391a2c512..a7d6f00f01 100644 --- a/services/src/main/java/org/keycloak/services/HttpResponseImpl.java +++ b/services/src/main/java/org/keycloak/services/HttpResponseImpl.java @@ -17,8 +17,7 @@ package org.keycloak.services; -import jakarta.ws.rs.core.HttpHeaders; -import org.keycloak.http.HttpCookie; +import jakarta.ws.rs.core.NewCookie; import org.keycloak.http.HttpResponse; import java.util.HashSet; @@ -27,7 +26,7 @@ import java.util.Set; public class HttpResponseImpl implements HttpResponse { private final org.jboss.resteasy.spi.HttpResponse delegate; - private Set cookies; + private Set newCookies; public HttpResponseImpl(org.jboss.resteasy.spi.HttpResponse delegate) { this.delegate = delegate; @@ -54,17 +53,17 @@ public class HttpResponseImpl implements HttpResponse { } @Override - public void setCookieIfAbsent(HttpCookie cookie) { - if (cookie == null) { + public void setCookieIfAbsent(NewCookie newCookie) { + if (newCookie == null) { throw new IllegalArgumentException("Cookie is null"); } - if (cookies == null) { - cookies = new HashSet<>(); + if (newCookies == null) { + newCookies = new HashSet<>(); } - if (cookies.add(cookie)) { - addHeader(HttpHeaders.SET_COOKIE, cookie.toHeaderValue()); + if (newCookies.add(newCookie)) { + delegate.addNewCookie(newCookie); } } diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java index fdd4a13032..241501028a 100644 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationSessionManager.java @@ -17,9 +17,7 @@ package org.keycloak.services.managers; -import jakarta.ws.rs.core.UriInfo; import org.jboss.logging.Logger; -import org.keycloak.common.util.ServerCookie.SameSiteAttributeValue; import org.keycloak.common.util.Time; import org.keycloak.cookie.CookieProvider; import org.keycloak.cookie.CookieType; diff --git a/services/src/main/resources/org/keycloak/protocol/oidc/endpoints/login-status-iframe.html b/services/src/main/resources/org/keycloak/protocol/oidc/endpoints/login-status-iframe.html index 649c9f11f9..ee640cffa0 100755 --- a/services/src/main/resources/org/keycloak/protocol/oidc/endpoints/login-status-iframe.html +++ b/services/src/main/resources/org/keycloak/protocol/oidc/endpoints/login-status-iframe.html @@ -109,14 +109,13 @@ } function getCookieByName(name) { - const cookies = new Map(); - for (const cookie of document.cookie.split(";")) { const [key, value] = cookie.split("=").map((value) => value.trim()); - cookies.set(key, value); + if (key === name) { + return value.startsWith('"') && value.endsWith('"') ? value.slice(1, -1) : value; + } } - - return cookies.get(name) ?? null; + return null; } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/DefaultCookieProviderTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/DefaultCookieProviderTest.java index b0af11fdfb..ea4e98a5ba 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/DefaultCookieProviderTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/cookies/DefaultCookieProviderTest.java @@ -19,7 +19,6 @@ import org.keycloak.testsuite.client.KeycloakTestingClient; import java.io.IOException; import java.util.List; import java.util.Map; -import java.util.Optional; public class DefaultCookieProviderTest extends AbstractKeycloakTest { @@ -64,6 +63,32 @@ public class DefaultCookieProviderTest extends AbstractKeycloakTest { assertCookie(response, "WELCOME_STATE_CHECKER", "my-welcome-csrf", "/auth/realms/master/testing/run-on-server", 300, false, true, "Strict", false); } + @Test + public void testSessionCookieValue() { + // Set cookie value with '/' that results in cookies value being quoted + final String sessionValue = "my-realm/5256327f-049f-4d01-acb9-68c7936bdeb3/c6cd1f10-40ab-44c0-b77b-6028350d8564"; + Response response = testing.server("master").runWithResponse(session -> { + CookieProvider cookies = session.getProvider(CookieProvider.class); + cookies.set(CookieType.SESSION, sessionValue, 444); + }); + + // Cookie values from response.getCookies() removes quotes + Assert.assertEquals(sessionValue, response.getCookies().get(CookieType.SESSION.getName()).getValue()); + + // Cookie values from the Set-Cookie header includes quotes + String setHeader = getSetHeader(response, CookieType.SESSION.getName()); + Assert.assertTrue(setHeader.startsWith(CookieType.SESSION.getName() + "=\"" + sessionValue + "\";")); + + // Send Cookie header with quoted value for cookie + filter.setHeader("Cookie", "KEYCLOAK_SESSION=\"" + sessionValue + "\";"); + + // Check cookie value matches original value without quotes + testing.server().run(session -> { + String cookieValue = session.getProvider(CookieProvider.class).get(CookieType.SESSION); + Assert.assertEquals(sessionValue, cookieValue); + }); + } + @Test public void testExpire() { filter.setHeader("Cookie", "AUTH_SESSION_ID=new;KC_RESTART=new;"); @@ -106,7 +131,7 @@ public class DefaultCookieProviderTest extends AbstractKeycloakTest { @Test public void testSameSiteLegacyExpire() { - filter.setHeader("Cookie", "AUTH_SESSION_ID=new;AUTH_SESSION_ID_LEGACY=legacy;KC_RESTART_LEGACY=ignore;KEYCLOAK_LOCALE=foobar"); + filter.setHeader("Cookie", "AUTH_SESSION_ID=new; AUTH_SESSION_ID_LEGACY=legacy; KC_RESTART_LEGACY=ignore; KEYCLOAK_LOCALE=foobar"); Response response = testing.server().runWithResponse(session -> { session.getProvider(CookieProvider.class).expire(CookieType.AUTH_SESSION_ID); @@ -115,10 +140,27 @@ public class DefaultCookieProviderTest extends AbstractKeycloakTest { Map cookies = response.getCookies(); Assert.assertEquals(2, cookies.size()); - assertCookie(response, "AUTH_SESSION_ID", "", "/auth/realms/master/", 0, false, false, null, true); + assertCookie(response, "AUTH_SESSION_ID", "", "/auth/realms/master/", 0, false, false, null, false); + assertCookie(response, "AUTH_SESSION_ID_LEGACY", "", "/auth/realms/master/", 0, false, false, null, false); + } + + @Test + public void testCustomCookie() { + Response response = testing.server().runWithResponse(session -> { + NewCookie newCookie = new NewCookie.Builder("mycookie") + .maxAge(1232) + .value("myvalue") + .path(session.getContext().getUri().getRequestUri().getRawPath()) + .build(); + session.getContext().getHttpResponse().setCookieIfAbsent(newCookie); + }); + + Map cookies = response.getCookies(); + Assert.assertEquals(1, cookies.size()); + assertCookie(response, "mycookie", "myvalue", "/auth/realms/master/testing/run-on-server", 1232, false, false, null, false); } - private void assertCookie(Response response, String name, String value, String path, int maxAge, boolean secure, boolean httpOnly, String sameSite, boolean hasLegacy) { + private void assertCookie(Response response, String name, String value, String path, int maxAge, boolean secure, boolean httpOnly, String sameSite, boolean verifyLegacy) { Map cookies = response.getCookies(); NewCookie cookie = cookies.get(name); Assert.assertNotNull(cookie); @@ -128,18 +170,22 @@ public class DefaultCookieProviderTest extends AbstractKeycloakTest { Assert.assertEquals(secure || "None".equals(sameSite), cookie.isSecure()); Assert.assertEquals(httpOnly, cookie.isHttpOnly()); - String setHeader = (String) response.getHeaders().get("Set-Cookie").stream().filter(v -> ((String) v).startsWith(name)).findFirst().get(); + String setHeader = getSetHeader(response, name); if (sameSite == null) { Assert.assertFalse(setHeader.contains("SameSite")); } else { - Assert.assertTrue(setHeader.contains("SameSite=" + sameSite)); + Assert.assertTrue("Expected SameSite=" + sameSite + ", header was: " + setHeader, setHeader.contains("SameSite=" + sameSite)); } - if (hasLegacy) { + if (verifyLegacy) { assertCookie(response, name + "_LEGACY", value, path, maxAge, secure, httpOnly, null, false); } } + private String getSetHeader(Response response, String name) { + return (String) response.getHeaders().get("Set-Cookie").stream().filter(v -> ((String) v).startsWith(name)).findFirst().get(); + } + private KeycloakTestingClient createTestingClient(String serverUrl) { ResteasyClientBuilder restEasyClientBuilder = KeycloakTestingClient.getRestEasyClientBuilder(serverUrl); ResteasyClient resteasyClient = restEasyClientBuilder.build(); diff --git a/themes/src/main/resources/theme/base/login/resources/js/authChecker.js b/themes/src/main/resources/theme/base/login/resources/js/authChecker.js index e7273249bb..18fd8f9520 100644 --- a/themes/src/main/resources/theme/base/login/resources/js/authChecker.js +++ b/themes/src/main/resources/theme/base/login/resources/js/authChecker.js @@ -32,7 +32,7 @@ function checkAuthState(authSessionId, tabId, loginRestartUrl) { // Attempt to parse the auth state as JSON. let authState; try { - authState = JSON.parse(authStateRaw); + authState = JSON.parse(decodeURIComponent(authStateRaw)); } catch (error) { // The auth state is not valid JSON, exit. return; @@ -64,12 +64,11 @@ function getAuthState() { } function getCookieByName(name) { - const cookies = new Map(); - for (const cookie of document.cookie.split(";")) { const [key, value] = cookie.split("=").map((value) => value.trim()); - cookies.set(key, value); + if (key === name) { + return value.startsWith('"') && value.endsWith('"') ? value.slice(1, -1) : value; + } } - - return cookies.get(name) ?? null; + return null; }