fix: remove bash -c for security hardening #1374
Merged
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. Replaced bash -c command execution with direct process execution for improved security 2. Changed from using bash shell to directly execute setxkbmap command 3. Implemented manual parsing of setxkbmap output instead of relying on grep and awk through bash 4. Added proper handling of output format including colon removal when present Log: Improved security by removing bash -c command execution Influence: 1. Test lock screen functionality still works correctly 2. Verify keyboard options are properly retrieved and restored 3. Test with different keyboard configurations to ensure parsing works correctly 4. Verify no regression in lock screen behavior 5. Test security by attempting command injection (should now be prevented) 6. Verify process execution works without bash shell dependencies fix: 移除 bash -c 以提高安全性 1. 将 bash -c 命令执行替换为直接进程执行以提高安全性 2. 从使用 bash shell 改为直接执行 setxkbmap 命令 3. 实现了手动解析 setxkbmap 输出，而不是通过 bash 依赖 grep 和 awk 4. 添加了适当的输出格式处理，包括存在冒号时的移除处理 Log: 通过移除 bash -c 命令执行提高了安全性 Influence: 1. 测试锁屏功能是否仍然正常工作 2. 验证键盘选项是否正确获取和恢复 3. 使用不同的键盘配置测试以确保解析正常工作 4. 验证锁屏行为没有回归问题 5. 测试安全性，尝试命令注入（现在应该被阻止） 6. 验证进程执行在没有 bash shell 依赖的情况下正常工作
18202781743
requested review from
BLumia,
fly602,
mhduiy,
robertkill,
wjyrich and
yixinshark
deepin pr auto review我来对这段代码的修改进行审查：
建议进一步改进：
if (process.exitCode() != 0) {
qWarning() << "Failed to execute setxkbmap -query";
return;
}
QRegularExpression re(R"(option:\s*(\S+))");
QRegularExpressionMatch match = re.match(output);
if (match.hasMatch()) {
originMap = match.captured(1);
}
if (!process.waitForFinished(5000)) { // 5秒超时
qWarning() << "setxkbmap command timeout";
process.kill();
return;
}总体来说，这是一个很好的改进，提高了代码的安全性和可维护性，虽然可能会有轻微的性能影响，但在实际使用中不会造成问题。 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReplaces a shell-based setxkbmap invocation with direct process execution and in-code parsing of its output to improve security and robustness around keyboard option handling in the X11 lock screen path. Sequence diagram for direct setxkbmap execution and parsing in x11LockScreensequenceDiagram
participant ShutdownApplet
participant QProcess
participant setxkbmap
ShutdownApplet->>QProcess: start(/usr/bin/setxkbmap, -query)
QProcess->>setxkbmap: exec -query
setxkbmap-->>QProcess: stdout (keyboard layout info)
QProcess-->>ShutdownApplet: readAllStandardOutput()
ShutdownApplet->>ShutdownApplet: split output into lines
loop each_line
ShutdownApplet->>ShutdownApplet: check line contains option
alt line contains option
ShutdownApplet->>ShutdownApplet: split by space (SkipEmptyParts)
alt parts size >= 2
ShutdownApplet->>ShutdownApplet: originMap = parts[1]
alt originMap endsWith colon
ShutdownApplet->>ShutdownApplet: originMap = originMap without trailing colon
end
ShutdownApplet->>ShutdownApplet: break loop
end
end
end
ShutdownApplet->>QProcess: start(/usr/bin/setxkbmap, -option grab:break_actions)
Flow diagram for parsing setxkbmap output to extract keyboard optionsflowchart TD
A[start x11LockScreen keyboard option retrieval] --> B[run /usr/bin/setxkbmap -query via QProcess]
B --> C[read stdout as UTF-8 string]
C --> D[split output by newline into lines]
D --> E{for each line}
E -->|next line| F{line contains option}
F -->|no| E
F -->|yes| G[split line by spaces with SkipEmptyParts into parts]
G --> H{parts size >= 2}
H -->|no| E
H -->|yes| I[set originMap to parts index 1]
I --> J{originMap ends with colon}
J -->|yes| K[originMap = originMap without trailing colon]
J -->|no| L[keep originMap unchanged]
K --> M[break loop]
L --> M[break loop]
M --> N[use originMap when setting lock screen keyboard options]
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 3 issues, and left some high level feedback:
- The parsing logic for the
setxkbmap -queryoutput will be more robust if you target the expected key explicitly (e.g., checkline.startsWith("options:")instead ofline.contains("option")) to avoid accidental matches on unrelated lines. - Consider using
process.waitForFinished()'s return value and/or checkingprocess.exitStatus()/exitCode()to handle failures fromsetxkbmapexplicitly, so that an unexpected error doesn’t silently leaveoriginMapunset.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The parsing logic for the `setxkbmap -query` output will be more robust if you target the expected key explicitly (e.g., check `line.startsWith("options:")` instead of `line.contains("option")`) to avoid accidental matches on unrelated lines.
- Consider using `process.waitForFinished()`'s return value and/or checking `process.exitStatus()/exitCode()` to handle failures from `setxkbmap` explicitly, so that an unexpected error doesn’t silently leave `originMap` unset.
## Individual Comments
### Comment 1
<location> `applets/dde-shutdown/shutdownapplet.cpp:89-91` </location>
<code_context>
- originMap = QString::fromUtf8(process.readAllStandardOutput()).trimmed();
+ QString output = QString::fromUtf8(process.readAllStandardOutput());
+ QStringList lines = output.split('\n');
+ for (const QString &line : lines) {
+ // 查找包含 "option" 的行（原 grep option 命令）
+ if (line.contains("option")) {
+ // 模拟 awk -F ' ' '{print $2}'
+ // 按空格分割，跳过空字符串
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Tighten the line match to avoid accidental matches on unrelated text.
`line.contains("option")` matches the old `grep option` behavior but is overly broad and could pick up unrelated text (e.g. comments or localized strings). Since `setxkbmap -query` has a structured output, prefer a stricter condition like `line.startsWith("options:")` (or similar) to ensure you only parse the intended line.
```suggestion
for (const QString &line : lines) {
// 只匹配以 "options:" 开头的行，避免误匹配其他包含 "option" 的文本
if (line.startsWith("options:")) {
```
</issue_to_address>
### Comment 2
<location> `applets/dde-shutdown/shutdownapplet.cpp:88-104` </location>
<code_context>
+ if (line.contains("option")) {
+ // 模拟 awk -F ' ' '{print $2}'
+ // 按空格分割，跳过空字符串
+ QStringList parts = line.split(' ', Qt::SkipEmptyParts);
+ if (parts.size() >= 2) {
+ originMap = parts[1];
+ // 如果第二个字段以冒号结尾，可能需要进一步处理
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider guarding against malformed output or unexpected field positions.
This parsing relies on the options value always being the second non-empty token, as in the old `awk '{print $2}'` usage. If `setxkbmap -query` output changes (extra columns, different spacing, tabs), it may silently set a wrong `originMap`. To make this more robust, explicitly skip the label token and join the rest, or use a regex to capture everything after `options:` so spacing and extra fields don’t affect the result.
```suggestion
QStringList lines = output.split('\n');
for (const QString &line : lines) {
// 查找以 "options:" 开头的行，避免依赖字段位置
const QString trimmed = line.trimmed();
const QString label = QStringLiteral("options:");
if (trimmed.startsWith(label)) {
// 提取 "options:" 之后的所有内容并去除首尾空白
const QString value = trimmed.mid(label.length()).trimmed();
if (!value.isEmpty()) {
originMap = value;
}
break;
}
}
```
</issue_to_address>
### Comment 3
<location> `applets/dde-shutdown/shutdownapplet.cpp:91-103` </location>
<code_context>
+ if (parts.size() >= 2) {
+ originMap = parts[1];
+ // 如果第二个字段以冒号结尾，可能需要进一步处理
+ if (originMap.endsWith(':')) {
+ originMap = originMap.left(originMap.length() - 1);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The trailing-colon handling may be unnecessary and could hide real values.
In typical `setxkbmap -query` output, the colon belongs to the first token (e.g. `options:`), not the value, so `parts[1]` shouldn’t end with `:`. Unconditionally stripping a trailing colon from `originMap` could silently change valid values if the output format changes. Unless you’ve seen actual cases where the value ends with `:`, consider removing this special handling.
```suggestion
if (line.contains("option")) {
// 模拟 awk -F ' ' '{print $2}'
// 按空格分割，跳过空字符串
QStringList parts = line.split(' ', Qt::SkipEmptyParts);
if (parts.size() >= 2) {
// 直接使用第二个字段作为原始配置值，不进行额外的冒号处理
originMap = parts[1];
}
break;
}
```
</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
+89
to
+91
| for (const QString &line : lines) { | ||
| // 查找包含 "option" 的行（原 grep option 命令） | ||
| if (line.contains("option")) { |
There was a problem hiding this comment.
suggestion (bug_risk): Tighten the line match to avoid accidental matches on unrelated text.
line.contains("option") matches the old grep option behavior but is overly broad and could pick up unrelated text (e.g. comments or localized strings). Since setxkbmap -query has a structured output, prefer a stricter condition like line.startsWith("options:") (or similar) to ensure you only parse the intended line.
Suggested change
| for (const QString &line : lines) { | |
| // 查找包含 "option" 的行（原 grep option 命令） | |
| if (line.contains("option")) { | |
| for (const QString &line : lines) { | |
| // 只匹配以 "options:" 开头的行，避免误匹配其他包含 "option" 的文本 | |
| if (line.startsWith("options:")) { |
Comment on lines
+88
to
+104
| QStringList lines = output.split('\n'); | ||
| for (const QString &line : lines) { | ||
| // 查找包含 "option" 的行（原 grep option 命令） | ||
| if (line.contains("option")) { | ||
| // 模拟 awk -F ' ' '{print $2}' | ||
| // 按空格分割，跳过空字符串 | ||
| QStringList parts = line.split(' ', Qt::SkipEmptyParts); | ||
| if (parts.size() >= 2) { | ||
| originMap = parts[1]; | ||
| // 如果第二个字段以冒号结尾，可能需要进一步处理 | ||
| if (originMap.endsWith(':')) { | ||
| originMap = originMap.left(originMap.length() - 1); | ||
| } | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Consider guarding against malformed output or unexpected field positions.
This parsing relies on the options value always being the second non-empty token, as in the old awk '{print $2}' usage. If setxkbmap -query output changes (extra columns, different spacing, tabs), it may silently set a wrong originMap. To make this more robust, explicitly skip the label token and join the rest, or use a regex to capture everything after options: so spacing and extra fields don’t affect the result.
Suggested change
| QStringList lines = output.split('\n'); | |
| for (const QString &line : lines) { | |
| // 查找包含 "option" 的行（原 grep option 命令） | |
| if (line.contains("option")) { | |
| // 模拟 awk -F ' ' '{print $2}' | |
| // 按空格分割，跳过空字符串 | |
| QStringList parts = line.split(' ', Qt::SkipEmptyParts); | |
| if (parts.size() >= 2) { | |
| originMap = parts[1]; | |
| // 如果第二个字段以冒号结尾，可能需要进一步处理 | |
| if (originMap.endsWith(':')) { | |
| originMap = originMap.left(originMap.length() - 1); | |
| } | |
| } | |
| break; | |
| } | |
| } | |
| QStringList lines = output.split('\n'); | |
| for (const QString &line : lines) { | |
| // 查找以 "options:" 开头的行，避免依赖字段位置 | |
| const QString trimmed = line.trimmed(); | |
| const QString label = QStringLiteral("options:"); | |
| if (trimmed.startsWith(label)) { | |
| // 提取 "options:" 之后的所有内容并去除首尾空白 | |
| const QString value = trimmed.mid(label.length()).trimmed(); | |
| if (!value.isEmpty()) { | |
| originMap = value; | |
| } | |
| break; | |
| } | |
| } |
Comment on lines
+91
to
+103
| if (line.contains("option")) { | ||
| // 模拟 awk -F ' ' '{print $2}' | ||
| // 按空格分割，跳过空字符串 | ||
| QStringList parts = line.split(' ', Qt::SkipEmptyParts); | ||
| if (parts.size() >= 2) { | ||
| originMap = parts[1]; | ||
| // 如果第二个字段以冒号结尾，可能需要进一步处理 | ||
| if (originMap.endsWith(':')) { | ||
| originMap = originMap.left(originMap.length() - 1); | ||
| } | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): The trailing-colon handling may be unnecessary and could hide real values.
In typical setxkbmap -query output, the colon belongs to the first token (e.g. options:), not the value, so parts[1] shouldn’t end with :. Unconditionally stripping a trailing colon from originMap could silently change valid values if the output format changes. Unless you’ve seen actual cases where the value ends with :, consider removing this special handling.
Suggested change
| if (line.contains("option")) { | |
| // 模拟 awk -F ' ' '{print $2}' | |
| // 按空格分割，跳过空字符串 | |
| QStringList parts = line.split(' ', Qt::SkipEmptyParts); | |
| if (parts.size() >= 2) { | |
| originMap = parts[1]; | |
| // 如果第二个字段以冒号结尾，可能需要进一步处理 | |
| if (originMap.endsWith(':')) { | |
| originMap = originMap.left(originMap.length() - 1); | |
| } | |
| } | |
| break; | |
| } | |
| if (line.contains("option")) { | |
| // 模拟 awk -F ' ' '{print $2}' | |
| // 按空格分割，跳过空字符串 | |
| QStringList parts = line.split(' ', Qt::SkipEmptyParts); | |
| if (parts.size() >= 2) { | |
| // 直接使用第二个字段作为原始配置值，不进行额外的冒号处理 | |
| originMap = parts[1]; | |
| } | |
| break; | |
| } |
yixinshark
approved these changes
Dec 22, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, 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 |
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.
improved security
grep and awk through bash
present
Log: Improved security by removing bash -c command execution
Influence:
correctly
prevented)
fix: 移除 bash -c 以提高安全性
Log: 通过移除 bash -c 命令执行提高了安全性
Influence:
Summary by Sourcery
Harden keyboard option handling in the X11 lock screen flow by removing shell-based command execution and parsing setxkbmap output directly.
Bug Fixes:
Enhancements: