Breaking change in environment variable configuration prefix support in dotnet 6

[matt-richardson]

Description

When using environment variable configuration, the prefix is no longer normalized like it was in dotnet 5.0.

As the environment variable prefix should contain : if on Windows, and __ if on Linux, then normalizing the prefix makes sense to me. Otherwise, I have to duplicate the OS detection/normalization code myself.

Reproduction Steps

1. Create a new console app
2. Add references to Microsoft.Extensions.Configuration, Microsoft.Extensions.Configuration.Binder and Microsoft.Extensions.Configuration.EnvironmentVariables
3. Add the following code

using System;
using Microsoft.Extensions.Configuration;

namespace ConsoleApp1
{
    public class Program
    {
        public static void Main(string[] args)
        {
            var environmentOverridesPrefix = typeof(Program).Namespace!.Replace(".", ":") + ":";

            var configuration = new ConfigurationBuilder()
                .AddEnvironmentVariables(environmentOverridesPrefix)
                .Build();

            var appSettings = new AppSettings();
            configuration.Bind(appSettings);

            Console.WriteLine(appSettings.Foo ?? "<null>");
        }
    }

    public class AppSettings
    {
        public string? Foo { get; set; }
    }
}

4. Set an environment variable ConsoleApp1__Foo=42

Expected behavior

It should print 42

Actual behavior

It prints <null>

Regression?

This works in dotnet 5.0.

Known Workarounds

Changing the prefix to use __ instead of : works:

eg, if I change the line
var environmentOverridesPrefix = typeof(Program).Namespace!.Replace(".", ":") + ":";
to
var environmentOverridesPrefix = typeof(Program).Namespace!.Replace(".", "__") + "__";
it works

Configuration

.NET version: 6.0.100
OS: Windows 11, 21H2 (OS Build 22000.346)
Arch: x64
Platform specific: no, we were facing it on linux as well

Other information

It appears that 9a322a5 changed the behavior as part of #42932 while trying to fix #40911.

[ghost]

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

When using environment variable configuration, the prefix is no longer normalized like it was in dotnet 5.0.

As the environment variable prefix should contain : if on Windows, and __ if on Linux, then normalizing the prefix makes sense to me. Otherwise, I have to duplicate the OS detection/normalization code myself.

Reproduction Steps

1. Create a new console app
2. Add references to Microsoft.Extensions.Configuration, Microsoft.Extensions.Configuration.Binder and Microsoft.Extensions.Configuration.EnvironmentVariables
3. Add the following code

using System;
using Microsoft.Extensions.Configuration;

namespace ConsoleApp1
{
    public class Program
    {
        public static void Main(string[] args)
        {
            var environmentOverridesPrefix = typeof(Program).Namespace!.Replace(".", ":") + ":";

            var configuration = new ConfigurationBuilder()
                .AddEnvironmentVariables(environmentOverridesPrefix)
                .Build();

            var appSettings = new AppSettings();
            configuration.Bind(appSettings);

            Console.WriteLine(appSettings.Foo ?? "<null>");
        }
    }

    public class AppSettings
    {
        public string? Foo { get; set; }
    }
}

4. Set an environment variable ConsoleApp1__Foo=42

Expected behavior

It should print 42

Actual behavior

It prints <null>

Regression?

This works in dotnet 5.0.

Known Workarounds

Changing the prefix to use __ instead of : works:

eg, if I change the line
var environmentOverridesPrefix = typeof(Program).Namespace!.Replace(".", ":") + ":";
to
var environmentOverridesPrefix = typeof(Program).Namespace!.Replace(".", "__") + "__";
it works

Configuration

.NET version: 6.0.100
OS: Windows 11, 21H2 (OS Build 22000.346)
Arch: x64
Platform specific: no, we were facing it on linux as well

Other information

It appears that 9a322a5 changed the behavior as part of #42932 while trying to fix #40911.

Author:	matt-richardson
Assignees:	-
Labels:	untriaged, area-Extensions-Configuration
Milestone:	-

[kasperk81]

-                    {"SQLCONNSTR__db1", "connStr"}
+                    {"SQLCONNSTR_db1", "connStr"}

existing test was changed in #42932 but there is no indication of this breaking behavior in https://docs.microsoft.com/en-us/dotnet/core/compatibility/6.0. new behavior ("what you see is what you get" without special case) makes more sense, but should be mentioned in compatibility docs.

[maryamariyan]

cc @Emzi0767 as PR author of #42932

[Emzi0767]

When I originally introduced the PR (around core 3.x, prior to 5.0 release), the prefix normalization wasn't documented (or at least not in a place where it should've been). I thusly considered this a bug. When I raised the issue, there was no objection to that statement. Since documentation wasn't clear on that subject, I considered the prefix normalization to be a bug. I therefore changed the behaviour to normalize variables only.

The PR was stuck in review for close to 4 months, and ultimately got a green light, which leads me to believe my interpretation was correct.

I don't mean this as an attack or anything of this sort, I'm simply stating my perspective. If I indeed caused a regression - I apologize.

Regarding #61577 (comment): that is an accidental mistake; while my memory doesn't go this far, I don't think it was my intention to modify tests. I've no idea why this particular test was changed.

[kasperk81]

as a user, new EnvironmentVariablesConfigurationProvider(prefix) does not indicate anything about one or more underscores being special characters. it can be any string allowed by the operating system.

in .net 5 there was a different kind of special casing, in .net 6 it's still special casing (breaking .net 5 guarantees).

if it's my call, i'd remove all special casing and magic from this type and keep it simple. that kind of breaking change would be well received.

[Emzi0767]

if it's my call, i'd remove all special casing and magic from this type and keep it simple.

Isn't that (almost) what we have right now? Prefix has no indication for special characters, so it's taken literally. Previously, processing did normalize -> filter + strip -> process. My PR inverted the first 2 steps; now it's filter + strip (literal) -> normalize -> process. As for stripped variable names, Configuration uses : as hierarchical separator, so it makes sense to normalize those bits at least.

The gist of the PR was that if you had an envvar like A__B__C=D, and set prefix to A__, provider would first normalize A__B__C to A:B:C, then filter it out as it does not begin with A__. Under new behaviour, A__B__C is accepted due to prefix of A__, then stripped so it becomes B__C, then normalized to B:C.

Under previous behaviour, if you used consistent envvar naming across platforms (A__B__C on both Windows and Unix), you'd need to set your prefix to A:, rather than A__. The environment variables were treated like Configuration keys, and prefix thusly applied to Configuration keys rather than environment variables. This, I believe, was not clearly documented. Indeed, looking at the docstring now, it says: <param name="prefix">A prefix used to filter the environment variables.</param>. So the docstring suggests the prefix filtering is applied to envvars (the input), not Configuration items (the output), and makes no mention of normalization. Admittedly, while inconsistent on setup, old behaviour it was consistent on results. New one inverts this.

The documentation mentions normalization, but it does not explicitly say whether this includes the prefix or not; all that is mentioned about it is that it's stripped. You have a gap for interpretation: is the filtered input normalized, or is the normalized input filtered?

In short, I believe the issue stems from ambiguity. What needs to be decided is which interpretation is canonical.

[kasperk81]

yes, your pr is improvement as i mentioned above. but still if we don't canonicalize it (NormalizeKey() part), it would make the implementation and interpretation tad simpler. in case someone needs pre/post processing, a Func overload can help aid the use-case.

[maryamariyan]

cc @HaoK what is your take on this?

[HaoK]

My quick take is that it seems like a breaking change that should be reverted

[wrexbe]

I think while the chance is low, the potential damage is higher, and that this is a more severe breaking change, because it can be completely silent. I think someone is going to update, everything is going to appear fine, but when they deploy it to production it is going to not override some of the settings. The program could run for a long time, and cause a lot of damage, before anybody notices it's not doing what it's supposed to do.

[maryamariyan]

So we are looking to revert it back to old behavior, seems like since we didn't advertise the bugfix in breaking change docs it is unlikely to cause problem if we regress it back.

and to mitigate and reduce ambiguity it would also be worth updating tests or adding docs.

[maryamariyan]

reopening as this needs to be ported to release/6.0

[maryamariyan]

We are looking to definitely fix this regression in our next 6.0 servicing release (#62916) and plan on adding a breaking change notification to communicate the change as well.

The fix involves reverting the original bug fix for underlying issue #42932 as it seemed to be in conflict with existing behavior. The workaround would be to replace double underscore in prefix with :.

Wanted to also circle back with @Emzi0767 again as PR author of #42932, to make sure this decision would not block you.

[Emzi0767]

There's nothing on my end. I think that an explicit note documenting this should be added to documentation somewhere however, so we can avoid future confusion and problems.

[matt-richardson]

It appears this has been fixed in 6.0.102?

[maryamariyan]

It appears this has been fixed in 6.0.102?

Here's the PR that got serviced in 6.0.2: #62916
Available in this list: https://github.com/dotnet/runtime/commits/v6.0.2?after=839cdfb0ecca5e0be3dbccd926e7651ef50fdf10+34&branch=v6.0.2

[dhedey]

EDIT: I missed this post on my earlier run through which explains this breaking change was intended: #61577 (comment) but I still wish to suggest that I believe all issues can be resolved satisfactorily.

---

As mentioned in the above comment, this 6.0.2 fix also has a hidden breaking change (bug?) from 6.0.0 and 6.0.1, and has broken a number of our production builds.

I'll document my findings and suggested workarounds / fixes below in more detail for anyone else suffering from this problem.

Summary

Prefixes which happen to contain __ defined via AddEnvironmentVariables no longer match with environment variables starting with the same prefix.

EG configuration which is set up with new ConfigurationBuilder().AddEnvironmentVariables("MY_PREFIX__") no longer finds environment variables such as MY_PREFIX__MyConfigKey

Background

We automatically pulled in the 6.0.2 docker image patch release via the default 6.0 tag. 6.0.2 is a security patch fixing CVE-2022-21986 - it's unfortunate having a breaking change in a patch version, but these things happen.

Details

In particular, the following now fails after pulling in a 6.0.2 patch release of the dotnet runtime:

                Environment.SetEnvironmentVariable("MY_PREFIX__ConfigKey, "myFooValue");
                var configuration = new ConfigurationBuilder()
                    .AddEnvironmentVariables("MY_PREFIX__")
                    .Build();

                var loadedConfig = configuration.GetValue<string?>("ConfigKey", null);

                Assert.Equals("myFooValue", loadedConfig); // FAILS in 6.0.2 as loadedConfig is null

There is nothing in the documentation to suggest __ should not be used in the prefix (or that : should be used instead) - this feels like it's an implementation detail leaking out unless I care about normalization.

Side note - I'm not quite sure what this test means by "Won't Normalise" in terms of expected behaviour - it seems to be asserting that this bug occurs, but maybe I'm missing an intended behaviour?

Proposed changes

I'd like to propose that, the following changes are made:

• The prefix is also created in normalised form and used for the prefix comparison - so that the comparison becomes normalizedKey.StartsWith(_normalizedPrefix, StringComparison.OrdinalIgnoreCase). This will mean that the prefix will be transparently matched as per the docs, normalisation doesn't need to be considered (unless you expect it) - and the above code works correctly again. Although this will need to be carefully implemented so as to not raise a regression here: Environment Variable configuration provider doesn't work if the prefix has 2 underscores #40911 - OR an alternative implementation would filter unnormalized key against unnormalised prefix before normalizing.
• This normalisation is documented so people aren't confused by magic:
  • Why AddEnvironmentVariables("MY_PREFIX:") picks up environment variables starting MY_PREFIX__
  • (After the proposed change) Why AddEnvironmentVariables("MY_PREFIX__") picks up environment variables starting MY_PREFIX:

Proposed workaround

In the AddEnvironmentVariables line, keep both the key with the __ (for running on 6.0.0 and 6.0.1) and add a new line with __ replaced by : (for running on 6.0.2).

This will enable you updating the app to work with existing configuration:

.AddEnvironmentVariables("MY_PREFIX__") 
.AddEnvironmentVariables("MY_PREFIX:") // Remove once https://github.com/dotnet/runtime/issues/61577#issuecomment-1044959384 is fixed

[Emzi0767]

Unfortunately this 6.0.2 fix also has a hidden breaking change (bug?) from 6.0.0 and 6.0.1, and has broken a number of our production builds.

These things happen! It's led to an afternoon of fun debugging, and a missed a release window - but I do enjoy an excuse for an afternoon of debugging, and let's be honest, I'll take any excuse not to release on a Friday 😄

I'll document my findings and suggested fixes below.

(@maryamariyan)

Summary

Prefixes which happen to contain __ defined via AddEnvironmentVariables no longer match with environment variables starting with the same prefix.

EG configuration which is set up with new ConfigurationBuilder().AddEnvironmentVariables("MY_PREFIX__") no longer finds environment variables such as MY_PREFIX__MyConfigKey

Background

We automatically pulled in the 6.0.2 docker image patch release via the default 6.0 tag. 6.0.2 is a security patch fixing CVE-2022-21986 - it's unfortunate having a breaking change in a patch version, but these things happen.

Details

In particular, the following now fails after pulling in a 6.0.2 patch release of the dotnet runtime:

                Environment.SetEnvironmentVariable("MY_PREFIX__ConfigKey, "myFooValue");
                var configuration = new ConfigurationBuilder()
                    .AddEnvironmentVariables("MY_PREFIX__")
                    .Build();

                var loadedConfig = configuration.GetValue<string?>("ConfigKey", null);

                Assert.Equals("myFooValue", loadedConfig); // FAILS in 6.0.2 as loadedConfig is null

There is nothing in the documentation to suggest __ should not be used in the prefix (or that : should be used instead) - this feels like it's an implementation detail leaking out unless I care about normalization.

Side note - I'm not quite sure what this test means by "Won't Normalise" in terms of expected behaviour - it seems to be asserting that this bug occurs, but maybe I'm missing an intended behaviour?

Proposed changes

I'd like to propose that, the following changes are made:

• The prefix is also created in normalised form and used for the prefix comparison - so that the comparison becomes normalizedKey.StartsWith(_normalizedPrefix, StringComparison.OrdinalIgnoreCase). This will mean that the prefix will be transparently matched as per the docs, normalisation doesn't need to be considered (unless you expect it) - and the above code works correctly again. Although this will need to be carefully implemented so as to not raise a regression here: Environment Variable configuration provider doesn't work if the prefix has 2 underscores #40911 - OR an alternative implementation would filter unnormalized key against unnormalised prefix before normalizing.
• This normalisation is documented so people aren't confused by magic:
  • Why AddEnvironmentVariables("MY_PREFIX:") picks up environment variables starting MY_PREFIX__
  • (After the proposed change) Why AddEnvironmentVariables("MY_PREFIX__") picks up environment variables starting MY_PREFIX:

Proposed workaround

In the AddEnvironmentVariables line, keep both the key with the __ (for running on 6.0.0 and 6.0.1) and add a new line with __ replaced by : (for running on 6.0.2).

This will enable you updating the app to work with existing configuration:

.AddEnvironmentVariables("MY_PREFIX__") 
.AddEnvironmentVariables("MY_PREFIX:") // Remove once https://github.com/dotnet/runtime/issues/61577#issuecomment-1044959384 is fixed

The behaviour you're describing is the original, intended, and undocumented behaviour. This revert fix reverted a change which addressed that. It's unlikely to be changed.

[dhedey]

The behaviour you're describing is the original, intended, and undocumented behaviour. This revert fix reverted a change which addressed that. It's unlikely to be changed.

I'm struggling a little to understand the motivation for this prefix not matching being intended behaviour? If it is, I feel it should be documented.

It appears all the various bug reports stem from things not matching when they should (because the code in its various formulations has ended up comparing a normalised prefix against an unnormalised key or vice versa, or compared an unnormalized prefix and key when a normalized comparison was required).

If the code ensured the comparison / filtering is either comparing normalized prefixes to normalized prefixes or unnormalized prefixes to unnormalized prefixes, I believe all bugs / confusing behaviours can be resolved simultaneously / satisfactorily.

I'm happy to stick in a PR to demonstrate this.

[dhedey]

For anyone else following this, I've moved this to the issue #65756 and put in the PR #65757 which I believe captures the intended change in #40911 without introducing the regression in #61577.

[maryamariyan]

FYI the breaking change doc related to this issue is getting added in dotnet/docs#31742