Skip to content

fix: narrow path traversal check pattern in DEnumerator buildUrl - #380

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
Johnson-zs:agent/bugfix/02144741-master-supplement
Aug 5, 2026
Merged

fix: narrow path traversal check pattern in DEnumerator buildUrl#380
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
Johnson-zs:agent/bugfix/02144741-master-supplement

Conversation

@Johnson-zs

@Johnson-zs Johnson-zs commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

补充修复

PR #377 已合入 master,将路径遍历检查限制为仅对 file:/// scheme 生效(修复了 gio trash 回收站问题)。

本 PR 将路径遍历检查模式从 contains('/') || contains('\\') || == "." || == ".." 收窄为 contains("../") || contains("..\\") || startsWith(".."),与 release/eagle、release/snipe 分支保持一致,只拦截实际的路径遍历尝试。

PMS: BUG-372733

Summary by Sourcery

Bug Fixes:

  • Prevent valid filenames from being incorrectly blocked by over-broad path traversal checks when building file URLs.

将路径遍历检查从 contains('/') || contains('\\') || == "." || == ".."
收窄为 contains("../") || contains("..\\") || startsWith(".."),
与 release/eagle、release/snipe 分支保持一致,只拦截实际的路径遍历尝试。

PMS: BUG-372733
Bug: https://pms.uniontech.com/bug-view-372733.html

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @Johnson-zs, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot

Copy link
Copy Markdown

[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.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sourcery-ai

sourcery-ai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Narrows the path traversal check in DEnumeratorPrivate::buildUrl to only catch real directory traversal attempts while keeping the restriction scoped to file:// (or empty) schemes.

Flow diagram for updated path traversal check in buildUrl

flowchart TD
    A[buildUrl url fileName] --> B[get scheme]
    B --> C{scheme is empty or file}
    C -- no --> D[proceed to build QUrl]
    C -- yes --> E{fileNameBa contains ../ or ..\\ or startsWith ..}
    E -- yes --> F[return empty QUrl]
    E -- no --> D[proceed to build QUrl]
Loading

File-Level Changes

Change Details Files
Tightened the path traversal detection logic in buildUrl to only block actual directory traversal patterns.
  • Replaced generic checks for any '/' or '\' and for '.'/'..' names with checks for explicit '../' and '..\' substrings
  • Added a prefix check for filenames starting with '..' to catch leading traversal segments while allowing other legitimate names
src/dfm-io/dfm-io/denumerator.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

★ 总体评分:40分

■ 【总体评价】

代码修复了过度拦截合法 trash 文件名的功能缺陷,但引入了严重的路径遍历安全漏洞
逻辑正确但因存在高危路径遍历漏洞扣60分,触发安全上限规则强制降分

■ 【详细分析】

  • 1.语法逻辑(基本正确)✓

代码使用了 QByteArray 的 contains 和 startsWith 方法进行字符串匹配,语法完全正确,能够准确识别以 ".." 开头或包含 "../"、".." 的字符串,符合预期的相对向上遍历拦截逻辑
潜在问题:无
建议:无

  • 2.代码质量(良好)✓

注释清晰解释了修改的业务背景(合法 trash 文件名包含斜杠和点号),代码表达式简洁明了,符合 Qt/C++ 的常规编码风格
潜在问题:无
建议:无

  • 3.代码性能(无性能问题)✓

contains 和 startsWith 均为 O(N) 的线性扫描操作,且文件名字符串长度通常极短,对系统性能无任何可感知影响
潜在问题:(如无则省略此行)
建议:无

  • 4.代码安全(存在 1 个安全漏洞)✕

漏洞对比统计:新增漏洞 1 个,减少漏洞 0 个,持平 0 个
修改后的逻辑移除了对单独斜杠的防御,导致绝对路径和特定拼接路径可以绕过检查,存在严重的目录穿越风险

  • 安全漏洞1([无] ):[路径遍历] 在 [DEnumeratorPrivate::buildUrl/denumerator.cpp] 中,[新逻辑仅拦截 "../"、".." 和以 ".." 开头的字符串,但完全放行以 "/" 或 "" 开头的绝对路径字符串(如 "/etc/shadow")。如果攻击者通过特制文件系统、恶意压缩包或 IPC 接口控制了 fileName 参数传入绝对路径,该检查会被直接绕过,可能导致后续 URL 拼接时发生路径覆盖,从而越权读取系统敏感文件。此外,类似 "foo/.." 这种不以 ".." 开头且不包含 "../" 的相对路径拼接同样可以绕过检查] ——非常重要

  • 建议:在保留对 trash 合法文件名(中间包含斜杠)兼容性的前提下,增加对绝对路径特征(以 "/" 或 "" 开头)的拦截,并补充对 "/.." 结尾特征字符串的拦截,防止通过目录跳转实现越权

■ 【改进建议代码示例】

diff --git a/src/dfm-io/dfm-io/denumerator.cpp b/src/dfm-io/dfm-io/denumerator.cpp
index c5aaa639..12345678 100644
--- a/src/dfm-io/dfm-io/denumerator.cpp
+++ b/src/dfm-io/dfm-io/denumerator.cpp
@@ -463,7 +463,10 @@ QUrl DEnumeratorPrivate::buildUrl(const QUrl &url, const char *fileName)
     // 这是合法的 trash 文件名而非恶意路径,故不应对其做路径遍历拦截。
     const QString scheme = url.scheme();
     if (scheme.isEmpty() || scheme == QLatin1String("file")) {
-        if (fileNameBa.contains('/') || fileNameBa.contains('\\') || fileNameBa == "." || fileNameBa == "..") {
+        if (fileNameBa.startsWith('/') || fileNameBa.startsWith('\\') ||
+            fileNameBa.contains("../") || fileNameBa.contains("..\\") ||
+            fileNameBa.endsWith("/..") || fileNameBa.endsWith("\\..") ||
+            fileNameBa == "..") {
             return QUrl();
         }
     }

@deepin-ci-robot

Copy link
Copy Markdown

@Johnson-zs: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
github-pr-review-ci 01c55c5 link true /test github-pr-review-ci

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@Johnson-zs

Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot

deepin-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown

This pr force merged! (status: blocked)

@deepin-bot
deepin-bot Bot merged commit dd2de0c into linuxdeepin:master Aug 5, 2026
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants