feat(lib): Allow env_add_path and shortcuts field to use variables#6616
feat(lib): Allow env_add_path and shortcuts field to use variables#6616abgox wants to merge 5 commits intoScoopInstaller:developfrom
Conversation
WalkthroughAdds PowerShell variable expansion to env_add_path, env_rm_path, and shortcuts by using ExpandString; adjusts path construction fallback to previous join/absolute logic when no expansion occurs; and reorders env_add_path to run after env_set in some flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/install.ps1 (2)
336-343: Consider extracting duplicated path expansion logic.Lines 336-343 duplicate the expansion logic from lines 320-327. Consider extracting to a helper function:
♻️ Proposed refactor
+function resolve_env_paths($env_add_path, $dir) { + $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 + } + } +} + 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 } - $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 - } - } + $path = resolve_env_paths $env_add_path $dir Add-Path -Path $path -TargetEnvVar $scoopPathEnvVar -Global:$global -Force } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/install.ps1` around lines 336 - 343, The block that expands PATH entries is duplicated; extract it into a helper function (e.g., Resolve-PathEntry or Expand-PathEntry) that accepts a directory ($dir) and a single path entry ($_), performs $ExecutionContext.InvokeCommand.ExpandString($_), compares the expanded value to the original, and returns either the expanded string or the result of Join-Path $dir $_ | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ } to preserve current behavior; then replace both duplicated blocks (the loop using $env_add_path.Where and the earlier occurrence) to call this new helper so the logic (ExpandString, Join-Path, Get-AbsolutePath, is_in_dir) is centralized.
322-326: Minor: Inconsistent spacing afterif.The new code uses
if($path_expandandif($targetwhile the existing codebase typically usesif ($conditionwith a space.🧹 Style consistency fix
- if($path_expand -eq $_){ + if ($path_expand -eq $_) { Join-Path $dir $_ | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ } - }else{ + } else { $path_expand }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/install.ps1` around lines 322 - 326, The if-statements in this snippet use `if($path_expand` (and elsewhere `if($target`) without the project-standard space; update these to use a space after `if` (e.g., `if ($path_expand -eq $_)`), keeping the same condition logic and return values for Join-Path/Get-AbsolutePath and the else branch, and apply the same spacing consistency for any other occurrences like `if ($target)` or similar in this file; do not change variable names `$path_expand`, `$dir`, `$_`, or the `is_in_dir` call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/install.ps1`:
- Around line 320-327: This block currently bypasses is_in_dir for expanded
entries ($env_add_path -> $path_expand) due to PR `#6616`; decide the intended
behavior and document it: if the bypass is intentional, add a concise code
comment above this block referencing PR `#6616`, explaining why expanded variables
are allowed to expand outside the install dir (the security rationale and any
mitigations), otherwise enforce validation by applying is_in_dir to $path_expand
(e.g., run Get-AbsolutePath/Join-Path then is_in_dir on $path_expand) so
expanded values cannot add arbitrary system paths. Ensure the comment references
$env_add_path, $path_expand, ExpandString, and is_in_dir so future reviewers can
find the logic.
In `@lib/shortcuts.ps1`:
- Around line 5-8: The use of $ExecutionContext.InvokeCommand.ExpandString to
compute $target (and the similar code handling entries at lines 17-20) evaluates
arbitrary PowerShell expressions and is unsafe for untrusted manifests; replace
this with a safe, restricted expansion routine: detect and expand only allowed
variable patterns (e.g., $home and $env:VAR names) for the value used to set
$target instead of calling ExpandString, or perform a whitelist/regex
substitution of environment variables, falling back to the current
[System.IO.Path]::Combine($dir, $_.item(0)) behavior when no safe expansions are
present; update the code references that call ExpandString (the
$ExecutionContext.InvokeCommand.ExpandString usage computing $target) to use the
new safe-expansion helper.
---
Nitpick comments:
In `@lib/install.ps1`:
- Around line 336-343: The block that expands PATH entries is duplicated;
extract it into a helper function (e.g., Resolve-PathEntry or Expand-PathEntry)
that accepts a directory ($dir) and a single path entry ($_), performs
$ExecutionContext.InvokeCommand.ExpandString($_), compares the expanded value to
the original, and returns either the expanded string or the result of Join-Path
$dir $_ | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ } to preserve
current behavior; then replace both duplicated blocks (the loop using
$env_add_path.Where and the earlier occurrence) to call this new helper so the
logic (ExpandString, Join-Path, Get-AbsolutePath, is_in_dir) is centralized.
- Around line 322-326: The if-statements in this snippet use `if($path_expand`
(and elsewhere `if($target`) without the project-standard space; update these to
use a space after `if` (e.g., `if ($path_expand -eq $_)`), keeping the same
condition logic and return values for Join-Path/Get-AbsolutePath and the else
branch, and apply the same spacing consistency for any other occurrences like
`if ($target)` or similar in this file; do not change variable names
`$path_expand`, `$dir`, `$_`, or the `is_in_dir` call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3e15c7c-e9d6-4ef6-8858-4d88bd6cae4c
📒 Files selected for processing (3)
CHANGELOG.mdlib/install.ps1lib/shortcuts.ps1
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/install.ps1`:
- Around line 320-327: The code currently uses
$ExecutionContext.InvokeCommand.ExpandString($_) and compares $path_expand to $_
to decide if a manifest entry contained variables, which misclassifies literal
entries like '$foo\bin' when $foo is undefined; change the logic in the block
that computes $path (and duplicate the same guard in env_rm_path) to detect
variable tokens before trusting ExpandString: if $_ contains a variable token
(e.g. matches '\$[A-Za-z_]' or '\$\{') then use $path_expand, otherwise preserve
the original literal $_ (or if ExpandString produced an empty string while $_
contained '$', treat it as literal). Update the $path_expand/$_ conditional so
literal entries are passed through unchanged and mirror the same fix in
env_rm_path to keep add/remove symmetric.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd6477c3-8a97-4052-9e59-6b428178952f
📒 Files selected for processing (2)
lib/install.ps1libexec/scoop-reset.ps1
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/install.ps1 (1)
335-341:⚠️ Potential issue | 🔴 CriticalSame issues apply here; keep symmetric with
env_add_path.This block has the identical security and correctness concerns as
env_add_pathabove. When fixing the expansion logic inenv_add_path, apply the same fix here to ensure paths are added and removed consistently.🔄 Proposed symmetric fix
$path = $env_add_path.Where({ $_ }) | ForEach-Object { - if ($_ -like '$*'){ - $ExecutionContext.InvokeCommand.ExpandString($_) - }else{ + $usesSupportedVariable = $_ -match '(^|[^`])\$(env:|home\b|\{env:|\{home\})' + if ($usesSupportedVariable) { + $path_expand = $ExecutionContext.InvokeCommand.ExpandString($_) + if ($path_expand -ne $_ -and $path_expand -ne '') { + $path_expand + } + } else { Join-Path $dir $_ | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/install.ps1` around lines 335 - 341, The $path construction block mirroring env_add_path must be changed to use the same secure, symmetric expansion logic as env_add_path: avoid blind ExpandString on entries that start with '$*', preserve literal entries and perform Join-Path + Get-AbsolutePath only for filesystem entries, and ensure is_in_dir($dir, $_) is used consistently; update the code in the $path = $env_add_path.Where({ $_ }) | ForEach-Object { ... } block to apply the identical validation, expansion and path-normalization steps (same checks/functions: ExpandString, Join-Path, Get-AbsolutePath, is_in_dir) you implemented for env_add_path so additions and removals behave consistently and safely.
🧹 Nitpick comments (1)
lib/install.ps1 (1)
63-63: Reordering is correct for variable expansion to work.Moving
env_add_pathafterenv_setensures that any environment variables defined in the manifest'senv_setare available in the current session whenenv_add_pathattempts to expand them.Static analysis suggests using named parameters for clarity:
- env_add_path $manifest $dir $global $architecture + env_add_path -manifest $manifest -dir $dir -global $global -arch $architecture🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/install.ps1` at line 63, The call to env_add_path should use named parameters for clarity and future-proofing: replace the positional invocation env_add_path $manifest $dir $global $architecture with a call that passes parameter names (e.g., -Manifest $manifest -Directory $dir -Global $global -Architecture $architecture) so readers and static analysis can clearly see which arguments map to which parameters; ensure you apply this change in the same invocation and any other places where env_add_path is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/install.ps1`:
- Around line 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.
---
Duplicate comments:
In `@lib/install.ps1`:
- Around line 335-341: The $path construction block mirroring env_add_path must
be changed to use the same secure, symmetric expansion logic as env_add_path:
avoid blind ExpandString on entries that start with '$*', preserve literal
entries and perform Join-Path + Get-AbsolutePath only for filesystem entries,
and ensure is_in_dir($dir, $_) is used consistently; update the code in the
$path = $env_add_path.Where({ $_ }) | ForEach-Object { ... } block to apply the
identical validation, expansion and path-normalization steps (same
checks/functions: ExpandString, Join-Path, Get-AbsolutePath, is_in_dir) you
implemented for env_add_path so additions and removals behave consistently and
safely.
---
Nitpick comments:
In `@lib/install.ps1`:
- Line 63: The call to env_add_path should use named parameters for clarity and
future-proofing: replace the positional invocation env_add_path $manifest $dir
$global $architecture with a call that passes parameter names (e.g., -Manifest
$manifest -Directory $dir -Global $global -Architecture $architecture) so
readers and static analysis can clearly see which arguments map to which
parameters; ensure you apply this change in the same invocation and any other
places where env_add_path is called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ce99c3a-da93-4423-8950-f81272e67c75
📒 Files selected for processing (2)
lib/install.ps1lib/shortcuts.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/shortcuts.ps1
| $path = $env_add_path.Where({ $_ }) | ForEach-Object { | ||
| if ($_ -like '$*'){ | ||
| $ExecutionContext.InvokeCommand.ExpandString($_) | ||
| }else{ | ||
| Join-Path $dir $_ | Get-AbsolutePath | Where-Object { is_in_dir $dir $_ } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 $_ })
There was a problem hiding this comment.
🧩 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:
-
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. -
Key format mismatch — the mapping uses
'$' + $_.Name(e.g.,$PROGRAMFILES), but the issue examples (#6605) use$env:ProgramFilesstyle. These keys won't match. -
PowerShell automatic variables (e.g.,
$home) aren't inEnv:*—Get-ChildItem Env:*only yields Windows environment variables, so$home\.local\binwould never be substituted. -
is_in_dirwill always reject absolute expanded paths — even if substitution worked, an expanded absolute path likeC:\Program Files\Git\binis 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.
There was a problem hiding this comment.
-
I can agree that $env: can be used as a prefix
-
$home is just an alias to $env:USERPROFILE
-
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.
There was a problem hiding this comment.
@chendler, all three points are well-taken. Agreed on all counts:
- Only
$env:prefix needed. $homeis redundant —$env:USERPROFILEcovers it.- The
is_in_dircheck must be preserved per the manifest spec. That constraint also provides a natural security boundary: any variable that expands to an absolute path outside$dirwill 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:
substituteuses-replace(case-insensitive in PowerShell), so$env:ProgramFiles,$env:PROGRAMFILES, and$env:programfilesall match correctly against the hashtable built fromGet-ChildItem Env:*.- No special-casing needed — the
$_ -like '$*'branch is gone entirely; every entry goes through the same pipeline. is_in_diris 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 inpost_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.
|
|
2 participants
Description
env_add_pathandshortcutsfield to use variables.env_setexists,env_add_pathcan also use the variables defined by it.Motivation and Context
Closes #6605
How Has This Been Tested?
Checklist:
developbranch.Summary by CodeRabbit