fix(conn): insecure conn, fail-close by default#64
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (59)
WalkthroughAdds a new ChangesAllow Insecure Session Fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
🤖 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 `@flutter/lib/mobile/pages/settings_page.dart`:
- Around line 773-788: The visibility condition for the "Allow insecure session
fallback" SettingsTile in settings_page.dart is too restrictive compared with
the adjacent TLS fallback toggle and the desktop/TIS variants. Update the guard
around this switch so it follows the same “not using public server” logic as the
other insecure-fallback controls, and make sure the tile only uses the shared
network/public-server visibility check rather than also depending on
disabledSettings/_hideNetwork in a way that hides just this one option.
In `@src/client.rs`:
- Around line 780-783: The fallback branch in the client handshake is returning
too early and can leave a queued server SignedId frame for the next read. Update
the logic in the `None` arm of the receive path in `src/client.rs` so it either
drains and stashes the server handshake frame before returning, or otherwise
ensures the `SignedId` sent by `src/server.rs` cannot remain pending when
`secure` is true and `signed_id_pk` is empty. Keep the behavior aligned with the
existing handshake flow around the client connection setup and the server-side
`SignedId` send path.
In `@src/server.rs`:
- Around line 275-278: The handshake parsing path still hard-fails on malformed
peer-key frames even when allow_insecure_fallback is enabled, so update the
handling around Message::parse_from_bytes to mirror the existing
empty/invalid-union fallback behavior. In the server-side handshake flow,
replace the direct bail! cases for malformed frames with the same fallback
branch used elsewhere when allow_insecure_fallback is true, and only return the
hard error when fallback is disabled. Apply this consistently in both affected
handshake checks so the new option behaves uniformly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c7e296c-60c1-4fa2-8b46-3e1054bbf568
📒 Files selected for processing (8)
flutter/lib/consts.dartflutter/lib/desktop/pages/desktop_setting_page.dartflutter/lib/mobile/pages/settings_page.dartlibs/hbb_commonsrc/client.rssrc/server.rssrc/server/connection.rssrc/ui/index.tis
fufesou
force-pushed
the
fix/insecure-conn-fail-close
branch
2 times, most recently
from
abceb44 to
60167c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/server.rs`:
- Around line 240-250: The handshake logic in the empty-peer-key branch is
clearing key confirmation unconditionally before the allow_insecure_fallback
check. Move Config::set_key_confirmed(false) into the
allow_insecure_fallback=true path in server::handshake handling so rejected
fail-close cases do not mutate state; keep the log::warn! and handshake
close/bail behavior unchanged for the insecure fallback branch.
- Around line 299-305: The close-reason send in send_handshake_close_reason is
not bounded by the handshake timeout, so a stalled peer can block the handshake
task during fail-close handling. Update this helper to perform the stream.send
call under the same CONNECT_TIMEOUT timeout used for the initial handshake path,
and handle the timeout/error consistently via allow_err! or equivalent so the
close-reason send cannot wait indefinitely.
In `@src/server/connection.rs`:
- Around line 1386-1394: The insecure-connection alarm path in `Connection` is
missing `conn_audit_ref`, so update `post_alarm_audit` to attach that field for
`AlarmAuditType::InsecureConnection` as well, not just `IpWhitelist`. Use the
existing connection audit context from the `Connection`/`post_alarm_audit` flow
so controlled-session insecure alarms remain correlatable with the audit record.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c0373d71-bfc4-4a10-a701-7a836a414f19
📒 Files selected for processing (8)
flutter/lib/consts.dartflutter/lib/desktop/pages/desktop_setting_page.dartflutter/lib/mobile/pages/settings_page.dartlibs/hbb_commonsrc/client.rssrc/server.rssrc/server/connection.rssrc/ui/index.tis
fufesou
force-pushed
the
fix/insecure-conn-fail-close
branch
2 times, most recently
from
9a7ab43 to
2f79b04
Compare
Signed-off-by: fufesou <linlong1266@gmail.com>
fufesou
force-pushed
the
fix/insecure-conn-fail-close
branch
from
2f79b04 to
5c10144
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes