From 1c71eb93db77f2f3fd7d5f8a5ce803b5250a93b1 Mon Sep 17 00:00:00 2001
From: Pedro Igor <pigor.craveiro@gmail.com>
Date: Thu, 20 Feb 2020 18:46:35 -0300
Subject: [PATCH] [KEYCLOAK-11576] - Properly handling redirect_uri parser
 errors

---
 .../protocol/oidc/utils/RedirectUtils.java    | 23 ++++++++++---------
 .../oauth/AuthorizationCodeTest.java          | 17 ++++++++++++++
 2 files changed, 29 insertions(+), 11 deletions(-)

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 5f89335010..0b108f1ace 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
@@ -84,8 +84,18 @@ public class RedirectUtils {
         KeycloakUriInfo uriInfo = session.getContext().getUri();
         RealmModel realm = session.getContext().getRealm();
 
-        if (redirectUri != null)
-            redirectUri = normalizeUrl(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) {
@@ -185,13 +195,4 @@ public class RedirectUtils {
         }
         return validRedirect;
     }
-
-    private static String normalizeUrl(String url) {
-        try {
-            URI uri = new URI(url);
-            return uri.normalize().toString();
-        } catch (URISyntaxException e) {
-            throw new IllegalArgumentException("Invalid URL syntax: " + e.getMessage());
-        }
-    }
 }
diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java
index a3b3f098eb..a82b58fc59 100755
--- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java
+++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java
@@ -114,6 +114,23 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
         String codeId = events.expectLogin().assertEvent().getDetails().get(Details.CODE_ID);
     }
 
+    @Test
+    public void testInvalidRedirectUri() {
+        ClientManager.realm(adminClient.realm("test")).clientId("test-app").addRedirectUris(oauth.getRedirectUri());
+
+        oauth.redirectUri(oauth.getRedirectUri() + "%20test");
+        oauth.openLoginForm();
+
+        assertTrue(errorPage.isCurrent());
+        assertEquals("Invalid parameter: redirect_uri", errorPage.getError());
+
+        oauth.redirectUri("ZAP%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%25n%25s%0A");
+        oauth.openLoginForm();
+
+        assertTrue(errorPage.isCurrent());
+        assertEquals("Invalid parameter: redirect_uri", errorPage.getError());
+    }
+
     @Test
     public void authorizationRequestNoState() throws IOException {
         oauth.stateParamHardcoded(null);