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

fix(env): Avoid automatic expansion of %% in env #5395

Merged
merged 11 commits into from
Feb 25, 2023

Conversation

WHYBBE
Copy link
Contributor

@WHYBBE WHYBBE commented Feb 20, 2023

Description

For example, the following command is used to obtain system environment variables.

(Get-Item -Path "HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager\Environment").GetValue(
    "Path",
    "",
    [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames)

The string obtained by it will not replace the %%.

Motivation and Context

Use the Registry to get the unexpanded environment variable string inspired by PowerShell/PowerShell#17518.
Fixes #5394
Related to #1813

How Has This Been Tested?

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

Use the Registry to get the unexpanded environment variable string inspired by PowerShell/PowerShell#17518
lib/core.ps1 Outdated Show resolved Hide resolved
On the one hand, it remains the same style as the `Get-Item`.
On the other hand, this modification is necessary due to the `REG_EXPAND_SZ` type.
@r15ch13
Copy link
Member

r15ch13 commented Feb 20, 2023

Probably fixes #5059 and with this implemented we should look into using dedicated %SCOOP_*% variables 😄

It should also be fixed in https://github.com/ScoopInstaller/Install as well

lib/core.ps1 Outdated Show resolved Hide resolved
@WHYBBE
Copy link
Contributor Author

WHYBBE commented Feb 20, 2023

Probably fixes #5059 and with this implemented we should look into using dedicated %SCOOP_*% variables 😄

It should also be fixed in https://github.com/ScoopInstaller/Install as well

Thank you for pointing out the related issue for me. I haven't found it after searching for a long time, so I specially raised a new issue.

I don't know what other unrelated functions my modification will affect. Perhaps more detailed testing is needed before it can be applied to ScoopInstaller/Install or used to implement %SCOOP_*%.

Use `OpenSubKey('Environment', $true)` to gain write access.
Do not expand environment names when getting env, use expand string when setting env.
@WHYBBE WHYBBE changed the title fix(scoop-update): Avoid % substitutions in env fix(env): Avoid % substitutions in env Feb 21, 2023
WHYBBE and others added 2 commits February 21, 2023 13:28
Try to get the value before getting the new type, if successful then get the type, otherwise the default type is `ExpandString`.
@rashil2000
Copy link
Member

LGTM!

Please update the changelog.

@WHYBBE WHYBBE changed the title fix(env): Avoid % substitutions in env fix(env): Avoid automatic expansion of %% in env Feb 22, 2023
@rashil2000
Copy link
Member

So we need this change in the Installer as well - https://github.com/ScoopInstaller/Install

Similar to Get-Env, we can create a function Set-Env and use it on https://github.com/ScoopInstaller/Install/blob/c99c2f1d53cf699d1a02af1d5ad0ec897c9b6120/install.ps1#L390

rashil2000
rashil2000 previously approved these changes Feb 22, 2023
@WHYBBE
Copy link
Contributor Author

WHYBBE commented Feb 22, 2023

So we need this change in the Installer as well - https://github.com/ScoopInstaller/Install

Similar to Get-Env, we can create a function Set-Env and use it on https://github.com/ScoopInstaller/Install/blob/c99c2f1d53cf699d1a02af1d5ad0ec897c9b6120/install.ps1#L390

Modified in ScoopInstaller/Install#44

lib/core.ps1 Outdated Show resolved Hide resolved
@r15ch13
Copy link
Member

r15ch13 commented Feb 23, 2023

If $val is $null then the variable should be removed.
Steps to reproduce:

  • scoop install coder
  • scoop uninstall coder

image

function env($name, $global, $val = '__get') {
    $RegisterKey = if ($global) {
        Get-Item -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager'
    } else {
        Get-Item -Path 'HKCU:'
    }

    if ($val -eq '__get') {
        $EnvRegisterKey = $RegisterKey.OpenSubKey('Environment')
        $RegistryValueOption = [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames
        $EnvRegisterKey.GetValue($name, $null, $RegistryValueOption)
    } else {
        $EnvRegisterKey = $RegisterKey.OpenSubKey('Environment', $true)
        if($null -eq $val) {
            $EnvRegisterKey.DeleteValue($name)
        } else {
            if($val.Contains('%')) {
                $EnvRegisterKey.SetValue($name, $val, [Microsoft.Win32.RegistryValueKind]::ExpandString)
            } else {
                $EnvRegisterKey.SetValue($name, $val, [Microsoft.Win32.RegistryValueKind]::String)
            }
        }
    }
}

If the environment variable value is empty, delete the environment variable

If the value of the environment variable contains `%`, the type sets to `ExpandString`; otherwise, the original type of the environment variable is retained; the default type of the new environment variable is `String`.
r15ch13
r15ch13 previously approved these changes Feb 25, 2023
rashil2000
rashil2000 previously approved these changes Feb 25, 2023
niheaven
niheaven previously approved these changes Feb 25, 2023
@niheaven niheaven dismissed stale reviews from rashil2000, r15ch13, and themself via bc1d22f February 25, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants