Escape action in the form_post.jwt and only decode path in RedirectUt…

…ils (#93) Closes #90 Signed-off-by: rmartinc <rmartinc@redhat.com>
keycloak · Jan 4, 2024 · 4188bc3 · 4188bc3
1 parent c46920b
commit 4188bc3
Show file tree

Hide file tree

Showing 5 changed files with 88 additions and 26 deletions.
diff --git a/common/src/main/java/org/keycloak/common/util/HttpPostRedirect.java b/common/src/main/java/org/keycloak/common/util/HttpPostRedirect.java
@@ -72,9 +72,12 @@ public String buildHtml(String title, String actionUrl, Map<String, String> para
         builder.append("</HEAD>")
                 .append("<BODY Onload=\"document.forms[0].submit()\">")
 
-                .append("<FORM METHOD=\"POST\" ACTION=\"").append(actionUrl).append("\">");
+                .append("<FORM METHOD=\"POST\" ACTION=\"")
+                .append(HtmlUtils.escapeAttribute(actionUrl))
+                .append("\">");
         for (Map.Entry<String, String> param : params.entrySet()) {
-            builder.append("<INPUT TYPE=\"HIDDEN\" NAME=\"").append(param.getKey()).append("\"").append(" VALUE=\"").append(param.getValue()).append("\"/>");
+            builder.append("<INPUT TYPE=\"HIDDEN\" NAME=\"").append(param.getKey()).append("\"").append(" VALUE=\"")
+                    .append(HtmlUtils.escapeAttribute(param.getValue())).append("\"/>");
         }
 
 

diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/OIDCRedirectUriBuilder.java
@@ -263,7 +263,9 @@ private Response buildFormPostResponse() {
             builder.append("  </HEAD>");
             builder.append("  <BODY Onload=\"document.forms[0].submit()\">");
 
-            builder.append("    <FORM METHOD=\"POST\" ACTION=\"" + redirectUri.toString() + "\">");
+            builder.append("    <FORM METHOD=\"POST\" ACTION=\"")
+                    .append(HtmlUtils.escapeAttribute(redirectUri.toString()))
+                    .append("\">");
 
             builder.append("  <INPUT TYPE=\"HIDDEN\" NAME=\"response\" VALUE=\"")
                     .append(HtmlUtils.escapeAttribute(session.tokens().encodeAndEncrypt(responseJWT)))

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
@@ -195,36 +195,28 @@ private static String getNormalizedRedirectUri(URI uri) {
         return redirectUri;
     }
 
-    // Decode redirectUri. We don't decode query and fragment as those can be encoded in the original URL.
+    // Decode redirectUri. Only path is decoded as other elements can be encoded in the original URL or cannot be encoded at all.
     // 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) {
         if (redirectUri == null) return null;
         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, false).preserveDefaultPort();
-            String origQuery = uriBuilder.getQuery();
-            String origFragment = uriBuilder.getFragment();
-            String origUserInfo = uriBuilder.getUserInfo();
-            String encodedRedirectUri = uriBuilder
-                    .replaceQuery(null)
-                    .fragment(null)
-                    .userInfo(null)
-                    .buildAsString();
-            String decodedRedirectUri = null;
+            if (uriBuilder.getPath() == null) {
+                return redirectUri;
+            }
+            String encodedPath = uriBuilder.getPath();
+            String decodedPath;
 
             for (int i = 0; i < MAX_DECODING_COUNT; i++) {
-                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, false).preserveDefaultPort()
-                            .replaceQuery(origQuery)
-                            .fragment(origFragment)
-                            .userInfo(origUserInfo)
-                            .buildAsString();
+                decodedPath = Encode.decode(encodedPath);
+                if (decodedPath.equals(encodedPath)) {
+                    // URL path is decoded. We can return it in the original redirect URI
+                    return uriBuilder.replacePath(decodedPath, false).buildAsString();
                 } else {
                     // Next attempt
-                    encodedRedirectUri = decodedRedirectUri;
+                    encodedPath = decodedPath;
                 }
             }
         } catch (IllegalArgumentException iae) {

diff --git a/services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java b/services/src/test/java/org/keycloak/protocol/oidc/utils/RedirectUtilsTest.java
@@ -194,4 +194,28 @@ public void testUserInfo() {
         Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://test@other.com", set, false));
         Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://test.com@other.com", set, false));
     }
+
+    @Test
+    public void testEncodedRedirectUri() {
+        Set<String> set = Stream.of(
+                "https://keycloak.org/test/*"
+        ).collect(Collectors.toSet());
+
+        Assert.assertEquals("https://keycloak.org/test/index.html", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/index.html", set, false));
+        Assert.assertEquals("https://keycloak.org/test?encodeTest=a%3Cb", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test?encodeTest=a%3Cb", set, false));
+        Assert.assertEquals("https://keycloak.org/test?encodeTest=a%3Cb#encode2=a%3Cb", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test?encodeTest=a%3Cb#encode2=a%3Cb", set, false));
+        Assert.assertEquals("https://keycloak.org/test/#encode2=a%3Cb", RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/#encode2=a%3Cb", set, false));
+
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/../", set, false));  // direct
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%2E%2E/", set, false));  // encoded
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test%2F%2E%2E%2F", set, false)); // encoded
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%252E%252E/", set, false)); // double-encoded
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%252E%252E/?some_query_param=some_value", set, false)); // double-encoded
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%252E%252E/#encodeTest=a%3Cb", set, false)); // double-encoded
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%25252E%25252E/", set, false)); // triple-encoded
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org/test/%2525252525252E%2525252525252E/", set, false)); // seventh-encoded
+
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak%2Eorg/test/", set, false));
+        Assert.assertNull(RedirectUtils.verifyRedirectUri(session, null, "https://keycloak.org%2Ftest%2F%40sample.com", set, false));
+    }
 }
diff --git a/...quillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java b/...quillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AuthorizationCodeTest.java
@@ -16,8 +16,6 @@
  */
 package org.keycloak.testsuite.oauth;
 
-import org.hamcrest.MatcherAssert;
-import org.hamcrest.Matchers;
 import org.jboss.arquillian.graphene.page.Page;
 import org.junit.Assert;
 import org.junit.Before;
@@ -29,7 +27,10 @@
 import org.keycloak.events.Errors;
 import org.keycloak.events.EventType;
 import org.keycloak.models.Constants;
+import org.keycloak.models.utils.KeycloakModelUtils;
+import org.keycloak.protocol.oidc.OIDCLoginProtocol;
 import org.keycloak.protocol.oidc.utils.OIDCResponseMode;
+import org.keycloak.representations.AuthorizationResponseToken;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.testsuite.AbstractKeycloakTest;
 import org.keycloak.testsuite.AssertEvents;
@@ -260,17 +261,57 @@ public void authorizationRequestFormPostResponseModeHTMLEntitiesRedirectUri() th
                 .update()) {
             oauth.responseMode(OIDCResponseMode.FORM_POST.value());
             oauth.responseType(OAuth2Constants.CODE);
-            oauth.redirectUri("/test?p=&gt;"); // set HTML entity &gt;
+            final String redirectUri = oauth.getRedirectUri() + "?p=&gt;"; // set HTML entity &gt;
+            oauth.redirectUri(redirectUri);
+            oauth.stateParamHardcoded(KeycloakModelUtils.generateId());
             oauth.doLogin("test-user@localhost", "password");
 
             WaitUtils.waitForPageToLoad();
             // if not properly encoded %3E would be received instead of &gt;
-            MatcherAssert.assertThat("Redirect page was not encoded", oauth.getDriver().getCurrentUrl(), Matchers.endsWith("/test?p=&gt;"));
+            Assert.assertEquals("Redirect page was not encoded", redirectUri, oauth.getDriver().getCurrentUrl());
+            String state = driver.findElement(By.id("state")).getText();
+            Assert.assertEquals(oauth.getState(), state);
+            Assert.assertNotNull(driver.findElement(By.id("code")).getText());
 
             events.expect(EventType.LOGIN)
                     .user(AssertEvents.isUUID())
                     .session(AssertEvents.isUUID())
                     .detail(Details.USERNAME, "test-user@localhost")
+                    .detail(OIDCLoginProtocol.RESPONSE_MODE_PARAM, OIDCResponseMode.FORM_POST.name().toLowerCase())
+                    .detail(OAuth2Constants.REDIRECT_URI, redirectUri)
+                    .assertEvent();
+        }
+    }
+
+    @Test
+    public void authorizationRequestFormPostJwtResponseModeHTMLEntitiesRedirectUri() throws IOException {
+        try (var c = ClientAttributeUpdater.forClient(adminClient, "test", "test-app")
+                .setRedirectUris(Collections.singletonList("*"))
+                .update()) {
+            oauth.responseMode(OIDCResponseMode.FORM_POST_JWT.value());
+            oauth.responseType(OAuth2Constants.CODE);
+            final String redirectUri = oauth.getRedirectUri() + "?p=&gt;"; // set HTML entity &gt;
+            oauth.redirectUri(redirectUri);
+            oauth.stateParamHardcoded(KeycloakModelUtils.generateId());
+            oauth.doLogin("test-user@localhost", "password");
+
+            WaitUtils.waitForPageToLoad();
+            // if not properly encoded %3E would be received instead of &gt;
+            Assert.assertEquals("Redirect page was not encoded", redirectUri, oauth.getDriver().getCurrentUrl());
+            String responseTokenEncoded = driver.findElement(By.id("response")).getText();
+            AuthorizationResponseToken responseToken = oauth.verifyAuthorizationResponseToken(responseTokenEncoded);
+            assertEquals("test-app", responseToken.getAudience()[0]);
+            Assert.assertNotNull(responseToken.getOtherClaims().get("code"));
+            Assert.assertNull(responseToken.getOtherClaims().get("error"));
+            Assert.assertEquals(oauth.getState(), responseToken.getOtherClaims().get("state"));
+            Assert.assertNotNull(responseToken.getOtherClaims().get("code"));
+
+            events.expect(EventType.LOGIN)
+                    .user(AssertEvents.isUUID())
+                    .session((String) responseToken.getOtherClaims().get("session_state"))
+                    .detail(Details.USERNAME, "test-user@localhost")
+                    .detail(OIDCLoginProtocol.RESPONSE_MODE_PARAM, OIDCResponseMode.FORM_POST_JWT.name().toLowerCase())
+                    .detail(OAuth2Constants.REDIRECT_URI, redirectUri)
                     .assertEvent();
         }
     }