From 709c6b5a473000635f4cbe4db1b0ae120a1cf87d Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 29 Mar 2023 10:23:28 +0200 Subject: [PATCH] Regressions in redirect URL verification when redirect_uri has encoded path or default port closes #16851 closes #16587 --- .../common/util/KeycloakUriBuilder.java | 17 +++++- .../common/util/KeycloakUriBuilderTest.java | 21 +++++++ .../oidc/endpoints/TokenEndpoint.java | 1 + .../protocol/oidc/utils/RedirectUtils.java | 60 ++++++++++++------- .../testsuite/oauth/OAuthRedirectUriTest.java | 38 +++++++++++- 5 files changed, 112 insertions(+), 25 deletions(-) diff --git a/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java b/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java index b85e257836..1a56f01634 100755 --- a/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java +++ b/common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java @@ -37,6 +37,8 @@ public class KeycloakUriBuilder { private String scheme; private int port = -1; + private boolean preserveDefaultPort = false; + private String userInfo; private String path; private String query; @@ -290,6 +292,19 @@ public class KeycloakUriBuilder { return this; } + /** + * When this is called, then the port will be preserved in the build URL even if it is default port for the protocol (http, https) + * + * For example: + * - KeycloakUriBuilder.fromUri("https://localhost:443/path").buildAsString() will return "https://localhost/path" (port not preserved) + * - KeycloakUriBuilder.fromUri("https://localhost:443/path").preserveDefaultPort().buildAsString() will return "https://localhost:443/path" (port is preserved even if default port) + * - KeycloakUriBuilder.fromUri("https://localhost/path").preserveDefaultPort().buildAsString() will return "https://localhost/path" (port not included even if "preserveDefaultPort" as it was not in the original URL) + */ + public KeycloakUriBuilder preserveDefaultPort() { + this.preserveDefaultPort = true; + return this; + } + protected static String paths(boolean encode, String basePath, String... segments) { String path = basePath; if (path == null) path = ""; @@ -429,7 +444,7 @@ public class KeycloakUriBuilder { if ("".equals(host)) throw new RuntimeException("empty host name"); replaceParameter(paramMap, fromEncodedMap, isTemplate, host, buffer, encodeSlash); } - if (port != -1 && !(("http".equals(scheme) && port == 80) || ("https".equals(scheme) && port == 443))) { + if (port != -1 && (preserveDefaultPort || !(("http".equals(scheme) && port == 80) || ("https".equals(scheme) && port == 443)))) { buffer.append(":").append(Integer.toString(port)); } } else if (authority != null) { diff --git a/common/src/test/java/org/keycloak/common/util/KeycloakUriBuilderTest.java b/common/src/test/java/org/keycloak/common/util/KeycloakUriBuilderTest.java index c4cdb8e260..414478fbaf 100644 --- a/common/src/test/java/org/keycloak/common/util/KeycloakUriBuilderTest.java +++ b/common/src/test/java/org/keycloak/common/util/KeycloakUriBuilderTest.java @@ -51,4 +51,25 @@ public class KeycloakUriBuilderTest { KeycloakUriBuilder.fromUri("https://localhost:8443/path?attr1={value}") .buildFromMap(Collections.singletonMap("value", "value1")).toString()); } + + @Test + public void testPort() { + Assert.assertEquals("https://localhost:8443/path", KeycloakUriBuilder.fromUri("https://localhost:8443/path").buildAsString()); + Assert.assertEquals("https://localhost:8443/path", KeycloakUriBuilder.fromUri("https://localhost:8443/path").preserveDefaultPort().buildAsString()); + + Assert.assertEquals("https://localhost/path", KeycloakUriBuilder.fromUri("https://localhost:443/path").buildAsString()); + Assert.assertEquals("https://localhost:443/path", KeycloakUriBuilder.fromUri("https://localhost:443/path").preserveDefaultPort().buildAsString()); + + Assert.assertEquals("http://localhost/path", KeycloakUriBuilder.fromUri("http://localhost:80/path").buildAsString()); + Assert.assertEquals("http://localhost:80/path", KeycloakUriBuilder.fromUri("http://localhost:80/path").preserveDefaultPort().buildAsString()); + + // Port always preserved (even if preserverPort not specified) due the port 80 doesn't match "https" scheme + Assert.assertEquals("https://localhost:80/path", KeycloakUriBuilder.fromUri("https://localhost:80/path").buildAsString()); + + // Port not in the build URL when it was not specified in the original URL (even if preserverPort() is true) + Assert.assertEquals("http://localhost/path", KeycloakUriBuilder.fromUri("http://localhost/path").buildAsString()); + Assert.assertEquals("http://localhost/path", KeycloakUriBuilder.fromUri("http://localhost/path").preserveDefaultPort().buildAsString()); + Assert.assertEquals("https://localhost/path", KeycloakUriBuilder.fromUri("https://localhost/path").buildAsString()); + Assert.assertEquals("https://localhost/path", KeycloakUriBuilder.fromUri("https://localhost/path").preserveDefaultPort().buildAsString()); + } } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java index 4eef558787..6c479b0de7 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java @@ -386,6 +386,7 @@ public class TokenEndpoint { if (redirectUri != null && !redirectUri.equals(redirectUriParam)) { event.error(Errors.INVALID_CODE); + logger.tracef("Parameter 'redirect_uri' did not match originally saved redirect URI used in initial OIDC request. Saved redirectUri: %s, redirectUri parameter: %s", redirectUri, redirectUriParam); throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_GRANT, "Incorrect redirect_uri", Response.Status.BAD_REQUEST); } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java index 9cb314b2e4..1cdcbbb787 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java @@ -93,20 +93,6 @@ public class RedirectUtils { KeycloakUriInfo uriInfo = session.getContext().getUri(); RealmModel realm = session.getContext().getRealm(); - redirectUri = decodeRedirectUri(redirectUri); - if (redirectUri != null) { - try { - URI uri = URI.create(redirectUri); - redirectUri = uri.normalize().toString(); - } catch (IllegalArgumentException cause) { - logger.debug("Invalid redirect uri", cause); - return null; - } catch (Exception cause) { - logger.debug("Unexpected error when parsing redirect uri", cause); - return null; - } - } - if (redirectUri == null) { if (!requireRedirectUri) { redirectUri = getSingleValidRedirectUri(validRedirects); @@ -120,12 +106,15 @@ public class RedirectUtils { logger.debug("No Redirect URIs supplied"); redirectUri = null; } else { - redirectUri = lowerCaseHostname(redirectUri); + // Make the validations against fully decoded and normalized redirect-url. This also allows wildcards (case when client configured "Valid redirect-urls" contain wildcards) + String decodedRedirectUri = decodeRedirectUri(redirectUri); + decodedRedirectUri = getNormalizedRedirectUri(decodedRedirectUri); + if (decodedRedirectUri == null) return null; - String r = redirectUri; + String r = decodedRedirectUri; Set resolveValidRedirects = resolveValidRedirects(session, rootUrl, validRedirects); - boolean valid = matchesRedirects(resolveValidRedirects, r); + boolean valid = matchesRedirects(resolveValidRedirects, r, true); if (!valid && (r.startsWith(Constants.INSTALLED_APP_URL) || r.startsWith(Constants.INSTALLED_APP_LOOPBACK)) && r.indexOf(':', Constants.INSTALLED_APP_URL.length()) >= 0) { int i = r.indexOf(':', Constants.INSTALLED_APP_URL.length()); @@ -140,8 +129,17 @@ public class RedirectUtils { r = sb.toString(); - valid = matchesRedirects(resolveValidRedirects, r); + valid = matchesRedirects(resolveValidRedirects, r, true); } + + // Return the original redirectUri, which can be partially encoded - for example http://localhost:8280/foo/bar%20bar%2092%2F72/3 . Just make sure it is normalized + redirectUri = getNormalizedRedirectUri(redirectUri); + + // We try to check validity also for original (encoded) redirectUrl, but just in case it exactly matches some "Valid Redirect URL" specified for client (not wildcards allowed) + if (!valid) { + valid = matchesRedirects(resolveValidRedirects, redirectUri, false); + } + if (valid && redirectUri.startsWith("/")) { redirectUri = relativeToAbsoluteURI(session, rootUrl, redirectUri); } @@ -155,6 +153,23 @@ public class RedirectUtils { } } + private static String getNormalizedRedirectUri(String redirectUri) { + if (redirectUri != null) { + try { + URI uri = URI.create(redirectUri); + redirectUri = uri.normalize().toString(); + } catch (IllegalArgumentException cause) { + logger.debug("Invalid redirect uri", cause); + return null; + } catch (Exception cause) { + logger.debug("Unexpected error when parsing redirect uri", cause); + return null; + } + redirectUri = lowerCaseHostname(redirectUri); + } + return redirectUri; + } + // Decode redirectUri. We don't decode query and fragment as those can be encoded in the original URL. // URL can be decoded multiple times (in case it was encoded multiple times, or some of it's parts were encoded multiple times) private static String decodeRedirectUri(String redirectUri) { @@ -162,7 +177,7 @@ public class RedirectUtils { int MAX_DECODING_COUNT = 5; // Max count of attempts for decoding URL (in case it was encoded multiple times) try { - KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(redirectUri); + KeycloakUriBuilder uriBuilder = KeycloakUriBuilder.fromUri(redirectUri).preserveDefaultPort(); String origQuery = uriBuilder.getQuery(); String origFragment = uriBuilder.getFragment(); String encodedRedirectUri = uriBuilder @@ -175,7 +190,7 @@ public class RedirectUtils { decodedRedirectUri = Encode.decode(encodedRedirectUri); if (decodedRedirectUri.equals(encodedRedirectUri)) { // URL is decoded. We can return it (after attach original query and fragment) - return KeycloakUriBuilder.fromUri(decodedRedirectUri) + return KeycloakUriBuilder.fromUri(decodedRedirectUri).preserveDefaultPort() .replaceQuery(origQuery) .fragment(origFragment) .buildAsString(); @@ -214,9 +229,10 @@ public class RedirectUtils { return sb.toString(); } - private static boolean matchesRedirects(Set validRedirects, String redirect) { + private static boolean matchesRedirects(Set validRedirects, String redirect, boolean allowWildcards) { + logger.tracef("matchesRedirects: redirect URL to check: %s, allow wildcards: %b, Configured valid redirect URLs: %s", redirect, allowWildcards, validRedirects); for (String validRedirect : validRedirects) { - if (validRedirect.endsWith("*") && !validRedirect.contains("?")) { + if (validRedirect.endsWith("*") && !validRedirect.contains("?") && allowWildcards) { // strip off the query component - we don't check them when wildcards are effective String r = redirect.contains("?") ? redirect.substring(0, redirect.indexOf("?")) : redirect; // strip off * diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java index 2959c4977b..f857b7f0ee 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java @@ -33,6 +33,8 @@ import org.junit.Rule; import org.junit.Test; import org.keycloak.OAuth2Constants; import org.keycloak.broker.provider.util.SimpleHttp; +import org.keycloak.common.util.Encode; +import org.keycloak.common.util.KeycloakUriBuilder; import org.keycloak.models.Constants; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.testsuite.AbstractKeycloakTest; @@ -113,7 +115,7 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { realm.client(installedApp2); ClientBuilder installedApp3 = ClientBuilder.create().clientId("test-wildcard").name("test-wildcard") - .redirectUris("http://example.com/foo/*", "http://with-dash.example.local/foo/*", "http://localhost:8280/foo/*") + .redirectUris("http://example.com/foo/*", "http://with-dash.example.local/foo/*", "http://localhost:8280/foo/*", "http://something.com:80/*") .secret("password"); realm.client(installedApp3); @@ -144,6 +146,11 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { .secret("password"); realm.client(installedApp8); + ClientBuilder installedApp9 = ClientBuilder.create().clientId("test-encoded-path").name("test-encoded-path") + .redirectUris("http://localhost:8280/foo/bar%20bar%2092%2F72/3", "http://localhost:8280/foo/bar%20bar%2092%2F72/44") + .secret("password"); + realm.client(installedApp9); + ClientBuilder installedAppCustomScheme = ClientBuilder.create().clientId("custom-scheme").name("custom-scheme") .redirectUris("android-app://org.keycloak.examples.cordova/https/keycloak-cordova-example.github.io/login") .secret("password"); @@ -338,6 +345,9 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { checkRedirectUri("http://example.com/foobar", false); checkRedirectUri("http://localhost:8280/foobar", false, true); + checkRedirectUri("http://something.com:80/some", true); + checkRedirectUri("http://localhost:8280/foo/bar%20bar%2092%2F72/3", true, true); + checkRedirectUri("http://example.com/foo/../", false); checkRedirectUri("http://example.com/foo/%2E%2E/", false); // url-encoded "http://example.com/foobar/../" checkRedirectUri("http://example.com/foo%2F%2E%2E%2F", false); // url-encoded "http://example.com/foobar/../" @@ -353,6 +363,13 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { checkRedirectUri("http://example.com/foo/#encode2=a%3Cb", true); } + // Client "Valid Redirect URL" is configured for exact match with supplied redirect-url + @Test + public void testRedirectUriWithEncodedPath() throws IOException { + oauth.clientId("test-encoded-path"); + checkRedirectUri("http://localhost:8280/foo/bar%20bar%2092%2F72/3", true, true); + } + @Test public void testDash() throws IOException { oauth.clientId("test-dash"); @@ -459,7 +476,10 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { } private void checkRedirectUri(String redirectUri, boolean expectValid, boolean checkCodeToToken) throws IOException { - oauth.redirectUri(redirectUri); + // Encoding parameter to make sure same String, which we're using would be used on server by RedirectUtils + // (as parameter is URL-decoded for one time on the server side by underlying server/resteasy) + String encodedRedirectUri = Encode.urlEncode(redirectUri); + oauth.redirectUri(encodedRedirectUri); if (!expectValid) { oauth.openLoginForm(); @@ -476,6 +496,20 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest { String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); Assert.assertNotNull(code); + // Test that browser URL where Keycloak redirected user matches with the used redirectUri + String browserUrlAfterRedirectFromKeycloak = KeycloakUriBuilder.fromUri(driver.getCurrentUrl()) + .replaceQueryParam(OAuth2Constants.CODE, null) + .replaceQueryParam(OAuth2Constants.STATE, null) + .replaceQueryParam(OAuth2Constants.SESSION_STATE, null) + .build().toString(); + if (browserUrlAfterRedirectFromKeycloak.endsWith("/")) browserUrlAfterRedirectFromKeycloak = browserUrlAfterRedirectFromKeycloak.substring(0, browserUrlAfterRedirectFromKeycloak.length() - 1); + if (Constants.INSTALLED_APP_URN.equals(redirectUri)) { + Assert.assertThat(browserUrlAfterRedirectFromKeycloak, Matchers.endsWith("oauth/oob")); + } else { + Assert.assertEquals(redirectUri, browserUrlAfterRedirectFromKeycloak); + } + + oauth.redirectUri(redirectUri); OAuthClient.AccessTokenResponse tokenResponse = oauth.doAccessTokenRequest(code, "password"); Assert.assertEquals("Expected success, but got error: " + tokenResponse.getError(), 200, tokenResponse.getStatusCode());