feat: prompt user on host key verification#943
Conversation
| Caution Review failedThe pull request is closed. WalkthroughAdds interactive SSH host-key verification (prompt, caching, persistence) plus UI wiring for dialogs, settings storage support for known fingerprints, SFTP host-key checks before transfers, a global navigator key, and new localization strings for host-key prompts in many languages. Changes
Sequence Diagram(s)sequenceDiagram participant User participant UI as Storage UI participant Helper as HostKeyHelper participant Store as SettingStore participant SSH as SSH Client / genClient participant Prompt as Prompt Flow User->>UI: Start SFTP action (upload/download) UI->>Helper: ensureHostKeyAcceptedForSftp(context, spi) Helper->>Store: read sshKnownHostFingerprints alt fingerprint known Helper-->>UI: return true else unknown Helper->>Prompt: show loading + trigger ensureKnownHostKey(spi) Prompt->>SSH: genClient + verify host key SSH-->>Prompt: fingerprint (md5 hex/base64, type) Prompt->>User: show fingerprint prompt User->>Prompt: accept / reject alt accept Prompt->>Store: persist fingerprint via onHostKeyAccepted Prompt-->>Helper: true Helper-->>UI: true else reject Prompt-->>Helper: false Helper-->>UI: false end end opt accepted UI->>SSH: proceed with SFTP operation end Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (17)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/view/page/storage/local.dart (1)
42-60: Refactor: Code duplication detected (see earlier comment).This
_ensureHostKeyAcceptedmethod is identical to the one inlib/view/page/storage/sftp.dart(lines 52-70). Please refer to the refactoring suggestion in that file to extract this to a shared utility.
🧹 Nitpick comments (14)
lib/l10n/app_zh_tw.arb (2)
174-175: Add translator notes and regenerate l10nString looks good. Please add ARB metadata (@"privateKeyNotFoundFmt": {"description": "...","placeholders":{"keyId":{"type":"String"}}}) to aid translators, and run flutter gen-l10n after ARB edits. Also confirm consumers pass keyId.
229-233: Placeholders and terminology check for SSH host key promptsCopy reads well. Please:
- Keep MD5 terms consistent across locales (e.g., “MD5 十六進位”).
- Add placeholder metadata for serverName and fingerprint.
- Regenerate localization classes (flutter gen-l10n).
lib/view/page/setting/entries/app.dart (2)
288-294: Gate “(Dev) Edit raw json” behind a developer flagCurrently visible to all users. Please gate behind BuildMode.isDebug, a hidden “dev mode” toggle, or Beta flag to prevent accidental destructive edits; optionally style with a danger color/icon.
296-404: Harden raw-settings editor: multi-encrypted selection, safer saves, and UI consistency
- Only first encrypted key is handled. If multiple exist, offer a picker of encrypted entries before decrypting.
- Non-encrypted path overwrites and deletes missing keys. Add a confirmation summarizing added/changed/removed counts; consider backing up the current map to a timestamped key before applying.
- Don’t retain password longer than needed; avoid class-level capture and clear locals after use.
- Use Btnx.cancelOk for dialog actions to match project UI conventions.
- Consider excluding internal keys by default (includeInternalKeys: false) with an “Include internal keys” toggle.
Minimal UI consistency diff (actions area):
- actions: [ - TextButton(onPressed: () => context.pop(null), child: Text(libL10n.cancel)), - TextButton(onPressed: () => context.pop(controller.text.trim()), child: Text(libL10n.ok)), - ], + actions: Btnx.cancelOk( + onCancel: () => context.pop(null), + onOk: () => context.pop(controller.text.trim()), + ),lib/view/page/setting/entries/home_tabs.dart (1)
86-90: Spacing change vs previous Card marginSwitch to Padding + .cardx changes horizontal/vertical spacing (8/1 vs previous Card’s margin, likely ~16/4). If visual density regressed, consider padding: EdgeInsets.symmetric(horizontal: 16, vertical: 4) to match prior layout. Reorderable key placement is correct.
lib/l10n/app_tr.arb (2)
174-175: Regenerate l10n and add placeholder metadataText is fine. Please add metadata with placeholder types/descriptions for keyId and run flutter gen-l10n.
229-233: Ensure consistent placeholder names and terminologyPlaceholders serverName and fingerprint look consistent. Consider “MD5 onaltılık” for “hex” if you prefer fully localized wording. Don’t forget to regenerate localization classes.
lib/l10n/app_pt.arb (1)
174-175: Add ARB descriptions for translators and regenerateLooks good. Please add @"privateKeyNotFoundFmt" metadata with placeholder schema and run flutter gen-l10n.
lib/data/store/setting.dart (1)
75-85: LGTM with suggestion: Consider documenting the key format.The implementation correctly persists SSH host key fingerprints with proper type conversion and safe defaults. The
fromObjconverter handles edge cases well.Consider adding a comment documenting the expected key format for clarity:
/// Trusted SSH host key fingerprints keyed by `serverId::keyType`. /// Example key: "server123::ssh-ed25519" or "server456::ssh-rsa" late final sshKnownHostFingerprints = propertyDefault<Map<String, String>>(lib/core/utils/server.dart (5)
59-62: New host-key API surface is solid; please document the contract.The added params and default wiring look good. Add brief dartdoc to clarify:
- knownHostFingerprints key format (“::”).
- onHostKeyAccepted persistence expectations and when it fires (new vs update).
- onHostKeyPrompt async behavior and fail-closed default.
Also applies to: 65-68
218-221: Unify mismatch log into a single message.Combine details into one line for easier scanning and consistent logger usage.
- Loggers.app.warning( - 'SSH host key mismatch for ${spi.name}', - 'expected $existing but received $fingerprintHex ($keyType)', - ); + Loggers.app.warning( + 'SSH host key mismatch for ${spi.name} ($keyType): expected $existing but received $fingerprintHex.' + );
298-331: ensureKnownHostKey flow is good; consider passing prompt/persist for consistency.You pass cache but rely on defaults for prompt/persist. Optional: thread through onHostKeyPrompt/onHostKeyAccepted so callers can customize UX in this preflight too.
232-240: Storage helpers are robust; consider growth control.Map copy/update with early‑return is good. Optionally add a cap/eviction or diagnostics (count, size) to prevent unbounded growth in long‑running installs.
Also applies to: 242-255
345-354: Fingerprint formatting helpers: clarify algorithm in names/comments._fingerprintToHex and _fingerprintToBase64 assume the input is already a digest. Rename or document (e.g., “MD5 digest to hex with colons”). If SHA256 is used, reflect that in labels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
lib/generated/l10n/l10n.dartis excluded by!**/generated/**lib/generated/l10n/l10n_de.dartis excluded by!**/generated/**lib/generated/l10n/l10n_en.dartis excluded by!**/generated/**lib/generated/l10n/l10n_es.dartis excluded by!**/generated/**lib/generated/l10n/l10n_fr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_id.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ja.dartis excluded by!**/generated/**lib/generated/l10n/l10n_nl.dartis excluded by!**/generated/**lib/generated/l10n/l10n_pt.dartis excluded by!**/generated/**lib/generated/l10n/l10n_ru.dartis excluded by!**/generated/**lib/generated/l10n/l10n_tr.dartis excluded by!**/generated/**lib/generated/l10n/l10n_uk.dartis excluded by!**/generated/**lib/generated/l10n/l10n_zh.dartis excluded by!**/generated/**
📒 Files selected for processing (24)
.github/workflows/analysis.yml(0 hunks)lib/app.dart(2 hunks)lib/core/app_navigator.dart(1 hunks)lib/core/utils/server.dart(7 hunks)lib/data/store/setting.dart(1 hunks)lib/l10n/app_de.arb(3 hunks)lib/l10n/app_en.arb(3 hunks)lib/l10n/app_es.arb(3 hunks)lib/l10n/app_fr.arb(3 hunks)lib/l10n/app_id.arb(3 hunks)lib/l10n/app_ja.arb(3 hunks)lib/l10n/app_nl.arb(3 hunks)lib/l10n/app_pt.arb(3 hunks)lib/l10n/app_ru.arb(3 hunks)lib/l10n/app_tr.arb(3 hunks)lib/l10n/app_uk.arb(3 hunks)lib/l10n/app_zh.arb(3 hunks)lib/l10n/app_zh_tw.arb(3 hunks)lib/view/page/server/connection_stats.dart(5 hunks)lib/view/page/setting/entries/app.dart(1 hunks)lib/view/page/setting/entries/home_tabs.dart(1 hunks)lib/view/page/setting/entries/server.dart(0 hunks)lib/view/page/storage/local.dart(3 hunks)lib/view/page/storage/sftp.dart(6 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/analysis.yml
- lib/view/page/setting/entries/server.dart
🧰 Additional context used
📓 Path-based instructions (3)
lib/l10n/**/*.arb
📄 CodeRabbit inference engine (CLAUDE.md)
lib/l10n/**/*.arb: After modifying ARB files, run: flutter gen-l10n to regenerate localization
Keep localized strings in ARB files under lib/l10n/ (Flutter i18n)
Files:
lib/l10n/app_ru.arblib/l10n/app_tr.arblib/l10n/app_uk.arblib/l10n/app_de.arblib/l10n/app_nl.arblib/l10n/app_pt.arblib/l10n/app_id.arblib/l10n/app_zh_tw.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_zh.arblib/l10n/app_ja.arblib/l10n/app_fr.arb
lib/data/store/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Persist state using Hive stores placed under lib/data/store/
Files:
lib/data/store/setting.dart
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/storage/local.dartlib/view/page/setting/entries/home_tabs.dartlib/view/page/server/connection_stats.dartlib/view/page/setting/entries/app.dartlib/view/page/storage/sftp.dart
🧠 Learnings (5)
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR PR: lollipopkit/flutter_server_box#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-08T09:36:41.682Z Learning: Applies to lib/data/model/sftp/**/*.dart : Place SFTP models under lib/data/model/sftp/ Applied to files:
lib/view/page/storage/local.dartlib/view/page/storage/sftp.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR PR: lollipopkit/flutter_server_box#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-08T09:36:41.682Z Learning: Applies to lib/data/model/ssh/**/*.dart : Place SSH models under lib/data/model/ssh/ Applied to files:
lib/view/page/storage/local.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR PR: lollipopkit/flutter_server_box#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-08T09:36:41.682Z Learning: Applies to lib/view/**/*.dart : Prefer widgets/utilities from fl_lib for common UI (e.g., CustomAppBar, context.showRoundDialog, Input, Btnx.cancelOk) Applied to files:
lib/app.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR PR: lollipopkit/flutter_server_box#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-08T09:36:41.682Z Learning: Applies to lib/view/**/*.dart : Prefer using libL10n strings before adding new ones to project l10n Applied to files:
lib/app.dart
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR PR: lollipopkit/flutter_server_box#0 File: CLAUDE.md:0-0 Timestamp: 2025-10-08T09:36:41.682Z Learning: Applies to lib/data/model/server/**/*.dart : Place server management models under lib/data/model/server/ Applied to files:
lib/view/page/storage/sftp.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (15)
lib/l10n/app_pt.arb (1)
229-233: Perfect! The verification is complete. All critical concerns have been resolved:✅ All six new keys exist in all 13 locales
✅ All consumers pass correct parameters matching the placeholders (serverName, fingerprint, keyId)
✅ Generated l10n files exist (lib/generated/l10n/) confirming flutter gen-l10n was runThe only remaining item from the original review is an optional suggestion about terminology ("hex" vs "hexadecimal"), which is a matter of localization preference, not a correctness issue.
No actionable issues require resolution.
lib/l10n/app_fr.arb (1)
174-174: Ensure localization regeneration after merge.As per coding guidelines, after modifying ARB files, you need to run
flutter gen-l10nto regenerate localization code. Please verify this step will be completed before deployment.Based on coding guidelines.
Also applies to: 229-233
lib/view/page/server/connection_stats.dart (2)
44-44: Nice UX improvement.Adding a tooltip to the refresh button improves accessibility and user experience.
73-140: Layout refactoring looks clean.The refactoring from Card-based to Padding-based layout with
.cardxsimplifies the structure while maintaining functionality. The inline success rate display and consolidated time items improve readability.lib/app.dart (1)
6-6: LGTM! Global navigator enables host key prompts.Wiring the global navigator key enables the app to show dialogs from non-UI contexts, which is essential for SSH host key verification prompts introduced in this PR.
Also applies to: 91-91
lib/view/page/storage/sftp.dart (1)
311-313: LGTM: Host key verification is consistently enforced.The host key verification is properly enforced before all SFTP operations (edit, save callback, download, upload). This ensures that users cannot accidentally perform SFTP operations with untrusted hosts, which is an important security measure.
Also applies to: 327-330, 354-356, 688-690
lib/core/app_navigator.dart (1)
1-8: LGTM: Clean global navigator implementation.The
AppNavigatorclass follows Flutter best practices for providing global navigator access. The use of an abstract final class with static members is appropriate for this utility pattern, and the optional context getter is useful for conditional navigation flows.lib/view/page/storage/local.dart (1)
396-398: LGTM: Host key verification before upload.The host key verification is properly enforced before uploading files to the remote server, preventing uploads to untrusted hosts.
lib/l10n/app_id.arb (1)
174-174: Reminder: Regenerate localization files.After modifying ARB files, ensure you run:
flutter gen-l10nAs per coding guidelines.
Also applies to: 229-233
lib/l10n/app_en.arb (1)
174-174: No action required—localization files already regenerated.The ARB files in
lib/l10n/and their corresponding generated files inlib/generated/l10n/were both modified in the same commit, confirming thatflutter gen-l10nwas already executed. The generated localization files are up-to-date.lib/l10n/app_zh.arb (1)
174-174: Reminder: Regenerate localization files.After modifying ARB files (lines 174, 229-233), ensure you run the following command to regenerate localization classes:
flutter gen-l10nPer coding guidelines for ARB modifications. Generated files are gitignored but should be regenerated locally to ensure app_localizations files reflect the new SSH host key verification strings.
lib/core/utils/server.dart (4)
109-115: Host-key verifier integration looks correct.Passing hostKeyVerifier.call into onVerifyHostKey for both pwd and key auth paths is consistent. Nice.
Also applies to: 124-124, 138-138
257-263: Fail‑closed default is secure; ensure non‑UI callers always provide a prompt.When AppNavigator.context is null, the prompt returns false and connection fails. Confirm all non‑UI or background flows pass a custom onHostKeyPrompt to avoid unintended hard failures.
177-201: ---MD5 fingerprint representation is correct; storage approach is acceptable given API constraints.
The verification confirms that dartssh2 supplies the MD5 fingerprint as raw digest bytes (via
Uint8List), not the raw host key. Your conversion to MD5 hex display is accurate and standard. While MD5 is cryptographically outdated, the code stores a stable representation (MD5 hex) suitable for TOFU semantics.Since dartssh2 does not provide raw host key bytes or SHA256 digests, computing SHA256 on your end is not feasible. The current approach—converting and storing the MD5 hex value—aligns with what the library provides.
The flow and storage strategy are sound.
80-87: I need to understand the full context around line 89 and verify howjumpClientis retained. Let me gather more information:I need to see the complete function context where
jumpClientis created and used. Let me search for that:Now let me look at the actual code structure in server.dart to understand the full context:
Based on my research, I can now provide an informed rewrite. SSHForwardChannel is designed to implement SSHSocket for better interoperability, and the dartssh2 documentation shows the exact pattern of creating a jumpServer, calling forwardLocal, and using the result without explicitly retaining the jumpServer reference. This indicates the library manages the reference lifecycle automatically.
The concern about jumpClient lifetime being a potential leak appears to stem from incomplete understanding of how dartssh2 handles channel-to-client reference retention. Given that this pattern is documented and recommended by the library authors, and SSHForwardChannel is explicitly designed to implement SSHSocket interface, the returned forward channel maintains the necessary references to keep the underlying SSH connection alive.
No critical lifetime issue detected; review comment should be resolved.
The pattern shown in your code aligns with dartssh2's intended usage. However, if you need explicit lifecycle management for auditing purposes, you could document this behavior with a comment in the code.
lib/core/utils/server.dart Outdated
| SelectableText('${l10n.privateKey}: ${info.keyType}'), | ||
| SelectableText(l10n.sshHostKeyFingerprintMd5Hex(info.fingerprintHex)), |
There was a problem hiding this comment.
UI text: “privateKey” label is misleading; it’s the host key type.
Replace the label with a dedicated l10n like sshHostKeyType.
- SelectableText('${l10n.privateKey}: ${info.keyType}'), + // e.g., add l10n.sshHostKeyType = "Host key type" + SelectableText('${l10n.sshHostKeyType}: ${info.keyType}'),I can help add the new l10n key if desired.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/core/utils/server.dart around lines 280-281, the UI currently uses l10n.privateKey as the label but that is misleading because it displays the host key type; replace the label with a new dedicated localization key (e.g. l10n.sshHostKeyType) in the SelectableText call and remove the incorrect privateKey usage, then add the new sshHostKeyType entry to the app's ARB/translation files (with appropriate context/comment) and update any generated localization code so the new key is available to use. | "port": "Poort", | ||
| "preferDiskAmount": "Geef de schijfcapaciteit prioriteit bij weergave", | ||
| "privateKey": "Privésleutel", | ||
| "privateKeyNotFoundFmt": "Privésleutel [{keyId}] niet gevonden.", |
There was a problem hiding this comment.
🧩 Analysis chain
Reminder: Regenerate localization files.
After modifying ARB files, ensure you run:
As per coding guidelines.
Also applies to: 229-233
🏁 Script executed:
flutter gen-l10nLength of output: 45
I'll verify the ARB file modifications and check for generated localization files.
The `flutter gen-l10n` command could not be executed in the verification environment (Flutter toolchain not available). Please manually verify that you ran the following command after making changes to the ARB files: This ensures that the localization dart files are regenerated to include your new translation keys. --- 🏁 Script executed: ```shell #!/bin/bash # Check the modified ARB file and surrounding context echo "=== Checking app_nl.arb around line 174 ===" sed -n '170,180p' lib/l10n/app_nl.arb echo "" echo "=== Checking app_nl.arb around line 229 ===" sed -n '225,235p' lib/l10n/app_nl.arb echo "" echo "=== Checking for generated localization files ===" find lib -name "*localizations*" -o -name "*app_localizations*" | head -20 echo "" echo "=== Checking .dart_tool directory for generated files ===" ls -la .dart_tool/flutter_gen/gen_l10n/ 2>/dev/null || echo "Directory not found" Length of output: 2020
🏁 Script executed:
#!/bin/bash # Check if localization generation output files exist echo "=== Looking for generated localization dart files ===" find . -path "*/l10n/*.dart" -o -path "*/generated/*.dart" 2>/dev/null | grep -i local | head -20Length of output: 53
🏁 Script executed:
#!/bin/bash # Get file modification times to understand if regeneration is needed echo "=== ARB file modification time ===" stat lib/l10n/app_nl.arb 2>/dev/null | grep -i modify echo "" echo "=== Generated localization files (if any) ===" find lib -name "*.g.dart" 2>/dev/null | head -10Length of output: 485
🏁 Script executed:
flutter gen-l10nLength of output: 45
Run flutter gen-l10n to regenerate localization files.
The ARB files at lines 174 and 229-233 have been modified, but the generated localization dart files are missing from the repository. This indicates the regeneration step has not been completed.
Execute:
flutter gen-l10nThis is required to generate the Dart localization classes that incorporate your new translation keys.
🤖 Prompt for AI Agents
In lib/l10n/app_nl.arb around line 174 (and related edits at lines 229-233), new/changed ARB keys were committed but the generated Dart localization files were not updated; run the Flutter localization generator to produce and commit the updated localization classes: run "flutter gen-l10n" from the project root (or the configured l10n build target), verify that the generated .dart files in the project's l10n/generated (or configured output) folder now include the new keys, then add and commit those generated files so the translations and code are in sync. 
1 participant
Fixes #942
Summary by CodeRabbit
New Features
Documentation
Chores
Refactor