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

andreasf · 2022-12-16T18:36:41Z

This fixes an issue with redirect URIs that already have URL-encoded parameters baked in.

I recently built an OIDC provider with Authorization Server for a client and noticed that some of their redirect URIs would cause problems.

PHP's Yii framework creates redirect URIs in the format https://example.com/index.php?r=site%2Fauth&authclient=foo. The authorization endpoint would reencode the %2F in the parameter to %252F and redirect users to a non-existing route.

This PR fixes the behavior while making sure that the encoding of the state parameter is unchanged.

Related to #875

… authorization response The redirect URI used in the test contains a URL-encoded parameter and is similar to redirect URIs of the popular Yii PHP framework. Also document how state is currently decoded and re-encoded, modifying it in the process.

…response Redirect URIs can contain query parameters and these can be URI encoded. UriComponentsBuilder does not decode query parameters in fromUriString(). They become part of the template which by default is encoded (again). This commit configures a UriBuilder that does not encode the template but only provided values.

…ncoding

jgrandja

Thanks for the PR @andreasf ! Please see review comments.

jgrandja · 2023-01-09T22:06:31Z

...a/org/springframework/security/oauth2/server/authorization/client/TestRegisteredClients.java

@@ -39,6 +39,18 @@ public static RegisteredClient.Builder registeredClient() {
 				.scope("scope1");
 	}

+	public static RegisteredClient.Builder registeredClientEncodedUri() {


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

jgrandja · 2023-01-09T22:10:39Z

samples/custom-consent-authorizationserver/samples-custom-consent-authorizationserver.gradle

@@ -8,6 +8,10 @@ group = project.rootProject.group
 version = project.rootProject.version
 sourceCompatibility = "17"

+test {


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

jgrandja · 2023-01-09T22:12:00Z

samples/default-authorizationserver/samples-default-authorizationserver.gradle

@@ -8,6 +8,10 @@ group = project.rootProject.group
 version = project.rootProject.version
 sourceCompatibility = "17"

+test {
+	useJUnitPlatform()


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

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?

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.

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.

jgrandja · 2023-01-09T22:12:47Z

samples/default-authorizationserver/src/main/java/sample/config/AuthorizationServerConfig.java

@@ -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")


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

jgrandja · 2023-01-09T22:14:39Z

...ult-authorizationserver/src/test/java/sample/DefaultAuthorizationServerApplicationTests.java


 	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)


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

jgrandja · 2023-01-09T22:17:12Z

...ult-authorizationserver/src/test/java/sample/DefaultAuthorizationServerApplicationTests.java

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

+	@Test
+	void authorizationRequestProperlyEncoded() {


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

jgrandja · 2023-01-09T22:20:40Z

...amework/security/oauth2/server/authorization/web/OAuth2AuthorizationEndpointFilterTests.java

@@ -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";


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

jgrandja · 2023-01-09T22:25:33Z

...default-authorizationserver/src/test/java/sample/DefaultAuthorizationServerConsentTests.java

@@ -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";


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

jgrandja · 2023-01-27T20:37:17Z

@andreasf Just checking back to see if you're able to apply the requested changes?

andreasf · 2023-01-31T17:11:40Z

@andreasf Just checking back to see if you're able to apply the requested changes?

I'm a bit busy but I'll try to take a look this week

Closes gh-1011

jgrandja · 2023-02-15T22:16:04Z

@andreasf We have a release next week and I wanted to get this fix in so I went ahead and added a polish commit and merged.

Issue gh-1011

andreasf added 5 commits December 13, 2022 10:35

Enable JUnit 5 test runner in samples

5063a98

Fix: do not re-encode redirect URI query parameters in error response

ca9e58c

CustomConsentAuthorizationServerTests: check redirect URI and state e…

1dfa38c

…ncoding

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 16, 2022

jgrandja requested changes Jan 9, 2023

View reviewed changes

jgrandja self-assigned this Jan 9, 2023

jgrandja added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 9, 2023

jgrandja added this to the 0.4.1 milestone Jan 9, 2023

jgrandja changed the title ~~Fix: URL-encoded parameters in redirect URI are encoded twice~~ URL-encoded parameters in redirect URI are encoded twice Jan 10, 2023

jgrandja pushed a commit that referenced this pull request Feb 15, 2023

Preserve encoding for authorization request redirect_uri parameter

26205a2

Closes gh-1011

jgrandja added a commit that referenced this pull request Feb 15, 2023

Polish gh-1011

30927ad

jgrandja closed this Feb 15, 2023

jgrandja mentioned this pull request Feb 15, 2023

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

Closed

jgrandja added a commit that referenced this pull request Feb 21, 2023

Polish authorization error response encoding

0cd5940

Issue gh-1011

jgrandja mentioned this pull request Feb 21, 2023

fix: error description not url-encoded #1078

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andreasf commented Dec 16, 2022

jgrandja left a comment

jgrandja Jan 9, 2023

jgrandja Jan 9, 2023

jgrandja Jan 9, 2023

andreasf Jan 16, 2023

jgrandja Jan 16, 2023

andreasf Jan 31, 2023

jgrandja Jan 9, 2023

jgrandja Jan 9, 2023 •

edited

Loading

jgrandja Jan 9, 2023

jgrandja Jan 9, 2023 •

edited

Loading

jgrandja Jan 9, 2023

jgrandja commented Jan 27, 2023

andreasf commented Jan 31, 2023

jgrandja commented Feb 15, 2023

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

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

Conversation

andreasf commented Dec 16, 2022

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented Jan 27, 2023

andreasf commented Jan 31, 2023

jgrandja commented Feb 15, 2023

jgrandja Jan 9, 2023 •

edited

Loading

jgrandja Jan 9, 2023 •

edited

Loading