Skip to content

feat(lib): Allow env_add_path and shortcuts field to use variables #6616

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.

Sign up for GitHub

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

Open
abgox wants to merge 5 commits into
base: develop
Choose a base branch
from
+29 −6
Open

feat(lib): Allow env_add_path and shortcuts field to use variables #6616

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
45eb5eb
feat(lib): allow env_add_path to use variables
abgox Mar 11, 2026
7991f7a
feat(lib): allow shortcuts to use variables
abgox Mar 11, 2026
3c05571
docs(changelog): update CHANGLOG
abgox Mar 11, 2026
7611222
feat: allow env_add_path to include variables defined in env_set
abgox Mar 11, 2026
0c22b23
refactor: use -like to check variables
abgox Mar 11, 2026
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- **install:** Add separator at the end of notes, highlight suggestions ([#6418](https://github.com/ScoopInstaller/Scoop/issues/6418))
- **download|scoop-download:** Add GitHub issue prompt when the default downloader fails ([#6539](https://github.com/ScoopInstaller/Scoop/issues/6539))
- **download|scoop-config:** Allow disabling automatic fallback to the default downloader when Aria2c download fails ([#6538](https://github.com/ScoopInstaller/Scoop/issues/6538))
- **lib** Allow `env_add_path` and `shortcuts` field to use variables ([#6616](https://github.com/ScoopInstaller/Scoop/issues/6616))

### Bug Fixes

Expand Down
18 changes: 16 additions & 2 deletions lib/install.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,14 @@ function env_add_path($manifest, $dir, $global, $arch) {
if (get_config USE_ISOLATED_PATH) {
Add-Path -Path ('%' + $scoopPathEnvVar + '%') -Global:$global
}
$path = $env_add_path.Where({ $_ }).ForEach({ Join-Path $dir $_ | Get-AbsolutePath }).Where({ is_in_dir $dir $_ })
$path = $env_add_path.Where({ $_ }) | ForEach-Object {
$path_expand = $ExecutionContext.InvokeCommand.ExpandString($_)
if($path_expand -eq $_){
Join-Path $dir $_ | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ }
}else{
$path_expand
}
}
Comment on lines +320 to +326
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Security: Expanded paths bypass containment validation.

The is_in_dir check that prevents adding arbitrary paths outside the app directory is completely bypassed for expanded variables. A malicious or misconfigured manifest could add system directories to PATH via entries like $env:WINDIR\System32.

Additionally, the past review concern remains unaddressed: undefined variables like $foo\bin will expand to \bin since ExpandString() replaces undefined variables with empty strings, corrupting the intended path.

🛡️ Proposed fix: validate expanded paths and check for supported variables only 
         $path = $env_add_path.Where({ $_ }) | ForEach-Object {
-            if ($_ -like '$*'){
-                $ExecutionContext.InvokeCommand.ExpandString($_)
-            }else{
+            # Only expand known safe variables ($env:*, $home)
+            $usesSupportedVariable = $_ -match '(^|[^`])\$(env:|home\b|\{env:|\{home\})'
+            if ($usesSupportedVariable) {
+                $path_expand = $ExecutionContext.InvokeCommand.ExpandString($_)
+                # Only use expanded value if expansion actually changed it
+                # (undefined variables expand to empty, corrupting the path)
+                if ($path_expand -ne $_ -and $path_expand -ne '') {
+                    $path_expand
+                }
+            } else {
                 Join-Path $dir $_ | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ }
             }
         }

Consider whether expanded paths should also be validated against a whitelist of allowed directories, or at minimum logged for audit purposes.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@lib/install.ps1` around lines 320 - 326, The expansion branch currently
bypasses containment checks: when
$ExecutionContext.InvokeCommand.ExpandString($_) is used on entries from
$env_add_path they are returned without running is_in_dir, allowing constructs
like '$env:WINDIR\System32' or undefined variables to produce paths outside the
app dir; update the expansion branch to (1) restrict which variable patterns are
permitted before calling ExpandString (e.g., only allow a whitelist like
$env:APPDIR/$env:USERPROFILE), (2) after ExpandString perform the same
Join-Path/Get-AbsolutePath/is_in_dir validation used in the non-expanded branch
and reject or log any expanded path that fails containment, and (3) treat
expansions that result in paths starting with a root or that contain unresolved
empty segments (from undefined vars) as invalid and handle them consistently
(log and skip or fail) so $path assignment always contains only validated,
in-dir absolute paths.

Copy link
Copy Markdown

@chendler chendler Mar 21, 2026

Choose a reason for hiding this comment

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

More save solution would be to use Scoop core substitute function.

$envSubstitutes = @{}
Get-ChildItem Env:* | ForEach-Object { $envSubstitutes['$' + $_.Name] = $_.Value }
$path = $env_add_path.Where({ $_ }).ForEach({ Join-Path $dir $_ | Get-AbsolutePath | ForEach-Object { substitute $_ $envSubstitutes } }).Where({ is_in_dir $dir $_ })

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 21, 2026

Choose a reason for hiding this comment

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

🧩 Analysis chain

Let me look at the substitute function implementation to understand its behavior.

🏁 Script executed:

#!/bin/bash
ast-grep --pattern 'function substitute($$$) {
  $$$
}'

Length of output: 0

🏁 Script executed:

#!/bin/bash
rg -n "function substitute" -A 15

Length of output: 1074

@chendler, the substitute approach is appealing because it preserves the is_in_dir containment check. However, there are a few issues with the proposed implementation worth considering:

  1. Substitution happens after Join-Path — for an entry like $env:ProgramFiles\Git\bin, the pipeline first produces <appdir>\$env:ProgramFiles\Git\bin, and then substitution transforms it into <appdir>\C:\Program Files\Git\bin, which is a garbled path.

  2. Key format mismatch — the mapping uses '$' + $_.Name (e.g., $PROGRAMFILES), but the issue examples (#6605) use $env:ProgramFiles style. These keys won't match.

  3. PowerShell automatic variables (e.g., $home) aren't in Env:* — Get-ChildItem Env:* only yields Windows environment variables, so $home\.local\bin would never be substituted.

  4. is_in_dir will always reject absolute expanded paths — even if substitution worked, an expanded absolute path like C:\Program Files\Git\bin is intentionally outside $dir, so it gets silently dropped by the .Where({ is_in_dir $dir $_ }) filter, meaning no variable-based absolute path would ever actually be added.

A corrected version that addresses these points would need to substitute before Join-Path, include both $env:NAME and $NAME keys, add PowerShell automatic variables, and then branch on whether the result is an absolute or relative path:

$envSubstitutes = @{}
Get-ChildItem Env:* | ForEach-Object {
    $envSubstitutes['$env:' + $_.Name] = $_.Value  # $env:ProgramFiles style
    $envSubstitutes['$' + $_.Name] = $_.Value      # bare $ProgramFiles style
}
$envSubstitutes['$home'] = $HOME  # PowerShell automatic variable

$path = $env_add_path.Where({ $_ }) | ForEach-Object {
    $expanded = substitute $_ $envSubstitutes
    if ([System.IO.Path]::IsPathRooted($expanded)) {
        # Absolute path from variable expansion — skip is_in_dir but validate it exists/is valid
        $expanded | Where-Object { Test-Path $_ -PathType Container -IsValid }
    } else {
        Join-Path $dir $expanded | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ }
    }
}

That said, skipping is_in_dir for expanded absolute paths is an intentional trade-off — it's the whole point of the feature. The security question then shifts to whether manifests in trusted buckets are sufficient gatekeeping, or whether an explicit allowlist of variable prefixes (e.g., only $env:* and $home) is warranted.

Copy link
Copy Markdown

@chendler chendler Mar 21, 2026

Choose a reason for hiding this comment

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

  1. I can agree that $env: can be used as a prefix

  2. $home is just an alias to $env:USERPROFILE

  3. Omitting is_in_dir check will contradict manifest spec defined here https://github.com/ScoopInstaller/Scoop/wiki/App-Manifests#optional-properties

env_add_path: Add this directory to the user's path (or system path if --global is used). The directory is relative to the install directory and must be inside the install directory.

I assume that features built-in features prevent side effects, for other special cases there is post_install script.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 21, 2026

Choose a reason for hiding this comment

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

@chendler, all three points are well-taken. Agreed on all counts:

  1. Only $env: prefix needed.
  2. $home is redundant — $env:USERPROFILE covers it.
  3. The is_in_dir check must be preserved per the manifest spec. That constraint also provides a natural security boundary: any variable that expands to an absolute path outside $dir will fail the containment check and be silently dropped, which is the correct behavior.

With those constraints in mind, here's the corrected implementation that substitutes $env: variables before Join-Path (fixing the ordering issue from my earlier comment) while keeping the existing containment pipeline intact:

function env_add_path($manifest, $dir, $global, $arch) {
    $env_add_path = arch_specific 'env_add_path' $manifest $arch
    $dir = $dir.TrimEnd('\')
    if ($env_add_path) {
        if (get_config USE_ISOLATED_PATH) {
            Add-Path -Path ('%' + $scoopPathEnvVar + '%') -Global:$global
        }
        $envSubstitutes = @{}
        Get-ChildItem Env:* | ForEach-Object { $envSubstitutes['$env:' + $_.Name] = $_.Value }
        $path = $env_add_path.Where({ $_ }).ForEach({
            Join-Path $dir (substitute $_ $envSubstitutes) | Get-AbsolutePath
        }).Where({ is_in_dir $dir $_ })
        Add-Path -Path $path -TargetEnvVar $scoopPathEnvVar -Global:$global -Force
    }
}

And symmetrically for env_rm_path:

function env_rm_path($manifest, $dir, $global, $arch) {
    $env_add_path = arch_specific 'env_add_path' $manifest $arch
    $dir = $dir.TrimEnd('\')
    if ($env_add_path) {
        $envSubstitutes = @{}
        Get-ChildItem Env:* | ForEach-Object { $envSubstitutes['$env:' + $_.Name] = $_.Value }
        $path = $env_add_path.Where({ $_ }).ForEach({
            Join-Path $dir (substitute $_ $envSubstitutes) | Get-AbsolutePath
        }).Where({ is_in_dir $dir $_ })
        Remove-Path -Path $path -Global:$global
        Remove-Path -Path $path -TargetEnvVar $scoopPathEnvVar -Global:$global
    }
}

Key properties of this approach:

  • substitute uses -replace (case-insensitive in PowerShell), so $env:ProgramFiles, $env:PROGRAMFILES, and $env:programfiles all match correctly against the hashtable built from Get-ChildItem Env:*.
  • No special-casing needed — the $_ -like '$*' branch is gone entirely; every entry goes through the same pipeline.
  • is_in_dir is always enforced — if an env var happens to expand to an absolute path, Join-Path $dir <absolute> produces a garbled path that fails containment, so it gets dropped. Intentional absolute paths outside the app dir belong in post_install, as you noted.
✏️ Learnings added 
Learnt from: chendler
Repo: ScoopInstaller/Scoop PR: 6616
File: lib/install.ps1:320-326
Timestamp: 2026-03-21T09:08:18.606Z
Learning: In Scoop's `env_add_path` and `env_rm_path` functions in `lib/install.ps1` (PowerShell), the manifest spec requires env_add_path entries to be relative to and inside the install directory (`is_in_dir` check must always be enforced). Variable expansion for these fields should only support `$env:NAME` style prefixes (not bare `$home`, as it equals `$env:USERPROFILE`), should be done using the existing `substitute` function with a hashtable built from `Get-ChildItem Env:*` keyed as `'$env:' + $_.Name`, and substitution must occur BEFORE `Join-Path $dir` so the containment pipeline remains intact. Bypassing `is_in_dir` via `ExpandString` contradicts the manifest spec.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Add-Path -Path $path -TargetEnvVar $scoopPathEnvVar -Global:$global -Force
}
}
Expand All @@ -326,7 +333,14 @@ function env_rm_path($manifest, $dir, $global, $arch) {
$env_add_path = arch_specific 'env_add_path' $manifest $arch
$dir = $dir.TrimEnd('\')
if ($env_add_path) {
$path = $env_add_path.Where({ $_ }).ForEach({ Join-Path $dir $_ | Get-AbsolutePath }).Where({ is_in_dir $dir $_ })
$path = $env_add_path.Where({ $_ }) | ForEach-Object {
$path_expand = $ExecutionContext.InvokeCommand.ExpandString($_)
if($path_expand -eq $_){
Join-Path $dir $_ | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ }
}else{
$path_expand
}
}
Remove-Path -Path $path -Global:$global # TODO: Remove after forced isolating Scoop path
Remove-Path -Path $path -TargetEnvVar $scoopPathEnvVar -Global:$global
}
Expand Down
10 changes: 8 additions & 2 deletions lib/shortcuts.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
function create_startmenu_shortcuts($manifest, $dir, $global, $arch) {
$shortcuts = @(arch_specific 'shortcuts' $manifest $arch)
$shortcuts | Where-Object { $_ -ne $null } | ForEach-Object {
$target = [System.IO.Path]::Combine($dir, $_.item(0))
$target = $ExecutionContext.InvokeCommand.ExpandString($_.item(0))
if($target -eq $_.item(0)){
$target = [System.IO.Path]::Combine($dir, $_.item(0))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
$target = New-Object System.IO.FileInfo($target)
$name = $_.item(1)
$arguments = ''
Expand All @@ -11,7 +14,10 @@ function create_startmenu_shortcuts($manifest, $dir, $global, $arch) {
$arguments = $_.item(2)
}
if ($_.length -ge 4) {
$icon = [System.IO.Path]::Combine($dir, $_.item(3))
$icon = $ExecutionContext.InvokeCommand.ExpandString($_.item(3))
if($icon -eq $_.item(3)){
$icon = [System.IO.Path]::Combine($dir, $_.item(3))
}
$icon = New-Object System.IO.FileInfo($icon)
}
$arguments = (substitute $arguments @{ '$dir' = $dir; '$original_dir' = $original_dir; '$persist_dir' = $persist_dir })
Expand Down