Fix | Scope configurable retry logic assembly resolution to opt-in callers

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Reliability/SqlConfigurableRetryLogicLoader.cs

@@ -50,14 +50,6 @@ public SqlConfigurableRetryLogicLoader(
             string cnnSectionName = SqlConfigurableRetryConnectionSection.Name,
             string cmdSectionName = SqlConfigurableRetryCommandSection.Name)
         {
-            #if NET
-            // Just only one subscription to this event is required.
-            // This class isn't supposed to be called more than one time;
-            // SqlConfigurableRetryLogicManager manages a single instance of this class.
-            System.Runtime.Loader.AssemblyLoadContext.Default.Resolving -= Default_Resolving;
-            System.Runtime.Loader.AssemblyLoadContext.Default.Resolving += Default_Resolving;
-            #endif
-
             AssignProviders(connectionRetryConfigs == null ? null : CreateRetryLogicProvider(cnnSectionName, connectionRetryConfigs),
                             commandRetryConfigs == null ? null : CreateRetryLogicProvider(cmdSectionName, commandRetryConfigs));
         }
@@ -119,20 +111,36 @@ private static SqlRetryLogicBaseProvider ResolveRetryLogicProvider(string config
                 throw new ArgumentNullException(nameof(retryMethod), StringsHelper.GetString(Strings.SQLCR_RetryMethodNullOrEmpty));
             }
 
-            Type type = null;
-            try
+            Type type;
+            if (string.IsNullOrEmpty(configurableRetryType))
             {
-                // Resolve a Type object from the given type name
-                // Different implementation in .NET Framework & .NET Core
-                type = LoadType(configurableRetryType);
+                // No custom retry logic type was configured, so there is nothing to resolve and the
+                // built-in factory is used to discover the requested retry method.
+                //
+                // Short-circuiting here matters for more than performance: on .NET, LoadType
+                // temporarily subscribes an assembly resolving handler to the default
+                // AssemblyLoadContext. That handler should never be installed on behalf of callers
+                // who did not explicitly opt in to loading a custom retry logic type.
+                type = typeof(SqlConfigurableRetryFactory);
+                SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> No custom retry logic type is configured; Using the internal `{2}` type.",
+                                                       TypeName, methodName, type.FullName);
             }
-            catch (Exception e)
+            else
             {
-                // Try to use 'SqlConfigurableRetryFactory' as a default type to discover retry methods
-                // if there is a problem, resolve using the 'configurableRetryType' type.
-                type = typeof(SqlConfigurableRetryFactory);
-                SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Unable to load the '{2}' type; Trying to use the internal `{3}` type: {4}",
-                                                       TypeName, methodName, configurableRetryType, type.FullName, e);
+                try
+                {
+                    // Resolve a Type object from the given type name
+                    // Different implementation in .NET Framework & .NET Core
+                    type = LoadType(configurableRetryType);
+                }
+                catch (Exception e)
+                {
+                    // Try to use 'SqlConfigurableRetryFactory' as a default type to discover retry methods
+                    // if there is a problem, resolve using the 'configurableRetryType' type.
+                    type = typeof(SqlConfigurableRetryFactory);
+                    SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Unable to load the '{2}' type; Trying to use the internal `{3}` type: {4}",
+                                                           TypeName, methodName, configurableRetryType, type.FullName, e);
+                }
             }
 
             // Run the function by using the resolved values to get the SqlRetryLogicBaseProvider object
@@ -336,25 +344,42 @@ private static ICollection<int> SplitErrorNumberList(string list)
         #region Type Resolution
 
         #if NET
+        /// <summary>
+        /// The directory that user-supplied configurable retry logic assemblies are probed from.
+        /// </summary>
+        /// <remarks>
+        /// This is deliberately the application base directory rather than the current working
+        /// directory. The working directory is ambient process state that can be changed at any
+        /// time and is not necessarily related to where the application's binaries live, so
+        /// probing it can load assemblies from an unintended and untrusted location.
+        /// </remarks>
+        private static string ProbingDirectory => AppContext.BaseDirectory;
+
         private static Assembly AssemblyResolver(AssemblyName arg)
         {
             string methodName = nameof(AssemblyResolver);
 
-            string fullPath = MakeFullPath(Environment.CurrentDirectory, arg.Name);
+            string fullPath = MakeFullPath(ProbingDirectory, arg.Name);
             SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Looking for '{2}' assembly by '{3}' full path."
                 , TypeName, methodName, arg, fullPath);
 
             return fullPath == null ? null : AssemblyLoadContext.Default.LoadFromAssemblyPath(fullPath);
         }
 
         /// <summary>
-        /// Load assemblies on request.
+        /// Load dependencies of a user-supplied configurable retry logic assembly on request.
         /// </summary>
+        /// <remarks>
+        /// This handler is only subscribed for the duration of the type resolution performed by
+        /// <see cref="LoadType"/>, and only when a custom retry logic type has been configured.
+        /// It must never remain subscribed to <see cref="AssemblyLoadContext.Default"/> after that,
+        /// because doing so changes assembly resolution behavior for the entire application.
+        /// </remarks>
         private static Assembly Default_Resolving(AssemblyLoadContext arg1, AssemblyName arg2)
         {
             string methodName = nameof(Default_Resolving);
 
-            string target = MakeFullPath(Environment.CurrentDirectory, arg2.Name);
+            string target = MakeFullPath(ProbingDirectory, arg2.Name);
             SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Looking for '{2}' assembly that is requested by '{3}' ALC from '{4}' path."
                 , TypeName, methodName, arg2, arg1, target);
 
@@ -371,7 +396,21 @@ private static Type LoadType(string fullyQualifiedName)
             string methodName = nameof(LoadType);
             SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> Entry point.", TypeName, methodName);
 
-            var result = Type.GetType(fullyQualifiedName, AssemblyResolver, TypeResolver);
+            Type result;
+
+            // Scope the resolving handler to this call only. Leaving it subscribed would alter
+            // assembly resolution for the whole application, so unrelated assemblies the host
+            // failed to find could be served from this component's probing directory.
+            AssemblyLoadContext.Default.Resolving += Default_Resolving;
+            try
+            {
+                result = Type.GetType(fullyQualifiedName, AssemblyResolver, TypeResolver);
+            }
+            finally
+            {
+                AssemblyLoadContext.Default.Resolving -= Default_Resolving;
+            }
+
             if (result != null)
             {
                 SqlClientEventSource.Log.TryTraceEvent("<sc.{0}.{1}|INFO> The '{2}' type is resolved.",

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConfigurableRetryLogicTest.cs

@@ -1,8 +1,10 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.IO;
+using System.Reflection;
 using System.Threading.Tasks;
 using Xunit;
 
@@ -80,5 +82,53 @@ public void ValidateRetryParameters()
             option.AuthorizedSqlCondition = null;
             SqlConfigurableRetryFactory.CreateIncrementalRetryProvider(option);
         }
+
+#if NET
+        /// <summary>
+        /// Regression test: triggering the configurable retry logic loader must not leave a
+        /// process-wide <see cref="System.Runtime.Loader.AssemblyLoadContext.Default"/> resolving
+        /// handler installed that probes the current working directory. The working directory is
+        /// ambient process state unrelated to where the application's binaries live, so resolving
+        /// assemblies from it can load code from an unintended location.
+        /// </summary>
+        [Fact]
+        public void RetryLogicProviderDoesNotEnableCurrentDirectoryAssemblyProbing()
+        {
+            // Touch the default retry logic providers to force SqlConfigurableRetryLogicLoader
+            // construction via its normal code path.
+            Assert.NotNull(new SqlCommand().RetryLogicProvider);
+            Assert.NotNull(new SqlConnection().RetryLogicProvider);
+
+            string probeDirectory = Path.Combine(Path.GetTempPath(), "mds-crl-plant-" + Guid.NewGuid().ToString("N"));
+            Directory.CreateDirectory(probeDirectory);
+            string originalCurrentDirectory = Environment.CurrentDirectory;
+
+            try
+            {
+                // A file that is not a valid assembly. If the loader probes the current working
+                // directory it will try to load this file and fail with BadImageFormatException.
+                // With correct behavior the runtime never looks here and reports the assembly as
+                // simply not found.
+                string assemblySimpleName = "MdsProbeAssembly_" + Guid.NewGuid().ToString("N");
+                File.WriteAllText(Path.Combine(probeDirectory, assemblySimpleName + ".dll"), "not an assembly");
+
+                Environment.CurrentDirectory = probeDirectory;
+
+                Assert.Throws<FileNotFoundException>(() => Assembly.Load(new AssemblyName(assemblySimpleName)));
+            }
+            finally
+            {
+                Environment.CurrentDirectory = originalCurrentDirectory;
+                try
+                {
+                    Directory.Delete(probeDirectory, true);
+                }
+                catch (IOException)
+                {
+                    // Best effort cleanup.
+                }
+            }
+        }
+#endif
     }
 }

src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/SqlConfigurableRetryLogicLoaderTest.cs

@@ -0,0 +1,141 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+#if NET
+
+using System;
+using System.Linq;
+using System.Reflection;
+using System.Runtime.Loader;
+using Xunit;
+
+namespace Microsoft.Data.SqlClient.UnitTests;
+
+/// <summary>
+/// Unit tests validating that <see cref="SqlConfigurableRetryLogicLoader"/> never leaves a
+/// process-wide assembly resolving handler attached to
+/// <see cref="AssemblyLoadContext.Default"/>.
+/// </summary>
+/// <remarks>
+/// A handler left attached there participates in resolution of every assembly the host
+/// application fails to find, not just the retry logic assembly the loader was interested in.
+/// That silently changes assembly loading behaviour for code that never opted in to this
+/// feature, and can serve unrelated assemblies out of this component's probing directory.
+/// </remarks>
+public class SqlConfigurableRetryLogicLoaderTest
+{
+    /// <summary>
+    /// The default code path: no configuration at all. The loader must not subscribe to the
+    /// default load context.
+    /// </summary>
+    [Fact]
+    public void Constructor_WithNoConfiguration_DoesNotSubscribeToDefaultLoadContext()
+    {
+        _ = new SqlConfigurableRetryLogicLoader(null, null);
+
+        AssertNoLoaderResolvingHandlerAttached();
+    }
+
+    /// <summary>
+    /// A configuration that supplies only a retry method, which is the documented way to select
+    /// one of the built-in retry providers. No custom type is being requested, so no assembly
+    /// resolution is required and no handler may be subscribed - not even transiently.
+    /// </summary>
+    [Theory]
+    [InlineData(null)]
+    [InlineData("")]
+    public void Constructor_WithoutRetryLogicType_DoesNotSubscribeToDefaultLoadContext(string? retryLogicType)
+    {
+        TestRetryConnectionSection section = CreateSection(retryLogicType);
+
+        SqlConfigurableRetryLogicLoader loader = new(section, null);
+
+        // The built-in factory still resolves the requested method.
+        Assert.NotNull(loader.ConnectionProvider);
+        AssertNoLoaderResolvingHandlerAttached();
+    }
+
+    /// <summary>
+    /// A configuration that requests a custom retry logic type legitimately needs assembly
+    /// resolution, but the handler must be removed again once type resolution has finished.
+    /// </summary>
+    [Fact]
+    public void Constructor_WithUnresolvableRetryLogicType_DoesNotLeaveHandlerSubscribed()
+    {
+        TestRetryConnectionSection section =
+            CreateSection("Some.Namespace.NoSuchType, Some.Assembly.That.Does.Not.Exist");
+
+        SqlConfigurableRetryLogicLoader loader = new(section, null);
+
+        // Resolution fails and falls back to the built-in factory rather than throwing.
+        Assert.NotNull(loader.ConnectionProvider);
+        AssertNoLoaderResolvingHandlerAttached();
+    }
+
+    private static TestRetryConnectionSection CreateSection(string? retryLogicType) =>
+        new()
+        {
+            RetryLogicType = retryLogicType!,
+            RetryMethod = nameof(SqlConfigurableRetryFactory.CreateFixedRetryProvider),
+            NumberOfTries = 2,
+            DeltaTime = TimeSpan.FromSeconds(1),
+            MinTimeInterval = TimeSpan.Zero,
+            MaxTimeInterval = TimeSpan.FromSeconds(10),
+        };
+
+    /// <summary>
+    /// Asserts that no delegate declared by <see cref="SqlConfigurableRetryLogicLoader"/> is
+    /// subscribed to the <see cref="AssemblyLoadContext.Default"/> resolving event.
+    /// </summary>
+    /// <remarks>
+    /// The event exposes only add/remove accessors, so its backing field is read reflectively.
+    /// If the runtime ever renames that field this assertion fails loudly rather than silently
+    /// passing, which is the desired behaviour for a regression test.
+    /// </remarks>
+    private static void AssertNoLoaderResolvingHandlerAttached()
+    {
+        FieldInfo? resolvingField = typeof(AssemblyLoadContext).GetField(
+            "_resolving",
+            BindingFlags.Instance | BindingFlags.NonPublic);
+
+        Assert.True(
+            resolvingField is not null,
+            "Could not locate the backing field of AssemblyLoadContext.Resolving. This test needs " +
+            "updating for the current runtime.");
+
+        Delegate? resolving = (Delegate?)resolvingField!.GetValue(AssemblyLoadContext.Default);
+
+        string[] offenders = resolving is null
+            ? []
+            : resolving.GetInvocationList()
+                .Where(handler => handler.Method.DeclaringType == typeof(SqlConfigurableRetryLogicLoader))
+                .Select(handler => handler.Method.Name)
+                .ToArray();
+
+        Assert.True(
+            offenders.Length == 0,
+            $"SqlConfigurableRetryLogicLoader left {offenders.Length} handler(s) subscribed to " +
+            $"AssemblyLoadContext.Default.Resolving: {string.Join(", ", offenders)}. A process-wide " +
+            "handler changes assembly resolution for the entire application.");
+    }
+
+    private sealed class TestRetryConnectionSection : ISqlConfigurableRetryConnectionSection
+    {
+        public TimeSpan DeltaTime { get; set; }
+
+        public TimeSpan MaxTimeInterval { get; set; }
+
+        public TimeSpan MinTimeInterval { get; set; }
+
+        public int NumberOfTries { get; set; }
+
+        public string RetryLogicType { get; set; } = string.Empty;
+
+        public string RetryMethod { get; set; } = string.Empty;
+
+        public string TransientErrors { get; set; } = string.Empty;
+    }
+}
+
+#endif