From 0cd594082e0beb5346eb0e2c94e8e82c71c11f1e Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Sun, 19 Feb 2023 05:23:59 -0500 Subject: [PATCH] Polish authorization error response encoding Issue gh-1011 --- .../OAuth2AuthorizationEndpointFilter.java | 14 +++++---- .../OAuth2AuthorizationCodeGrantTests.java | 31 +++++++++++++++++++ ...Auth2AuthorizationEndpointFilterTests.java | 18 ++++++----- 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java index 76bcbe6a..08b8aa5f 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java @@ -298,13 +298,12 @@ public final class OAuth2AuthorizationEndpointFilter extends OncePerRequestFilte UriComponentsBuilder uriBuilder = UriComponentsBuilder .fromUriString(authorizationCodeRequestAuthentication.getRedirectUri()) .queryParam(OAuth2ParameterNames.CODE, authorizationCodeRequestAuthentication.getAuthorizationCode().getTokenValue()); - String redirectUri; if (StringUtils.hasText(authorizationCodeRequestAuthentication.getState())) { uriBuilder.queryParam( OAuth2ParameterNames.STATE, UriUtils.encode(authorizationCodeRequestAuthentication.getState(), StandardCharsets.UTF_8)); } - redirectUri = uriBuilder.build(true).toUriString(); // build(true) -> Components are explicitly encoded + String redirectUri = uriBuilder.build(true).toUriString(); // build(true) -> Components are explicitly encoded this.redirectStrategy.sendRedirect(request, response, redirectUri); } @@ -331,18 +330,21 @@ public final class OAuth2AuthorizationEndpointFilter extends OncePerRequestFilte .fromUriString(authorizationCodeRequestAuthentication.getRedirectUri()) .queryParam(OAuth2ParameterNames.ERROR, error.getErrorCode()); if (StringUtils.hasText(error.getDescription())) { - uriBuilder.queryParam(OAuth2ParameterNames.ERROR_DESCRIPTION, error.getDescription()); + uriBuilder.queryParam( + OAuth2ParameterNames.ERROR_DESCRIPTION, + UriUtils.encode(error.getDescription(), StandardCharsets.UTF_8)); } if (StringUtils.hasText(error.getUri())) { - uriBuilder.queryParam(OAuth2ParameterNames.ERROR_URI, error.getUri()); + uriBuilder.queryParam( + OAuth2ParameterNames.ERROR_URI, + UriUtils.encode(error.getUri(), StandardCharsets.UTF_8)); } - String redirectUri; if (StringUtils.hasText(authorizationCodeRequestAuthentication.getState())) { uriBuilder.queryParam( OAuth2ParameterNames.STATE, UriUtils.encode(authorizationCodeRequestAuthentication.getState(), StandardCharsets.UTF_8)); } - redirectUri = uriBuilder.build(true).toUriString(); // build(true) -> Components are explicitly encoded + String redirectUri = uriBuilder.build(true).toUriString(); // build(true) -> Components are explicitly encoded this.redirectStrategy.sendRedirect(request, response, redirectUri); } diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OAuth2AuthorizationCodeGrantTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OAuth2AuthorizationCodeGrantTests.java index 7f75750d..95d71a7d 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OAuth2AuthorizationCodeGrantTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/config/annotation/web/configurers/OAuth2AuthorizationCodeGrantTests.java @@ -127,6 +127,7 @@ import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; +import org.springframework.web.util.UriUtils; import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.CoreMatchers.containsString; @@ -455,6 +456,36 @@ public class OAuth2AuthorizationCodeGrantTests { .andExpect(status().isBadRequest()); } + // gh-1011 + @Test + public void requestWhenConfidentialClientWithPkceAndMissingCodeChallengeThenErrorResponseEncoded() throws Exception { + this.spring.register(AuthorizationServerConfiguration.class).autowire(); + + String redirectUri = "https://example.com/callback-1?param=encoded%20parameter%20value"; + RegisteredClient registeredClient = TestRegisteredClients.registeredClient() + .redirectUris(redirectUris -> { + redirectUris.clear(); + redirectUris.add(redirectUri); + }) + .clientSettings(ClientSettings.builder().requireProofKey(true).build()) + .build(); + this.registeredClientRepository.save(registeredClient); + + MultiValueMap authorizationRequestParameters = getAuthorizationRequestParameters(registeredClient); + MvcResult mvcResult = this.mvc.perform(get(DEFAULT_AUTHORIZATION_ENDPOINT_URI) + .params(authorizationRequestParameters) + .with(user("user"))) + .andExpect(status().is3xxRedirection()) + .andReturn(); + String redirectedUrl = mvcResult.getResponse().getRedirectedUrl(); + String expectedRedirectUri = redirectUri + "&" + + "error=invalid_request&" + + "error_description=" + UriUtils.encode("OAuth 2.0 Parameter: code_challenge", StandardCharsets.UTF_8) + "&" + + "error_uri=" + UriUtils.encode("https://datatracker.ietf.org/doc/html/rfc7636#section-4.4.1", StandardCharsets.UTF_8) + "&" + + "state=" + STATE_URL_ENCODED; + assertThat(redirectedUrl).isEqualTo(expectedRedirectUri); + } + @Test public void requestWhenCustomTokenGeneratorThenUsed() throws Exception { this.spring.register(AuthorizationServerConfigurationWithTokenGenerator.class).autowire(); diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java index 3597131d..6dfd58ce 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java @@ -280,12 +280,17 @@ public class OAuth2AuthorizationEndpointFilterTests { @Test public void doFilterWhenAuthorizationRequestAuthenticationExceptionThenErrorResponse() throws Exception { - RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build(); + RegisteredClient registeredClient = TestRegisteredClients.registeredClient() + .redirectUris(redirectUris -> { + redirectUris.clear(); + redirectUris.add("https://example.com?param=encoded%20parameter%20value"); + }) + .build(); OAuth2AuthorizationCodeRequestAuthenticationToken authorizationCodeRequestAuthentication = new OAuth2AuthorizationCodeRequestAuthenticationToken( AUTHORIZATION_URI, registeredClient.getClientId(), principal, - registeredClient.getRedirectUris().iterator().next(), STATE, registeredClient.getScopes(), null); - OAuth2Error error = new OAuth2Error("errorCode", "errorDescription", "errorUri"); + registeredClient.getRedirectUris().iterator().next(), "client state", registeredClient.getScopes(), null); + OAuth2Error error = new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST, "error description", "error uri"); when(this.authenticationManager.authenticate(any())) .thenThrow(new OAuth2AuthorizationCodeRequestAuthenticationException(error, authorizationCodeRequestAuthentication)); @@ -300,8 +305,7 @@ public class OAuth2AuthorizationEndpointFilterTests { assertThat(response.getStatus()).isEqualTo(HttpStatus.FOUND.value()); assertThat(response.getRedirectedUrl()).isEqualTo( - request.getParameter(OAuth2ParameterNames.REDIRECT_URI) + - "?error=errorCode&error_description=errorDescription&error_uri=errorUri&state=state"); + "https://example.com?param=encoded%20parameter%20value&error=invalid_request&error_description=error%20description&error_uri=error%20uri&state=client%20state"); assertThat(SecurityContextHolder.getContext().getAuthentication()).isSameAs(this.principal); } @@ -546,7 +550,7 @@ public class OAuth2AuthorizationEndpointFilterTests { OAuth2AuthorizationCodeRequestAuthenticationToken authorizationCodeRequestAuthenticationResult = new OAuth2AuthorizationCodeRequestAuthenticationToken( AUTHORIZATION_URI, registeredClient.getClientId(), principal, this.authorizationCode, - registeredClient.getRedirectUris().iterator().next(), STATE, registeredClient.getScopes()); + registeredClient.getRedirectUris().iterator().next(), "client state", registeredClient.getScopes()); authorizationCodeRequestAuthenticationResult.setAuthenticated(true); when(this.authenticationManager.authenticate(any())) .thenReturn(authorizationCodeRequestAuthenticationResult); @@ -568,7 +572,7 @@ public class OAuth2AuthorizationEndpointFilterTests { .isEqualTo(REMOTE_ADDRESS); assertThat(response.getStatus()).isEqualTo(HttpStatus.FOUND.value()); assertThat(response.getRedirectedUrl()).isEqualTo( - "https://example.com?param=encoded%20parameter%20value&code=code&state=state"); + "https://example.com?param=encoded%20parameter%20value&code=code&state=client%20state"); } @Test