fix(dashboard): close AttachEventsAsync re-entrancy window; pin ACL decision-table corners (SEC-25 review)

Follow-up to the per-session event ACL. Part of that change rode into 693a78d via a concurrent agent's pathspec-less commit; this commit carries the review fixes and uses pathspecs on the commit itself so it cannot recur in either direction. Gating the page's subscribe seam made AttachEvents asynchronous — it awaits the authentication state — and that await is a suspension point the synchronous version did not have. On a rapid A -> B navigation the suspended A continuation resumes after B's parameter set has run to completion, re-reads the live SessionId (now B's), and attaches B a SECOND time. The ACL is not bypassed — the newer attach already cleared that same session — but the fields holding B's first subscription are overwritten in place, so nothing ever disposes it: its EventsHubViewerRegistry entry is never released, which keeps the mirror cloning events for a session the page is no longer watching through that handle, and its pump is never cancelled. A resource leak the ACL work introduced. OnParametersSetAsync now claims a monotonic _attachGeneration synchronously, before its first await, and AttachEventsAsync re-checks it after the await and before any field write or Subscribe call. A stale attach returns rather than detaching: it owns nothing, and tearing down there would destroy the newer attach's subscription. DetachEventsAsync needs no such guard — it captures and nulls the live fields synchronously before it awaits, so a resumed detach only unwinds what it already took ownership of. Same dispatcher-owned identity idea as the existing ReferenceEquals guards in PumpEventsAsync and MarkDisconnectedAsync, one level up. The interleaving is not expressible with the static HtmlRenderer idiom the other page tests use: it renders a root component once and exposes no parameter-update seam. The new test therefore adds a minimal Renderer subclass whose only job is to mount a component and drive a second SetParametersAsync into it while the first is parked on a gated AuthenticationStateProvider. That subclass is the lone reason for a narrowly scoped BL0006 suppression, justified in place: it is test-only scaffolding that never ships, and the cost of the warning coming true is a compile break in one test file on an SDK bump. Confirmed non-vacuous by mutation — with the generation check disabled the test goes red on the doubled subscription and the two passing ACL tests stay green. Two decision-table corners are now pinned rather than implied. Admin x nonexistent session id resolves to ALLOW, because the admin bypass is evaluated before the registry lookup; a plausible "look the session up first, it reads better" refactor would flip it, so a test documents the ordering. EventsHub's remarks said "an unknown session id is denied" without qualification, which read as universal; they now state that the bypass is checked first and every rule below it is a non-Admin rule. HubTokenServiceTests gains the truly-absent-field case: a hand-built payload JSON with no Tags key at all, protected through the same purpose, which is the shape every in-flight token has across the deploy that introduces the field. The existing test covered present-but-empty, which does not exercise the null coalesce that stands between a legacy token and a crash on the hub auth path. ProtectorPurpose became internal so the test cannot drift from the real purpose string. Tag-count cardinality cap considered and recorded as a deliberate non-goal. Build 0 warnings / 0 errors; 48 filtered (ACL/hub/token/page) and 257 dashboard tests pass.
2026-08-17 04:35:59 -04:00
parent 693a78db7d
commit 7ec0b3594c
7 changed files with 299 additions and 16 deletions
@@ -349,3 +349,20 @@ is checked first.
  identity does not bypass.
 - Tag *values* are never logged at either seam; only the identifiers and the
  allow/deny outcome are observable.
+- **The page gate needed a re-entrancy guard.** Making `AttachEvents` async (it now
+  awaits the authentication state) introduced a suspension point the synchronous
+  version did not have, and with it a window: on a rapid A → B navigation the
+  suspended A continuation resumes, re-reads the live `SessionId` — now B's — and
+  attaches B a second time, overwriting the fields that hold B's first subscription.
+  That subscription is then unreachable: never disposed, its `EventsHubViewerRegistry`
+  entry never released (so the mirror keeps cloning events for it), its pump never
+  cancelled. Not an ACL bypass — the newer attach had already cleared the same session
+  — but a resource leak the ACL work created. `OnParametersSetAsync` now claims a
+  monotonic `_attachGeneration` synchronously before its first await, and
+  `AttachEventsAsync` re-checks it after the await and before any field write or
+  `Subscribe`; a stale attach returns without attaching (it owns nothing, and tearing
+  down would destroy the newer attach's subscription). `DetachEventsAsync` needs no
+  guard: it captures and nulls the live fields synchronously before it awaits.
+- The admin-bypass-before-lookup ordering means an Administrator naming a session id
+  the registry does not have is **allowed**, not denied. Deliberate, and pinned by a
+  test so a "look the session up first" refactor cannot flip it silently.
@@ -185,6 +185,16 @@ else
    // AttachEventsAsync is the only writer, and it writes on the renderer's dispatcher.
    private bool _eventsAuthorized = true;
    private string? _subscribedSessionId;
+    // Identifies the attach currently entitled to publish subscription state. Bumped
+    // synchronously by OnParametersSetAsync before it awaits anything, so a suspended
+    // AttachEventsAsync continuation can tell that a newer parameter set overtook it — the
+    // same dispatcher-owned identity idea as the ReferenceEquals guards in PumpEventsAsync
+    // and MarkDisconnectedAsync, one level up. Without it, the await on the authentication
+    // state opens a window in which a rapid A -> B navigation lets the stale continuation
+    // re-read the live SessionId and attach B a second time, orphaning B's first
+    // subscription (never disposed, its viewer registration never released, its pump never
+    // cancelled) behind the fields it overwrites.
+    private int _attachGeneration;
    private readonly LinkedList<MxEvent> _recentEvents = new();

    private bool CanManage { get; set; }
@@ -208,11 +218,17 @@ else
    {
        if (!string.Equals(_subscribedSessionId, SessionId, StringComparison.Ordinal))
        {
+            // Claimed before the first await, so every attach that follows carries a token
+            // that a later parameter set can invalidate. DetachEventsAsync needs no such
+            // guard: it captures and nulls the live fields synchronously before it awaits,
+            // so a resumed detach only unwinds what it already took ownership of.
+            int generation = ++_attachGeneration;
+
            // Deliberately no ConfigureAwait(false): the resumption must stay on the
            // renderer's dispatcher so the new subscription is published to
            // _eventSubscription from the same thread the pump's guard reads it on.
            await DetachEventsAsync();
-            await AttachEventsAsync();
+            await AttachEventsAsync(generation);
        }
    }

@@ -302,7 +318,7 @@ else
    // whether a subscription is created at all — the generation guards, the pump, and the detach
    // coupling below it are untouched, so a denied page holds no subscription to leak and never
    // registers a viewer, which keeps the broadcaster's mirror off for that session.
-    private async Task AttachEventsAsync()
+    private async Task AttachEventsAsync(int generation)
    {
        if (string.IsNullOrWhiteSpace(SessionId))
        {
@@ -313,6 +329,19 @@ else
        // land back on the renderer's dispatcher, which is where the fields below are owned.
        AuthenticationState authenticationState = await AuthenticationStateProvider.GetAuthenticationStateAsync();

+        // Checked before ANY field write and before Subscribe, because both are the damage: a
+        // newer parameter set may have run start-to-finish while this continuation was parked,
+        // and SessionId now reads as ITS session. Attaching here would not bypass the ACL (the
+        // newer attach already cleared the same session), but it would strand the live
+        // subscription — overwritten in place, so nothing ever disposes it or releases its
+        // viewer registration, and the mirror stays on for a session nobody is watching. A
+        // stale attach owns nothing, so it returns rather than detaching: tearing down here
+        // would destroy the newer attach's subscription.
+        if (generation != _attachGeneration)
+        {
+            return;
+        }
+
        _subscribedSessionId = SessionId;
        _eventsAuthorized = SessionAcl.CanViewSession(authenticationState.User, SessionId);

@@ -29,7 +29,11 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard;
 /// </remarks>
 public sealed class HubTokenService
 {
-    private const string ProtectorPurpose = "ZB.MOM.WW.MxGateway.Dashboard.HubToken.v1";
+    // Internal rather than private so a test can protect a hand-built payload through the same
+    // purpose and assert how Validate reads a payload shape this class no longer mints (a token
+    // predating the Tags field). Copying the literal into the test instead would let the two
+    // drift and silently turn that test into an assertion about an unrelated protector.
+    internal const string ProtectorPurpose = "ZB.MOM.WW.MxGateway.Dashboard.HubToken.v1";

    // Hub bearer tokens are single-purpose, data-protection-encrypted, and NOT server-side
    // revocable. A short lifetime bounds the exposure window of a token captured from a proxy
@@ -40,9 +40,14 @@ public sealed class EventsHub(
    /// only checks that the caller carries one of the dashboard roles, which by
    /// itself would let any Viewer subscribe to any session id they name. The
    /// per-session decision is <see cref="IDashboardSessionAcl"/>'s
-    /// (SEC-25 / TST-15): Administrators see every session, a Viewer sees a
-    /// session only when its tags intersect their granted tags, and an unknown
-    /// session id is denied. A denied caller is not joined to the group and is
+    /// (SEC-25 / TST-15). The admin bypass is evaluated first, so an
+    /// Administrator joins any session id they name; every check below it
+    /// applies to non-Admin callers only. For those: a Viewer sees a session
+    /// only when its tags intersect their granted tags, an untagged session
+    /// follows <c>Dashboard:UntaggedSessionVisibility</c>, and a session id the
+    /// registry does not have is denied outright — the phantom-id denial is
+    /// therefore a non-Admin rule, not a universal one.
+    /// A denied caller is not joined to the group and is
    /// not registered with <see cref="EventsHubViewerRegistry"/>, so the mirror
    /// stays off for a session nobody is legitimately watching. The same ACL
    /// gates the in-process seam used by the session-details page, so neither
@@ -47,8 +47,24 @@ public sealed class DashboardSessionAclTests
    }

    /// <summary>
-    /// An unknown session id is denied even for a caller holding every configured tag: no
-    /// subscription is created for a session the registry does not have.
+    /// The decision table's order is load-bearing at exactly one corner: an Administrator naming
+    /// a session id the registry does not have is ALLOWED, because the admin bypass is checked
+    /// before the lookup. Pinned deliberately — reordering the two checks (a plausible "look the
+    /// session up first, it reads better" refactor) would flip this to a denial and quietly change
+    /// what an Administrator's hub join does for a session that closed a moment ago.
+    /// </summary>
+    [Fact]
+    public void CanViewSession_AdministratorAndUnknownSession_Allowed()
+    {
+        DashboardSessionAcl acl = CreateAcl();
+
+        Assert.True(acl.CanViewSession(Principal(roles: [DashboardRoles.Admin]), "session-does-not-exist"));
+    }
+
+    /// <summary>
+    /// An unknown session id is denied for a non-Admin even when they hold every configured tag:
+    /// no subscription is created for a session the registry does not have. The Administrator
+    /// counterpart above is the deliberate exception.
    /// </summary>
    [Fact]
    public void CanViewSession_UnknownSession_Denied()
@@ -245,7 +245,39 @@ public sealed class HubTokenServiceTests
        Assert.Empty(result.FindAll(DashboardAuthenticationDefaults.DashboardTagClaimType));
    }

-    private static HubTokenService CreateService(Dictionary<string, string[]>? groupToTag = null)
+    /// <summary>
+    /// A token minted before the payload carried tags at all still validates, and yields an empty
+    /// grant rather than throwing or rejecting. Distinct from the empty-grant test above, which
+    /// exercises a <c>Tags</c> key that is present and empty: this one protects a hand-built
+    /// payload with the key genuinely ABSENT, which is the shape every in-flight token has across
+    /// the deploy that introduces the field. Deserialization leaves the field null, and the
+    /// null-coalesce in <c>Validate</c> is the only thing standing between that and a crash on
+    /// the hub's authentication path.
+    /// </summary>
+    [Fact]
+    public void Validate_TokenMintedBeforeTagsFieldExisted_YieldsEmptyGrant()
+    {
+        EphemeralDataProtectionProvider dataProtection = new();
+        HubTokenService service = CreateService(dataProtection: dataProtection);
+
+        // The pre-field payload shape, verbatim: no "Tags" key anywhere.
+        const string LegacyPayload = """{"Name":"frank","NameIdentifier":"frank-id","Roles":["Viewer"]}""";
+        string legacyToken = dataProtection
+            .CreateProtector(HubTokenService.ProtectorPurpose)
+            .ToTimeLimitedDataProtector()
+            .Protect(LegacyPayload, HubTokenService.TokenLifetime);
+
+        ClaimsPrincipal? result = service.Validate(legacyToken);
+
+        Assert.NotNull(result);
+        Assert.Equal("frank", result.Identity?.Name);
+        Assert.True(result.IsInRole(DashboardRoles.Viewer));
+        Assert.Empty(result.FindAll(DashboardAuthenticationDefaults.DashboardTagClaimType));
+    }
+
+    private static HubTokenService CreateService(
+        Dictionary<string, string[]>? groupToTag = null,
+        IDataProtectionProvider? dataProtection = null)
    {
        GatewayOptions options = new()
        {
@@ -255,6 +287,8 @@ public sealed class HubTokenServiceTests
            },
        };

-        return new HubTokenService(new EphemeralDataProtectionProvider(), Options.Create(options));
+        return new HubTokenService(
+            dataProtection ?? new EphemeralDataProtectionProvider(),
+            Options.Create(options));
    }
 }
@@ -3,6 +3,7 @@ using System.Security.Claims;
 using System.Threading.Channels;
 using Microsoft.AspNetCore.Components;
 using Microsoft.AspNetCore.Components.Authorization;
+using Microsoft.AspNetCore.Components.RenderTree;
 using Microsoft.AspNetCore.Components.Web;
 using Microsoft.AspNetCore.Components.Web.HtmlRendering;
 using Microsoft.Extensions.DependencyInjection;
@@ -73,7 +74,72 @@ public sealed class SessionDetailsPageEventAclTests
        Assert.DoesNotContain(DeniedMessage, html, StringComparison.Ordinal);
    }

-    private static async Task<string> RenderAsync(RecordingEventSubscriber subscriber, bool allow)
+    /// <summary>
+    /// The re-entrancy guard on <c>AttachEventsAsync</c>: a rapid A -> B navigation must leave
+    /// exactly one live subscription, not two.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// Gating the ACL made attach asynchronous — it awaits the authentication state — and that
+    /// await is a suspension point the synchronous version did not have. The interleaving this
+    /// test forces is the one that window admits: A's attach parks on the auth state, B's whole
+    /// parameter set runs to completion behind it, and only then does A resume. A now reads
+    /// <c>SessionId</c> as B's and, ungurarded, subscribes to B a SECOND time — overwriting the
+    /// fields holding B's first subscription, which is then unreachable: never disposed, its
+    /// <c>EventsHubViewerRegistry</c> entry never released (so the mirror keeps cloning events
+    /// for it), its pump never cancelled.
+    /// </para>
+    /// <para>
+    /// The assertion is deliberately about subscription COUNT and disposal rather than about the
+    /// ACL: the guard is a resource-lifecycle fix, and the second attach was never an
+    /// authorization bypass — B had already been cleared by the newer attach.
+    /// </para>
+    /// <para>
+    /// This case needs a renderer that can re-set parameters on the SAME component instance, which
+    /// the static <see cref="HtmlRenderer"/> used by the tests above cannot do — it renders a root
+    /// component once and exposes no parameter-update seam. Hence the minimal
+    /// <see cref="ParameterDrivingRenderer"/> below, which is the smallest thing that can express
+    /// a second <c>SetParametersAsync</c> while the first is still suspended.
+    /// </para>
+    /// </remarks>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task Page_WhenNavigationOvertakesASuspendedAttach_LeavesOneSubscription()
+    {
+        RecordingEventSubscriber subscriber = new();
+        // Call 1 is OnInitializedAsync's CanManage lookup; call 2 is the first session's attach,
+        // which is the one that must be caught mid-flight.
+        GatedAuthenticationStateProvider auth = new(gateOnCall: 2);
+
+        ServiceCollection services = BuildServices(subscriber, allow: true, authenticationStateProvider: auth);
+        await using ServiceProvider provider = services.BuildServiceProvider();
+        await using ParameterDrivingRenderer renderer = new(provider, provider.GetRequiredService<ILoggerFactory>());
+
+        SessionDetailsPage page = await renderer.MountAsync<SessionDetailsPage>();
+
+        // Not awaited: it parks inside the first attach, which is the whole point.
+        Task first = renderer.SetParametersAsync(page, "session-a");
+        await auth.Entered.WaitAsync(TestTimeout);
+
+        // The overtaking navigation completes end to end while the first attach is suspended.
+        await renderer.SetParametersAsync(page, "session-b").WaitAsync(TestTimeout);
+
+        auth.Release();
+        await first.WaitAsync(TestTimeout);
+
+        Assert.Empty(renderer.Exceptions);
+        // Without the generation guard this is ["session-b", "session-b"] and the first of the two
+        // is stranded — the exact leak the guard exists to prevent.
+        Assert.Equal(["session-b"], subscriber.SubscribedSessionIds);
+        Assert.Empty(subscriber.UndisposedAfterReplacement);
+    }
+
+    private static readonly TimeSpan TestTimeout = TimeSpan.FromSeconds(30);
+
+    private static ServiceCollection BuildServices(
+        RecordingEventSubscriber subscriber,
+        bool allow,
+        AuthenticationStateProvider? authenticationStateProvider = null)
    {
        ServiceCollection services = new();
        services.AddLogging();
@@ -82,9 +148,15 @@ public sealed class SessionDetailsPageEventAclTests
        services.AddSingleton<IDashboardSessionAdminService>(new NonManagingSessionAdminService());
        services.AddSingleton<IDashboardSessionEventSubscriber>(subscriber);
        services.AddSingleton<IDashboardSessionAcl>(new StubSessionAcl(allow));
-        services.AddSingleton<AuthenticationStateProvider>(new StubAuthenticationStateProvider());
+        services.AddSingleton<AuthenticationStateProvider>(
+            authenticationStateProvider ?? new StubAuthenticationStateProvider());

-        await using ServiceProvider provider = services.BuildServiceProvider();
+        return services;
+    }
+
+    private static async Task<string> RenderAsync(RecordingEventSubscriber subscriber, bool allow)
+    {
+        await using ServiceProvider provider = BuildServices(subscriber, allow).BuildServiceProvider();
        await using HtmlRenderer renderer = new(provider, provider.GetRequiredService<ILoggerFactory>());

        return await renderer.Dispatcher.InvokeAsync(async () =>
@@ -107,31 +179,137 @@ public sealed class SessionDetailsPageEventAclTests

    private sealed class RecordingEventSubscriber : IDashboardSessionEventSubscriber
    {
+        private readonly List<IdleSubscription> _handedOut = [];
+
        /// <summary>Gets the session ids <see cref="Subscribe"/> was called with, in order.</summary>
        public List<string> SubscribedSessionIds { get; } = [];

+        /// <summary>
+        /// Gets the subscriptions that were superseded by a later one and never disposed — the
+        /// signature of a stranded subscription, whose viewer registration is never released. The
+        /// most recent subscription is excluded because the page legitimately still holds it.
+        /// </summary>
+        public IReadOnlyList<IdleSubscription> UndisposedAfterReplacement =>
+            [.. _handedOut.SkipLast(1).Where(subscription => !subscription.IsDisposed)];
+
        /// <inheritdoc />
        public IDashboardEventSubscription Subscribe(string sessionId)
        {
            SubscribedSessionIds.Add(sessionId);
+            IdleSubscription subscription = new();
+            _handedOut.Add(subscription);

-            return new IdleSubscription();
+            return subscription;
        }

        // A subscription whose channel never yields and never completes, so the page's pump parks
        // exactly as it would against a quiet session.
-        private sealed class IdleSubscription : IDashboardEventSubscription
+        internal sealed class IdleSubscription : IDashboardEventSubscription
        {
            private readonly Channel<MxEvent> _channel = Channel.CreateUnbounded<MxEvent>();

+            /// <summary>Gets a value indicating whether the page released this subscription.</summary>
+            public bool IsDisposed { get; private set; }
+
            /// <inheritdoc />
            public ChannelReader<MxEvent> Reader => _channel.Reader;

            /// <inheritdoc />
-            public void Dispose() => _channel.Writer.TryComplete();
+            public void Dispose()
+            {
+                IsDisposed = true;
+                _channel.Writer.TryComplete();
+            }
        }
    }

+    // Gates one nominated call so a test can suspend an attach exactly where the ACL check made it
+    // asynchronous, and let a second parameter set overtake it.
+    private sealed class GatedAuthenticationStateProvider(int gateOnCall) : AuthenticationStateProvider
+    {
+        private readonly TaskCompletionSource _entered = new(TaskCreationOptions.RunContinuationsAsynchronously);
+        private readonly TaskCompletionSource _release = new(TaskCreationOptions.RunContinuationsAsynchronously);
+        private int _calls;
+
+        /// <summary>Completes once the gated call has been entered and is parked.</summary>
+        public Task Entered => _entered.Task;
+
+        /// <summary>Lets the parked call finish.</summary>
+        public void Release() => _release.TrySetResult();
+
+        /// <inheritdoc />
+        public override async Task<AuthenticationState> GetAuthenticationStateAsync()
+        {
+            if (Interlocked.Increment(ref _calls) == gateOnCall)
+            {
+                _entered.TrySetResult();
+                await _release.Task.ConfigureAwait(false);
+            }
+
+            return new AuthenticationState(new ClaimsPrincipal(new ClaimsIdentity(
+                [new Claim(ClaimTypes.Name, "viewer-user"), new Claim(ClaimTypes.Role, DashboardRoles.Viewer)],
+                authenticationType: "test",
+                nameType: ClaimTypes.Name,
+                roleType: ClaimTypes.Role)));
+        }
+    }
+
+    // The smallest renderer that can drive a SECOND parameter set into an already-mounted
+    // component instance. HtmlRenderer renders a root component once and offers no such seam, so
+    // the interleaving under test is inexpressible with it; everything here is plumbing around
+    // Renderer's protected mount/parameter surface, with no behaviour of its own.
+    //
+    // BL0006 warns that Microsoft.AspNetCore.Components.RenderTree is not for use outside the
+    // Blazor framework because those types may change between releases. Suppressed here and only
+    // here: this is test-only scaffolding (the same thing component-testing packages do), it never
+    // ships, and the cost of the warning coming true is a compile break in one test file on an SDK
+    // bump — not a production defect. Production code must keep honouring BL0006.
+#pragma warning disable BL0006
+    private sealed class ParameterDrivingRenderer(IServiceProvider services, ILoggerFactory loggerFactory)
+        : Renderer(services, loggerFactory)
+    {
+        /// <summary>Gets exceptions the renderer surfaced, so a test never passes over a swallowed fault.</summary>
+        public List<Exception> Exceptions { get; } = [];
+
+        /// <inheritdoc />
+        public override Dispatcher Dispatcher { get; } = Dispatcher.CreateDefault();
+
+        /// <summary>Instantiates the component with DI-injected properties and attaches it as a root.</summary>
+        /// <typeparam name="TComponent">Component type to mount.</typeparam>
+        /// <returns>The mounted component instance.</returns>
+        public Task<TComponent> MountAsync<TComponent>()
+            where TComponent : IComponent
+        {
+            return Dispatcher.InvokeAsync(() =>
+            {
+                TComponent component = (TComponent)InstantiateComponent(typeof(TComponent));
+                AssignRootComponentId(component);
+
+                return component;
+            });
+        }
+
+        /// <summary>Sets the session-id parameter on an already-mounted page.</summary>
+        /// <param name="component">The mounted page.</param>
+        /// <param name="sessionId">Session id to render.</param>
+        /// <returns>The task the component's parameter-set lifecycle returns.</returns>
+        public Task SetParametersAsync(IComponent component, string sessionId)
+        {
+            return Dispatcher.InvokeAsync(() => component.SetParametersAsync(
+                ParameterView.FromDictionary(new Dictionary<string, object?>
+                {
+                    [nameof(SessionDetailsPage.SessionId)] = sessionId,
+                })));
+        }
+
+        /// <inheritdoc />
+        protected override void HandleException(Exception exception) => Exceptions.Add(exception);
+
+        /// <inheritdoc />
+        protected override Task UpdateDisplayAsync(in RenderBatch renderBatch) => Task.CompletedTask;
+    }
+#pragma warning restore BL0006
+
    private sealed class StubSnapshotService : IDashboardSnapshotService
    {
        /// <inheritdoc />