DefaultAntiforgeryTokenStore ignores cookie Path set in CookieBuilder.Build
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 281
Description
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Describe the bug
[DefaultAntiforgeryTokenStore](https://github.com/dotnet/aspnetcore/blob/v10.0.2/src/Antiforgery/src/Internal/DefaultAntiforgeryTokenStore.cs#L86) has code to default the cookie path to `context.Request.PathBase` if there is no Path specified in configuration:
```
public void SaveCookieToken(HttpContext httpContext, string token)
{
Debug.Assert(httpContext != null);
Debug.Assert(token != null);
var options = _options.Cookie.Build(httpContext);
if (_options.Cookie.Path != null)
{
options.Path = _options.Cookie.Path;
}
else
{
var pathBase = httpContext.Request.PathBase.ToString();
if (!string.IsNullOrEmpty(pathBase))
{
options.Path = pathBase;
}
}
httpContext.Response.Cookies.Append(_options.Cookie.Name!, token, options);
}
```
However, the logic here checks `if (_options.Cookie.Path != null)` rather than `if (options.Path != null)`.
The result of this is that if the `CookieBuilder` instance on `_options` has provided a path to use for the cookie during `_options.Cookie.Build(httpContext)`, then that path is ignored (unless `context.Request.PathBase` happens to be null).
In our situation, our routing configuration means that at the point `DefaultAntiforgeryTokenStore.SaveCookieToken` is called, PathBase is incorrect. Rather than have a hardcoded `Path` (which could change per request), or changing our routing configuration, we would like to reuse [RequestPathBaseCookieBuilder](https://github.com/dotnet/aspnetcore/blob/v10.0.2/src/Security/Authentication/Core/src/RequestPathBaseCookieBuilder.cs) and use that to set a sensible cookie path (as it is already doing for authentication cookies).
I assume that the reason the code above is checking `_options.Cookie.Path` rather than `options.Path` is because the default `CookieBuilder` normalizes the resulting `CookieOptions.Path` to `"/"` if `CookieBuilder.Path` is null, and as such checking `options.Path != null` would never be true.
As a suggestion, changing [AntiforgeryOptions](https://github.com/dotnet/aspnetcore/blob/main/src/Antiforgery/src/AntiforgeryOptions.cs#L18) to use a custom `CookieBuilder` subclass, and moving the existing logic out of `DefaultAntiforgeryTokenStore` and into the new builder would address the issue:
```
public class AntiforgeryCookieBuilder : CookieBuilder {
public override CookieOptions Build(HttpContext context, DateTimeOffset expiresFrom) {
CookieOptions cookieOptions = base.Build(context, expiresFrom);
// The base CookieBuilder sets cookieOptions.Path to "/" if this.Path is null.
// We override this behavior to use the request's PathBase in preference to "/" if available.
if (this.Path == null) {
string pathBase = context.Request.PathBase.ToString();
if (string.IsNullOrEmpty(pathBase)) {
cookieOptions.Path = pathBase;
}
}
return cookieOptions;
}
}
```
The risk here would be if existing users are relying on the behaviour where they override `options.CookieBuilder` (i.e. setting it to a new instance rather than changing properties on the default `CookieBuilder` instance), and they _also_ leave `Path` on that `CookieBuilder` unset. In that case the behaviour would change: the resulting final cookie path would be `"/"` rather than `request.PathBase`. This shouldn't _break_ anything, but could expose the anti-forgery cookie to a wider set of URLs than previously (but note that the default options for this cookie aren't that strict currently anyway, e.g. `CookieSecurePolicy.None`, and that the cookie on its own is not sufficient to bypass anti-forgery protection).
### Expected Behavior
If a custom `CookieBuilder` class is used (e.g. `RequestPathBaseCookieBuilder`), the `Path` property on the `CookieOptions` object returned by `Build` should be used in preference to the `Path` property on the custom CookieBuilder object.
### Steps To Reproduce
```
using Microsoft.AspNetCore.Antiforgery;
public partial class Program {
private class CustomCookieBuilder : CookieBuilder {
public override CookieOptions Build(HttpContext context, DateTimeOffset expiresFrom) {
CookieOptions cookieOptions = base.Build(context, expiresFrom);
// override the cookie path. This is what we *want* the resulting cookie path to be,
// however this will currently be overwritten to request.PathBase.
cookieOptions.Path = "/custom-path";
return cookieOptions;
}
}
private static void Main(string[] args) {
var builder = WebApplication.CreateBuilder(args);
const string cookieName = "AntiforgeryCookie";
builder.Services.AddAntiforgery(options => {
options.Cookie = new CustomCookieBuilder {
Name = cookieName
};
});
var app = builder.Build();
app.MapGet("/", (HttpContext context, IAntiforgery antiforgeryService) => {
context.Request.PathBase = "/some-path-base";
antiforgeryService.GetAndStoreTokens(context);
// string shown in the browser will have "path=/some-path-base", not
// "path=/custom-path" as specified by CustomCookieBuilder above.
return context.Response.Headers.SetCookie.ToString();
});
app.Run();
}
}
```
### Exceptions (if any)
_No response_
### .NET Version
10.0.102
### Anything else?
_No response_
Contributor guide
Assessment
This issue has not been assessed yet.