fix: the tips of unable to open file #182
Merged
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
Reviewer's GuideAdds permission-control awareness when loading images so that files blocked by FileArmor produce a clear 'no permission' error instead of a generic damaged-file error, including a new helper for permission checks and updated translations. Sequence diagram for updated image loading with permission controlsequenceDiagram
actor User
participant FileHander
participant FileArmor1
User->>FileHander: loadImage(file)
FileHander->>FileHander: checkFileBeforeLoad(file, false)
FileHander->>FileHander: legalPath = toLegalFile(file)
FileHander->>FileHander: img = loadImage_helper(legalPath, this)
FileHander->>FileHander: pathControl(legalPath)
activate FileHander
FileHander->>FileArmor1: GetApps(docPath_or_picPath)
FileArmor1-->>FileHander: QDBusMessage reply_with_app_list
FileHander-->>FileHander: isBlocked = app_list_contains_deepin_draw
deactivate FileHander
alt File_blocked_by_FileArmor
FileHander->>FileHander: setError(EFileNotExist, No_permissions_to_open_it)
FileHander-->>User: return_empty_QImage()
else File_not_blocked_and_image_invalid
FileHander->>FileHander: img.isNull()
FileHander->>FileHander: setError(EDamagedImageFile, Damaged_file_unable_to_open_it)
FileHander-->>User: return_empty_QImage()
else File_not_blocked_and_image_valid
FileHander-->>User: return_QImage(img)
end
Class diagram for FileHander with new pathControl methodclassDiagram
class FileHander {
+static bool isLegalFile(QString file)
+static QString toLegalFile(QString file)
+static bool pathControl(QString sPath)
+PageContext* loadDdf(QString file)
+QImage loadImage(QString file)
+bool saveToDdf(PageContext* context, QString file)
}
class QDBusInterface {
+QDBusInterface(QString service, QString path, QString interface, QDBusConnection connection)
+bool isValid()
+QDBusMessage call(QString method, QString arg)
}
class QDBusMessage {
+int type()
+QList~QVariant~ arguments()
}
class QStandardPaths {
+static QStringList standardLocations(int type)
+static QString findExecutable(QString fileName)
}
FileHander ..> QDBusInterface : uses
FileHander ..> QDBusMessage : uses
FileHander ..> QStandardPaths : uses
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 there - I've reviewed your changes - here's some feedback:
- In
pathControl, you only check the first entry fromDocumentsLocationandPicturesLocation; if multiple locations are configured (e.g., multiple mounted dirs), consider iterating all returned paths to avoid missing controlled files. - The name and comment for
pathControlsuggest it returns whether the path is under access control, but the implementation returns true only when access is denied for deepin-draw; consider renaming or clarifying the comment to reflect that it specifically checks for denied permission for this app. - Because
pathControlperforms a synchronous D-Bus call on every image load for applicable paths, consider caching the result per directory or file to avoid repeated blocking calls in hot paths likeloadImage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `pathControl`, you only check the first entry from `DocumentsLocation` and `PicturesLocation`; if multiple locations are configured (e.g., multiple mounted dirs), consider iterating all returned paths to avoid missing controlled files.
- The name and comment for `pathControl` suggest it returns whether the path is under access control, but the implementation returns true only when access is denied for deepin-draw; consider renaming or clarifying the comment to reflect that it specifically checks for denied permission for this app.
- Because `pathControl` performs a synchronous D-Bus call on every image load for applicable paths, consider caching the result per directory or file to avoid repeated blocking calls in hot paths like `loadImage`.
## Individual Comments
### Comment 1
<location> `src/service/filehander.cpp:691-696` </location>
<code_context>
+ return false;
+ }
+
+ if (tmpDocLocation.size() > 0) {
+ QString docPath = tmpDocLocation.first();
+ if (sPath.startsWith(docPath)) {
+ reply = iface.call("GetApps", docPath);
+ } else {
+ if (tmpPicLocation.size() > 0) {
+ QString picPath = tmpPicLocation.first();
+ if (sPath.startsWith(picPath)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Picture location is only checked when a documents location exists, which looks unintended.
Because the `tmpPicLocation` branch is nested inside the `tmpDocLocation.size() > 0` check, picture paths are never evaluated when there are no document locations. If Documents and Pictures should be checked independently, consider restructuring so each is handled in its own `if` block, e.g.:
```cpp
if (!tmpDocLocation.isEmpty()) {
const QString &docPath = tmpDocLocation.first();
if (sPath.startsWith(docPath)) {
reply = iface.call("GetApps", docPath);
}
}
if (!reply.isValid() && !tmpPicLocation.isEmpty()) {
const QString &picPath = tmpPicLocation.first();
if (sPath.startsWith(picPath)) {
reply = iface.call("GetApps", picPath);
}
}
```
</issue_to_address>
### Comment 2
<location> `src/service/filehander.cpp:705-706` </location>
<code_context>
+
+ if (reply.type() == QDBusMessage::ReplyMessage) {
+ QList<QString> lValue = reply.arguments().takeFirst().toStringList();
+ QString strApp = QStandardPaths::findExecutable("deepin-draw");
+ // 此路径对画板进行了权限禁用
+ if (lValue.contains(strApp)) {
+ return true;
+ }
</code_context>
<issue_to_address>
**suggestion:** Handling of an empty executable path from `findExecutable` is unclear and may lead to accidental matches.
`QStandardPaths::findExecutable` returns an empty string when the binary is not found. In that case, `lValue.contains(strApp)` becomes `lValue.contains("")`, which could match an empty entry in the D-Bus reply. Please guard against this, e.g.:
```cpp
QString strApp = QStandardPaths::findExecutable("deepin-draw");
if (!strApp.isEmpty() && lValue.contains(strApp)) {
return true;
}
```
This ensures we only treat the path as controlled when the executable is actually resolvable.
```suggestion
if (reply.type() == QDBusMessage::ReplyMessage) {
QList<QString> lValue = reply.arguments().takeFirst().toStringList();
QString strApp = QStandardPaths::findExecutable("deepin-draw");
// 此路径对画板进行了权限禁用
if (!strApp.isEmpty() && lValue.contains(strApp)) {
return true;
}
```
</issue_to_address>
### Comment 3
<location> `src/service/filehander.cpp:780-781` </location>
<code_context>
QImage img = loadImage_helper(legalPath, this);
- if (img.isNull()) {
+ if (pathControl(legalPath)){
+ qWarning() << "Failed to load image: No permissions";
+ d_pri()->setError(EFileNotExist, tr("No permissions to open it"));
+ return QImage();
+ } else if (img.isNull()) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `EFileNotExist` for a permission-denied case may be misleading for callers.
This branch reports a permission error using `EFileNotExist` and the message "No permissions to open it". If callers distinguish between missing files and permission failures, this can cause incorrect behavior (e.g., recreating a file instead of prompting for permission changes). Prefer a dedicated permission error (e.g. `EPermissionDenied`), or confirm that all existing callers truly treat this as equivalent to "file not found" and won't mis-handle it.
</issue_to_address>
### Comment 4
<location> `src/service/filehander.cpp:679` </location>
<code_context>
return result;
}
+bool FileHander::pathControl(const QString &sPath)
+{
+ static QStringList tmpDocLocation = QStandardPaths::standardLocations(QStandardPaths::DocumentsLocation);
</code_context>
<issue_to_address>
**issue (complexity):** Consider flattening the branching in `pathControl` and checking permissions before actually loading the image in `loadImage` to simplify control flow and make the logic easier to follow.
You can keep the behavior but reduce complexity and make the flow clearer with a small refactor of `pathControl` and a tiny reorder in `loadImage`.
### 1. Flatten `pathControl` branching
The current nesting:
```cpp
if (tmpDocLocation.size() > 0) {
QString docPath = tmpDocLocation.first();
if (sPath.startsWith(docPath)) {
reply = iface.call("GetApps", docPath);
} else {
if (tmpPicLocation.size() > 0) {
QString picPath = tmpPicLocation.first();
if (sPath.startsWith(picPath)) {
reply = iface.call("GetApps", picPath);
}
}
}
}
```
can be flattened without changing behavior:
```cpp
bool FileHander::pathControl(const QString &sPath)
{
static const QStringList tmpDocLocation =
QStandardPaths::standardLocations(QStandardPaths::DocumentsLocation);
static const QStringList tmpPicLocation =
QStandardPaths::standardLocations(QStandardPaths::PicturesLocation);
QDBusInterface iface("com.deepin.FileArmor1",
"/com/deepin/FileArmor1",
"com.deepin.FileArmor1",
QDBusConnection::systemBus());
if (!iface.isValid()) {
qWarning() << "Failed to connect to D-Bus interface";
return false;
}
QString targetPath;
if (!tmpDocLocation.isEmpty() && sPath.startsWith(tmpDocLocation.first())) {
targetPath = tmpDocLocation.first();
} else if (!tmpPicLocation.isEmpty() && sPath.startsWith(tmpPicLocation.first())) {
targetPath = tmpPicLocation.first();
}
if (targetPath.isEmpty())
return false;
QDBusMessage reply = iface.call("GetApps", targetPath);
if (reply.type() != QDBusMessage::ReplyMessage)
return false;
const QStringList apps = reply.arguments().takeFirst().toStringList();
const QString drawPath = QStandardPaths::findExecutable("deepin-draw");
// 此路径对画板进行了权限禁用
return apps.contains(drawPath);
}
```
Changes:
- Flatter control flow (no deep nesting).
- Early-return on empty `targetPath` and invalid replies.
- Same semantics: `true` only when the reply is valid and contains `deepin-draw`.
### 2. Check permissions before loading in `loadImage`
Currently you load the image before checking permissions, which complicates reasoning:
```cpp
auto legalPath = toLegalFile(file);
QImage img = loadImage_helper(legalPath, this);
if (pathControl(legalPath)) {
qWarning() << "Failed to load image: No permissions";
d_pri()->setError(EFileNotExist, tr("No permissions to open it"));
return QImage();
} else if (img.isNull()) {
...
}
```
You can make the flow simpler and avoid doing work when access is forbidden:
```cpp
QImage FileHander::loadImage(const QString &file)
{
qDebug() << "Loading image file:" << file;
d_pri()->unsetError();
if (!checkFileBeforeLoad(file, false))
return QImage();
const auto legalPath = toLegalFile(file);
if (pathControl(legalPath)) {
qWarning() << "Failed to load image: No permissions";
d_pri()->setError(EFileNotExist, tr("No permissions to open it"));
return QImage();
}
QImage img = loadImage_helper(legalPath, this);
if (img.isNull()) {
qWarning() << "Failed to load image, file may be damaged";
d_pri()->setError(EDamagedImageFile, tr("Damaged file, unable to open it"));
return QImage();
}
#if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
img = convertToSRgbColorSpace(img);
#else
img = img.convertToFormat(QImage::Format_ARGB32);
#endif
return img;
}
```
This keeps all observable behavior intact (same errors, same return values) but:
- Makes the permission check explicit and early.
- Clarifies the meaning of the `pathControl` result in the context of `loadImage`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lichaofan2008
force-pushed
the
develop/snipe1
branch
from
9cf18a9 to
221bc63
Compare
fix the tips of unable to open file. Log: fix the tips of unable to open file. Bug: https://pms.uniontech.com/bug-view-271335.html v20 BUG 分支合一到v25主线 Task: https://pms.uniontech.com/task-view-383477.html
lichaofan2008
force-pushed
the
develop/snipe1
branch
from
221bc63 to
eafb3ca
Compare
deepin pr auto review我来对这段代码进行审查，主要从语法逻辑、代码质量、性能和安全性几个方面分析：
建议的改进方案： bool FileHander::pathControl(const QString &sPath)
{
if (sPath.isEmpty()) {
return false;
}
// 规范化路径，防止路径遍历攻击
QString canonicalPath = QDir(sPath).canonicalPath();
if (canonicalPath.isEmpty()) {
qWarning() << "Invalid path:" << sPath;
return false;
}
// 缓存标准路径
static const QStringList allowedLocations = []() {
QStringList locations;
locations << QStandardPaths::standardLocations(QStandardPaths::DocumentsLocation);
locations << QStandardPaths::standardLocations(QStandardPaths::PicturesLocation);
if (locations.empty()) {
qWarning() << "Failed to get standard locations";
}
return locations;
}();
if (allowedLocations.empty()) {
return false;
}
// 检查路径是否在允许的目录中
bool isInAllowedLocation = false;
QString matchedLocation;
for (const QString &location : allowedLocations) {
QString canonicalLocation = QDir(location).canonicalPath();
if (canonicalPath.startsWith(canonicalLocation + "/")) {
isInAllowedLocation = true;
matchedLocation = canonicalLocation;
break;
}
}
if (!isInAllowedLocation) {
return false;
}
// 缓存D-Bus接口
static const QDBusInterface iface("com.deepin.FileArmor1",
"/com/deepin/FileArmor1",
"com.deepin.FileArmor1",
QDBusConnection::systemBus());
if (!iface.isValid()) {
qWarning() << "Failed to connect to D-Bus interface";
return false;
}
// 调用D-Bus接口
QDBusMessage reply = iface.call("GetApps", matchedLocation);
if (reply.type() != QDBusMessage::ReplyMessage) {
qWarning() << "D-Bus call failed:" << reply.errorMessage();
return false;
}
QList<QString> lValue = reply.arguments().takeFirst().toStringList();
qDebug() << "App list:" << lValue;
QString strApp = QStandardPaths::findExecutable("deepin-draw");
if (!strApp.isEmpty() && lValue.contains(strApp)) {
qWarning() << "Permission denied for app:" << strApp;
return true;
}
return false;
}这个改进版本：
这些改进使代码更安全、更高效，同时保持了原有的功能。 |
lzwind
approved these changes
Dec 2, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lichaofan2008, lzwind 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
|
/forcemerge |
Contributor
|
This pr force merged! (status: unstable) |
deepin-bot
bot
merged commit a94713c
into
linuxdeepin:develop/snipe
15 of 16 checks passed
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.
fix the tips of unable to open file.
Log: fix the tips of unable to open file.
Bug: https://pms.uniontech.com/bug-view-271335.html
v20 BUG 分支合一到v25主线
Task: https://pms.uniontech.com/task-view-383477.html
Summary by Sourcery
Handle permission-controlled image paths before loading and adjust user-facing error messages accordingly.
Bug Fixes:
Enhancements: