fix: add path traversal check in mkTempDir function #193
Conversation
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
- Implemented a validation check in the mkTempDir function to reject infix values containing "..", enhancing security against path traversal vulnerabilities. This change improves the robustness of the temporary directory creation process.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a security-focused validation to Utils::mkTempDir by rejecting infix values containing ".." and logging diagnostic messages, preventing potential path traversal when creating temporary directories. Sequence diagram for updated mkTempDir validation flowsequenceDiagram
participant Caller
participant Utils
participant QDebug
participant QWarning
Caller->>Utils: mkTempDir(infix)
Utils->>QDebug: log Creating temp dir with infix
alt infix contains ..
Utils->>QWarning: log Invalid infix contains path traversal
Utils-->>Caller: QString()
else infix valid
Utils-->>Caller: QString(dirPath)
end
Class diagram for Utils with updated mkTempDirclassDiagram
class Utils {
+QString mkTempDir(QString infix)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这段代码的修改主要是为了增加安全性和调试能力，整体方向是正确的，但仍有改进空间。以下是对这段代码的详细审查意见： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
改进后的代码示例QString Utils::mkTempDir(const QString &infix)
{
qDebug() << "Utils::mkTempDir - Creating temp dir with infix:" << infix;
// 路径遍历检查：拒绝包含 ".." 或路径分隔符的 infix
if (infix.contains("..") || infix.contains('/') || infix.contains('\\')) {
qWarning() << "Utils::mkTempDir - Invalid infix contains path traversal or separators:" << infix;
return QString(); // 或者返回默认临时目录
}
// 限制 infix 长度（可选）
if (infix.length() > 32) {
qWarning() << "Utils::mkTempDir - Infix too long:" << infix;
return QString();
}
// Construct template like "/var/tmp/diskmanager-XXXXXX" or "/var/tmp/diskmanager-INFIX-XXXXXX"
QString dirTemplate = "/var/tmp/";
// ... 其余代码
}总结
|
![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 2 issues, and left some high level feedback:
- The path traversal check is quite narrow; consider rejecting any infix containing path separators (e.g., '/', '\') and/or using QDir::cleanPath and comparing against the original to more robustly prevent traversal rather than only checking for "..".
- The added qDebug/qWarning logs in mkTempDir may be noisy in production; consider using a logging category or lowering verbosity (or guarding with a debug flag) if this function is called frequently.
- The new comment is in Chinese while surrounding comments/code appear to be in English; consider aligning the comment language with the rest of the file for consistency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The path traversal check is quite narrow; consider rejecting any infix containing path separators (e.g., '/', '\\') and/or using QDir::cleanPath and comparing against the original to more robustly prevent traversal rather than only checking for "..".
- The added qDebug/qWarning logs in mkTempDir may be noisy in production; consider using a logging category or lowering verbosity (or guarding with a debug flag) if this function is called frequently.
- The new comment is in Chinese while surrounding comments/code appear to be in English; consider aligning the comment language with the rest of the file for consistency.
## Individual Comments
### Comment 1
<location> `basestruct/utils.cpp:662-666` </location>
<code_context>
QString Utils::mkTempDir(const QString &infix)
{
+ qDebug() << "Utils::mkTempDir - Creating temp dir with infix:" << infix;
+
+ // 路径遍历检查：拒绝包含 ".." 的 infix
</code_context>
<issue_to_address>
**suggestion (performance):** Consider avoiding unconditional qDebug noise in a frequently used utility
Since this helper is used in many places, an unconditional qDebug here could generate excessive logs in normal runs. If this is for troubleshooting, consider guarding it behind a debug/verbose flag or moving the logging to a higher-level caller that invokes mkTempDir.
```suggestion
QString Utils::mkTempDir(const QString &infix)
{
#ifdef QT_DEBUG
qDebug() << "Utils::mkTempDir - Creating temp dir with infix:" << infix;
#endif
// 路径遍历检查：拒绝包含 ".." 的 infix
```
</issue_to_address>
### Comment 2
<location> `basestruct/utils.cpp:667-669` </location>
<code_context>
+ qDebug() << "Utils::mkTempDir - Creating temp dir with infix:" << infix;
+
+ // 路径遍历检查：拒绝包含 ".." 的 infix
+ if (infix.contains("..")) {
+ qWarning() << "Utils::mkTempDir - Invalid infix contains path traversal:" << infix;
+ return QString();
+ }
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Path traversal mitigation likely also needs to handle slashes and absolute paths
Rejecting ".." is helpful, but infix values that include path separators ("/" or "\\") or start with a slash could still affect the final path, depending on how dirTemplate and infix are combined. To avoid mkTempDir escaping the intended base directory, also reject any path separators and leading slashes, or restrict infix to a filename-like pattern (e.g., alphanumerics, dash, underscore).
</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
662
to
+666
| QString Utils::mkTempDir(const QString &infix) | ||
| { | ||
| qDebug() << "Utils::mkTempDir - Creating temp dir with infix:" << infix; | ||
|
|
||
| // 路径遍历检查：拒绝包含 ".." 的 infix |
There was a problem hiding this comment.
suggestion (performance): Consider avoiding unconditional qDebug noise in a frequently used utility
Since this helper is used in many places, an unconditional qDebug here could generate excessive logs in normal runs. If this is for troubleshooting, consider guarding it behind a debug/verbose flag or moving the logging to a higher-level caller that invokes mkTempDir.
Suggested change
| QString Utils::mkTempDir(const QString &infix) | |
| { | |
| qDebug() << "Utils::mkTempDir - Creating temp dir with infix:" << infix; | |
| // 路径遍历检查：拒绝包含 ".." 的 infix | |
| QString Utils::mkTempDir(const QString &infix) | |
| { | |
| #ifdef QT_DEBUG | |
| qDebug() << "Utils::mkTempDir - Creating temp dir with infix:" << infix; | |
| #endif | |
| // 路径遍历检查：拒绝包含 ".." 的 infix |
Comment on lines
+667
to
+669
| if (infix.contains("..")) { | ||
| qWarning() << "Utils::mkTempDir - Invalid infix contains path traversal:" << infix; | ||
| return QString(); |
There was a problem hiding this comment.
🚨 suggestion (security): Path traversal mitigation likely also needs to handle slashes and absolute paths
Rejecting ".." is helpful, but infix values that include path separators ("/" or "\") or start with a slash could still affect the final path, depending on how dirTemplate and infix are combined. To avoid mkTempDir escaping the intended base directory, also reject any path separators and leading slashes, or restrict infix to a filename-like pattern (e.g., alphanumerics, dash, underscore).
max-lvs
approved these changes
Feb 11, 2026
Author
|
/merge |
Contributor
|
This pr cannot be merged! (status: unstable) |
max-lvs
approved these changes
Feb 11, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiHua000, max-lvs 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 |
Author
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
This change improves the robustness of the temporary directory creation process.
Summary by Sourcery
Bug Fixes: