Merged PR 33543: [8.0] Prevent delivery of events to disposed components

# Prevent delivery of events to disposed components

Prevents delivery of events to disposed components.

## Description

In some cases, it was possible to bypass validation rules on an EditForm by triggering a disposal and a form-submit synchronously together.

Fixes MSRC 81646

## Customer Impact

In some cases, may allow bypass of server-side validation rules.

## Regression?

- [x] Yes
- [ ] No

5.0

## Risk

- [ ] High
- [x] Medium
- [ ] Low

This change should not break any reasonable existing apps, since we don't consider it expected that events would ever be delivered to disposed components. It would never have been possible in WebAssembly - the possibility only existed in Server due to network latency, and even then only if the client is behaving very unusually. However it is theoretically possible that some app makes use of this bug intentionally, and if so they would encounter a change of behavior when upgrading to .NET 8 RC2 or GA.

Normally we wouldn't consider a bugfix like this to be "medium" risk - I'm only selecting that level because of it being introduced so close to GA.

Note that separate fixes are being delivered in patches for 6.0 and 7.0.

## Verification

- [x] Manual (required)
- [x] Automated

## Packaging changes reviewed?

- [ ] Yes
- [ ] No
- [x] N/A

Author: SteveSandersonMS

src/Components/Components/src/RenderTree/RenderTreeDiffBuilder.cs

@@ -978,7 +978,7 @@ private static void InitializeNewAttributeFrame(ref DiffContext diffContext, ref
             newFrame.AttributeNameField.Length >= 3 &&
             newFrame.AttributeNameField.StartsWith("on", StringComparison.Ordinal))
         {
-            diffContext.Renderer.AssignEventHandlerId(ref newFrame);
+            diffContext.Renderer.AssignEventHandlerId(diffContext.ComponentId, ref newFrame);
         }
     }
 

src/Components/Components/src/RenderTree/Renderer.Log.cs

@@ -63,5 +63,16 @@ public static void HandlingEvent(ILogger logger, ulong eventHandlerId, EventArgs
                 HandlingEvent(logger, eventHandlerId, eventArgs?.GetType().Name ?? "null");
             }
         }
+
+        [LoggerMessage(6, LogLevel.Debug, "Skipping attempt to raise event {EventId} of type '{EventType}' because the component ID {ComponentId} was already disposed", EventName = "SkippingEventOnDisposedComponent", SkipEnabledCheck = true)]
+        public static partial void SkippingEventOnDisposedComponent(ILogger logger, int componentId, ulong eventId, string eventType);
+
+        public static void SkippingEventOnDisposedComponent(ILogger logger, int componentId, ulong eventHandlerId, EventArgs? eventArgs)
+        {
+            if (logger.IsEnabled(LogLevel.Debug)) // This is almost always false, so skip the evaluations
+            {
+                SkippingEventOnDisposedComponent(logger, componentId, eventHandlerId, eventArgs?.GetType().Name ?? "null");
+            }
+        }
     }
 }

src/Components/Components/src/RenderTree/Renderer.cs

@@ -28,7 +28,7 @@ public abstract partial class Renderer : IDisposable, IAsyncDisposable
     private readonly Dictionary<int, ComponentState> _componentStateById = new Dictionary<int, ComponentState>();
     private readonly Dictionary<IComponent, ComponentState> _componentStateByComponent = new Dictionary<IComponent, ComponentState>();
     private readonly RenderBatchBuilder _batchBuilder = new RenderBatchBuilder();
-    private readonly Dictionary<ulong, EventCallback> _eventBindings = new Dictionary<ulong, EventCallback>();
+    private readonly Dictionary<ulong, (int RenderedByComponentId, EventCallback Callback)> _eventBindings = new();
     private readonly Dictionary<ulong, ulong> _eventHandlerIdReplacements = new Dictionary<ulong, ulong>();
     private readonly ILogger _logger;
     private readonly ComponentFactory _componentFactory;
@@ -416,7 +416,22 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie
             _pendingTasks ??= new();
         }
 
-        var callback = GetRequiredEventCallback(eventHandlerId);
+        var (renderedByComponentId, callback) = GetRequiredEventBindingEntry(eventHandlerId);
+
+        // If this event attribute was rendered by a component that's since been disposed, don't dispatch the event at all.
+        // This can occur because event handler disposal is deferred, so event handler IDs can outlive their components.
+        // The reason the following check is based on "which component rendered this frame" and not on "which component
+        // receives the callback" (i.e., callback.Receiver) is that if parent A passes a RenderFragment with events to child B,
+        // and then child B is disposed, we don't want to dispatch the events (because the developer considers them removed
+        // from the UI) even though the receiver A is still alive.
+        if (!_componentStateById.ContainsKey(renderedByComponentId))
+        {
+            // This is not an error since it can happen legitimately (in Blazor Server, the user might click a button at the same
+            // moment that the component is disposed remotely, and then the click event will arrive after disposal).
+            Log.SkippingEventOnDisposedComponent(_logger, renderedByComponentId, eventHandlerId, eventArgs);
+            return Task.CompletedTask;
+        }
+
         Log.HandlingEvent(_logger, eventHandlerId, eventArgs);
 
         // Try to match it up with a receiver so that, if the event handler later throws, we can route the error to the
@@ -480,7 +495,7 @@ public virtual Task DispatchEventAsync(ulong eventHandlerId, EventFieldInfo? fie
     /// <returns>The parameter type expected by the event handler. Normally this is a subclass of <see cref="EventArgs"/>.</returns>
     public Type GetEventArgsType(ulong eventHandlerId)
     {
-        var methodInfo = GetRequiredEventCallback(eventHandlerId).Delegate?.Method;
+        var methodInfo = GetRequiredEventBindingEntry(eventHandlerId).Callback.Delegate?.Method;
 
         // The DispatchEventAsync code paths allow for the case where Delegate or its method
         // is null, and in this case the event receiver just receives null. This won't happen
@@ -581,7 +596,7 @@ protected virtual void AddPendingTask(ComponentState? componentState, Task task)
         _pendingTasks?.Add(task);
     }
 
-    internal void AssignEventHandlerId(ref RenderTreeFrame frame)
+    internal void AssignEventHandlerId(int renderedByComponentId, ref RenderTreeFrame frame)
     {
         var id = ++_lastEventHandlerId;
 
@@ -593,15 +608,15 @@ internal void AssignEventHandlerId(ref RenderTreeFrame frame)
             //
             // When that happens we intentionally box the EventCallback because we need to hold on to
             // the receiver.
-            _eventBindings.Add(id, callback);
+            _eventBindings.Add(id, (renderedByComponentId, callback));
         }
         else if (frame.AttributeValueField is MulticastDelegate @delegate)
         {
             // This is the common case for a delegate, where the receiver of the event
             // is the same as delegate.Target. In this case since the receiver is implicit we can
             // avoid boxing the EventCallback object and just re-hydrate it on the other side of the
             // render tree.
-            _eventBindings.Add(id, new EventCallback(@delegate.Target as IHandleEvent, @delegate));
+            _eventBindings.Add(id, (renderedByComponentId, new EventCallback(@delegate.Target as IHandleEvent, @delegate)));
         }
 
         // NOTE: we do not to handle EventCallback<T> here. EventCallback<T> is only used when passing
@@ -645,14 +660,14 @@ internal void TrackReplacedEventHandlerId(ulong oldEventHandlerId, ulong newEven
         _eventHandlerIdReplacements.Add(oldEventHandlerId, newEventHandlerId);
     }
 
-    private EventCallback GetRequiredEventCallback(ulong eventHandlerId)
+    private (int RenderedByComponentId, EventCallback Callback) GetRequiredEventBindingEntry(ulong eventHandlerId)
     {
-        if (!_eventBindings.TryGetValue(eventHandlerId, out var callback))
+        if (!_eventBindings.TryGetValue(eventHandlerId, out var entry))
         {
             throw new ArgumentException($"There is no event handler associated with this event. EventId: '{eventHandlerId}'.", nameof(eventHandlerId));
         }
 
-        return callback;
+        return entry;
     }
 
     private ulong FindLatestEventHandlerIdInChain(ulong eventHandlerId)

src/Components/Components/test/RendererTest.cs

@@ -2619,6 +2619,56 @@ public void RenderBatch_DoesNotDisposeComponentMultipleTimes()
         Assert.True(component.Disposed);
     }
 
+    [Fact]
+    public async Task DoesNotDispatchEventsAfterOwnerComponentIsDisposed()
+    {
+        // Arrange
+        var renderer = new TestRenderer();
+        var eventCount = 0;
+        Action<EventArgs> origEventHandler = args => { eventCount++; };
+        var component = new ConditionalParentComponent<EventComponent>
+        {
+            IncludeChild = true,
+            ChildParameters = new Dictionary<string, object>
+            {
+                { nameof(EventComponent.OnTest), origEventHandler }
+            }
+        };
+        var rootComponentId = renderer.AssignRootComponentId(component);
+        component.TriggerRender();
+        var batch = renderer.Batches.Single();
+        var rootComponentDiff = batch.DiffsByComponentId[rootComponentId].Single();
+        var rootComponentFrame = batch.ReferenceFrames[0];
+        var childComponentFrame = rootComponentDiff.Edits
+            .Select(e => batch.ReferenceFrames[e.ReferenceFrameIndex])
+            .Where(f => f.FrameType == RenderTreeFrameType.Component)
+            .Single();
+        var childComponentId = childComponentFrame.ComponentId;
+        var childComponentDiff = batch.DiffsByComponentId[childComponentFrame.ComponentId].Single();
+        var eventHandlerId = batch.ReferenceFrames
+            .Skip(childComponentDiff.Edits[0].ReferenceFrameIndex) // Search from where the child component frames start
+            .Where(f => f.FrameType == RenderTreeFrameType.Attribute)
+            .Single(f => f.AttributeEventHandlerId != 0)
+            .AttributeEventHandlerId;
+
+        // Act/Assert 1: Event handler fires when we trigger it
+        Assert.Equal(0, eventCount);
+        var renderTask = renderer.DispatchEventAsync(eventHandlerId, args: null);
+        Assert.True(renderTask.IsCompletedSuccessfully);
+        Assert.Equal(1, eventCount);
+        await renderTask;
+
+        // Now remove the EventComponent, but without ever acknowledging the renderbatch, so the event handler doesn't get disposed
+        var disposalBatchAcknowledgementTcs = new TaskCompletionSource();
+        component.IncludeChild = false;
+        renderer.NextRenderResultTask = disposalBatchAcknowledgementTcs.Task;
+        component.TriggerRender();
+
+        // Act/Assert 2: Can no longer fire the original event. It's not an error but the delegate was not invoked.
+        await renderer.DispatchEventAsync(eventHandlerId, args: null);
+        Assert.Equal(1, eventCount);
+    }
+
     [Fact]
     public async Task DisposesEventHandlersWhenAttributeValueChanged()
     {

src/Components/test/E2ETest/Tests/FormsTest.cs

@@ -990,6 +990,39 @@ public void CanHaveModelLevelValidationErrors()
         Browser.Collection(logEntries, x => Assert.Equal("OnValidSubmit", x));
     }
 
+    [Fact]
+    public async Task CannotSubmitEditFormSynchronouslyAfterItWasRemoved()
+    {
+        var appElement = MountSimpleValidationComponent();
+
+        var submitButtonFinder = By.CssSelector("button[type=submit]");
+        Browser.Exists(submitButtonFinder);
+
+        // Remove the form then immediately also submit it, so the server receives both
+        // the 'remove' and 'submit' commands (in that order) before it updates the UI
+        appElement.FindElement(By.Id("remove-form")).Click();
+
+        try
+        {
+            appElement.FindElement(submitButtonFinder).Click();
+        }
+        catch (NoSuchElementException)
+        {
+            // This should happen on WebAssembly because the form will be removed synchronously
+            // That means the test has passed
+            return;
+        }
+
+        // Wait for the removal to complete, which is intentionally delayed to ensure
+        // this test can submit a second instruction before the first is processed.
+        Browser.DoesNotExist(submitButtonFinder);
+
+        // Verify that the form submit event was not processed, even if we wait a while
+        // to be really sure the second instruction was processed.
+        await Task.Delay(1000);
+        Browser.DoesNotExist(By.Id("last-callback"));
+    }
+
     private Func<string[]> CreateValidationMessagesAccessor(IWebElement appElement, string messageSelector = ".validation-message")
     {
         return () => appElement.FindElements(By.CssSelector(messageSelector))

src/Components/test/testassets/BasicTestApp/FormsTest/SimpleValidationComponent.razor

@@ -1,37 +1,42 @@
 @using System.ComponentModel.DataAnnotations
 @using Microsoft.AspNetCore.Components.Forms
 
-<EditForm Model="@this" OnValidSubmit="@HandleValidSubmit" OnInvalidSubmit="@HandleInvalidSubmit" autocomplete="off">
-    <DataAnnotationsValidator />
-
-    <p class="user-name">
-        User name: <input @bind="UserName" class="@context.FieldCssClass(() => UserName)" />
-    </p>
-    <p class="accepts-terms">
-        Accept terms: <input type="checkbox" @bind="AcceptsTerms" class="@context.FieldCssClass(() => AcceptsTerms)" />
-    </p>
-
-    <button type="submit">Submit</button>
-
-    @* Could use <ValidationSummary /> instead, but this shows it can be done manually *@
-    <ul class="validation-errors">
-        @foreach (var message in context.GetValidationMessages())
-        {
-            <li class="validation-message">@message</li>
-        }
-    </ul>
-
-</EditForm>
+@if (!removeForm)
+{
+    <EditForm Model="@this" OnValidSubmit="@HandleValidSubmit" OnInvalidSubmit="@HandleInvalidSubmit" autocomplete="off">
+        <DataAnnotationsValidator />
+
+        <p class="user-name">
+            User name: <input @bind="UserName" class="@context.FieldCssClass(() => UserName)" />
+        </p>
+        <p class="accepts-terms">
+            Accept terms: <input type="checkbox" @bind="AcceptsTerms" class="@context.FieldCssClass(() => AcceptsTerms)" />
+        </p>
+
+        <button type="submit">Submit</button>
+
+        @* Could use <ValidationSummary /> instead, but this shows it can be done manually *@
+        <ul class="validation-errors">
+            @foreach (var message in context.GetValidationMessages())
+            {
+                <li class="validation-message">@message</li>
+            }
+        </ul>
+    </EditForm>
+}
 
 @if (lastCallback != null)
 {
     <span id="last-callback">@lastCallback</span>
 }
 
+<p><button id="remove-form" @onclick="RemoveForm">Remove form</button></p>
+
 @code {
     protected virtual bool UseExperimentalValidator => false;
 
     string lastCallback;
+    bool removeForm;
 
     [Required(ErrorMessage = "Please choose a username")]
     public string UserName { get; set; }
@@ -49,4 +54,10 @@
     {
         lastCallback = "OnInvalidSubmit";
     }
+
+    void RemoveForm()
+    {
+        removeForm = true;
+        Thread.Sleep(1000); // To ensure we can dispatch another event before this completes
+    }
 }