[Analyzer] Prefer HttpRequest.ReadFormAsync over HttpRequest.Form

JamesNK commented 1 year ago

Background and Motivation

HttpRequest.Form is a blocking API (it reads the request body which may not be available yet). Using it can cause thread exhaustion and server throughput problems.

HttpRequest.Form is confusing to people as similar APIs such HttpRequest.QueryString, HttpRequest.Cookies, HttpRequest.Headers, etc, are safe to use.

Proposed Analyzer

Analyzer Behavior and Message

Using HttpRequest.Form should generate a warning. There are some cases where it is safe to use, so we need to figure out how broad to make the warning.

One option could be:

API used inside an async method = warning
API used inside a sync method = info

See discussion at https://github.com/dotnet/aspnetcore/issues/44390#issuecomment-1269279517 for more info.

Severity Level

[ ] Error
[x] Warning (async method?)
[x] Info (sync method?)
[ ] Hidden

Usage Scenarios

public Task<Result> SaveProduct()
{
    var productName = Request.Form["product_name"]; // WARNING HERE
    _repository.Add(new Product { Name = productName });
    await _repository.SaveAsync();
    return SuccessResult();
}

Risks

HttpRequest.Form is safe if the form has already been loaded. People who know and rely on that behavior and like the terseness of HttpRequest.Form would be impacted. They could either suppress the analyzer or assign the form to a local variable:

public Task<Result> SaveProduct()
{
    var form = Request.ReadFormAsync();
    var productName = form["product_name"];
    var productPrice = form["product_price"];
    // etc
}

Tratcher commented 1 year ago

HttpRequest.Form is safe if the form has already been loaded. People who know and rely on that behavior and like the terseness of HttpRequest.Form would be impacted.

This is why we haven't attempted this in the past. I don't think an analyzer can reliably detect when Form is safe or not.

Alternative: Change the runtime behavior instead so Form throws instead of doing unsafe sync over async. Include an appcontext switch for back-compat. There should be a property/method you can call to see if the form was read to avoid the exception.

ghost commented 1 year ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

dotnet / aspnetcore