Suggest using parameter binding instead of manual parsing of HttpContext

[davidfowl]

Background and Motivation

Although we support scenarios where users can manual bind parameters from the HttpContext associated with their route handler, it is generally recommended to leverage the existing binding logic in ASP.NET to support this.

Note: we prototyped this analyzer as part of the 2022 hackathon.

Proposed Analyzer

Analyzer Behavior and Message

When we determine that the user has accessed the HttpContext.HttpRequest value to extract parameters, provide a warning with the following message.

Recommend using built-in parameter binding instead of manually processing HTTP request.

Category

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

Severity Level

• Error
• Warning
• Info
• Hidden

Usage Scenarios

Some patterns we can detect:

Untyped parsing

app.MapGet("/hey/{name}", (HttpRequest req) => $"Hello {req.RouteValues["name"]}");

Suggested fix:

app.MapGet("/hey/{name}", (string name) => $"Hello {name}");

We should can remove the HttpRequest if it's not referenced by anything in the method.

Typed parsing

If we can detect the parsing of those members:

app.MapGet("/todos/{id}", (HttpRequest req) =>
{
    if (int.TryParse((string?)req.RouteValues["id"], out var id))
    {
        return Results.BadRequest();
    }
    
    var todo = db.Find(id);
    if (todo is null)
    {
        return Results.BadRequest();
    }

    return Results.Ok(todo);
});

To

app.MapGet("/todos/{id}", (int id) =>
{
    var todo = db.Find(id);
    if (todo is null)
    {
        return Results.BadRequest();
    }

    return Results.Ok(todo);
});

This should also work for the query string:

app.MapGet("/todos", async (HttpContext context) =>
{
    if (!int.TryParse(context.Request.Query["pageIndex"], out var pageIndex))
    {
        context.Response.StatusCode = 400;
        return;
    }

    if (!int.TryParse(context.Request.Query["pageSize"], out var pageSize))
    {
        context.Response.StatusCode = 400;
        return;
    }

    await context.Response.WriteAsJsonAsync(db.GetTodos().Take(pageIndex).Skip(pageSize));
});

Suggested change:

app.MapGet("/todos", (int pageIndex, int pageSize, HttpContext context) =>
{
    await context.Response.WriteAsJsonAsync(db.GetTodos().Take(pageIndex).Skip(pageSize));
});

Risks

There are some scenarios where users may prefer to manually parse parameters from HttpContext.HttpRequest but the warning level of this analyzer is sufficient.

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.

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.