Identityserver4: Previous workaround for PostLogoutRedirectUrl without IdTokenHint no longer works.

[KevinCathcart]

(In short the raw paramter workaround from back in #544, is no longer possible. )

Currently post_logout_redirect_uri does not work without id_token_hint also being provided.

This is unfortunate. Since id_token_hint is only RECOMMENDED, not all clients will send it. Also because an id_token can at least potentially be large (e.g. if it directly has claims because it was requested in an implicit flow with response_type=id_token) so it may not always be possible to include it as a URL parameter.

Since respecting the post_logout_redirect_uri is only a SHOULD, this is not a conformance issue. Obviously it is also necessary to ensure the signoff endpoint does not operate as an open redirecter, so only Uris that are registered for some client should be respected.

The id_token_hint is used to determine the client so as to perform this check. While in theory simply checking if the post_logout_redirect_uri is associated with any client could work, the client Store APIs have no way to support this.

All of this was talked about way back in #544. Back then a workaround was added. It became possible to access the raw parameters from the end session endpoint in the LogoutMessage.Parameters collection.
therefore the logout controller could retrieve the post_logout_redirect_uri, and validate it via some means not available to IdentityServer itself. Great!

Except, that #1262 removed the availability of the raw post_logout_redirect_uri parameter by adding this line:

https://github.com/IdentityServer/IdentityServer4/blob/3bcf55337fe91db2c7fcb6333cf811ec5b6a223e/src/IdentityServer4/Models/Messages/LogoutRequest.cs#L41

Ideally the code would be changed to only remove post_logout_redirect_uri and state if request.PostLogOutUri != null, because in that case, we are obviously preserving them.

In the mean time I have created a workaround where I wrap the existing IEndSessionRequestValidator with one that copies the two values to other parameter names when the ValidatedRequest object has a null PostLogOutUri. This is very inconvenient to do since the exiting EndSessionRequestValidator is internal.

[janstadt]

Instead of relying on the id_token, would you be able to inspect the incoming request off of the HttpContext and figure out the client who requested the logout? Look up the client by the clientUri to get the postRedirectUri's for that client and compare it to the parameter that has came in?

I cant keep the flow straight, but when a client does Signout("oidc") or whatever the scheme is, it calls the endsession endpoint in the OP and at that point you should be able to see who's making the request (via the referrer header or something) and use that info to figure out who the client is.

I havent validated it or anything but we've just ran into the issue where our redirect url back to the app that initiated single signout on our logout page was null after we removed SaveTokens=true from our clients since our cookies were getting big.

[brockallen]

Parameters.Remove(OidcConstants.EndSessionRequest.PostLogoutRedirectUri);

IIRC this was done to cut down on the serialized payload (but i'd have to really dig into it to be sure). This was necessary since we changed how we send it all to using a query string param and not as a cookie.

As for not being an open redirecter, could an attacker not take advantage of a client with a XSS vulnerability in their state param? I could see that as an attack against not validating the id_token_hint. As you can imagine, we're just trying to be as cautious as possible and avoid being an open redirecter.

As for replacing the behavior, did you instead implement a custom IEndSessionRequestValidator? That might have been easier.

@leastprivilege -- thoughts?

[brockallen]

Any other inputs to this conversation @KevinCathcart? Don't forget that if you don't like the behavior you can always replace the IEndSessionRequestValidator with your own.

[brockallen]

Closing. If you still have issues, feel free to reopen.

[KevinCathcart]

The biggest issue with replacing the IEndSessionRequestValidator is that the existing code relies heavily on internal classes, so replacing it with custom code requires either modifying the library, using private reflection, or copying a whole bunch of code from the library. None are great options.

[brockallen]

If you can list those and/or open an issue to make them public, we can consider it in the next release.

[KevinCathcart]

Hmm... I guess i was misremembering, since the IEndSessionRequestValidator was not the first thing I tried to hook.

Just copying that whole EndSessionRequestValidator class and modifying it should work.

If EndSessionRequestValidator were made public code copying could even be eliminated. A wrapper could simply request that this exact class be injected, delegate to it, and then could adjust the response however it wants. That is actually what my current code does, except that it jumps through some hoops to get the existing instance while avoiding private reflection.

[janstadt]

We are doing exactly what @KevinCathcart has mentioned above. Would be nice if EndSessionRequestValidator was made public.

[brockallen]

2560

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.