Environment variables modifications by extensions are not correctly propagated to integrated terminal

[xavth]

Bug Description

Context

Extensions can modify the environment variables of the integrated terminal using environmentVariableCollection. In particular an extension can append or prepend a value to an existing environment variable; most notably this allows extensions to add directories to the PATH in order to make relevant commands available in the terminal.

This feature was added to Theia in #8523.

Description

With the potential exception of environment variables overridden in the TerminalOptions passed when creating the terminal, the integrated terminal will inherit the environment variables of the parent process. However, once an extension appends or prepends a value to such an inherited environment variable, instead of concatenating the previous value and the appended or prepended value, the previous value is actually replaced by the value that was meant to be appended / prepended.

This is particularly problematic when the variable in question is the PATH: if the user happens to activate an extension that (innocently) modifies the PATH, every terminal opened from that point on will suddenly have its PATH reduced to the value added by the extension, meaning most commands that were previously available will not work anymore.

I believe this issue does not happen when the environment variable is set in TerminalOptions.

Steps to Reproduce

I use the demo extension https://lab.nexedi.com/jerome/demo-environmentvariablecollection to demonstrate this bug in Gitpod:

1. Build Theia from master branch

build_theia_from_master.mp4

2. Start Theia with a specific environment variable (SOME_VARIABLE in the example):

$ SOME_VARIABLE=path/number/one:path/number/two:path/number/three yarn theia start

3. Open this Theia in a new browser tab and check that the environment variable has the expected value:

$ echo $SOME_VARIABLE
path/number/one:path/number/two:path/number/three

start_theia_and_check_environment_variable.mp4

4. Download the demo extension https://lab.nexedi.com/jerome/demo-environmentvariablecollection; a pre-built vsix is made available in this note which shows this dowload-link: https://lab.nexedi.com/jerome/demo-environmentvariablecollection/uploads/9998512a5d90c48508dfc4c87eeda4ac/demo-environmentvariablecollection-0.0.1.vsix.

$ wget https://lab.nexedi.com/jerome/demo-environmentvariablecollection/uploads/9998512a5d90c48508dfc4c87eeda4ac/demo-environmentvariablecollection-0.0.1.vsix.

download_extension_vsix.mp4

5. Install the extension using the "Install from vsix" toolbar button.

install_demo_extension_from_vsix.mp4

6. Use the extension to append path/number/four to SOME_VARIABLE:

• go to View -> Find Command (Ctrl+Shift+P)
• select Demo environmentVariableCollection API
• select Append to terminal environment variable
• type SOME_VARIABLE and hit Enter
• type path/number/four and hit Enter

append_to_environment_variable_and_check_result.mp4

7. Open a new terminal and check the value of the environment variable:

$ echo $SOME_VARIABLE
path/number/four

The expected value would have been path/number/one:path/number/two:path/number/three:path/number/four.

Likely cause

Below is what I could figure out:

• The applyToProcessEnvironment method at https://github.com/eclipse-theia/theia/blob/v1.12.1/packages/terminal/src/node/base-terminal-server.ts#L244 is where the various environment variable modifications requested by extensions are applied to the environment map that will later be used to set the terminal environment.

• The initial environment map passed to applyToProcessEnvironment is just the map of overridden environment variables set in TerminalOptions, as can be seen here https://github.com/eclipse-theia/theia/blob/v1.12.1/packages/terminal/src/node/shell-terminal-server.ts#L39, meaning it's potentially just an empty map, and that it doesn't contain all the environment variables of the parent process.

• But in applyToProcessEnvironment on lines 255 and 258, if the environment variable modified by an extension is not in the initial map, then an the value is prepended or appended to an empty string. It should instead be be prepended or appended to the environment variable inherited from the parent process.

[alvsan09]

Hi @xavth,
Your description above is excellent, and the provided plugin really helps,
To continue the story, the provided variable customization is then applied to the process environment variables, see [1],
This could have pretty bad consequences if e.g. the PATH variable is being affected as seen in [2].

I am looking at a solution where the environment variables to be applied to the process get initialized before calling applyToProcessEnvironment.

1. https://github.com/eclipse-theia/theia/blob/master/packages/terminal/src/node/shell-process.ts#L52
2. commands don't work because of no PATH variable in docker images theia-ide/theia-apps#437