release #1389
Closed
release #1389
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
18202781743
requested review from
BLumia,
fly602,
mhduiy,
robertkill,
wjyrich and
yixinshark
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a DTK preference override config for dde-shell and introduces a whitelist-based validation for notification action commands, ensuring only configured safe commands are executed while wiring the new override file into the build/install system. Sequence diagram for whitelist validation of notification action commandssequenceDiagram
actor User
participant NotificationCenter
participant NotificationManager
participant DConfig
participant QProcess
participant TargetApplication
User ->> NotificationCenter: click notification_action
NotificationCenter ->> NotificationManager: doActionInvoked(entity, actionId)
NotificationManager ->> NotificationManager: parse entity to get args
NotificationManager ->> NotificationManager: cmd = args.takeFirst()
NotificationManager ->> DConfig: create(appId=org.deepin.dde.shell, configName=org.deepin.dde.shell.notification)
DConfig -->> NotificationManager: DConfig_instance
NotificationManager ->> DConfig: value(key=safeCommands)
DConfig -->> NotificationManager: safeCommands_list
alt command_is_whitelisted
NotificationManager ->> QProcess: construct and configure
QProcess ->> QProcess: setProgram(cmd)
QProcess ->> QProcess: setArguments(args)
QProcess ->> QProcess: start()
QProcess ->> TargetApplication: execute cmd with args
else command_not_allowed
NotificationManager ->> NotificationManager: log warning and return
end
Updated class diagram for NotificationManager command execution flowclassDiagram
class NotificationManager {
+void doActionInvoked(NotifyEntity entity, QString actionId)
-void executeCommand(QString command, QStringList arguments)
}
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 {
+QStringList actions
+QStringList hints
}
NotificationManager ..> DConfig : uses_for_safeCommands
NotificationManager ..> QProcess : uses_for_execution
NotificationManager ..> NotifyEntity : reads_action_args
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 left some high level feedback:
- Creating a new DConfig instance on every action invocation may be unnecessarily expensive; consider caching the config object or the safeCommands list at a higher scope and reusing it.
- DConfig::create() can potentially fail; add a null check and a sensible fallback (e.g., deny all commands or allow a minimal default list) rather than dereferencing config unconditionally.
- If the safeCommands list is missing or empty in configuration, the current logic will block all commands; make it explicit whether this behavior is intended and guard against misconfiguration if not.
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 unnecessarily expensive; consider caching the config object or the safeCommands list at a higher scope and reusing it.
- DConfig::create() can potentially fail; add a null check and a sensible fallback (e.g., deny all commands or allow a minimal default list) rather than dereferencing config unconditionally.
- If the safeCommands list is missing or empty in configuration, the current logic will block all commands; make it explicit whether this behavior is intended and guard against misconfiguration if not.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: 18202781743 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进行详细审查：
class NotificationManager {
private:
QScopedPointer<DConfig> m_config; // 作为类成员
QStringList m_safeCommands; // 缓存安全命令列表
public:
NotificationManager() {
m_config.reset(DConfig::create("org.deepin.dde.shell", "org.deepin.dde.shell.notification"));
m_safeCommands = m_config->value("safeCommands").toStringList();
}
void doActionInvoked(const NotifyEntity &entity, const QString &actionKey) {
// ...其他代码...
if (!m_safeCommands.contains(cmd)) {
qWarning(notifyLog) << "The command is not allowed to be executed:" << cmd;
return;
}
// ...执行命令...
}
};
总体来说，这是一个很好的安全改进，通过白名单机制有效提升了系统的安全性。代码实现基本合理，但在性能优化方面还有提升空间。 |
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.
Summary by Sourcery
Enforce a whitelist for notification action commands and wire up DTK preference override configuration for the shell.
Bug Fixes:
Build: