SSH/server utility cleanup and small code security improvements#1221
Merged
Conversation
- Remove unused DecyptPem function (zero call sites) - Remove unused tryParseCompletedOutput static method (only used in tests) - Fix typo: loadIndentity -> loadIdentity - Update persistent_shell_test.dart to drop tests for removed method
…keys - sftp_sudo: use shell builtin printf instead of echo to pipe sudo password, so the password does not appear in the process argument list visible via ps - server_func_btns: write temp private key to system temp directory instead of the predictable app doc path, and extend retention to 5s for SSH connection establishment - server_func_btns: unconditionally clear clipboard after password copy timeout instead of skipping when content changed
- server_dedup: accept optional existingServers param to avoid duplicate ServerStore.fetch() calls in _resolveServers - all.dart: replace manual _isSameOrder with listEquals - server_private_info: replace _sameStringList with listEquals
- chan: change updateHomeWidget from void async to Future<void> with try-catch so callers can use unawaited properly - home: mark updateHomeWidget calls with unawaited - main: skip Computer.shared.turnOn when no servers configured - server: narrow _isJumpFailoverError keyword matching to avoid false positives on unrelated errors containing 'network' or 'socket'
- setting: rename backupasswd -> backupPassword - misc: rename multiBlankreg -> multiBlankReg for consistency - server: add ordering invariant comment for ServerConn operator<
- Restore decryptPem (was wrongly removed as dead code, actually called in private_key/edit.dart via Computer.shared.start) - Fix typo: decyptPem -> decryptPem in both definition and call site
Revert to checking clipboard content before clearing: only wipe the clipboard if it still holds the password. If the user copied something else during the 25s window, preserve their content.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThis PR renames several misspelled identifiers, adds optional existing-server inputs to deduplication helpers, switches list comparisons to Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/view/widget/server_func_btns.dart (2)
211-228: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse a unique temporary key path instead of
srvbox_pk_${spi.keyId}.Line 217 creates a stable filename in the shared system temp directory. Parallel SSH launches with the same key can clobber/delete each other’s key file, and a predictable private-key path is easier for local processes to discover. Create a unique temp directory/file per launch and clean up that directory.
Proposed fix
- File? tempKeyFile; + Directory? tempKeyDir; final shouldGenKey = spi.keyId != null; @@ if (shouldGenKey) { - final path = await () async { - final tempKeyFileName = 'srvbox_pk_${spi.keyId}'; - - /// Use system temp directory so the key file has a short-lived - /// random path and is cleaned up by the OS on reboot. - return Directory.systemTemp.path.joinPath(tempKeyFileName); - }(); - final file = File(path); - tempKeyFile = file; - if (await file.exists()) { - await file.delete(); - } + final dir = await Directory.systemTemp.createTemp('srvbox_pk_'); + tempKeyDir = dir; + final file = File(dir.path.joinPath('identity_key')); + final path = file.path; final keyContent = getPrivateKey(spi.keyId!); @@ } finally { - final file = tempKeyFile; - if (file != null && await file.exists()) { + final dir = tempKeyDir; + if (dir != null && await dir.exists()) { unawaited( Future.delayed(const Duration(seconds: 5), () async { try { - if (await file.exists()) { - await file.delete(); + if (await dir.exists()) { + await dir.delete(recursive: true); }Also applies to: 299-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/view/widget/server_func_btns.dart` around lines 211 - 228, The temp key generation in server_func_btns.dart uses a stable filename via the temp path logic around shouldGenKey, which can collide across parallel launches and leave the private key easy to discover. Update the key file creation in that block, and the matching code path around the later duplicate section, to generate a unique temporary directory or file per launch instead of srvbox_pk_${spi.keyId}, then write the key there and clean up the directory when done.
251-284: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDelete the temp SSH key after
sshexits, not on a 5s timer. The delayed cleanup can run before a slow terminal has startedsshand opened the-ifile, causing intermittent “identity file not found” failures. A wrapper that removes the key once the SSH process finishes would avoid the race.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/view/widget/server_func_btns.dart` around lines 251 - 284, The temporary SSH key cleanup in the launch flow is happening on a fixed delay, which can race with the terminal starting ssh and opening the identity file. Update the SSH launch path in server_func_btns.dart so the temp key is removed only after the ssh process exits, using the existing ssh/terminal launch logic around sshCommand and the platform-specific branches instead of a timer-based cleanup.
🧹 Nitpick comments (1)
lib/core/utils/server_dedup.dart (1)
119-127: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winApply the same single-fetch pattern to the SSH-config import path.
_resolveServersnow fetchesexistingonce and reuses it for bothdeduplicateServersandresolveNameConflicts. However,_processSSHServersinlib/view/page/setting/entries/ssh.dartstill calls both helpers withoutexistingServers, resulting in two separateServerStore.instance.fetch()calls on that import path — the exact redundancy this refactor removes here.♻️ Suggested fix (outside this file, in lib/view/page/setting/entries/ssh.dart)
Future<void> _processSSHServers(List<Spi> servers) async { - final deduplicated = ServerDeduplication.deduplicateServers(servers); - final resolved = ServerDeduplication.resolveNameConflicts(deduplicated); + final existing = ServerStore.instance.fetch(); + final deduplicated = ServerDeduplication.deduplicateServers(servers, existingServers: existing); + final resolved = ServerDeduplication.resolveNameConflicts(deduplicated, existingServers: existing); final summary = ServerDeduplication.getImportSummary(servers, resolved);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/core/utils/server_dedup.dart` around lines 119 - 127, The SSH-config import path still performs redundant ServerStore.instance.fetch() calls because _processSSHServers in ssh.dart invokes deduplicateServers and resolveNameConflicts without reusing a shared existing list. Update _processSSHServers to follow the same pattern as _resolveServers by fetching existing servers once, then passing that value into both deduplicateServers and resolveNameConflicts so the import flow only hits the store a single time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/core/utils/server.dart`:
- Around line 257-260: _update _isJumpFailoverError() in server.dart so it also
treats the Dart/OS wording "Network is unreachable" as a failover-triggering
network error; extend the existing errStr.contains checks alongside the current
network unreachable/socketexception/failed host lookup/forwardlocal matches,
keeping the logic in the same helper._
In `@lib/main.dart`:
- Around line 88-94: `Computer.shared` is only initialized inside the
`serversCount > 0` guard, but later backup/private-key/server flows call
`Computer.shared.start(...)` regardless of that count. Update `main` so
`Computer.shared.turnOn(...)` is invoked even when `serversCount == 0`, or make
the startup lazy by ensuring `turnOn()` runs before the first `start(...)` call
in the relevant flows. Use the existing `Computer.shared` and `turnOn`/`start`
symbols to locate the startup path and remove the dependency on `serversCount`.
---
Outside diff comments:
In `@lib/view/widget/server_func_btns.dart`:
- Around line 211-228: The temp key generation in server_func_btns.dart uses a
stable filename via the temp path logic around shouldGenKey, which can collide
across parallel launches and leave the private key easy to discover. Update the
key file creation in that block, and the matching code path around the later
duplicate section, to generate a unique temporary directory or file per launch
instead of srvbox_pk_${spi.keyId}, then write the key there and clean up the
directory when done.
- Around line 251-284: The temporary SSH key cleanup in the launch flow is
happening on a fixed delay, which can race with the terminal starting ssh and
opening the identity file. Update the SSH launch path in server_func_btns.dart
so the temp key is removed only after the ssh process exits, using the existing
ssh/terminal launch logic around sshCommand and the platform-specific branches
instead of a timer-based cleanup.
---
Nitpick comments:
In `@lib/core/utils/server_dedup.dart`:
- Around line 119-127: The SSH-config import path still performs redundant
ServerStore.instance.fetch() calls because _processSSHServers in ssh.dart
invokes deduplicateServers and resolveNameConflicts without reusing a shared
existing list. Update _processSSHServers to follow the same pattern as
_resolveServers by fetching existing servers once, then passing that value into
both deduplicateServers and resolveNameConflicts so the import flow only hits
the store a single time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 640b0dfd-89b8-4077-a547-4a79f3272f56
📒 Files selected for processing (17)
lib/core/chan.dartlib/core/utils/server.dartlib/core/utils/server_dedup.dartlib/core/utils/sftp_sudo.dartlib/data/model/container/ps.dartlib/data/model/server/server.dartlib/data/model/server/server_private_info.dartlib/data/provider/server/all.dartlib/data/res/misc.dartlib/data/ssh/persistent_shell.dartlib/data/store/setting.dartlib/main.dartlib/view/page/home.dartlib/view/page/private_key/edit.dartlib/view/page/setting/entries/app.dartlib/view/widget/server_func_btns.darttest/persistent_shell_test.dart
💤 Files with no reviewable changes (2)
- test/persistent_shell_test.dart
- lib/data/ssh/persistent_shell.dart
- server.dart: add 'network is unreachable' to failover error matching - main.dart: restore unconditional Computer.shared.turnOn to avoid hangs when backup/private-key flows call Computer.shared.start with zero servers configured - server_func_btns: use unique temp directory per launch instead of stable filename, and extend key cleanup delay to 30s, delete the whole temp directory on cleanup - ssh.dart: reuse a single Stores.server.fetch() call in _processSSHServers for both deduplicateServers and resolveNameConflicts
GT-610
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/view/widget/server_func_btns.dart`:
- Around line 294-304: The cleanup in server_func_btns.dart currently always
waits 30 seconds before deleting the temporary SSH key directory, even when SSH
never launched. Update the logic around the `Future.delayed` cleanup and the
`Process.start` launch path to track whether SSH actually started successfully,
and delete the temp directory immediately on failure paths while keeping the
grace delay only after a successful launch. Use the existing `file`, `parent`,
and `Loggers.app.warning` cleanup block as the place to adjust the conditional
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a42fe372-aff0-4702-a529-6dd5ceea309a
📒 Files selected for processing (3)
lib/core/utils/server.dartlib/view/page/setting/entries/ssh.dartlib/view/widget/server_func_btns.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/core/utils/server.dart
Track whether SSH actually started via sshLaunched flag set after each successful Process.start. On failure paths (exception, unsupported platform, or key prep error) the temp key directory is deleted right away in the finally block instead of waiting 30 seconds.
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
1 participant
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.
dartssh2 still have some issues, so not going to update here
Summary by CodeRabbit