Skip to content
New issue

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

#

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

Already on GitHub? # to your account

GH-303 Install-ChocolateyPath Expands Variables in PATH, Overwriting #373

Closed
wants to merge 1 commit into from
Closed

GH-303 Install-ChocolateyPath Expands Variables in PATH, Overwriting #373

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions src/chocolatey.resources/helpers/functions/Get-EnvironmentVariable.ps1
View file
Original file line number Diff line number Diff line change
@@ -10,11 +10,27 @@
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

function Get-EnvironmentVariable([string] $Name, [System.EnvironmentVariableTarget] $Scope) {
[Environment]::GetEnvironmentVariable($Name, $Scope)
}

# Some enhancements to think about here.
# $machinePath = [Microsoft.Win32.Registry]::LocalMachine.OpenSubKey("SYSTEM\CurrentControlSet\Control\Session Manager\Environment\").GetValue("PATH", "", [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames).ToString();
# limitations under the License.

function Get-EnvironmentVariable([string] $Name, [System.EnvironmentVariableTarget] $Scope, [bool] $PreserveVariables = $False) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more elegant (and more conforming with PowerShell conventions) to declare the argument as [switch] instead of [bool].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true

if ($pathType -eq [System.EnvironmentVariableTarget]::Machine) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter name is Scope, not pathType.

$reg = [Microsoft.Win32.Registry]::Machine.OpenSubKey("Environment", $true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at my comment on line 36 - that's the machine registry path. It's not top level at the hive like it is for HKCU.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this has not yet been updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnadel still waiting for fixes here as well.

} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not handle the third possible EnvironmentVariableTarget, Process (which has nothing to do with the registry).

$reg = [Microsoft.Win32.Registry]::CurrentUser.OpenSubKey("Environment", $true)
}

if ($PreserveVariables -eq $True) {
$option = [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames
} else {
$option = [Microsoft.Win32.RegistryValueOptions]::None
}

$value = $reg.GetValue('Path', $null, $option)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path should be $Name

Copy link
Contributor Author

@mnadel mnadel Jul 30, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this hasn't yet been updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnadel Take a look here, this PR hasn't been updated.


$reg.Close()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RegistryKey objects wrap unmanaged resources (registry handles), so their usage should be guarded by try...finally:

<open the key>
try
{
    ...
}
finally
{
    $reg.Close()
}

Otherwise, the handles will be leaked in case of errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


return $value
}

# Some enhancements to think about here.
# $machinePath = [Microsoft.Win32.Registry]::LocalMachine.OpenSubKey("SYSTEM\CurrentControlSet\Control\Session Manager\Environment\").GetValue("PATH", "", [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames).ToString();
6 changes: 3 additions & 3 deletions src/chocolatey.resources/helpers/functions/Install-ChocolateyPath.ps1
View file
Original file line number Diff line number Diff line change
@@ -10,8 +10,8 @@
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# limitations under the License.
function Install-ChocolateyPath {
param(
[string] $pathToInstall,
@@ -26,7 +26,7 @@ param(
if (!$envPath.ToLower().Contains($pathToInstall.ToLower()))
{
Write-Host "PATH environment variable does not have $pathToInstall in it. Adding..."
$actualPath = Get-EnvironmentVariable -Name 'Path' -Scope $pathType
$actualPath = Get-EnvironmentVariable -Name 'Path' -Scope $pathType -PreserveVariables $True

$statementTerminator = ";"
#does the path end in ';'?