fix: restrict path traversal check to file scheme in DEnumerator buildUrl - #378
Merged
deepin-bot[bot] merged 1 commit intoAug 5, 2026
Conversation
…dUrl 1. DEnumeratorPrivate::buildUrl() 对所有 scheme 的文件名做路径遍历检查，导致 gio trash:/// 后端使用反斜杠分隔的扁平文件名被误判为恶意路径并返回空 URL; 2. 将路径遍历检查限制为仅对 file:/// 或无 scheme 的本地文件系统生效，gio trash:/// 等虚拟文件系统的合法文件名不受影响; 3. 修复手动分区场景下 /media 挂载点的回收站文件无法显示和清空的问题; Log: 修复 buildUrl 路径遍历安全检查误伤 gio trash 反斜杠文件名导致回收站无法显示和清空的问题 PMS: BUG-372733 Bug: https://pms.uniontech.com/bug-view-372733.html
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Johnson-zs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRestricts the path traversal safeguard in DEnumeratorPrivate::buildUrl() to only apply for local file-system URLs (file scheme or no scheme), so that gio trash:/// backends using backslash-based flat filenames are no longer incorrectly rejected. Flow diagram for scheme-restricted path traversal check in buildUrlflowchart TD
A[buildUrl called with url and fileName] --> B[get scheme from url]
B --> C{scheme is empty or file}
C --> D[perform path traversal check on fileNameBa]:::check
C --> E[skip path traversal check]:::skip
D --> F[return QUrl if traversal pattern found]
E --> G[continue building path]
D --> G[continue building path]
classDef check fill:#ffe0e0,stroke:#cc0000
classDef skip fill:#e0f0ff,stroke:#0044aa
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 - I've found 1 issue, and left some high level feedback:
- Instead of checking
scheme.isEmpty() || scheme == QLatin1String("file"), consider usingurl.isLocalFile()to more robustly detect local file URLs and avoid relying on an empty scheme as a proxy. - Since the trash backend’s backslash usage is now explicitly exempted from traversal checks, it may be worth briefly noting in the function’s interface or a higher-level comment that traversal checks are scheme-dependent, to avoid future regressions reintroducing the same issue.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of checking `scheme.isEmpty() || scheme == QLatin1String("file")`, consider using `url.isLocalFile()` to more robustly detect local file URLs and avoid relying on an empty scheme as a proxy.
- Since the trash backend’s backslash usage is now explicitly exempted from traversal checks, it may be worth briefly noting in the function’s interface or a higher-level comment that traversal checks are scheme-dependent, to avoid future regressions reintroducing the same issue.
## Individual Comments
### Comment 1
<location path="src/dfm-io/dfm-io/denumerator.cpp" line_range="391-392" />
<code_context>
+ // gio 的 trash:/// 后端对非用户主目录挂载点的回收站文件，使用反斜杠分隔的扁平路径
+ // 作为 GFileInfo 的 standard::name（例如 "\media\user\dev\.Trash-1000\files\x"），
+ // 这是合法的 trash 文件名而非恶意路径，故不应对其做路径遍历拦截。
+ const QString scheme = url.scheme();
+ if (scheme.isEmpty() || scheme == QLatin1String("file")) {
+ if (fileNameBa.contains("../") || fileNameBa.contains("..\\") || fileNameBa.startsWith("..")) {
+ return QUrl();
</code_context>
<issue_to_address>
**🚨 issue (security):** Consider restricting the relaxed check to specific non-local schemes instead of all non-file URLs
The current condition disables traversal checks for every non-empty, non-`file` scheme. For custom schemes that still map to filesystem-like backends, this could allow names with `../` or `..\` to pass unchecked. Since the need is specifically for `trash:///`, consider limiting the relaxed behavior to that scheme (or an explicit allowlist of schemes) rather than applying it to all non-file URLs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+391
to
+392
| const QString scheme = url.scheme(); | ||
| if (scheme.isEmpty() || scheme == QLatin1String("file")) { |
There was a problem hiding this comment.
🚨 issue (security): Consider restricting the relaxed check to specific non-local schemes instead of all non-file URLs
The current condition disables traversal checks for every non-empty, non-file scheme. For custom schemes that still map to filesystem-like backends, this could allow names with ../ or ..\ to pass unchecked. Since the need is specifically for trash:///, consider limiting the relaxed behavior to that scheme (or an explicit allowlist of schemes) rather than applying it to all non-file URLs.
deepin pr auto review★ 总体评分：100分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 当前代码已为最优解，无需额外修改，此处展示其完整上下文以供校验
QUrl DEnumeratorPrivate::buildUrl(const QUrl &url, const char *fileName)
{
if (!fileName) {
return QUrl();
}
QByteArray fileNameBa(fileName);
// 路径遍历检查仅对本地文件系统 (file:/// 或无 scheme) 生效
// gio 的 trash:/// 后端对非用户主目录挂载点的回收站文件，使用反斜杠分隔的扁平路径
// 作为 GFileInfo 的 standard::name（例如 "\media\user\dev\.Trash-1000\files\x"），
// 这是合法的 trash 文件名而非恶意路径，故不应对其做路径遍历拦截。
const QString scheme = url.scheme();
if (scheme.isEmpty() || scheme == QLatin1String("file")) {
if (fileNameBa.contains("../") || fileNameBa.contains("..\\") || fileNameBa.startsWith("..")) {
return QUrl();
}
}
QByteArray path;
// ... 后续原有逻辑
} |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
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
2 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.
根因分析
DEnumeratorPrivate::buildUrl()中的路径遍历安全检查fileNameBa.contains('\\')对 giotrash:///后端使用反斜杠分隔的扁平文件名（如\media\user\dev\.Trash-1000\files\x）返回空QUrl("")，导致/media挂载点的回收站文件无法显示和清空。fd494fb（PMS #367075, 2026-07-16）引入了该检查QUrl("")，触发kIsNotTrashFileError修复方案
将路径遍历检查限制为仅对
file:///或无 scheme 的本地文件系统生效，trash:///等 gio 虚拟文件系统跳过该检查。改动安全评估
低风险。修改仅限制检查的适用 scheme 范围，不改变
buildUrl()签名或返回值语义。对file:///scheme 行为完全不变。Summary by Sourcery
Bug Fixes: