diff --git a/services/src/main/java/org/keycloak/services/ServicesLogger.java b/services/src/main/java/org/keycloak/services/ServicesLogger.java index 53a080def6..8c01b99fb8 100644 --- a/services/src/main/java/org/keycloak/services/ServicesLogger.java +++ b/services/src/main/java/org/keycloak/services/ServicesLogger.java @@ -471,4 +471,8 @@ public interface ServicesLogger extends BasicLogger { @LogMessage(level = DEBUG) @Message(id=107, value="Skipping create admin user. Admin already exists in realm '%s'.") void addAdminUserFailedAdminExists(String realm); + + @LogMessage(level = WARN) + @Message(id=108, value="URI '%s' doesn't match any trustedHost or trustedDomain") + void uriDoesntMatch(String uri); } diff --git a/services/src/main/java/org/keycloak/services/clientregistration/policy/impl/TrustedHostClientRegistrationPolicy.java b/services/src/main/java/org/keycloak/services/clientregistration/policy/impl/TrustedHostClientRegistrationPolicy.java index 84f2662153..6d79c701b4 100644 --- a/services/src/main/java/org/keycloak/services/clientregistration/policy/impl/TrustedHostClientRegistrationPolicy.java +++ b/services/src/main/java/org/keycloak/services/clientregistration/policy/impl/TrustedHostClientRegistrationPolicy.java @@ -19,6 +19,8 @@ package org.keycloak.services.clientregistration.policy.impl; import java.net.InetAddress; import java.net.MalformedURLException; +import java.net.URI; +import java.net.URISyntaxException; import java.net.URL; import java.net.UnknownHostException; import java.util.LinkedList; @@ -64,7 +66,6 @@ public class TrustedHostClientRegistrationPolicy implements ClientRegistrationPo } - @Override public void beforeUpdate(ClientRegistrationContext context, ClientModel clientModel) throws ClientRegistrationPolicyException { verifyHost(); @@ -214,26 +215,17 @@ public class TrustedHostClientRegistrationPolicy implements ClientRegistrationPo checkURLTrusted(adminUrl, trustedHosts, trustedDomains); } for (String redirect : resolvedRedirects) { - checkURLTrusted(redirect, trustedHosts, trustedDomains); + checkURITrusted(redirect, trustedHosts, trustedDomains); } } - protected void checkURLTrusted(String url, List trustedHosts, List trustedDomains) throws ClientRegistrationPolicyException { try { String host = new URL(url).getHost(); - for (String trustedHost : trustedHosts) { - if (host.equals(trustedHost)) { - return; - } - } - - for (String trustedDomain : trustedDomains) { - if (host.endsWith(trustedDomain)) { - return; - } + if (checkHostTrusted(host, trustedHosts, trustedDomains)) { + return; } } catch (MalformedURLException mfe) { logger.debugf(mfe, "URL '%s' is malformed", url); @@ -244,6 +236,38 @@ public class TrustedHostClientRegistrationPolicy implements ClientRegistrationPo throw new ClientRegistrationPolicyException("URL doesn't match any trusted host or trusted domain"); } + protected void checkURITrusted(String uri, List trustedHosts, List trustedDomains) throws ClientRegistrationPolicyException { + try { + String host = new URI(uri).getHost(); + + if (checkHostTrusted(host, trustedHosts, trustedDomains)) { + return; + } + } catch (URISyntaxException use) { + logger.debugf(use, "URI '%s' is malformed", uri); + throw new ClientRegistrationPolicyException("URI is malformed"); + } + + ServicesLogger.LOGGER.uriDoesntMatch(uri); + throw new ClientRegistrationPolicyException("URI doesn't match any trusted host or trusted domain"); + } + + private boolean checkHostTrusted(String host, List trustedHosts, List trustedDomains) { + for (String trustedHost : trustedHosts) { + if (host.equals(trustedHost)) { + return true; + } + } + + for (String trustedDomain : trustedDomains) { + if (host.endsWith(trustedDomain)) { + return true; + } + } + + return false; + } + private static String relativeToAbsoluteURI(String rootUrl, String relative) { if (relative == null) { @@ -270,6 +294,6 @@ public class TrustedHostClientRegistrationPolicy implements ClientRegistrationPo // True by default private boolean parseBoolean(String propertyKey) { String val = componentModel.getConfig().getFirst(propertyKey); - return val==null || Boolean.parseBoolean(val); + return val == null || Boolean.parseBoolean(val); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationPoliciesTest.java index 9217e0ab42..d6d0077454 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientRegistrationPoliciesTest.java @@ -194,6 +194,21 @@ public class ClientRegistrationPoliciesTest extends AbstractClientRegistrationTe } + @Test + public void testAnonCreateWithInvalidRedirectScheme() throws Exception { + setTrustedHost("localhost"); + OIDCClientRepresentation client = createRepOidc(); + + client.setRedirectUris(Collections.singletonList("invalid://bad.host")); + assertOidcFail(ClientRegOp.CREATE, client, 403, "URI doesn't match"); + + client.setRedirectUris(Collections.singletonList("invalid://localhost")); + OIDCClientRepresentation oidcClientRep = reg.oidc().create(client); + + assertRegAccessToken(oidcClientRep.getRegistrationAccessToken(), RegistrationAuth.ANONYMOUS); + } + + @Test public void testAnonUpdateWithTrustedHost() throws Exception { setTrustedHost("localhost"); @@ -201,7 +216,7 @@ public class ClientRegistrationPoliciesTest extends AbstractClientRegistrationTe // Fail update client client.setRedirectUris(Collections.singletonList("http://bad:8080/foo")); - assertOidcFail(ClientRegOp.UPDATE, client, 403, "URL doesn't match"); + assertOidcFail(ClientRegOp.UPDATE, client, 403, "URI doesn't match"); // Should be fine now client.setRedirectUris(Collections.singletonList("http://localhost:8080/foo")); @@ -209,6 +224,7 @@ public class ClientRegistrationPoliciesTest extends AbstractClientRegistrationTe } + @Test public void testRedirectUriWithDomain() throws Exception { // Change the policy to avoid checking hosts @@ -229,7 +245,7 @@ public class ClientRegistrationPoliciesTest extends AbstractClientRegistrationTe // Check new client can't be created anymore oidcClientRep = createRepOidc("http://www.host.com", "http://www.example.com"); - assertOidcFail(ClientRegOp.CREATE, oidcClientRep, 403, "URL doesn't match"); + assertOidcFail(ClientRegOp.CREATE, oidcClientRep, 403, "URI doesn't match"); }