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

Possible bug in Install-ChocolateyPath.ps1 #1015

Closed
DarwinJS opened this issue Oct 13, 2016 · 9 comments
Closed

Possible bug in Install-ChocolateyPath.ps1 #1015

DarwinJS opened this issue Oct 13, 2016 · 9 comments
Assignees
Milestone

Comments

@DarwinJS
Copy link
Contributor

This line checks the existing path without qualifying it for "user" or "machine"
https://github.com/darwinjs/choco/blob/master/src/chocolatey.resources/helpers/functions/Install-ChocolateyPath.ps1#L78

However, the path addition statements later in the code are structured so as to add the path only to the requested scope.

If a packager calls the function with the intent of adding to "machine", shouldn't the code add it to machine EVEN IF it already exists in user?

Paths in machine are [a] available when no one is logged in (e.g. group policy processing), [b] are available to system processes and services (that do not login as the current user) and [c] are available to all users - so it differs significantly in functionality.

It seems if line 78 also qualified the check by user or machine it would seem to properly handle situations where the path exists in user, but the intent is to make the software available to all users or the system and I think it should also accurately handle situations where the intent is to only update the user's path for them to use the software.

@DarwinJS DarwinJS changed the title Possible edge case bug in Install-ChocolateyPath.ps1 Possible bug in Install-ChocolateyPath.ps1 Oct 13, 2016
@ferventcoder
Copy link
Member

Agreed. Hacktoberfest it is.

@DarwinJS
Copy link
Contributor Author

Ok - I'll do this one too as well.

@ferventcoder
Copy link
Member

IIRC, this entire function could use some modernization (or best practices for PowerShell added in). Splitting all values into an array at ; would be a good start.

@DarwinJS
Copy link
Contributor Author

OK cool - that's exactly the code I had planned for uninstall, but wasn't sure how much to stick to the existing patterns.

@DarwinJS
Copy link
Contributor Author

@DarwinJS
Copy link
Contributor Author

Adding some more problem data here - the function does not work when the path being requested is a parent of a path that is already on the path (stumbled upon when using 'c:\users' as a test case).

I believe this would be fixed by parsing the path into an array and comparing our path to each array item for an exact, case insensitive match.

However, this will be more changes than the if statement.

@DarwinJS
Copy link
Contributor Author

PR #1017

@brunoyb
Copy link

brunoyb commented Mar 18, 2017

Another related issue with that line is that it compares what I want to add with $env:Path always expanded, which means it will always add to the Path again if I'm actually trying to add something that is not expanded (e.g. '%ProgramFiles%\example').

It seems this is already covered by the PR, but I thought it would be nice to keep it documented here (I was searching for an issue and found that this already fixes it).

@ferventcoder ferventcoder added this to the 0.10.7 milestone Apr 22, 2017
@ferventcoder ferventcoder modified the milestones: 0.10.7, 0.10.8 Jun 6, 2017
@ferventcoder ferventcoder modified the milestone: 0.10.9 Aug 27, 2017
@ferventcoder ferventcoder modified the milestones: 0.10.10, 0.10.9 Aug 29, 2017
@ferventcoder ferventcoder modified the milestones: 0.10.10, 0.10.12 Mar 27, 2018
@ferventcoder ferventcoder modified the milestones: 0.10.12, 0.10.13 May 3, 2018
@ferventcoder ferventcoder modified the milestones: 0.10.13, 0.10.14, 0.10.15 Mar 9, 2019
@ferventcoder ferventcoder modified the milestones: 0.10.15, 0.10.16 Apr 1, 2019
@gep13 gep13 modified the milestones: 0.10.16, 0.10.17 May 31, 2019
@ferventcoder ferventcoder modified the milestones: 0.10.17, 0.10.18 Jan 14, 2020
@ferventcoder ferventcoder modified the milestones: 0.10.18, 0.10.x Apr 14, 2021
@vexx32 vexx32 modified the milestones: Future, 2.3.0 Sep 18, 2024
@vexx32
Copy link
Member

vexx32 commented Sep 18, 2024

I am no longer able to reproduce this issue as of 2.3.0 with the following steps on Windows:

  1. Clone this repository
  2. From the repository root, run the following commands in PowerShell:
    $testpath = "C:\test\path\001"
    choco pack './tests/packages/add-path/add-path.nuspec'
    
    # set current process PATH
    $env:PATH = $env:PATH + ";$testPath"
    
    choco install add-path -s . -y --params "'/Path=$testpath /Scope=User'"
  3. Monitor the output, which shows that the User-scope path is checked, finds that the PATH value is not present, and adds it at the appropriate scope, ignoring that the path value is in the current-process PATH value entirely.
  4. Verify the change by checking the output from [Environment]::GetEnvironmentVariable('PATH', 'User')

As such, I'll be closing this issue as resolved.

@vexx32 vexx32 closed this as completed Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants