Merge pull request #3177 from vmuzikar/KEYCLOAK-3421

KEYCLOAK-3421 Validation for URI fragments in redirect_uri

services/src/main/java/org/keycloak/services/clientregistration/AbstractClientRegistrationProvider.java

@@ -29,8 +29,12 @@ import org.keycloak.models.utils.RepresentationToModel;
 import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.services.ErrorResponseException;
 import org.keycloak.services.ForbiddenException;
+import org.keycloak.services.resources.admin.AdminRoot;
+import org.keycloak.services.validation.ClientValidator;
+import org.keycloak.services.validation.ValidationMessages;
 
 import javax.ws.rs.core.Response;
+import java.util.Properties;
 
 /**
  * @author <a href="mailto:sthorger@redhat.com">Stian Thorgersen</a>
@@ -50,6 +54,16 @@ public abstract class AbstractClientRegistrationProvider implements ClientRegist
 
         auth.requireCreate();
 
+        ValidationMessages validationMessages = new ValidationMessages();
+        if (!ClientValidator.validate(client, validationMessages)) {
+            String errorCode = validationMessages.fieldHasError("redirectUris") ? ErrorCodes.INVALID_REDIRECT_URI : ErrorCodes.INVALID_CLIENT_METADATA;
+            throw new ErrorResponseException(
+                    errorCode,
+                    validationMessages.getStringMessages(),
+                    Response.Status.BAD_REQUEST
+            );
+        }
+
         try {
             ClientModel clientModel = RepresentationToModel.createClient(session, session.getContext().getRealm(), client, true);
 
@@ -104,6 +118,16 @@ public abstract class AbstractClientRegistrationProvider implements ClientRegist
             throw new ErrorResponseException(ErrorCodes.INVALID_CLIENT_METADATA, "Client Identifier modified", Response.Status.BAD_REQUEST);
         }
 
+        ValidationMessages validationMessages = new ValidationMessages();
+        if (!ClientValidator.validate(rep, validationMessages)) {
+            String errorCode = validationMessages.fieldHasError("redirectUris") ? ErrorCodes.INVALID_REDIRECT_URI : ErrorCodes.INVALID_CLIENT_METADATA;
+            throw new ErrorResponseException(
+                    errorCode,
+                    validationMessages.getStringMessages(),
+                    Response.Status.BAD_REQUEST
+            );
+        }
+
         RepresentationToModel.updateClient(rep, client);
         rep = ModelToRepresentation.toRepresentation(client);
 

services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java

@@ -40,6 +40,7 @@ import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.CredentialRepresentation;
 import org.keycloak.representations.idm.UserRepresentation;
 import org.keycloak.representations.idm.UserSessionRepresentation;
+import org.keycloak.services.ErrorResponseException;
 import org.keycloak.services.ServicesLogger;
 import org.keycloak.services.clientregistration.ClientRegistrationTokenUtils;
 import org.keycloak.services.managers.ClientManager;
@@ -48,6 +49,8 @@ import org.keycloak.services.managers.ResourceAdminManager;
 import org.keycloak.services.resources.KeycloakApplication;
 import org.keycloak.services.ErrorResponse;
 import org.keycloak.common.util.Time;
+import org.keycloak.services.validation.ClientValidator;
+import org.keycloak.services.validation.ValidationMessages;
 
 import javax.ws.rs.Consumes;
 import javax.ws.rs.DELETE;
@@ -63,10 +66,7 @@ import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;
 
-import java.util.ArrayList;
+import java.util.*;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
 
 import static java.lang.Boolean.TRUE;
 
@@ -126,6 +126,16 @@ public class ClientResource {
             throw new NotFoundException("Could not find client");
         }
 
+        ValidationMessages validationMessages = new ValidationMessages();
+        if (!ClientValidator.validate(rep, validationMessages)) {
+            Properties messages = AdminRoot.getMessages(session, realm, auth.getAuth().getToken().getLocale());
+            throw new ErrorResponseException(
+                    validationMessages.getStringMessages(),
+                    validationMessages.getStringMessages(messages),
+                    Response.Status.BAD_REQUEST
+            );
+        }
+
         try {
             updateClientFromRep(rep, client, session);
             adminEvent.operation(OperationType.UPDATE).resourcePath(uriInfo).representation(rep).success();

services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java

@@ -17,7 +17,6 @@
 package org.keycloak.services.resources.admin;
 
 import org.jboss.resteasy.annotations.cache.NoCache;
-import org.jboss.resteasy.spi.NotFoundException;
 import org.jboss.resteasy.spi.ResteasyProviderFactory;
 import org.keycloak.events.admin.OperationType;
 import org.keycloak.events.admin.ResourceType;
@@ -28,16 +27,13 @@ import org.keycloak.models.RealmModel;
 import org.keycloak.models.utils.ModelToRepresentation;
 import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.services.ErrorResponse;
+import org.keycloak.services.ErrorResponseException;
 import org.keycloak.services.ServicesLogger;
 import org.keycloak.services.managers.ClientManager;
+import org.keycloak.services.validation.ClientValidator;
+import org.keycloak.services.validation.ValidationMessages;
 
-import javax.ws.rs.Consumes;
+import javax.ws.rs.*;
-import javax.ws.rs.GET;
-import javax.ws.rs.POST;
-import javax.ws.rs.Path;
-import javax.ws.rs.PathParam;
-import javax.ws.rs.Produces;
-import javax.ws.rs.QueryParam;
 import javax.ws.rs.core.Context;
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
@@ -45,6 +41,7 @@ import javax.ws.rs.core.UriInfo;
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Properties;
 
 /**
  * Base resource class for managing a realm's clients.
@@ -122,6 +119,16 @@ public class ClientsResource {
     public Response createClient(final @Context UriInfo uriInfo, final ClientRepresentation rep) {
         auth.requireManage();
 
+        ValidationMessages validationMessages = new ValidationMessages();
+        if (!ClientValidator.validate(rep, validationMessages)) {
+            Properties messages = AdminRoot.getMessages(session, realm, auth.getAuth().getToken().getLocale());
+            throw new ErrorResponseException(
+                    validationMessages.getStringMessages(),
+                    validationMessages.getStringMessages(messages),
+                    Response.Status.BAD_REQUEST
+            );
+        }
+
         try {
             ClientModel clientModel = ClientManager.createClient(session, realm, rep, true);
 

services/src/main/java/org/keycloak/services/validation/ClientValidator.java

@@ -0,0 +1,54 @@
+/*
+ *
+ *  * Copyright 2016 Red Hat, Inc. and/or its affiliates
+ *  * and other contributors as indicated by the @author tags.
+ *  *
+ *  * Licensed under the Apache License, Version 2.0 (the "License");
+ *  * you may not use this file except in compliance with the License.
+ *  * You may obtain a copy of the License at
+ *  *
+ *  * http://www.apache.org/licenses/LICENSE-2.0
+ *  *
+ *  * Unless required by applicable law or agreed to in writing, software
+ *  * distributed under the License is distributed on an "AS IS" BASIS,
+ *  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  * See the License for the specific language governing permissions and
+ *  * limitations under the License.
+ *
+ */
+
+package org.keycloak.services.validation;
+
+import org.keycloak.representations.idm.ClientRepresentation;
+
+/**
+ * @author Vaclav Muzikar <vmuzikar@redhat.com>
+ */
+public class ClientValidator {
+    /**
+     * Checks if the Client's Redirect URIs doesn't contain any URI fragments (like http://example.org/auth#fragment)
+     *
+     * @see <a href="https://issues.jboss.org/browse/KEYCLOAK-3421">KEYCLOAK-3421</a>
+     * @param client
+     * @param messages
+     * @return true if Redirect URIs doesn't contain any URI with fragments
+     */
+    public static boolean validate(ClientRepresentation client, ValidationMessages messages) {
+        boolean isValid = true;
+
+        if (client.getRedirectUris() != null) {
+            long urisWithFragmentCount = client.getRedirectUris().stream().filter(p -> p.contains("#")).count();
+            if (urisWithFragmentCount > 0) {
+                messages.add("redirectUris", "Redirect URIs must not contain an URI fragment", "clientRedirectURIsFragmentError");
+                isValid = false;
+            }
+        }
+
+        if (client.getRootUrl() != null && client.getRootUrl().contains("#")) {
+            messages.add("rootUrl", "Root URL must not contain an URL fragment", "clientRootURLFragmentError");
+            isValid = false;
+        }
+
+        return isValid;
+    }
+}

services/src/main/java/org/keycloak/services/validation/ValidationMessage.java

@@ -0,0 +1,100 @@
+/*
+ *
+ *  * Copyright 2016 Red Hat, Inc. and/or its affiliates
+ *  * and other contributors as indicated by the @author tags.
+ *  *
+ *  * Licensed under the Apache License, Version 2.0 (the "License");
+ *  * you may not use this file except in compliance with the License.
+ *  * You may obtain a copy of the License at
+ *  *
+ *  * http://www.apache.org/licenses/LICENSE-2.0
+ *  *
+ *  * Unless required by applicable law or agreed to in writing, software
+ *  * distributed under the License is distributed on an "AS IS" BASIS,
+ *  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  * See the License for the specific language governing permissions and
+ *  * limitations under the License.
+ *
+ */
+
+package org.keycloak.services.validation;
+
+import java.text.MessageFormat;
+import java.util.Properties;
+
+/**
+ * @author Vaclav Muzikar <vmuzikar@redhat.com>
+ */
+public class ValidationMessage {
+    private String fieldId;
+    private String message;
+    private String localizedMessageKey;
+    private Object[] localizedMessageParameters;
+
+    public ValidationMessage(String message) {
+        this.message = message;
+    }
+
+    public ValidationMessage(String message, String localizedMessageKey, Object... localizedMessageParameters) {
+        this.message = message;
+        this.localizedMessageKey = localizedMessageKey;
+        this.localizedMessageParameters = localizedMessageParameters;
+    }
+
+    public String getFieldId() {
+        return fieldId;
+    }
+
+    public void setFieldId(String fieldId) {
+        this.fieldId = fieldId;
+    }
+
+    public String getLocalizedMessageKey() {
+        return localizedMessageKey;
+    }
+
+    public void setLocalizedMessageKey(String localizedMessageKey) {
+        this.localizedMessageKey = localizedMessageKey;
+    }
+
+    public Object[] getLocalizedMessageParameters() {
+        return localizedMessageParameters;
+    }
+
+    public void setLocalizedMessageParameters(Object[] localizedMessageParameters) {
+        this.localizedMessageParameters = localizedMessageParameters;
+    }
+
+    public String getMessage() {
+        return message;
+    }
+
+    public String getMessage(Properties localizedMessages) {
+        if (getLocalizedMessageKey() != null) {
+            return MessageFormat.format(localizedMessages.getProperty(getLocalizedMessageKey(), getMessage()), getLocalizedMessageParameters());
+        }
+        else {
+            return getMessage();
+        }
+    }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+
+        ValidationMessage message1 = (ValidationMessage) o;
+
+        if (getFieldId() != null ? !getFieldId().equals(message1.getFieldId()) : message1.getFieldId() != null)
+            return false;
+        return getMessage() != null ? getMessage().equals(message1.getMessage()) : message1.getMessage() == null;
+
+    }
+
+    @Override
+    public int hashCode() {
+        int result = getFieldId() != null ? getFieldId().hashCode() : 0;
+        result = 31 * result + (getMessage() != null ? getMessage().hashCode() : 0);
+        return result;
+    }
+}

services/src/main/java/org/keycloak/services/validation/ValidationMessages.java

@@ -0,0 +1,83 @@
+/*
+ *
+ *  * Copyright 2016 Red Hat, Inc. and/or its affiliates
+ *  * and other contributors as indicated by the @author tags.
+ *  *
+ *  * Licensed under the Apache License, Version 2.0 (the "License");
+ *  * you may not use this file except in compliance with the License.
+ *  * You may obtain a copy of the License at
+ *  *
+ *  * http://www.apache.org/licenses/LICENSE-2.0
+ *  *
+ *  * Unless required by applicable law or agreed to in writing, software
+ *  * distributed under the License is distributed on an "AS IS" BASIS,
+ *  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  * See the License for the specific language governing permissions and
+ *  * limitations under the License.
+ *
+ */
+
+package org.keycloak.services.validation;
+
+import java.util.*;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+
+/**
+ * @author Vaclav Muzikar <vmuzikar@redhat.com>
+ */
+public class ValidationMessages {
+    private Set<ValidationMessage> messages = new LinkedHashSet<>();
+
+    public ValidationMessages() {}
+
+    public ValidationMessages(String... messages) {
+        for (String message : messages) {
+            add(message);
+        }
+    }
+
+    public void add(String message) {
+        messages.add(new ValidationMessage(message));
+    }
+
+    public void add(String message, String localizedMessageKey) {
+        messages.add(new ValidationMessage(message, localizedMessageKey));
+    }
+
+    public void add(String fieldId, String message, String localizedMessageKey) {
+        ValidationMessage validationMessage = new ValidationMessage(message, localizedMessageKey);
+        validationMessage.setFieldId(fieldId);
+        add(validationMessage);
+    }
+
+    public void add(ValidationMessage message) {
+        messages.add(message);
+    }
+
+    public boolean fieldHasError(String fieldId) {
+        for (ValidationMessage message : messages) {
+            if (message.getFieldId().equals(fieldId)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    public Set<ValidationMessage> getMessages() {
+        return Collections.unmodifiableSet(messages);
+    }
+
+    protected String getStringMessages(Function<? super ValidationMessage, ? extends String> function) {
+        return messages.stream().map(function).collect(Collectors.joining("; "));
+    }
+
+    public String getStringMessages() {
+        return getStringMessages(ValidationMessage::getMessage);
+    }
+
+    public String getStringMessages(Properties localizedMessages) {
+        return getStringMessages(x -> x.getMessage(localizedMessages));
+    }
+}

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ClientTest.java

@@ -33,16 +33,14 @@ import org.keycloak.representations.adapters.action.PushNotBeforeAction;
 import org.keycloak.representations.adapters.action.TestAvailabilityAction;
 import org.keycloak.representations.idm.*;
 
+import javax.ws.rs.BadRequestException;
 import javax.ws.rs.NotFoundException;
 import javax.ws.rs.core.Response;
 
 import java.io.IOException;
-import java.util.Collections;
+import java.util.*;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
 
+import org.keycloak.services.ErrorResponseException;
 import org.keycloak.testsuite.Assert;
 import org.keycloak.testsuite.util.AdminEventPaths;
 import org.keycloak.testsuite.util.ClientBuilder;
@@ -212,6 +210,52 @@ public class ClientTest extends AbstractAdminTest {
         assertEquals("service-account-serviceclient", userRep.getUsername());
     }
 
+    // KEYCLOAK-3421
+    @Test
+    public void createClientWithFragments() {
+        ClientRepresentation client = ClientBuilder.create()
+                .clientId("client-with-fragment")
+                .rootUrl("http://localhost/base#someFragment")
+                .redirectUris("http://localhost/auth", "http://localhost/auth#fragment", "http://localhost/auth*", "/relative")
+                .build();
+
+        Response response = realm.clients().create(client);
+        assertUriFragmentError(response);
+    }
+
+    // KEYCLOAK-3421
+    @Test
+    public void updateClientWithFragments() {
+        ClientRepresentation client = ClientBuilder.create()
+                .clientId("client-with-fragment")
+                .redirectUris("http://localhost/auth", "http://localhost/auth*")
+                .build();
+        Response response = realm.clients().create(client);
+        ClientResource clientResource = realm.clients().get(ApiUtil.getCreatedId(response));
+
+        client = clientResource.toRepresentation();
+        client.setRootUrl("http://localhost/base#someFragment");
+        List<String> redirectUris = client.getRedirectUris();
+        redirectUris.add("http://localhost/auth#fragment");
+        redirectUris.add("/relative");
+        client.setRedirectUris(redirectUris);
+
+        try {
+            clientResource.update(client);
+            fail("Should fail");
+        }
+        catch (BadRequestException e) {
+            assertUriFragmentError(e.getResponse());
+        }
+    }
+
+    private void assertUriFragmentError(Response response) {
+        assertEquals(response.getStatus(), 400);
+        String error = response.readEntity(OAuth2ErrorRepresentation.class).getError();
+        assertTrue("Error response doesn't mention Redirect URIs fragments", error.contains("Redirect URIs must not contain an URI fragment"));
+        assertTrue("Error response doesn't mention Root URL fragments", error.contains("Root URL must not contain an URL fragment"));
+    }
+
     @Test
     public void pushRevocation() {
         testingClient.testApp().clearAdminActions();

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCClientRegistrationTest.java

@@ -102,6 +102,23 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest {
         return response;
     }
 
+    private void assertCreateFail(OIDCClientRepresentation client, int expectedStatusCode) {
+        assertCreateFail(client, expectedStatusCode, null);
+    }
+
+    private void assertCreateFail(OIDCClientRepresentation client, int expectedStatusCode, String expectedErrorContains) {
+        try {
+            reg.oidc().create(client);
+            Assert.fail("Not expected to successfuly register client");
+        } catch (ClientRegistrationException expected) {
+            HttpErrorException httpEx = (HttpErrorException) expected.getCause();
+            Assert.assertEquals(expectedStatusCode, httpEx.getStatusLine().getStatusCode());
+            if (expectedErrorContains != null) {
+                assertTrue("Error response doesn't contain expected text", httpEx.getErrorResponse().contains(expectedErrorContains));
+            }
+        }
+    }
+
     @Test
     public void testCreateWithTrustedHost() throws Exception {
         reg.auth(null);
@@ -109,17 +126,10 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest {
         OIDCClientRepresentation client = createRep();
 
         // Failed to create client
-        try {
+        assertCreateFail(client, 401);
-            reg.oidc().create(client);
-            Assert.fail("Not expected to successfuly register client");
-        } catch (ClientRegistrationException expected) {
-            HttpErrorException httpEx = (HttpErrorException) expected.getCause();
-            Assert.assertEquals(401, httpEx.getStatusLine().getStatusCode());
-        }
 
         // Create trusted host entry
-        Response response = adminClient.realm(REALM_NAME).clientRegistrationTrustedHost().create(ClientRegistrationTrustedHostRepresentation.create("localhost", 2, 2));
+        createTrustedHost("localhost", 2);
-        Assert.assertEquals(201, response.getStatus());
 
         // Successfully register client
         reg.oidc().create(client);
@@ -132,13 +142,20 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest {
         reg.oidc().create(client);
 
         // Failed to create 3rd client
-        try {
+        assertCreateFail(client, 401);
-            reg.oidc().create(client);
+    }
-            Assert.fail("Not expected to successfuly register client");
+
-        } catch (ClientRegistrationException expected) {
+    // KEYCLOAK-3421
-            HttpErrorException httpEx = (HttpErrorException) expected.getCause();
+    @Test
-            Assert.assertEquals(401, httpEx.getStatusLine().getStatusCode());
+    public void createClientWithUriFragment() {
-        }
+        reg.auth(null);
+
+        createTrustedHost("localhost", 1);
+
+        OIDCClientRepresentation client = createRep();
+        client.setRedirectUris(Arrays.asList("http://localhost/auth", "http://localhost/auth#fragment", "http://localhost/auth*"));
+
+        assertCreateFail(client, 400, "URI fragment");
     }
 
     @Test
@@ -323,4 +340,9 @@ public class OIDCClientRegistrationTest extends AbstractClientRegistrationTest {
         }
     }
 
+    private void createTrustedHost(String name, int count) {
+        Response response = adminClient.realm(REALM_NAME).clientRegistrationTrustedHost().create(ClientRegistrationTrustedHostRepresentation.create(name, count, count));
+        Assert.assertEquals(201, response.getStatus());
+    }
+
 }

themes/src/main/resources/theme/base/admin/messages/messages_en.properties

@@ -12,3 +12,6 @@ ldapErrorMissingClientId=Client ID needs to be provided in config when Realm Rol
 ldapErrorCantPreserveGroupInheritanceWithUIDMembershipType=Not possible to preserve group inheritance and use UID membership type together.
 ldapErrorCantWriteOnlyForReadOnlyLdap=Can't set write only when LDAP provider mode is not WRITABLE
 ldapErrorCantWriteOnlyAndReadOnly=Can't set write-only and read-only together
+
+clientRedirectURIsFragmentError=Redirect URIs must not contain an URI fragment
+clientRootURLFragmentError=Root URL must not contain an URL fragment

themes/src/main/resources/theme/base/admin/resources/js/controllers/clients.js

@@ -1111,6 +1111,12 @@ module.controller('ClientDetailCtrl', function($scope, realm, client, templates,
             }, $scope.client, function() {
                 $route.reload();
                 Notifications.success("Your changes have been saved to the client.");
+            }, function(error) {
+                if (error.status == 400 && error.data.error_description) {
+                    Notifications.error(error.data.error_description);
+                } else {
+                    Notifications.error('Unexpected error when updating client');
+                }
             });
         }
     };
@@ -1225,6 +1231,12 @@ module.controller('CreateClientCtrl', function($scope, realm, client, templates,
             var id = l.substring(l.lastIndexOf("/") + 1);
             $location.url("/realms/" + realm.realm + "/clients/" + id);
             Notifications.success("The client has been created.");
+        }, function(error) {
+            if (error.status == 400 && error.data.error_description) {
+                Notifications.error(error.data.error_description);
+            } else {
+                Notifications.error('Unexpected error when creating client');
+            }
         });
     };