Skip to content

Conversation

@yixinshark
Copy link
Contributor

@yixinshark yixinshark commented Jan 27, 2026

This reverts commit 526e65d.

Summary by Sourcery

Bug Fixes:

  • Restore the original notification server configuration by reverting the earlier safeCommands whitelist update for the notification service.

@sourcery-ai
Copy link

sourcery-ai bot commented Jan 27, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Reverts a previous change to the notification service safeCommands whitelist configuration, restoring the prior JSON config for org.deepin.dde.shell.notification.

File-Level Changes

Change Details Files
Restore previous notification server configuration, effectively undoing safeCommands whitelist updates.
  • Revert the JSON configuration content for org.deepin.dde.shell.notification to its state before commit 526e65d.
  • Remove any safeCommands whitelist entries or modifications introduced by the reverted commit and restore original values and structure.
panels/notification/server/configs/org.deepin.dde.shell.notification.json

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • Since this is reverting a change to the safeCommands whitelist, please double-check that the reverted configuration remains consistent with other related service configs to avoid divergent security behavior across panels.
  • It would be helpful to capture in the commit message or PR description why the original whitelist update is being reverted (e.g., regression, compatibility issue, or policy change) so future readers understand the rationale for this rollback.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Since this is reverting a change to the safeCommands whitelist, please double-check that the reverted configuration remains consistent with other related service configs to avoid divergent security behavior across panels.
- It would be helpful to capture in the commit message or PR description why the original whitelist update is being reverted (e.g., regression, compatibility issue, or policy change) so future readers understand the rationale for this rollback.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

deepin pr auto review

这份代码审查针对 org.deepin.dde.shell.notification.json 配置文件的变更,主要涉及安全白名单 safeCommands 的修改。

审查概要

本次修改从 safeCommands 列表中移除了 busctlgdbus 两个命令。这是一个安全加固的变更,旨在减少潜在的攻击面,防止通过通知系统直接调用底层的 D-Bus 工具。

详细审查意见

1. 代码逻辑与安全性

  • 改进点(安全性增强)
    • 移除 busctlgdbus:这两个工具是强大的 D-Bus 交互工具,允许读写系统属性、调用方法。如果它们保留在白名单中,恶意应用可能会构造特定的通知参数,利用这两个工具执行未经授权的系统操作(例如修改系统设置、调用敏感接口)。将它们移除是正确的安全决策,遵循了“最小权限原则”。
  • 潜在风险(兼容性)
    • 需要确认系统内部是否有合法的、通过通知机制调用 busctlgdbus 的功能。如果有,这些功能将失效。考虑到列表中保留了 dbus-sendqdbus,通常可以通过这些工具替代,或者直接通过 D-Bus 接口调用,因此兼容性风险较低。

2. 代码质量与规范

  • 格式规范:JSON 格式正确,缩进和引号使用符合规范。
  • 列表维护
    • 列表中混合了相对路径命令(如 xdg-open)和绝对路径命令(如 /usr/lib/...)。虽然这在功能上可行,但在安全配置中,强烈建议统一使用绝对路径
    • 建议:将 xdg-open, dbus-send, qdbus 等也改为绝对路径(如 /usr/bin/xdg-open)。这样可以防止攻击者通过修改 PATH 环境变量来劫持命令执行,从而进一步提升安全性。

3. 代码性能

  • 此配置文件变更仅影响内存中的配置加载,对运行时性能无直接影响。

总结与建议

结论:该变更是一个良好的安全加固措施,移除了存在潜在滥用风险的高级系统工具。

改进建议

  1. 统一路径格式:为了最大程度的安全保障,建议将 safeCommands 中的所有命令项都更新为绝对路径。
    • 例如:将 "xdg-open" 改为 "/usr/bin/xdg-open"
  2. 测试验证:在合并此改动后,应重点测试通知中心相关的系统交互功能(如打开文件、控制中心跳转等),确保移除 busctlgdbus 后未破坏任何依赖它们的正常业务流程。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, xionglinlin, yixinshark

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yixinshark
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Jan 27, 2026

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit dacc605 into linuxdeepin:master Jan 27, 2026
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants