Skip to content

fix(client,core,server): stop admin channel actions from desyncing th… - #29

Merged
klobucar merged 1 commit into
mainfrom
jk.admin-channel-fixes
Aug 16, 2026
Merged

klobucar merged 1 commit into
mainfrom
jk.admin-channel-fixes

Conversation

@klobucar

Copy link
Copy Markdown
Owner

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), or
AdminResponse (0x43 DeleteChannel) — they all fell into the unhandled
default case. That left the frame unread on the stream, so the next read
misinterpreted 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_persistent always assigned position: 0 to new
channels (colliding with each other) and get_server_snapshot iterated a
DashMap with no sort, so the position field documented for "custom
ordering" in the protocol never actually ordered anything client-side.

How tested

  • cargo test --workspace, cargo fmt --all -- --check, and
    cargo clippy --workspace --all-targets -- -D warnings all clean.
  • macOS client builds (xcodebuild ... build succeeds) and
    AuraTests passes, scoped to that target (not the full scheme, per
    the UI-test Gatekeeper issue).
  • Confirmed three AuraTests failures
    (MlsProtocolTests/testThreePartyMlsGroup,
    FuzzTests/testServerProfileWithRandomData,
    ConnectionRetryTests/testSavedConnectionParameters) are pre-exis
    reproducing them against unmodified main before this branch's ch
    not regressions from this PR.

Notes for the reviewer

  • Added three new UniFFI records/decoders
    (CreateChannelResponseRecord, MetadataUpdateResponseRecord,
    AdminResponseRecord) in uniffi_bindings.rs and regenerated the
    bindings (gitignored, regenerate locally with `make generate-swift
    before building).
  • A rejected admin action whose error message is exactly "Admin required"
    now clears the client's local isAdmin flag immediately, instead of
    leaving stale admin UI visible until reconnect — a small mitigatio
    the flag only being set once at login and never otherwise refreshed.
  • Deliberately left out of scope, to avoid scope creep beyond "fix w
    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

  • Commit messages follow Conventional Commits
  • Commits are signed off (git commit -s) per the DCO
  • cargo fmt --all clean
  • cargo clippy --workspace --all-targets -- -D warnings clean
  • cargo test --workspace passes
  • Wire-format / protocol changes have a corresponding update in no wire format changes (only decoding responses the protocol alreadydefines)
  • Crypto changes have a corresponding update in docs/MLS_SECURIotocol_deviations.md — n/a

…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>
@klobucar
klobucar merged commit bf889f6 into main Aug 16, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant