Skip to content

URL-encoded parameters in redirect URI are encoded twice #1011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

URL-encoded parameters in redirect URI are encoded twice #1011

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5063a98
Enable JUnit 5 test runner in samples
andreasf Dec 13, 2022
5eb9458
DefaultAuthorizationServerApplicationTests: check for redirect URI in…
andreasf Dec 16, 2022
1b1f4d3
Fix: do not re-encode redirect URI query parameters in authorization …
andreasf Dec 16, 2022
ca9e58c
Fix: do not re-encode redirect URI query parameters in error response
andreasf Dec 16, 2022
1dfa38c
CustomConsentAuthorizationServerTests: check redirect URI and state e…
andreasf Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions ...framework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@
import org.springframework.util.Assert;
import org.springframework.util.StringUtils;
import org.springframework.web.filter.OncePerRequestFilter;
import org.springframework.web.util.DefaultUriBuilderFactory;
import org.springframework.web.util.UriBuilder;
import org.springframework.web.util.UriBuilderFactory;
import org.springframework.web.util.UriComponentsBuilder;

/**
Expand Down Expand Up @@ -296,8 +299,8 @@ private void sendAuthorizationResponse(HttpServletRequest request, HttpServletRe

OAuth2AuthorizationCodeRequestAuthenticationToken authorizationCodeRequestAuthentication =
(OAuth2AuthorizationCodeRequestAuthenticationToken) authentication;
UriComponentsBuilder uriBuilder = UriComponentsBuilder
.fromUriString(authorizationCodeRequestAuthentication.getRedirectUri())
UriBuilder uriBuilder = valuesOnlyEncodingUriBuilderFactory()
.uriString(authorizationCodeRequestAuthentication.getRedirectUri())
.queryParam(OAuth2ParameterNames.CODE, authorizationCodeRequestAuthentication.getAuthorizationCode().getTokenValue());
String redirectUri;
if (StringUtils.hasText(authorizationCodeRequestAuthentication.getState())) {
Expand All @@ -306,7 +309,7 @@ private void sendAuthorizationResponse(HttpServletRequest request, HttpServletRe
queryParams.put(OAuth2ParameterNames.STATE, authorizationCodeRequestAuthentication.getState());
redirectUri = uriBuilder.build(queryParams).toString();
} else {
redirectUri = uriBuilder.toUriString();
redirectUri = uriBuilder.build().toString();
}
this.redirectStrategy.sendRedirect(request, response, redirectUri);
}
Expand All @@ -330,27 +333,32 @@ private void sendErrorResponse(HttpServletRequest request, HttpServletResponse r
this.logger.trace("Redirecting to client with error");
}

UriComponentsBuilder uriBuilder = UriComponentsBuilder
.fromUriString(authorizationCodeRequestAuthentication.getRedirectUri())
UriBuilder uriBuilder = valuesOnlyEncodingUriBuilderFactory()
.uriString(authorizationCodeRequestAuthentication.getRedirectUri())
.queryParam(OAuth2ParameterNames.ERROR, error.getErrorCode());
Map<String, String> queryParams = new HashMap<>();
if (StringUtils.hasText(error.getDescription())) {
uriBuilder.queryParam(OAuth2ParameterNames.ERROR_DESCRIPTION, error.getDescription());
uriBuilder.queryParam(OAuth2ParameterNames.ERROR_DESCRIPTION, "{error_description}");
queryParams.put(OAuth2ParameterNames.ERROR_DESCRIPTION, error.getDescription());
}
if (StringUtils.hasText(error.getUri())) {
uriBuilder.queryParam(OAuth2ParameterNames.ERROR_URI, error.getUri());
uriBuilder.queryParam(OAuth2ParameterNames.ERROR_URI, "{error_uri}");
queryParams.put(OAuth2ParameterNames.ERROR_URI, error.getUri());
}
String redirectUri;
if (StringUtils.hasText(authorizationCodeRequestAuthentication.getState())) {
uriBuilder.queryParam(OAuth2ParameterNames.STATE, "{state}");
Map<String, String> queryParams = new HashMap<>();
queryParams.put(OAuth2ParameterNames.STATE, authorizationCodeRequestAuthentication.getState());
redirectUri = uriBuilder.build(queryParams).toString();
} else {
redirectUri = uriBuilder.toUriString();
}
String redirectUri = uriBuilder.build(queryParams).toString();
this.redirectStrategy.sendRedirect(request, response, redirectUri);
}

private UriBuilderFactory valuesOnlyEncodingUriBuilderFactory() {
DefaultUriBuilderFactory uriBuilderFactory = new DefaultUriBuilderFactory();
uriBuilderFactory.setEncodingMode(DefaultUriBuilderFactory.EncodingMode.VALUES_ONLY);
return uriBuilderFactory;
}

/**
* For internal use only.
*/
Expand Down
12 changes: 12 additions & 0 deletions ...rg/springframework/security/oauth2/server/authorization/client/TestRegisteredClients.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,18 @@ public static RegisteredClient.Builder registeredClient() {
.scope("scope1");
}

public static RegisteredClient.Builder registeredClientEncodedUri() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this and leverage the existing TestRegisteredClients.registeredClient() and add the url-encoded redirect-uri

return RegisteredClient.withId("registration-1")
.clientId("client-1")
.clientIdIssuedAt(Instant.now().truncatedTo(ChronoUnit.SECONDS))
.clientSecret("secret-1")
.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE)
.authorizationGrantType(AuthorizationGrantType.REFRESH_TOKEN)
.clientAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC)
.redirectUri("https://example.com?param=is%2Fencoded")
.scope("scope1");
}

public static RegisteredClient.Builder registeredClient2() {
return RegisteredClient.withId("registration-2")
.clientId("client-2")
Expand Down
14 changes: 7 additions & 7 deletions ...work/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
public class OAuth2AuthorizationEndpointFilterTests {
private static final String DEFAULT_AUTHORIZATION_ENDPOINT_URI = "/oauth2/authorize";
private static final String AUTHORIZATION_URI = "https://provider.com/oauth2/authorize";
private static final String STATE = "state";
private static final String STATE = "previously encoded/state";
Copy link
Collaborator

@jgrandja jgrandja Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert all changes related to the state parameter since this was fixed in gh-875 and test coverage is included in the associated commit

private static final String REMOTE_ADDRESS = "remote-address";
private AuthenticationManager authenticationManager;
private OAuth2AuthorizationEndpointFilter filter;
Expand Down Expand Up @@ -285,7 +285,7 @@ public void doFilterWhenAuthorizationRequestAuthenticationExceptionThenErrorResp
new OAuth2AuthorizationCodeRequestAuthenticationToken(
AUTHORIZATION_URI, registeredClient.getClientId(), principal,
registeredClient.getRedirectUris().iterator().next(), STATE, registeredClient.getScopes(), null);
OAuth2Error error = new OAuth2Error("errorCode", "errorDescription", "errorUri");
OAuth2Error error = new OAuth2Error("errorCode", "errorDescription", "errorUri#section");
when(this.authenticationManager.authenticate(any()))
.thenThrow(new OAuth2AuthorizationCodeRequestAuthenticationException(error, authorizationCodeRequestAuthentication));

Expand All @@ -299,7 +299,7 @@ public void doFilterWhenAuthorizationRequestAuthenticationExceptionThenErrorResp
verifyNoInteractions(filterChain);

assertThat(response.getStatus()).isEqualTo(HttpStatus.FOUND.value());
assertThat(response.getRedirectedUrl()).isEqualTo("https://example.com?error=errorCode&error_description=errorDescription&error_uri=errorUri&state=state");
assertThat(response.getRedirectedUrl()).isEqualTo("https://example.com?error=errorCode&error_description=errorDescription&error_uri=errorUri%23section&state=previously%20encoded%2Fstate");
assertThat(SecurityContextHolder.getContext().getAuthentication()).isSameAs(this.principal);
}

Expand Down Expand Up @@ -459,7 +459,7 @@ public void doFilterWhenAuthorizationRequestConsentRequiredWithCustomConsentUriT
verifyNoInteractions(filterChain);

assertThat(response.getStatus()).isEqualTo(HttpStatus.FOUND.value());
assertThat(response.getRedirectedUrl()).isEqualTo("http://localhost/oauth2/custom-consent?scope=scope1%20scope2&client_id=client-1&state=state");
assertThat(response.getRedirectedUrl()).isEqualTo("http://localhost/oauth2/custom-consent?scope=scope1%20scope2&client_id=client-1&state=previously%20encoded/state");
}

@Test
Expand Down Expand Up @@ -535,7 +535,7 @@ public void doFilterWhenAuthorizationRequestConsentRequiredWithPreviouslyApprove

@Test
public void doFilterWhenAuthorizationRequestAuthenticatedThenAuthorizationResponse() throws Exception {
RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build();
RegisteredClient registeredClient = TestRegisteredClients.registeredClientEncodedUri().build();
OAuth2AuthorizationCodeRequestAuthenticationToken authorizationCodeRequestAuthenticationResult =
new OAuth2AuthorizationCodeRequestAuthenticationToken(
AUTHORIZATION_URI, registeredClient.getClientId(), principal, this.authorizationCode,
Expand All @@ -560,7 +560,7 @@ public void doFilterWhenAuthorizationRequestAuthenticatedThenAuthorizationRespon
.extracting(WebAuthenticationDetails::getRemoteAddress)
.isEqualTo(REMOTE_ADDRESS);
assertThat(response.getStatus()).isEqualTo(HttpStatus.FOUND.value());
assertThat(response.getRedirectedUrl()).isEqualTo("https://example.com?code=code&state=state");
assertThat(response.getRedirectedUrl()).isEqualTo("https://example.com?param=is%2Fencoded&code=code&state=previously%20encoded%2Fstate");
}

@Test
Expand Down Expand Up @@ -591,7 +591,7 @@ public void doFilterWhenAuthenticationRequestAuthenticatedThenAuthorizationRespo
verifyNoInteractions(filterChain);

assertThat(response.getStatus()).isEqualTo(HttpStatus.FOUND.value());
assertThat(response.getRedirectedUrl()).isEqualTo("https://example.com?code=code&state=state");
assertThat(response.getRedirectedUrl()).isEqualTo("https://example.com?code=code&state=previously%20encoded%2Fstate");
}

private void doFilterWhenAuthorizationRequestInvalidParameterThenError(RegisteredClient registeredClient,
Expand Down
4 changes: 4 additions & 0 deletions samples/custom-consent-authorizationserver/samples-custom-consent-authorizationserver.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ group = project.rootProject.group
version = project.rootProject.version
sourceCompatibility = "17"

test {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert all changes in custom-consent-authorizationserver since we have test coverage in default-authorizationserver and there is no need to duplicate

useJUnitPlatform()
}

repositories {
mavenCentral()
maven { url 'https://repo.spring.io/milestone' }
Expand Down
1 change: 1 addition & 0 deletions ...om-consent-authorizationserver/src/main/java/sample/config/AuthorizationServerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public RegisteredClientRepository registeredClientRepository() {
.authorizationGrantType(AuthorizationGrantType.REFRESH_TOKEN)
.authorizationGrantType(AuthorizationGrantType.CLIENT_CREDENTIALS)
.redirectUri("http://127.0.0.1:8080/login/oauth2/code/messaging-client-oidc")
.redirectUri("http://127.0.0.1:8080/login/oauth2/code/messaging-client-oidc?param=is%2Fencoded")
.redirectUri("http://127.0.0.1:8080/authorized")
.scope(OidcScopes.OPENID)
.scope(OidcScopes.PROFILE)
Expand Down
26 changes: 23 additions & 3 deletions ...nsent-authorizationserver/src/test/java/sample/CustomConsentAuthorizationServerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
package sample;

import java.io.IOException;
import java.net.URLDecoder;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.List;

Expand All @@ -36,6 +39,7 @@
import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationConsentService;
import org.springframework.security.test.context.support.WithMockUser;
import org.springframework.test.context.junit.jupiter.SpringExtension;
import org.springframework.web.util.UriComponents;
import org.springframework.web.util.UriComponentsBuilder;

import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -58,15 +62,18 @@ public class CustomConsentAuthorizationServerTests {
@MockBean
private OAuth2AuthorizationConsentService authorizationConsentService;

private final String redirectUri = "http://127.0.0.1/login/oauth2/code/messaging-client-oidc";
private final String redirectUri = "http://127.0.0.1:8080/login/oauth2/code/messaging-client-oidc?param=is%2Fencoded";
public final String state = "some+%20encoded%2Fstate";
public final String reencodedState = "some%20%20encoded%2Fstate";

private final String authorizationRequestUri = UriComponentsBuilder
.fromPath("/oauth2/authorize")
.queryParam("response_type", "code")
.queryParam("client_id", "messaging-client")
.queryParam("scope", "openid message.read message.write")
.queryParam("state", "state")
.queryParam("redirect_uri", this.redirectUri)
.queryParam("state", this.state)
.queryParam("redirect_uri", URLEncoder.encode(this.redirectUri, StandardCharsets.UTF_8))
.build()
.toUriString();

@BeforeEach
Expand All @@ -77,6 +84,15 @@ public void setUp() {
when(this.authorizationConsentService.findById(any(), any())).thenReturn(null);
}

@Test
void authorizationRequestProperlyEncoded() {
UriComponents uriComponents = UriComponentsBuilder.fromUriString(authorizationRequestUri).build();

assertThat(uriComponents.getQueryParams().getFirst("state")).isEqualTo(state);
String redirectUri = uriComponents.getQueryParams().getFirst("redirect_uri");
assertThat(URLDecoder.decode(redirectUri, StandardCharsets.UTF_8)).isEqualTo(this.redirectUri);
}

@Test
@WithMockUser("user1")
public void whenUserConsentsToAllScopesThenReturnAuthorizationCode() throws IOException {
Expand Down Expand Up @@ -105,6 +121,8 @@ public void whenUserConsentsToAllScopesThenReturnAuthorizationCode() throws IOEx
String location = approveConsentResponse.getResponseHeaderValue("location");
assertThat(location).startsWith(this.redirectUri);
assertThat(location).contains("code=");
String state = UriComponentsBuilder.fromHttpUrl(location).build().getQueryParams().getFirst("state");
assertThat(state).isEqualTo(this.reencodedState);
}

@Test
Expand All @@ -121,6 +139,8 @@ public void whenUserCancelsConsentThenReturnAccessDeniedError() throws IOExcepti
String location = cancelConsentResponse.getResponseHeaderValue("location");
assertThat(location).startsWith(this.redirectUri);
assertThat(location).contains("error=access_denied");
String state = UriComponentsBuilder.fromHttpUrl(location).build().getQueryParams().getFirst("state");
assertThat(state).isEqualTo(this.reencodedState);
}

}
4 changes: 4 additions & 0 deletions samples/default-authorizationserver/samples-default-authorizationserver.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ group = project.rootProject.group
version = project.rootProject.version
sourceCompatibility = "17"

test {
useJUnitPlatform()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed since it's automatically applied by the SpringJavaPlugin

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent quite a while figuring out why the tests don't run locally and in CI, in my opinion this config should have been added in the "Upgrade to JUnit 5" commit.

Did you try it out and are tests running (and failing the build) on your machine without explicitly enabling JUnit 5?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that the samples do not use the io.spring.convention.spring-module plugin, so SpringJavaPlugin is not applied as I previously mentioned. Sorry for the confusion there.

However, I'm wondering why the tests are failing on your end? The tests in default-authorizationserver sample are passing locally and in CI for quite some time now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't make myself fully clear. The tests are not failing in their current state.

What I meant was: if Gradle says the build was successful, that does not necessarily mean that it ran tests. To check if it does, I like to make a test fail, e.g. by throwing a RuntimeException. The failing test should also lead to a failing build.

For CI, I checked the logs of several commits for the test output – I think there's a DB-related message. This message did not appear after the upgrade to JUnit 5.

}

repositories {
mavenCentral()
maven { url 'https://repo.spring.io/milestone' }
Expand Down
1 change: 1 addition & 0 deletions ...es/default-authorizationserver/src/main/java/sample/config/AuthorizationServerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ public RegisteredClientRepository registeredClientRepository(JdbcTemplate jdbcTe
.authorizationGrantType(AuthorizationGrantType.REFRESH_TOKEN)
.authorizationGrantType(AuthorizationGrantType.CLIENT_CREDENTIALS)
.redirectUri("http://127.0.0.1:8080/login/oauth2/code/messaging-client-oidc")
.redirectUri("http://127.0.0.1:8080/login/oauth2/code/messaging-client-oidc?param=is%2Fencoded")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change is%2Fencoded to encoded%20parameter%20value

.redirectUri("http://127.0.0.1:8080/authorized")
.scope(OidcScopes.OPENID)
.scope(OidcScopes.PROFILE)
Expand Down
27 changes: 24 additions & 3 deletions ...-authorizationserver/src/test/java/sample/DefaultAuthorizationServerApplicationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
package sample;

import java.io.IOException;
import java.net.URLDecoder;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;

import com.gargoylesoftware.htmlunit.Page;
import com.gargoylesoftware.htmlunit.WebClient;
Expand All @@ -33,6 +36,7 @@
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.http.HttpStatus;
import org.springframework.test.context.junit.jupiter.SpringExtension;
import org.springframework.web.util.UriComponents;
import org.springframework.web.util.UriComponentsBuilder;

import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -46,15 +50,18 @@
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@AutoConfigureMockMvc
public class DefaultAuthorizationServerApplicationTests {
private static final String REDIRECT_URI = "http://127.0.0.1:8080/login/oauth2/code/messaging-client-oidc";
private static final String REDIRECT_URI = "http://127.0.0.1:8080/login/oauth2/code/messaging-client-oidc?param=is%2Fencoded";
public static final String STATE = "some+%20encoded%2Fstate";
public static final String REENCODED_STATE = "some%20%20encoded%2Fstate";

private static final String AUTHORIZATION_REQUEST = UriComponentsBuilder
.fromPath("/oauth2/authorize")
.queryParam("response_type", "code")
.queryParam("client_id", "messaging-client")
.queryParam("scope", "openid")
.queryParam("state", "some-state")
.queryParam("redirect_uri", REDIRECT_URI)
.queryParam("state", STATE)
Copy link
Collaborator

@jgrandja jgrandja Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert all changes related to the state parameter since this was fixed in gh-875 and test coverage is included in the associated commit

.queryParam("redirect_uri", URLEncoder.encode(REDIRECT_URI, StandardCharsets.UTF_8))
.build()
.toUriString();

@Autowired
Expand All @@ -67,6 +74,16 @@ public void setUp() {
this.webClient.getCookieManager().clearCookies(); // log out
}

@Test
void authorizationRequestProperlyEncoded() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this test since it directly tests UriComponentsBuilder, which is not owned by this project.

UriComponents uriComponents = UriComponentsBuilder.fromUriString(AUTHORIZATION_REQUEST).build();

assertThat(uriComponents.getQueryParams().getFirst("state")).isEqualTo(STATE);

String redirectUri = uriComponents.getQueryParams().getFirst("redirect_uri");
assertThat(URLDecoder.decode(redirectUri, StandardCharsets.UTF_8)).isEqualTo(REDIRECT_URI);
}

@Test
public void whenLoginSuccessfulThenDisplayNotFoundError() throws IOException {
HtmlPage page = this.webClient.getPage("/");
Expand Down Expand Up @@ -108,6 +125,10 @@ public void whenLoggingInAndRequestingTokenThenRedirectsToClientApplication() th

assertThat(response.getStatusCode()).isEqualTo(HttpStatus.MOVED_PERMANENTLY.value());
String location = response.getResponseHeaderValue("location");

String state = UriComponentsBuilder.fromHttpUrl(location).build().getQueryParams().getFirst("state");
assertThat(state).isEqualTo(REENCODED_STATE);

assertThat(location).startsWith(REDIRECT_URI);
assertThat(location).contains("code=");
}
Expand Down
25 changes: 22 additions & 3 deletions ...ault-authorizationserver/src/test/java/sample/DefaultAuthorizationServerConsentTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
package sample;

import java.io.IOException;
import java.net.URLDecoder;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.List;

Expand All @@ -36,6 +39,7 @@
import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationConsentService;
import org.springframework.security.test.context.support.WithMockUser;
import org.springframework.test.context.junit.jupiter.SpringExtension;
import org.springframework.web.util.UriComponents;
import org.springframework.web.util.UriComponentsBuilder;

import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -58,15 +62,18 @@ public class DefaultAuthorizationServerConsentTests {
@MockBean
private OAuth2AuthorizationConsentService authorizationConsentService;

private final String redirectUri = "http://127.0.0.1/login/oauth2/code/messaging-client-oidc";
private final String redirectUri = "http://127.0.0.1:8080/login/oauth2/code/messaging-client-oidc?param=is%2Fencoded";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert all changes in DefaultAuthorizationServerConsentTests since we have same test coverage in DefaultAuthorizationServerApplicationTests

public final String state = "some+%20encoded%2Fstate";
public final String reencodedState = "some%20%20encoded%2Fstate";

private final String authorizationRequestUri = UriComponentsBuilder
.fromPath("/oauth2/authorize")
.queryParam("response_type", "code")
.queryParam("client_id", "messaging-client")
.queryParam("scope", "openid message.read message.write")
.queryParam("state", "state")
.queryParam("redirect_uri", this.redirectUri)
.queryParam("state", this.state)
.queryParam("redirect_uri", URLEncoder.encode(this.redirectUri, StandardCharsets.UTF_8))
.build()
.toUriString();

@BeforeEach
Expand All @@ -77,6 +84,15 @@ public void setUp() {
when(this.authorizationConsentService.findById(any(), any())).thenReturn(null);
}

@Test
void authorizationRequestProperlyEncoded() {
UriComponents uriComponents = UriComponentsBuilder.fromUriString(authorizationRequestUri).build();

assertThat(uriComponents.getQueryParams().getFirst("state")).isEqualTo(state);
String redirectUri = uriComponents.getQueryParams().getFirst("redirect_uri");
assertThat(URLDecoder.decode(redirectUri, StandardCharsets.UTF_8)).isEqualTo(this.redirectUri);
}

@Test
@WithMockUser("user1")
public void whenUserConsentsToAllScopesThenReturnAuthorizationCode() throws IOException {
Expand Down Expand Up @@ -121,6 +137,9 @@ public void whenUserCancelsConsentThenReturnAccessDeniedError() throws IOExcepti
String location = cancelConsentResponse.getResponseHeaderValue("location");
assertThat(location).startsWith(this.redirectUri);
assertThat(location).contains("error=access_denied");

String state = UriComponentsBuilder.fromHttpUrl(location).build().getQueryParams().getFirst("state");
assertThat(state).isEqualTo(reencodedState);
}

}