Release #1390
Merged
Release #1390
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
1. Added installation of override configuration files in debian/dde- shell.install 2. Updated CMakeLists.txt to install DTK preference override configuration 3. Created new override file shell/overrides/ org.deepin.dtk.preference.json 4. The override file defines themeType configuration with proper metadata including Chinese translations Log: Added DTK preference override configuration for theme type settings Influence: 1. Verify that the override configuration file is properly installed to /usr/share/dsg/configs/overrides/org.deepin.dde.shell/ 2. Test that theme type configuration is available through DTK configuration system 3. Check that the themeType setting can be read and written correctly 4. Verify Chinese translations for configuration name and description 5. Ensure the configuration has proper permissions (readwrite) and visibility (public) 6. Test that the override doesn't break existing configuration loading fix: 添加DTK偏好设置覆盖配置 1. 在debian/dde-shell.install中添加覆盖配置文件的安装 2. 更新CMakeLists.txt以安装DTK偏好设置覆盖配置 3. 创建新的覆盖文件shell/overrides/org.deepin.dtk.preference.json 4. 覆盖文件定义了themeType配置，包含完整元数据和中文翻译 Log: 添加了主题类型设置的DTK偏好设置覆盖配置 Influence: 1. 验证覆盖配置文件是否正确安装到/usr/share/dsg/configs/overrides/ org.deepin.dde.shell/ 2. 测试主题类型配置是否可通过DTK配置系统访问 3. 检查themeType设置能否正确读写 4. 验证配置名称和描述的中文翻译 5. 确保配置具有正确的权限（读写）和可见性（公开） 6. 测试覆盖配置不会破坏现有的配置加载 PMS: BUG-345091
Add safeCommands whitelist in dconfig with default safe commands Validate commands against whitelist before execution Log: add command whitelist validation for notification actions
deepin pr auto review我来对这个 Git diff 进行全面的代码审查：
在 notificationmanager.cpp 中的改进是很好的安全实践： QScopedPointer<DConfig> config(DConfig::create("org.deepin.dde.shell", "org.deepin.dde.shell.notification"));
QStringList safeCommands = config->value("safeCommands").toStringList();
if (!safeCommands.contains(cmd)) {
qWarning(notifyLog) << "The command is not allowed to be executed:" << cmd << safeCommands;
return;
}优点：
建议改进：
在 org.deepin.dde.shell.notification.json 中添加的 safeCommands 配置项是合理的，但建议：
在 dde-shell.install 中的修改： 这个改动是合理的，但建议：
添加的配置覆盖文件支持是好的实践，但建议：
这个配置文件结构合理，但建议：
总体建议：
这些改进主要关注了安全性、可维护性和用户体验，有助于提高代码质量和系统稳定性。 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a configurable safelist for notification action commands, enforcing it at execution time, and wires up DTK configuration/override metadata and packaging for dde-shell to ship the new settings. Sequence diagram for safelisted notification action command executionsequenceDiagram
participant User
participant NotificationUI
participant NotificationManager
participant DConfig
participant QProcess
User->>NotificationUI: Click notification action
NotificationUI->>NotificationManager: doActionInvoked(entity, actionKey)
activate NotificationManager
NotificationManager->>NotificationManager: Parse action command and args
NotificationManager->>DConfig: create(org.deepin.dde.shell, org.deepin.dde.shell.notification)
DConfig-->>NotificationManager: DConfig instance
NotificationManager->>DConfig: value(safeCommands)
DConfig-->>NotificationManager: QStringList safeCommands
NotificationManager->>NotificationManager: Check cmd in safeCommands
alt Command not in safelist
NotificationManager-->>NotificationManager: Log warning and return
else Command allowed
NotificationManager->>QProcess: setProgram(cmd)
NotificationManager->>QProcess: setArguments(args)
NotificationManager->>QProcess: start()
end
deactivate NotificationManager
Class diagram for NotificationManager safelist enforcementclassDiagram
class NotificationManager {
+void doActionInvoked(NotifyEntity entity, QString actionKey)
}
class DConfig {
+static DConfig* create(QString appId, QString configName)
+QVariant value(QString key)
}
class QProcess {
+void setProgram(QString program)
+void setArguments(QStringList arguments)
+void start()
}
class NotifyEntity {
}
NotificationManager ..> DConfig : uses
NotificationManager ..> QProcess : uses
NotificationManager ..> NotifyEntity : parameter
class SafeCommandPolicy {
+QStringList safeCommands
+bool isAllowed(QString cmd)
}
NotificationManager ..> SafeCommandPolicy : enforces
SafeCommandPolicy : safeCommands loaded via DConfig safeCommands
SafeCommandPolicy : deny execution if cmd not contained
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:
- Creating a new
DConfiginstance on every action invocation may be expensive; consider caching the config object or thesafeCommandslist and updating it only when the configuration changes. - If
safeCommandsis empty, all commands will be blocked; double-check whether a missing/empty safelist should instead fall back to a default set or allow existing behavior to avoid surprising runtime failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Creating a new `DConfig` instance on every action invocation may be expensive; consider caching the config object or the `safeCommands` list and updating it only when the configuration changes.
- If `safeCommands` is empty, all commands will be blocked; double-check whether a missing/empty safelist should instead fall back to a default set or allow existing behavior to avoid surprising runtime failures.
## Individual Comments
### Comment 1
<location> `panels/notification/server/notificationmanager.cpp:535-536` </location>
<code_context>
if (!args.isEmpty()) {
QString cmd = args.takeFirst(); // 命令
+ QScopedPointer<DConfig> config(DConfig::create("org.deepin.dde.shell", "org.deepin.dde.shell.notification"));
+ QStringList safeCommands = config->value("safeCommands").toStringList();
+
+ if (!safeCommands.contains(cmd)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against DConfig::create failure and avoid dereferencing a null config pointer.
`DConfig::create` can return `nullptr` (e.g. backend failure or missing schema), so `config->value(...)` is a crash risk. Add a null check (e.g. `if (!config) { /* deny all commands + log */ }`) before calling `value()` and choose an appropriate fallback behaviour.
</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
+535
to
+536
| QScopedPointer<DConfig> config(DConfig::create("org.deepin.dde.shell", "org.deepin.dde.shell.notification")); | ||
| QStringList safeCommands = config->value("safeCommands").toStringList(); |
There was a problem hiding this comment.
issue (bug_risk): Guard against DConfig::create failure and avoid dereferencing a null config pointer.
DConfig::create can return nullptr (e.g. backend failure or missing schema), so config->value(...) is a crash risk. Add a null check (e.g. if (!config) { /* deny all commands + log */ }) before calling value() and choose an appropriate fallback behaviour.
yixinshark
approved these changes
Jan 6, 2026
mhduiy
approved these changes
Jan 6, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy, yixinshark 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 |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, mhduiy, yixinshark 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 |
yixinshark
merged commit 4fb633a
into
linuxdeepin:release/deepin25.0.10
16 of 17 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.
Summary by Sourcery
Restrict notification action command execution to a configurable safelist and integrate related configuration overrides into the shell build.
Bug Fixes:
Enhancements:
Build: