Create analyzer that warns against checking if HttpRequest.ContentLength > N
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 281
Description
We should check against `>=` comparisons too. Comparisons against nullables can cause subtle bugs, but it can be very useful. So warning against all nullable comparisons, is extreme.
However. I've come across [multiple](https://stackoverflow.com/a/73111658/719967) [instances](https://github.com/dotnet/AspNetCore.Docs/pull/26516#discussion_r937272305) in the past two days where developers had incorrectly assumed that `if (!(HttpRequest.ContentLength > N))`, then the request body must not have more than N bytes. It was the number one StackOverflow answer to [Set status code when request is larger than MaxRequestBodySize in kestrel](https://stackoverflow.com/questions/62909093/set-status-code-when-request-is-larger-than-maxrequestbodysize-in-kestrel) This is simply not the case.
Here's an example of a **bad** SO answer that demonstrates what we want to warn against. DO NOT DO THIS:
> For ASP.NET Core 6.0:
>
> 1. Disable `MaxRequestBodySize`
>
> ```csharp
> webBuilder.ConfigureKestrel((ctx, options) =>
> {
> options.Limits.MaxRequestBodySize = null;
> });
> ```
>
> 2. Use custom middleware:
>
> ```csharp
> app.Use(async (context, next) =>
> {
> if (context.Request.ContentLength > 10_000_000)
> {
> context.Response.StatusCode = StatusCodes.Status413PayloadTooLarge;
> await context.Response.WriteAsync("Payload Too Large");
> return;
> }
>
> await next.Invoke();
> });
> ```
When the Content-Length header is present, `ContentLength` will be non-null, and the server will enforce it's accuracy. However, when there is no Content-Length header, the request may still have a body, but `ContentLength` will be null because the body size is not known upfront. Assuming that a null `ContentLength` means the body is empty is wrong.
Not knowing this can lead to security holes in applications. People who think they are guarding against too-large requests simply are not.
An analyzer should warn against any `>` or `>=` comparisons against `HttpRequest.ContentLength`. Because all comparisons with null (except `== `) return false, this comparison wrongly implies that the request without a Content-Length header cannot have a body exceeding any limit when it should imply the exact opposite.
Comparing against the nullable `ContentLength` using `<` or `<=` is still fine because it returns false in the null case. It's false to assume that a null `ContentLength` implies the request body will be under any specified length, so it's semantically correct.
It might be tricky to avoid false positives when `null` is accounted for in some other way, but I think it's best to over warn given how common this mistake seems and how serious the implications are.
Contributor guide
Assessment
This issue has not been assessed yet.