Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Antiforgery middleware #38314

Closed
wants to merge 1 commit into from
Closed

Antiforgery middleware #38314

wants to merge 1 commit into from

Conversation

pranavkm
Copy link
Contributor

No description provided.

}
}

private static Task WriteAntiforgeryInvalidResponseAsync(HttpContext context, string? message)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed like doing a ProblemDetails would be a possible thing to do here. The current filter in MVC simply returns a status code but it felt like we could offer more details.

The interesting bit to note is that these requests should have primarily been triggered by a browser POST so returning JSON is a bit unusual.

Copy link
Member

Choose a reason for hiding this comment

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

Seems wrong to be forcibly returning JSON here. Why wouldn't we just let it throw or throw BadHttpRequestException or something else to enable the app to handle how this responds to the client?

Copy link
Member

Choose a reason for hiding this comment

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

Or make it an event.

private readonly RequestDelegate _next;
private readonly ILogger<AntiforgeryMiddleware> _logger;

public AntiforgeryMiddleware(IAntiforgery antiforgery, RequestDelegate next, ILogger<AntiforgeryMiddleware> logger)
Copy link
Member

Choose a reason for hiding this comment

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

Is this middleware important enough to be included in security check in EndpointMiddleware?

if (endpoint.Metadata.GetMetadata<IAuthorizeData>() != null &&
!httpContext.Items.ContainsKey(AuthorizationMiddlewareInvokedKey))
{
ThrowMissingAuthMiddlewareException(endpoint);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's on my list of things to do. We also need to add a StartupAnalyzer that suggests ordering this after Routing and AuthZ middlewares.

else
{
Log.AntiforgeryValidationFailed(_logger, message);
await context.Response.WriteAsJsonAsync(new ProblemDetails
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this doesn't set the status code to 400 or call WriteAntiforgeryInvalidResponseAsync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops

@dotnet dotnet deleted a comment from Sahabee Nov 12, 2021
@pranavkm
Copy link
Contributor Author

@Tratcher pointed out that we should be applying the form limits before this middleware is executed. In MVC, this is currently implemented as a separate auth filter: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/RequestFormLimitsAttribute.cs, https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/Filters/RequestFormLimitsFilter.cs.

Would we want to roll these two things in to a separate RequestFormMiddleware? It feels unwieldy to have two separate middlewares with specific ordering requirements.

/cc @DamianEdwards in case you wanted to chime in.

@DamianEdwards
Copy link
Member

Is the intent here to just have this middleware validate antiforgery tokens/cookies/headers if present in the request? i.e. issuing tokens and sending them back to the client is still up to the framework or manual app code.

{
// Validation not needed for these request types.
return true;
return (true, null);
}

var tokens = await _tokenStore.GetRequestTokensAsync(httpContext);
Copy link
Member

Choose a reason for hiding this comment

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

Something should catch the AntiforgeryValidationException and convert it to a 400.

try
{
form = await httpContext.Request.ReadFormAsync();
}
catch (InvalidDataException ex)
{
// ReadFormAsync can throw InvalidDataException if the form content is malformed.
// Wrap it in an AntiforgeryValidationException and allow the caller to handle it as just another antiforgery failure.
throw new AntiforgeryValidationException(Resources.AntiforgeryToken_UnableToReadRequest, ex);
}
catch (IOException ex)
{
// Reading the request body (which happens as part of ReadFromAsync) may throw an exception if a client disconnects.
// Wrap it in an AntiforgeryValidationException and allow the caller to handle it as just another antiforgery failure.
throw new AntiforgeryValidationException(Resources.AntiforgeryToken_UnableToReadRequest, ex);
}

@pranavkm
Copy link
Contributor Author

Is the intent here to just have this middleware validate antiforgery tokens/cookies/headers if present in the request? i.e. issuing tokens and sending them back to the client is still up to the framework or manual app code.

Today, Antiforgery is implemented as a filter in MVC. There's an attribute that also acts as an auth filter. It reads the form and performs a validation. Razor Pages add an implicit antiforgery filter that is active for all non-idempotent (POST / PUT / DELETE etc) requests. We'd like to enable Antiforgery to become available outside of MVC. My goal was to continue relying on these attributes, but as metadata that indicated if the user intended to perform antiforgery validation for that endpoint.

In minimal endpoints, this might look like:

app.MapPost("/post", (IFormFile formFile) => { ... })
   .ValidateAntiforgery(); // Maybe we can infer this based on the presence of things that come from a HTTP form.

Generating the antiforgery token that goes in to the form is still tied to the app framework. MVC will generate if you're using the form tag helper / Html.BeginForm(). For a form post using minimal, a user can use the IAntiforgery service to produce it. This PR doesn't attempt to make that experience better.

@pranavkm pranavkm closed this Feb 10, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants