Only add default services once in WebApplicationBuilder (#34782)

- Only add default services once
- Lazily read ForwardedHeaders_Enabled config at starup
- Add test verifying service scope validation is enabled in dev

Author: halter73

src/DefaultBuilder/src/BootstrapHostBuilder.cs

@@ -1,8 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Collections.Generic;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
@@ -15,16 +13,20 @@ internal class BootstrapHostBuilder : IHostBuilder
     {
         private readonly ConfigurationManager _configuration;
         private readonly WebHostEnvironment _environment;
-
+        private readonly IServiceCollection _serviceCollection;
         private readonly HostBuilderContext _hostContext;
 
         private readonly List<Action<IConfigurationBuilder>> _configureHostActions = new();
         private readonly List<Action<HostBuilderContext, IConfigurationBuilder>> _configureAppActions = new();
+        private readonly List<Action<HostBuilderContext, IServiceCollection>> _configureServicesActions = new();
+
+        private readonly List<Action<IHostBuilder>> _remainingOperations = new();
 
-        public BootstrapHostBuilder(ConfigurationManager configuration, WebHostEnvironment webHostEnvironment)
+        public BootstrapHostBuilder(ConfigurationManager configuration, WebHostEnvironment webHostEnvironment, IServiceCollection serviceCollection)
         {
             _configuration = configuration;
             _environment = webHostEnvironment;
+            _serviceCollection = serviceCollection;
 
             _hostContext = new HostBuilderContext(Properties)
             {
@@ -51,6 +53,12 @@ public IHostBuilder ConfigureContainer<TContainerBuilder>(Action<HostBuilderCont
         {
             // This is not called by HostingHostBuilderExtensions.ConfigureDefaults currently, but that could change in the future.
             // If this does get called in the future, it should be called again at a later stage on the ConfigureHostBuilder.
+            if (configureDelegate is null)
+            {
+                throw new ArgumentNullException(nameof(configureDelegate));
+            }
+
+            _remainingOperations.Add(hostBuilder => hostBuilder.ConfigureContainer<TContainerBuilder>(configureDelegate));
             return this;
         }
 
@@ -60,33 +68,40 @@ public IHostBuilder ConfigureHostConfiguration(Action<IConfigurationBuilder> con
             return this;
         }
 
-        public string? GetSetting(string key)
-        {
-            return _configuration[key];
-        }
-
         public IHostBuilder ConfigureServices(Action<HostBuilderContext, IServiceCollection> configureDelegate)
         {
             // HostingHostBuilderExtensions.ConfigureDefaults calls this via ConfigureLogging
-            // during the initial config stage. It should be called again later on the ConfigureHostBuilder.
+            _configureServicesActions.Add(configureDelegate ?? throw new ArgumentNullException(nameof(configureDelegate)));
             return this;
         }
 
         public IHostBuilder UseServiceProviderFactory<TContainerBuilder>(IServiceProviderFactory<TContainerBuilder> factory) where TContainerBuilder : notnull
         {
             // This is not called by HostingHostBuilderExtensions.ConfigureDefaults currently, but that could change in the future.
             // If this does get called in the future, it should be called again at a later stage on the ConfigureHostBuilder.
+            if (factory is null)
+            {
+                throw new ArgumentNullException(nameof(factory));
+            }
+
+            _remainingOperations.Add(hostBuilder => hostBuilder.UseServiceProviderFactory<TContainerBuilder>(factory));
             return this;
         }
 
         public IHostBuilder UseServiceProviderFactory<TContainerBuilder>(Func<HostBuilderContext, IServiceProviderFactory<TContainerBuilder>> factory) where TContainerBuilder : notnull
         {
             // HostingHostBuilderExtensions.ConfigureDefaults calls this via UseDefaultServiceProvider
             // during the initial config stage. It should be called again later on the ConfigureHostBuilder.
+            if (factory is null)
+            {
+                throw new ArgumentNullException(nameof(factory));
+            }
+
+            _remainingOperations.Add(hostBuilder => hostBuilder.UseServiceProviderFactory<TContainerBuilder>(factory));
             return this;
         }
 
-        internal void RunConfigurationCallbacks()
+        public void RunDefaultCallbacks(HostBuilder innerBuilder)
         {
             foreach (var configureHostAction in _configureHostActions)
             {
@@ -103,6 +118,16 @@ internal void RunConfigurationCallbacks()
             }
 
             _environment.ApplyConfigurationSettings(_configuration);
+
+            foreach (var configureServicesAction in _configureServicesActions)
+            {
+                configureServicesAction(_hostContext, _serviceCollection);
+            }
+
+            foreach (var callback in _remainingOperations)
+            {
+                callback(innerBuilder);
+            }
         }
     }
 }

src/DefaultBuilder/src/ConfigureHostBuilder.cs

@@ -1,8 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Collections.Generic;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Hosting;
@@ -39,8 +37,6 @@ internal ConfigureHostBuilder(ConfigurationManager configuration, WebHostEnviron
             };
         }
 
-        internal bool ConfigurationEnabled { get; set; }
-
         IHost IHostBuilder.Build()
         {
             throw new NotSupportedException($"Call {nameof(WebApplicationBuilder)}.{nameof(WebApplicationBuilder.Build)}() instead.");
@@ -49,12 +45,9 @@ IHost IHostBuilder.Build()
         /// <inheritdoc />
         public IHostBuilder ConfigureAppConfiguration(Action<HostBuilderContext, IConfigurationBuilder> configureDelegate)
         {
-            if (ConfigurationEnabled)
-            {
-                // Run these immediately so that they are observable by the imperative code
-                configureDelegate(_context, _configuration);
-                _environment.ApplyConfigurationSettings(_configuration);
-            }
+            // Run these immediately so that they are observable by the imperative code
+            configureDelegate(_context, _configuration);
+            _environment.ApplyConfigurationSettings(_configuration);
 
             return this;
         }
@@ -74,12 +67,9 @@ public IHostBuilder ConfigureContainer<TContainerBuilder>(Action<HostBuilderCont
         /// <inheritdoc />
         public IHostBuilder ConfigureHostConfiguration(Action<IConfigurationBuilder> configureDelegate)
         {
-            if (ConfigurationEnabled)
-            {
-                // Run these immediately so that they are observable by the imperative code
-                configureDelegate(_configuration);
-                _environment.ApplyConfigurationSettings(_configuration);
-            }
+            // Run these immediately so that they are observable by the imperative code
+            configureDelegate(_configuration);
+            _environment.ApplyConfigurationSettings(_configuration);
 
             return this;
         }

src/DefaultBuilder/src/ForwardedHeadersOptionsSetup.cs

@@ -0,0 +1,34 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.HttpOverrides;
+using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.Options;
+
+namespace Microsoft.AspNetCore
+{
+    internal class ForwardedHeadersOptionsSetup : IConfigureOptions<ForwardedHeadersOptions>
+    {
+        private readonly IConfiguration _configuration;
+
+        public ForwardedHeadersOptionsSetup(IConfiguration configuration)
+        {
+            _configuration = configuration;
+        }
+
+        public void Configure(ForwardedHeadersOptions options)
+        {
+            if (!string.Equals("true", _configuration["ForwardedHeaders_Enabled"], StringComparison.OrdinalIgnoreCase))
+            {
+                return;
+            }
+
+            options.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto;
+            // Only loopback proxies are allowed by default. Clear that restriction because forwarders are
+            // being enabled by explicit configuration.
+            options.KnownNetworks.Clear();
+            options.KnownProxies.Clear();
+        }
+    }
+}

src/DefaultBuilder/src/ForwardedHeadersStartupFilter.cs

@@ -4,13 +4,26 @@
 using System;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Hosting;
+using Microsoft.Extensions.Configuration;
 
 namespace Microsoft.AspNetCore
 {
     internal class ForwardedHeadersStartupFilter : IStartupFilter
     {
+        private readonly IConfiguration _configuration;
+
+        public ForwardedHeadersStartupFilter(IConfiguration configuration)
+        {
+            _configuration = configuration;
+        }
+
         public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
         {
+            if (!string.Equals("true", _configuration["ForwardedHeaders_Enabled"], StringComparison.OrdinalIgnoreCase))
+            {
+                return next;
+            }
+
             return app =>
             {
                 app.UseForwardedHeaders();

src/DefaultBuilder/src/WebApplicationBuilder.cs

@@ -20,10 +20,13 @@ public sealed class WebApplicationBuilder
         private const string EndpointRouteBuilderKey = "__EndpointRouteBuilder";
 
         private readonly HostBuilder _hostBuilder = new();
-        private readonly ConfigureHostBuilder _deferredHostBuilder;
-        private readonly ConfigureWebHostBuilder _deferredWebHostBuilder;
+        private readonly BootstrapHostBuilder _bootstrapHostBuilder;
         private readonly WebHostEnvironment _environment;
         private readonly WebApplicationServiceCollection _services = new();
+
+        // This is effectively readonly because it is always set inline by the ConfigureWebHostDefaults callback in the ctor.
+        private IWebHostBuilder _capturedWebHostBuilder = default!;
+
         private WebApplication? _builtApplication;
 
         internal WebApplicationBuilder(Assembly? callingAssembly, string[]? args = null)
@@ -34,31 +37,34 @@ internal WebApplicationBuilder(Assembly? callingAssembly, string[]? args = null)
             Environment = _environment = new WebHostEnvironment(callingAssembly);
 
             Configuration.SetBasePath(_environment.ContentRootPath);
-            Services.AddSingleton(Environment);
 
             // Run methods to configure both generic and web host defaults early to populate config from appsettings.json
             // environment variables (both DOTNET_ and ASPNETCORE_ prefixed) and other possible default sources to prepopulate
             // the correct defaults.
-            var bootstrapBuilder = new BootstrapHostBuilder(Configuration, _environment);
-            bootstrapBuilder.ConfigureDefaults(args);
-            bootstrapBuilder.ConfigureWebHostDefaults(configure: _ => { });
-            bootstrapBuilder.RunConfigurationCallbacks();
+            _bootstrapHostBuilder = new BootstrapHostBuilder(Configuration, _environment, Services);
+            _bootstrapHostBuilder.ConfigureDefaults(args);
+            _bootstrapHostBuilder.ConfigureWebHostDefaults(webHostBuilder =>
+            {
+                // Runs inline.
+                webHostBuilder.Configure(ConfigureApplication);
 
-            Logging = new LoggingBuilder(Services);
-            WebHost = _deferredWebHostBuilder = new ConfigureWebHostBuilder(Configuration, _environment, Services);
-            Host = _deferredHostBuilder = new ConfigureHostBuilder(Configuration, _environment, Services);
+                // We need to override the application name since the call to Configure will set it to
+                // be the calling assembly's name.
+                webHostBuilder.UseSetting(WebHostDefaults.ApplicationKey, _environment.ApplicationName);
 
-            // Add default services
-            _deferredHostBuilder.ConfigureDefaults(args);
+                // Store this so we can apply settings from _environment during build.
+                _capturedWebHostBuilder = webHostBuilder;
+            });
+            _bootstrapHostBuilder.RunDefaultCallbacks(_hostBuilder);
 
-            // Enable changes here because we need to pick up configuration sources added by the generic web host
-            _deferredHostBuilder.ConfigurationEnabled = true;
-            _deferredHostBuilder.ConfigureWebHostDefaults(configure: _ => { });
+            Logging = new LoggingBuilder(Services);
+            WebHost = new ConfigureWebHostBuilder(Configuration, _environment, Services);
+            Host = new ConfigureHostBuilder(Configuration, _environment, Services);
 
             // This is important because GenericWebHostBuilder does the following and we want to preserve the WebHostBuilderContext:
             // context.Properties[typeof(WebHostBuilderContext)] = webHostBuilderContext;
             // context.Properties[typeof(WebHostOptions)] = options;
-            foreach (var (key, value) in _deferredHostBuilder.Properties)
+            foreach (var (key, value) in _bootstrapHostBuilder.Properties)
             {
                 _hostBuilder.Properties[key] = value;
             }
@@ -102,6 +108,9 @@ internal WebApplicationBuilder(Assembly? callingAssembly, string[]? args = null)
         /// <returns>A configured <see cref="WebApplication"/>.</returns>
         public WebApplication Build()
         {
+            // Apply changes made to WebApplicationBuilder.Environment directly before building.
+            _environment.ApplyEnvironmentSettings(_capturedWebHostBuilder, _hostBuilder);
+
             // Copy the configuration sources into the final IConfigurationBuilder
             _hostBuilder.ConfigureHostConfiguration(builder =>
             {
@@ -116,10 +125,36 @@ public WebApplication Build()
                 }
             });
 
-            // We call ConfigureWebHostDefaults AGAIN because config might be added like "ForwardedHeaders_Enabled"
-            // which can add even more services. If not for that, we probably call _hostBuilder.ConfigureWebHost(ConfigureWebHost)
-            // instead in order to avoid duplicate service registration.
-            _hostBuilder.ConfigureWebHostDefaults(ConfigureWebHost);
+            // This needs to go here to avoid adding the IHostedService that boots the server twice (the GenericWebHostService).
+            // Copy the services that were added via WebApplicationBuilder.Services into the final IServiceCollection
+            _hostBuilder.ConfigureServices((context, services) =>
+            {
+                // We've only added services configured by the GenericWebHostBuilder and WebHost.ConfigureWebDefaults
+                // at this point. HostBuilder news up a new ServiceCollection in HostBuilder.Build() we haven't seen
+                // until now, so we cannot clear these services even though some are redundant because
+                // we called ConfigureWebHostDefaults on both the _deferredHostBuilder and _hostBuilder.
+
+                // Ideally, we'd only call _hostBuilder.ConfigureWebHost(ConfigureWebHost) instead of
+                // _hostBuilder.ConfigureWebHostDefaults(ConfigureWebHost) to avoid some duplicate service descriptors,
+                // but we want to add services in the WebApplicationBuilder constructor so code can inspect
+                // WebApplicationBuilder.Services. At the same time, we want to be able which services are loaded
+                // to react to config changes (e.g. ForwardedHeadersStartupFilter).
+                foreach (var s in _services)
+                {
+                    services.Add(s);
+                }
+
+                // Add any services to the user visible service collection so that they are observable
+                // just in case users capture the Services property. Orchard does this to get a "blueprint"
+                // of the service collection
+
+                // Drop the reference to the existing collection and set the inner collection
+                // to the new one. This allows code that has references to the service collection to still function.
+                _services.InnerCollection = services;
+            });
+
+            // Run the other callbacks on the final host builder
+            Host.RunDeferredCallbacks(_hostBuilder);
 
             _builtApplication = new WebApplication(_hostBuilder.Build());
 
@@ -202,44 +237,6 @@ private void ConfigureApplication(WebHostBuilderContext context, IApplicationBui
             }
         }
 
-        private void ConfigureWebHost(IWebHostBuilder genericWebHostBuilder)
-        {
-            genericWebHostBuilder.Configure(ConfigureApplication);
-
-            // This needs to go here to avoid adding the IHostedService that boots the server twice (the GenericWebHostService).
-            // Copy the services that were added via WebApplicationBuilder.Services into the final IServiceCollection
-            genericWebHostBuilder.ConfigureServices((context, services) =>
-            {
-                // We've only added services configured by the GenericWebHostBuilder and WebHost.ConfigureWebDefaults
-                // at this point. HostBuilder news up a new ServiceCollection in HostBuilder.Build() we haven't seen
-                // until now, so we cannot clear these services even though some are redundant because
-                // we called ConfigureWebHostDefaults on both the _deferredHostBuilder and _hostBuilder.
-
-                // Ideally, we'd only call _hostBuilder.ConfigureWebHost(ConfigureWebHost) instead of
-                // _hostBuilder.ConfigureWebHostDefaults(ConfigureWebHost) to avoid some duplicate service descriptors,
-                // but we want to add services in the WebApplicationBuilder constructor so code can inspect
-                // WebApplicationBuilder.Services. At the same time, we want to be able which services are loaded
-                // to react to config changes (e.g. ForwardedHeadersStartupFilter).
-                foreach (var s in _services)
-                {
-                    services.Add(s);
-                }
-
-                // Add any services to the user visible service collection so that they are observable
-                // just in case users capture the Services property. Orchard does this to get a "blueprint"
-                // of the service collection
-
-                // Drop the reference to the existing collection and set the inner collection
-                // to the new one. This allows code that has references to the service collection to still function.
-                _services.InnerCollection = services;
-            });
-
-            // Run the other callbacks on the final host builder
-            _deferredHostBuilder.RunDeferredCallbacks(_hostBuilder);
-
-            _environment.ApplyEnvironmentSettings(genericWebHostBuilder);
-        }
-
         private static IEndpointRouteBuilder? GetEndpointRouteBuilder(IApplicationBuilder app)
         {
             app.Properties.TryGetValue(EndpointRouteBuilderKey, out var value);