fix(download): 限制依赖库下载路径#3035
Open
lhx077 wants to merge 1 commit into
Open
Conversation
pcl-ce-automation
Bot
added
🛠️ 等待审查
审阅者指南添加了一个强化的辅助方法，用于解析库下载路径，并将其接入 Minecraft 的库下载代码中，以防止路径遍历以及逃逸出预期的 libraries 目录。 文件级变更
提示与命令与 Sourcery 交互
自定义你的使用体验前往你的 控制面板 以：
获取帮助Original review guide in EnglishReviewer's GuideAdds a hardened helper for resolving library download paths and wires it into Minecraft library download code to prevent path traversal and escaping the intended libraries directory. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我这边有一些整体性的反馈：
- 在
McLibGetSafeDownloadPath中，Path.IsPathRooted已经会拒绝驱动器盘符和 UNC 根路径，因此额外的StartsWith("\")和驱动器盘符正则检查看起来是多余的；建议简化这部分逻辑，只依赖Path/Files相关的辅助方法。 - 你在分割路径前先把分隔符统一规范为反斜杠，然后再同时按
\和/来 split；由于/已经被替换过了，split 可以简化为只按\分割，或者在规范化之前就进行 split，以避免多余的分隔符处理。 - 用于检测驱动器盘符的
Regex.IsMatch在每次调用时都会执行；如果保留这一检查，建议改为简单的字符串判断，或者使用静态预编译的正则，以减少在热点路径上的开销。
面向 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- In `McLibGetSafeDownloadPath`, `Path.IsPathRooted` already rejects drive and UNC roots, so the extra `StartsWith("\")` and drive-letter regex checks look redundant; consider simplifying this logic to rely on `Path`/`Files` helpers only.
- You normalize to backslashes before splitting on both `\` and `/`; since `/` has already been replaced, the split can be simplified to just `\` or applied before normalization to avoid redundant separators.
- The `Regex.IsMatch` for drive letters is executed on every call; if you keep it, consider replacing it with a simple string check or a static precompiled regex to reduce overhead in hot paths.帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进后续的审查。
Original comment in English
Hey - I've left some high level feedback:
- In
McLibGetSafeDownloadPath,Path.IsPathRootedalready rejects drive and UNC roots, so the extraStartsWith("\")and drive-letter regex checks look redundant; consider simplifying this logic to rely onPath/Fileshelpers only. - You normalize to backslashes before splitting on both
\and/; since/has already been replaced, the split can be simplified to just\or applied before normalization to avoid redundant separators. - The
Regex.IsMatchfor drive letters is executed on every call; if you keep it, consider replacing it with a simple string check or a static precompiled regex to reduce overhead in hot paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `McLibGetSafeDownloadPath`, `Path.IsPathRooted` already rejects drive and UNC roots, so the extra `StartsWith("\")` and drive-letter regex checks look redundant; consider simplifying this logic to rely on `Path`/`Files` helpers only.
- You normalize to backslashes before splitting on both `\` and `/`; since `/` has already been replaced, the split can be simplified to just `\` or applied before normalization to avoid redundant separators.
- The `Regex.IsMatch` for drive letters is executed on every call; if you keep it, consider replacing it with a simple string check or a static precompiled regex to reduce overhead in hot paths.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 872b249674
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? McLibGet((string)library["name"], customMcFolder: customMcFolder) | ||
| : Path.Combine(customMcFolder, "libraries", library["downloads"]["artifact"]["path"].ToString() | ||
| .Replace("/", @"\")), | ||
| : McLibGetSafeDownloadPath(customMcFolder, library["downloads"]["artifact"]["path"]), |
There was a problem hiding this comment.
When an untrusted manifest supplies a downloads.artifact.path that McLibGetSafeDownloadPath rejects, this call is still inside the broad try whose catch below adds a fallback token using localPath derived from the same manifest's name. That means rejected metadata does not stop processing; if the same library name contains traversal separators in its non-group fields, the downloader can still receive a path from McLibGet outside libraries, bypassing the new containment check. Let the IOException propagate or skip the library instead of falling back after a containment failure.
Useful? React with 👍 / 👎.
SALTWOOD
changed the title
LinQingYuu
requested changes
Jun 10, 2026
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
3 participants
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.
Motivation
downloads.*.pathvalues from escaping the intendedlibrariesfolder and enabling arbitrary file writes.Description
McLibGetSafeDownloadPath(customMcFolder, JsonNode)that normalizes the metadata path, rejects absolute/drive/UNC paths and..traversal, and verifies the resolved path is insidecustomMcFolder\librariesusingFiles.IsPathWithinDirectory.Path.Combine(customMcFolder, "libraries", ...)usages for artifact and native-classifier paths withMcLibGetSafeDownloadPathand addedusing PCL.Core.IOto the file.IOExceptionfor invalid or escaping paths to stop processing untrusted metadata.pathis preserved by keeping theMcLibGetfallback.Testing
git diff --checkand it completed without whitespace/format errors.dotnet --infoto run build/tests butdotnetis not available in the execution environment, so compile-time tests were not run.Codex Task
由 Sourcery 提供的摘要
加强对 Minecraft 库下载的处理，防止不安全的元数据路径逃离预期的 libraries 目录。
错误修复：
功能改进：
Original summary in English
Summary by Sourcery
Harden Minecraft library download handling to prevent unsafe metadata paths from escaping the intended libraries directory.
Bug Fixes:
Enhancements: