Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602 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 |
Reviewer's GuideAPIAdd caller-sender parameters and authorization checks to the exported Plymouth methods, backed by dedicated polkit actions.
ImplementationValidate theme names and require exact list membership, while restoring the previous theme if initramfs rebuilding fails.
ConfigurationDefine the Plymouth polkit actions and their authorization policy for theme changes and scaling.
DocumentationAdd localized policy prompt and description strings for the new Plymouth authorization actions.
TestsTest allowed and rejected theme-name forms and verify exact matching against listed themes.
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 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="bin/dde-system-daemon/plymouth.go" line_range="195" />
<code_context>
+// with the security loader skip the prompt, everyone else needs the polkit
+// action.
+func (d *Daemon) SetPlymouthTheme(sender dbus.Sender, themeName string) *dbus.Error {
+ if err := d.authorize(sender, polkitActionSetPlymouthTheme); err != nil {
+ logger.Warningf("SetPlymouthTheme authorization failed: %q", err.Error())
+ return dbusutil.ToError(err)
</code_context>
<issue_to_address>
**Local callers bypass theme authorization**
When an ordinary local caller invokes `ScalePlymouth` with a supported scale, `ScalePlymouth` calls `scalePlymouth` without checking the caller, and that path changes the boot theme and runs an initramfs rebuild as root. Local users can therefore change the theme and repeatedly trigger privileged work.
Add a sender and authorization check to `ScalePlymouth` before it calls `scalePlymouth`.
</issue_to_address>
### Comment 2
<location path="bin/dde-system-daemon/plymouth.go" line_range="199" />
<code_context>
+ logger.Warningf("SetPlymouthTheme authorization failed: %q", err.Error())
+ return dbusutil.ToError(err)
+ }
return dbusutil.ToError(d.setPlymouthTheme(themeName))
}
</code_context>
<issue_to_address>
**Configured theme and boot image diverge**
When `plymouth-set-default-theme` succeeds but `update-initramfs` fails or the daemon stops before the rebuild completes, `setPlymouthTheme` leaves the new persistent theme in place without restoring it or repairing the initramfs, so the configured theme and boot image are inconsistent.
Make the theme change and initramfs rebuild failure-safe by restoring the prior theme or repairing the initramfs when the rebuild does not complete.
</issue_to_address>
### Comment 3
<location path="bin/dde-system-daemon/plymouth.go" line_range="211" />
<code_context>
themelist := string(themelistout)
- if !strings.Contains(themelist, themeName) {
- return fmt.Errorf("The themeName %s does not exist in plymouth themelist", themeName)
+ if err := checkPlymouthThemeName(themelist, themeName); err != nil {
+ return err
}
</code_context>
<issue_to_address>
**Invalid names launch a root command**
When an authorized caller supplies a syntactically invalid theme name, `setPlymouthTheme` runs `plymouth-set-default-theme --list` before rejecting the name, so the invalid request launches a root subprocess and can return a command error instead of an invalid-name error.
Validate the name with `isValidPlymouthThemeName` before launching the command, then check valid names against the theme list.
</issue_to_address>
### Comment 4
<location path="bin/dde-system-daemon/plymouth_test.go" line_range="46-47" />
<code_context>
+ {"uos-ssd-logo\n--list", false},
+ {"uos-ssd-logo;id", false},
+ {"uos-ssd-logo$(id)", false},
+ {strings.Repeat("a", plymouthThemeMaxLength), true},
+ {strings.Repeat("a", plymouthThemeMaxLength+1), false},
+ }
+
</code_context>
<issue_to_address>
**Length limit regression goes unnoticed**
When `plymouthThemeMaxLength` is accidentally raised above 64, testIsValidPlymouthThemeName builds both boundary inputs from `plymouthThemeMaxLength`, so raising the limit above 64 makes the test accept the new boundary and reject only the next byte; it passes even though names longer than the intended 64 characters are accepted.
Use an independent literal 64-character boundary in the test, or assert that `plymouthThemeMaxLength` remains 64.
</issue_to_address>
fly602
force-pushed
the
agent/agent/41c7eac15e24
branch
from
November 13, 2026 07:53
27164f5 to
efd34c3
Compare
1. SetPlymouthTheme on the root-privileged org.deepin.dde.Daemon1 interface took no sender and performed no authorization, so any local user could switch the boot theme and make the daemon run update-initramfs -u -k all as root. It now takes the caller's sender and authorizes through the existing d.authorize helper, using org.deepin.dde.daemon.set-plymouth-theme (auth_admin). 2. ScalePlymouth performs the same root operation and is used by dde-control-center's boot animation size setting and by dde-services' display-scale synchronization. It had the same missing authorization; it now takes the sender and uses org.deepin.dde.daemon.scale-plymouth with allow_active=yes so an active local session can still apply the scale, while inactive and remote callers are rejected. 3. SetPlymouthTheme keeps the strict auth_admin policy; the relaxed active-session rule applies only to ScalePlymouth. No security-loader registration is added in this change, so the polkit fallback remains the source of authorization. 4. The theme name check previously used strings.Contains over the theme list, accepting an empty string and fragments of a real theme. It now requires a plausible theme identifier and an exact match with one entry of `plymouth-set-default-theme --list`. 5. Add tests for the name validation and exact-match semantics, including option-like names, path traversal, separators and multi-entry values. fix: 为 Plymouth 主题操作补鉴权 1. root 权限的 org.deepin.dde.Daemon1 接口上，SetPlymouthTheme 原先不接收 sender 也不鉴权，任意本地用户可切换开机主题并让守护进程以 root 执行 update-initramfs；现在接收 sender，并通过 d.authorize 使用 org.deepin.dde.daemon.set-plymouth-theme（auth_admin）鉴权。 2. ScalePlymouth 执行同样的 root 操作，且 DCC 的开机动画大小设置和 dde-services 的显示缩放同步都在调用它；它同样缺少鉴权。现在增加 sender，并通过 org.deepin.dde.daemon.scale-plymouth 鉴权： allow_active=yes，本地活跃会话仍可调整，非活跃/远程调用拒绝。 3. SetPlymouthTheme 继续保持 auth_admin；放宽只针对 ScalePlymouth。 本次不新增 security-loader 登记，继续使用 polkit 回退。 4. 主题名校验原先使用 strings.Contains，空串和真实主题名片段都能通过； 现在要求合法标识符且与 `plymouth-set-default-theme --list` 精确匹配。 5. 新增测试覆盖名称校验和精确匹配，包括选项式参数、路径穿越、分隔符和 多行拼接值。 Log: 普通用户无法再通过 SetPlymouthTheme 修改启动画面主题；ScalePlymouth 仅对本地活跃会话放行，非活跃/远程调用被拒绝。 Influence: 1) 普通用户无 polkit 代理时调用 SetPlymouthTheme 应被拒绝且配置不变；2) DCC 本地活跃会话调整启动动画大小仍能生效；3) 非活跃/远程调用 ScalePlymouth 被拒绝；4) 空串、片段、选项式、路径穿越主题名在命令执行前被拒绝；5) 构建、plymouth 测试和 commitlint 通过。 PMS: BUG-379743
fly602
force-pushed
the
agent/agent/41c7eac15e24
branch
from
November 13, 2026 07:57
efd34c3 to
0cf643d
Compare
1. Read the currently configured theme before changing it in both SetPlymouthTheme and ScalePlymouth. 2. When the theme command succeeds but the initramfs rebuild fails, write the previous theme back and return an error that reports both the rebuild failure and whether the rollback succeeded. This keeps /etc/plymouth/plymouthd.conf aligned with the still-existing initramfs. 3. Do not roll back when the theme change command/unit itself fails: in that case the old configuration is expected to be intact, and running another root command would add noise. Rollback is only attempted after a confirmed theme switch followed by a failed initramfs update. 4. Apply the same failure handling to ScalePlymouth's transient initramfs unit, since it has the same divergence risk as SetPlymouthTheme. fix: initramfs 重建失败时恢复 Plymouth 主题 1. SetPlymouthTheme 和 ScalePlymouth 在切换前记录当前主题。 2. 主题切换成功但 initramfs 重建失败时，把旧主题写回，并返回同时包含 重建失败和回滚结果的错误，避免配置指向新主题而 initramfs 仍是旧主题。 3. 主题切换命令/unit 本身失败时不回滚：此时旧配置一般未被改变，额外执行 root 命令没有意义。只有确认主题已切换、随后 initramfs 更新失败才回滚。 4. ScalePlymouth 的 initramfs transient unit 也采用同样的失败处理。 Log: Plymouth 主题切换后若 initramfs 重建失败，会尝试恢复旧主题，避免下次启动主题与 initramfs 不一致。 Influence: 1) 模拟 update-initramfs 失败时，旧主题被写回且配置与旧 initramfs 保持一致；2) 主题设置命令本身失败时不执行额外回滚命令；3) 回滚失败时错误信息同时包含原始失败和回滚失败；4) 构建、测试和 commitlint 通过。 PMS: BUG-379743
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 语法正确，逻辑清晰 ✅ 通过 潜在问题： 建议： ['代码逻辑正确，无需修改'] 2. 代码质量 ✅评价： 代码结构清晰，注释完整 ✅ 通过 潜在问题： 建议： ['代码质量良好，无需修改'] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： ['性能良好，无需优化'] 4. 代码安全 🔒评价： 存在0个安全漏洞 ✅ 通过
安全漏洞详情： 建议： ['安全修复完善，建议考虑对 currentPlymouthTheme 返回值也做 isValidPlymouthThemeName 校验作为纵深防御'] 💡 改进建议代码示例// 可选的纵深防御：对 currentPlymouthTheme 返回值也做格式校验
func currentPlymouthTheme() (string, error) {
out, err := exec.Command("plymouth-set-default-theme").Output()
if err != nil {
return "", fmt.Errorf("get current plymouth theme failed: %w", err)
}
theme := strings.TrimSpace(string(out))
if theme == "" {
return "", fmt.Errorf("current plymouth theme is empty")
}
if !isValidPlymouthThemeName(theme) {
return "", fmt.Errorf("current plymouth theme %q has unexpected format", theme)
}
return theme, nil
}本报告由 AI 代码审查工具自动生成 |
fly602
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
问题
org.deepin.dde.Daemon1.SetPlymouthTheme由 root 运行的dde-system-daemon导出，原实现既不接收调用者sender，也没有任何鉴权（bin/dde-system-daemon/plymouth.go:148旧版）。D-Bus 策略misc/conf/org.deepin.dde.Daemon1.conf对 default 上下文放行整个接口，因此任意本地用户就能：/etc/plymouth/plymouthd.conf）；update-initramfs -u -k all，持续消耗 CPU 与磁盘。同时主题名只做
strings.Contains(themelist, themeName)子串判断，空串和真实主题名的片段都能通过校验。来源：bughunter
CWE-862（dde-system-daemon: SetPlymouthTheme D-Bus 方法缺少鉴权校验…），PMSBUG-379743。改动
bin/dde-system-daemon/plymouth.goSetPlymouthTheme(sender dbus.Sender, themeName string)：进入方法体先d.authorize(sender, polkitActionSetPlymouthTheme)，复用bin/dde-system-daemon/camera_privacy.go:588已有的 security-loader 白名单 + polkit 兜底路径，未授权直接返回、不执行任何外部命令。isValidPlymouthThemeName：拒绝空串、超长（>64）、以-/.开头、含/、空白或其它非[A-Za-z0-9._-]字符的名称。checkPlymouthThemeName：要求与plymouth-set-default-theme --list的某一项完全一致，替换原先的子串判断。setPlymouthTheme里复制粘贴来的调试日志文案（end ScalePlymouth→end SetPlymouthTheme）。misc/polkit-action/org.deepin.dde.daemon.system.policy.inorg.deepin.dde.daemon.set-plymouth-theme，allow_any=no / allow_inactive=no / allow_active=auth_admin，与同文件只读保护动作一致；不新增策略文件，沿用POLICIES构建流程。misc/ts/org.deepin.dde.daemon.system.policy/policy.ts及policy_zh_CN/zh_HK/zh_TW.tspolicy.ts由deepin-policy-ts-convert policy2ts生成，逐字一致）。bin/dde-system-daemon/plymouth_test.go（新增）--list/--rebuild-initrd/-l）、路径穿越、/、空格、换行、命令替换、长度边界。--list输出下的整名通过，片段/前缀/空串/多行拼接值被拒；空主题列表也拒绝。兼容性
SetPlymouthTheme无 Go 调用方；本工作区查到的调用方只有控制中心ScalePlymouth（dde-control-center/src/plugin-commoninfo/operation/commoninfoproxy.cpp:267），那是另一个方法，本次不动。deepin-security-loader启动，d.authorize走白名单免弹窗；否则会弹 polkit 认证（auth_admin）。若确定存在无会话的守护进程调用方，需要像Timedate1那样在对应仓库登记permission-interfaces（参考 dde-control-center 提交e720faf10）。dbus.Sender由总线注入，方法签名变化对调用方透明，不需要改调用方代码。验证
go build ./bin/dde-system-daemon/、go vet ./bin/dde-system-daemon/通过（仅有依赖库既有 cgo 警告）。go test ./bin/dde-system-daemon/与go test ./bin/dde-system-daemon/ -race -run 'Plymouth|Camera|Virtual'通过。deepin-policy-ts-convert ts2policy misc/polkit-action/org.deepin.dde.daemon.system.policy.in misc/ts/org.deepin.dde.daemon.system.policy /tmp/x.policy生成的策略含新 action 且带 zh_CN/zh_HK/zh_TW 文案；policy.ts与policy2ts输出无差异。linuxdeepin/action-conventionalcommits-checker（Check: Passed）。未做动态验证：需要把改动装进 dde-daemon，在有 system bus 与 polkit 的会话里调用。
验收步骤：
gdbus call --system --dest org.deepin.dde.Daemon1 --object-path /org/deepin/dde/Daemon1 --method org.deepin.dde.Daemon1.SetPlymouthTheme uos-ssd-logo应返回未授权错误，/etc/plymouth/plymouthd.conf内容与 mtime 不变，journal 中无plymouth-set-default-theme/update-initramfs记录。--list、空串、uos-ssd（片段）、../text应返回参数错误，且不执行任何命令。uos-ssd-logo，主题切换成功、initramfs 重建完成。deepin-security-loader启动的调用方执行同一操作不弹认证框。风险
auth_admin对活跃会话生效；如果真实调用方不在 security-loader 白名单里，用户体验会从「静默生效」变成「弹认证框」。这一点需要在合入前确认消费方启动方式（评审关注点）。[A-Za-z0-9._-]，可接受；如后续支持自定义主题需同步放宽。update-initramfs，原始「任意用户写放大」的攻击面已消除；如需防重复触发，可另开改动加冷却时间。Summary by Sourcery
Protect privileged Plymouth theme operations with authorization, strict theme validation, and rollback handling.
Bug Fixes:
Enhancements:
Tests: