ref(systemd): Fix safety bugs and improve performance#1020
ref(systemd): Fix safety bugs and improve performance#1020GT-610 merged 4 commits intolollipopkit:mainfrom
Conversation
In systemd unit processing, filtering of special characters in unit names has been added to prevent command injection and security issues. Additionally, the rendering performance of the unit list has been optimized by merging unnecessary watch calls and removing confirmation dialogs to simplify the operation process.
Display a confirmation dialog when stopping or restarting systemd units to prevent accidental operations. For other operations, directly navigate to the SSH page to execute commands.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughSanitizes systemd unit names in command construction and provider parsing, adds quoting in generated shell loops, and refactors the UI unit-action flow into helper methods that centralize confirmation and SSH navigation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SystemdPage
participant ConfirmDialog
participant Navigator
participant SSHPage
participant SystemdProvider
User->>SystemdPage: selects unit + action
SystemdPage->>SystemdPage: _handleUnitFuncSelected(action, unit)
alt action is stop/restart
SystemdPage->>ConfirmDialog: show command + confirm button
ConfirmDialog->>User: request confirmation (CountDownBtn)
User-->>ConfirmDialog: confirms
ConfirmDialog->>SystemdPage: confirmed
end
SystemdPage->>Navigator: _navigateToSsh(command)
Navigator->>SSHPage: open with command
SSHPage->>SystemdProvider: request command execution (build/sanitize command)
SystemdProvider-->>SSHPage: returns constructed command / result
SSHPage->>User: show SSH session / execution output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)lib/data/provider/**/*.dart📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-01-17T04:24:16.501ZApplied to files:
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5705b3959a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/data/provider/systemd.dart`:
- Around line 80-83: The current extraction in unitNames_ uses .split('.').first
which truncates dotted unit names (e.g., org.cups.cupsd.service); change the
logic that derives name (the variable computed from unitNames.map in
systemd.dart) to remove only the final extension by locating the last '.' (e.g.,
lastIndexOf('.') and substring up to that index) instead of taking the first
segment, and then apply the sanitization; if you need to preserve dots in the
final sanitized name also update the RegExp(r'[^a-zA-Z0-9\-_]') to allow '.'
(e.g., include \.) so dots aren't stripped out after extraction.
🧹 Nitpick comments (2)
lib/data/model/server/systemd.dart (1)
97-100: Good security fix for command injection prevention.The sanitization correctly strips potentially dangerous characters from unit names before shell execution. However, note a minor inconsistency: this regex allows dots (
.) but the provider's_parseUnitObj(line 81-82) strips the extension and disallows dots in its sanitization pattern[^a-zA-Z0-9\-_].Since the
namefield is populated from the provider's sanitized output (which has no dots), this allowance is harmless but could be tightened for defense-in-depth:Optional: Align regex with provider
- return '$prefix ${func.name} ${name.replaceAll(RegExp(r'[^a-zA-Z0-9\-_.]'), '')}'; + return '$prefix ${func.name} ${name.replaceAll(RegExp(r'[^a-zA-Z0-9\-_]'), '')}';lib/view/page/systemd.dart (1)
103-133: Clean separation of concerns following UI guidelines.The refactoring into
_handleUnitFuncSelected,_showConfirmDialog, and_navigateToSshproperly separates the action handling logic from the build methods. This aligns with the coding guideline to split UI into build, actions, and utils.One minor note:
void _showConfirmDialog(String cmd) asyncuses theasync voidpattern. While common in Flutter UI callbacks, consider returningFuture<void>for better error propagation if needed in the future:Optional signature change
- void _showConfirmDialog(String cmd) async { + Future<void> _showConfirmDialog(String cmd) async {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/data/model/server/systemd.dartlib/data/provider/systemd.dartlib/view/page/systemd.dart
🧰 Additional context used
📓 Path-based instructions (4)
lib/view/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/view/**/*.dart: Prefer widgets/utilities from fl_lib for common UI (e.g., CustomAppBar, context.showRoundDialog, Input, Btnx.cancelOk)
Prefer using libL10n strings before adding new ones to project l10n
Split UI into build, actions, and utils; use extension on to separate concerns
Files:
lib/view/page/systemd.dart
lib/data/provider/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Riverpod providers by feature under lib/data/provider/
Files:
lib/data/provider/systemd.dart
lib/data/model/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
After modifying annotated model files, run: dart run build_runner build --delete-conflicting-outputs
Files:
lib/data/model/server/systemd.dart
lib/data/model/server/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place server management models under lib/data/model/server/
Files:
lib/data/model/server/systemd.dart
🧠 Learnings (1)
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/view/**/*.dart : Split UI into build, actions, and utils; use extension on to separate concerns
Applied to files:
lib/view/page/systemd.dart
🔇 Additional comments (3)
lib/data/provider/systemd.dart (1)
84-91: Good shell safety improvement with proper quoting.The changes correctly quote unit names in both the for-loop initialization and the
systemctl showinvocation, preventing word-splitting and glob expansion issues that could occur with special characters.lib/view/page/systemd.dart (2)
76-77: Good performance optimization by combining watches.Watching a tuple
(p.units, p.scopeFilter)ensures a single subscription that triggers rebuilds when either value changes, reducing unnecessary widget rebuilds compared to separate watch calls. This aligns with the PR's performance improvement goals.
113-128: Well-designed confirmation dialog for destructive operations.The 3-second countdown with
CountDownBtnis a good UX pattern that prevents accidental execution of stop/restart commands. Displaying the command in a code block viaSimpleMarkdownprovides transparency about what will be executed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Extend the regular expression to allow more valid characters, including dots, @, and colons, in system unit names, to support a broader range of unit naming conventions
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/provider/systemd.dart (1)
107-113: Preserve dotted unit names when parsingId=.
val.split('.')still truncates dotted unit names (e.g.,org.cups.cupsd.service→org). Now that dots are allowed upstream, parse using the last dot to keep the full base name.🐛 Suggested fix
- final vals = val.split('.'); - name = vals.first; - type = vals.last; + final dotIdx = val.lastIndexOf('.'); + if (dotIdx <= 0 || dotIdx == val.length - 1) continue; + name = val.substring(0, dotIdx); + type = val.substring(dotIdx + 1);
🤖 Fix all issues with AI agents
In `@lib/data/provider/systemd.dart`:
- Around line 88-89: The shell loop building systemctl invocations can be
tricked if a unit name begins with '-' because systemctl may parse it as an
option; update the command invocation in the code that builds the loop over
unitNames_ (the for unit in ... loop and the systemctl show call) to pass an
explicit end-of-options marker by adding -- before "$unit" (i.e. call systemctl
show -- -- "$unit"), ensuring unit names cannot be interpreted as options.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/data/model/server/systemd.dartlib/data/provider/systemd.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/data/model/server/systemd.dart
🧰 Additional context used
📓 Path-based instructions (1)
lib/data/provider/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Riverpod providers by feature under lib/data/provider/
Files:
lib/data/provider/systemd.dart
🧠 Learnings (1)
📓 Common learnings
Learnt from: GT-610
Repo: lollipopkit/flutter_server_box PR: 0
File: :0-0
Timestamp: 2026-01-22T05:10:54.484Z
Learning: For all future pull requests in the flutter_server_box repository, check if participants from issues resolved by the PR are added to the `participants` field in `lib/data/res/github_id.dart`. If any participants are missing, propose adding them. First verify if they already exist or have been added in the PR commits.
🔇 Additional comments (2)
lib/data/provider/systemd.dart (2)
80-85: Good update to unit-name extraction and allowed characters.This is a solid improvement for handling valid systemd naming conventions more safely.
1-3: No issues are closed by this commit, so no participants need to be added.The commit message contains no issue closure references (e.g., "Closes
#XXX"). Review participants for added/updated code if this PR resolves any issues through other means (e.g., PR description), and add them to theparticipantsfield inlib/data/res/github_id.dartif needed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
When dealing with service IDs containing multiple dots (such as org.cups.cupsd.service), correctly extract the service name and type. When there are no dots in the service ID, set the type to an empty string.
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.