Poor handling of circuit handlers with unresolved dependencies in Blazor

OliverShels commented 8 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Describe the bug

When running a Blazor Server Side application with a custom CircuitHandler, if the circuit handler cannot be resolved due to missing dependencies, the application will fail in a very obscure and hard to debug way.

For example, consider the following initialization code in a Program.cs:

using Microsoft.AspNetCore.Components.Server.Circuits;

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddRazorPages();
builder.Services.AddServerSideBlazor().AddCircuitOptions(x => x.DetailedErrors = true);

builder.Services.AddScoped<CircuitHandler, MyBadCircuitHandler>();

var app = builder.Build();

app.UseHttpsRedirection();

app.UseStaticFiles();

app.UseRouting();

app.MapBlazorHub();
app.MapFallbackToPage("/_Host");

app.Run();

public interface IMyUnresolvedDependency { }

public class MyBadCircuitHandler : CircuitHandler
{
    public MyBadCircuitHandler(IMyUnresolvedDependency dep) { }
}

MyBadCircuitHandler cannot be instantiated by the dependency injection framework as it has an unresolved dependency.

When running this application, the circuit will fail to open and the client side page will show the fallback blazor-error-ui error element. In the browser console, blazor.js will print the message, Error: The circuit failed to initialize. with no other context. On the server side, nothing is obviously wrong - though by enabling Debug log messages we can find:

dbug: Microsoft.AspNetCore.Components.Server.ComponentHub[6]
      Circuit initialization failed
      System.InvalidOperationException: Unable to resolve service for type 'Indices.IndexEditor.Ui.Common.FromWpfMessageListener' while attempting to activate 'Indices.IndexEditor.Ui.Common.DefaultCircuitHandler'.
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateArgumentCallSites(ServiceIdentifier serviceIdentifier, Type implementationType, CallSiteChain callSiteChain, ParameterInfo[] parameters, Boolean throwIfCallSiteNotFound)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateConstructorCallSite(ResultCache lifetime, ServiceIdentifier serviceIdentifier, Type implementationType, CallSiteChain callSiteChain)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact(ServiceDescriptor descriptor, ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain, Int32 slot)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateEnumerable(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateCallSite(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.GetCallSite(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain)
         at Microsoft.Extensions.DependencyInjection.ServiceProvider.CreateServiceAccessor(ServiceIdentifier serviceIdentifier)
         at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
         at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier serviceIdentifier, ServiceProviderEngineScope serviceProviderEngineScope)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.GetService(Type serviceType)
         at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType)
         at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService[T](IServiceProvider provider)
         at Microsoft.AspNetCore.Components.Server.Circuits.CircuitFactory.CreateCircuitHostAsync(IReadOnlyList`1 components, CircuitClientProxy client, String baseUri, String uri, ClaimsPrincipal user, IPersistentComponentStateStore store)         at Microsoft.AspNetCore.Components.Server.ComponentHub.StartCircuit(String baseUri, String uri, String serializedComponentRecords, String applicationState)

I think that this error should be more clearly presented to the developer, as it is currently quite hard to debug.

I have created a repository that reproduces the issue here: https://github.com/OliverShels/BlazorCircuitBug

This behaviour exists in net6, net7, and net8. I haven't checked any others.

Expected Behavior

If the developer supplies a circuit handler that cannot be instantiated, then:

The blazor server should print an exception message at a higher severity level than 'Debug'.
- (Ideally, the server should throw an exception in such a way that my debugger can see it - though maybe this is hard to achieve).
The browser console should print the exception message if 'DetailedErrors' are enabled for the app.

Steps To Reproduce

I have created a repository that reproduces the issue here: https://github.com/OliverShels/BlazorCircuitBug

The repository is based on the .net7 'Blazor Server App Empty' template in Visual Studio with changes in two files:

Program.cs, to add the bad circuit handler;
'appsettings.json`, to change the log severity.

Exceptions (if any)

No response

.NET Version

8.0.100

Anything else?

Error in the browser console:

Error in the server logs, after enabling 'Debug' log severity:

Thanks very much!

javiercn commented 8 months ago

@OliverShels thanks for contacting us.

We don't hide DI errors by wrapping them in other errors if that's the ask. That's an issue in the app configuration rather than a problem in the framework.

If you see an error like this, you can break on the error by enabling first chance exceptions. We don't log errors to the server console when they can be caused by untrusted input as that can be leveraged to saturate the server logs with errors and trigger incident responses, etc.

We could consider sending the callstack to the client, but I'd rather update the message to point to the server logs instead.

The browser console should print the exception message if 'DetailedErrors' are enabled for the app.

We are very conservative about the things we send to the client as it's easy to disclose private information by accident, such as leaving detailedErrors on. In that code path an error is always the result of either "bad input" or an application configuration error.

I'll let others on the team chime in, the code for this is here, and provided we all agree, we would be ok accepting a PR for this. https://github.com/dotnet/aspnetcore/blob/3079a17461ddcd4c8eadb97f13675a381c806870/src/Components/Server/src/ComponentHub.cs#L157

ghost commented 8 months ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

javiercn commented 8 months ago

/cc @dotnet/aspnet-blazor-eng in case you have an opinion on whether: 1) Send the callstack to the client when "detailedErrors" is on. 2) Whether its worth updating the error message to point to the server logs.

OliverShels commented 8 months ago

@javiercn Thanks very much for your prompt response and explanation.

Just to clarify for my understanding:

1) In my example, the exception was the result of a mistake by the app developer, but this will not always be the case. If the exception is caused by malicious user input then it is important to not log at a high severity. It is not possible in general to tell the difference between malicious input and a code mistake.

It is OK to log at a 'Debug' level as debug logs are not shown by default, and anyway are unlikely to trigger alerts in monitoring systems.

So therefore you think that CircuitFactory.CreateCircuitHostAsync should not log exceptions at a log level higher than 'Debug'.

2) You think that CircuitFactory.CreateCircuitHostAsync should probably not send full exception information to the client, even if DetailedErrors is on, because its pretty common for app developers to accidentally leave DetailedErrors on in production. The exception message could leak private information.

You instead think that the error message should be updated to direct the app developer to the server logs, perhaps something like:

Error: The circuit failed to initialize. Check the server Debug-level server logs for more details.

Just to add a bit of context - I encountered this error this morning after making a big refactor to lots of files. It took me 3 hours to narrow down the problem and I would like to save future developers that time. 😃

dotnet / aspnetcore