Response mode is always 'query' for missing PKCE code challenge error response

[asos-hughwoods]

Which version of Duende IdentityServer are you using?

7.0

Which version of .NET are you using?

8.0

Describe the bug

When making an authorize request in the auth code flow:

• the client is configured to require PKCE
• the request is missing code_challenge and code_challenge_method parameters

Then the response mode for errors is always query, even if the authorize request specifies request_mode=fragment

To Reproduce

1. Running Identity Server

• Configure a client to require PKCE
• Make a request to /authorize without code_challenge and code_challenge_method
• Add a request parameter response_mode=fragment

IIdentityServerInteractionService.GetErrorContextAsync always returns a RedirectUri with the error in the query
e.g. https://localhost/callback?error=invalid_request&error_description=code%20challenge%20required#_=_

2. As a unit test

Add a case to
test/IdentityServer.UnitTests/Validation/AuthorizeRequest Validation/Authorize_ProtocolValidation_PKCE.cs

    [Theory]
    [InlineData("query")]
    [InlineData("fragment")]
    public async Task openid_code_request_missing_challenge_should_be_rejected_with_requested_response_mode(string responseMode)
    {
        var parameters = new NameValueCollection();
        parameters.Add(OidcConstants.AuthorizeRequest.ClientId, "codeclient.pkce");
        parameters.Add(OidcConstants.AuthorizeRequest.Scope, "openid");
        parameters.Add(OidcConstants.AuthorizeRequest.RedirectUri, "https://server/cb");
        parameters.Add(OidcConstants.AuthorizeRequest.ResponseType, OidcConstants.ResponseTypes.Code);

        var validator = Factory.CreateAuthorizeRequestValidator();
        var result = await validator.ValidateAsync(parameters);

        result.IsError.Should().Be(true);
        result.Error.Should().Be(OidcConstants.AuthorizeErrors.InvalidRequest);
        result.ErrorDescription.Should().Be("code challenge required");
        result.ValidatedRequest.ResponseMode.Should().Be(responseMode);
    }

Expected behaviour

The error response should be in the fragment if response_mode=fragment e.g.
https://localhost/callback#error=invalid_request&error_description=code%20challenge%20required

The behaviour is described in https://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#ResponseModes

fragment

In this mode, Authorization Response parameters are encoded in the fragment added to the redirect_uri when redirecting back to the Client.

[RolandGuijt]

We are investigating this and will come back with a reply asap.

[RolandGuijt]

When an error occurs when the authorization endpoint is called IdentityServer doesn't redirect to the client except for a handful of errors we call safe errors. Omitting code_challenge and code_challenge_method is not a safe error.
This is to prevent attackers from using the IdP as an open redirector resulting in attack vectors. The process is described here.
The document also describes mitigations (2.3). IdentityServer's behavior matches the second option there. It will show an error page that displays the error.
So using the redirectUrl yourself to redirect to the client in the error controller/page is probably not what you want because it will reopen the attack vectors.

Having said that: the question still is: should the redirectUrl respect the response_mode? IdentityServer shows this behavior because when the request comes in, it will first validate the PKCE parameters. If it finds any problems it will return the error page immediately. That code is before the validation/processing of response_mode the default of which is "query".
If you're interested to see that in IdentityServer's code: it's in AuthorizeRequestValidator in the ValidateCoreParameters method. There ValidatePckeParameters is called, returning from the method if an error is found. And below that the response_mode is applied.

The Duende team is going to discuss this behavior. We have to think about the implications when we change this behavior: it might be a breaking change. I'll keep you posted.
Thanks a lot for finding and reporting this issue!

[RolandGuijt]

I've created an issue in the IdentityServer repo so this can be discussed.
Please track this issue using that from now on.