KEYCLOAK-11996 Authorization Endpoint does not return an error when a request includes a parameter more than once (#6696)

Co-authored-by: stianst <stianst@gmail.com>

Co-authored-by: Takashi Norimatsu <takashi.norimatsu.ws@hitachi.com>

services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java

@@ -55,6 +55,7 @@ import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
 
+import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -111,7 +112,7 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase {
     }
 
     private Response process(MultivaluedMap<String, String> params) {
-        String clientId = params.getFirst(OIDCLoginProtocol.CLIENT_ID_PARAM);
+        String clientId = AuthorizationEndpointRequestParserProcessor.getClientId(event, session, params);
 
         checkSsl();
         checkRealm();
@@ -125,6 +126,11 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase {
             return errorResponse;
         }
 
+        if (request.getInvalidRequestMessage() != null) {
+            event.error(Errors.INVALID_REQUEST);
+            return redirectErrorToClient(parsedResponseMode, Errors.INVALID_REQUEST, request.getInvalidRequestMessage());
+        }
+
         if (!TokenUtil.isOIDCRequest(request.getScope())) {
             ServicesLogger.LOGGER.oidcScopeMissing();
         }

services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequest.java

@@ -25,6 +25,8 @@ import java.util.Map;
  */
 public class AuthorizationEndpointRequest {
 
+    String invalidRequestMessage;
+
     String clientId;
     String redirectUriParam;
     String responseType;
@@ -120,4 +122,8 @@ public class AuthorizationEndpointRequest {
     public String getDisplay() {
         return display;
     }
+
+    public String getInvalidRequestMessage() {
+        return invalidRequestMessage;
+    }
 }

services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthorizationEndpointRequestParserProcessor.java

@@ -33,6 +33,7 @@ import org.keycloak.services.messages.Messages;
 import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
 import java.io.InputStream;
+import java.util.List;
 
 /**
  * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
@@ -43,7 +44,13 @@ public class AuthorizationEndpointRequestParserProcessor {
         try {
             AuthorizationEndpointRequest request = new AuthorizationEndpointRequest();
 
-            new AuthzEndpointQueryStringParser(requestParams).parseRequest(request);
+            AuthzEndpointQueryStringParser parser = new AuthzEndpointQueryStringParser(requestParams);
+            parser.parseRequest(request);
+
+            if (parser.getInvalidRequestMessage() != null) {
+                request.invalidRequestMessage = parser.getInvalidRequestMessage();
+                return request;
+            }
 
             String requestParam = requestParams.getFirst(OIDCLoginProtocol.REQUEST_PARAM);
             String requestUriParam = requestParams.getFirst(OIDCLoginProtocol.REQUEST_URI_PARAM);
@@ -83,4 +90,15 @@ public class AuthorizationEndpointRequestParserProcessor {
             throw new ErrorPageException(session, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST);
         }
     }
+
+    public static String getClientId(EventBuilder event, KeycloakSession session, MultivaluedMap<String, String> requestParams) {
+        List<String> clientParam = requestParams.get(OIDCLoginProtocol.CLIENT_ID_PARAM);
+        if (clientParam != null && clientParam.size() == 1) {
+            return clientParam.get(0);
+        } else {
+            event.error(Errors.INVALID_REQUEST);
+            throw new ErrorPageException(session, Response.Status.BAD_REQUEST, Messages.INVALID_REQUEST);
+        }
+    }
+
 }

services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointQueryStringParser.java

@@ -18,6 +18,7 @@
 package org.keycloak.protocol.oidc.endpoints.request;
 
 import javax.ws.rs.core.MultivaluedMap;
+
 import java.util.Set;
 
 /**
@@ -29,23 +30,40 @@ class AuthzEndpointQueryStringParser extends AuthzEndpointRequestParser {
 
     private final MultivaluedMap<String, String> requestParams;
 
+    private String invalidRequestMessage = null;
+
     public AuthzEndpointQueryStringParser(MultivaluedMap<String, String> requestParams) {
         this.requestParams = requestParams;
     }
 
     @Override
     protected String getParameter(String paramName) {
+        checkDuplicated(requestParams, paramName);
         return requestParams.getFirst(paramName);
     }
 
     @Override
     protected Integer getIntParameter(String paramName) {
+        checkDuplicated(requestParams, paramName);
         String paramVal = requestParams.getFirst(paramName);
         return paramVal==null ? null : Integer.parseInt(paramVal);
     }
 
+    public String getInvalidRequestMessage() {
+        return invalidRequestMessage;
+    }
+
     @Override
     protected Set<String> keySet() {
         return requestParams.keySet();
     }
+
+    private void checkDuplicated(MultivaluedMap<String, String> requestParams, String paramName) {
+        if (invalidRequestMessage == null) {
+            if (requestParams.get(paramName) != null && requestParams.get(paramName).size() != 1) {
+                invalidRequestMessage = "duplicated parameter";
+            }
+        }
+    }
+
 }

services/src/main/java/org/keycloak/protocol/oidc/endpoints/request/AuthzEndpointRequestParser.java

@@ -70,10 +70,8 @@ abstract class AuthzEndpointRequestParser {
         // https://tools.ietf.org/html/rfc7636#section-6.1
         KNOWN_REQ_PARAMS.add(OIDCLoginProtocol.CODE_CHALLENGE_PARAM);
         KNOWN_REQ_PARAMS.add(OIDCLoginProtocol.CODE_CHALLENGE_METHOD_PARAM);
-
     }
 
-
     public void parseRequest(AuthorizationEndpointRequest request) {
         String clientId = getParameter(OIDCLoginProtocol.CLIENT_ID_PARAM);
 
@@ -104,7 +102,6 @@ abstract class AuthzEndpointRequestParser {
         extractAdditionalReqParams(request.additionalReqParams);
     }
 
-
     protected void extractAdditionalReqParams(Map<String, String> additionalReqParams) {
         for (String paramName : keySet()) {
             if (!KNOWN_REQ_PARAMS.contains(paramName)) {
@@ -130,7 +127,6 @@ abstract class AuthzEndpointRequestParser {
         return newVal==null ? previousVal : newVal;
     }
 
-
     protected abstract String getParameter(String paramName);
 
     protected abstract Integer getIntParameter(String paramName);

testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java

@@ -157,6 +157,8 @@ public class OAuthClient {
     private String codeChallengeMethod;
     private String origin;
 
+    private Map<String, String> customParameters;
+
     private boolean openid = true;
 
     private Supplier<CloseableHttpClient> httpClient = OAuthClient::newCloseableHttpClient;
@@ -222,6 +224,7 @@ public class OAuthClient {
         codeChallenge = null;
         codeChallengeMethod = null;
         origin = null;
+        customParameters = null;
         openid = true;
     }
 
@@ -884,7 +887,11 @@ public class OAuthClient {
         }
         if (codeChallengeMethod != null) {
             b.queryParam(OAuth2Constants.CODE_CHALLENGE_METHOD, codeChallengeMethod);
-        }  
+        }
+        if (customParameters != null) {
+            customParameters.keySet().stream().forEach(i -> b.queryParam(i, customParameters.get(i)));
+        }
+
         return b.build(realm).toString();
     }
 
@@ -1053,6 +1060,14 @@ public class OAuthClient {
         return this;
     }
 
+    public OAuthClient addCustomerParameter(String key, String value) {
+        if (customParameters == null) {
+            customParameters = new HashMap<>();
+        }
+        customParameters.put(key, value);
+        return this;
+    }
+
     public static class AuthorizationEndpointResponse {
 
         private boolean isRedirected;

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

@@ -16,13 +16,13 @@
  */
 package org.keycloak.testsuite.oauth;
 
+import org.jboss.arquillian.graphene.page.Page;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.keycloak.OAuth2Constants;
 import org.keycloak.OAuthErrorException;
-import org.keycloak.connections.infinispan.InfinispanConnectionProvider;
 import org.keycloak.events.Details;
 import org.keycloak.events.Errors;
 import org.keycloak.models.Constants;
@@ -30,6 +30,7 @@ import org.keycloak.protocol.oidc.utils.OIDCResponseMode;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.testsuite.AbstractKeycloakTest;
 import org.keycloak.testsuite.AssertEvents;
+import org.keycloak.testsuite.pages.ErrorPage;
 import org.keycloak.testsuite.pages.PageUtils;
 import org.keycloak.testsuite.util.ClientManager;
 import org.keycloak.testsuite.util.OAuthClient;
@@ -38,9 +39,12 @@ import org.openqa.selenium.By;
 import javax.ws.rs.core.UriBuilder;
 import java.io.IOException;
 import java.net.URI;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 import static org.keycloak.testsuite.admin.AbstractAdminTest.loadJson;
 
 /**
@@ -51,6 +55,9 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
     @Rule
     public AssertEvents events = new AssertEvents(this);
 
+    @Page
+    private ErrorPage errorPage;
+
     @Override
     public void addTestRealms(List<RealmRepresentation> testRealms) {
         RealmRepresentation realmRepresentation = loadJson(getClass().getResourceAsStream("/testrealm.json"), RealmRepresentation.class);
@@ -70,7 +77,7 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
 
         OAuthClient.AuthorizationEndpointResponse response = oauth.doLogin("test-user@localhost", "password");
 
-        Assert.assertTrue(response.isRedirected());
+        assertTrue(response.isRedirected());
         Assert.assertNotNull(response.getCode());
         assertEquals("OpenIdConnect.AuthenticationProperties=2302984sdlk", response.getState());
         Assert.assertNull(response.getError());
@@ -101,7 +108,7 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
 
         OAuthClient.AuthorizationEndpointResponse response = oauth.doLogin("test-user@localhost", "password");
 
-        Assert.assertTrue(response.isRedirected());
+        assertTrue(response.isRedirected());
         Assert.assertNotNull(response.getCode());
 
         String codeId = events.expectLogin().assertEvent().getDetails().get(Details.CODE_ID);
@@ -113,7 +120,7 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
 
         OAuthClient.AuthorizationEndpointResponse response = oauth.doLogin("test-user@localhost", "password");
 
-        Assert.assertTrue(response.isRedirected());
+        assertTrue(response.isRedirected());
         Assert.assertNotNull(response.getCode());
         Assert.assertNull(response.getState());
         Assert.assertNull(response.getError());
@@ -128,7 +135,7 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
         driver.navigate().to(b.build().toURL());
 
         OAuthClient.AuthorizationEndpointResponse errorResponse = new OAuthClient.AuthorizationEndpointResponse(oauth);
-        Assert.assertTrue(errorResponse.isRedirected());
+        assertTrue(errorResponse.isRedirected());
         Assert.assertEquals(errorResponse.getError(), OAuthErrorException.UNSUPPORTED_RESPONSE_TYPE);
 
         events.expectLogin().error(Errors.INVALID_REQUEST).user((String) null).session((String) null).clearDetails().detail(Details.RESPONSE_TYPE, "tokenn").assertEvent();
@@ -197,4 +204,39 @@ public class AuthorizationCodeTest extends AbstractKeycloakTest {
         Assert.assertNull(currentUri.getRawFragment());
     }
 
+    @Test
+    public void authorizationRequestParamsMoreThanOnce() throws IOException {
+        oauth.stateParamHardcoded("OpenIdConnect.AuthenticationProperties=2302984sdlk");
+        Map<String, String> extraParams = new HashMap<>();
+
+        oauth.addCustomerParameter(OAuth2Constants.SCOPE, "read_write")
+            .addCustomerParameter(OAuth2Constants.STATE, "abcdefg")
+            .addCustomerParameter(OAuth2Constants.SCOPE, "pop push");
+
+        oauth.openLoginForm();
+
+        assertEquals("invalid_request", oauth.getCurrentQuery().get("error"));
+        assertEquals("duplicated parameter", oauth.getCurrentQuery().get("error_description"));
+
+        events.expectLogin().error(Errors.INVALID_REQUEST).user((String) null).session((String) null).clearDetails().assertEvent();
+    }
+
+    @Test
+    public void authorizationRequestClientParamsMoreThanOnce() throws IOException {
+        oauth.stateParamHardcoded("OpenIdConnect.AuthenticationProperties=2302984sdlk");
+
+        oauth.addCustomerParameter(OAuth2Constants.SCOPE, "read_write")
+                .addCustomerParameter(OAuth2Constants.CLIENT_ID, "client2client")
+                .addCustomerParameter(OAuth2Constants.REDIRECT_URI, "https://www.example.com")
+                .addCustomerParameter(OAuth2Constants.STATE, "abcdefg")
+                .addCustomerParameter(OAuth2Constants.SCOPE, "pop push");
+
+        oauth.openLoginForm();
+
+        assertTrue(errorPage.isCurrent());
+        assertEquals("Invalid Request", errorPage.getError());
+
+        events.expectLogin().error(Errors.INVALID_REQUEST).user((String) null).session((String) null).client((String) null).clearDetails().assertEvent();
+    }
+    
 }