fix(desktop): pin relay and signer for create_channel - #7104
Chessing234 wants to merge 1 commit into
Conversation
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested. Reviewed head 3d45198030d65de4fec82754914af05b2e56934b against base/merge-base c3132c3ee982d194cd0198ad07b57ec8bd726e4e. The explicit submit helper correctly preserves the captured values, but this does not yet close #6363: the initial snapshot can mix communities, the reread still changes signer, and a newly successful stale result can trigger follow-up writes against the new community.
Validation: source/call-path review, independent helper/auth review, clean git diff --check, and focused execution of the source-extracted stream/forum callbacks and create mutation hook with real React Query provider unmount and delayed create completion. Both callbacks continued into canvas application and navigation after the simulated community switch. This was not a live Tauri/two-relay test; no broad suites were rerun. Please add deterministic capture/in-flight/result-delivery switch regressions, including same-signer communities and both channel types.
| let events = query_relay_at( | ||
| &state, | ||
| &relay_base, |
There was a problem hiding this comment.
[P1] Discard stale create completion before follow-up writes
With the same signer on A and B, start a templated channel from the channel browser, delay A's /events response, close the browser and switch to B, then release the response. The browser permits closing while creation is pending (ChannelBrowserDialog.tsx:415). This new pinned reread can successfully return A's metadata, whereas the previous reread targeted B and failed for the absent UUID. The still-running callers in AppShell.tsx:554-565 and 583-593 then execute applyCanvas, goChannel, and template-agent setup without a community/unmount guard. useApplyTemplate.ts:37-54 sends the captured template text via setCanvas, and commands/canvas.rs:46-54 publishes using the current backend relay. Thus A's template content can be sent to B; B rejecting the unknown channel does not undo that disclosure. Even without a template, the stale callback navigates B to A's channel ID.
The keyed QueryClient prevents cache inheritance, but it does not cancel the awaited mutation continuation. Focused probes executed the source-extracted stream and forum callbacks plus the actual create-mutation hook with a real React Query provider unmount; both continued into those sinks after a simulated A-to-B switch, including when the canvas sink rejected. Native I/O was stubbed, so this is continuation evidence plus the traced native submission path, not a live HTTP reproduction. Fence/discard stale results at the initiating caller before any post-create side effect (with expected-context checks for subsequent writes), and add a same-signer switch test for both stream and forum. This successful-stale-result path is exposed by pinning the reread.
There was a problem hiding this comment.
appshell bails before applyCanvas/goChannel if the active community changed mid-create.
| let creator_keys = state.signing_keys()?; | ||
| let creator_pubkey = creator_keys.public_key().to_hex(); | ||
| submit_event_with_keys(builder, &state, &creator_keys, None).await?; | ||
| let relay_base = relay_api_base_url_with_override(&state); | ||
| submit_event_at_with_keys(builder, &state, &relay_base, &creator_keys).await?; |
There was a problem hiding this comment.
[P1] Capture or validate one coherent relay/signer context
“Before any await” does not make these reads atomic: apply_workspace runs its mutation in spawn_blocking (commands/workspace.rs:176-224) and updates relay_url_override and keys under separate mutexes. An allowed ordering is: this command reads A's keys; the workspace worker installs B's relay/keys; this command reads B's relay. submit_event_at_with_keys then sends A's signed channel name/description to B, exactly the cross-community write this fix is meant to prevent. There is no shared apply lock or caller-expected scope/signer validation here. Capture a coherent context or validate both values against the initiating caller's context, and use only that validated snapshot. Add a deterministic test that interleaves workspace application between the two reads. This is an unclosed part of #6363, not an unrelated pre-existing issue.
There was a problem hiding this comment.
took a single signing_and_relay_scope snapshot under the same lock order as apply_workspace.
| let events = query_relay_at( | ||
| &state, | ||
| &relay_base, |
There was a problem hiding this comment.
[P2] Pin the metadata query's authentication to creator_keys
query_relay_at pins only the destination. It awaits admission and calls build_nip98_auth_header (relay.rs:375-379), which locks the current state.keys (relay.rs:126-127). If A's create succeeds and the identity switches to B before that call, the reread queries relay A as B. On a membership-gated A where B is not a member, /query returns 403 (api/bridge.rs:1053-1059, api/mod.rs:135-140), so this command reports failure after successfully creating the channel. Use query_relay_at_with_keys(..., &creator_keys, None) as the existing open_dm path does, and test a key replacement while the create request is suspended. The live-key reread predates this patch but leaves its promised signer-preserving round trip incomplete.
There was a problem hiding this comment.
metadata reread now goes through query_relay_at_with_keys with the pinned creator_keys.
Capture keys and relay under one lock order, reread metadata with the pinned signer, and drop AppShell post-create side effects after a mid-flight community switch so a stale completion cannot write into the new tenant. Signed-off-by: Taksh <takshkothari09@gmail.com>
f405b9a to
4e41fc4
Compare
|
addressed the three notes + added signed-off-by for dco. |
|
pinned a single signing_and_relay_scope before any await, metadata reread uses the pinned creator keys, and appshell bails if the active community changed mid-create. |
|
awaiting-review — tip pins signing_and_relay_scope + creator_keys metadata reread and bails appshell on mid-create community switch. carl's changes_requested was on an older sha. |
Summary
create_channelalready capturedcreator_keys, but submit and the metadata reread still resolved the live workspace relay after awaits*_athelpers for the whole round-tripFixes #6363.
Test plan
submit_event_at_with_keys+query_relay_atwith a pinnedrelay_base