feat(virustotal): automatic Virustotal check as option #6525
Conversation
WalkthroughAdds a VirusTotal integration and threads a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as libexec/scoop-<br/>install/download/update
participant Pipeline as lib/download.ps1<br/>lib/install.ps1
participant VT as lib/virustotal.ps1
participant API as VirusTotal API
User->>CLI: scoop install/download -w<br/>(or USE_VIRUSTOTAL config)
CLI->>Pipeline: call install_app / Invoke-ScoopDownload<br/>with check_virustotal=true
alt check_virustotal enabled
Pipeline->>VT: Test-UrlsWithVirusTotal(urls, manifest, arch)
VT->>API: Query by URL or hash / Submit for scan
API-->>VT: Analysis results / status
VT-->>Pipeline: Filtered safe URLs or report (abort)
end
alt URLs safe (or VT disabled)
Pipeline->>Pipeline: Download file(s) (aria2 or native)
Pipeline->>Pipeline: Verify hash / continue install
Pipeline-->>CLI: Success
else Threat detected
Pipeline-->>CLI: Abort with warning/report
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libexec/scoop-virustotal.ps1 (1)
59-74: Ensure_ERRconstants and$exit_codeare initialized locally.
$_ERR_NO_INFOand$exit_codeare still used here, but they are no longer defined in this script after delegating tolib\virustotal.ps1. Unless those variables are exported into the caller’s scope,$exit_code -bor $_ERR_NO_INFOwill operate on$nulland the finalexit $exit_codecan exit with$null. Please reintroduce the local initializations (e.g.,$exit_code = 0) or import the shared values explicitly before use.
🧹 Nitpick comments (1)
lib/hash.ps1 (1)
16-28: Consider extracting hash types to a constant.The list of supported hash types
@('md5', 'sha1', 'sha256', 'sha512')is hardcoded. Consider extracting this to a module-level or script-level constant to improve maintainability and make it easier to extend support for additional hash algorithms in the future.Example:
+$script:SupportedHashTypes = @('md5', 'sha1', 'sha256', 'sha512') + function get_hash([String] $multihash) { $type, $hash = $multihash -split ':' if (!$hash) { # no type specified, assume sha256 $type, $hash = 'sha256', $multihash } - if (@('md5', 'sha1', 'sha256', 'sha512') -notcontains $type) { + if ($script:SupportedHashTypes -notcontains $type) { return $null, "Hash type '$type' isn't supported." } return $type, $hash.ToLower() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.md(1 hunks)bin/checkhashes.ps1(1 hunks)lib/autoupdate.ps1(1 hunks)lib/download.ps1(6 hunks)lib/hash.ps1(1 hunks)lib/install.ps1(2 hunks)lib/virustotal.ps1(1 hunks)libexec/scoop-config.ps1(1 hunks)libexec/scoop-download.ps1(1 hunks)libexec/scoop-install.ps1(3 hunks)libexec/scoop-update.ps1(8 hunks)libexec/scoop-virustotal.ps1(3 hunks)
🔇 Additional comments (12)
CHANGELOG.md (1)
7-8: Verify issue reference.The changelog entry references issue #6525, but according to the PR description, the linked issue is #6521. Please verify which issue number is correct.
lib/hash.ps1 (1)
3-14: LGTM!The function correctly matches URLs to their corresponding hashes in the manifest. The error handling for missing URLs is appropriate, and the null filtering ensures robustness.
bin/checkhashes.ps1 (1)
45-45: LGTM!The import correctly exposes the
get_hashfunction from the newly created hash module, supporting the refactoring effort to centralize hash-related utilities.lib/autoupdate.ps1 (1)
3-4: LGTM!The import makes the
get_hashfunction available early in the autoupdate execution flow, consistent with the refactoring to centralize hash utilities.libexec/scoop-config.ps1 (1)
95-97: Clarify timing of VirusTotal checks.The documentation states "Scoop will always use VirusTotal to scan files after downloading", but based on the PR objectives mentioning "pre-download checks" and the code showing
check_virustotalpassed to download functions, it appears the checks may occur before or during download, not after. Please verify and update the documentation to accurately reflect when VirusTotal validation occurs.libexec/scoop-download.ps1 (1)
24-24: LGTM!The import correctly exposes
hash_for_urlfunctionality needed for URL hash verification in the download flow.lib/install.ps1 (2)
9-9: LGTM!The function signature correctly extends
install_appwith the new$check_virustotalparameter, maintaining backward compatibility with a sensible default of$false.
53-53: LGTM!The parameter is correctly threaded through to
Invoke-ScoopDownload, enabling VirusTotal checks during the download phase when requested.libexec/scoop-install.ps1 (4)
27-27: LGTM!The help text correctly documents the new VirusTotal check option with appropriate warning about potential performance impact ("may be slow").
47-47: LGTM!The getopt call correctly recognizes both short (
-w) and long (--virustotal-check) option formats for the new VirusTotal check feature.
52-52: LGTM!The flag computation correctly prioritizes the CLI option over the configuration value, providing flexible control over VirusTotal checks per invocation or globally via config.
137-137: LGTM!The
check_virustotalparameter is correctly passed toinstall_app, completing the integration of VirusTotal checks into the installation flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/virustotal.ps1 (2)
4-13: Consider exposing the exit code for command-line usage.The script-scoped
$exit_code(line 13) is updated by helper functions but remains invisible to callers. While this doesn't affect theTest-UrlsWithVirusTotalworkflow (which aborts directly), it may preventscoop virustotalcommand from exiting with the correct code.Consider one of these approaches:
- Return the exit code from
virustotal_check_appso callers can use it- Expose it via
$global:exit_codeif multiple components need to read it- Document that
Test-UrlsWithVirusTotalis abort-only and exit codes are for other consumersOption 1 (preferred for scoop virustotal command):
function virustotal_check_app($app, $manifest, $architecture, $api_key, $scan) { [int]$index = 0 $urls = script:url $manifest $architecture $urls | ForEach-Object { $url = $_ $index++ if ($urls.GetType().IsArray) { info "$app`: url $index" } $hash = hash_for_url $manifest $url $architecture Check-VirusTotalUrl $app $url $hash $api_key $scan } + return $script:exit_code }Callers would then use:
$exitCode = virustotal_check_app $app $manifest $architecture $api_key $scan exit $exitCode
62-82: Consider separating counts from engine lists for clarity.The report properties
'FileReport.Malicious'and'FileReport.Suspicious'contain either an integer0(no detections) or an array of engine names (when detections exist). This type mixing works with the current-eq 0comparison inTest-UrlsWithVirusTotal(line 334) but is fragile and may confuse consumers expecting consistent types.Consider adding separate count properties for clearer contracts:
[PSCustomObject] @{ 'App.Name' = $app 'App.Url' = $url 'App.Hash' = $hash 'App.HashType' = $null 'App.Size' = filesize $fileSize 'FileReport.Url' = $report_url 'FileReport.Hash' = $report_hash - 'FileReport.Malicious' = if ($maliciousResults) { $maliciousResults } else { 0 } - 'FileReport.Suspicious' = if ($suspiciousResults) { $suspiciousResults } else { 0 } + 'FileReport.Malicious' = if ($maliciousResults) { $maliciousResults } else { @() } + 'FileReport.Suspicious' = if ($suspiciousResults) { $suspiciousResults } else { @() } + 'FileReport.MaliciousCount' = $malicious + 'FileReport.SuspiciousCount' = $suspicious 'FileReport.Timeout' = $timeout 'FileReport.Undetected' = $undetected 'UrlReport.Url' = $null }Then update the comparison to use counts:
- if ($maliciousResults -eq 0 -and $suspiciousResults -eq 0) { + if ($file_report.'FileReport.MaliciousCount' -eq 0 -and $file_report.'FileReport.SuspiciousCount' -eq 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/download.ps1(6 hunks)lib/virustotal.ps1(1 hunks)libexec/scoop-download.ps1(3 hunks)libexec/scoop-update.ps1(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libexec/scoop-update.ps1
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T19:13:23.445Z
Learnt from: felicijus
PR: ScoopInstaller/Scoop#6525
File: lib/virustotal.ps1:215-221
Timestamp: 2025-10-16T19:13:23.445Z
Learning: In PowerShell, `-inotin` is a valid case-insensitive containment comparison operator that checks if a value is not contained in a collection. The operator family includes `-notin` (case-sensitive), `-inotin` (case-insensitive), and `-cnotin` (case-sensitive with culture-aware comparison).
Applied to files:
lib/virustotal.ps1
🔇 Additional comments (4)
lib/download.ps1 (2)
345-349: LGTM: aria2 path now correctly applies VT filtering.The aria2 download path now filters URLs through
Test-UrlsWithVirusTotalwhen$check_virustotalis enabled, preventing malicious URLs from being downloaded. This resolves the previous concern about unsafe URLs bypassing checks in the aria2 flow.
707-716: No issues found; regex changes are safe and protective.The regex pattern expansion from
[._-]to[\w._-]is additive and doesn't break filename extraction. The change allows capturing filenames with letters and digits in query parameters (e.g.,?var=value), and the fallback logic at lines 709–714 protects against over-extraction by reverting toAbsolutePathorFragmentwhen needed. Test cases validate all edge cases: plain URLs, query parameters, and fragment overrides. The change improves robustness without introducing breaking changes.lib/virustotal.ps1 (1)
319-348: LGTM: Pre-download VT filtering is well-designed.The
Test-UrlsWithVirusTotalfunction correctly:
- Retrieves the API key only when VT checks are enabled
- Iterates through all URLs and checks each one
- Collects only safe URLs (those with 0 malicious and 0 suspicious detections)
- Aborts before download if no URLs pass validation
This provides strong protection against malicious downloads.
libexec/scoop-download.ps1 (1)
109-113: No issue found—Test-UrlsWithVirusTotal correctly aborts when filtering returns no safe URLs.The verification confirms that
Test-UrlsWithVirusTotal(lib/virustotal.ps1:343-345) aborts with a clear error message if no URLs pass the VirusTotal check. The foreach loop at libexec/scoop-download.ps1:115 will never receive an empty array because the script terminates before that point. The code handles this scenario correctly and safely.
…picious counts, undetected, unsafe and total
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
lib/virustotal.ps1 (1)
5-13: Script-scoped exit code remains invisible to callers.This issue was flagged in a previous review and is still unresolved. Callers of functions in this module cannot read
$script:exit_codebecause script scope is private. Consider returning the exit code fromvirustotal_check_appor using$global:exit_codeinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/virustotal.ps1(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T19:13:23.481Z
Learnt from: felicijus
PR: ScoopInstaller/Scoop#6525
File: lib/virustotal.ps1:215-221
Timestamp: 2025-10-16T19:13:23.481Z
Learning: In PowerShell, `-inotin` is a valid case-insensitive containment comparison operator that checks if a value is not contained in a collection. The operator family includes `-notin` (case-sensitive), `-inotin` (case-insensitive), and `-cnotin` (case-sensitive with culture-aware comparison).
Applied to files:
lib/virustotal.ps1
🔇 Additional comments (5)
lib/virustotal.ps1 (5)
15-21: LGTM!The URL-to-ID conversion correctly implements VirusTotal's base64url encoding (RFC 4648 §5).
92-134: LGTM!The URL report retrieval logic correctly handles both completed analyses and in-progress scans.
147-202: LGTM!The submission logic with single-retry rate-limit handling is appropriate. The
$retryingparameter prevents infinite recursion.
256-263: Remove the review comment - no scope issue exists.The
$matchesautomatic variable from the regex match at line 218 correctly persists to line 259. PowerShell's try-catch blocks do not create new variable scopes (only function calls do), and neitherGet-VirusTotalResultByUrlnorGet-VirusTotalResultByHashperform-matchoperations that would overwrite$matches. The variable remains accessible in the nested try block as intended.
310-310: Disregard this review comment.The use of
script:urlat line 310 is correct. The architecture uses dot-sourcing where the entry point (./libexec/scoop-virustotal.ps1) importsmanifest.ps1into the script scope, making theurlfunction available tovirustotal.ps1and other library files via thescript:scope prefix. This pattern is consistently used inlib/hash.ps1as well, confirming it's intentional design rather than an error.Likely an incorrect or invalid review comment.
…ing 'p' in 'Report')
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
lib/virustotal.ps1 (7)
5-14: Script-scoped exit code remains invisible to callers.This is the same issue flagged in previous reviews. All public helpers update
$script:exit_code, but consumers (e.g.,scoop-virustotal.ps1) cannot access it because script scope is private. Either expose it globally or return the exit code fromvirustotal_check_app.
88-88: Incorrect variable reference.This issue from previous reviews is still present. The line uses
$exit_codeinstead of$script:exit_codeon the right-hand side.Apply this diff:
- $Script:exit_code = $exit_code -bor $script:_ERR_UNSAFE + $script:exit_code = $script:exit_code -bor $script:_ERR_UNSAFE
209-209: Incorrect variable reference.This issue from previous reviews is still present. The line uses
$_ERR_NO_API_KEYinstead of$script:_ERR_NO_API_KEY.Apply this diff:
- " scoop config virustotal_api_key <API key>") $_ERR_NO_API_KEY + " scoop config virustotal_api_key <API key>") $script:_ERR_NO_API_KEY
239-239: Incorrect variable reference.This issue from previous reviews is still present. The line uses
$exit_codeinstead of$script:exit_codeon the right-hand side.Apply this diff:
- $script:exit_code = $exit_code -bor $script:_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTION
273-273: Incorrect variable reference.This issue from previous reviews is still present. The line uses
$exit_codeinstead of$script:exit_codeon the right-hand side.Apply this diff:
- $script:exit_code = $exit_code -bor $script:_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTION
294-294: Incorrect variable reference.This issue from previous reviews is still present. The line uses
$exit_codeinstead of$script:exit_codeon the right-hand side.Apply this diff:
- $script:exit_code = $exit_code -bor $script:_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTION
335-340: Add null safety check for FileReport.Unsafe.This issue from previous reviews is still valid. The code assumes
FileReport.Unsafealways exists and is numeric, but if the report structure varies or the check fails, this property might be null or missing.Apply this diff:
- if ($file_report.'FileReport.Unsafe' -eq 0) { + if ($null -ne $file_report.'FileReport.Unsafe' -and $file_report.'FileReport.Unsafe' -eq 0) { info "$app`: Safe URL: $url" $safe_urls += $url } else {
🧹 Nitpick comments (1)
lib/virustotal.ps1 (1)
323-349: Consider handling null/empty reports more explicitly.When
Check-VirusTotalUrlencounters errors (lines 248, 276, 282, 303), it returns nothing. TheForEach-Objecton line 331 safely handles this (no-op), but you might want to track failed checks separately to provide clearer error messages to users about which URLs failed vs which were unsafe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/virustotal.ps1(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T19:13:23.481Z
Learnt from: felicijus
PR: ScoopInstaller/Scoop#6525
File: lib/virustotal.ps1:215-221
Timestamp: 2025-10-16T19:13:23.481Z
Learning: In PowerShell, `-inotin` is a valid case-insensitive containment comparison operator that checks if a value is not contained in a collection. The operator family includes `-notin` (case-sensitive), `-inotin` (case-insensitive), and `-cnotin` (case-sensitive with culture-aware comparison).
Applied to files:
lib/virustotal.ps1
🔇 Additional comments (3)
lib/virustotal.ps1 (3)
15-21: LGTM!The base64-url encoding logic is correct and follows the standard pattern for VirusTotal URL IDs.
92-134: LGTM!The URL report query logic correctly handles cases where the hash is not available and provides appropriate user feedback.
310-310: ****The
script:urlfunction is properly available. The entry pointscoop-virustotal.ps1explicitly dot-sourceslib/manifest.ps1(which defines theurlfunction) before loadinglib/virustotal.ps1, making the function accessible in the script scope at runtime. This is a standard module loading pattern consistently used throughout the codebase.Likely an incorrect or invalid review comment.
| if ($url_report.'UrlReport.Hash' -and ($file_report_not_found -eq $true) -and $hash) { | ||
| try { | ||
| $file_report = Get-VirusTotalResultByHash $url_report.'UrlReport.Hash' $url $app $api_key | ||
| if ($file_report.'FileReport.Hash' -ieq $matches['hash']) { |
There was a problem hiding this comment.
Incorrect hash comparison.
The code compares against $matches['hash'], but $matches was populated from the regex match at line 218 and may be out of scope or overwritten by nested try/catch blocks. Use the local $hash variable instead.
Apply this diff:
- if ($file_report.'FileReport.Hash' -ieq $matches['hash']) {
+ if ($file_report.'FileReport.Hash' -ieq $hash) {🤖 Prompt for AI Agents
In lib/virustotal.ps1 around line 259, the comparison uses $matches['hash']
which may be out of scope or overwritten; replace that usage with the local
$hash variable (the one populated from the regex at line 218) so the file report
is compared to the intended hash. Ensure the $hash variable is in scope at this
location and update the if condition to compare $file_report.'FileReport.Hash'
-ieq $hash.
There was a problem hiding this comment.
Out of scoop since this code was just copied to the lib\virustotal.ps1.
There was a problem hiding this comment.
@felicijus, understood! Since this is a refactoring PR and the code was copied as-is, I can see why fixing this would be out of scope.
Would you like me to open a follow-up issue to track this potential bug with the $matches['hash'] comparison? That way it can be addressed in a separate PR focused on bug fixes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/download.ps1 (1)
685-690: Consider removing the commented-out code.Since
Get-RemoteFileSizehas been moved tolib/helper/file-information.ps1and is now imported (line 5), the commented-out function definition is redundant and can be removed to reduce clutter.Apply this diff:
-# function Get-RemoteFileSize ($Uri) { -# $response = Invoke-WebRequest -Uri $Uri -Method HEAD -UseBasicParsing -# if (!$response.Headers.StatusCode) { -# $response.Headers.'Content-Length' | ForEach-Object { [int]$_ } -# } -# } -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bin/checkhashes.ps1(1 hunks)lib/autoupdate.ps1(1 hunks)lib/download.ps1(6 hunks)lib/helper/file-information.ps1(1 hunks)lib/helper/hash.ps1(1 hunks)lib/virustotal.ps1(1 hunks)libexec/scoop-download.ps1(3 hunks)libexec/scoop-info.ps1(1 hunks)libexec/scoop-update.ps1(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- libexec/scoop-info.ps1
🚧 Files skipped from review as they are similar to previous changes (2)
- bin/checkhashes.ps1
- lib/virustotal.ps1
🔇 Additional comments (20)
lib/helper/hash.ps1 (2)
3-14: LGTM!The function correctly retrieves the hash for a given URL by matching indices between the URL and hash arrays from the manifest. The abort on missing URL is appropriate for catching manifest inconsistencies.
16-28: LGTM!The function correctly parses multihash strings, defaults to SHA256 when no type is specified, validates against supported algorithms, and returns a consistent tuple format for success/failure cases.
lib/autoupdate.ps1 (1)
3-4: LGTM!The import correctly sources the new centralized hash helper, enabling reuse of
get_hashacross multiple modules.libexec/scoop-download.ps1 (5)
21-21: LGTM!The help text clearly documents the new VirusTotal option and appropriately warns users about potential performance impact.
25-25: LGTM!The import provides access to the centralized
hash_for_urlfunction used in the download flow.
35-41: LGTM!The option parsing correctly handles both CLI flags and config-based defaults, with appropriate precedence (CLI > config > false).
107-107: LGTM!Correctly propagates the VirusTotal check flag to the aria2 download path.
109-145: LGTM!The conditional URL filtering correctly applies VirusTotal checks only when enabled, allowing the download flow to skip flagged URLs while preserving existing behavior when the feature is disabled.
lib/download.ps1 (5)
3-6: LGTM!The imports correctly provide access to the refactored hash helpers, file information utilities, and new VirusTotal functionality.
10-60: LGTM!The VirusTotal integration is correctly implemented with backward-compatible defaults, proper flag propagation to both aria2 and non-aria2 paths, and conditional URL filtering that preserves existing behavior when disabled.
343-539: LGTM!The aria2 download function correctly applies VirusTotal filtering before building the download queue, ensuring that flagged URLs are never passed to aria2 when checks are enabled.
476-478: LGTM!The quote style updates align with PowerShell best practices (preferring single quotes when no interpolation is needed).
698-718: LGTM!The refinements to
url_remote_filenameimprove URL parsing logic for edge cases involving query parameters, version-like filenames, and URL fragments.libexec/scoop-update.ps1 (7)
14-14: LGTM!The help text clearly documents the new VirusTotal option with appropriate performance warning.
25-25: LGTM!The hash helper import provides access to
hash_for_urlused in the update flow.
34-43: LGTM!Option parsing correctly handles the VirusTotal flag with proper precedence (CLI flag > config > default false).
264-264: LGTM!The function signature update maintains backward compatibility with a sensible default.
306-344: LGTM!The update flow correctly integrates VirusTotal checking, with proper URL filtering in both aria2 and non-aria2 paths, ensuring flagged URLs are excluded from downloads when checks are enabled.
390-396: LGTM!The VirusTotal flag is correctly propagated to all installation paths, ensuring both the main app and dependencies are checked when enabled.
476-476: LGTM!The flag is correctly passed from the CLI options to each individual update invocation, completing the integration.
| function Get-RemoteFileSize ($Uri) { | ||
| $response = Invoke-WebRequest -Uri $Uri -Method HEAD -UseBasicParsing | ||
| if (!$response.Headers.StatusCode) { | ||
| $response.Headers.'Content-Length' | ForEach-Object { [int]$_ } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the StatusCode check logic.
The condition on line 5 checks !$response.Headers.StatusCode, but StatusCode is not a header—it's a property of the response object itself ($response.StatusCode). This check will likely always evaluate to true (since Headers.StatusCode doesn't exist), causing the function to return Content-Length unconditionally.
Apply this diff to fix the logic:
function Get-RemoteFileSize ($Uri) {
$response = Invoke-WebRequest -Uri $Uri -Method HEAD -UseBasicParsing
- if (!$response.Headers.StatusCode) {
+ if ($response.StatusCode -eq 200) {
$response.Headers.'Content-Length' | ForEach-Object { [int]$_ }
}
}📝 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.
| function Get-RemoteFileSize ($Uri) { | |
| $response = Invoke-WebRequest -Uri $Uri -Method HEAD -UseBasicParsing | |
| if (!$response.Headers.StatusCode) { | |
| $response.Headers.'Content-Length' | ForEach-Object { [int]$_ } | |
| } | |
| } | |
| function Get-RemoteFileSize ($Uri) { | |
| $response = Invoke-WebRequest -Uri $Uri -Method HEAD -UseBasicParsing | |
| if ($response.StatusCode -eq 200) { | |
| $response.Headers.'Content-Length' | ForEach-Object { [int]$_ } | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/helper/file-information.ps1 around lines 3 to 8, the condition
incorrectly checks $response.Headers.StatusCode (which doesn't exist) causing
the Content-Length to be returned unconditionally; update the check to inspect
the response.StatusCode property instead (for example ensure
$response.StatusCode -ge 200 -and $response.StatusCode -lt 300 or -eq 200) and
only read and cast $response.Headers.'Content-Length' to int when the status
code indicates success.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
lib/virustotal.ps1 (6)
259-263: Compare to $hash, not$matches['hash'].$matches may be stale/out of scope. Use the resolved $hash variable.
- if ($file_report.'FileReport.Hash' -ieq $matches['hash']) { + if ($file_report.'FileReport.Hash' -ieq $hash) {
88-90: Fix exit-code accumulation scope.RHS uses
$exit_code(undefined/outer). Use the script-scoped variable.- if ($unsafe -gt 0) { - $Script:exit_code = $exit_code -bor $script:_ERR_UNSAFE - } + if ($unsafe -gt 0) { + $script:exit_code = $script:exit_code -bor $script:_ERR_UNSAFE + }
205-213: Wrong variable in abort: use script-scoped error code.
$_ERR_NO_API_KEY→$script:_ERR_NO_API_KEY.- if (!$api_key) { - abort ("VirusTotal API key is not configured`n" + - " You could get one from https://www.virustotal.com/gui/my-apikey and set with`n" + - " scoop config virustotal_api_key <API key>") $_ERR_NO_API_KEY - } + if (!$api_key) { + abort ("VirusTotal API key is not configured`n" + + " You could get one from https://www.virustotal.com/gui/my-apikey and set with`n" + + " scoop config virustotal_api_key <API key>") $script:_ERR_NO_API_KEY + }
295-299: Fix exit-code scope in exception path (again).- $script:exit_code = $exit_code -bor $script:_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTION
240-241: Fix exit-code scope in exception path.- $script:exit_code = $exit_code -bor $script:_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTION
274-275: Fix exit-code scope in URL exception path.- $script:exit_code = $exit_code -bor $script:_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTION
🧹 Nitpick comments (8)
lib/helper/hash.ps1 (2)
3-14: Guard against URL/hash index mismatch.If URLs and hashes get out of sync, indexing may throw. Add a bounds check and clearer error.
function hash_for_url($manifest, $url, $arch) { @@ - $index = [array]::IndexOf($urls, $url) - if ($index -eq -1) { abort "Couldn't find hash in manifest for '$url'." } - - @($hashes)[$index] + $index = [array]::IndexOf($urls, $url) + if ($index -eq -1) { abort "Couldn't find hash in manifest for '$url'." } + if ($index -ge $hashes.Length) { abort "Hash entry missing for URL index $index in manifest." } + @($hashes)[$index] }
16-28: Validate hex length per algorithm (optional).Add simple length checks to catch malformed hashes early.
function get_hash([String] $multihash) { @@ - return $type, $hash.ToLower() + $hash = $hash.ToLower() + $expectedLen = switch ($type) { + 'md5' { 32 } + 'sha1' { 40 } + 'sha256' { 64 } + 'sha512' { 128 } + } + if ($hash -notmatch '^[0-9a-f]+$' -or $hash.Length -ne $expectedLen) { + return $null, "Invalid $type hash format." + } + return $type, $hash }libexec/scoop-download.ps1 (1)
106-108: Pass $bucket through aria2 path (avoid dynamic scope).Invoke-CachedAria2Download uses $bucket for messages but doesn’t accept it. Thread it explicitly to avoid ambient variable reliance.
- Invoke-CachedAria2Download $app $version $manifest $architecture $cachedir $manifest.cookie $use_cache $curr_check_hash $check_virustotal + Invoke-CachedAria2Download $app $version $manifest $bucket $architecture $cachedir $manifest.cookie $use_cache $curr_check_hash $check_virustotalYou’ll need the matching signature change in lib/download.ps1 (see comment there).
lib/download.ps1 (4)
20-28: Optional: Lazy-load VT module only when enabled.Dot-sourcing virustotal.ps1 always loads VT helpers. Consider importing only when $check_virustotal is true to reduce startup overhead.
22-22: Pass $bucket into aria2 helper.Signature currently omits $bucket but uses it internally. Add it to avoid dynamic scope leakage.
- Invoke-CachedAria2Download $app $version $manifest $architecture $dir $cookies $use_cache $check_hash $check_virustotal + Invoke-CachedAria2Download $app $version $manifest $bucket $architecture $dir $cookies $use_cache $check_hash $check_virustotal
343-351: Add $bucket parameter to aria2 helper to match usage.The function references $bucket in messages but doesn’t accept it.
-function Invoke-CachedAria2Download ($app, $version, $manifest, $architecture, $dir, $cookies = $null, $use_cache = $true, $check_hash = $true, $check_virustotal = $false) { +function Invoke-CachedAria2Download ($app, $version, $manifest, $bucket, $architecture, $dir, $cookies = $null, $use_cache = $true, $check_hash = $true, $check_virustotal = $false) {Also update both call sites (in this file and libexec/scoop-download.ps1) as shown in adjacent comments.
714-747: check_hash relies on ambient $url.Passing $url explicitly would de-couple this helper from caller context and fix misleading output when $url isn’t in scope.
-function check_hash($file, $hash, $app_name) { +function check_hash($file, $hash, $app_name, $url) { @@ - Write-Host $(url_remote_filename $url) -ForegroundColor Cyan -NoNewline + Write-Host $(url_remote_filename $url) -ForegroundColor Cyan -NoNewline @@ - $msg += "URL: $url`n" + $msg += "URL: $url`n"And pass $url at call sites:
- Invoke-ScoopDownload:
$ok, $err = check_hash "$dir\$fname" $manifest_hash $(show_app $app $bucket) $url- Invoke-CachedAria2Download:
$ok, $err = check_hash $data.$url.source $manifest_hash $(show_app $app $bucket) $urllib/virustotal.ps1 (1)
336-341: Null‑safety for FileReport.Unsafe.When only a URL report is available, this property can be null. Guard the comparison.
- if ($file_report.'FileReport.Unsafe' -eq 0) { + if ($null -ne $file_report.'FileReport.Unsafe' -and [int]$file_report.'FileReport.Unsafe' -eq 0) { info "$app`: Safe URL: $url" $safe_urls += $url } else { warn "$app`: Unsafe URL: $url" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
bin/checkhashes.ps1(1 hunks)lib/autoupdate.ps1(1 hunks)lib/download.ps1(5 hunks)lib/helper/file-information.ps1(1 hunks)lib/helper/hash.ps1(1 hunks)lib/virustotal.ps1(1 hunks)libexec/scoop-download.ps1(3 hunks)libexec/scoop-info.ps1(1 hunks)libexec/scoop-update.ps1(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/helper/file-information.ps1
- lib/autoupdate.ps1
- libexec/scoop-info.ps1
- libexec/scoop-update.ps1
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-16T19:13:23.481Z
Learnt from: felicijus
PR: ScoopInstaller/Scoop#6525
File: lib/virustotal.ps1:215-221
Timestamp: 2025-10-16T19:13:23.481Z
Learning: In PowerShell, `-inotin` is a valid case-insensitive containment comparison operator that checks if a value is not contained in a collection. The operator family includes `-notin` (case-sensitive), `-inotin` (case-insensitive), and `-cnotin` (case-sensitive with culture-aware comparison).
Applied to files:
lib/virustotal.ps1
🔇 Additional comments (2)
bin/checkhashes.ps1 (1)
45-45: Import looks good.Dot-sourcing the new hash helper is correct and aligns with later get_hash usage.
lib/download.ps1 (1)
21-26: Thread VT filtering in meta downloader correctly.VT filtering is good. Ensure $urls is built before filtering (already present here). No action needed.
| try { | ||
| $file_report = Get-VirusTotalResultByHash $url_report.'UrlReport.Hash' $url $app $api_key | ||
| $file_report.'App.Hash' = $hash | ||
| $file_report.'App.HashType' = $algo | ||
| $file_report.'UrlReport.Url' = $url_report.'UrlReport.Url' | ||
| $file_report | ||
| warn "$app`: Unable to check hash match for $url" | ||
| } catch [Exception] { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Spurious warning always prints.
This warn executes even on success right after outputting $file_report. Remove it or guard it.
- $file_report
- warn "$app`: Unable to check hash match for $url"
+ $file_report
+ # (optional) add warn only when a mismatch condition is detected elsewhere🤖 Prompt for AI Agents
In lib/virustotal.ps1 around lines 287 to 294, the warn statement is executed
unconditionally even when Get-VirusTotalResultByHash succeeds; remove the
unconditional warn or move/guard it so it only runs when an actual hash mismatch
or error occurs (e.g., perform the hash match check after receiving $file_report
and only call warn if the hashes differ, or else move the warn into the catch
block to run on exceptions).
There was a problem hiding this comment.
Out of scoop since this code was just copied to the lib\virustotal.ps1.
There was a problem hiding this comment.
@felicijus, understood! Since this is pre-existing code being extracted, it makes sense to keep this PR focused on the refactoring work.
Would you like me to create a follow-up issue to track fixing this spurious warning? That way it won't be forgotten and can be addressed in a future PR dedicated to fixing the logic.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
lib/virustotal.ps1 (3)
6-9: Fix exit code scoping and make it observable.Assignments inside functions create locals; the top-level $exit_code isn’t updated. Also, error-code constants should be script-scoped for consistency. Update scoping and usage.
Apply:
-$_ERR_UNSAFE = 2 -$_ERR_EXCEPTION = 4 -$_ERR_NO_INFO = 8 -$_ERR_NO_API_KEY = 16 +$script:_ERR_UNSAFE = 2 +$script:_ERR_EXCEPTION = 4 +$script:_ERR_NO_INFO = 8 +$script:_ERR_NO_API_KEY = 16 -$exit_code = 0 +$script:exit_code = 0- if ($unsafe -gt 0) { - $exit_code = $exit_code -bor $_ERR_UNSAFE - } + if ($unsafe -gt 0) { + $script:exit_code = $script:exit_code -bor $script:_ERR_UNSAFE + }- abort ("VirusTotal API key is not configured`n" + - " You could get one from https://www.virustotal.com/gui/my-apikey and set with`n" + - " scoop config virustotal_api_key <API key>") $_ERR_NO_API_KEY + abort ("VirusTotal API key is not configured`n" + + " You could get one from https://www.virustotal.com/gui/my-apikey and set with`n" + + " scoop config virustotal_api_key <API key>") $script:_ERR_NO_API_KEY- $exit_code = $exit_code -bor $_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTION- $exit_code = $exit_code -bor $_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTION- $exit_code = $exit_code -bor $_ERR_EXCEPTION + $script:exit_code = $script:exit_code -bor $script:_ERR_EXCEPTIONOptionally expose a getter so callers can read the final code:
function Get-VirusTotalExitCode { $script:exit_code }Also applies to: 14-15, 88-91, 210-211, 240-241, 274-275, 295-296
259-263: Compare to $hash, not $matches['hash'].$matches may be stale or unset; use the normalized $hash variable.
Apply:
- if ($file_report.'FileReport.Hash' -ieq $matches['hash']) { + if ($file_report.'FileReport.Hash' -ieq $hash) {
292-294: Remove/guard spurious warning.warn runs unconditionally even on success, misleading users.
Apply:
- $file_report - warn "$app`: Unable to check hash match for $url" + $file_report + # warn only when a mismatch is detected elsewhere
🧹 Nitpick comments (2)
lib/virustotal.ps1 (2)
189-195: Optional: add jitter and a cap to backoff.Simple linear sleep can herd; add jitter and cap to improve throughput under rate limits.
Example:
- Start-Sleep -s (60 + $script:requests) + $base = 45 + $jitter = Get-Random -Minimum 5 -Maximum 20 + $delay = [Math]::Min(120, $base + $script:requests + $jitter) + Start-Sleep -Seconds $delay
31-33: Optional: set timeouts on HTTP calls.Prevent hangs due to stalled connections.
Apply:
- $response = Invoke-WebRequest -Uri $api_url -Method GET -Headers $headers -UseBasicParsing + $response = Invoke-WebRequest -Uri $api_url -Method GET -Headers $headers -UseBasicParsing -TimeoutSec 60- $response = Invoke-WebRequest -Uri $api_url -Method GET -Headers $headers -UseBasicParsing + $response = Invoke-WebRequest -Uri $api_url -Method GET -Headers $headers -UseBasicParsing -TimeoutSec 60- $response = Invoke-WebRequest -Uri $api_url -Method POST -Headers $headers -ContentType $content_type -Body $body -UseBasicParsing + $response = Invoke-WebRequest -Uri $api_url -Method POST -Headers $headers -ContentType $content_type -Body $body -UseBasicParsing -TimeoutSec 60Also applies to: 100-101, 166-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/virustotal.ps1(1 hunks)libexec/scoop-download.ps1(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libexec/scoop-download.ps1
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-16T19:13:23.481Z
Learnt from: felicijus
PR: ScoopInstaller/Scoop#6525
File: lib/virustotal.ps1:215-221
Timestamp: 2025-10-16T19:13:23.481Z
Learning: In PowerShell, `-inotin` is a valid case-insensitive containment comparison operator that checks if a value is not contained in a collection. The operator family includes `-notin` (case-sensitive), `-inotin` (case-insensitive), and `-cnotin` (case-sensitive with culture-aware comparison).
Applied to files:
lib/virustotal.ps1
📚 Learning: 2025-08-31T01:39:35.684Z
Learnt from: z-Fng
PR: ScoopInstaller/Scoop#6473
File: libexec/scoop-download.ps1:135-135
Timestamp: 2025-08-31T01:39:35.684Z
Learning: In the Scoop codebase, `Invoke-ScoopDownload` in lib/download.ps1 calls `abort` when hash verification fails, which terminates script execution completely rather than trying remaining mirrors. The scoop-download.ps1 script should maintain consistency with this behavior by using `continue app_loop` to skip to the next app rather than trying remaining mirrors for the current app.
Applied to files:
lib/virustotal.ps1
🔇 Additional comments (3)
lib/virustotal.ps1 (3)
16-22: LGTM: stable URL ID encoding.URL-safe Base64 conversion looks correct.
69-86: Good: preserve numeric counts and add engine lists.Keeps Malicious/Suspicious totals while exposing engine names; avoids downstream breakage.
345-347: LGTM: abort only when no URLs pass.Matches PR objective to proceed when at least one URL is safe.
| $body = "url=$encoded_url" | ||
| $result = Invoke-WebRequest -Uri $api_url -Method POST -Headers $headers -ContentType $content_type -Body $body -UseBasicParsing | ||
| if ($result.StatusCode -eq 200) { | ||
| $id = ((json_path $result '$.data.id') -split '-')[1] | ||
| $url_report_url = "https://www.virustotal.com/gui/url/$id" | ||
| $fileSize = Get-RemoteFileSize $url |
There was a problem hiding this comment.
Parse JSON from the response content, not the WebResponse object.
POST branch passes the IWR response object to json_path; use .Content.
Apply:
- $result = Invoke-WebRequest -Uri $api_url -Method POST -Headers $headers -ContentType $content_type -Body $body -UseBasicParsing
- if ($result.StatusCode -eq 200) {
- $id = ((json_path $result '$.data.id') -split '-')[1]
+ $response = Invoke-WebRequest -Uri $api_url -Method POST -Headers $headers -ContentType $content_type -Body $body -UseBasicParsing
+ if ($response.StatusCode -eq 200) {
+ $json = $response.Content
+ $id = ((json_path $json '$.data.id') -split '-')[1]🤖 Prompt for AI Agents
In lib/virustotal.ps1 around lines 165 to 170, the code passes the full
Invoke-WebRequest response object to json_path; update the call to use the
response body string (e.g. $result.Content) so json_path parses the JSON text,
e.g. replace json_path $result '$.data.id' with json_path $result.Content
'$.data.id' (or pipe $result.Content | ConvertFrom-Json and extract the field)
and keep the rest of the logic unchanged.
There was a problem hiding this comment.
Out of scoop since this code was just copied to the lib\virustotal.ps1.
There was a problem hiding this comment.
@felicijus, understood! Since this is a refactor/extraction PR and the code was copied as-is, it makes sense to keep the scope focused. The inconsistency can be addressed in a follow-up if needed. Thanks for clarifying!
| if ($_.Exception.Response.StatusCode -eq 404) { | ||
| $file_report_not_found = $true | ||
| warn "$app`: File report not found. Will search by url instead." | ||
| } else { | ||
| warn "$app`: VirusTotal file report query failed`: $($_.Exception.Message)" | ||
| if ($_.Exception.Response) { | ||
| warn "`tAPI returned $($_.Exception.Response.StatusCode)" | ||
| } | ||
| return |
There was a problem hiding this comment.
Guard against null Exception.Response before reading StatusCode.
Accessing $_.Exception.Response.StatusCode can NPE when Response is null.
Apply:
- if ($_.Exception.Response.StatusCode -eq 404) {
+ if ($_.Exception.Response -and $(_.Exception.Response.StatusCode) -eq 404) {and:
- if ($_.Exception.Response) {
+ if ($_.Exception.Response) {
warn "`tAPI returned $($_.Exception.Response.StatusCode)"
}Repeat the same pattern in the later catch block.
Also applies to: 275-283
🤖 Prompt for AI Agents
In lib/virustotal.ps1 around lines 241 to 249, the catch block reads
$_.Exception.Response.StatusCode without checking if Response is $null which can
cause a null reference; update the condition to first test that
$_.Exception.Response is not $null (e.g. use -and or a nested if) before
accessing StatusCode and adjust logging accordingly (check Response exists, then
check StatusCode==404, else log message and, if Response exists, log its
StatusCode); apply the identical null-guard pattern to the later catch block
around lines 275 to 283.
There was a problem hiding this comment.
Out of scoop since this code was just copied to the lib\virustotal.ps1.
There was a problem hiding this comment.
@felicijus Understood! Since this is refactored code and the issue pre-dates this PR, it makes sense to keep the scope focused on the extraction itself.
If you'd like to track this null-guard improvement for a future fix, I can open a follow-up issue for you. Just let me know!
|
✅ Done for now, not sure about testing since there are currently non for the Virustotal parts. The other Issues still unresolved would be other bug fixes I guess... To keep this PR at least a bit contained. |
1 participant
Description
Motivation and Context
Centralize VirusTotal interactions in a reusable library so install/update/download workflows can optionally pre-validate remote files and block downloads flagged as malicious or suspicious.
Look:
Relates to virus scanning feature and prior virustotal command refactors.
Closes #6521
How Has This Been Tested?
Checklist:
developbranch.Summary by CodeRabbit
New Features
Refactor