Improve handling for loopback redirect-uri validation (#195) (#33189)

Closes #33116

Signed-off-by: stianst <stianst@gmail.com>

server-spi-private/src/main/java/org/keycloak/models/Constants.java

@@ -43,8 +43,6 @@ public final class Constants {
     public static final Collection<String> defaultClients = Arrays.asList(ACCOUNT_MANAGEMENT_CLIENT_ID, ADMIN_CLI_CLIENT_ID, BROKER_SERVICE_CLIENT_ID, REALM_MANAGEMENT_CLIENT_ID, ADMIN_CONSOLE_CLIENT_ID);
 
     public static final String INSTALLED_APP_URN = "urn:ietf:wg:oauth:2.0:oob";
-    public static final String INSTALLED_APP_URL = "http://localhost";
-    public static final String INSTALLED_APP_LOOPBACK = "http://127.0.0.1";
 
     public static final String READ_TOKEN_ROLE = "read-token";
     public static final String[] BROKER_SERVICE_ROLES = {READ_TOKEN_ROLE};

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

@@ -18,6 +18,7 @@
 package org.keycloak.protocol.oidc.utils;
 
 import org.jboss.logging.Logger;
+import org.keycloak.common.util.KeycloakUriBuilder;
 import org.keycloak.common.util.UriUtils;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.Constants;
@@ -28,7 +29,9 @@ import org.keycloak.services.Urls;
 import org.keycloak.services.util.ResolveRelative;
 
 import java.net.URI;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.Set;
 import java.util.TreeSet;
 import java.util.regex.Pattern;
@@ -38,6 +41,8 @@ import java.util.regex.Pattern;
  */
 public class RedirectUtils {
 
+    public static final Set<String> LOOPBACK_INTERFACES = new HashSet<>(Arrays.asList("localhost", "127.0.0.1", "[::1]"));
+
     private static final Logger logger = Logger.getLogger(RedirectUtils.class);
 
     public static String verifyRedirectUri(KeycloakSession session, String redirectUri, ClientModel client) {
@@ -95,20 +100,9 @@ public class RedirectUtils {
 
             String valid = matchesRedirects(resolveValidRedirects, r, allowWildcards);
 
-            if (valid == null && (r.startsWith(Constants.INSTALLED_APP_URL) || r.startsWith(Constants.INSTALLED_APP_LOOPBACK)) && r.indexOf(':', Constants.INSTALLED_APP_URL.length()) >= 0) {
+            if (valid == null && "http".equals(originalRedirect.getScheme()) && LOOPBACK_INTERFACES.contains(originalRedirect.getHost())) {
-                int i = r.indexOf(':', Constants.INSTALLED_APP_URL.length());
+                String redirectWithDefaultPort = KeycloakUriBuilder.fromUri(originalRedirect).port(80).buildAsString();
-
+                valid = matchesRedirects(resolveValidRedirects, redirectWithDefaultPort, allowWildcards);
-                StringBuilder sb = new StringBuilder();
-                sb.append(r.substring(0, i));
-
-                i = r.indexOf('/', i);
-                if (i >= 0) {
-                    sb.append(r.substring(i));
-                }
-
-                r = sb.toString();
-
-                valid = matchesRedirects(resolveValidRedirects, r, allowWildcards);
             }
 
             if (valid != null && !originalRedirect.isAbsolute()) {

services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java

@@ -71,6 +71,45 @@ public class RedirectUtilsTest {
         Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.com/test", set, false));
     }
 
+    @Test
+    public void testVerifyRedirectUriNative() {
+        Set<String> set = Stream.of(
+                "http://127.0.0.1",
+                "http://127.0.0.1/callback",
+                "http://[::1]",
+                "http://[::1]/callback",
+                "http://127.0.0.1/callback",
+                "http://localhost",
+                "http://localhost/callback"
+        ).collect(Collectors.toSet());
+
+        Assert.assertEquals("http://127.0.0.1", RedirectUtils.verifyRedirectUri(session, null, "http://127.0.0.1", set, false));
+        Assert.assertEquals("http://127.0.0.1:12324", RedirectUtils.verifyRedirectUri(session, null, "http://127.0.0.1:12324", set, false));
+        Assert.assertEquals("http://127.0.0.1/callback", RedirectUtils.verifyRedirectUri(session, null, "http://127.0.0.1/callback", set, false));
+        Assert.assertEquals("http://127.0.0.1:12324/callback", RedirectUtils.verifyRedirectUri(session, null, "http://127.0.0.1:12324/callback", set, false));
+        Assert.assertEquals("http://[::1]", RedirectUtils.verifyRedirectUri(session, null, "http://[::1]", set, false));
+        Assert.assertEquals("http://[::1]:12324", RedirectUtils.verifyRedirectUri(session, null, "http://[::1]:12324", set, false));
+        Assert.assertEquals("http://[::1]/callback", RedirectUtils.verifyRedirectUri(session, null, "http://[::1]/callback", set, false));
+        Assert.assertEquals("http://[::1]:12324/callback", RedirectUtils.verifyRedirectUri(session, null, "http://[::1]:12324/callback", set, false));
+        Assert.assertEquals("http://localhost", RedirectUtils.verifyRedirectUri(session, null, "http://localhost", set, false));
+        Assert.assertEquals("http://localhost:12324", RedirectUtils.verifyRedirectUri(session, null, "http://localhost:12324", set, false));
+        Assert.assertEquals("http://localhost/callback", RedirectUtils.verifyRedirectUri(session, null, "http://localhost/callback", set, false));
+        Assert.assertEquals("http://localhost:12324/callback", RedirectUtils.verifyRedirectUri(session, null, "http://localhost:12324/callback", set, false));
+
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://127.0.0.1@invalid.com", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://127.0.0.1:12324@invalid.com", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://127.0.0.1@invalid.com/callback", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://127.0.0.1:12324@invalid.com/callback", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://[::1]@invalid.com", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://[::1]:12324@invalid.com", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://[::1]@invalid.com/callback", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://[::1]:12324@invalid.com/callback", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://localhost@invalid.com", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://localhost:12324@invalid.com", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://localhost@invalid.com/callback", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "http://localhost:12324@invalid.com/callback", set, false));
+    }
+
     @Test
     public void testverifyRedirectUriMixedSchemes() {
         Set<String> set = Stream.of(

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

@@ -106,12 +106,12 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest {
         RealmBuilder realm = RealmBuilder.edit(realmRepresentation).testEventListener();
 
         ClientBuilder installedApp = ClientBuilder.create().clientId("test-installed").name("test-installed")
-                .redirectUris(Constants.INSTALLED_APP_URN, Constants.INSTALLED_APP_URL)
+                .redirectUris(Constants.INSTALLED_APP_URN, "http://localhost")
                 .secret("password");
         realm.client(installedApp);
 
         ClientBuilder installedApp2 = ClientBuilder.create().clientId("test-installed2").name("test-installed2")
-                .redirectUris(Constants.INSTALLED_APP_URL + "/myapp")
+                .redirectUris("http://localhost/myapp")
                 .secret("password");
         realm.client(installedApp2);
 
@@ -158,12 +158,12 @@ public class OAuthRedirectUriTest extends AbstractKeycloakTest {
         realm.client(installedAppCustomScheme);
 
         ClientBuilder installedAppLoopback = ClientBuilder.create().clientId("test-installed-loopback").name("test-installed-loopback")
-                .redirectUris(Constants.INSTALLED_APP_LOOPBACK)
+                .redirectUris("http://127.0.0.1")
                 .secret("password");
         realm.client(installedAppLoopback);
 
         ClientBuilder installedAppLoopback2 = ClientBuilder.create().clientId("test-installed-loopback2").name("test-installed-loopback2")
-                .redirectUris(Constants.INSTALLED_APP_LOOPBACK + "/myapp")
+                .redirectUris("http://127.0.0.1/myapp")
                 .secret("password");
         realm.client(installedAppLoopback2);