From 64b5f42c4a9813cb94b33ae606181fb841b68a28 Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Wed, 31 Jan 2024 19:33:56 +0100 Subject: [PATCH] Revert new behaviour around setting secure flag for cookies (#26650) Closes #26649 Signed-off-by: stianst --- .../cookie/DefaultCookieProvider.java | 14 ++----------- .../cookies/DefaultCookieProviderTest.java | 20 +++++++++---------- 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/services/src/main/java/org/keycloak/cookie/DefaultCookieProvider.java b/services/src/main/java/org/keycloak/cookie/DefaultCookieProvider.java index e4e18a8283..e8fc33f12d 100644 --- a/services/src/main/java/org/keycloak/cookie/DefaultCookieProvider.java +++ b/services/src/main/java/org/keycloak/cookie/DefaultCookieProvider.java @@ -48,7 +48,7 @@ public class DefaultCookieProvider implements CookieProvider { HttpCookie newCookie = new HttpCookie(1, name, value, path, null, null, maxAge, secure, httpOnly, sameSite); context.getHttpResponse().setCookieIfAbsent(newCookie); - logger.tracef("Setting cookie: name: %s, path: %s, same-site: %s, http-only: %s, max-age: %d", name, path, sameSite, httpOnly, maxAge); + 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)) { @@ -57,7 +57,7 @@ public class DefaultCookieProvider implements CookieProvider { HttpCookie legacyCookie = new HttpCookie(1, legacyName, value, path, null, null, maxAge, secure, httpOnly, null); context.getHttpResponse().setCookieIfAbsent(legacyCookie); - logger.tracef("Setting legacy cookie: name: %s, path: %s, same-site: %s, http-only: %s, max-age: %d", legacyName, path, sameSite, httpOnly, 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); @@ -127,16 +127,6 @@ public class DefaultCookieProvider implements CookieProvider { return true; } - if ("https".equals(requestUri.getScheme())) { - return true; - } - - // Browsers consider 127.0.0.1, localhost and *.localhost as secure contexts - String frontendHostname = context.getUri(UrlType.FRONTEND).getRequestUri().getHost(); - if (frontendHostname.equals("127.0.0.1") || frontendHostname.equals("localhost") || frontendHostname.endsWith(".localhost")) { - return true; - } - return false; } 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 238b269f81..b0af11fdfb 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 @@ -53,15 +53,15 @@ public class DefaultCookieProviderTest extends AbstractKeycloakTest { cookies.set(CookieType.WELCOME_CSRF, "my-welcome-csrf"); }); Assert.assertEquals(12, response.getCookies().size()); - assertCookie(response, "AUTH_SESSION_ID", "my-auth-session-id", "/auth/realms/master/", -1, true, true, "None", true); - assertCookie(response, "KC_AUTH_STATE", "my-auth-state", "/auth/realms/master/", 111, true, false, null, false); - assertCookie(response, "KC_RESTART", "my-auth-restart", "/auth/realms/master/", -1, true, true, null, false); - assertCookie(response, "KC_STATE_CHECKER", "my-auth-detached", "/auth/realms/master/", 222, true, true, null, false); - assertCookie(response, "KEYCLOAK_IDENTITY", "my-identity", "/auth/realms/master/", 333, true, true, "None", true); - assertCookie(response, "KEYCLOAK_LOCALE", "my-locale", "/auth/realms/master/", -1, true, true, null, false); - assertCookie(response, "KEYCLOAK_REMEMBER_ME", "my-username", "/auth/realms/master/", 31536000, true, true, null, false); - assertCookie(response, "KEYCLOAK_SESSION", "my-session", "/auth/realms/master/", 444, true, false, "None", true); - assertCookie(response, "WELCOME_STATE_CHECKER", "my-welcome-csrf", "/auth/realms/master/testing/run-on-server", 300, true, true, "Strict", false); + assertCookie(response, "AUTH_SESSION_ID", "my-auth-session-id", "/auth/realms/master/", -1, false, true, "None", true); + assertCookie(response, "KC_AUTH_STATE", "my-auth-state", "/auth/realms/master/", 111, false, false, null, false); + assertCookie(response, "KC_RESTART", "my-auth-restart", "/auth/realms/master/", -1, false, true, null, false); + assertCookie(response, "KC_STATE_CHECKER", "my-auth-detached", "/auth/realms/master/", 222, false, true, null, false); + assertCookie(response, "KEYCLOAK_IDENTITY", "my-identity", "/auth/realms/master/", 333, false, true, "None", true); + assertCookie(response, "KEYCLOAK_LOCALE", "my-locale", "/auth/realms/master/", -1, false, true, null, false); + assertCookie(response, "KEYCLOAK_REMEMBER_ME", "my-username", "/auth/realms/master/", 31536000, false, true, null, false); + assertCookie(response, "KEYCLOAK_SESSION", "my-session", "/auth/realms/master/", 444, false, false, "None", true); + assertCookie(response, "WELCOME_STATE_CHECKER", "my-welcome-csrf", "/auth/realms/master/testing/run-on-server", 300, false, true, "Strict", false); } @Test @@ -125,7 +125,7 @@ public class DefaultCookieProviderTest extends AbstractKeycloakTest { Assert.assertEquals(value, cookie.getValue()); Assert.assertEquals(path, cookie.getPath()); Assert.assertEquals(maxAge, cookie.getMaxAge()); - Assert.assertEquals(secure, cookie.isSecure()); + 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();