From bea66b62162b8506a5e9c50690741d5b8d7333ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Brawa=C5=84ski?= Date: Wed, 30 Sep 2020 03:24:16 +0200 Subject: [PATCH 1/6] Prevent envvar prefix normalization. Fixes #40911 The fix implementation works like so: - The environment variable prefix is stripped from a variable name. - The variable name is then normalized. - The prefix is then prepended to the variable name. - Additionally, the filtering now occurs at the same time as parsing/transformation. --- ...vironmentVariablesConfigurationProvider.cs | 23 +++++++++---- .../tests/EnvironmentVariablesTest.cs | 32 +++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs index 604e544a8ba47..409894d3c891f 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs @@ -52,12 +52,14 @@ internal void Load(IDictionary envVariables) IEnumerable filteredEnvVariables = envVariables .Cast() - .SelectMany(AzureEnvToAppEnv) - .Where(entry => ((string)entry.Key).StartsWith(_prefix, StringComparison.OrdinalIgnoreCase)); + .SelectMany(x => AzureEnvToAppEnv(x, _prefix)); foreach (DictionaryEntry envVariable in filteredEnvVariables) { - string key = ((string)envVariable.Key).Substring(_prefix.Length); + string key = envVariable.Key as string; + if (key.StartsWith(_prefix, StringComparison.OrdinalIgnoreCase)) + key = key.Substring(_prefix.Length); + data[key] = (string)envVariable.Value; } @@ -69,7 +71,7 @@ private static string NormalizeKey(string key) return key.Replace("__", ConfigurationPath.KeyDelimiter); } - private static IEnumerable AzureEnvToAppEnv(DictionaryEntry entry) + private static IEnumerable AzureEnvToAppEnv(DictionaryEntry entry, string envPrefix = default) { string key = (string)entry.Key; string prefix = string.Empty; @@ -94,12 +96,21 @@ private static IEnumerable AzureEnvToAppEnv(DictionaryEntry ent { prefix = CustomPrefix; } - else + else if (key.StartsWith(envPrefix, StringComparison.OrdinalIgnoreCase)) { - entry.Key = NormalizeKey(key); + // This prevents the prefix from being normalized. + // We can also do a fast path branch, I guess? No point in reallocating if the prefix is empty. + entry.Key = !string.IsNullOrEmpty(envPrefix) + ? envPrefix + NormalizeKey(key.Substring(envPrefix.Length)) + : NormalizeKey(key); + yield return entry; yield break; } + else + { + yield break; + } // Return the key-value pair for connection string yield return new DictionaryEntry( diff --git a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs index 533a93fa83b54..b3f7ca99486d8 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs @@ -153,6 +153,38 @@ public void ReplaceDoubleUnderscoreInEnvironmentVariables() Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:_db1_ProviderName")); } + [Fact] + public void ReplaceDoubleUnderscoreInEnvironmentVariablesButNotPrefix() + { + var dict = new Hashtable() + { + {"test__prefix__with__double__underscores__data__ConnectionString", "connection"}, + {"SQLCONNSTR__db1", "connStr"} + }; + var envConfigSrc = new EnvironmentVariablesConfigurationProvider("test__prefix__with__double__underscores__"); + + envConfigSrc.Load(dict); + + Assert.Equal("connection", envConfigSrc.Get("data:ConnectionString")); + Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:_db1_ProviderName")); + } + + [Fact] + public void ReplaceDoubleUnderscoreInEnvironmentVariablesWithDuplicatedPrefix() + { + var dict = new Hashtable() + { + {"test__test__ConnectionString", "connection"}, + {"SQLCONNSTR__db1", "connStr"} + }; + var envConfigSrc = new EnvironmentVariablesConfigurationProvider("test__"); + + envConfigSrc.Load(dict); + + Assert.Equal("connection", envConfigSrc.Get("test:ConnectionString")); + Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:_db1_ProviderName")); + } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] public void BindingDoesNotThrowIfReloadedDuringBinding() { From 7f8a0a2cd53762c923c509f09ef8f2454b22e630 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Brawa=C5=84ski?= Date: Wed, 30 Sep 2020 03:37:02 +0200 Subject: [PATCH 2/6] Account for the fact that envPrefix is never null. --- .../src/EnvironmentVariablesConfigurationProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs index 409894d3c891f..35129bc1dcd5c 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs @@ -71,7 +71,7 @@ private static string NormalizeKey(string key) return key.Replace("__", ConfigurationPath.KeyDelimiter); } - private static IEnumerable AzureEnvToAppEnv(DictionaryEntry entry, string envPrefix = default) + private static IEnumerable AzureEnvToAppEnv(DictionaryEntry entry, string envPrefix) { string key = (string)entry.Key; string prefix = string.Empty; @@ -100,7 +100,7 @@ private static IEnumerable AzureEnvToAppEnv(DictionaryEntry ent { // This prevents the prefix from being normalized. // We can also do a fast path branch, I guess? No point in reallocating if the prefix is empty. - entry.Key = !string.IsNullOrEmpty(envPrefix) + entry.Key = envPrefix != string.Empty ? envPrefix + NormalizeKey(key.Substring(envPrefix.Length)) : NormalizeKey(key); From ffac54bfb55f175a0d670793952547e245d0a75f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Brawa=C5=84ski?= Date: Sun, 20 Dec 2020 23:26:20 +0100 Subject: [PATCH 3/6] Remove LINQ per #44923 --- ...vironmentVariablesConfigurationProvider.cs | 132 ++++++++---------- .../tests/EnvironmentVariablesTest.cs | 28 +++- 2 files changed, 77 insertions(+), 83 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs index 35129bc1dcd5c..96aca8442f8aa 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs @@ -4,7 +4,6 @@ using System; using System.Collections; using System.Collections.Generic; -using System.Linq; namespace Microsoft.Extensions.Configuration.EnvironmentVariables { @@ -18,112 +17,91 @@ public class EnvironmentVariablesConfigurationProvider : ConfigurationProvider private const string SqlServerPrefix = "SQLCONNSTR_"; private const string CustomPrefix = "CUSTOMCONNSTR_"; - private const string ConnStrKeyFormat = "ConnectionStrings:{0}"; - private const string ProviderKeyFormat = "ConnectionStrings:{0}_ProviderName"; - private readonly string _prefix; /// /// Initializes a new instance. /// - public EnvironmentVariablesConfigurationProvider() : this(string.Empty) - { } + public EnvironmentVariablesConfigurationProvider() => + _prefix = string.Empty; /// /// Initializes a new instance with the specified prefix. /// /// A prefix used to filter the environment variables. - public EnvironmentVariablesConfigurationProvider(string prefix) - { + public EnvironmentVariablesConfigurationProvider(string prefix) => _prefix = prefix ?? string.Empty; - } /// /// Loads the environment variables. /// - public override void Load() - { + public override void Load() => Load(Environment.GetEnvironmentVariables()); - } internal void Load(IDictionary envVariables) { var data = new Dictionary(StringComparer.OrdinalIgnoreCase); - IEnumerable filteredEnvVariables = envVariables - .Cast() - .SelectMany(x => AzureEnvToAppEnv(x, _prefix)); - - foreach (DictionaryEntry envVariable in filteredEnvVariables) + foreach (DictionaryEntry entry in envVariables) { - string key = envVariable.Key as string; - if (key.StartsWith(_prefix, StringComparison.OrdinalIgnoreCase)) - key = key.Substring(_prefix.Length); - - data[key] = (string)envVariable.Value; + string key = entry.Key as string; + string provider = null; + string prefix; + + if (key.StartsWith(MySqlServerPrefix, StringComparison.OrdinalIgnoreCase)) + { + prefix = MySqlServerPrefix; + provider = "MySql.Data.MySqlClient"; + } + else if (key.StartsWith(SqlAzureServerPrefix, StringComparison.OrdinalIgnoreCase)) + { + prefix = SqlAzureServerPrefix; + provider = "System.Data.SqlClient"; + } + else if (key.StartsWith(SqlServerPrefix, StringComparison.OrdinalIgnoreCase)) + { + prefix = SqlServerPrefix; + provider = "System.Data.SqlClient"; + } + else if (key.StartsWith(CustomPrefix, StringComparison.OrdinalIgnoreCase)) + { + prefix = CustomPrefix; + } + else if (key.StartsWith(_prefix, StringComparison.OrdinalIgnoreCase)) + { + // This prevents the prefix from being normalized. + // We can also do a fast path branch, I guess? No point in reallocating if the prefix is empty. + key = NormalizeKey(key.Substring(_prefix.Length)); + data[key] = entry.Value as string; + + continue; + } + else + { + continue; + } + + // Add the key-value pair for connection string, and optionally provider name + key = NormalizeKey(key.Substring(prefix.Length)); + AddIfPrefixed(data, $"ConnectionStrings:{key}", (string)entry.Value); + if (provider != null) + { + AddIfPrefixed(data, $"ConnectionStrings:{key}_ProviderName", provider); + } } Data = data; } - private static string NormalizeKey(string key) + private void AddIfPrefixed(Dictionary data, string key, string value) { - return key.Replace("__", ConfigurationPath.KeyDelimiter); - } - - private static IEnumerable AzureEnvToAppEnv(DictionaryEntry entry, string envPrefix) - { - string key = (string)entry.Key; - string prefix = string.Empty; - string provider = string.Empty; - - if (key.StartsWith(MySqlServerPrefix, StringComparison.OrdinalIgnoreCase)) + if (key.StartsWith(_prefix, StringComparison.OrdinalIgnoreCase)) { - prefix = MySqlServerPrefix; - provider = "MySql.Data.MySqlClient"; - } - else if (key.StartsWith(SqlAzureServerPrefix, StringComparison.OrdinalIgnoreCase)) - { - prefix = SqlAzureServerPrefix; - provider = "System.Data.SqlClient"; - } - else if (key.StartsWith(SqlServerPrefix, StringComparison.OrdinalIgnoreCase)) - { - prefix = SqlServerPrefix; - provider = "System.Data.SqlClient"; - } - else if (key.StartsWith(CustomPrefix, StringComparison.OrdinalIgnoreCase)) - { - prefix = CustomPrefix; - } - else if (key.StartsWith(envPrefix, StringComparison.OrdinalIgnoreCase)) - { - // This prevents the prefix from being normalized. - // We can also do a fast path branch, I guess? No point in reallocating if the prefix is empty. - entry.Key = envPrefix != string.Empty - ? envPrefix + NormalizeKey(key.Substring(envPrefix.Length)) - : NormalizeKey(key); - - yield return entry; - yield break; - } - else - { - yield break; - } - - // Return the key-value pair for connection string - yield return new DictionaryEntry( - string.Format(ConnStrKeyFormat, NormalizeKey(key.Substring(prefix.Length))), - entry.Value); - - if (!string.IsNullOrEmpty(provider)) - { - // Return the key-value pair for provider name - yield return new DictionaryEntry( - string.Format(ProviderKeyFormat, NormalizeKey(key.Substring(prefix.Length))), - provider); + key = key.Substring(_prefix.Length); + data[key] = value; } } + + private static string NormalizeKey(string key) => key.Replace("__", ConfigurationPath.KeyDelimiter); } } diff --git a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs index b3f7ca99486d8..49cc7ca64bab3 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs @@ -143,14 +143,14 @@ public void ReplaceDoubleUnderscoreInEnvironmentVariables() var dict = new Hashtable() { {"data__ConnectionString", "connection"}, - {"SQLCONNSTR__db1", "connStr"} + {"SQLCONNSTR_db1", "connStr"} }; var envConfigSrc = new EnvironmentVariablesConfigurationProvider(); envConfigSrc.Load(dict); Assert.Equal("connection", envConfigSrc.Get("data:ConnectionString")); - Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:_db1_ProviderName")); + Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:db1_ProviderName")); } [Fact] @@ -159,14 +159,30 @@ public void ReplaceDoubleUnderscoreInEnvironmentVariablesButNotPrefix() var dict = new Hashtable() { {"test__prefix__with__double__underscores__data__ConnectionString", "connection"}, - {"SQLCONNSTR__db1", "connStr"} + {"SQLCONNSTR_db1", "connStr"} }; var envConfigSrc = new EnvironmentVariablesConfigurationProvider("test__prefix__with__double__underscores__"); envConfigSrc.Load(dict); Assert.Equal("connection", envConfigSrc.Get("data:ConnectionString")); - Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:_db1_ProviderName")); + Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:db1_ProviderName")); + } + + [Fact] + public void ReplaceDoubleUnderscoreInEnvironmentVariablesButNotInAnomalousPrefix() + { + var dict = new Hashtable() + { + {"_____EXPERIMENTAL__data__ConnectionString", "connection"}, + {"SQLCONNSTR_db1", "connStr"} + }; + var envConfigSrc = new EnvironmentVariablesConfigurationProvider("_____EXPERIMENTAL__"); + + envConfigSrc.Load(dict); + + Assert.Equal("connection", envConfigSrc.Get("data:ConnectionString")); + Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:db1_ProviderName")); } [Fact] @@ -175,14 +191,14 @@ public void ReplaceDoubleUnderscoreInEnvironmentVariablesWithDuplicatedPrefix() var dict = new Hashtable() { {"test__test__ConnectionString", "connection"}, - {"SQLCONNSTR__db1", "connStr"} + {"SQLCONNSTR_db1", "connStr"} }; var envConfigSrc = new EnvironmentVariablesConfigurationProvider("test__"); envConfigSrc.Load(dict); Assert.Equal("connection", envConfigSrc.Get("test:ConnectionString")); - Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:_db1_ProviderName")); + Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:db1_ProviderName")); } [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] From e5de56cd0eddfb97272718155d56810a8a1f51f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Brawa=C5=84ski?= Date: Sun, 20 Dec 2020 23:42:30 +0100 Subject: [PATCH 4/6] Fix bad tests resulting from misunderstanding how loader is supposed to work. --- .../tests/EnvironmentVariablesTest.cs | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs index 49cc7ca64bab3..d23fe8faf3d1a 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/tests/EnvironmentVariablesTest.cs @@ -158,15 +158,13 @@ public void ReplaceDoubleUnderscoreInEnvironmentVariablesButNotPrefix() { var dict = new Hashtable() { - {"test__prefix__with__double__underscores__data__ConnectionString", "connection"}, - {"SQLCONNSTR_db1", "connStr"} + {"test__prefix__with__double__underscores__data__ConnectionString", "connection"} }; var envConfigSrc = new EnvironmentVariablesConfigurationProvider("test__prefix__with__double__underscores__"); envConfigSrc.Load(dict); Assert.Equal("connection", envConfigSrc.Get("data:ConnectionString")); - Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:db1_ProviderName")); } [Fact] @@ -174,19 +172,31 @@ public void ReplaceDoubleUnderscoreInEnvironmentVariablesButNotInAnomalousPrefix { var dict = new Hashtable() { - {"_____EXPERIMENTAL__data__ConnectionString", "connection"}, - {"SQLCONNSTR_db1", "connStr"} + {"_____EXPERIMENTAL__data__ConnectionString", "connection"} }; var envConfigSrc = new EnvironmentVariablesConfigurationProvider("_____EXPERIMENTAL__"); envConfigSrc.Load(dict); Assert.Equal("connection", envConfigSrc.Get("data:ConnectionString")); - Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:db1_ProviderName")); } [Fact] public void ReplaceDoubleUnderscoreInEnvironmentVariablesWithDuplicatedPrefix() + { + var dict = new Hashtable() + { + {"test__test__ConnectionString", "connection"} + }; + var envConfigSrc = new EnvironmentVariablesConfigurationProvider("test__"); + + envConfigSrc.Load(dict); + + Assert.Equal("connection", envConfigSrc.Get("test:ConnectionString")); + } + + [Fact] + public void PrefixPreventsLoadingSqlConnectionStrings() { var dict = new Hashtable() { @@ -198,7 +208,7 @@ public void ReplaceDoubleUnderscoreInEnvironmentVariablesWithDuplicatedPrefix() envConfigSrc.Load(dict); Assert.Equal("connection", envConfigSrc.Get("test:ConnectionString")); - Assert.Equal("System.Data.SqlClient", envConfigSrc.Get("ConnectionStrings:db1_ProviderName")); + Assert.Throws(() => envConfigSrc.Get("ConnectionStrings:db1_ProviderName")); } [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))] From 2152e25eb335f2f664969b4c5d6f357f393d22bc Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 20 Nov 2020 18:57:40 -0500 Subject: [PATCH 5/6] Fixed merge conflicts From fb915c423ac8078628b25aecd29325b9242c0e42 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Fri, 20 Nov 2020 18:57:40 -0500 Subject: [PATCH 6/6] Actually merge #44923 and fix conflicts. --- .../src/EnvironmentVariablesConfigurationProvider.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs index 96aca8442f8aa..46fa51b60e9c4 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs @@ -44,7 +44,7 @@ internal void Load(IDictionary envVariables) foreach (DictionaryEntry entry in envVariables) { - string key = entry.Key as string; + string key = (string)entry.Key; string provider = null; string prefix;