Ensuring the generated redirect URL is valid

[troydai]

In issue #844, @PinpointTownes wrote:

The null ref was in OIDC.

On a related note, we should add more checks to ensure an authorization request or a logout request cannot be generated when the authorization/logout endpoint path resolved from the OIDC configuration or set from the RedirectToIdentityProvider event is null.

Currently, if the authorization endpoint is undefined, the challenge is applied to the current address (e.g ?response_type=id_token...), which can result in an endless loop. 3 OpenIddict users reported a similar behavior in their own apps.

You can easily reproduce it by cloning the OpenIddict MVC samples and updating the server sample to disable the authorization endpoint:

services.AddOpenIddict<ApplicationUser, IdentityRole<Guid>, ApplicationDbContext, Guid>()
    // .EnableAuthorizationEndpoint("/connect/authorize")
    // .EnableLogoutEndpoint("/connect/logout")
    .EnableTokenEndpoint("/connect/token")
    // .EnableUserinfoEndpoint("/connect/userinfo")

    // .AllowAuthorizationCodeFlow()
    // .AllowRefreshTokenFlow()
    .AllowPasswordFlow()

    .DisableHttpsRequirement();

[Tratcher]

Note there is an existing check for this in HandleUnauthorizedAsync:

if (!Uri.IsWellFormedUriString(redirectUri, UriKind.Absolute))
{
    Logger.InvalidAuthenticationRequestUrl(redirectUri);
}

Note it only logs. This was because Katana did not apply challenges immediately, it waited until OnSendingHeaders or pipeline-unwind so any exception thrown would not be visible to user code. Core applies challenges immediately when called from user code. While pipeline unwind automatic challenges are still supported, they are not the normal flow. Also, throwing an exception has the advantage of changing this infinite redirect to a 500.

[kevinchalet]

👍 for throwing an exception (an InvalidOperationException, maybe?)

[liquidboy]

thought id add this in here for histories sake ...
On my project we just upgraded to netcore 1.1.0 and suddenly our SignOut's were failing ... Traced it to the changes implemented in this issue around checking if IssuerAddress was null or empty ...

https://twitter.com/josefajardo/status/819329380421804032

I still haven't worked out how to fix it BUT atleast I know what caused it ..