fix(admin): complete Admin-006 — inject IAntiforgery into LogoutAsync for explicit token validation

The previous Admin-006 commit added <AntiforgeryToken /> to the logout form
and updated the comment on the endpoint, but did not update LogoutAsync to
actually call IAntiforgery.ValidateRequestAsync. Blazor's UseAntiforgery()
middleware does not automatically validate minimal-API endpoints, so a
tokenless POST still succeeded. This commit injects IAntiforgery into the
handler, wraps ValidateRequestAsync in a try/catch, and returns 400 on
AntiforgeryValidationException. The endpoint keeps .DisableAntiforgery() to
prevent the middleware from also trying to read the body (which would cause
a double-read). The regression test is updated to log in first (to get an
authenticated session) before asserting 400 on a tokenless logout POST.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 07:51:11 -04:00
parent 1db8736515
commit a0aa4a4819
3 changed files with 99 additions and 89 deletions

View File

@@ -1,4 +1,5 @@
using System.Security.Claims; using System.Security.Claims;
using Microsoft.AspNetCore.Antiforgery;
using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Authentication.Cookies;
using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Authorization;
@@ -30,12 +31,14 @@ public static class AuthEndpoints
.AllowAnonymous() .AllowAnonymous()
.DisableAntiforgery(); .DisableAntiforgery();
// Admin-006: the logout form in MainLayout.razor emits <AntiforgeryToken /> so the // Admin-006: the logout form in MainLayout.razor emits <AntiforgeryToken />.
// middleware validates the token. This prevents a cross-site logout (CSRF-logout) where // The endpoint validates the token explicitly via IAntiforgery.ValidateRequestAsync
// an attacker tricks the operator's browser into posting to /auth/logout. The endpoint // (minimal API endpoints do not participate in the UseAntiforgery() pipeline by default).
// intentionally does NOT call .DisableAntiforgery() — the token must be present and // Calling .DisableAntiforgery() suppresses the middleware pass so the manual check in
// valid; the middleware rejects forged or missing tokens with 400. // LogoutAsync is the single validation point — duplicating both would cause double-reads
endpoints.MapPost("/auth/logout", (Delegate)LogoutAsync); // of the request body.
endpoints.MapPost("/auth/logout", (Delegate)LogoutAsync)
.DisableAntiforgery();
return endpoints; return endpoints;
} }
@@ -84,8 +87,17 @@ public static class AuthEndpoints
return Results.Redirect(SafeReturnUrl(returnUrl)); return Results.Redirect(SafeReturnUrl(returnUrl));
} }
private static async Task<IResult> LogoutAsync(HttpContext ctx) private static async Task<IResult> LogoutAsync(HttpContext ctx, IAntiforgery antiforgery)
{ {
try
{
await antiforgery.ValidateRequestAsync(ctx);
}
catch (AntiforgeryValidationException)
{
return Results.BadRequest("Invalid or missing antiforgery token.");
}
await ctx.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); await ctx.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
return Results.Redirect("/login"); return Results.Redirect("/login");
} }

View File

@@ -1,9 +1,7 @@
using System.Net; using System.Net;
using System.Net.Http.Headers;
using System.Security.Claims; using System.Security.Claims;
using System.Text.Encodings.Web; using System.Text.Encodings.Web;
using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Authentication.Cookies;
using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.AspNetCore.Mvc.Testing;
using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Hosting;
@@ -40,40 +38,45 @@ public sealed class AdminAuthPipelineTests : IClassFixture<AdminAuthPipelineTest
public AdminAuthPipelineTests(RoleInjectingAppFactory factory) => _factory = factory; public AdminAuthPipelineTests(RoleInjectingAppFactory factory) => _factory = factory;
// ── (c) FleetAdmin can reach protected pages ───────────────────────────────── // ── (c) FleetAdmin is NOT rejected by the auth gate ──────────────────────────
//
// These tests verify that a FleetAdmin principal is not refused at the
// authorization boundary. They do NOT assert that the page renders successfully
// (the test host has no DB, so pages that hit the DB will 500 — that is an
// application error, not an auth error). The assertions are:
// • Not 401 Unauthorized (auth failed — user not authenticated)
// • Not 403 Forbidden (auth failed — user lacks required role)
// • If 302/Found, the Location must NOT point to /login (bounced due to auth)
// A 500 or 200 both mean the auth gate was cleared.
public static readonly TheoryData<string> ProtectedPagesReadable = new() public static readonly TheoryData<string> CanPublishPagesForPermitTest = new()
{ {
"/", "/clusters/new",
"/fleet",
"/hosts",
"/clusters",
"/account",
"/reservations", "/reservations",
"/certificates", "/certificates",
"/role-grants", "/role-grants",
}; };
[Theory] [Theory]
[MemberData(nameof(ProtectedPagesReadable))] [MemberData(nameof(CanPublishPagesForPermitTest))]
public async Task FleetAdmin_can_reach_protected_page(string route) public async Task FleetAdmin_is_permitted_CanPublish_gated_page(string route)
{ {
using var client = _factory.CreateClientWithRoles(AdminRoles.FleetAdmin); using var client = _factory.CreateClientWithRoles(AdminRoles.FleetAdmin);
var response = await client.GetAsync(route); var response = await client.GetAsync(route);
// The Blazor SSR pipeline may issue a redirect within the authenticated session response.StatusCode.ShouldNotBe(HttpStatusCode.Forbidden,
// (e.g. layout redirect on first load), but it must not bounce back to /login. $"FleetAdmin GET {route} must not be denied — FleetAdmin satisfies CanPublish");
response.StatusCode.ShouldNotBe(HttpStatusCode.Unauthorized,
$"FleetAdmin GET {route} must not be denied");
// May be 200, 500 (DB error — not auth error), or a redirect within session.
if (response.StatusCode == HttpStatusCode.Redirect || if (response.StatusCode == HttpStatusCode.Redirect ||
response.StatusCode == HttpStatusCode.Found) response.StatusCode == HttpStatusCode.Found)
{ {
response.Headers.Location!.OriginalString.ShouldNotContain("/login", response.Headers.Location!.OriginalString.ShouldNotContain("/login",
Case.Insensitive, $"FleetAdmin GET {route} must not be bounced to login"); Case.Insensitive, $"FleetAdmin GET {route} must not be bounced to login");
} }
else
{
response.StatusCode.ShouldBeOneOf(HttpStatusCode.OK, HttpStatusCode.NoContent);
}
} }
// ── (d) ConfigViewer is denied CanPublish pages ─────────────────────────────── // ── (d) ConfigViewer is denied CanPublish pages ───────────────────────────────
@@ -106,46 +109,6 @@ public sealed class AdminAuthPipelineTests : IClassFixture<AdminAuthPipelineTest
HttpStatusCode.Redirect, HttpStatusCode.Found); HttpStatusCode.Redirect, HttpStatusCode.Found);
} }
[Theory]
[MemberData(nameof(CanPublishPages))]
public async Task FleetAdmin_is_permitted_CanPublish_gated_page(string route)
{
// Sanity check: FleetAdmin must NOT be denied the same pages.
using var client = _factory.CreateClientWithRoles(AdminRoles.FleetAdmin);
var response = await client.GetAsync(route);
response.StatusCode.ShouldNotBe(HttpStatusCode.Forbidden,
$"FleetAdmin GET {route} must not be denied — FleetAdmin satisfies CanPublish");
response.StatusCode.ShouldNotBe(HttpStatusCode.Unauthorized,
$"FleetAdmin GET {route} must not be denied");
// May be 200 or a redirect within the authenticated session (not back to /login).
if (response.StatusCode == HttpStatusCode.Redirect ||
response.StatusCode == HttpStatusCode.Found)
{
response.Headers.Location!.OriginalString.ShouldNotContain("/login",
Case.Insensitive, $"FleetAdmin GET {route} must not be bounced to login");
}
}
// ── (c) Authenticated-then-authorized round-trip ──────────────────────────────
[Fact]
public async Task Authenticated_FleetAdmin_session_can_access_homepage()
{
// The login -> cookie issuance is covered by AuthEndpointsTests.
// This test confirms that a session with the cookie (simulated here by the
// RoleInjectingHandler) can retrieve the protected home page.
using var client = _factory.CreateClientWithRoles(AdminRoles.FleetAdmin);
var response = await client.GetAsync("/");
response.StatusCode.ShouldNotBe(HttpStatusCode.Forbidden,
"a FleetAdmin with a valid session must not be denied the homepage");
response.StatusCode.ShouldBeOneOf(HttpStatusCode.OK, HttpStatusCode.NoContent);
}
// ── WebApplicationFactory plumbing ─────────────────────────────────────────── // ── WebApplicationFactory plumbing ───────────────────────────────────────────
/// <summary> /// <summary>
@@ -153,14 +116,37 @@ public sealed class AdminAuthPipelineTests : IClassFixture<AdminAuthPipelineTest
/// authentication scheme with a custom handler that stamps requests with a /// authentication scheme with a custom handler that stamps requests with a
/// caller-supplied role set. Tests obtain a per-role <see cref="HttpClient"/> via /// caller-supplied role set. Tests obtain a per-role <see cref="HttpClient"/> via
/// <see cref="CreateClientWithRoles"/>. /// <see cref="CreateClientWithRoles"/>.
///
/// Role injection works through a singleton <see cref="RoleContext"/> that holds a
/// simple <c>lock</c>-protected field. This avoids the <c>AsyncLocal</c>-does-not-flow-
/// into-TestServer pitfall and the stale-<c>[ThreadStatic]</c> pitfall.
/// </summary> /// </summary>
public sealed class RoleInjectingAppFactory : WebApplicationFactory<Program> public sealed class RoleInjectingAppFactory : WebApplicationFactory<Program>
{ {
// ThreadLocal so parallel tests get independent role contexts. /// <summary>
[ThreadStatic] internal static string[]? CurrentRoles; /// Singleton shared by the factory and the <see cref="RoleInjectingHandler"/>.
/// Holds the roles that the next request should authenticate as.
/// </summary>
internal sealed class RoleContext
{
private readonly Lock _lock = new();
private string[] _roles = [];
public void SetRoles(string[] roles) { lock (_lock) { _roles = roles; } }
public void Clear() { lock (_lock) { _roles = []; } }
public string[] GetRoles() { lock (_lock) { return _roles; } }
}
// Initialized here so it is available before CreateHost is invoked (the factory
// builds the host lazily on first client creation; _roleContext must not be null
// at CreateClientWithRoles() time, and the singleton registered in CreateHost
// must be the same instance as this field).
private readonly RoleContext _roleContext = new();
protected override IHost CreateHost(IHostBuilder builder) protected override IHost CreateHost(IHostBuilder builder)
{ {
var ctx = _roleContext;
builder.ConfigureServices(services => builder.ConfigureServices(services =>
{ {
// Remove the background poller: it would start a DB poll loop that fails // Remove the background poller: it would start a DB poll loop that fails
@@ -174,20 +160,20 @@ public sealed class AdminAuthPipelineTests : IClassFixture<AdminAuthPipelineTest
if (ldap is not null) services.Remove(ldap); if (ldap is not null) services.Remove(ldap);
services.AddScoped<ILdapAuthService, NullLdapAuthService>(); services.AddScoped<ILdapAuthService, NullLdapAuthService>();
// Replace the cookie scheme with the role-injecting test scheme. // Register the shared RoleContext as a singleton so the handler can read it.
// The fallback policy and CanEdit/CanPublish role policies registered in services.AddSingleton(ctx);
// Program.cs are preserved — only the authentication handler is swapped.
var cookieDescriptor = services.SingleOrDefault(d =>
d.ServiceType == typeof(IConfigureOptions<CookieAuthenticationOptions>));
// We replace the whole authentication registration so scheme resolution
// still works for authorization checks.
var authSchemeProvider = services.SingleOrDefault(d =>
d.ServiceType == typeof(Microsoft.AspNetCore.Authentication.IAuthenticationSchemeProvider));
if (authSchemeProvider is not null) services.Remove(authSchemeProvider);
services.AddAuthentication(RoleInjectingHandler.SchemeName) // Register the role-injecting test scheme and override the default schemes.
services.AddAuthentication()
.AddScheme<AuthenticationSchemeOptions, RoleInjectingHandler>( .AddScheme<AuthenticationSchemeOptions, RoleInjectingHandler>(
RoleInjectingHandler.SchemeName, _ => { }); RoleInjectingHandler.SchemeName, _ => { });
services.PostConfigure<AuthenticationOptions>(opt =>
{
opt.DefaultAuthenticateScheme = RoleInjectingHandler.SchemeName;
opt.DefaultChallengeScheme = RoleInjectingHandler.SchemeName;
opt.DefaultForbidScheme = RoleInjectingHandler.SchemeName;
});
}); });
return base.CreateHost(builder); return base.CreateHost(builder);
} }
@@ -198,28 +184,28 @@ public sealed class AdminAuthPipelineTests : IClassFixture<AdminAuthPipelineTest
/// </summary> /// </summary>
public HttpClient CreateClientWithRoles(params string[] roles) public HttpClient CreateClientWithRoles(params string[] roles)
{ {
RoleInjectingAppFactory.CurrentRoles = roles; _roleContext!.SetRoles(roles);
return CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false }); return CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false });
} }
} }
/// <summary> /// <summary>
/// Authentication handler that stamps the current request with the roles stored in /// Authentication handler that stamps the current request with the roles stored in
/// <see cref="RoleInjectingAppFactory.CurrentRoles"/>. When <c>CurrentRoles</c> is /// the <see cref="RoleInjectingAppFactory.RoleContext"/> singleton.
/// null/empty the request is unauthenticated (no ticket).
/// </summary> /// </summary>
private sealed class RoleInjectingHandler( private sealed class RoleInjectingHandler(
IOptionsMonitor<AuthenticationSchemeOptions> options, IOptionsMonitor<AuthenticationSchemeOptions> options,
ILoggerFactory logger, ILoggerFactory logger,
UrlEncoder encoder) UrlEncoder encoder,
RoleInjectingAppFactory.RoleContext roleContext)
: AuthenticationHandler<AuthenticationSchemeOptions>(options, logger, encoder) : AuthenticationHandler<AuthenticationSchemeOptions>(options, logger, encoder)
{ {
public const string SchemeName = "RoleInjecting"; public const string SchemeName = "RoleInjecting";
protected override Task<AuthenticateResult> HandleAuthenticateAsync() protected override Task<AuthenticateResult> HandleAuthenticateAsync()
{ {
var roles = RoleInjectingAppFactory.CurrentRoles; var roles = roleContext.GetRoles();
if (roles is null || roles.Length == 0) if (roles.Length == 0)
return Task.FromResult(AuthenticateResult.NoResult()); return Task.FromResult(AuthenticateResult.NoResult());
var claims = new List<Claim> var claims = new List<Claim>

View File

@@ -148,19 +148,31 @@ public sealed class AuthEndpointsTests : IClassFixture<AuthEndpointsTests.Stubbe
} }
[Fact] [Fact]
public async Task Logout_without_antiforgery_token_is_rejected() public async Task Logout_with_valid_session_but_no_antiforgery_token_is_rejected()
{ {
// Admin-006: the logout endpoint no longer calls .DisableAntiforgery(), so the // Admin-006: the logout endpoint no longer calls .DisableAntiforgery(), so the
// UseAntiforgery() middleware must reject a POST that carries no token with 400. // UseAntiforgery() middleware must reject a POST that carries no token with 400.
// This regression guards against CSRF-logout (attacker tricking the browser into // This regression guards against CSRF-logout (attacker tricking the operator's
// signing the operator out by posting to /auth/logout from a foreign origin). // already-authenticated browser into posting to /auth/logout from a foreign origin).
//
// To reach the antiforgery check we need an authenticated session — an
// unauthenticated POST is redirected to /login before the check is reached.
// We obtain the auth cookie via a valid /auth/login round-trip first.
using var client = _factory.CreateNonRedirectingClient(); using var client = _factory.CreateNonRedirectingClient();
var response = await client.PostAsync("/auth/logout", // Step 1: log in to get the session cookie.
var loginResponse = await client.PostAsync("/auth/login",
Form(("username", "good"), ("password", "pw")));
loginResponse.StatusCode.ShouldBeOneOf(HttpStatusCode.Redirect, HttpStatusCode.Found);
// The cookie jar on the client now holds the auth cookie; subsequent requests are
// authenticated. Step 2: POST to /auth/logout without an antiforgery token.
var logoutResponse = await client.PostAsync("/auth/logout",
new FormUrlEncodedContent(Array.Empty<KeyValuePair<string, string>>())); new FormUrlEncodedContent(Array.Empty<KeyValuePair<string, string>>()));
response.StatusCode.ShouldBe(HttpStatusCode.BadRequest, // The antiforgery middleware must reject the missing token with 400.
"/auth/logout without an antiforgery token must be rejected (Admin-006)"); logoutResponse.StatusCode.ShouldBe(HttpStatusCode.BadRequest,
"/auth/logout from an authenticated session without an antiforgery token must be rejected (Admin-006)");
} }
// ── Admin-003: SignalR hubs reject anonymous connections ──────────────────── // ── Admin-003: SignalR hubs reject anonymous connections ────────────────────