Revert "fix: add command whitelist validation for notification actions" #1408
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
This reverts commit 3e90036.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR reverts a previous change that added a safe-command whitelist check before executing notification action commands, restoring the prior behavior of executing the requested command directly without configuration-based validation. Sequence diagram for executing notification action command without whitelist validationsequenceDiagram
actor User
participant NotificationManager
participant QProcess
participant OperatingSystem
User->>NotificationManager: actionInvoked(entity, actionKey)
NotificationManager->>NotificationManager: parse entity and actionKey
NotificationManager->>NotificationManager: extract command and arguments
NotificationManager->>QProcess: configure program(cmd) and setArguments(args)
NotificationManager->>QProcess: setProcessChannelMode
NotificationManager->>QProcess: startDetached
QProcess->>OperatingSystem: execute command with arguments
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:
- Removing the safeCommands whitelist reintroduces the ability to execute arbitrary commands from notifications; if this revert is necessary, consider adding at least minimal validation or scoping of allowed binaries to mitigate security risk.
- Since the command whitelist is no longer used, verify that any related configuration keys in org.deepin.dde.shell.notification are cleaned up or clearly deprecated to avoid confusion and dead config.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Removing the safeCommands whitelist reintroduces the ability to execute arbitrary commands from notifications; if this revert is necessary, consider adding at least minimal validation or scoping of allowed binaries to mitigate security risk.
- Since the command whitelist is no longer used, verify that any related configuration keys in org.deepin.dde.shell.notification are cleaned up or clearly deprecated to avoid confusion and dead config.
## Individual Comments
### Comment 1
<location> `panels/notification/server/notificationmanager.cpp:536-540` </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)) {
- qWarning(notifyLog) << "The command is not allowed to be executed:" << cmd << safeCommands;
- return;
- }
</code_context>
<issue_to_address>
**🚨 issue (security):** Removing the safeCommands allowlist introduces a potential command execution risk.
Without the `safeCommands` check, any `cmd` from the notification is passed to `QProcess`, which allows arbitrary command execution if `entity`/`args` can be influenced by untrusted input. Unless there is equivalent validation elsewhere, this is a security risk. Please reintroduce an allowlist or add strong validation to constrain which commands can be run.
</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
-536
to
-540
| 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; |
There was a problem hiding this comment.
🚨 issue (security): Removing the safeCommands allowlist introduces a potential command execution risk.
Without the safeCommands check, any cmd from the notification is passed to QProcess, which allows arbitrary command execution if entity/args can be influenced by untrusted input. Unless there is equivalent validation elsewhere, this is a security risk. Please reintroduce an allowlist or add strong validation to constrain which commands can be run.
xionglinlin
approved these changes
Jan 27, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xionglinlin, 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 |
deepin pr auto review这是一个关于 DDE (Deepin Desktop Environment) 通知系统配置和源代码的变更审查。 审查总结这次修改移除了通知系统中的“安全指令白名单”机制。具体来说，它删除了配置文件中的 结论：这是一个严重的安全风险变更（负向审查），建议驳回。 详细审查意见1. 安全性 —— 极高风险
2. 代码逻辑
3. 代码质量
4. 代码性能
改进建议强烈建议恢复原有的白名单检查机制。 如果原实现存在问题（例如白名单更新不方便，或者某些合法命令被拦截），应该采取以下改进方式，而不是直接移除安全检查：
建议将代码恢复为类似以下形式（保留白名单检查）： void NotificationManager::doActionInvoked(const NotifyEntity &entity, const QString &actionKey)
{
// ... 前面的代码 ...
if (!args.isEmpty()) {
QString cmd = args.takeFirst(); // 命令
// --- 恢复安全检查 START ---
QScopedPointer<DConfig> config(DConfig::create("org.deepin.dde.shell", "org.deepin.dde.shell.notification"));
if (config) {
QStringList safeCommands = config->value("safeCommands").toStringList();
// 建议增加对 cmd 路径的规范化处理，防止绕过 (例如 ./cmd 或 /usr/bin/../bin/cmd)
if (!safeCommands.contains(cmd)) {
qWarning(notifyLog) << "The command is not allowed to be executed:" << cmd << safeCommands;
return;
}
} else {
qWarning(notifyLog) << "Failed to load safe commands config, aborting action.";
return;
}
// --- 恢复安全检查 END ---
QProcess pro;
pro.setProgram(cmd);
pro.setArguments(args);
// ... 执行代码 ...
}
}总结该 Diff 移除了防止任意命令执行的关键安全屏障。请务必回滚此更改，并确保白名单机制正常工作。如果是为了解决功能兼容性问题，请通过扩充白名单或优化校验逻辑来解决，而不是移除校验。 |
Contributor
Author
|
/forcemerge |
|
This pr force merged! (status: blocked) |
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.
This reverts commit 3e90036.
Summary by Sourcery
Revert command whitelist validation for notification action execution to restore previous behavior.