From dd22a7cbf66706808c6793297a600e75545d9e73 Mon Sep 17 00:00:00 2001 From: kenny lopez Date: Fri, 24 Jul 2026 10:18:26 -0700 Subject: [PATCH] fix(desktop): harden catalog publication imports --- .../src/commands/personas/snapshot/tests.rs | 8 +++ .../agents/lib/personaCatalogRelay.test.mjs | 67 ++++++++++++++++++- .../agents/lib/personaCatalogRelay.ts | 60 ++++++++++------- desktop/src/testing/e2eBridge.ts | 8 +++ desktop/tests/e2e/agents.spec.ts | 14 ++++ desktop/tests/helpers/bridge.ts | 2 + 6 files changed, 133 insertions(+), 26 deletions(-) diff --git a/desktop/src-tauri/src/commands/personas/snapshot/tests.rs b/desktop/src-tauri/src/commands/personas/snapshot/tests.rs index c7ddbc37a..333ec5aa6 100644 --- a/desktop/src-tauri/src/commands/personas/snapshot/tests.rs +++ b/desktop/src-tauri/src/commands/personas/snapshot/tests.rs @@ -632,6 +632,14 @@ fn import_non_allowlist_mode_preserved_when_keep_false() { ); } +#[test] +fn import_catalog_owner_only_without_allowlist_succeeds() { + let minted = resolve_snapshot_import_behavior(Some("owner-only"), &[], None, false).unwrap(); + + assert_eq!(minted.respond_to, RespondTo::OwnerOnly); + assert!(minted.respond_to_allowlist.is_empty()); +} + /// Non-allowlist mode with a non-empty list and keep=true: preserve mode + list. /// The toggle WAS shown (list is non-empty) so keep_allowlist applies. #[test] diff --git a/desktop/src/features/agents/lib/personaCatalogRelay.test.mjs b/desktop/src/features/agents/lib/personaCatalogRelay.test.mjs index dd04627e1..c3db37858 100644 --- a/desktop/src/features/agents/lib/personaCatalogRelay.test.mjs +++ b/desktop/src/features/agents/lib/personaCatalogRelay.test.mjs @@ -24,6 +24,7 @@ function catalogEvent({ sourcePersonaId = "reviewer", status = "published", memoryLevel = "none", + avatarUrl = null, }) { const sourceUpdatedAt = `2026-07-23T00:00:0${createdAt}.000Z`; const content = @@ -37,7 +38,7 @@ function catalogEvent({ memoryLevel, agent: { displayName: "Relay Reviewer", - avatarUrl: null, + avatarUrl, systemPrompt: "Review changes.", runtime: "goose", model: "claude", @@ -106,6 +107,68 @@ test("catalog coordinates remain independent across authors", () => { ); }); +test("equal-second catalog heads use the relay's lowest-id tie-break", () => { + const publications = catalogPublicationsFromEvents([ + catalogEvent({ + createdAt: 1, + id: "b".repeat(64), + status: "published", + }), + catalogEvent({ + createdAt: 1, + id: "a".repeat(64), + status: "unpublished", + }), + ]); + + assert.equal(publications.length, 1); + assert.equal(publications[0].status, "unpublished"); +}); + +test("an invalid canonical head does not resurrect an older publication", () => { + const invalidHead = { + ...catalogEvent({ createdAt: 2, id: "a".repeat(64) }), + content: "{}", + }; + const publications = catalogPublicationsFromEvents([ + catalogEvent({ createdAt: 1, id: "older-valid" }), + invalidHead, + ]); + + assert.deepEqual(publications, []); +}); + +test("catalog avatars accept only bounded http or https URLs", () => { + assert.equal( + catalogPublicationsFromEvents([ + catalogEvent({ + createdAt: 1, + id: "safe-avatar", + avatarUrl: "https://relay.example/avatar.png", + }), + ]).length, + 1, + ); + for (const [index, avatarUrl] of [ + "data:image/png;base64,abc", + "javascript:alert(1)", + "ftp://relay.example/avatar.png", + `https://relay.example/${"a".repeat(2_049)}`, + ].entries()) { + assert.equal( + catalogPublicationsFromEvents([ + catalogEvent({ + createdAt: index + 1, + id: `unsafe-avatar-${index}`, + sourcePersonaId: `unsafe-avatar-${index}`, + avatarUrl, + }), + ]).length, + 0, + ); + } +}); + test("catalog snapshot sanitization strips secrets and response allowlists", () => { const source = { format: "buzz-agent-snapshot", @@ -140,7 +203,7 @@ test("catalog snapshot sanitization strips secrets and response allowlists", () ); assert.equal(sanitized.definition.systemPrompt, "Review changes."); - assert.equal(sanitized.definition.respondTo, "allowlist"); + assert.equal(sanitized.definition.respondTo, "owner-only"); assert.equal("respondToAllowlist" in sanitized.definition, false); assert.equal("envVars" in sanitized.definition, false); assert.equal("privateKeyNsec" in sanitized.definition, false); diff --git a/desktop/src/features/agents/lib/personaCatalogRelay.ts b/desktop/src/features/agents/lib/personaCatalogRelay.ts index 4f3d36dbf..40965d275 100644 --- a/desktop/src/features/agents/lib/personaCatalogRelay.ts +++ b/desktop/src/features/agents/lib/personaCatalogRelay.ts @@ -103,6 +103,29 @@ function isMemoryLevel(value: unknown): value is SnapshotMemoryLevel { return value === "none" || value === "core" || value === "everything"; } +function isSafeHttpUrl(value: unknown): value is string { + if ( + typeof value !== "string" || + value.length === 0 || + /[\s()]/u.test(value) + ) { + return false; + } + try { + const parsed = new URL(value); + return parsed.protocol === "https:" || parsed.protocol === "http:"; + } catch { + return false; + } +} + +function isSafeAvatarUrl(value: unknown): value is string | null { + return ( + value === null || + (typeof value === "string" && value.length <= 2_048 && isSafeHttpUrl(value)) + ); +} + function isSafeSnapshotReference( value: unknown, ): value is CatalogSnapshotReference { @@ -112,17 +135,7 @@ function isSafeSnapshotReference( const size = value.size; const url = value.url; return ( - typeof url === "string" && - url.length > 0 && - !/[\s()]/u.test(url) && - (() => { - try { - const parsed = new URL(url); - return parsed.protocol === "https:" || parsed.protocol === "http:"; - } catch { - return false; - } - })() && + isSafeHttpUrl(url) && typeof sha256 === "string" && /^[0-9a-f]{64}$/u.test(sha256) && typeof size === "number" && @@ -181,7 +194,7 @@ function parseCatalogContent(event: RelayEvent): CatalogContent | null { typeof parsed.agent.displayName !== "string" || parsed.agent.displayName.trim().length === 0 || typeof parsed.agent.systemPrompt !== "string" || - stringOrNull(parsed.agent.avatarUrl) === undefined || + !isSafeAvatarUrl(parsed.agent.avatarUrl) || stringOrNull(parsed.agent.runtime) === undefined || stringOrNull(parsed.agent.model) === undefined || stringOrNull(parsed.agent.provider) === undefined || @@ -219,7 +232,7 @@ export function catalogPublicationsFromEvents( ): PersonaCatalogPublication[] { const sorted = [...events].sort( (left, right) => - right.created_at - left.created_at || right.id.localeCompare(left.id), + right.created_at - left.created_at || left.id.localeCompare(right.id), ); const seenCoordinates = new Set(); const publications: PersonaCatalogPublication[] = []; @@ -403,15 +416,17 @@ export function sanitizeCatalogSnapshotBytes( if (typeof parsed.definition.sourceIsBuiltIn === "boolean") { definition.sourceIsBuiltIn = parsed.definition.sourceIsBuiltIn; } - for (const key of [ - "systemPrompt", - "runtime", - "model", - "provider", - "respondTo", - ]) { + for (const key of ["systemPrompt", "runtime", "model", "provider"]) { copyOptionalString(parsed.definition, definition, key); } + if (parsed.definition.respondTo === "allowlist") { + definition.respondTo = "owner-only"; + } else if ( + parsed.definition.respondTo === "owner-only" || + parsed.definition.respondTo === "anyone" + ) { + definition.respondTo = parsed.definition.respondTo; + } for (const key of [ "parallelism", "idleTimeoutSeconds", @@ -485,10 +500,7 @@ export function sanitizeCatalogSnapshotBytes( } function publicAvatarUrl(avatarUrl: string | null): string | null { - if (!avatarUrl || avatarUrl.startsWith("data:") || avatarUrl.length > 2_048) { - return null; - } - return avatarUrl; + return isSafeAvatarUrl(avatarUrl) ? avatarUrl : null; } function monotonicCreatedAt(previousCreatedAt?: number | null): number { diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index d743bc090..7c2376008 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -114,6 +114,8 @@ type MockPersonaSeed = { model?: string | null; provider?: string | null; namePool?: string[]; + respondTo?: "owner-only" | "allowlist" | "anyone"; + respondToAllowlist?: string[]; }; type MockTeamSeed = { @@ -2160,6 +2162,11 @@ function resetMockPersonas(config?: E2eConfig) { model: persona.model ?? null, provider: persona.provider ?? null, name_pool: persona.namePool ?? [], + respond_to: persona.respondTo ?? null, + respond_to_allowlist: + persona.respondTo === "allowlist" + ? [...(persona.respondToAllowlist ?? [])] + : [], is_builtin: false, is_active: persona.isActive ?? true, source_team: persona.sourceTeam ?? null, @@ -10260,6 +10267,7 @@ export function maybeInstallE2eTauriMocks() { runtime: persona?.runtime ?? null, model: persona?.model ?? null, provider: persona?.provider ?? null, + respondTo: persona?.respond_to ?? null, respondToAllowlist: persona?.respond_to_allowlist ?? [], namePool: persona?.name_pool ?? [], }, diff --git a/desktop/tests/e2e/agents.spec.ts b/desktop/tests/e2e/agents.spec.ts index 0dbe74a2d..bb4b1cbf1 100644 --- a/desktop/tests/e2e/agents.spec.ts +++ b/desktop/tests/e2e/agents.spec.ts @@ -1333,6 +1333,8 @@ test("custom personas can be shared to the relay catalog", async ({ page }) => { { id: personaId, displayName: "Catalog Analyst", + respondTo: "allowlist", + respondToAllowlist: [TEST_IDENTITIES.alice.pubkey], systemPrompt: `## Design System And Styling - For design-system changes, check the local guidance in \`DESIGN.md\`, \`docs/color-token-mapping.md\`, \`src/shared/ui/AGENTS.md\`, and \`src/features/design-system/AGENTS.md\` before judging the implementation. @@ -1408,6 +1410,18 @@ This deliberately long fenced-code example must not establish the minimum width .click(); await expect(catalogAccess).toHaveText("Agent only"); await expect(publishCatalogUpdatesButton).toHaveCount(0); + const uploadCommand = (await readAgentShareCommands(page)).find( + (entry) => entry.command === "upload_media_bytes", + ); + const uploadedSnapshot = JSON.parse( + new TextDecoder().decode( + Uint8Array.from( + (uploadCommand?.payload as { data?: number[] } | undefined)?.data ?? [], + ), + ), + ); + expect(uploadedSnapshot.definition.respondTo).toBe("owner-only"); + expect(uploadedSnapshot.definition).not.toHaveProperty("respondToAllowlist"); await page .getByTestId("persona-share-dialog") .getByRole("button", { name: "Close" }) diff --git a/desktop/tests/helpers/bridge.ts b/desktop/tests/helpers/bridge.ts index 545e24422..3203010b2 100644 --- a/desktop/tests/helpers/bridge.ts +++ b/desktop/tests/helpers/bridge.ts @@ -104,6 +104,8 @@ type MockPersonaSeed = { /** Provider pinned on the persona. Leave empty for Codex/Claude runtimes. */ provider?: string | null; namePool?: string[]; + respondTo?: "owner-only" | "allowlist" | "anyone"; + respondToAllowlist?: string[]; }; type MockTeamSeed = {