fix: the tips of unable to open file #181
Closed
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
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
Reviewer's GuideAdds a permission-checking pathControl helper that queries FileArmor over D-Bus before loading images, and wires it into loadImage to report a more accurate error when the file cannot be opened due to permissions, including updating the Chinese translation resources. Sequence diagram for loadImage with FileArmor permission checksequenceDiagram
participant Caller
participant FileHander
participant QStandardPaths
participant QDBusInterface as FileArmorProxy
participant QDBusMessage as DBusReply
Caller->>FileHander: loadImage(file)
activate FileHander
FileHander->>FileHander: checkFileBeforeLoad(file, false)
FileHander->>FileHander: toLegalFile(file)
FileHander->>FileHander: pathControl(legalPath)
activate FileHander
FileHander->>QDBusInterface: construct FileArmorProxy
alt DocumentsLocation
FileHander->>QStandardPaths: standardLocations(DocumentsLocation)
QStandardPaths-->>FileHander: docLocations
FileHander->>FileArmorProxy: call(GetApps, docPath)
FileArmorProxy-->>FileHander: DBusReply
end
alt PicturesLocation
FileHander->>QStandardPaths: standardLocations(PicturesLocation)
QStandardPaths-->>FileHander: picLocations
FileHander->>FileArmorProxy: call(GetApps, picPath)
FileArmorProxy-->>FileHander: DBusReply
end
FileHander->>DBusReply: type()
DBusReply-->>FileHander: ReplyMessage
FileHander->>DBusReply: arguments()
DBusReply-->>FileHander: appList
FileHander->>QStandardPaths: findExecutable(deepin-draw)
QStandardPaths-->>FileHander: strApp
FileHander-->>Caller: true if appList contains strApp
deactivate FileHander
alt pathControl returns true
FileHander->>FileHander: setError(EFileNotExist, No permissions to open it)
FileHander-->>Caller: null QImage
else pathControl returns false
FileHander->>FileHander: loadImage_helper(legalPath, this)
FileHander-->>Caller: QImage (may be valid or null)
alt QImage is null
FileHander->>FileHander: setError(EDamagedImageFile, Damaged file, unable to open it)
end
end
deactivate FileHander
Class diagram for updated FileHander permission handlingclassDiagram
class FileHander {
+static bool isLegalFile(QString file)
+static QString toLegalFile(QString file)
+static bool pathControl(QString sPath)
+PageContext* loadDdf(QString file)
+bool saveToDdf(PageContext* context, QString file)
+QImage loadImage(QString file)
}
class QDBusInterface {
+QDBusInterface(QString service, QString path, QString interface, QDBusConnection connection)
+bool isValid()
+QDBusMessage call(QString method, QString arg1)
}
class QDBusMessage {
+int type()
+QList~QVariant~ arguments()
}
class QStandardPaths {
+static QStringList standardLocations(int type)
+static QString findExecutable(QString executableName)
}
class QImage {
+bool isNull()
}
class FileArmorService {
+QDBusMessage GetApps(QString path)
}
FileHander ..> QDBusInterface : uses in pathControl
FileHander ..> QDBusMessage : uses in pathControl
FileHander ..> QStandardPaths : uses in pathControl
FileHander ..> QImage : uses in loadImage
QDBusInterface ..> QDBusMessage : returns
FileArmorService <.. QDBusInterface : D-Bus proxy
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 assume the DBus reply contains at least one argument and directly callreply.arguments().takeFirst().toStringList(); consider checking that the message type isReplyMessageand thatarguments().size() > 0before accessing to avoid runtime issues if the service returns an unexpected payload. - The duplicated logic for handling Documents and Pictures paths in
pathControl(callingstandardLocations, picking the first entry, then doing an identicalstartsWithandGetAppscall) could be simplified by iterating over the relevantQStandardPaths::StandardLocationvalues to reduce duplication and make future changes easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `pathControl`, you assume the DBus reply contains at least one argument and directly call `reply.arguments().takeFirst().toStringList()`; consider checking that the message type is `ReplyMessage` and that `arguments().size() > 0` before accessing to avoid runtime issues if the service returns an unexpected payload.
- The duplicated logic for handling Documents and Pictures paths in `pathControl` (calling `standardLocations`, picking the first entry, then doing an identical `startsWith` and `GetApps` call) could be simplified by iterating over the relevant `QStandardPaths::StandardLocation` values to reduce duplication and make future changes easier.
## Individual Comments
### Comment 1
<location> `src/service/filehander.cpp:687-694` </location>
<code_context>
+ QStringList tmpDocLocation = QStandardPaths::standardLocations(QStandardPaths::DocumentsLocation);
+ if (tmpDocLocation.size() > 0) {
+ QString docPath = tmpDocLocation.first();
+ if (sPath.startsWith(docPath)) {
+ reply = iface.call("GetApps", docPath);
+ }
+ }
+ QStringList tmpPicLocation = QStandardPaths::standardLocations(QStandardPaths::PicturesLocation);
+ if (tmpPicLocation.size() > 0) {
+ QString picPath = tmpPicLocation.first();
+ if (sPath.startsWith(picPath)) {
+ reply = iface.call("GetApps", picPath);
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `startsWith` for path matching can incorrectly classify similarly-prefixed directories as protected.
Because `sPath.startsWith(docPath)` / `sPath.startsWith(picPath)` also matches paths that only share a prefix (e.g. `/home/user/Documents2/...`), permissions could be applied to directories outside the intended Documents/Pictures folders.
To avoid this, normalize both paths and enforce a directory boundary, e.g.:
- Ensure `docPath`/`picPath` end with a path separator before the `startsWith` check, or
- Canonicalize and compare directories via `QDir` (e.g. using `QDir(docPath)` and a helper like `isRelativeTo` if available).
This prevents false positives on similarly named directories.
Suggested implementation:
```cpp
QStringList tmpDocLocation = QStandardPaths::standardLocations(QStandardPaths::DocumentsLocation);
if (tmpDocLocation.size() > 0) {
QString docPath = QDir::cleanPath(tmpDocLocation.first());
QString normalizedPath = QDir::cleanPath(sPath);
QString docPathWithSep = docPath;
if (!docPathWithSep.endsWith(QDir::separator())) {
docPathWithSep += QDir::separator();
}
if (normalizedPath == docPath || normalizedPath.startsWith(docPathWithSep)) {
reply = iface.call("GetApps", docPath);
}
}
QStringList tmpPicLocation = QStandardPaths::standardLocations(QStandardPaths::PicturesLocation);
if (tmpPicLocation.size() > 0) {
QString picPath = QDir::cleanPath(tmpPicLocation.first());
QString normalizedPath = QDir::cleanPath(sPath);
QString picPathWithSep = picPath;
if (!picPathWithSep.endsWith(QDir::separator())) {
picPathWithSep += QDir::separator();
}
if (normalizedPath == picPath || normalizedPath.startsWith(picPathWithSep)) {
reply = iface.call("GetApps", picPath);
}
```
1. At the top of `src/service/filehander.cpp`, ensure you include the QDir header:
- Add `#include <QDir>` alongside the other Qt includes if it is not already present.
2. If `pathControl` is performance-sensitive and called very frequently, consider factoring the normalization of `sPath` out so it is computed once per function call rather than once per location block.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lichaofan2008 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 |
deepin pr auto review我来对这个代码diff进行审查，主要关注语法逻辑、代码质量、性能和安全性方面：
改进建议： bool FileHander::pathControl(const QString &sPath)
{
static const QStringList standardPaths = {
QStandardPaths::standardLocations(QStandardPaths::DocumentsLocation).first(),
QStandardPaths::standardLocations(QStandardPaths::PicturesLocation).first()
};
QDBusInterface iface("com.deepin.FileArmor1", "/com/deepin/FileArmor1",
"com.deepin.FileArmor1", QDBusConnection::systemBus());
if (!iface.isValid()) {
return false;
}
// 检查路径是否在受控目录下
QString matchedPath;
for (const QString &standardPath : standardPaths) {
if (sPath.startsWith(standardPath + "/")) { // 添加'/'确保完整目录匹配
matchedPath = standardPath;
break;
}
}
if (matchedPath.isEmpty()) {
return false;
}
// 执行DBus调用并处理返回值
QDBusMessage reply = iface.call("GetApps", matchedPath);
if (reply.type() != QDBusMessage::ReplyMessage) {
qWarning() << "DBus call failed:" << reply.errorMessage();
return false;
}
// 检查应用权限
const QString deepinDrawPath = QStandardPaths::findExecutable("deepin-draw");
if (deepinDrawPath.isEmpty()) {
return false;
}
const QStringList allowedApps = reply.arguments().takeFirst().toStringList();
return allowedApps.contains(deepinDrawPath);
}其他改进建议：
if (img.isNull()) {
if (pathControl(legalPath)) {
qWarning() << "Failed to load image: No permissions";
d_pri()->setError(EFileNotExist, tr("No permissions to open it"));
} else {
qWarning() << "Failed to load image, file may be damaged";
d_pri()->setError(EDamagedImageFile, tr("Damaged file, unable to open it"));
}
}
这些改进可以提高代码的可维护性、安全性和性能。 |
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.
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 files and update user-facing error messages when opening files fails.
Bug Fixes:
Enhancements: