UNIX.dog
/
keycloak
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Regressions in redirect URL verification when redirect_uri has encoded path or default port 

closes #16851
closes #16587
This commit is contained in:
mposolda 2023-03-29 10:23:28 +02:00 committed by Marek Posolda
parent 639d931251
commit 709c6b5a47
5 changed files with 112 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
common/src/main/java/org/keycloak/common/util/KeycloakUriBuilder.java
View File

@ -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) {

21
common/src/test/java/org/keycloak/common/util/KeycloakUriBuilderTest.java
View File

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

1
services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java
View File

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

60
services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java
View File

@ -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<String> 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<String> validRedirects, String redirect) {
private static boolean matchesRedirects(Set<String> 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 *

38
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OAuthRedirectUriTest.java
View File

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