diff --git a/code-reviews/Admin/findings.md b/code-reviews/Admin/findings.md index 1833584..42c153a 100644 --- a/code-reviews/Admin/findings.md +++ b/code-reviews/Admin/findings.md @@ -108,13 +108,13 @@ | Severity | Medium | | Category | Security | | Location | `Components/Layout/MainLayout.razor:47-49`, `Program.cs:129,131-135` | -| Status | Open | +| Status | Resolved | **Description:** `app.UseAntiforgery()` is enabled, but the Sign-out form (`
`) renders no antiforgery token, and the `MapPost("/auth/logout", ...)` endpoint does not call `.DisableAntiforgery()` or otherwise opt out. Depending on framework version this either makes logout fail with a 400 for legitimate users, or — if the endpoint is treated as exempt — leaves logout as an unprotected state-changing POST (CSRF logout). The same concern applies to the login form once Admin-005 is addressed. **Recommendation:** Emit an antiforgery token in the logout form and let `UseAntiforgery()` validate it; or explicitly and deliberately mark the endpoint `.DisableAntiforgery()` if a tokenless logout is intended. Verify login/logout round-trips after the change. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `` added to the sign-out form in `MainLayout.razor` and `.DisableAntiforgery()` removed from the `/auth/logout` endpoint so `UseAntiforgery()` validates the token; a tokenless POST now returns 400, preventing CSRF-logout. The login endpoint retains `.DisableAntiforgery()` (login is not a state-changing operation CSRF can abuse). `AuthEndpointsTests.Logout_without_antiforgery_token_is_rejected` regression-guards this. ### Admin-007 diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Layout/MainLayout.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Layout/MainLayout.razor index 7b300ad..85d991c 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Layout/MainLayout.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Layout/MainLayout.razor @@ -45,6 +45,7 @@ .Where(c => c.Type.EndsWith("/role")).Select(c => c.Value)) + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Security/AuthEndpoints.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Security/AuthEndpoints.cs index 5aae189..31c1397 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Security/AuthEndpoints.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Security/AuthEndpoints.cs @@ -21,15 +21,21 @@ public static class AuthEndpoints public static IEndpointRouteBuilder MapAuthEndpoints(this IEndpointRouteBuilder endpoints) { // Anonymous: the login POST is the only way in, so the fallback authorization policy - // (Admin-001) must not gate it. DisableAntiforgery — the static form posts with - // data-enhance="false" and renders no token; the cookie scheme + LDAP bind are the - // gate here. (Admin-006 covers emitting a token for a hardened build.) + // (Admin-001) must not gate it. DisableAntiforgery — the static Login.razor form posts + // with data-enhance="false" and renders no antiforgery token; the cookie scheme + LDAP + // bind are the authentication gate here. Login is not a state-changing operation that + // CSRF can abuse (the attacker cannot know the resulting cookie), so tokenless-login is + // the standard Web pattern. endpoints.MapPost("/auth/login", (Delegate)LoginAsync) .AllowAnonymous() .DisableAntiforgery(); - endpoints.MapPost("/auth/logout", (Delegate)LogoutAsync) - .DisableAntiforgery(); + // Admin-006: the logout form in MainLayout.razor emits so the + // middleware validates the token. This prevents a cross-site logout (CSRF-logout) where + // an attacker tricks the operator's browser into posting to /auth/logout. The endpoint + // intentionally does NOT call .DisableAntiforgery() — the token must be present and + // valid; the middleware rejects forged or missing tokens with 400. + endpoints.MapPost("/auth/logout", (Delegate)LogoutAsync); return endpoints; } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AuthEndpointsTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AuthEndpointsTests.cs index 7c324f4..682b850 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AuthEndpointsTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/AuthEndpointsTests.cs @@ -148,16 +148,19 @@ public sealed class AuthEndpointsTests : IClassFixture>())); - // No antiforgery 400 — the endpoint opts out (Admin-006 note in AuthEndpoints). - response.StatusCode.ShouldBeOneOf(HttpStatusCode.Redirect, HttpStatusCode.Found); - response.Headers.Location!.OriginalString.ShouldContain("/login"); + response.StatusCode.ShouldBe(HttpStatusCode.BadRequest, + "/auth/logout without an antiforgery token must be rejected (Admin-006)"); } // ── Admin-003: SignalR hubs reject anonymous connections ────────────────────