Repository navigation
Conversation
Reviewer's GuideAPIAuthorize the externally callable theme-update method before file access, close rejected file descriptors, and only extend service lifetime for authorized requests.
ImplementationMake theme-to-xsettings regeneration report failures, tolerate optional keys, validate string values, and reject malformed DPI or scale data instead of silently reporting success.
ConfigurationConfigure the greeter service and D-Bus ownership so the root-running service can perform trusted Polkit authorization, and add the theme-update Polkit action.
TestsAdd tests for safe xsettings value rendering, blocked injection, missing DPI errors, and tolerated optional keys.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[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 |
![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 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="bin/dde-greeter-setter/manager.go" line_range="28" />
<code_context>
dbusPath = "/org/deepin/dde/Greeter1"
dbusInterface = dbusServiceName
+
+ polkitActionUpdateQtTheme = "org.deepin.dde.greeter.update-qt-theme"
)
</code_context>
<issue_to_address>
**Greeter theme updates are denied**
When the package is built and installed without a separately supplied greeter policy, `CheckPolkitAuth` cannot resolve `org.deepin.dde.greeter.update-qt-theme`, so `UpdateGreeterQtTheme` returns an authorization error even for active-session callers and theme updates fail.
Add a tracked policy definition for this action and include it in the policy generation and installation process.
Also at `bin/dde-greeter-setter/manager.go:68-69`.
</issue_to_address>
### Comment 2
<location path="bin/dde-greeter-setter/manager.go" line_range="72" />
<code_context>
+ string(sender), polkitActionUpdateQtTheme)
+ if err != nil {
+ logger.Warningf("failed to authorize %v: %v", sender, err)
+ return dbusutil.ToError(err)
+ }
+ if !authorized {
</code_context>
<issue_to_address>
**Rejected calls exhaust file descriptors**
When polkit denies authorization or returns an error and callers repeatedly pass an FD, `UpdateGreeterQtTheme` returns before `updateGreeterQtTheme` wraps and closes `fd`, so rejected requests leak descriptors until the greeter service exhausts its file-descriptor limit and stops handling requests.
Close `fd` on every authorization-error and denial path, or take ownership of it and defer its close before checking authorization.
Also at `bin/dde-greeter-setter/manager.go:74-77`.
</issue_to_address>
### Comment 3
<location path="bin/dde-greeter-setter/manager.go" line_range="84" />
<code_context>
logger.Warning(err)
+ return dbusutil.ToError(err)
}
updateXSettingsConfig()
- return dbusutil.ToError(err)
+ return nil
</code_context>
<issue_to_address>
**Concurrent updates leave theme inconsistent**
When two authorized D-Bus calls update the theme at the same time, `updateGreeterQtTheme` writes through the same temporary file and renames it while another request may still be writing; concurrent `updateXSettingsConfig` calls can also leave `xsettingsd.conf` describing a different theme from `qt-theme.ini`. Callers can see an error despite a changed theme, or the installed theme and settings can be corrupted or inconsistent.
Serialize the complete theme replacement and xsettings regeneration so only one update runs at a time.
Also at `bin/dde-greeter-setter/manager.go:79`.
</issue_to_address>
### Comment 4
<location path="bin/dde-greeter-setter/manager.go" line_range="85" />
<code_context>
}
updateXSettingsConfig()
- return dbusutil.ToError(err)
+ return nil
}
</code_context>
<issue_to_address>
**Failed settings writes report success**
When the accepted theme lacks usable DPI data or the xsettings config cannot be opened or written, `UpdateGreeterQtTheme` ignores failures from `updateXSettingsConfig` and returns nil after replacing the theme, leaving `qt-theme.ini` and `xsettingsd.conf` inconsistent or partial while callers see success.
Make `updateXSettingsConfig` return errors for invalid DPI data and config open or write failures, then propagate those errors from `UpdateGreeterQtTheme`.
</issue_to_address>
### Comment 5
<location path="bin/dde-greeter-setter/manager.go" line_range="68-69" />
<code_context>
+
+ // The theme file lives in /etc/lightdm and is shared by every user of
+ // the machine, so the caller must be authorized before it is touched.
+ authorized, err := securityloader.CheckPolkitAuth(m.service.Conn(),
+ string(sender), polkitActionUpdateQtTheme)
+ if err != nil {
+ logger.Warningf("failed to authorize %v: %v", sender, err)
</code_context>
<issue_to_address>
**Denied callers keep service alive**
When an unauthorized local caller repeatedly invokes the method, `DelayAutoQuit` runs before `CheckPolkitAuth`, so an unauthorized caller can repeatedly invoke this method and keep the otherwise auto-quitting service running indefinitely despite every request being denied.
Move `DelayAutoQuit` after successful authorization.
</issue_to_address>
### Comment 6
<location path="bin/dde-greeter-setter/manager_test.go" line_range="79-82" />
<code_context>
+ // The rendered line must never contain a second key/value pair, no
+ // matter what the caller put into the theme file.
+ value := "Sans\"\nNet/IconThemeName \"attacker\"\nQt/FontName \"" + "x"
+ quoted := quoteXSettingsValue(value)
+ assert.Empty(t, quoted)
+ if quoted != "" {
+ assert.NotContains(t, fmt.Sprintf("%s %s\n", xsettingsKeyFont, quoted), "\nNet/")
+ }
+}
</code_context>
<issue_to_address>
**Injection regression goes undetected**
When `updateXSettingsConfig` writes the raw value instead of using `quoteXSettingsValue`, `Test_updateXSettingsConfigInjectionBlocked` calls only `quoteXSettingsValue`; `assert.Empty` passes and the following `if quoted != ""` skips its assertion. `updateXSettingsConfig` can therefore write the malicious value unsafely while this test still passes.
Exercise `updateXSettingsConfig` with a malicious theme value and assert the generated configuration contains no injected key.
</issue_to_address>| dbusPath = "/org/deepin/dde/Greeter1" | ||
| dbusInterface = dbusServiceName | ||
|
|
||
| polkitActionUpdateQtTheme = "org.deepin.dde.greeter.update-qt-theme" |
There was a problem hiding this comment.
🟠 High · Greeter theme updates are denied
When the package is built and installed without a separately supplied greeter policy, CheckPolkitAuth cannot resolve org.deepin.dde.greeter.update-qt-theme, so UpdateGreeterQtTheme returns an authorization error even for active-session callers and theme updates fail.
Add a tracked policy definition for this action and include it in the policy generation and installation process.
Also at bin/dde-greeter-setter/manager.go:68-69.
Prompt for AI agents
In `bin/dde-greeter-setter/manager.go` at line 28:
**Greeter theme updates are denied**
When the package is built and installed without a separately supplied greeter policy, `CheckPolkitAuth` cannot resolve `org.deepin.dde.greeter.update-qt-theme`, so `UpdateGreeterQtTheme` returns an authorization error even for active-session callers and theme updates fail.
Add a tracked policy definition for this action and include it in the policy generation and installation process.
Also at `bin/dde-greeter-setter/manager.go:68-69`.
fly602
force-pushed
the
agent/agent/9955638ca40d
branch
from
November 12, 2026 13:44
bf1867b to
509ed91
Compare
1. UpdateGreeterQtTheme took the caller's sender and never used it, so any
local user could overwrite /etc/lightdm/deepin/qt-theme.ini, a file
shared by every account, and make the service rebuild xsettingsd.conf
from it.
2. The method now authorizes the caller against the new polkit action
org.deepin.dde.greeter.update-qt-theme before touching the files. The
action keeps the current product behaviour: an active session may
update the theme, because the session's XSettings service does that
whenever the user changes scale or fonts and cannot show a prompt,
while inactive sessions and session-less callers are refused.
3. updateXSettingsConfig built lines with fmt.Sprintf("%s \"%s\"", key,
value) from values the caller controls, so a quote or newline injected
extra xsettingsd keys. Non-integer values now go through
quoteXSettingsValue, which rejects quotes, backslashes and control
characters; such a value is skipped instead of written.
4. A failed theme write now returns an error instead of rebuilding
xsettingsd.conf from a stale file and reporting success.
fix: 登录界面主题补鉴权并修复配置注入
1. UpdateGreeterQtTheme 收到 sender 却从未使用，任意本地用户可覆盖全机
共享的 /etc/lightdm/deepin/qt-theme.ini，并让服务据此重写
xsettingsd.conf。
2. 方法在读写文件前先用新增的 polkit 动作
org.deepin.dde.greeter.update-qt-theme 鉴权，并保持现有产品行为：
活跃会话可修改（会话内的 XSettings 服务在用户调整缩放/字体时调用
该接口，无法弹出认证框），非活跃会话与无会话调用者一律拒绝。
3. updateXSettingsConfig 原先用 fmt.Sprintf 拼接调用方可控的值，引号或
换行即可注入额外的 xsettingsd 键；非整数值现在走
quoteXSettingsValue，拒绝引号、反斜杠与控制字符，命中则跳过该键。
4. 主题写入失败时返回错误，不再用旧文件重建 xsettingsd.conf 并报成功。
Log: 只有活跃会话内的用户可以修改登录界面主题，主题文件中的异常值不再能注入 xsettings 配置。
Influence: 1) 非活跃会话（如 ssh）调用 UpdateGreeterQtTheme 应被拒绝，qt-theme.ini 内容与 mtime 不变；2) 桌面活跃会话内调整缩放或字体仍能正常更新登录界面主题，不弹认证框；3) 构造含引号或换行的 qt-theme.ini 值，xsettingsd.conf 不应出现额外键；4) 写入失败时接口返回错误且不重建 xsettingsd.conf。
PMS: BUG-379743
fly602
force-pushed
the
agent/agent/9955638ca40d
branch
from
November 12, 2026 13:53
509ed91 to
441c0a8
Compare
1. Close the caller-supplied Unix FD when authorization fails, before returning from UpdateGreeterQtTheme. Otherwise repeated rejected calls leak descriptors until the greeter service stops accepting requests. 2. Serialize the complete theme-file replacement and xsettings regeneration. Two authorized callers can otherwise interleave writes through the fixed .tmp file and leave qt-theme.ini and xsettingsd.conf describing different themes. 3. Make updateXSettingsConfig return errors for theme load, xsettings open/write, and DPI/scale parsing failures, and propagate them from UpdateGreeterQtTheme instead of returning success after a partial update. Optional missing theme keys remain tolerated. 4. Move DelayAutoQuit after successful authorization so rejected callers cannot keep the auto-quitting service alive. 5. Replace the helper-only injection test with tests that exercise the real theme-to-xsettings conversion, including a quote-based injected key, missing DPI, and optional keys. 6. Guard against a nil file from an invalid Unix FD. 7. Run dde-greeter-setter as root and allow root to own org.deepin.dde.Greeter1. PolicyKit only permits trusted callers to check authorization for another identity; while the service ran as lightdm, CheckAuthorization was rejected for every caller, including active ones. fix: 加固登录界面主题更新 1. 鉴权失败时关闭调用方传入的 Unix FD，避免反复拒绝调用导致文件描述符 泄漏。 2. 将主题文件替换和 xsettings 重新生成串行化，避免两个已授权调用交错写入 固定 .tmp 文件，导致两个配置文件描述不同主题。 3. updateXSettingsConfig 对主题加载、xsettings 打开/写入、DPI/缩放解析失败 返回错误，并由 UpdateGreeterQtTheme 传播，不再部分更新后报成功；可选 主题键缺失仍然容忍。 4. DelayAutoQuit 移到鉴权成功后，避免未授权调用让服务一直存活。 5. 用真实主题到 xsettings 转换的测试替代只测 helper 的注入测试，覆盖含 引号的注入键、缺少 DPI、可选键缺失。 6. 对非法 Unix FD 做 nil 检查。 7. dde-greeter-setter 改为 root 运行，并允许 root 拥有 org.deepin.dde.Greeter1。polkit 只允许受信任调用者为其他身份检查 授权；服务以 lightdm 运行时，所有调用者的 CheckAuthorization 都会被 拒绝，包括活跃会话。 Log: 登录界面主题更新在鉴权失败时不再泄漏 fd，并发更新不再互相覆盖，xsettings 写入失败会返回错误；活跃会话可以正常更新，非活跃/无会话调用被拒绝。 Influence: 1) 反复未授权调用不会持续增加 greeter 进程 fd；2) 两个活跃会话并发更新时 qt-theme.ini 与 xsettingsd.conf 保持一致；3) 缺少 DPI 或 xsettings 写入失败时接口返回错误；4) 未授权调用不会延长服务自动退出时间；5) 注入测试实际覆盖 updateXSettingsConfig；6) 活跃会话更新成功，非活跃/无会话调用返回 access denied。 PMS: BUG-379743
deepin pr auto review🤖 AI 代码审查报告总体评分： 📊 总体评价
🔍 详细分析1. 语法逻辑 ✅评价： 边界处理需小幅完善 ✅ 通过 潜在问题： 建议： ['在 updateGreeterQtTheme 函数中 os.NewFile 返回 nil 的分支增加 closeUnixFD(fd) 调用，确保所有错误路径都正确释放文件描述符'] 2. 代码质量 ✅评价： 代码结构清晰，注释完整 ✅ 通过 潜在问题： 建议： ['将 len(substr) < 2 修改为 len(substr) < 3，使边界检查与正则捕获组数量一致，提高代码可读性和可维护性'] 3. 代码性能 ✅评价： 性能良好，资源使用合理 ✅ 通过 潜在问题： 建议： ['可将 regexp.MustCompile(^([0-9]+),([0-9]+)$) 提取为包级变量，避免每次调用重新编译，虽当前调用频率低影响不大，但属于最佳实践'] 4. 代码安全 🔒评价： 存在0个安全漏洞 ✅ 通过
安全漏洞详情： 建议： ['建议在 updateGreeterQtTheme 的 os.NewFile nil 分支也调用 closeUnixFD(fd)，虽然该路径极少触发，但保持一致的资源清理习惯有助于防御性编程'] 💡 改进建议代码示例// bin/dde-greeter-setter/manager.go
// 修复 updateGreeterQtTheme 中 os.NewFile 返回 nil 时的文件描述符泄漏
func updateGreeterQtTheme(fd dbus.UnixFD) error {
f := os.NewFile(uintptr(fd), "")
if f == nil {
closeUnixFD(fd) // 新增：确保所有错误路径都关闭文件描述符
return fmt.Errorf("invalid file descriptor %d", fd)
}
defer f.Close()
// ... 其余逻辑不变
}
// 同时建议修正 len(substr) 检查以提升可读性：
// 修改前: if len(substr) < 2 {
// 修改后: if len(substr) < 3 {本报告由 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.
安全问题修复
对应报告项：USRC
Greeter1_登录界面配置未鉴权与 bughuntercwe_306_dde_greeter（第 5 项，两条为同一处代码）。修复内容
UpdateGreeterQtTheme收到sender却从未使用，任意本地用户可覆盖全机共享的/etc/lightdm/deepin/qt-theme.ini，并让服务重写xsettingsd.conf。现在入口先用新增的 polkit 动作org.deepin.dde.greeter.update-qt-theme鉴权，未授权直接返回，不读写任何文件。org.deepin.dde.XSettings1（dde-servicesxsettingsmanager.cpp里在调整缩放/字体后调用），它没有可用的认证弹窗，而「活跃会话调整缩放即调整登录界面主题」是现有行为。因此策略里allow_active=yes、allow_inactive=no、allow_any=no：活跃会话免认证，非活跃会话（ssh、后台会话）与无会话调用被拒。原先定义后从未被调用的org.deepin.dde.greeter.doAction保持不动。updateXSettingsConfig用fmt.Sprintf("%s \"%s\"\n", key, value)拼接，value来自调用方可控的主题文件，含引号或换行即可注入额外 xsettingsd 键。非整数值改为走quoteXSettingsValue，拒绝引号、反斜杠与控制字符，命中即跳过该键而不是写入。xsettingsd.conf并返回成功。验证
go build ./bin/dde-greeter-setter/、go vet通过（仅依赖库既有 cgo warning）。go test ./bin/dde-greeter-setter/通过；新增quoteXSettingsValue用例覆盖引号、反斜杠、换行、制表符、DEL 与中文，以及「注入后不会出现第二组键」的断言。验收步骤
UpdateGreeterQtTheme：应返回拒绝，/etc/lightdm/deepin/qt-theme.ini内容与 mtime 不变。"或换行的qt-theme.ini值：xsettingsd.conf不应出现额外键。xsettingsd.conf。PMS: BUG-379743
Summary by Sourcery
Secure greeter theme updates with session-aware authorization, safe configuration generation, and reliable error handling.
New Features:
Bug Fixes:
Enhancements:
Deployment:
Tests: