fix: bat security#658
Open
Yundi339 wants to merge 1 commit into
Open
Conversation
Summary by CodeRabbitBug Fixes
Chores
总览PR 修改了三个 Windows 驱动安装脚本，强化了数据解析、路径处理和临时文件管理的安全性与可靠性。gamepad 脚本改用 JSON 结构化解析替代文本流提取；migrate-config 脚本引入代码注入防护并优化日志清理；vsink 脚本采用随机临时目录替代固定路径。 变更内容Windows 脚本安全性强化
代码审查工作量估计🎯 3 (中等复杂度) | ⏱️ ~20 分钟 可能相关的 PR
🚥 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 |
![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 (1)
src_assets/windows/misc/vsink/install-vsink.bat (1)
50-58: ⚡ Quick win建议检查
mkdir的执行结果。防御性清理逻辑（第 52-57 行）设计良好，能有效阻止 hardlink/junction 攻击。但第 58 行的
mkdir未检查错误码——如果创建失败（如权限问题或极低概率的竞态），脚本会继续执行并在后续步骤产生令人困惑的错误。♻️ 建议的改进
mkdir "%temp_dir%" +if %errorLevel% neq 0 ( + echo ERROR: Could not create temp directory: %temp_dir% + pause + exit /b 1 +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src_assets/windows/misc/vsink/install-vsink.bat` around lines 50 - 58, The mkdir call that creates the temp directory using the variable %temp_dir% is not checked for failure; after mkdir "%temp_dir%" add a verification (e.g., test ERRORLEVEL or check if "%temp_dir%" exists) and on failure emit a clear error via echo and exit /b 1 so the script does not continue when the temp directory was not created successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src_assets/windows/misc/gamepad/install-gamepad.bat`:
- Line 65: The for /F line embeds the batch variable %release_json% directly
into the PowerShell -Command string, which breaks when the path contains quotes
and is a security/quoting risk; instead export the path into an environment
variable (e.g., set "RELEASE_JSON=%release_json%") and then invoke PowerShell
reading $env:RELEASE_JSON inside the -Command so PowerShell parses the path
natively and you avoid injecting cmd quoting into the command; update the for /F
invocation that sets browser_download_url and the surrounding logic to use the
environment variable (and remove direct %release_json% usage) so Get-Content
uses $env:RELEASE_JSON safely.
In `@src_assets/windows/misc/migration/migrate-config.bat`:
- Around line 58-63: The batch currently runs the PowerShell replacement
unconditionally which causes errors when apps.json is missing; change
migrate-config.bat so that after setting MIGRATE_APPS_JSON it checks for file
existence and only runs the PowerShell Replace when the file exists (e.g. use a
conditional check like if exist "%MIGRATE_APPS_JSON%" ... or inside PowerShell
use Test-Path -LiteralPath $p before performing the (Get-Content
...).Replace(... ) | Set-Content -LiteralPath $p); keep the existing use of
MIGRATE_APPS_JSON and the -LiteralPath usage and still clear MIGRATE_APPS_JSON
afterwards.
---
Nitpick comments:
In `@src_assets/windows/misc/vsink/install-vsink.bat`:
- Around line 50-58: The mkdir call that creates the temp directory using the
variable %temp_dir% is not checked for failure; after mkdir "%temp_dir%" add a
verification (e.g., test ERRORLEVEL or check if "%temp_dir%" exists) and on
failure emit a clear error via echo and exit /b 1 so the script does not
continue when the temp directory was not created successfully.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0da4a07a-437e-4f58-9ea2-34e8c272b54c
📒 Files selected for processing (3)
src_assets/windows/misc/gamepad/install-gamepad.batsrc_assets/windows/misc/migration/migrate-config.batsrc_assets/windows/misc/vsink/install-vsink.bat
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Windows
🔇 Additional comments (3)
src_assets/windows/misc/vsink/install-vsink.bat (1)
41-48: LGTM!使用 GUID 生成随机目录名，配合
%RANDOM%作为降级方案，有效防止了攻击者预先占位固定路径的攻击向量。src_assets/windows/misc/migration/migrate-config.bat (1)
76-80: LGTM!src_assets/windows/misc/gamepad/install-gamepad.bat (1)
51-58: LGTM!Also applies to: 67-67
| rem findstr + substring stripping. The previous approach silently produced an | ||
| rem invalid URL if the JSON layout shifted, or if any asset name contained | ||
| rem characters that confused `set` parsing. | ||
| for /F "tokens=* USEBACKQ delims=" %%F in (`powershell -NoProfile -Command "try { $r = Get-Content -LiteralPath '%release_json%' -Raw ^| ConvertFrom-Json; $a = $r.assets ^| Where-Object { $_.name -like '*.exe' } ^| Select-Object -First 1; if ($a -and $a.browser_download_url) { $a.browser_download_url } } catch { }"`) do set "browser_download_url=%%F" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 只读验证：定位是否仍存在将批处理变量内联到 PowerShell 单引号字符串的模式
rg -n -C2 --iglob "*.bat" "Get-Content -LiteralPath '%[A-Za-z0-9_]+%'"Repository: AlkaidLab/foundation-sunshine
Length of output: 860
🏁 Script executed:
cat -n src_assets/windows/misc/gamepad/install-gamepad.batRepository: AlkaidLab/foundation-sunshine
Length of output: 4892
避免在 PowerShell 命令字符串中直接嵌入批处理路径变量。
Line 65 将 %release_json% 直接拼接到 PowerShell -Command 参数的单引号字符串内。当该路径包含单引号字符时（如通过网络路径或特殊环境），会破坏 PowerShell 字符串边界，导致解析失败甚至命令注入风险。建议改用环境变量传参方案，由 PowerShell 本地解析环境变量，避免 cmd 层面的嵌入歧义。
建议修改
set "release_json=%temp_dir%\vigembus_release.json"
curl -f -s -L --connect-timeout 10 --max-time 20 !proxy! -o "%release_json%" "%latest_release_url%"
if errorlevel 1 (
echo ERROR: Could not fetch ViGEmBus release metadata.
exit /b 1
)
rem Step 2: parse the JSON via PowerShell ConvertFrom-Json instead of fragile
rem findstr + substring stripping. The previous approach silently produced an
rem invalid URL if the JSON layout shifted, or if any asset name contained
rem characters that confused `set` parsing.
+set "RELEASE_JSON=%release_json%"
-for /F "tokens=* USEBACKQ delims=" %%F in (`powershell -NoProfile -Command "try { $r = Get-Content -LiteralPath '%release_json%' -Raw ^| ConvertFrom-Json; $a = $r.assets ^| Where-Object { $_.name -like '*.exe' } ^| Select-Object -First 1; if ($a -and $a.browser_download_url) { $a.browser_download_url } } catch { }"`) do set "browser_download_url=%%F"
+for /F "tokens=* USEBACKQ delims=" %%F in (`powershell -NoProfile -Command "try { $r = Get-Content -LiteralPath $env:RELEASE_JSON -Raw ^| ConvertFrom-Json; $a = $r.assets ^| Where-Object { $_.name -like '*.exe' } ^| Select-Object -First 1; if ($a -and $a.browser_download_url) { $a.browser_download_url } } catch { }"`) do set "browser_download_url=%%F"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src_assets/windows/misc/gamepad/install-gamepad.bat` at line 65, The for /F
line embeds the batch variable %release_json% directly into the PowerShell
-Command string, which breaks when the path contains quotes and is a
security/quoting risk; instead export the path into an environment variable
(e.g., set "RELEASE_JSON=%release_json%") and then invoke PowerShell reading
$env:RELEASE_JSON inside the -Command so PowerShell parses the path natively and
you avoid injecting cmd quoting into the command; update the for /F invocation
that sets browser_download_url and the surrounding logic to use the environment
variable (and remove direct %release_json% usage) so Get-Content uses
$env:RELEASE_JSON safely.
Comment on lines
+58
to
+63
| rem Fix apps.json image path values that point at the old covers directory. | ||
| rem Pass the path via environment to PowerShell and use -LiteralPath to avoid | ||
| rem PowerShell code injection if the install path contains characters like ' or $. | ||
| set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json" | ||
| powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p" | ||
| set "MIGRATE_APPS_JSON=" |
There was a problem hiding this comment.
缺少 apps.json 存在性保护会触发无意义报错
Line 62 在 apps.json 不存在但 covers 存在时仍会执行替换逻辑，导致 PowerShell 报错。建议先判断文件是否存在再执行替换。
🔧 建议修改
- set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json"
- powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p"
- set "MIGRATE_APPS_JSON="
+ if exist "%NEW_DIR%\apps.json" (
+ set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json"
+ powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p"
+ set "MIGRATE_APPS_JSON="
+ )📝 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.
Suggested change
| rem Fix apps.json image path values that point at the old covers directory. | |
| rem Pass the path via environment to PowerShell and use -LiteralPath to avoid | |
| rem PowerShell code injection if the install path contains characters like ' or $. | |
| set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json" | |
| powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p" | |
| set "MIGRATE_APPS_JSON=" | |
| rem Fix apps.json image path values that point at the old covers directory. | |
| rem Pass the path via environment to PowerShell and use -LiteralPath to avoid | |
| rem PowerShell code injection if the install path contains characters like ' or $. | |
| if exist "%NEW_DIR%\apps.json" ( | |
| set "MIGRATE_APPS_JSON=%NEW_DIR%\apps.json" | |
| powershell -NoProfile -Command "$p = $env:MIGRATE_APPS_JSON; (Get-Content -LiteralPath $p).Replace('.\/covers\/', '.\/config\/covers\/') | Set-Content -LiteralPath $p" | |
| set "MIGRATE_APPS_JSON=" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src_assets/windows/misc/migration/migrate-config.bat` around lines 58 - 63,
The batch currently runs the PowerShell replacement unconditionally which causes
errors when apps.json is missing; change migrate-config.bat so that after
setting MIGRATE_APPS_JSON it checks for file existence and only runs the
PowerShell Replace when the file exists (e.g. use a conditional check like if
exist "%MIGRATE_APPS_JSON%" ... or inside PowerShell use Test-Path -LiteralPath
$p before performing the (Get-Content ...).Replace(... ) | Set-Content
-LiteralPath $p); keep the existing use of MIGRATE_APPS_JSON and the
-LiteralPath usage and still clear MIGRATE_APPS_JSON afterwards.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…security