[Analyzer] Prefer HttpRequest.ReadFormAsync over HttpRequest.Form

[JamesNK]

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 #44390 (comment) for more info.

Category

• Design
• Documentation
• Globalization
• Interoperability
• Maintainability
• Naming
• Performance
• Reliability
• Security
• Style
• Usage

Severity Level

• Error
• Warning (async method?)
• 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]

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]

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.