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

OpenIdConnect middleware throws on logout messages within callback path (regression in 4.1.0) #476

Closed
will-bartlett opened this issue Sep 6, 2022 · 1 comment
Milestone

Comments

@will-bartlett
Copy link

I have an OpenID Connect site https://domain.example with redirect URI https://domain.example/redir used for both login redirect_uri and logout post_logoug_redirect_uri. On 4.0.1, this worked fine. After upgrading to 4.2.2 related to CVE-2022-29117, I found a regression here that seems to have been introduced by #297 . Following RP initiated logout, the OpenId Connect middleware makes a request like https://sts.example/logout?state=OpenIdConnectProperties.Abc&redirect_uri=https%3A%2F%2Fdomain.example%2Fredir which results in a response to my site like https://domain.example/redir?state=OpenIDConnectProperties.Abc. In 4.0.1, because the response from RP initiated logout is a GET, AuthenticateCoreAsync left openIdConnectMessage null, and returned null (no authentication) on line 220. After #297 , openIdConnectMessage was renamed to authorizationResponse and set on GET requests on line 213. It then fails 70 lines later as being an invalid authorize message, as an authorize message must either include "code" or "id_token".

The prior code seems to have more of the right ideas. This message is indeed a openIdConnectMessage, and NOT an authorizationResponse yet. The middleware should first decode the state using something like Helper.LookupSignOut - and then, depending on the data in the state, either process the message as an authorizationResponse or an authorizationResponseRevoke.

@Tratcher
Copy link
Member

Tratcher commented Sep 6, 2022

Offline notes: post_logoug_redirect_uri and redirect_uri should use different values to avoid conflicts like this. We'll re-visit if this doesn't prove viable.

The state does not currently contain info specific to sign-in or sign-out.

@Tratcher Tratcher modified the milestones: Backlog, Discussions Sep 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants