fix(checkver)!: Harden checkver script evaluation#6652
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCheckver now runs user ChangesCheckver Script Execution
Sequence Diagram(s)sequenceDiagram
participant CLI as User/CLI
participant Runner as bin/checkver.ps1
participant Source as Remote Page
participant JobHost as PowerShell Job
participant Script as checkver.script
CLI->>Runner: invoke checkver flow (with checkver.script)
Runner->>Source: fetch page (assign $page)
Runner->>Runner: Get-CheckverScriptPrelude()
Runner->>Runner: Invoke-SanitizeScriptBlock(parse & reject `$using:`)
Runner->>JobHost: Start-Job(prelude + validated script)
JobHost->>Script: execute in constrained environment (env whitelist, ConstrainedLanguage, ErrorAction=Stop)
Script->>JobHost: emit output
JobHost->>Runner: Receive-Job output (trim -> $page)
Runner->>Runner: if (!$page) -> treat as missing content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
chawyehsu
force-pushed
the
push-wqlvqmwnluqo
branch
from
efe9c95 to
bddff3b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@bin/checkver.ps1`:
- Around line 340-342: Replace the text-replacement hack that mutates $script
("$script = $script -replace '\$using:','$'") with AST-based validation: parse
$script using the PowerShell AST parser (e.g.,
[System.Management.Automation.Language.Parser]::ParseInput), search the AST for
UsingExpressionAst nodes (AST.FindAll or equivalent), and if any
UsingExpressionAst is found throw/return an error rejecting the script; this
ensures you reject outer-scope $using: captures without silently altering quoted
strings, here-strings, comments, or regexes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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 `@bin/checkver.ps1`:
- Line 348: The Start-Job/Receive-Job pipeline that assigns $page uses
Out-String which drops the job error stream; modify the job invocation around
Start-Job / Receive-Job / $scriptBlock so you capture stderr from the job (e.g.,
redirect within the job script block with 2>&1 or use Receive-Job -ErrorVariable
and/or Start-Job -ErrorVariable) and merge or append any captured error text
into $page (or a separate $pageError) so the calling code can log the actual job
error instead of only the generic "couldn't retrieve content" message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1da6f7a0-c38e-4972-8bbd-774c48690d46
📒 Files selected for processing (2)
CHANGELOG.mdbin/checkver.ps1
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
chawyehsu
force-pushed
the
push-wqlvqmwnluqo
branch
from
bddff3b to
750202a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/checkver.ps1`:
- Around line 341-352: ParseInput() can return syntax errors in $errors which
must be checked before calling [scriptblock]::Create; update the block around
ParseInput/$ast to inspect $errors (and/or $ast.InvocationErrors) after the
ParseInput call and, if any errors exist, skip this manifest (e.g., use
next/continue with an explanatory message) instead of proceeding to create the
script block; ensure the check happens before Get-CheckverScriptPrelude and the
[scriptblock]::Create($script -join "`n") call so malformed checkver.script
entries don't abort the whole run.
- Line 354: Add a timeout when executing the checkver.script job so a hung
manifest can't block the whole run: when creating the job with Start-Job and
capturing output into $page, replace the unbounded Receive-Job -Wait call with a
pattern that uses Wait-Job -Timeout (or Receive-Job -Timeout) and if the wait
times out, Stop-Job/Remove-Job, log an error, and set $page to an appropriate
fallback (empty string or error marker). Update the block around Start-Job /
Receive-Job that sets $page so it enforces a configurable timeout and performs
cleanup (Stop-Job/Remove-Job) and logging on timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a7cce12-da2f-4693-8f18-5c19c5115789
📒 Files selected for processing (2)
CHANGELOG.mdbin/checkver.ps1
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
| $script = $prelude + @($script) | ||
| $scriptBlock = [scriptblock]::Create($script -join "`n") | ||
|
|
||
| $page = (Start-Job -ScriptBlock $scriptBlock | Receive-Job -Wait -AutoRemoveJob | Out-String).Trim() |
There was a problem hiding this comment.
Add a timeout around checkver.script execution.
Receive-Job -Wait has no upper bound. One buggy or hostile manifest script can now stall the entire checkver run indefinitely.
Proposed fix
- $page = (Start-Job -ScriptBlock $scriptBlock | Receive-Job -Wait -AutoRemoveJob | Out-String).Trim()
+ $job = Start-Job -ScriptBlock $scriptBlock
+ if (-not (Wait-Job $job -Timeout 30)) {
+ Stop-Job $job | Out-Null
+ Remove-Job $job -Force | Out-Null
+ next 'checkver.script timed out'
+ continue
+ }
+ $page = (Receive-Job $job -AutoRemoveJob | Out-String).Trim()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $page = (Start-Job -ScriptBlock $scriptBlock | Receive-Job -Wait -AutoRemoveJob | Out-String).Trim() | |
| $job = Start-Job -ScriptBlock $scriptBlock | |
| if (-not (Wait-Job $job -Timeout 30)) { | |
| Stop-Job $job | Out-Null | |
| Remove-Job $job -Force | Out-Null | |
| next 'checkver.script timed out' | |
| continue | |
| } | |
| $page = (Receive-Job $job -AutoRemoveJob | Out-String).Trim() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/checkver.ps1` at line 354, Add a timeout when executing the
checkver.script job so a hung manifest can't block the whole run: when creating
the job with Start-Job and capturing output into $page, replace the unbounded
Receive-Job -Wait call with a pattern that uses Wait-Job -Timeout (or
Receive-Job -Timeout) and if the wait times out, Stop-Job/Remove-Job, log an
error, and set $page to an appropriate fallback (empty string or error marker).
Update the block around Start-Job / Receive-Job that sets $page so it enforces a
configurable timeout and performs cleanup (Stop-Job/Remove-Job) and logging on
timeout.
750202a to
e8f9fbe
Compare
| # Use `UseBasicParsing` for Invoke-WebRequest by default (WindowsPowerShell) | ||
| '$PSDefaultParameterValues["Invoke-WebRequest:UseBasicParsing"] = $true', | ||
| # Use ConstrainedLanguage language mode to restrict checkver.script capabilities | ||
| '$ExecutionContext.SessionState.LanguageMode = "ConstrainedLanguage"', |
There was a problem hiding this comment.
Isn't the restriction on checkver.script a bit too strict? I'm wondering if it's necessary to be this strict, considering that this script mostly runs in a CI environment.
It adds too many constraints. For example, it prevents both redirection-based logic and download-based version fetching. Currently, we have no better way to get the latest version for these two scenarios.
- Redirection-based version fetching: https://github.com/ScoopInstaller/Extras/blob/4d67fee27762282036867bd0f625206e8d007408/bucket/xmind.json#L27-L32
- Download-based version fetching: https://github.com/ScoopInstaller/Extras/blob/4d67fee27762282036867bd0f625206e8d007408/bucket/spotify.json#L66-L74
There was a problem hiding this comment.
Redirection-based version fetching: https://github.com/ScoopInstaller/Extras/blob/4d67fee27762282036867bd0f625206e8d007408/bucket/xmind.json#L27-L32
This can to be updated to use Invoke-WebRequest to get HTTP redirect URLs, e.g. extras/rtss.
Download-based version fetching: https://github.com/ScoopInstaller/Extras/blob/4d67fee27762282036867bd0f625206e8d007408/bucket/spotify.json#L66-L74
They should put the temporary files to $env:TEMP/$env:TMP (which is why these env vars are allowed in the implementation) instead of relying on the internal cache_path function and placing files expected to be in $env:TEMP to Scoop's cache dir.
Isn't the restriction on checkver.script a bit too strict? I'm wondering if it's necessary to be this strict, considering that this script mostly runs in a CI environment.
From what I can tell, these implementations are all necessary.
There was a problem hiding this comment.
Redirection-based version fetching: https://github.com/ScoopInstaller/Extras/blob/4d67fee27762282036867bd0f625206e8d007408/bucket/xmind.json#L27-L32
This can to be updated to use
Invoke-WebRequestto get HTTP redirect URLs, e.g. extras/rtss.
Yes, but having to support both PowerShell 5 and PowerShell 7 makes it more cumbersome (ScoopInstaller/Extras#17052 (comment)) — not as clean as using HttpWebRequest.
But it's OK, maybe we can add a feature to get redirect URLs later (#6526).
Download-based version fetching: https://github.com/ScoopInstaller/Extras/blob/4d67fee27762282036867bd0f625206e8d007408/bucket/spotify.json#L66-L74
They should put the temporary files to
$env:TEMP/$env:TMP(which is why these env vars are allowed in the implementation) instead of relying on the internalcache_pathfunction and placing files expected to be in$env:TEMPto Scoop's cache dir.
You're right. I missed that point.
There was a problem hiding this comment.
From what I can tell, these implementations are all necessary.
Could you share the reasons?
The examples mentioned above are just common scenarios.
Is it possible to add an option that enables restrictions by default, but allows them to be disabled via configuration (both globally and in the manifest)? There may be still some special cases where workarounds have been implemented on checkver, and in such cases, without an option to disable restrictions, we would have to deprecate these manifests as they cannot be updated. e.g., https://github.com/ScoopInstaller/Extras/blob/e066b75375e385789965bc64a9c90cfca7e496be/bucket/qq-nt.json#L35
Additionally, should access to GitHub tokens also be permitted? There may be situations where a token is needed to access the GitHub API multiple times within the script.
There was a problem hiding this comment.
Could you share the reasons?
Sorry, I'm unable to disclose more on this.
Is it possible to add an option that enables restrictions by default, but allows them to be disabled via configuration
That would render the change meaningless.
Additionally, should access to GitHub tokens also be permitted? There may be situations where a token is needed to access the GitHub API multiple times within the script.
I searched the entire org and found only 3 manifests that explicitly use GitHub Tokens in the scriptblock. Is this truly an absolutely necessary action without alternatives? This involves the misuse of internal functions, which is a key focus of this PR and its subsequent refinements.
There was a problem hiding this comment.
Yes, but having to support both PowerShell 5 and PowerShell 7 makes it more cumbersome (ScoopInstaller/Extras#17052 (comment)) — not as clean as using HttpWebRequest.
But it's OK, maybe we can add a feature to get redirect URLs later (#6526).
Actually in my dev branch I've made a attempt to provide an allowed builtin function to get redirect URLs. This attempt is feasible, but it doesn't seem concise or intuitive enough to me, so it wasn't included in this PR. I believe we will have a more complete solution for #6526.
There was a problem hiding this comment.
Is it possible to add an option that enables restrictions by default, but allows them to be disabled via configuration
That would render the change meaningless.
If we impose such strict restrictions without providing a way to disable them, we'll likely lose a great deal of flexibility. Once this change gets merged, some community buckets may consider bypassing these restrictions, and we'll only end up restricting ourselves. Some manifests may even be deprecated simply because they can no longer fetch the latest versions. I think just limiting this behavior to the official bucket should be fine. I've also consistently advocated for reducing the use of internal functions. Additionally, using the built-in GitHub Actions token instead of a PAT isn't so risky, is it?
Additionally, should access to GitHub tokens also be permitted? There may be situations where a token is needed to access the GitHub API multiple times within the script.
I searched the entire org and found only 3 manifests that explicitly use GitHub Tokens in the scriptblock. Is this truly an absolutely necessary action without alternatives? This involves the misuse of internal functions, which is a key focus of this PR and its subsequent refinements.
If only a single GitHub API request is needed for version detection, we can place the URL in checkver.github and use the returned result via $page. However, if multiple requests are required, this approach won't work.
Perhaps we could ask the other maintainers for their thoughts?
- Avoids potential breaking changes and environment restrictions from the upstream - See ScoopInstaller/Scoop#6652
* Avoids potential breaking changes and environment restrictions from the upstream * Relates to ScoopInstaller/Scoop#6652 * Relates to ScoopInstaller/Scoop#6653
chawyehsu
force-pushed
the
push-wqlvqmwnluqo
branch
from
e8f9fbe to
c7f9470
Compare
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
chawyehsu
force-pushed
the
push-wqlvqmwnluqo
branch
from
c7f9470 to
26e0ccb
Compare
Description
This PR is considered to be a breaking change to
checkver.script.checkverrefinement seriesPrior attempts
Motivation and Context
Enforce a safer checkver script evaluation in a contranined runtime. Limit side effects by restricting access to outer variables and functions.
How Has This Been Tested?
Run
checkver.ps1over local buckets and validate manifestcheckver.scriptevaluation.Checklist:
developbranch.Summary by CodeRabbit
Refactor
Bug Fixes
Changelog