fix: harden ProxyCommand SSH config parsing and validation#1118
Conversation
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR trims Possibly related PRs
Suggested reviewers
🚥 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)
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.
🧹 Nitpick comments (2)
lib/core/utils/ssh_config.dart (1)
185-217: Add focused tests for_stripInlineCommentedge cases.Given Line 185–217 now handles escaping and quote state, please add parser tests for: quoted
#, escaped\#, and whitespace-prefixed inline comments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/core/utils/ssh_config.dart` around lines 185 - 217, Add focused unit tests for the _stripInlineComment function to cover the newly-handled edge cases: (1) a '#' character inside single and double quotes which should be preserved (e.g., "'#'" and '"#"' return the full string), (2) an escaped hash like '\#' which should not start a comment and should be preserved, and (3) a true inline comment that is whitespace-prefixed (e.g., "value # comment") which should return only the part before the comment. Create tests that assert exact expected outputs for each case and include examples combining escapes and quotes to ensure the parser's escaped/quote state handling in _stripInlineComment is validated.lib/view/page/server/edit/actions.dart (1)
7-12: Reuse_validationErrorMessagein_onSaveto keep one source of truth.Line 7–12 introduces a good mapper, but
_onSavestill hardcodes the same message path. Centralizing avoids drift when newSpiValidationErrorvalues are added.Refactor proposal
- final validationError = spi.validate(); - if (validationError == - SpiValidationError.jumpServerAndProxyCommandConflict) { - context.showSnackBar( - l10n.jumpServerAndProxyCommandCannotBeUsedTogether, - ); - return; - } + final validationError = spi.validate(); + if (validationError != null) { + context.showSnackBar(_validationErrorMessage(validationError)); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/view/page/server/edit/actions.dart` around lines 7 - 12, The _onSave handler duplicates a validation message instead of using the central mapper; update _onSave to call _validationErrorMessage(error) (where error is the SpiValidationError returned during save) and use its returned string for the UI/logging; also make _validationErrorMessage(SpiValidationError) exhaustive or add a default/fallback return so it never returns null (e.g., a generic l10n.validationFailed) to avoid runtime nulls when new SpiValidationError values are added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/core/utils/ssh_config.dart`:
- Around line 185-217: Add focused unit tests for the _stripInlineComment
function to cover the newly-handled edge cases: (1) a '#' character inside
single and double quotes which should be preserved (e.g., "'#'" and '"#"' return
the full string), (2) an escaped hash like '\#' which should not start a comment
and should be preserved, and (3) a true inline comment that is
whitespace-prefixed (e.g., "value # comment") which should return only the part
before the comment. Create tests that assert exact expected outputs for each
case and include examples combining escapes and quotes to ensure the parser's
escaped/quote state handling in _stripInlineComment is validated.
In `@lib/view/page/server/edit/actions.dart`:
- Around line 7-12: The _onSave handler duplicates a validation message instead
of using the central mapper; update _onSave to call
_validationErrorMessage(error) (where error is the SpiValidationError returned
during save) and use its returned string for the UI/logging; also make
_validationErrorMessage(SpiValidationError) exhaustive or add a default/fallback
return so it never returns null (e.g., a generic l10n.validationFailed) to avoid
runtime nulls when new SpiValidationError values are added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3ea0ae4f-36c7-4d56-af1c-0d7174e6b043
⛔ Files ignored due to path filters (1)
lib/generated/l10n/l10n_zh.dartis excluded by!**/generated/**
📒 Files selected for processing (3)
lib/core/utils/ssh_config.dartlib/data/model/server/server_private_info.dartlib/view/page/server/edit/actions.dart
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/ssh_config_test.dart (1)
355-389: Good test coverage for the inline comment stripping logic.The tests cover the key scenarios well. Consider adding a test for the edge case where
#appears without preceding whitespace (e.g.,'foo#bar'should remain unchanged since#is not preceded by whitespace):💡 Optional: additional edge case test
test('preserves hash not preceded by whitespace', () { expect( SSHConfig.stripInlineCommentForTest('foo#bar'), 'foo#bar', ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ssh_config_test.dart` around lines 355 - 389, Add a unit test to cover the edge case where a hash appears without preceding whitespace so it isn't treated as an inline comment: add a new test in the _stripInlineComment group that calls SSHConfig.stripInlineCommentForTest('foo#bar') and expects 'foo#bar' to be returned (reference: SSHConfig.stripInlineCommentForTest and the _stripInlineComment test group).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/ssh_config_test.dart`:
- Around line 355-389: Add a unit test to cover the edge case where a hash
appears without preceding whitespace so it isn't treated as an inline comment:
add a new test in the _stripInlineComment group that calls
SSHConfig.stripInlineCommentForTest('foo#bar') and expects 'foo#bar' to be
returned (reference: SSHConfig.stripInlineCommentForTest and the
_stripInlineComment test group).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3cabd119-713d-48f2-a018-e294ce364df8
📒 Files selected for processing (3)
lib/core/utils/ssh_config.dartlib/view/page/server/edit/actions.darttest/ssh_config_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/view/page/server/edit/actions.dart
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Continue fix of #1117.
Summary by CodeRabbit
New Features
Bug Fixes
Tests