Handle ClientData parsing errors in SessionCodeChecks gracefully

- Move ClientData parsing out of SessionCodeChecks ctor - Respond with a bad request if invalid client data is presented Closes keycloak#32515 Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com> Signed-off-by: Alexander Schwartz <aschwart@redhat.com> Co-authored-by: Alexander Schwartz <aschwart@redhat.com>
randreoni · Sep 5, 2024 · 693a63b · 693a63b
1 parent 83a5789
commit 693a63b
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 17 deletions.
diff --git a/server-spi-private/src/main/java/org/keycloak/protocol/ClientData.java b/server-spi-private/src/main/java/org/keycloak/protocol/ClientData.java
@@ -96,17 +96,12 @@ public String toString() {
         return String.format("ClientData [ redirectUri=%s, responseType=%s, responseMode=%s, state=%s ]", redirectUri, responseType, responseMode, state);
     }
 
-    public static ClientData decodeClientDataFromParameter(String clientDataParam) {
-        try {
-            if (ObjectUtil.isBlank(clientDataParam)) {
-                return null;
-            } else {
-                byte[] cdataJson = Base64Url.decode(clientDataParam);
-                return JsonSerialization.readValue(cdataJson, ClientData.class);
-            }
-        } catch (IOException ioe) {
-            logger.warnf("ClientData parameter in invalid format. ClientData parameter was %s", clientDataParam);
+    public static ClientData decodeClientDataFromParameter(String clientDataParam) throws IOException {
+        if (ObjectUtil.isBlank(clientDataParam)) {
             return null;
+        } else {
+            byte[] cdataJson = Base64Url.decode(clientDataParam);
+            return JsonSerialization.readValue(cdataJson, ClientData.class);
         }
     }
 

diff --git a/...-spi-private/src/test/java/org/keycloak/broker/provider/util/IdentityBrokerStateTest.java b/...-spi-private/src/test/java/org/keycloak/broker/provider/util/IdentityBrokerStateTest.java
@@ -6,6 +6,8 @@
 import org.keycloak.models.RealmModel;
 import org.keycloak.protocol.ClientData;
 
+import java.io.IOException;
+
 
 public class IdentityBrokerStateTest {
 
@@ -48,7 +50,7 @@ public void testDecodedWithClientIdAnActualUuid() {
     }
 
     @Test
-    public void testDecodedWithClientIdAnActualUuidBASE64UriFriendly() {
+    public void testDecodedWithClientIdAnActualUuidBASE64UriFriendly() throws IOException {
 
         // Given
         String state = "gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk";
@@ -90,7 +92,7 @@ public void testEncodedWithClientIdUUid() {
     }
 
     @Test
-    public void testEncodedWithClientIdNotUUid() {
+    public void testEncodedWithClientIdNotUUid() throws IOException {
         // Given
         String encoded = "gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk.vpISZLVDAc0.aHR0cDovL2kuYW0uYW4udXJs";
         String clientId = "http://i.am.an.url";
@@ -107,7 +109,7 @@ public void testEncodedWithClientIdNotUUid() {
     }
 
     @Test
-    public void testEncodedWithClientData() {
+    public void testEncodedWithClientData() throws IOException {
         // Given
         String encoded = "gNrGamIDGKpKSI9yOrcFzYTKoFGH779_WNCacAelkhk.vpISZLVDAc0.aHR0cDovL2kuYW0uYW4udXJs.eyJydSI6Imh0dHBzOi8vbXktcmVkaXJlY3QtdXJpIiwicnQiOiJjb2RlIiwicm0iOiJxdWVyeSIsInN0Ijoic29tZS1zdGF0ZSJ9";
         String clientId = "http://i.am.an.url";

diff --git a/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java b/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java
@@ -19,6 +19,7 @@
 
 import static org.keycloak.services.managers.AuthenticationManager.authenticateIdentityCookie;
 
+import java.io.IOException;
 import java.net.URI;
 
 import jakarta.ws.rs.core.Response;
@@ -42,8 +43,6 @@
 import org.keycloak.protocol.ClientData;
 import org.keycloak.protocol.LoginProtocol;
 import org.keycloak.protocol.RestartLoginCookie;
-import org.keycloak.protocol.oidc.endpoints.AuthorizationEndpointChecker;
-import org.keycloak.protocol.oidc.endpoints.request.AuthorizationEndpointRequest;
 import org.keycloak.services.ErrorPage;
 import org.keycloak.services.ServicesLogger;
 import org.keycloak.services.managers.AuthenticationManager;
@@ -76,7 +75,7 @@ public class SessionCodeChecks {
     private final String code;
     private final String execution;
     private final String clientId;
-    private final ClientData clientData;
+    private final String clientDataString;
     private final String tabId;
     private final String flowPath;
     private final String authSessionId;
@@ -97,7 +96,7 @@ public SessionCodeChecks(RealmModel realm, UriInfo uriInfo, HttpRequest request,
         this.tabId = tabId;
         this.flowPath = flowPath;
         this.authSessionId = authSessionId;
-        this.clientData = ClientData.decodeClientDataFromParameter(clientData);
+        this.clientDataString = clientData;
     }
 
 
@@ -174,6 +173,17 @@ public AuthenticationSessionModel initialVerifyAuthSession() {
 
         }
 
+        ClientData clientData;
+        try {
+            clientData = ClientData.decodeClientDataFromParameter(clientDataString);
+        } catch (RuntimeException | IOException e) {
+            logger.debugf(e, "ClientData parameter in invalid format. ClientData parameter was %s", clientDataString);
+            event.detail(Details.REASON, "Invalid client data: " + e.getMessage());
+            event.error(Errors.INVALID_REQUEST);
+            response = ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST);
+            return null;
+        }
+
         if (authSession != null) {
             session.getProvider(LoginFormsProvider.class).setAuthenticationSession(authSession);
             return authSession;