Skip to content
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.

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

OidcClientInitiatedLogoutSuccessHandler url-encodes PostLogoutRedirectUri twice #9511

Closed
hosea opened this issue Mar 18, 2021 · 6 comments
Closed
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: backported An issue that has been backported to maintenance branches status: feedback-provided Feedback has been provided type: bug A general bug
Milestone

Comments

@hosea
Copy link
Contributor

hosea commented Mar 18, 2021

The OidcClientInitiatedLogoutSuccessHandler url-encodes the PostLogoutRedirectUri twice. This leads to corrupted URLs.

My used postLogoutRedirectUri is:
https://localhost:8443/loginselect?forwardUrl=secureduserinfo%3F0-1.-userinfo-sessioninvalidate

OidcClientInitiatedLogoutSuccessHandler adds this uri as queryparam "post_logout_redirect_uri" to the generated targetUrl. URL-encoding this uri as queryparam should lead to a queryparam like this:
...&post_logout_redirect_uri =https://localhost:8443/loginselect?forwardUrl%3Dsecureduserinfo%253F0-1.-userinfo-sessioninvalidate
But it is url-encoded twice:
...&post_logout_redirect_uri=https://localhost:8443/loginselect?forwardUrl%3Dsecureduserinfo%25253F0-1.-userinfo-sessioninvalidate
(%2525 instead of %25)

Version: spring-security-oauth2-client 5.4.5

@hosea hosea added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Mar 18, 2021
@jgrandja
Copy link
Contributor

jgrandja commented Mar 19, 2021

@hosea The supplied postLogoutRedirectUri is already encoded:

https://localhost:8443/loginselect?forwardUrl=secureduserinfo%3F0-1.-userinfo-sessioninvalidate -> %3F

I believe if you change it to the (un)encoded version, it will work:

https://localhost:8443/loginselect?forwardUrl=secureduserinfo?0-1.-userinfo-sessioninvalidate -> %3F to ?

Please try this and let me know if it worked.

@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Mar 19, 2021
@jgrandja jgrandja self-assigned this Mar 19, 2021
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Mar 26, 2021
@hosea
Copy link
Contributor Author

hosea commented Mar 30, 2021

Hi @jgrandja,

unfortunatly this does not work. If I replace the encoding with an unencoded version, the result is also wrong:
Unencoded version: https://localhost:8443/loginselect?forwardUrl=secureduserinfo?0-1.-userinfo-sessioninvalidate
Result: ...&post_logout_recdirect_uri=https://localhost:8443/loginselect?forwardUrl%3Dsecureduserinfo?0-1.-userinfo-sessioninvalidate
Now the "?" is never encoded.

But I could break down the problem a little bit. I think, the method
private URI postLogoutRedirectUri(HttpServletRequest request) in OidcClientInitiatedLogoutSuccessHandler is not working as expected. The intention of this method is to replace the well-known placeholder "baseUrl" but leave the rest of the postLogoutRedirectUri unchanged. But "unchanged" is not true in my case. My postLogoutRedirectUri is https://localhost:8443/loginselect?forwardUrl=secureduserinfo%3F0-1.-userinfo-sessioninvalidate but after processing this method the Uri https://localhost:8443/loginselect?forwardUrl=secureduserinfo%253F0-1.-userinfo-sessioninvalidate is used.

Kind regards
Hans

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Mar 30, 2021
@hosea
Copy link
Contributor Author

hosea commented Mar 30, 2021

Test.java.gz

Hi @jgrandja,
I just attached a simple Java-Program that does the same processing steps with my URI as OidcClientInitiatedLogoutSuccessHandler.
Hope that helps. I used it for reproducing the uri processing.
Kind regards
Hans

@jgrandja jgrandja assigned jzheaux and unassigned jgrandja Apr 5, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Apr 5, 2021

Thanks, @hosea, I was able to confirm the issue.

It appears that both postLogoutRedirectUri and endpointUri are encoding the post_logout_redirect_uri value.

I think this can be addressed by changing postLogoutRedirectUri(HttpServletRequest) to call toUriString instead of toUri.

@hosea, are you able to submit a PR that addresses the issue and adds a test to confirm the bug is fixed?

@jzheaux jzheaux added the type: bug A general bug label Apr 5, 2021
@jzheaux jzheaux modified the milestones: 5.5.0-RC1, 5.5.0 Apr 5, 2021
hosea added a commit to hosea/spring-security that referenced this issue Apr 22, 2021
…. Now encodes already encoded queryparameters in postLogoutRedirectUrl correctly
@hosea
Copy link
Contributor Author

hosea commented Apr 22, 2021

Hi @jzheaux ,

changing from toUri to toUriString solved the issue. I fixed it, added a test and submitted a PR #9672

Thank you
Kind regards
Hans

@jzheaux jzheaux modified the milestones: 5.5.0, 5.6.0-M1 May 13, 2021
jzheaux added a commit that referenced this issue May 26, 2021
jzheaux pushed a commit that referenced this issue May 26, 2021
Now encodes already encoded queryparameters in postLogoutRedirectUrl
correctly

Closes gh-9511
jzheaux added a commit that referenced this issue May 26, 2021
@spring-projects-issues spring-projects-issues added the status: backported An issue that has been backported to maintenance branches label May 26, 2021
jzheaux pushed a commit that referenced this issue May 26, 2021
Now encodes already encoded queryparameters in postLogoutRedirectUrl
correctly

Closes gh-9511
jzheaux added a commit that referenced this issue May 26, 2021
jzheaux pushed a commit that referenced this issue May 26, 2021
Now encodes already encoded queryparameters in postLogoutRedirectUrl
correctly

Closes gh-9511
jzheaux added a commit that referenced this issue May 26, 2021
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
Now encodes already encoded queryparameters in postLogoutRedirectUrl
correctly

Closes spring-projectsgh-9511
akohli96 pushed a commit to akohli96/spring-security that referenced this issue Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: backported An issue that has been backported to maintenance branches status: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants