Support environment variables in the settings #15082

ianjoneill · 2023-04-01T18:43:00Z

Existing environment variables can be referenced by enclosing the name in percent characters (e.g. %PATH%).

Tests added and manually tested.

Closes #2785
Closes #9233

Co-authored-by: Chris Tapley chris.tapley.81@gmail.com

* use StringMap as a container * recursively resolve ${env:NAME} vars in values in the StringMap or in the process' environment * implement some tests

* add comments to ResolveEnvironmentVariableValue * use wil helper * remove unused code

* allow appending or prepending process environment variables * don't handle all exceptions on validation failure

* Use INHERITABLE_SETTING macro

zadjii-msft · 2023-04-01T19:18:28Z

holy shit the madman actually did it

ianjoneill · 2023-04-01T21:11:54Z

Test failures are CI flakes

zadjii-msft · 2023-04-03T14:45:01Z

idle thought before reading the code: the original PR might predate layering. Lets see how this works with layering. Can the defaults say (for example) TERM=windows-terminal, and then a profile say TERM=, to unset it? Or does setting it in a profile entirely replace the lower layer? If it's a per-variable layering, we could use that to vaguely do #11057.

ianjoneill · 2023-04-03T15:49:28Z

Currently the concrete profile's environment completely replaces the one specified in defaults.

So if you've got:

"defaults": {
  "environment": {
    "Foo": "Bar"
  }
},
"list": [
  {
    "name": "PowerShell",
    "environment": {
      "Baz": "Qux"
    }
  }
]

Your PowerShell profile will get Baz=Qux and your cmd profile will get Foo=Bar.

zadjii-msft · 2023-04-04T14:04:24Z

Some discussion notes: We're all happy with it being non-layering for now. Maybe in the future we can re-evaluate that, but this is much better than what we had before.

zadjii-msft · 2023-04-04T14:10:42Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

+ til::env environment;
 auto zeroEnvMap = wil::scope_exit([&]() noexcept {
 // Can't zero the keys, but at least we can zero the values.
- for (auto& [name, value] : environment)
+ for (auto& [name, value] : environment.as_map())
 {
 ::SecureZeroMemory(value.data(), value.size() * sizeof(decltype(value.begin())::value_type));
 }

- environment.clear();
+ environment.as_map().clear();
 });


Should til::env just do this clearing as a part of regenerate? Is this even needed anymore now that it's just a til::env?

Unless I'm missing something, if we did this as part of regenerate() the map would be cleared on exit of the method which would mean the map would be populated and immediately cleared.

It would probably make sense to extract this for loop into a clear() method on til::env, so that the caller doesn't need to worry about the internal representation.

With regards to whether it's necessary, I can't answer that - I don't know why the code originally did this. It's possible the environment could contain sensitive information (e.g. temporary credentials), which you may want to clear out.

I've refactored this into a clear() method in til::env.

DHowett

bunch of notes. thanks for bringing this back from the dead @ianjoneill :)

DHowett · 2023-04-04T17:42:18Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -288,6 +289,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 }
 _reloadEnvironmentVariables = winrt::unbox_value_or<bool>(settings.TryLookup(L"reloadEnvironmentVariables").try_as<Windows::Foundation::IPropertyValue>(),
 _reloadEnvironmentVariables);
+ _profileGuid = winrt::unbox_value_or<winrt::guid>(settings.TryLookup(L"profileGuid").try_as<Windows::Foundation::IPropertyValue>(), _profileGuid);


in an ideal world (which is not the world in which we live), the Connection classes would not have any concept of a "profile". Even so, I'm cool with this since we've committed so many other layering violations in the past five years.

you don't need to do anything about this, i just felt like I had a duty to say it

Ah - it seemed silly both this and TerminalPage doing the same dance fiddling with WSLENV though.

Hmmmm... now that we have the WSLENV dance happening over all provided environment variables in ConptyConnection, do you think that we could get away with moving profile GUID up to TerminalPage?

It could become annoying because now we need to copy the user's provided environment map (if there is one! but we do have to copy it because we don't want to edit it inplace since the one living on the profile is durable)... but after all, if TPage provides WT_PROFILE_ID via the environment map it will automatically show up in WSLENV

I think that depends on if we think people will try and override that environment variable.

The keys in the IMap are case sensitive - so if someone has added say wt_profile_id as an environment variable, that'll be stored separately - so back in ConptyConnection one of the two values will be put into the connection's environment.

Fair!
We can fix the interface later.

One potential solution would be to pass two environment variable maps to ConptyConnection - one containing user variables and one containing "terminal" variables. We'd insert the "terminal" ones into the environment first, and then the user ones. I can implement that as a follow-up if you're happy with that approach (and please say if you have a better name for "terminal" variables too!).

src/cascadia/TerminalSettingsModel/JsonUtils.h

src/cascadia/TerminalSettingsModel/Profile.idl

src/cascadia/TerminalSettingsModel/TerminalSettings.idl

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

src/cascadia/TerminalConnection/ConptyConnection.cpp

ianjoneill

Some thoughts before I open Visual Studio

ianjoneill · 2023-04-04T19:55:24Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

+ til::env environment;
 auto zeroEnvMap = wil::scope_exit([&]() noexcept {
 // Can't zero the keys, but at least we can zero the values.
- for (auto& [name, value] : environment)
+ for (auto& [name, value] : environment.as_map())
 {
 ::SecureZeroMemory(value.data(), value.size() * sizeof(decltype(value.begin())::value_type));
 }

- environment.clear();
+ environment.as_map().clear();
 });


Unless I'm missing something, if we did this as part of regenerate() the map would be cleared on exit of the method which would mean the map would be populated and immediately cleared.

It would probably make sense to extract this for loop into a clear() method on til::env, so that the caller doesn't need to worry about the internal representation.

With regards to whether it's necessary, I can't answer that - I don't know why the code originally did this. It's possible the environment could contain sensitive information (e.g. temporary credentials), which you may want to clear out.

src/cascadia/TerminalConnection/ConptyConnection.cpp

ianjoneill · 2023-04-04T19:59:36Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -288,6 +289,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 }
 _reloadEnvironmentVariables = winrt::unbox_value_or<bool>(settings.TryLookup(L"reloadEnvironmentVariables").try_as<Windows::Foundation::IPropertyValue>(),
 _reloadEnvironmentVariables);
+ _profileGuid = winrt::unbox_value_or<winrt::guid>(settings.TryLookup(L"profileGuid").try_as<Windows::Foundation::IPropertyValue>(), _profileGuid);


Ah - it seemed silly both this and TerminalPage doing the same dance fiddling with WSLENV though.

ianjoneill · 2023-04-06T20:44:31Z

Just FYI I should be able to take a look at this tomorrow (Friday).

DHowett · 2023-04-07T15:30:26Z

This is very subtle (and I am only testing as of >5 hours ago, so please ignore if changed), but you can resurrect inherited environment variables depending on sort order.

I don't know if I think this is an issue worth fixing, but it is fun!

"environment": {
    "Z_NEW_PATH": "%PATH%", // gets new path!
    "PATH": "A",
    "0_OLD_PATH": "%PATH%", // gets old path!
}

ianjoneill · 2023-04-07T18:15:51Z

This is seemingly how Windows resolves environment variables - though the order as far as I can tell is the order they're inserted into the registry.

Starting off with a system environment variable FOO=Foo and setting up the registry with REG_EXPAND_SZ user variables 0_FOO=%FOO%, FOO=NewFoo and Z_FOO=%FOO%, as below:

Leads to them being resolved as 0_FOO=Foo, FOO=NewFoo and Z_FOO=NewFoo in a conhost window, as below:

Worth pointing out though that if you create the FOO user variable in the system properties window, it's set up as a REG_SZ variable - so it does get resolved first, leading to 0_FOO and Z_FOO both resolving to NewFoo.

I guess we could put in a pre-processing step that inserts variables with values not containing percent characters into the environment map first. It would require iterating over the values in the map twice though and would complicate the logic somewhat - your call.

DHowett · 2023-04-07T18:47:19Z

Oh, no, I'm totally happy with how it works today! I legitimately found it interesting 😄

DHowett

I love it. Thanks @ianjoneill and @christapley :)

christapley and others added 18 commits February 25, 2021 21:18

implement env vars in settings (microsoft#2785)

b0c426b

* use StringMap as a container * recursively resolve ${env:NAME} vars in values in the StringMap or in the process' environment * implement some tests

Invoke-CodeFormat

5036cb0

Code review feedback

b4efbb8

* add comments to ResolveEnvironmentVariableValue * use wil helper * remove unused code

Code review update (KalleOlaviNiemitalo)

5d68c94

* allow appending or prepending process environment variables * don't handle all exceptions on validation failure

Fix spelling mistake

ea56d64

Merge branch 'main' into feature/2785-profile-env-vars-with-string-map

9ec2218

fix build (microsoft#2785)

f1c1cc4

* Use INHERITABLE_SETTING macro

fix code formatting (microsoft#2785)

0b6f411

Run unix2dos on TerminalSettings.h and Profile.h

46a9b35

Merge branch 'main' into feature/2785-profile-env-vars-with-string-map

ad11e67

It compiles

95cacbc

It works

3d2e3fd

Add warning about environment variables with different cases

e4c9cbb

Tests

fb7ea2d

Remove refactored methods

12c1939

Format

b8165ac

Tidy

92d2351

Schema

c60e2e5

ianjoneill mentioned this pull request Apr 1, 2023

implement env vars in settings (#2785) #9287

Closed

6 tasks

Don't clobber the WSLENV environment variable

3c8554f

ianjoneill mentioned this pull request Apr 1, 2023

Order of environment variables different in WT #9233

Closed

zadjii-msft added the Needs-Discussion Something that requires a team discussion before we can proceed label Apr 3, 2023

microsoft-github-policy-service bot added the Priority-3 A description (P3) label Apr 3, 2023

zadjii-msft approved these changes Apr 4, 2023

View reviewed changes

DHowett requested changes Apr 4, 2023

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 4, 2023

ianjoneill commented Apr 4, 2023

View reviewed changes

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 4, 2023

zadjii-msft added this to the Terminal v1.18 milestone Apr 4, 2023

zadjii-msft self-assigned this Apr 6, 2023

DHowett added a commit that referenced this pull request Apr 6, 2023

PRE-MERGE #15082 Support environment variables in the settings

5af2721

ianjoneill added 7 commits April 7, 2023 11:07

Move til::details::wstring_case_insensitive_compare

f1a538c

Make clear() a function of til::env

460be47

Helpfully update WSLENV

722f34e

Address JSON comments

e7e3f59

Switch to IMap

de5b246

Fix tests

7231f0c

Audit mode

0bfe19e

DHowett added a commit that referenced this pull request Apr 11, 2023

PRE-MERGE #15082 Support environment variables in the settings

90122f8

zadjii-msft removed the Needs-Discussion Something that requires a team discussion before we can proceed label Apr 11, 2023

zadjii-msft removed their assignment Apr 11, 2023

DHowett approved these changes Apr 11, 2023

View reviewed changes

DHowett merged commit 56d451d into microsoft:main Apr 11, 2023

ianjoneill mentioned this pull request Apr 12, 2023

Settings error/warning dialog seems to self-dismiss on startup #15170

Closed

zadjii-msft mentioned this pull request Sep 20, 2023

wt.exe should accept env vars on the commandline #16008

Open

2 tasks

This was referenced Jun 6, 2024

We forgot to add profile.environment to the docs MicrosoftDocs/terminal#781

Open

Support custom environment. #17379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support environment variables in the settings #15082

Support environment variables in the settings #15082

ianjoneill commented Apr 1, 2023 •

edited by DHowett

Loading

zadjii-msft commented Apr 1, 2023

ianjoneill commented Apr 1, 2023

zadjii-msft commented Apr 3, 2023

ianjoneill commented Apr 3, 2023

zadjii-msft commented Apr 4, 2023

zadjii-msft Apr 4, 2023

ianjoneill Apr 4, 2023

ianjoneill Apr 7, 2023

DHowett left a comment

DHowett Apr 4, 2023

DHowett Apr 4, 2023

ianjoneill Apr 4, 2023

DHowett Apr 10, 2023

ianjoneill Apr 11, 2023

DHowett Apr 11, 2023

ianjoneill Apr 12, 2023

ianjoneill left a comment

ianjoneill Apr 4, 2023

ianjoneill Apr 4, 2023

ianjoneill commented Apr 6, 2023

DHowett commented Apr 7, 2023

ianjoneill commented Apr 7, 2023

DHowett commented Apr 7, 2023

DHowett left a comment

Support environment variables in the settings #15082

Support environment variables in the settings #15082

Conversation

ianjoneill commented Apr 1, 2023 • edited by DHowett Loading

zadjii-msft commented Apr 1, 2023

ianjoneill commented Apr 1, 2023

zadjii-msft commented Apr 3, 2023

ianjoneill commented Apr 3, 2023

zadjii-msft commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianjoneill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianjoneill commented Apr 6, 2023

DHowett commented Apr 7, 2023

ianjoneill commented Apr 7, 2023

DHowett commented Apr 7, 2023

DHowett left a comment

Choose a reason for hiding this comment

ianjoneill commented Apr 1, 2023 •

edited by DHowett

Loading