Ensure server-side Blazor failure modes are correct (e.g., clients can't cause a global unhandled exception)

[SteveSandersonMS]

This is part of #10472

Clients shouldn't be able to make the process terminate, whether accidentally or deliberately.

Currently there may be cases where they can (e.g., with JSInterop, sending a call with an unknown object ID).

We need to check:

• All places where we receive input from the client. Ensure there's no way that accidentally or deliberately bad input can cause a global exception. It is fine if they can kill their own circuit (though should be logged), but it shouldn't be able to affect other circuits.
  • JS interop inbound calls, starting from ComponentHub.BeginInvokeDotNetFromJS
  • Replies to outbound JS interop calls (sync and async)
  • Event notifications (starting from renderer.DispatchEventAsync, treating input as untrusted)
    • OK, because any unhandled exception in DispatchEventAsync becomes a regular failed sync/async JS interop call, so this is scoped to the circuit
  • Render batch ACKs, starting from ComponentHub.OnRenderCompleted
    • OK. If it's an unknown batch ID, we send an error to the circuit's exception handler. For arbitrary exceptions besides this (which may be impossible), it's a regular hub method exception, which SignalR handles by logging it and sending a redacted copy of the exception info to the client, and terminates that one connection/circuit.
    • Action needed Clients can send an arbitrary errorMessageOrNull value, which will typically end up in server logs. So they might be able to mislead the developer into thinking that other errors are happening. Developers need to understand somehow that these error messages are untrusted input and should be treated with suspicion. Clarify that ComponentHub.OnRenderCompleted's errorMessage is untrusted #11843
  • All other client-invokable hub methods on ComponentHub:
    • OnDisconnectedAsync
      • OK. If a client chooses to call this unexpectedly, we will just disconnect their circuit. There's already a check to ensure we don't disconnect more than once here.
    • StartCircuit
      • Action needed. Currently we don't check whether the hub instance is already associated with a circuit. A client could ask us to start an unlimited number of circuits here within a single connection. Also we don't validate that the supplied uriAbsolute/baseUriAbsolute are non-null valid URLs - I'm not sure how this could be misused, but we should validate since we have no reason not to. In ComponentHub.StartCircuit, prevent creating multiple circuits #11841 Validate uriAbsolute/baseUriAbsolute #11842
    • ConnectCircuit
      • Action needed. This might be OK already, but it doesn't check whether there's already a circuit associated with this connection. If there is, maybe we need to no-op gracefully instead of calling _circuitRegistry.ConnectAsync. Some code inside _circuitRegistry.ConnectAsync talks about handling this case, but I don't know why it's not just a no-op. In ComponentHub.StartCircuit, prevent creating multiple circuits #11841
  • Anything with [JSInvokable] in any of our server-side shipping assemblies
    • RendererRegistryEventDispatcher.DispatchEvent
      • OK. You can easily cause an exception in parsing, but that's just a normal failed JS interop call. All the state it accesses is per-circuit.
    • RemoteUriHelper.NotifyLocationChanged
      • Action needed We don't check the supplied absoluteUri string is a valid URL or even non-null. I don't know how you could do anything bad, but we should consider validating that it's a non-null parseable URL. Validate uriAbsolute/baseUriAbsolute #11842
  • Any static state in any of our shipping assemblies. This is only tangentally related, but it's worth checking that we don't have any state that clients can read/write in a way that would either disclose info about other circuits or would get the application into state where it would crash or behave badly
    • See further comments about this below. I've checked throughout .Components, .Server, and .Browser (== .Web). There are some action items there, but I'll check this one off here because no further investigation is needed to find more action items. Blazor: minor tweaks to ensure shared state doesn't cross circuits #11849
• All boundaries between framework and application code within a circuit, to ensure that any exceptions in application code do not kill the entire process. Again, it's fine for an unhandled application exception to kill a circuit, but not the whole process.
  • Component construction and initialization / dependency injection
    • Creation of IComponent instances (e.g., if default constructor throws)
    • Setters on component instance for DI-injected properties
    • Initialization logic in DI services (e.g., if they throw while being instantiated)
    • Action needed. All three of the above occur inside ComponentFactory.InstantiateComponent, which in turn happens either inside render tree diffing, or CreateInitialRenderAsync (prerendering), or RemoteRenderer.AddComponentAsync (for non-prerendered root components). The tree diffing and non-prerendered root component cases at least, we again just pass exceptions to the renderer's UnhandledException event, which trusts the client to disconnect, and it can just choose not to. We need the server to enforce disconnection on these errors. Enforce circuit termination on unhandled exception #11845
  • Component lifecycle methods from the IComponent perspective
    • IComponent.Configure
      • Action needed. Always called from Renderer's AttachAndInitComponent, which is called from render tree diffing, non-prerendered root component initialization, and prerendering. Ignoring prerendering (this is investigated elsewhere), exceptions will be handled the same as for the component initialization cases above. Again, this needs action because we're trusting the client to disconnect. Enforce circuit termination on unhandled exception #11845
    • IComponent.SetParametersAsync
      • Action needed. SetParametersAsync is called from ComponentState.SetDirectParameters and ComponentState.NotifyCascadingValueChanged, both of which capture any exception as a failing task, which is then passed to _renderer.AddToPendingTasks. The renderer spots failed tasks and calls its own HandleException with them, but besides calling that, swallows the exception. As such, rendering attempts to continue and will behave as if nothing's wrong. RemoteRenderer's HandleException does send the exception info to the client, which then disconnects the circuit, which makes sense at development time. But in production, what if a bad client chooses not to disconnect the circuit? Then they continue with the application in an unknown state. This shouldn't be allowed: the server should proactively kill the circuit if there's an unhandled rendering exception. Enforce circuit termination on unhandled exception #11845
  • Component lifecycle methods from the ComponentBase perspective. Want to be sure the failure modes make sense at this higher level too.
    • ComponentBase.ctor - Duplicate: it's the same as IComponent's default constructor
    • ComponentBase.BuildRenderTree - Semi-OK: This is called when the framework executes ProcessRenderQueue, which has a big try/catch around RenderInExistingBatch. Exceptions get shuttled to the renderer's HandleException, which has problems described elsewhere around letting clients ignore exceptions. Once we fix that, this will be fine, so marking as OK to avoid duplication.
    • ComponentBase.OnInit - Duplicate: this reduces to SetParametersAsync
    • ComponentBase.OnInitAsync - Duplicate: this reduces to SetParametersAsync
    • ComponentBase.OnParametersSet - Duplicate: this reduces to SetParametersAsync
    • ComponentBase.OnParametersSetAsync - Duplicate: this reduces to SetParametersAsync
    • ComponentBase.StateHasChanged- Duplicate: called either by the developer themselves from other lifecycle methods, or by ComponentBase from its own lifecycle methods. So there's nothing distinct to investigate here.
    • ComponentBase.ShouldRender - Duplicate: called only by ComponentBase itself from StateHasChanged
    • ComponentBase.OnAfterRender - Duplicate: this is the implementation of IHandleAfterRender.OnAfterRenderAsync
    • ComponentBase.OnAfterRenderAsync - Duplicate: this is the implementation of IHandleAfterRender.OnAfterRenderAsync
    • ComponentBase.Invoke - Called by user code. If you're already on the sync context, just invokes the supplied delegate synchronously, so exceptions just go upstream. If you're not on the sync context, you get back a Task representing the success/exception of your callback, so it becomes up to the caller how exceptions are handled. It's no different from invoking your own action directly.
    • ComponentBase.InvokeAsync - Same behavior as ComponentBase.Invoke, even though some of the internal code paths are different.
    • ComponentBase.SetParametersAsync - Duplicate: this is the implementation of IComponent.SetParametersAsync, described above.
  • Event handler callbacks
    • IHandleEvent.HandleEventAsync
      • Action needed. One place this is called from is BindMethods.DispatchEventAsync, which comes with a comment saying "this is a temporary polyfill that doesn't do proper error handling and will be removed". Can this be removed now? @rynowak Remove temporary logic from BindMethods.DispatchEventAsync #11846
      • Action needed. Besides that, it's called from EventCallback/EventCallback<T>'s InvokeAsync. The first of these is called by Renderer.DispatchEventAsync , which is the code path for event notifications from the client (via the JS-invokable RendererRegistryEventDispatcher.DispatchEvent). It does a try/catch around the callback invocation, and for any exceptions, it calls the Renderer's HandleException which is the same anti-pattern as we see elsewhere whereby it trusts the client to disconnect. Enforce circuit termination on unhandled exception #11845
      • Action needed. In the case where the event handler throws synchronously, there's a bug in Renderer.DispatchEventAsync. The task variable is left with a null value, which then throws a nullref exception in GetErrorHandledTask. Renderer.DispatchEventAsync throws null reference exception if event handler throws synchronously #11847
  • After-render callbacks
    • IHandleAfterRender.OnAfterRenderAsync
      • Action needed. This is called from Renderer.NotifyRenderCompleted, which has the same anti-pattern as elsewhere, whereby we pass sync/async exceptions to renderer's HandleException and trust the client to disconnect. We need the server to enforce circuit termination on exception. Enforce circuit termination on unhandled exception #11845
  • Component and DI service disposal
    • IDisposable.Dispose called on a component instance
      • Action needed. In ComponentState.DisposeInBatch, there's a comment saying "TODO: Handle components throwing during dispose. Shouldn't break the whole render batch.". I would disagree with this: I would now say that unhandled exceptions in component disposal should abandon rendering and terminate the whole circuit. We should check the behavior is correct and remove the comment. Enforce circuit termination on unhandled exception #11845
      • Action needed. Well, non-action needed in this case. When we're disposing a Renderer, we loop through all the component instances and Dispose them. If that method fails, we call HandleException but otherwise swallow the exception. This is the correct behavior, since it's essential we don't fail to dispose other components or finish disposing the renderer itself. However, if we fix HandleException to force-kill the circuit immediately, we have to be sure we don't break this disposal logic by halting it on the first exception. Enforce circuit termination on unhandled exception #11845
    • Disposal of DI services injected into a component
      • Action needed. CircuitHost.DisposeAsync should use try/finally to guarantee that _scope.Dispose still happens even if any of OnConnectionDownAsync/OnCircuitDownAsync/Renderer.Dispose throws. Blazor doesn't always dispose the circuit's DI scope #11848
  • Routing (probably nothing to dig into here, as routing is implemented as a regular component with no special powers)
    • Discovery of components; parsing route templates, etc.
      • All OK. The Router is just a normal userland component with no special powers, so all its possible failure modes are the same as normal user components. For example, if URL template parsing throws, this reduces to an exception in SetParametersAsync, so is the same as described above.
  • Prerendering (make sure some top-level exception handler would catch all sync and async exceptions)
    • Action needed. If there's an unhandled exception during CircuitPrerenderer.PrerenderComponentAsync, this bubbles up to be the same as an unhandled exception during rendering a Razor page, which is good and correct. However, in this case we fail to call CleanupCircuitState, so any DI services already injected into the circuit don't get disposed. Blazor doesn't always dispose the circuit's DI scope #11848
  • Arbitrary errors at render time, even outside the calls to BuildRenderTree etc.
    • For example, if a component holds on to a ParameterCollection instance, it could later mutate it to corrupt the memory inside a RenderTreeBuilder that was being used by a later render cycle. This could lead to an exception in any part of the rendering system. We must be sure that (1) this can't affect any other circuit (e.g., to crash it, or to read/write state across circuits) and (2) that arbitrary render-time exceptions at worst only bring down one circuit, not the whole process.
      • Semi-OK. Virtually all of the rendering code runs inside Renderer.ProcessRenderQueue, which has a big try/catch and marshals exceptions to HandleException. This is not completely OK, because of the antipattern described above whereby we're trusting clients then to terminate the connection. Once we fix that so the server enforces immediate circuit termination, this should be OK. Marking as verified to avoid so much duplication. The rendering logic outside ProcessRenderQueue is pretty much entirely just under the control of the framework, so it's hard to see what you'd do to make it fail, but even if it does throw, it's likely to reduce to being an error on whatever triggered the rendering (usually a StateHasChanged call from ComponentBase, which is already described above).

To make this tractable, I'll mostly be focused on the low-level responsibility boundaries and ensuring that we are catching and dealing with any exceptions that may occur. Mostly this won't involve tracing into higher-level features. For example it's unnecessary to ask the question "what if an auth policy evaluator throws" because that's something that happens within AuthorizeView, which is a component that only has the power to do what any other userland component can do.

[SteveSandersonMS]

@javiercn I've assigned this to myself because this is part of #10472, but I understand you're already doing some work in this area. Can we coordinate to figure out which parts of this you're doing?

[javiercn]

Yep. Here is a list of stuff

• Invoke .NET interop with malformed JSON data
• Invoke JSInterop completion with malformed JSON data
• Invoke .NET interop with invalid .NET object ref
• Invoke .NET interop with empty string assembly.
• Invoke .NET interop with empty string method identifier.
• Invoke .NET interop non invokable method.
• Invoke .NET interop non existing method.
• Invoke .NET interop non-existing object reference.

[SteveSandersonMS]

OK, @javiercn and I discussed: he's going to deal with resilience of JS interop cases (ensuring we fail in sensible ways and that clients can't cause a global unhandled exception in any case).

I'll deal with the prevention of global exceptions in other places.

[javiercn]

@SteveSandersonMS can we also have bullet points for other areas? Even just to confirm that those are handled. I will have to look at other areas no matter what for completeness from the security point of view, I just want the list to be common and in one place, does that make sense?

[SteveSandersonMS]

Here are a list of all the statics in Microsoft.AspNetCore.Components:

Still to do: similar investigation for .Server and .Browser

---

Action needed

• Make readonly
  • EventCallbackFactoryBinderExtensions.ConvertToBool/ConvertToNullableBool
  • InputNumber._parser
  • InputNumber._stepAttributeValue
• UIEventArgs.Empty
  • This may get passed to application code then mutated (you can write to Type). We should ensure we don't rely on this not being mutated.

#11849

Readonly never-mutated const-like things

• ComponentFactory._injectablePropertyBindingFlags
• EventCallback.Factory
• EventCallback.Empty
• EventCallbackFactoryBinderExtensions.ConvertToString
• EventCallbackWorkItem.Empty
• ValidationRequestedEventArgs.Empty
• ValidationStateChangedEventArgs.Empty
• JsonSerializerOptionsProvider.Options
• ParameterCollection._emptyCollectionFrames (though note: theoretically a developer could write code that mutates it. It would be an extremely strange thing to do)
• ParameterCollection._emptyCollection
• PlatformInfo.IsWebAssembly
• HtmlRenderer.SelfClosingElements
• RendererSynchronizationContext.ExecutionContextThunk
• RendererSynchronizationContext.BackgroundWorkThunk
• RenderTreeBuilder.BoxedTrue/BoxedFalse/ComponentReferenceCaptureInvalidParentMessage
• RouteContext.Separator
• Router._queryOrHashStartChar
• RouteTemplate.Separators
• TemplateParser.InvalidParameterNameCharacters

Lazily-initialized but thereafter readonly const-like things

• BinderConverterCache._convertToEnum
• BinderConverterCache._convertToNullableEnum

Caches populated using only non-user-specific (and non-APPLICATION-specific) data

• CascadingParameterState._cachedInfos
• BinderConverterCache._cache
• EditContextDataAnnotationsExtensions._propertyInfoCache
• ParameterCollectionExtensions._cachedWritersByType
• RouteConstraint._cachedConstraints

Other

• ElementRef._nextIdForWebAssemblyOnly
  • OK, only used if PlatformInfo.IsWebAssembly

[rynowak]

Also think about whether these cases have good unit tests. Many of these could easily be covered.

[rynowak]

This is a good list 👍

[SteveSandersonMS]

Statics in .Server, all of which are OK:

Const-likes

• BlazorPackHubProtocol.ProtocolVersion
• Sequence.DefaultLengthFromArrayPool
• SequenceSegment.IsValueTypeElement
• CircuitHost.Log.*
• CircuitPrerenderer.CircuitHostKey
• CircuitPrerenderer.NavigationStatusKey
• CircuitRegistry.Log.*
• RemoteRenderer.CanceledTask
• RemoteRenderer.Log.*
• ComponentHub.CircuitKey
• ComponentHub.DefaultPath

Async-local

• CircuitHost._current

[SteveSandersonMS]

And for .Browser:

Const-likes

• BrowserUriHelperInterop.*

Async-local

• RendererRegistry._current

Action needed

• RendererRegistry._globalRegistry
  • This is only intended for use when running on WASM. AFAIK, there's no way it could be used in server-side code. However, there's not any super-explicit mechanism to guarantee we don't introduce any future bug whereby it does get used in server-side code, and then shares state between users. I recommend we wrap it in some accessor that throws if !PlatformInfo.IsWebAssembly (possibly caching this info).

#11849

[SteveSandersonMS]

OK, I've gone through all the investigations and split out all the action items into separate linked issues. So I'm now unassigning myself from this, leaving the remaining JSInterop bits to be looked at by @javiercn.

[javiercn]

All the JS interop is done as part of #11958

[SteveSandersonMS]

OK great, marking this one as done then.

[javiercn]

@SteveSandersonMS When you sign off on my PR :)

[javiercn]

My work here is done #11958