fix(client,core,server): stop admin channel actions from desyncing th… - #29
Merged
Merged
Conversation
…e control stream The macOS client's control-stream reader dispatched on the message-type byte but never drained the length-prefixed payload for CreateChannelResponse (0x40), the UpdateChannel/UpdateProfile MetadataUpdateResponse (0x41/0x42), or the DeleteChannel AdminResponse (0x43) — they all fell into the unhandled default case. Every admin channel action (and every profile update, by any user) left that frame unread on the stream, so the next loop iteration misread the previous frame's length prefix as a new message type and corrupted all control- stream parsing for the rest of the session. Add UniFFI records/decoders for the three response shapes (uniffi_bindings.rs) and wire them into the client's message switch, draining and decoding each payload via the existing receiveHardenedPayload path. Failures now surface as a system event instead of being silently dropped, and a rejection with the server's exact "Admin required" message clears the client's isAdmin flag immediately rather than leaving stale admin UI visible until reconnect. Also fix channel ordering: new channels always got position 0 (colliding with each other), and get_server_snapshot iterated a DashMap with no sort, so the position field documented for "custom ordering" never actually ordered anything. New channels now get the next position, and the snapshot sorts by (position, channel_id) to match the DB's own ORDER BY. Verified: full cargo test --workspace, fmt, and clippy -D warnings all clean; macOS client builds and AuraTests passes (three unrelated pre-existing failures confirmed present on unmodified main: MlsProtocolTests/testThreePartyMlsGroup, FuzzTests/ testServerProfileWithRandomData, ConnectionRetryTests/ testSavedConnectionParameters). Signed-off-by: Jonathon Klobucar <jonathon.klobucar@gmail.com>
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
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.
What this changes
Fixes a control-stream desync bug in the macOS client where admin channel
actions (and any user's profile update) corrupted the connection, plus a
server-side bug where channel ordering never worked despite the protocol
supporting it.
Why
The client's control-stream reader dispatched on the message-type byte but
never read the length-prefixed payload for
CreateChannelResponse(0x40),MetadataUpdateResponse(0x41 UpdateChannel / 0x42 UpdateProfile), orAdminResponse(0x43 DeleteChannel) — they all fell into the unhandleddefaultcase. That left the frame unread on the stream, so the next readmisinterpreted the previous frame's length prefix as a new message type and
corrupted control-stream parsing for the rest of the session. In practice:
any admin creating/updating/deleting a channel, or any user updating their
own profile, desynced their own connection, even though the action usually
succeeded server-side — the client just never found out and then misparsed
everything after.
Separately,
create_channel_persistentalways assignedposition: 0to newchannels (colliding with each other) and
get_server_snapshotiterated aDashMapwith no sort, so thepositionfield documented for "customordering" in the protocol never actually ordered anything client-side.
How tested
cargo test --workspace,cargo fmt --all -- --check, andcargo clippy --workspace --all-targets -- -D warningsall clean.xcodebuild ... buildsucceeds) andAuraTestspasses, scoped to that target (not the full scheme, perthe UI-test Gatekeeper issue).
AuraTestsfailures(
MlsProtocolTests/testThreePartyMlsGroup,FuzzTests/testServerProfileWithRandomData,ConnectionRetryTests/testSavedConnectionParameters) are pre-exisreproducing them against unmodified
mainbefore this branch's chnot regressions from this PR.
Notes for the reviewer
(
CreateChannelResponseRecord,MetadataUpdateResponseRecord,AdminResponseRecord) inuniffi_bindings.rsand regenerated thebindings (gitignored, regenerate locally with `make generate-swift
before building).
"Admin required"now clears the client's local
isAdminflag immediately, instead ofleaving stale admin UI visible until reconnect — a small mitigatio
the flag only being set once at login and never otherwise refreshed.
broken": drag-to-reorder UI (ordering now works, but there's still no way
to change order after creation), "create as Lobby" support, and
admin-channel UI for the desktop (.NET) client, which currently has none
at all.
Checklist
git commit -s) per the DCOcargo fmt --allcleancargo clippy --workspace --all-targets -- -D warningscleancargo test --workspacepassesdocs/MLS_SECURIotocol_deviations.md— n/a