mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(desktop): bind the checked workspace relay to the agent spawn; scope sidebar view prefs; email-only viewer commit match
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 <thomasp@squareup.com> Signed-off-by: Thomas Petersen <thomasp@squareup.com>
This commit is contained in:
co-authored by
Thomas Petersen
parent
a3b0d04593
commit
6191763cfc
@@ -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),
|
||||
|
||||
@@ -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<ManagedAgentRuntimeKey, ManagedAgentPairRuntime>,
|
||||
owner_hex: Option<&str>,
|
||||
workspace_relay: &crate::relay::ScopedWorkspaceRelay,
|
||||
) -> Result<(), String> {
|
||||
let relay_url = {
|
||||
use tauri::Manager;
|
||||
let state = app.state::<crate::app_state::AppState>();
|
||||
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
|
||||
|
||||
@@ -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") {
|
||||
|
||||
@@ -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<ScopedWorkspaceRelay, String> {
|
||||
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();
|
||||
|
||||
@@ -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]]);
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -142,11 +142,11 @@ function SidebarProjectsSectionContent() {
|
||||
const [projectToDelete, setProjectToDelete] = React.useState<Project | null>(
|
||||
null,
|
||||
);
|
||||
const [filter, setFilter] = React.useState<SidebarProjectsFilter>(
|
||||
readSidebarProjectsFilter,
|
||||
const [filter, setFilter] = React.useState<SidebarProjectsFilter>(() =>
|
||||
readSidebarProjectsFilter(relayOrigin, currentPubkey),
|
||||
);
|
||||
const [sort, setSort] = React.useState<SidebarProjectsSort>(
|
||||
readSidebarProjectsSort,
|
||||
const [sort, setSort] = React.useState<SidebarProjectsSort>(() =>
|
||||
readSidebarProjectsSort(relayOrigin, currentPubkey),
|
||||
);
|
||||
const [projectExpansion, setProjectExpansion] =
|
||||
React.useState<SidebarProjectExpansionState>(() =>
|
||||
@@ -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) => {
|
||||
|
||||
@@ -9,11 +9,26 @@ export type SidebarProjectsFilter = "added" | "owned";
|
||||
export type SidebarProjectsSort = "name" | "created";
|
||||
export type SidebarProjectExpansionState = Record<string, boolean>;
|
||||
|
||||
// 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.
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user