dotnet / dotnet/aspnetcore

Request timeouts are not applied without the middleware, no error given

Open
#52,025 3 comments 0 reactions 0 assignees View on GitHub
area-middleware area-routing
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 5h
Merged PRs (30d)
276

Description

### Is there an existing issue for this?

- [X] I have searched the existing issues

### Describe the bug

The request timeout feature relies on options, middleware, and attributes. If uses supply options or attributes, but forget the middleware, there's no indication that the timeouts aren't being applied.

### Expected Behavior

Configuring timeouts on routes (or a default timeout policy ?) without adding the middleware should produce at least a runtime error rather than process the request without a timeout (dangerous). Note we also shouldn't short circuit endpoints with timeouts, we don't know what the short circuit logic is going to do.

An analyzer might help too.

We already have similar checks for auth, cors, and anti-forgery.
https://github.com/dotnet/aspnetcore/blob/997a1e8e659e69dbf7e157518da18cbca91f5183/src/Http/Routing/src/EndpointRoutingMiddleware.cs#L161-L178
https://github.com/dotnet/aspnetcore/blob/997a1e8e659e69dbf7e157518da18cbca91f5183/src/Http/Routing/src/EndpointMiddleware.cs#L39-L58

Found when consuming this feature in YARP, and mitigated there: https://github.com/microsoft/reverse-proxy/pull/2307

### Steps To Reproduce

```csharp
using Microsoft.AspNetCore.Http.Timeouts;

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddRequestTimeouts();

var app = builder.Build();
// app.UseRequestTimeouts(); // Woops, forgot this, nothing works.

app.MapGet("/", async (HttpContext context) => {
try
{
await Task.Delay(TimeSpan.FromSeconds(10), context.RequestAborted);
}
catch (TaskCanceledException)
{
return Results.Content("Timeout!", "text/plain");
}

return Results.Content("No timeout!", "text/plain");
}).WithRequestTimeout(TimeSpan.FromSeconds(2));
// Returns "Timeout!"

app.MapGet("/attribute",
[RequestTimeout(milliseconds: 2000)] async (HttpContext context) => {
try
{
await Task.Delay(TimeSpan.FromSeconds(10), context.RequestAborted);
}
catch (TaskCanceledException)
{
return Results.Content("Timeout!", "text/plain");
}

return Results.Content("No timeout!", "text/plain");
});
// Returns "Timeout!"

app.Run();
```

### Exceptions (if any)

None (but there should be).

### .NET Version

.NET 8

### Anything else?

This enforcement pattern isn't scalable/extensible. 3rd parties can't use the Endpoint/RoutingMiddleware to do their own enforcement for the presence of middleware. And for us, if we keep adding checks it's going to get messy. Could this be abstracted to a service? What happens if the user then forgets to add the service?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.