Do not allow query string parameters to /oauth/token

[#137401169] https://www.pivotaltracker.com/story/show/137401169

server/src/main/java/org/cloudfoundry/identity/uaa/oauth/token/UaaTokenEndpoint.java

@@ -26,11 +26,14 @@
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RequestParam;
 
+import javax.servlet.http.HttpServletRequest;
 import java.security.Principal;
 import java.util.Map;
 import java.util.Set;
 
 import static java.util.Collections.singleton;
+import static org.springframework.http.HttpStatus.NOT_ACCEPTABLE;
+import static org.springframework.util.StringUtils.hasText;
 import static org.springframework.web.bind.annotation.RequestMethod.GET;
 import static org.springframework.web.bind.annotation.RequestMethod.POST;
 
@@ -47,7 +50,12 @@ public ResponseEntity<OAuth2AccessToken> doDelegateGet(Principal principal,
 
     @RequestMapping(value = "**", method = POST)
     public ResponseEntity<OAuth2AccessToken> doDelegatePost(Principal principal,
-                                                            @RequestParam Map<String, String> parameters) throws HttpRequestMethodNotSupportedException {
+                                                            @RequestParam Map<String, String> parameters,
+                                                            HttpServletRequest request) throws HttpRequestMethodNotSupportedException {
+        if (hasText(request.getQueryString())) {
+            logger.debug("Call to /oauth/token contains a query string. Aborting.");
+            throw new HttpRequestMethodNotSupportedException("POST");
+        }
         return super.postAccessToken(principal, parameters);
     }
 
@@ -56,6 +64,25 @@ public void setAllowedRequestMethods(Set<HttpMethod> allowedRequestMethods) {
         super.setAllowedRequestMethods(singleton(HttpMethod.POST));
     }
 
+    @ExceptionHandler(HttpRequestMethodNotSupportedException.class)
+    @Override
+    public ResponseEntity<OAuth2Exception> handleHttpRequestMethodNotSupportedException(HttpRequestMethodNotSupportedException e) throws Exception {
+        ResponseEntity<OAuth2Exception> result =  super.handleHttpRequestMethodNotSupportedException(e);
+        if (HttpMethod.POST.matches(e.getMethod())) {
+            OAuth2Exception cause = new OAuth2Exception("Parameters must be passed in the body of the request", result.getBody().getCause()) {
+                public String getOAuth2ErrorCode() {
+                    return "query_string_not_allowed";
+                }
+
+                public int getHttpErrorCode() {
+                    return NOT_ACCEPTABLE.value();
+                }
+            };
+            result = new ResponseEntity<>(cause, result.getHeaders(), NOT_ACCEPTABLE);
+        }
+        return result;
+    }
+
     @ExceptionHandler(Exception.class)
     @Override
     public ResponseEntity<OAuth2Exception> handleException(Exception e) throws Exception {

server/src/test/java/org/cloudfoundry/identity/uaa/oauth/token/UaaTokenEndpointTests.java

@@ -18,6 +18,8 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.springframework.http.HttpMethod;
+import org.springframework.http.ResponseEntity;
+import org.springframework.security.oauth2.common.exceptions.OAuth2Exception;
 import org.springframework.test.util.ReflectionTestUtils;
 import org.springframework.web.HttpRequestMethodNotSupportedException;
 
@@ -32,6 +34,7 @@
 import static org.mockito.Mockito.mock;
 import static org.springframework.http.HttpMethod.GET;
 import static org.springframework.http.HttpMethod.POST;
+import static org.springframework.http.HttpStatus.NOT_ACCEPTABLE;
 
 
 public class UaaTokenEndpointTests {
@@ -55,10 +58,35 @@ public void setAllowedRequestMethods() throws Exception {
     }
 
     @Test(expected = HttpRequestMethodNotSupportedException.class)
-    public void call_to_get_always_throws() throws Exception {
+    public void call_to_get_always_throws_super_method() throws Exception {
         UaaTokenEndpoint endpoint = new UaaTokenEndpoint();
         endpoint.setAllowedRequestMethods(allowedRequestMethods);
-        endpoint.getAccessToken(mock(Principal.class), emptyMap());
+        try {
+            endpoint.getAccessToken(mock(Principal.class), emptyMap());
+        } catch (HttpRequestMethodNotSupportedException e) {
+            assertEquals("GET", e.getMethod());
+            throw e;
+        }
+    }
+
+
+    @Test(expected = HttpRequestMethodNotSupportedException.class)
+    public void call_to_get_always_throws_override_method() throws Exception {
+        UaaTokenEndpoint endpoint = new UaaTokenEndpoint();
+        endpoint.setAllowedRequestMethods(allowedRequestMethods);
+        try {
+            endpoint.doDelegateGet(mock(Principal.class), emptyMap());
+        } catch (HttpRequestMethodNotSupportedException e) {
+            assertEquals("GET", e.getMethod());
+            throw e;
+        }
+    }
+
+    @Test
+    public void call_to_post_with_query_string_throws_not_acceptable() throws Exception {
+        ResponseEntity<OAuth2Exception> result = endpoint.handleHttpRequestMethodNotSupportedException(new HttpRequestMethodNotSupportedException("POST"));
+        assertNotNull(result);
+        assertEquals(NOT_ACCEPTABLE, result.getStatusCode());
     }
 
 }

uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/token/TokenMvcMockTests.java

@@ -211,14 +211,33 @@ public void token_endpoint_post() throws Exception {
         try_token_with_non_post(post("/oauth/token"), status().isOk());
     }
 
-    public void try_token_with_non_post(MockHttpServletRequestBuilder builder, ResultMatcher status) throws Exception {
+    @Test
+    public void token_endpoint_post_query_string() throws Exception {
+        String username = setUpUserForPasswordGrant();
+
+        getMockMvc().perform(
+            post("/oauth/token?client_id=cf&client_secret=&grant_type=password&username={username}&password=secret", username)
+                .accept(APPLICATION_JSON)
+                .contentType(APPLICATION_FORM_URLENCODED))
+            .andDo(print())
+            .andExpect(status().isNotAcceptable())
+            .andExpect(content().string(containsString("query_string_not_allowed")))
+            .andExpect(content().string(containsString("Parameters must be passed in the body of the request")));
+    }
+
+    public String setUpUserForPasswordGrant() throws Exception {
         String username = "testuser" + generator.generate();
         String userScopes = "uaa.user";
         ScimUser user = setUpUser(username, userScopes, OriginKeys.UAA, IdentityZone.getUaa().getId());
         ScimUserProvisioning provisioning = getWebApplicationContext().getBean(ScimUserProvisioning.class);
         ScimUser scimUser = provisioning.retrieve(user.getId());
         assertNull(scimUser.getLastLogonTime());
         assertNull(scimUser.getPreviousLogonTime());
+        return username;
+    }
+
+    public void try_token_with_non_post(MockHttpServletRequestBuilder builder, ResultMatcher status) throws Exception {
+        String username = setUpUserForPasswordGrant();
 
         getMockMvc().perform(
             builder