Set response status in `HttpResponse.OnStarting`, or skip setting response status?

[ravindUwU]

Confirm you've already contributed to this project or that you sponsor it

• I confirm I'm a sponsor or a contributor

Describe the solution you'd like

Hello! 😊
ASP.NET Core doesn't seem to run minimal API request delegates if middleware sets an error status code before minimal API filters run: dotnet/aspnetcore#60659. This becomes problematic when OpenIddict server is configured to passthrough errors to be handled by a minimal API endpoint.

Setting the status code in the HttpResponse.OnStarting callback however, seems fine: dotnet/aspnetcore#60659 (comment). Could OpenIddict set response status codes this way 🤔?

Additional context

No response

[kevinchalet]

Hey @ravindUwU,

Could OpenIddict set response status codes this way 🤔?

I wish it was that easy but that method has unfortunately a downside: it would become a lot harder to safely compose the HTTP status code via the OpenIddict events model:

• Multiple HttpResponse.OnStarting callbacks can be registered (even outside OpenIddict): depending on the exact point/moment the callback was registered, the "winning" one might be hard to predict.
• HttpResponse.OnStarting callbacks are invoked in the reverse order, which complicates things if you want to customize/override the status code attached by OpenIddict.
• The callback registered by OpenIddict might later interfere with the component responsible for writing the actual response (e.g in this case, a minimal action).

That said, I agree the current situation is definitely far from ideal as it's a super-hard-to-debug issue. The fix should be made in ASP.NET Core itself (IMHO their design is at best... "questionable"), but maybe we can find a way to detect that via the endpoint metadata and throw an exception to indicate the "minimal action + endpoint filter + error pass-through mode" combo doesn't work due to that limitation?

If you have a repro project with OpenIddict (I'd expect both the client and server stacks to be affected, as both work the same way), that would be awesome if you could share it 😃

Cheers.

[ravindUwU]

The fix should be made in ASP.NET Core itself (IMHO their design is at best... "questionable")

Yeah!

...maybe we can find a way to detect that via the endpoint metadata and throw an exception to indicate the "minimal action + endpoint filter + error pass-through mode" combo doesn't work...

Unsure if throwing is the best approach here, because the throw would eventually have to be removed once the ASP.NET Core bug is fixed? I'm also unsure if the presence of filters can be determined using endpoint metadata.

Error passthrough is typically used to produce a custom error response/page, isn't it? Thoughts on 🤔,

• Not setting the response status code in OpenIddict if error passthrough is enabled, with the expectation that the endpoint handler would do this as appropriate?
• Maybe with an opt-in (e.g., EnableErrorPassthrough(setStatusCode: false) or equivalent) for compatibility?
• Maybe including the OpenIddict-"suggested" response status code within OpenIddictResponse so that the handler can use it?

If you have a repro project with OpenIddict...

I think this might be as minimal as I can make it 😅. .NET 9, <PackageReference Include="OpenIddict.AspNetCore" Version="7.0.0" />. Notice how,

• ✅ GET /connect/authorize/1 produces a 400 Bad Request with the message "Custom error response for invalid_request" as expected.
• ❌ GET /connect/authorize/2 produces a 400 Bad Request with a blank body.

using Microsoft.AspNetCore;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;

public class Program
{
	public static void Main(string[] args)
	{
		var builder = WebApplication.CreateBuilder(args);

		builder.Services.AddOpenIddict()
			.AddCore((options) =>
			{
			})
			.AddServer((options) =>
			{
				options
					.SetAuthorizationEndpointUris("connect/authorize/1", "connect/authorize/2")
					.SetTokenEndpointUris("connect/token")
					.AllowAuthorizationCodeFlow()
					.AddEphemeralEncryptionKey()
					.AddEphemeralSigningKey();

				options
					.UseAspNetCore()
					.DisableTransportSecurityRequirement()
					.EnableAuthorizationEndpointPassthrough()
					.EnableErrorPassthrough();
			});

		var app = builder.Build();

		app.MapGet("connect/authorize/1", AuthorizeEndpointHandler);

		app.MapGet("connect/authorize/2", AuthorizeEndpointHandler)
			.AddEndpointFilter((ctx, next) => next(ctx));

		static string AuthorizeEndpointHandler(HttpContext ctx)
		{
			var oiRes = ctx.GetOpenIddictServerResponse();

			return oiRes?.Error is not null
				? $"Custom error response for {oiRes.Error}"
				: "Authorize form";
		}

		app.Run();
	}
}

[kevinchalet]

Thanks a lot for that great repro, @ravindUwU! I played with it extensively and came to the same conclusion as you: the information I was looking for doesn't seem to be (easily?) available 😢

Error passthrough is typically used to produce a custom error response/page, isn't it?

Yep, it's the most common scenario. Another common scenario is to handle access_denied errors directly in the callback handler when using the OpenIddict client (e.g to redirect the user agent to a special page asking the user to restart the authentication flow). For those who prefer a different approach, it's also possible to enable the status code pages middleware integration to do similar things: https://documentation.openiddict.com/integrations/aspnet-core#status-code-pages-middleware-integration

Regarding your suggestions, they are not unreasonable but since the ASP.NET Core design issue has more implications that I originally anticipated (see dotnet/aspnetcore#60659 (comment)), I'm not so sure it's something we should detect/workaround specifically in OpenIddict itself. Before changing anything our side, let's see what the ASP.NET team wants to do with that issue in the next .NET version 😄

Out of curiosity, what's your use case for relying on endpoint filters for the OpenIddict endpoints?

[ravindUwU]

...let's see what the ASP.NET team wants to do with that issue in the next .NET version 😄

No worries! 😊

...what's your use case for relying on endpoint filters for the OpenIddict endpoints?

If I recall correctly, I noticed this because I had an endpoint filter-based exception handler that was globally applied to a route group from which all endpoints (including the OpenIddict endpoints) were mapped, i.e., var root = app.MapGroup("").AddEndpointFilter(...) followed by subsequent root.Map*(...) calls. The filter wasn't specifically for use with the OpenIddict endpoints.

[rajeshgithub001]

Hi @ravindUwU,

I would like to contribute ?

Cheers

[kevinchalet]

@rajeshgithub001 hey. If you're interested in contributing, you'll need to ping the ASP.NET folks and ask them if they'd accept a PR to fix the issue. More info here: dotnet/aspnetcore#60659 (comment).

[rajeshgithub001]

Hi @kevinchalet,

I am trying to find out Technical Know-How document, to get used to on this project .

Please share the location where it is available, if possible ?

Thanks.

[kevinchalet]

The ASP.NET Core issue sadly didn't get any traction and there isn't much we can do on our side.
Closing.

[ravindUwU]

Hi @kevinchalet, thoughts implementing the following proposals from #2375 (comment) until the underlying .NET issue is addressed 🤔?

• Not setting the response status code in OpenIddict if error passthrough is enabled, with the expectation that the endpoint handler would do this as appropriate?
• Maybe with an opt-in (e.g., EnableErrorPassthrough(setStatusCode: false) or equivalent) for compatibility?
• Maybe including the OpenIddict-"suggested" response status code within OpenIddictResponse so that the handler can use it?

[kevinchalet]

@ravindUwU hey.

Honestly, I'm not super comfortable adding a workaround (+ a public API assuming it needs to be opt-in) for an ASP.NET Core bug.
That said, it's definitely possible to work around it yourself using the events model:

options.AddEventHandler<ApplyAuthorizationResponseContext>(builder =>
{
    builder.SetOrder(OpenIddictServerAspNetCoreHandlers.AttachHttpResponseCode<
        ApplyAuthorizationResponseContext>.Descriptor.Order + 1);

    builder.UseInlineHandler(context =>
    {
        var response = context.Transaction.GetHttpRequest()?.HttpContext.Response ??
            throw new InvalidOperationException();

        if (response.StatusCode is not 200)
        {
            response.StatusCode = 200; // Reset the response status code to its default value.
        }

        return ValueTask.CompletedTask;
    });
});