diff --git a/code-reviews/Admin/findings.md b/code-reviews/Admin/findings.md index 86c825f..57a8645 100644 --- a/code-reviews/Admin/findings.md +++ b/code-reviews/Admin/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 12 | +| Open findings | 10 | ## Checklist coverage @@ -33,13 +33,13 @@ | Severity | Critical | | Category | Security | | Location | `Components/Routes.razor:4-11`, `Program.cs:150` | -| Status | Open | +| Status | Resolved | **Description:** The router uses a plain `RouteView` (not `AuthorizeRouteView`), and `MapRazorComponents()` is registered without `.RequireAuthorization()`. A page-level `[Authorize]` attribute on a routable Razor component is only enforced when the router is `AuthorizeRouteView` — with `RouteView` the attribute is inert. Consequently every page in the app, including those that carry `@attribute [Authorize]` (`ClusterDetail`, `DraftEditor`, `Reservations`, `RoleGrants`, `Certificates`, `VirtualTags`, `ScriptedAlarms`, `ScriptLog`, `DiffViewer`, `ImportEquipment`, `Account`), is reachable by a fully unauthenticated user. There is no authentication gate anywhere in the pipeline. An anonymous browser can read the full fleet configuration, audit log, certificates and ACLs, and exercise mutating pages (see Admin-002). **Recommendation:** Replace `RouteView` with `AuthorizeRouteView` in `Routes.razor` (with a `` slot that redirects to `/login`), or call `.RequireAuthorization()` on the `MapRazorComponents` endpoint with `/login` and `/auth/*` explicitly allowed anonymous. Add a fallback policy (`AddAuthorizationBuilder().SetFallbackPolicy(...)`) so new pages are secure-by-default. Re-verify every page after the gate is in place. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `Routes.razor` switched to `AuthorizeRouteView` with a `NotAuthorized` slot routing unauthenticated callers to `/login` via a new `RedirectToLogin` component; `AddAuthorizationBuilder().SetFallbackPolicy(RequireAuthenticatedUser())` makes pages secure-by-default; `Login.razor` opts out with `[AllowAnonymous]` so the login page and static assets stay anonymous. Covered by `PageAuthorizationTests` (verified failing pre-fix, passing post-fix). ### Admin-002 @@ -48,13 +48,13 @@ | Severity | Critical | | Category | Security | | Location | `Components/Pages/Clusters/NewCluster.razor:1-7`, `Home.razor`, `Fleet.razor`, `Hosts.razor`, `AlarmsHistorian.razor`, `Clusters/ClustersList.razor`, `Clusters/Generations.razor`, `Drivers/FocasDetail.razor` | -| Status | Open | +| Status | Resolved | **Description:** Several routable pages carry no authorization attribute at all. Most critically `NewCluster` (`/clusters/new`) is a mutating page — its `CreateAsync` writes a new `ServerCluster` row and a draft generation. Combined with Admin-001 (the router does not enforce `[Authorize]` either), an unauthenticated user can create clusters and seed config-DB rows. `Home`, `Fleet`, `Hosts`, `AlarmsHistorian`, `ClustersList`, `Generations` and `FocasDetail` likewise expose fleet topology, host status, historian diagnostics and generation history to anonymous callers. **Recommendation:** Add `@attribute [Authorize(...)]` to every routable page with the role/policy appropriate to its function (`NewCluster` and other write surfaces -> `CanPublish`/`CanEdit`; read pages -> an authenticated-user policy). A solution-wide fallback policy (see Admin-001) is the durable fix; per-page attributes remain the explicit declaration of intent. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `@attribute [Authorize]` added to every unprotected routable page (`Home`, `Fleet`, `Hosts`, `AlarmsHistorian`, `ClustersList`, `FocasDetail`, `ModbusAddressPreview`, `ModbusDiagnostics`); `NewCluster` gated with `[Authorize(Policy = "CanPublish")]` per the admin-ui.md FleetAdmin cluster-create flow. Re-triage note: `Clusters/Generations.razor` carries no `@page` directive — it is a child component of `ClusterDetail`, not a routable page, so it needs no attribute (it inherits the parent route's gate). The Admin-001 fallback policy is the durable secure-by-default backstop; the per-page attributes are the explicit declaration of intent. Covered by `PageAuthorizationTests`. ### Admin-003 diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/AlarmsHistorian.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/AlarmsHistorian.razor index 4f34d37..34b1424 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/AlarmsHistorian.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/AlarmsHistorian.razor @@ -1,4 +1,5 @@ @page "/alarms/historian" +@attribute [Microsoft.AspNetCore.Authorization.Authorize] @using Microsoft.AspNetCore.Components.Web @using ZB.MOM.WW.OtOpcUa.Admin.Services @using ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/ClustersList.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/ClustersList.razor index 2e733e7..577d5a5 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/ClustersList.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/ClustersList.razor @@ -1,4 +1,5 @@ @page "/clusters" +@attribute [Microsoft.AspNetCore.Authorization.Authorize] @using ZB.MOM.WW.OtOpcUa.Admin.Services @using ZB.MOM.WW.OtOpcUa.Configuration.Entities @inject ClusterService ClusterSvc diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/NewCluster.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/NewCluster.razor index d3a4d8e..4a585b1 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/NewCluster.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Clusters/NewCluster.razor @@ -1,4 +1,8 @@ @page "/clusters/new" +@* Cluster creation is a FleetAdmin operation per admin-ui.md "Add a new cluster" — + CanPublish gates it (Admin-002). Without this attribute the page was reachable + and its CreateAsync write path exploitable by any caller. *@ +@attribute [Microsoft.AspNetCore.Authorization.Authorize(Policy = "CanPublish")] @using System.ComponentModel.DataAnnotations @using Microsoft.AspNetCore.Components.Web @using ZB.MOM.WW.OtOpcUa.Admin.Services diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Drivers/FocasDetail.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Drivers/FocasDetail.razor index 2cd13dd..b35266f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Drivers/FocasDetail.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Drivers/FocasDetail.razor @@ -1,4 +1,5 @@ @page "/drivers/focas/{InstanceId}" +@attribute [Microsoft.AspNetCore.Authorization.Authorize] @using ZB.MOM.WW.OtOpcUa.Admin.Services @inject FocasDriverDetailService DetailSvc diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Fleet.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Fleet.razor index e258558..d5779d0 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Fleet.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Fleet.razor @@ -1,4 +1,5 @@ @page "/fleet" +@attribute [Microsoft.AspNetCore.Authorization.Authorize] @using Microsoft.AspNetCore.Components.Web @using Microsoft.EntityFrameworkCore @using ZB.MOM.WW.OtOpcUa.Configuration diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Home.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Home.razor index 4c14634..96d3400 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Home.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Home.razor @@ -1,4 +1,5 @@ @page "/" +@attribute [Microsoft.AspNetCore.Authorization.Authorize] @using Microsoft.AspNetCore.Components.Web @using ZB.MOM.WW.OtOpcUa.Admin.Services @using ZB.MOM.WW.OtOpcUa.Configuration.Entities diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Hosts.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Hosts.razor index e51a631..f897969 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Hosts.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Hosts.razor @@ -1,4 +1,5 @@ @page "/hosts" +@attribute [Microsoft.AspNetCore.Authorization.Authorize] @using Microsoft.AspNetCore.Components.Web @using Microsoft.AspNetCore.SignalR.Client @using Microsoft.EntityFrameworkCore diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Login.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Login.razor index 56e9473..cec8f2a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Login.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Login.razor @@ -1,4 +1,7 @@ @page "/login" +@* The login page must stay anonymously reachable — otherwise the fallback + authorization policy (Admin-001) would lock operators out of the only way in. *@ +@attribute [Microsoft.AspNetCore.Authorization.AllowAnonymous] @using System.Security.Claims @using Microsoft.AspNetCore.Authentication @using Microsoft.AspNetCore.Authentication.Cookies diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Modbus/ModbusAddressPreview.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Modbus/ModbusAddressPreview.razor index ed64c32..6ae4e4e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Modbus/ModbusAddressPreview.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Modbus/ModbusAddressPreview.razor @@ -1,4 +1,5 @@ @page "/modbus/address-preview" +@attribute [Microsoft.AspNetCore.Authorization.Authorize] @using Microsoft.AspNetCore.Components.Web @using ZB.MOM.WW.OtOpcUa.Driver.Modbus @rendermode RenderMode.InteractiveServer diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Modbus/ModbusDiagnostics.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Modbus/ModbusDiagnostics.razor index 7d14694..fee2944 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Modbus/ModbusDiagnostics.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Pages/Modbus/ModbusDiagnostics.razor @@ -1,4 +1,5 @@ @page "/modbus/diagnostics/{DriverInstanceId}" +@attribute [Microsoft.AspNetCore.Authorization.Authorize] @using Microsoft.AspNetCore.Components.Web @using ZB.MOM.WW.OtOpcUa.Admin.Services @rendermode RenderMode.InteractiveServer diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/RedirectToLogin.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/RedirectToLogin.razor new file mode 100644 index 0000000..7ab743c --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/RedirectToLogin.razor @@ -0,0 +1,16 @@ +@* Server-side redirect to the login page for an unauthenticated request. + Used by AuthorizeRouteView's NotAuthorized slot (Admin-001). The current + path is carried through as returnUrl so the operator lands back where + they aimed after signing in. *@ +@inject NavigationManager Nav + +@code { + protected override void OnInitialized() + { + var returnUrl = Nav.ToBaseRelativePath(Nav.Uri); + var target = string.IsNullOrEmpty(returnUrl) || returnUrl == "login" + ? "login" + : $"login?returnUrl={Uri.EscapeDataString("/" + returnUrl)}"; + Nav.NavigateTo(target, forceLoad: true); + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Routes.razor b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Routes.razor index c23e1d4..06e028b 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Routes.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Components/Routes.razor @@ -1,9 +1,30 @@ +@using Microsoft.AspNetCore.Components.Authorization @using Microsoft.AspNetCore.Components.Routing @using ZB.MOM.WW.OtOpcUa.Admin.Components.Layout - + @* AuthorizeRouteView (not a plain RouteView) is what makes a page-level + [Authorize] attribute actually enforced — with RouteView the attribute + is inert (Admin-001). Unauthenticated users hit the NotAuthorized slot + and are bounced to /login; the route is preserved as returnUrl. *@ + + + @if (context.User.Identity?.IsAuthenticated != true) + { + + } + else + { + +

You do not have permission to view this page.

+
+ } +
+ +

Authorizing…

+
+

Not found.

diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Program.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Program.cs index 11e855b..7c98a6e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Program.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Admin/Program.cs @@ -28,9 +28,17 @@ builder.Services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationSc o.ExpireTimeSpan = TimeSpan.FromHours(8); }); +// Secure-by-default: the fallback policy requires an authenticated user for any +// endpoint (and any routable page) that carries no explicit authorization metadata, +// so a newly added page cannot accidentally ship anonymously reachable (Admin-001/002). +// Pages/endpoints that must stay anonymous opt out with [AllowAnonymous] — the login +// page, the /auth/* endpoints and static files all do. builder.Services.AddAuthorizationBuilder() .AddPolicy("CanEdit", p => p.RequireRole(AdminRoles.ConfigEditor, AdminRoles.FleetAdmin)) - .AddPolicy("CanPublish", p => p.RequireRole(AdminRoles.FleetAdmin)); + .AddPolicy("CanPublish", p => p.RequireRole(AdminRoles.FleetAdmin)) + .SetFallbackPolicy(new Microsoft.AspNetCore.Authorization.AuthorizationPolicyBuilder() + .RequireAuthenticatedUser() + .Build()); builder.Services.AddCascadingAuthenticationState(); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/PageAuthorizationTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/PageAuthorizationTests.cs new file mode 100644 index 0000000..7c00526 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/PageAuthorizationTests.cs @@ -0,0 +1,140 @@ +using System.Net; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Admin.Tests; + +/// +/// Regression coverage for Admin-001 / Admin-002 — the router must enforce page-level +/// [Authorize] attributes and the fallback authorization policy must keep every +/// routable page (and mutating route) secure-by-default, while the login page, the +/// /auth/* endpoints and static assets stay anonymously reachable. +/// +/// These are HTTP-pipeline tests: they do not touch the config DB (the +/// registration is lazy), so they run without the +/// central SQL Server. The hosted service is stripped out +/// so the test host does not spin up a background DB poll loop. +/// +public sealed class PageAuthorizationTests : IClassFixture +{ + private readonly AdminAppFactory _factory; + + public PageAuthorizationTests(AdminAppFactory factory) => _factory = factory; + + /// + /// A over the Admin app's + /// Program. Removes the background poller so the host starts clean without DB + /// access and never follows redirects so the auth gate is observable as a 302. + /// + public sealed class AdminAppFactory : WebApplicationFactory + { + protected override IHost CreateHost(IHostBuilder builder) + { + builder.ConfigureServices(services => + { + var poller = services.SingleOrDefault(d => + d.ImplementationType?.Name == "FleetStatusPoller"); + if (poller is not null) + services.Remove(poller); + }); + return base.CreateHost(builder); + } + + public HttpClient CreateNonRedirectingClient() => + CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false }); + } + + public static readonly TheoryData ProtectedRoutes = new() + { + "/", // Home — fleet overview + "/fleet", // Fleet topology + "/hosts", // Host status + "/clusters", // Cluster list + "/alarms/historian", // Historian diagnostics + "/clusters/new", // NewCluster — MUTATING write surface (Admin-002) + "/reservations", // CanPublish-gated page + "/certificates", // FleetAdmin-gated page + "/role-grants", // CanPublish-gated page + "/account", // Authenticated-user page + }; + + [Theory] + [MemberData(nameof(ProtectedRoutes))] + public async Task Anonymous_request_to_a_protected_page_is_rejected(string route) + { + using var client = _factory.CreateNonRedirectingClient(); + + var response = await client.GetAsync(route); + + // The cookie auth handler challenges an unauthenticated request with a 302 to + // the configured LoginPath; a 401/403 is also an acceptable "not allowed in". + if (response.StatusCode == HttpStatusCode.Redirect || + response.StatusCode == HttpStatusCode.Found) + { + response.Headers.Location!.OriginalString.ShouldContain("/login", + Case.Insensitive, $"anonymous GET {route} must bounce to the login page"); + } + else + { + response.StatusCode.ShouldBeOneOf(HttpStatusCode.Unauthorized, HttpStatusCode.Forbidden); + } + + response.StatusCode.ShouldNotBe(HttpStatusCode.OK, + $"anonymous GET {route} must not be served — page-level [Authorize] / fallback policy must gate it"); + } + + [Fact] + public async Task Anonymous_post_to_a_mutating_route_does_not_reach_the_handler() + { + using var client = _factory.CreateNonRedirectingClient(); + + // /clusters/new is the cluster-creation page; an anonymous POST to it must be + // gated before any CreateAsync write path runs. + var response = await client.PostAsync("/clusters/new", + new FormUrlEncodedContent(Array.Empty>())); + + response.StatusCode.ShouldNotBe(HttpStatusCode.OK, + "anonymous POST to /clusters/new must not be served"); + } + + [Fact] + public async Task Login_page_is_anonymously_reachable() + { + using var client = _factory.CreateNonRedirectingClient(); + + var response = await client.GetAsync("/login"); + + response.StatusCode.ShouldBe(HttpStatusCode.OK, + "the login page must stay anonymous or operators can never sign in"); + var body = await response.Content.ReadAsStringAsync(); + body.ShouldContain("sign in", Case.Insensitive); + } + + [Fact] + public async Task Static_assets_remain_anonymously_reachable() + { + using var client = _factory.CreateNonRedirectingClient(); + + // Vendored CSS served by the static-files middleware (not an endpoint) must not + // be caught by the fallback authorization policy. + foreach (var asset in new[] { "/app.css", "/theme.css" }) + { + var response = await client.GetAsync(asset); + response.StatusCode.ShouldBeOneOf(HttpStatusCode.OK, HttpStatusCode.NotModified); + } + } + + [Fact] + public async Task Blazor_framework_script_remains_anonymously_reachable() + { + using var client = _factory.CreateNonRedirectingClient(); + + // The Blazor runtime must load before any auth interaction can happen. + var response = await client.GetAsync("/_framework/blazor.web.js"); + + response.StatusCode.ShouldBeOneOf(HttpStatusCode.OK, HttpStatusCode.NotModified); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/ZB.MOM.WW.OtOpcUa.Admin.Tests.csproj b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/ZB.MOM.WW.OtOpcUa.Admin.Tests.csproj index 4702f61..7950c45 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/ZB.MOM.WW.OtOpcUa.Admin.Tests.csproj +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Admin.Tests/ZB.MOM.WW.OtOpcUa.Admin.Tests.csproj @@ -13,6 +13,7 @@ + all runtime; build; native; contentfiles; analyzers; buildtransitive