From 6191763cfc83533ae47646c162cd5921b46c86e1 Mon Sep 17 00:00:00 2001 From: Wintermute <165f0c871dd2586bb18b6aa109eeaf57bb2132ff4d27b10120f4368a0f627022@buzz.block.builderlab.xyz> Date: Mon, 17 Aug 2026 15:07:41 -0400 Subject: [PATCH] fix(desktop): bind the checked workspace relay to the agent spawn; scope sidebar view prefs; email-only viewer commit match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-seven review found a residual check/use gap in the round-six startup fix, plus review nits: 1. start_local_agent_with_preflight asserted the relay scope after the mesh-preflight await, but the check was not bound to the spawn: start_managed_agent_process independently re-read the workspace override at spawn time, so an A->B community switch landing after the check but before the spawn still activated the (agent, relay) pair in tenant B. The scope check now BINDS its validated read: bind_expected_relay_scope returns a ScopedWorkspaceRelay newtype whose only constructor is the check itself, and start_managed_agent_process takes that type instead of re-reading mutable state — a spawn consuming an unchecked relay no longer typechecks. Regressions cover the switch-after-check-before-spawn interleaving at the scope layer and the pair-key derivation. 2. commitMatchesViewerGitIdentity matched on name OR email, so any commit authored under the viewer's display name borrowed their avatar. Now email-only, with a shared-display-name regression. 3. Sidebar projects filter/sort preferences were stored globally while expansion and membership are relay+pubkey scoped; a community or identity switch leaked view preferences across tenants. They now share the same scoped key derivation and re-read on scope change. Co-authored-by: Thomas Petersen Signed-off-by: Thomas Petersen --- desktop/src-tauri/src/commands/agents.rs | 32 +++--- .../src-tauri/src/managed_agents/runtime.rs | 16 +-- desktop/src-tauri/src/relay.rs | 5 +- desktop/src-tauri/src/relay/scope.rs | 101 +++++++++++++++++- .../lib/projectContributorMatching.test.mjs | 16 +++ .../lib/projectContributorMatching.ts | 11 +- .../sidebar/ui/SidebarProjectsSection.tsx | 16 +-- .../sidebar/ui/listSidebarProjects.ts | 73 +++++++++++-- 8 files changed, 227 insertions(+), 43 deletions(-) diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index d83e45328..e8f5b1a1c 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -232,14 +232,15 @@ pub(super) async fn start_local_agent_with_preflight( // The mesh preflight above is the suspension window Projects callbacks // capture their scope against: a community switch during that await - // would otherwise spawn this pair keyed to the *new* workspace relay - // (`start_managed_agent_process` re-reads the active override at spawn - // time). Re-assert the caller's captured scope after the await, before - // the spawn side effect — fail closed instead of activating the agent - // in the wrong tenant. - crate::relay::assert_expected_relay_scope( + // would otherwise spawn this pair keyed to the *new* workspace relay. + // Read the workspace relay ONCE, assert the caller's captured scope + // against that exact read, and hand the same bound value to the spawn + // below — the check is tied to its use, so a switch landing after this + // point can no longer retarget the spawn (it only changes state this + // call no longer consults). + let workspace_relay_url = crate::relay::bind_expected_relay_scope( expected_relay_url, - &crate::relay::relay_api_base_url_with_override(state), + crate::relay::relay_ws_url_with_override(state), )?; let _store_guard = state @@ -276,7 +277,13 @@ pub(super) async fn start_local_agent_with_preflight( } } } - start_managed_agent_process(app, record, &mut runtimes, Some(owner_hex))?; + start_managed_agent_process( + app, + record, + &mut runtimes, + Some(owner_hex), + &workspace_relay_url, + )?; save_managed_agents(app, &records)?; if let Some(saved_record) = records.iter().find(|r| r.pubkey == pubkey) { retain_managed_agent_pending(app, state, saved_record); @@ -924,10 +931,11 @@ pub async fn start_managed_agent( // activates the (agent, relay) pair — a channel/tool-capable side effect // — so a stale callback must fail closed here before any spawn or deploy // when the active community or identity changed while it was suspended. - // The local path re-asserts the relay scope again at spawn time - // (`start_managed_agent_process`), after the mesh preflight awaits; the - // provider path re-asserts against the relay embedded in the deploy - // payload before deploying. + // After the mesh-preflight awaits, the local path re-checks and BINDS + // the workspace relay (`bind_expected_relay_scope`) so the spawn consumes + // the checked value rather than re-reading mutable state; the provider + // path asserts against the relay embedded in the deploy payload before + // deploying. crate::relay::assert_expected_relay_scope( expected_relay_url.as_deref(), &crate::relay::relay_api_base_url_with_override(&state), diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index b1c342e99..a5102adc1 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -932,20 +932,20 @@ fn child_rust_log_filter() -> String { } } +/// Spawn (or adopt) the runtime pair for `record` on the caller's bound +/// workspace relay. `workspace_relay` can only be produced by +/// `bind_expected_relay_scope`, so this spawn consumes — by construction — +/// the exact workspace-relay read the caller's scope assertion passed on; it +/// never re-reads the mutable override (see `relay::scope`). pub fn start_managed_agent_process( app: &AppHandle, record: &mut ManagedAgentRecord, runtimes: &mut HashMap, owner_hex: Option<&str>, + workspace_relay: &crate::relay::ScopedWorkspaceRelay, ) -> Result<(), String> { - let relay_url = { - use tauri::Manager; - let state = app.state::(); - crate::relay::effective_agent_relay_url( - &record.relay_url, - &crate::relay::relay_ws_url_with_override(&state), - ) - }; + let relay_url = + crate::relay::effective_agent_relay_url(&record.relay_url, workspace_relay.as_str()); let key = ManagedAgentRuntimeKey::new(record.pubkey.clone(), &relay_url)?; if let Some(runtime) = runtimes.get_mut(&key) { if runtime diff --git a/desktop/src-tauri/src/relay.rs b/desktop/src-tauri/src/relay.rs index 576f873b6..c35bc808b 100644 --- a/desktop/src-tauri/src/relay.rs +++ b/desktop/src-tauri/src/relay.rs @@ -85,7 +85,10 @@ pub fn relay_http_base_url(relay_url: &str) -> String { } mod scope; -pub use scope::{assert_expected_relay_scope, assert_expected_signer}; +pub use scope::{ + assert_expected_relay_scope, assert_expected_signer, bind_expected_relay_scope, + ScopedWorkspaceRelay, +}; pub fn relay_api_base_url() -> String { if let Some(base) = configured_env_var("BUZZ_RELAY_HTTP") { diff --git a/desktop/src-tauri/src/relay/scope.rs b/desktop/src-tauri/src/relay/scope.rs index f58362088..fbd52e807 100644 --- a/desktop/src-tauri/src/relay/scope.rs +++ b/desktop/src-tauri/src/relay/scope.rs @@ -26,6 +26,39 @@ pub fn assert_expected_relay_scope( Ok(()) } +/// A workspace-relay read that has passed the caller-captured scope check. +/// +/// The only constructor is [`bind_expected_relay_scope`], so any side effect +/// that takes this type is proven — by construction — to consume the exact +/// value the check passed on, never a re-read of the mutable override. This +/// closes the check/use gap where a workspace switch landing between a scope +/// assertion and the side effect retargets it to a tenant the caller never +/// validated. +#[derive(Debug)] +pub struct ScopedWorkspaceRelay(String); + +impl ScopedWorkspaceRelay { + pub fn as_str(&self) -> &str { + &self.0 + } +} + +/// Validate a caller-captured relay scope against one workspace-relay read +/// and bind that exact read for the side effect to consume. +/// +/// `None` preserves the unscoped behavior for callers without a tenant +/// boundary — the read is still bound so the side effect stays single-read. +pub fn bind_expected_relay_scope( + expected_relay_url: Option<&str>, + workspace_relay_url: String, +) -> Result { + assert_expected_relay_scope( + expected_relay_url, + &relay_http_base_url(&workspace_relay_url), + )?; + Ok(ScopedWorkspaceRelay(workspace_relay_url)) +} + /// Fail closed when a caller-captured signer identity no longer matches the /// identity a command actually read. /// @@ -57,7 +90,7 @@ pub fn assert_expected_signer( #[cfg(test)] mod tests { - use super::{assert_expected_relay_scope, assert_expected_signer}; + use super::{assert_expected_relay_scope, assert_expected_signer, bind_expected_relay_scope}; #[test] fn matching_scope_passes_across_ws_http_normalization() { @@ -87,6 +120,72 @@ mod tests { assert_expected_relay_scope(Some(" "), "https://anything.example").unwrap(); } + #[test] + fn bound_scope_is_immune_to_a_switch_landing_after_the_bind() { + // Models the round-7 startup race: the caller captured tenant A, the + // post-preflight bind reads the workspace relay while it is still A, + // and THEN the switch to B lands — after the check, before the spawn. + // The spawn consumes the BOUND value, not a re-read, so the pair can + // only ever be keyed to the tenant the caller validated; the switch + // mutates state the spawn no longer consults. + let mut workspace = "wss://tenant-a.example".to_string(); + let bound = + bind_expected_relay_scope(Some("wss://tenant-a.example"), workspace.clone()).unwrap(); + workspace = "wss://tenant-b.example".to_string(); // the switch lands post-check + assert_eq!(bound.as_str(), "wss://tenant-a.example"); + assert_ne!( + bound.as_str(), + workspace, + "spawn input must be the checked value" + ); + } + + #[test] + fn bind_fails_closed_when_the_switch_lands_before_the_read() { + // The switch landed during the preflight await, so the one workspace + // read already sees tenant B: no relay may be released to the spawn. + let error = bind_expected_relay_scope( + Some("wss://tenant-a.example"), + "wss://tenant-b.example".to_string(), + ) + .unwrap_err(); + assert!(error.contains("active community changed"), "{error}"); + } + + #[test] + fn bind_returns_the_exact_read_for_unscoped_callers() { + let bound = bind_expected_relay_scope(None, "wss://anything.example".to_string()).unwrap(); + assert_eq!(bound.as_str(), "wss://anything.example"); + } + + #[test] + fn pair_key_derives_from_the_bound_relay_not_the_post_switch_workspace() { + // Round-7 regression (check/use gap): the caller captured tenant A, + // the post-preflight bind passed while the workspace still read A, + // and the A→B switch lands AFTER the check but BEFORE the spawn. + // This mirrors the exact key derivation `start_managed_agent_process` + // performs — `effective_agent_relay_url(record, bound.as_str())` into + // `ManagedAgentRuntimeKey::new` — and proves the pair (and the + // receipt, keyed by the same value) can only ever be keyed to the + // tenant the caller validated: the switch mutates state the spawn no + // longer consults, so no runtime pair can exist in B. + let mut workspace = "wss://tenant-a.example".to_string(); + let bound = bind_expected_relay_scope(Some("wss://tenant-a.example"), workspace.clone()) + .expect("scope matches at bind time"); + workspace = "wss://tenant-b.example".to_string(); // the switch lands post-check + + let record_relay = "ws://localhost:3000"; // ignored by design (agents-everywhere) + let relay_url = crate::relay::effective_agent_relay_url(record_relay, bound.as_str()); + let key = crate::managed_agents::ManagedAgentRuntimeKey::new("a".repeat(64), &relay_url) + .expect("keyable relay"); + assert_eq!(key.relay_url, "wss://tenant-a.example"); + assert_ne!( + key.relay_url, + crate::relay::effective_agent_relay_url(record_relay, &workspace), + "a pair keyed to the post-switch tenant must be unrepresentable" + ); + } + #[test] fn matching_signer_passes_case_insensitively() { let keys = nostr::Keys::generate(); diff --git a/desktop/src/features/projects/lib/projectContributorMatching.test.mjs b/desktop/src/features/projects/lib/projectContributorMatching.test.mjs index 29aa1faf8..e652f7205 100644 --- a/desktop/src/features/projects/lib/projectContributorMatching.test.mjs +++ b/desktop/src/features/projects/lib/projectContributorMatching.test.mjs @@ -249,6 +249,22 @@ test("viewer git identity does not claim other authors' commits", () => { assert.equal(matched, null); }); +test("a shared display name alone never borrows the viewer's identity", () => { + // Two contributors can share a display name; only the git email — which + // the viewer's own commits actually carry — may attribute a commit to the + // viewer's pubkey. + const commit = makeCommit({ + authorName: "Thomas Petersen", + authorEmail: "impostor@example.org", + }); + const matched = profileForCommit(commit, PROFILES, new Map(), { + pubkey: USER_PUBKEY, + name: "Thomas Petersen", + email: "thomasp@squareup.com", + }); + assert.equal(matched, null); +}); + test("signed PR mapping wins over the viewer git identity", () => { const commit = makeCommit(); const map = new Map([[commit.hash, AGENT_PUBKEY]]); diff --git a/desktop/src/features/projects/lib/projectContributorMatching.ts b/desktop/src/features/projects/lib/projectContributorMatching.ts index c1abfe483..ff7abc63a 100644 --- a/desktop/src/features/projects/lib/projectContributorMatching.ts +++ b/desktop/src/features/projects/lib/projectContributorMatching.ts @@ -257,14 +257,13 @@ function commitMatchesViewerGitIdentity( commit: ProjectRepoCommit, viewer: ViewerGitIdentity, ) { - const name = commit.authorName.trim().toLowerCase(); + // Email-only equality. A display name is not an identity: two contributors + // can share "Alex Chen", and a name-based match would borrow the viewer's + // avatar for a stranger's commit. The git email is what the viewer's own + // commits actually carry, so requiring it loses nothing legitimate. const email = commit.authorEmail.trim().toLowerCase(); - const viewerName = viewer.name?.trim().toLowerCase() ?? ""; const viewerEmail = viewer.email?.trim().toLowerCase() ?? ""; - return ( - (email.length > 0 && email === viewerEmail) || - (name.length > 0 && name === viewerName) - ); + return email.length > 0 && email === viewerEmail; } /** diff --git a/desktop/src/features/sidebar/ui/SidebarProjectsSection.tsx b/desktop/src/features/sidebar/ui/SidebarProjectsSection.tsx index 7b9754e70..994afeb41 100644 --- a/desktop/src/features/sidebar/ui/SidebarProjectsSection.tsx +++ b/desktop/src/features/sidebar/ui/SidebarProjectsSection.tsx @@ -142,11 +142,11 @@ function SidebarProjectsSectionContent() { const [projectToDelete, setProjectToDelete] = React.useState( null, ); - const [filter, setFilter] = React.useState( - readSidebarProjectsFilter, + const [filter, setFilter] = React.useState(() => + readSidebarProjectsFilter(relayOrigin, currentPubkey), ); - const [sort, setSort] = React.useState( - readSidebarProjectsSort, + const [sort, setSort] = React.useState(() => + readSidebarProjectsSort(relayOrigin, currentPubkey), ); const [projectExpansion, setProjectExpansion] = React.useState(() => @@ -192,6 +192,10 @@ function SidebarProjectsSectionContent() { setProjectExpansion( readSidebarProjectExpansion(relayOrigin, currentPubkey), ); + // Filter/sort are scoped like expansion: re-read on identity/community + // change (currentPubkey is undefined until the identity query resolves). + setFilter(readSidebarProjectsFilter(relayOrigin, currentPubkey)); + setSort(readSidebarProjectsSort(relayOrigin, currentPubkey)); }, [currentPubkey, relayOrigin]); const addedProjectAddressSet = React.useMemo( () => new Set(addedProjectAddresses), @@ -228,11 +232,11 @@ function SidebarProjectsSectionContent() { const handleFilterChange = (next: SidebarProjectsFilter) => { setFilter(next); - writeSidebarProjectsFilter(next); + writeSidebarProjectsFilter(next, relayOrigin, currentPubkey); }; const handleSortChange = (next: SidebarProjectsSort) => { setSort(next); - writeSidebarProjectsSort(next); + writeSidebarProjectsSort(next, relayOrigin, currentPubkey); }; const setProjectExpanded = (project: Project, expanded: boolean) => { setProjectExpansion((current) => { diff --git a/desktop/src/features/sidebar/ui/listSidebarProjects.ts b/desktop/src/features/sidebar/ui/listSidebarProjects.ts index 1b2171ffe..752e2dc8e 100644 --- a/desktop/src/features/sidebar/ui/listSidebarProjects.ts +++ b/desktop/src/features/sidebar/ui/listSidebarProjects.ts @@ -9,11 +9,26 @@ export type SidebarProjectsFilter = "added" | "owned"; export type SidebarProjectsSort = "name" | "created"; export type SidebarProjectExpansionState = Record; +// Every persisted sidebar preference — filter, sort, expansion, membership — +// is scoped to the relay+pubkey identity: a community or identity switch must +// not leak one tenant's view of the sidebar into another. +function scopedPreferenceKey( + baseKey: string, + relayOrigin: string | null, + currentPubkey?: string, +) { + return `${baseKey}:${encodeURIComponent(relayOrigin ?? "unknown")}:${currentPubkey ?? "anonymous"}`; +} + function expandedProjectsStorageKey( relayOrigin: string | null, currentPubkey?: string, ) { - return `${SIDEBAR_PROJECTS_EXPANDED_KEY}:${encodeURIComponent(relayOrigin ?? "unknown")}:${currentPubkey ?? "anonymous"}`; + return scopedPreferenceKey( + SIDEBAR_PROJECTS_EXPANDED_KEY, + relayOrigin, + currentPubkey, + ); } export function readSidebarProjectExpansion( @@ -64,35 +79,75 @@ export function selectedProjectRouteId(pathname: string): string | undefined { } } -export function readSidebarProjectsFilter(): SidebarProjectsFilter { +export function readSidebarProjectsFilter( + relayOrigin: string | null, + currentPubkey?: string, +): SidebarProjectsFilter { try { - const value = globalThis.localStorage?.getItem(SIDEBAR_PROJECTS_FILTER_KEY); + const value = globalThis.localStorage?.getItem( + scopedPreferenceKey( + SIDEBAR_PROJECTS_FILTER_KEY, + relayOrigin, + currentPubkey, + ), + ); return value === "owned" ? "owned" : "added"; } catch { return "added"; } } -export function writeSidebarProjectsFilter(filter: SidebarProjectsFilter) { +export function writeSidebarProjectsFilter( + filter: SidebarProjectsFilter, + relayOrigin: string | null, + currentPubkey?: string, +) { try { - globalThis.localStorage?.setItem(SIDEBAR_PROJECTS_FILTER_KEY, filter); + globalThis.localStorage?.setItem( + scopedPreferenceKey( + SIDEBAR_PROJECTS_FILTER_KEY, + relayOrigin, + currentPubkey, + ), + filter, + ); } catch { // Persistence is best-effort; the in-memory toggle still works. } } -export function readSidebarProjectsSort(): SidebarProjectsSort { +export function readSidebarProjectsSort( + relayOrigin: string | null, + currentPubkey?: string, +): SidebarProjectsSort { try { - const value = globalThis.localStorage?.getItem(SIDEBAR_PROJECTS_SORT_KEY); + const value = globalThis.localStorage?.getItem( + scopedPreferenceKey( + SIDEBAR_PROJECTS_SORT_KEY, + relayOrigin, + currentPubkey, + ), + ); return value === "created" ? "created" : "name"; } catch { return "name"; } } -export function writeSidebarProjectsSort(sort: SidebarProjectsSort) { +export function writeSidebarProjectsSort( + sort: SidebarProjectsSort, + relayOrigin: string | null, + currentPubkey?: string, +) { try { - globalThis.localStorage?.setItem(SIDEBAR_PROJECTS_SORT_KEY, sort); + globalThis.localStorage?.setItem( + scopedPreferenceKey( + SIDEBAR_PROJECTS_SORT_KEY, + relayOrigin, + currentPubkey, + ), + sort, + ); } catch { // Persistence is best-effort; the in-memory toggle still works. }