Skip to content

Avoid cookie login redirects for known API endpoints #62816

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

Merged
merged 6 commits into from
Jul 24, 2025
Merged

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Jul 18, 2025

This adds IApiEndpointMetadata as new public API. It doesn't add any attributes or extension methods, but we do add it automatically for endpoints where RDF/RDG read a JSON request body or write a JSON response body. ApiController endpoints, SignalR endpoints and endpoints with TypedResults return-types that we know to be API-oriented also get this metadata automatically.

API Proposal: #62883

Fixes #9039

@Copilot Copilot AI review requested due to automatic review settings July 18, 2025 23:57
@halter73 halter73 requested review from BrennanConroy, a team and captainsafia as code owners July 18, 2025 23:57
@github-actions github-actions bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jul 18, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces IApiEndpointMetadata as a new public interface to identify API endpoints that should avoid cookie authentication redirects in favor of status codes. The interface is automatically added to endpoints that expect JSON request bodies or return JSON responses, signaling they are programmatic APIs rather than browser-navigated endpoints.

Key changes:

  • Adds IApiEndpointMetadata interface and internal implementation
  • Modifies cookie authentication to check for this metadata and return 401/403 status codes instead of redirects
  • Updates HTTP Results types and request delegate generation to automatically add the metadata

Reviewed Changes

Copilot reviewed 107 out of 108 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Http/Http.Abstractions/src/Metadata/IApiEndpointMetadata.cs Defines the new public interface for API endpoint identification
src/Security/Authentication/Cookies/src/CookieAuthenticationEvents.cs Updates authentication logic to check for API endpoints and return status codes
src/Mvc/Mvc.Core/src/ApiControllerAttribute.cs Makes ApiControllerAttribute implement IApiEndpointMetadata
Multiple HTTP Results files Adds IApiEndpointMetadata to various result types' PopulateMetadata methods
Request delegate generator files Updates code generation to automatically add API metadata
Comments suppressed due to low confidence (1)

src/Security/Authentication/test/CookieTests.cs:130

  • The comment references 'INonBrowserEndpointMetadata' but the code adds 'ApiEndpointMetadata.Instance'. The comment should be updated to reflect the actual metadata being added.
        var transaction = await SendAsync(server, "http://example.com/protected/CustomRedirect");

@gfoidl gfoidl added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer and removed area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Jul 21, 2025
@@ -125,6 +126,9 @@ public static ConnectionEndpointRouteBuilder MapConnections(this IEndpointRouteB
{
e.Metadata.Add(data);
}

// Add INonBrowserEndpointMetadata to indicate this is a non-browser endpoint (SignalR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Add INonBrowserEndpointMetadata to indicate this is a non-browser endpoint (SignalR)
// Add IApiEndpointMetadata to indicate this is a non-browser endpoint (SignalR)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if there are any blazor endpoints that should also be marked?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the big reasons that I went with this approach rather than trying to detect fetch requests is because blazor will gracefully handle redirects during enhanced navigation. If we started returning 401s instead of redirecting, we'd break the default Blazor template with individual auth. We could special case blazor fetch requests to avoid this, but I thought it was indicative of the risk of defaulting to 401 for all fetch requests.

For the circuit endpoints, we already get the better behavior because if this change to the SignalR endpoints. It's possible that I'm missing some Blazor related API endpoints that don't get picked up automatically by the RDF/RDG changes (MapIdentityApi is an example of endpoints that get the metadata for free this way), but I'm not sure what they'd be off the top of my head. @javiercn @MackinnonBuck Do you have any suggestions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var disconnectEndpoint = endpoints.Map(
(path.EndsWith('/') ? path : path + "/") + "disconnect/",
endpoints.CreateApplicationBuilder().UseMiddleware<CircuitDisconnectMiddleware>().Build())
.WithDisplayName("Blazor disconnect");
var jsInitializersEndpoint = endpoints.Map(
(path.EndsWith('/') ? path : path + "/") + "initializers/",
endpoints.CreateApplicationBuilder().UseMiddleware<CircuitJavaScriptInitializationMiddleware>().Build())
.WithDisplayName("Blazor initializers");

These are what I was thinking of @dotnet/aspnet-blazor-eng thoughts?

@halter73 halter73 enabled auto-merge (squash) July 23, 2025 22:18
@halter73
Copy link
Member Author

/backport to release/10.0-preview7

Copy link
Contributor

Started backporting to release/10.0-preview7: https://github.com/dotnet/aspnetcore/actions/runs/16484743945

@halter73 halter73 merged commit cc6b317 into main Jul 24, 2025
30 checks passed
@halter73 halter73 deleted the halter73/9039 branch July 24, 2025 01:47
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-rc1 milestone Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ApiController redirects to login page
4 participants