Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Environment Variable configuration provider doesn't work if the prefix has 2 underscores #40911

Closed
Emzi0767 opened this issue Aug 16, 2020 · 4 comments · Fixed by #42932
Closed

Comments

@Emzi0767
Copy link
Contributor

Description

Environment variable loader doesn't load variables if the prefix is set to a string with one or more double underscores.

The problem lies in the incorrect order of operations here:

IEnumerable<DictionaryEntry> filteredEnvVariables = envVariables
.Cast<DictionaryEntry>()
.SelectMany(AzureEnvToAppEnv)
.Where(entry => ((string)entry.Key).StartsWith(_prefix, StringComparison.OrdinalIgnoreCase));

The keys are first normalized (that is __ is replaced with separator; default :), then they are filtered. The operations should happen the other way around.

To reproduce the issue:

  1. Create a new ASP.NET Core application
  2. In Startup.cs, in the constructor, add the following code:
var cfg = new ConfigurationBuilder().AddEnvironmentVariables("TEST__").Build();
Console.WriteLine(cfg["Test1:Test2"] ?? "<null>");
  1. In project properties, under Debug, add the following environment variable: TEST__TEST1__TEST2 with a value of 42.
  2. Run the project.

Expected result

Console prints 42.

Actual result

Console prints <null>.

Configuration

This affected my application running on ASP.NET Core 3.1.7, running on Windows 10 Pro 2004 x64, as well as Alpine Linux 3.12.0 x64 (running in Docker).

Browsing source of the master branch reveals the buggy code remains, which suggests the bug is present in .NET 5 as well.

Regression?

Not as far as I can tell.

Other information

Tried on Core 3.1, the problem appears to be present in .NET 5 as well.

Currently, the problem can be mitigated by replacing all instances of __ in the prefix with : or whatever other separator was configured, if one was configured. Modifying line 1 from repro to .AddEnvironmentVariables("TEST:") will cause the program to print 42 instead of <null>.

This caveat is not documented, and therefore I consider it a bug.

The issue might not be trivial to fix, as I've noticed that several other prefixes are tested as well.

In my code, I decided to use __ for consistency (so that Test1:Test2 becomes TEST__TEST1__TEST2 rather than TEST_TEST1__TEST2).

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Aug 16, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Aug 17, 2020

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

@eerhardt
Copy link
Member

eerhardt commented Sep 2, 2020

Moving to 6.0 as this isn't a regression in 5.0, and isn't critical to fix for 5.0.

@am11
Copy link
Member

am11 commented Sep 3, 2020

Unless there is a reason to confine it to strictly one or two underscores, it would probably make sense if the solution be bit more versatile like handling more than two underscores and mixed cases like prefix: _____EXPERIMENTAL__ key: FOO.

Emzi0767 added a commit to Emzi0767/dotnet-runtime that referenced this issue Sep 30, 2020
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.
safern pushed a commit that referenced this issue Dec 23, 2020
…g environment variables from being parsed (#42932)

* 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.

* Account for the fact that envPrefix is never null.

* Remove LINQ per #44923

* Fix bad tests resulting from misunderstanding how loader is supposed to work.

* Fixed merge conflicts

* Actually merge #44923 and fix conflicts.

Co-authored-by: Stephen Toub <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Jan 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants