diff --git a/.changeset/fix-visibility-tier-gate-blockers.md b/.changeset/fix-visibility-tier-gate-blockers.md new file mode 100644 index 0000000000..33acaea934 --- /dev/null +++ b/.changeset/fix-visibility-tier-gate-blockers.md @@ -0,0 +1,11 @@ +--- +--- + +Close the three-tier agent visibility blockers flagged in the PR #2793 review that landed before the review was addressed. + +- **POST /api/me/member-profile (create) now runs the same tier gate as PUT.** A fresh profile containing `visibility: 'public'` on an Explorer-tier org was accepted verbatim, then filtered-through strictly without a tier re-check. Extracted the coercion into a shared helper (`agent-visibility-gate`), wired into both paths, now returns structured `warnings[]` on the create response too. +- **MCP `addAgent` now defaults to `members_only`** instead of `public`. Callers without an API-access tier could implicitly publish an agent via Addie, bypassing the explicit `/publish` tier check. +- **`applyAgentVisibility` re-reads the membership tier inside its transaction.** The outer `requireApiAccessTier` check is a fast-fail only; a concurrent Stripe downgrade committing between the outer read and the profile UPDATE would otherwise slip a `public` write past the gate. +- **`demotePublicAgentsOnTierDowngrade` now wraps the profile lock + update in a single transaction** with `SELECT ... FOR UPDATE`. Previously it read and wrote the profile as two statements, so a concurrent PATCH could reinsert a `public` entry between them. The two transactional guarantees together (apply + demote) mean a tier-gated publish and a tier downgrade can't interleave past each other's lock. + +New unit tests cover the shared gate helper and the transactional demote path; existing enforcement tests were rewritten to exercise the new pg client contract. Full unit suite green (1811 pass). diff --git a/server/public/member-profile.html b/server/public/member-profile.html index 199f5eb6af..9ad3b45fef 100644 --- a/server/public/member-profile.html +++ b/server/public/member-profile.html @@ -2242,15 +2242,37 @@

Tags

const wasNewProfile = isNewProfile; isNewProfile = false; + // Server may have coerced visibility settings the caller isn't + // eligible for (e.g. public → members_only on a non-API-access + // tier). Surface those warnings so the user knows we didn't + // silently persist what they asked for. + const warnings = Array.isArray(data.warnings) ? data.warnings : []; + const downgrades = warnings.filter(w => w && w.code === 'visibility_downgraded'); + // If this was a new profile creation, redirect to dashboard with success message if (wasNewProfile) { const orgParam = currentOrgId ? `&org=${currentOrgId}` : ''; - window.location.href = `/dashboard?profile_created=true${orgParam}`; + const warnParam = downgrades.length ? `&visibility_downgraded=${downgrades.length}` : ''; + window.location.href = `/dashboard?profile_created=true${orgParam}${warnParam}`; return; } - // For updates, show success message and stay on page - showSuccess('Profile saved successfully!'); + // For updates, show success message and stay on page. + // If visibility entries got downgraded, pin the reason above the + // generic banner so the user sees the upgrade CTA. + if (downgrades.length > 0) { + const agents = downgrades.map(w => w.agent_url).filter(Boolean); + const detail = agents.length + ? ` Affected agents: ${agents.join(', ')}.` + : ''; + showError( + `Saved, but ${downgrades.length} agent${downgrades.length > 1 ? 's' : ''} could not be listed publicly — ` + + `public listing requires Professional tier or higher, so they were stored as members_only instead.${detail} ` + + `Upgrade at /membership to publish publicly.` + ); + } else { + showSuccess('Profile saved successfully!'); + } saveBtn.disabled = false; saveBtn.textContent = 'Save Profile'; diff --git a/server/src/addie/mcp/member-tools.ts b/server/src/addie/mcp/member-tools.ts index 14f5060473..8dd7c20b94 100644 --- a/server/src/addie/mcp/member-tools.ts +++ b/server/src/addie/mcp/member-tools.ts @@ -1088,7 +1088,7 @@ export const MEMBER_TOOLS: AddieTool[] = [ { name: 'save_agent', description: - 'Save an agent URL to the organization\'s context and add it to the dashboard for compliance monitoring. Optionally store credentials securely (encrypted, never shown in conversations). Three auth modes, any of which may be combined with a new or existing save: (1) static bearer/basic via `auth_token`, (2) OAuth 2.0 client credentials (RFC 6749 §4.4, machine-to-machine) via `oauth_client_credentials`. Use this when users want to connect their agent, set up compliance monitoring, save their agent for testing, or provide credentials.', + 'Save an agent URL to the organization\'s context and add it to the dashboard for compliance monitoring. New agents land in the dashboard with `members_only` visibility — discoverable to fellow Professional-tier (or higher) members, but not publicly listed in the directory or brand.json. To list publicly, the caller promotes the agent via the dashboard publish flow; that flow gates on an API-access subscription tier. Optionally store credentials securely (encrypted, never shown in conversations). Three auth modes, any of which may be combined with a new or existing save: (1) static bearer/basic via `auth_token`, (2) OAuth 2.0 client credentials (RFC 6749 §4.4, machine-to-machine) via `oauth_client_credentials`. Use this when users want to connect their agent, set up compliance monitoring, save their agent for testing, or provide credentials.', usage_hints: 'use for "connect my agent", "add agent for compliance monitoring", "save my agent", "remember this agent URL", "store my auth token", "configure client credentials", "save OAuth client credentials"', input_schema: { type: 'object', @@ -4903,7 +4903,14 @@ export function createMemberToolHandlers( if (profile) { const agents = profile.agents || []; if (!agents.some((a: any) => a.url === agentUrl)) { - agents.push({ url: agentUrl, name: displayName, visibility: 'public' }); + // Default to members_only, not public. The public directory + // requires an API-access tier (Professional+); defaulting to + // 'public' here lets Addie implicitly publish an agent for an + // Explorer-tier caller who hasn't been tier-gated. Members_only + // keeps the agent discoverable to peer members with API access + // and lets the owner promote to public through the explicit, + // tier-checked /publish route when eligible. + agents.push({ url: agentUrl, name: displayName, visibility: 'members_only' }); await memberDb.updateProfile(profile.id, { agents }); } } @@ -4977,7 +4984,7 @@ export function createMemberToolHandlers( response += `\n🔐 OAuth client-credentials saved securely for token endpoint ${clientCredentials.token_endpoint}\n`; response += `_The client secret is encrypted and will never be shown again. The SDK exchanges and refreshes at test time._\n`; } - response += `\nThe agent has been added to your dashboard. When you test this agent, I'll automatically use the saved credentials.`; + response += `\nThe agent has been added to your dashboard with **members_only** visibility — other Professional-tier members can discover it, but it won't appear in the public directory. To publish publicly, use the dashboard publish flow (requires a Professional or higher subscription). When you test this agent, I'll automatically use the saved credentials.`; return response; } catch (error) { diff --git a/server/src/routes/member-profiles.ts b/server/src/routes/member-profiles.ts index 062c067d67..9b3a4f302b 100644 --- a/server/src/routes/member-profiles.ts +++ b/server/src/routes/member-profiles.ts @@ -27,6 +27,7 @@ import type { MemberBrandInfo, AgentVisibility, AgentConfig } from "../types.js" import type { CrawlerService } from "../crawler.js"; import { validateCrawlDomain } from "../utils/url-security.js"; import { recordProfilePublishedIfNeeded } from "../services/profile-publish-event.js"; +import { gateAgentVisibilityForCaller, type VisibilityWarning } from "../services/agent-visibility-gate.js"; const orgKnowledgeDb = new OrgKnowledgeDatabase(); @@ -310,6 +311,36 @@ export function createMemberProfileRouter(config: MemberProfileRoutesConfig): Ro } } + // Gate agent visibility on create using the same helper the PUT + // path uses. Without this, an Explorer-tier user creating their + // first profile can land `visibility: 'public'` directly in the + // JSONB — subsequent readers filter strictly on `=== 'public'` + // with no tier re-check, so the entry stays public forever. + const createOrgForTier = await orgDb.getOrganization(targetOrgId); + const createCallerHasApi = hasApiAccess(resolveMembershipTier(createOrgForTier)); + const { agents: gatedAgents, warnings: createWarnings } = + gateAgentVisibilityForCaller(agents, createCallerHasApi); + + // Same tier gate for the profile-level `is_public` flag. The + // `/visibility` PUT route gates this through hasActiveSubscription + // (line 1343), but the POST create and PUT bulk-update paths + // previously accepted the raw body value — same smuggle class as + // the agent-visibility bug this PR fixes. + let effectiveIsPublic = is_public === true; + if (effectiveIsPublic && !isDevModeEnabled()) { + if (!(await orgDb.hasActiveSubscription(targetOrgId))) { + effectiveIsPublic = false; + createWarnings.push({ + code: 'visibility_downgraded', + agent_url: 'profile', + requested: 'public', + applied: 'members_only', + reason: 'tier_required', + message: 'Making the profile publicly visible requires an active paid membership; stored as private instead.', + }); + } + } + const profile = await memberDb.createProfile({ workos_organization_id: targetOrgId, display_name, @@ -323,11 +354,11 @@ export function createMemberProfileRouter(config: MemberProfileRoutesConfig): Ro linkedin_url, twitter_url, offerings: offerings || [], - agents: agents || [], + agents: gatedAgents, headquarters, markets: markets || [], tags: tags || [], - is_public: is_public ?? false, + is_public: effectiveIsPublic, show_in_carousel: show_in_carousel ?? false, }); @@ -385,7 +416,10 @@ export function createMemberProfileRouter(config: MemberProfileRoutesConfig): Ro logger.info({ profileId: profile.id, orgId: targetOrgId, slug, durationMs: Date.now() - startTime }, 'POST /api/me/member-profile completed'); - res.status(201).json({ profile }); + res.status(201).json({ + profile, + ...(createWarnings.length ? { warnings: createWarnings } : {}), + }); } catch (error) { logger.error({ err: error, durationMs: Date.now() - startTime }, 'POST /api/me/member-profile error'); res.status(500).json({ @@ -496,41 +530,33 @@ export function createMemberProfileRouter(config: MemberProfileRoutesConfig): Ro // Enforce the tier gate on agent visibility so bulk-profile updates // cannot bypass the per-agent PATCH. Non-API-access callers may only // set 'private' or 'members_only' on any agent in the array; when - // they send 'public' we downgrade and tell them we did. - const warnings: Array> = []; + // they send 'public' we downgrade and tell them we did. Shared with + // the POST create path via gateAgentVisibilityForCaller. + let warnings: VisibilityWarning[] = []; if (Array.isArray(updates.agents)) { const localOrgForTier = await orgDb.getOrganization(targetOrgId); const callerHasApi = hasApiAccess(resolveMembershipTier(localOrgForTier)); - updates.agents = updates.agents.map((raw: unknown) => { - const a = (raw ?? {}) as Record; - const requested = a.visibility; - let visibility: AgentVisibility; - if (isValidAgentVisibility(requested)) { - visibility = requested; - } else if (a.is_public === true) { - visibility = 'public'; - } else { - visibility = 'private'; - } - if (visibility === 'public' && !callerHasApi) { - warnings.push({ - code: 'visibility_downgraded', - agent_url: a.url, - requested: 'public', - applied: 'members_only', - reason: 'tier_required', - message: 'Publicly listing an agent requires Professional tier or higher; stored as members_only instead.', - }); - visibility = 'members_only'; - } - const cleaned: Record = { - url: a.url, - visibility, - }; - if (typeof a.name === 'string') cleaned.name = a.name; - if (typeof a.type === 'string') cleaned.type = a.type; - return cleaned; - }); + const gated = gateAgentVisibilityForCaller(updates.agents, callerHasApi); + updates.agents = gated.agents; + warnings = gated.warnings; + } + + // Same gate for the profile-level `is_public` flag. The dedicated + // `/visibility` PUT route already gates this; the bulk-profile + // update path previously forwarded the raw body value, reopening + // the same smuggle a non-paying caller could use on POST create. + if (updates.is_public === true && !isDevModeEnabled()) { + if (!(await orgDb.hasActiveSubscription(targetOrgId))) { + updates.is_public = false; + warnings.push({ + code: 'visibility_downgraded', + agent_url: 'profile', + requested: 'public', + applied: 'members_only', + reason: 'tier_required', + message: 'Making the profile publicly visible requires an active paid membership; left as private.', + }); + } } const profile = await memberDb.updateProfileByOrgId(targetOrgId, updates); @@ -649,6 +675,7 @@ export function createMemberProfileRouter(config: MemberProfileRoutesConfig): Ro ): Promise< | { status: 404; body: { error: string } } | { status: 400; body: { error: string } } + | { status: 403; body: { error: string; message: string } } | { status: 200; body: Record } > { const pool = getPool(); @@ -667,6 +694,44 @@ export function createMemberProfileRouter(config: MemberProfileRoutesConfig): Ro await client.query('ROLLBACK'); return { status: 404, body: { error: 'Profile not found' } }; } + + // Re-read tier INSIDE the transaction when the caller is trying + // to publish. The outer `requireApiAccessTier` check is a + // fast-fail for UX, but it reads the org row before this tx + // starts — a concurrent Stripe downgrade webhook can commit + // between the outer check and this UPDATE, letting a `public` + // write land on an org that's no longer API-access. Reading the + // tier-relevant columns here closes that window: if the org was + // downgraded after our outer check, we see the new state and + // ROLLBACK. The Stripe demote path locks member_profiles via + // FOR UPDATE, so it can't interleave past our own lock. + if (target === 'public') { + const orgRow = await client.query<{ + membership_tier: string | null; + subscription_price_lookup_key: string | null; + subscription_status: string | null; + subscription_amount: number | null; + subscription_interval: string | null; + is_personal: boolean; + }>( + `SELECT membership_tier, subscription_price_lookup_key, subscription_status, + subscription_amount, subscription_interval, is_personal + FROM organizations + WHERE workos_organization_id = $1`, + [orgId] + ); + const currentTier = resolveMembershipTier(orgRow.rows[0] ?? null); + if (!hasApiAccess(currentTier)) { + await client.query('ROLLBACK'); + return { + status: 403, + body: { + error: 'tier_required', + message: 'Publicly listing an agent requires Professional tier or higher.', + }, + }; + } + } const row = profileRow.rows[0] as { id: string; agents: unknown; primary_brand_domain: string | null }; const parsedAgents = typeof row.agents === 'string' ? JSON.parse(row.agents) diff --git a/server/src/services/agent-visibility-enforcement.ts b/server/src/services/agent-visibility-enforcement.ts index 2ff9559a5b..71fe2d9591 100644 --- a/server/src/services/agent-visibility-enforcement.ts +++ b/server/src/services/agent-visibility-enforcement.ts @@ -6,8 +6,8 @@ * API-access members while respecting the gate on public listing. */ -import { MemberDatabase } from '../db/member-db.js'; import { BrandDatabase } from '../db/brand-db.js'; +import { getPool } from '../db/client.js'; import { hasApiAccess, type MembershipTier, @@ -29,30 +29,92 @@ export interface DemoteResult { * strip them from the org's primary brand.json manifest. * * Returns null when no action was taken (no downgrade, or no profile). + * + * The profile read + write runs inside a single transaction with + * `SELECT ... FOR UPDATE` on the profile row, so a concurrent PUT / + * PATCH / publish against the same profile can't reinsert a `public` + * entry between our read and write. The inner tier check in + * `applyAgentVisibility` (#2793 follow-up) is the other side of this + * invariant: it blocks on the same profile row before committing a + * new `public` write, so the two paths can't interleave past each + * other's lock. */ export async function demotePublicAgentsOnTierDowngrade( orgId: string, oldTier: MembershipTier | null, newTier: MembershipTier | null, - memberDb: MemberDatabase = new MemberDatabase(), brandDb: BrandDatabase = new BrandDatabase(), ): Promise { if (!hasApiAccess(oldTier)) return null; if (hasApiAccess(newTier)) return null; - const profile = await memberDb.getProfileByOrgId(orgId); - if (!profile) return null; + const pool = getPool(); + const client = await pool.connect(); + let profile: { id: string; agents: AgentConfig[]; primary_brand_domain: string | null } | null = null; + let demotedUrls = new Set(); + try { + await client.query('BEGIN'); - const agents = profile.agents || []; - const publicAgents = agents.filter((a) => a.visibility === 'public'); - if (publicAgents.length === 0) return null; + const row = await client.query<{ + id: string; + agents: unknown; + primary_brand_domain: string | null; + }>( + `SELECT id, agents, primary_brand_domain + FROM member_profiles + WHERE workos_organization_id = $1 + FOR UPDATE`, + [orgId], + ); + if (row.rowCount === 0) { + await client.query('ROLLBACK'); + return null; + } + const r = row.rows[0]; + const parsedAgents = typeof r.agents === 'string' + ? JSON.parse(r.agents) + : Array.isArray(r.agents) ? r.agents : []; + const agents: AgentConfig[] = (parsedAgents as unknown[]).map((a) => { + const o = (a ?? {}) as Record; + const v = o.visibility; + const visibility = v === 'private' || v === 'members_only' || v === 'public' + ? v + : o.is_public === true ? 'public' : 'private'; + return { + url: String(o.url ?? ''), + visibility, + ...(typeof o.name === 'string' ? { name: o.name } : {}), + ...(typeof o.type === 'string' ? { type: o.type as AgentConfig['type'] } : {}), + }; + }); - const demotedUrls = new Set(publicAgents.map((a) => a.url)); - const updatedAgents: AgentConfig[] = agents.map((a) => - demotedUrls.has(a.url) ? { ...a, visibility: 'members_only' as const } : a - ); + const publicAgents = agents.filter((a) => a.visibility === 'public'); + if (publicAgents.length === 0) { + await client.query('ROLLBACK'); + return null; + } - await memberDb.updateProfileByOrgId(orgId, { agents: updatedAgents }); + demotedUrls = new Set(publicAgents.map((a) => a.url)); + const updatedAgents: AgentConfig[] = agents.map((a) => + demotedUrls.has(a.url) ? { ...a, visibility: 'members_only' as const } : a + ); + profile = { id: r.id, agents: updatedAgents, primary_brand_domain: r.primary_brand_domain }; + + await client.query( + `UPDATE member_profiles + SET agents = $1::jsonb, updated_at = NOW() + WHERE id = $2`, + [JSON.stringify(updatedAgents), r.id], + ); + await client.query('COMMIT'); + } catch (err) { + await client.query('ROLLBACK').catch(() => {}); + throw err; + } finally { + client.release(); + } + + if (!profile) return null; let brandJsonCleared = false; if (profile.primary_brand_domain) { diff --git a/server/src/services/agent-visibility-gate.ts b/server/src/services/agent-visibility-gate.ts new file mode 100644 index 0000000000..6a365684cb --- /dev/null +++ b/server/src/services/agent-visibility-gate.ts @@ -0,0 +1,70 @@ +/** + * Normalize a caller-supplied agents[] and enforce the visibility tier + * gate. Non-API-access callers cannot set `visibility: 'public'`; any + * such entries are downgraded to `members_only` and a structured warning + * is emitted so the caller knows we coerced them. Shared between the + * POST (create) and PUT (update) paths of /api/me/member-profile so + * neither can be smuggled past the per-agent /publish tier check. + */ + +import { isValidAgentVisibility, isValidAgentType } from '../types.js'; +import type { AgentConfig, AgentVisibility } from '../types.js'; + +export interface VisibilityWarning { + code: 'visibility_downgraded'; + /** + * String identifier for what got downgraded — an agent URL, or the + * sentinel `'profile'` when the profile-level `is_public` flag was + * the target. Coerced to string at emit time so the wire shape is + * trustworthy for clients that render it. + */ + agent_url: string; + requested: 'public'; + applied: 'members_only'; + reason: 'tier_required'; + message: string; +} + +export function gateAgentVisibilityForCaller( + rawAgents: unknown, + callerHasApi: boolean, +): { agents: AgentConfig[]; warnings: VisibilityWarning[] } { + if (!Array.isArray(rawAgents)) return { agents: [], warnings: [] }; + const warnings: VisibilityWarning[] = []; + const agents = rawAgents.map((raw: unknown) => { + const a = (raw ?? {}) as Record; + const requested = a.visibility; + let visibility: AgentVisibility; + if (isValidAgentVisibility(requested)) { + visibility = requested; + } else if (a.is_public === true) { + visibility = 'public'; + } else { + visibility = 'private'; + } + const url = typeof a.url === 'string' ? a.url : String(a.url ?? ''); + if (visibility === 'public' && !callerHasApi) { + warnings.push({ + code: 'visibility_downgraded', + agent_url: url, + requested: 'public', + applied: 'members_only', + reason: 'tier_required', + message: 'Publicly listing an agent requires Professional tier or higher; stored as members_only instead.', + }); + visibility = 'members_only'; + } + // Drop unknown `type` values instead of casting — the field flows + // into brand.json (`agentEntry.type`) so an arbitrary tenant string + // would become a durable artifact in other members' manifests. + const typeValue = typeof a.type === 'string' && isValidAgentType(a.type) ? a.type : undefined; + const cleaned: AgentConfig = { + url, + visibility, + ...(typeof a.name === 'string' ? { name: a.name } : {}), + ...(typeValue ? { type: typeValue } : {}), + }; + return cleaned; + }); + return { agents, warnings }; +} diff --git a/server/tests/unit/agent-visibility-enforcement.test.ts b/server/tests/unit/agent-visibility-enforcement.test.ts index 3201aeb1c8..35a9b41c42 100644 --- a/server/tests/unit/agent-visibility-enforcement.test.ts +++ b/server/tests/unit/agent-visibility-enforcement.test.ts @@ -1,23 +1,107 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import type { AgentConfig } from '../../src/types.js'; +/** + * `demotePublicAgentsOnTierDowngrade` uses `getPool()` + raw SQL with + * `SELECT … FOR UPDATE` so the read and write can't interleave with a + * concurrent publish. The mocks below emulate the client contract that + * the service depends on: BEGIN / SELECT / UPDATE / COMMIT / ROLLBACK. + */ +vi.mock('../../src/db/client.js', () => { + const release = vi.fn(); + const client: { query: ReturnType; release: typeof release } = { + query: vi.fn(), + release, + }; + const connect = vi.fn(async () => client); + return { + getPool: () => ({ connect }), + query: vi.fn(), + // test-accessor so each test can reconfigure the client's query stub + __client: client, + __connect: connect, + }; +}); + +import { demotePublicAgentsOnTierDowngrade } from '../../src/services/agent-visibility-enforcement.js'; +import * as dbClient from '../../src/db/client.js'; + +/** + * Validated module-level bindings for the mock accessors. The mock + * factory above always exports these, but the real `db/client.js` + * module type doesn't describe them — so we reach in, validate the + * shape once, and hand TypeScript (and CodeQL) a concretely-typed + * binding to use everywhere else. Throws loudly if the mock plumbing + * ever drifts, which is a test-setup bug and should not silently + * degrade into nullish property access at runtime. + */ +interface MockedPgClient { + query: ReturnType; + release: ReturnType; +} +const mockedModule = dbClient as unknown as { + __client?: MockedPgClient; + __connect?: ReturnType; +}; +const mockedClient: MockedPgClient = (() => { + const c = mockedModule.__client; + if (!c || typeof c.query?.mockReset !== 'function' || typeof c.release?.mockReset !== 'function') { + throw new Error('Mocked pg client not wired — vi.mock factory drifted.'); + } + return c; +})(); +const mockedConnect: ReturnType = (() => { + const c = mockedModule.__connect; + if (!c || typeof c.mockClear !== 'function') { + throw new Error('Mocked getPool connect not wired — vi.mock factory drifted.'); + } + return c; +})(); + +type SelectRow = { + id: string; + agents: unknown; + primary_brand_domain: string | null; +}; + +type RecordedQuery = { sql: string; params: unknown[] }; + +/** + * Wire the pg client mock so the tx flow (BEGIN → SELECT FOR UPDATE → + * UPDATE → COMMIT) behaves as described. Records all queries so tests + * can assert the exact SQL + params, not just that *some* UPDATE ran. + */ +function mockProfileTx(rows: SelectRow[]): { recorded: RecordedQuery[]; updateArgs: unknown[][] } { + const recorded: RecordedQuery[] = []; + const updateArgs: unknown[][] = []; + mockedClient.query.mockReset(); + mockedClient.release.mockReset(); + mockedClient.query.mockImplementation(async (sql: string, params?: unknown[]) => { + recorded.push({ sql, params: params ?? [] }); + if (sql === 'BEGIN' || sql === 'COMMIT' || sql === 'ROLLBACK') { + return { rowCount: 0, rows: [] }; + } + if (sql.includes('SELECT id, agents, primary_brand_domain')) { + return { rowCount: rows.length, rows }; + } + if (sql.trim().startsWith('UPDATE member_profiles')) { + updateArgs.push(params ?? []); + return { rowCount: 1, rows: [] }; + } + throw new Error(`Unexpected query: ${sql}`); + }); + return { recorded, updateArgs }; +} + describe('demotePublicAgentsOnTierDowngrade', () => { - let memberDb: any; let brandDb: any; - let demote: typeof import('../../src/services/agent-visibility-enforcement.js').demotePublicAgentsOnTierDowngrade; - beforeEach(async () => { - memberDb = { - getProfileByOrgId: vi.fn(), - updateProfileByOrgId: vi.fn().mockResolvedValue(null), - }; + beforeEach(() => { brandDb = { getDiscoveredBrandByDomain: vi.fn(), updateManifestAgents: vi.fn().mockResolvedValue(undefined), }; - ({ demotePublicAgentsOnTierDowngrade: demote } = await import( - '../../src/services/agent-visibility-enforcement.js' - )); + mockedConnect.mockClear(); }); function agent(url: string, visibility: AgentConfig['visibility']): AgentConfig { @@ -25,68 +109,95 @@ describe('demotePublicAgentsOnTierDowngrade', () => { } it('no-op when old tier had no API access', async () => { - const result = await demote('org1', 'individual_academic', null, memberDb, brandDb); + const result = await demotePublicAgentsOnTierDowngrade( + 'org1', 'individual_academic', null, brandDb, + ); expect(result).toBeNull(); - expect(memberDb.getProfileByOrgId).not.toHaveBeenCalled(); + expect(mockedConnect).not.toHaveBeenCalled(); }); it('no-op when new tier still has API access', async () => { - const result = await demote('org1', 'company_icl', 'individual_professional', memberDb, brandDb); + const result = await demotePublicAgentsOnTierDowngrade( + 'org1', 'company_icl', 'individual_professional', brandDb, + ); expect(result).toBeNull(); - expect(memberDb.getProfileByOrgId).not.toHaveBeenCalled(); + expect(mockedConnect).not.toHaveBeenCalled(); + }); + + it('locks the profile row with SELECT FOR UPDATE on the supplied orgId', async () => { + // Mechanism test — regressions that drop FOR UPDATE or pass the + // wrong org param are the exact thing this PR's transactional + // rewrite is meant to prevent. + const { recorded } = mockProfileTx([ + { id: 'p1', agents: [agent('https://a', 'public')], primary_brand_domain: null }, + ]); + await demotePublicAgentsOnTierDowngrade('org-target', 'individual_professional', null, brandDb); + const selectProfile = recorded.find(r => r.sql.includes('SELECT id, agents, primary_brand_domain')); + expect(selectProfile, 'should issue a SELECT for the profile row').toBeTruthy(); + expect(selectProfile!.sql).toMatch(/FOR UPDATE/); + expect(selectProfile!.params[0]).toBe('org-target'); }); it('no-op when org has no profile', async () => { - memberDb.getProfileByOrgId.mockResolvedValue(null); - const result = await demote('org1', 'individual_professional', null, memberDb, brandDb); + const { recorded } = mockProfileTx([]); + const result = await demotePublicAgentsOnTierDowngrade( + 'org1', 'individual_professional', null, brandDb, + ); expect(result).toBeNull(); - expect(memberDb.updateProfileByOrgId).not.toHaveBeenCalled(); + const sqls = recorded.map(r => r.sql); + expect(sqls).toContain('BEGIN'); + expect(sqls).toContain('ROLLBACK'); + expect(sqls.some(q => q.trim().startsWith('UPDATE'))).toBe(false); }); it('no-op when profile has no public agents', async () => { - memberDb.getProfileByOrgId.mockResolvedValue({ - agents: [agent('https://a.example', 'private'), agent('https://b.example', 'members_only')], - primary_brand_domain: null, - }); - const result = await demote('org1', 'individual_professional', 'individual_academic', memberDb, brandDb); + const agents = [agent('https://a.example', 'private'), agent('https://b.example', 'members_only')]; + const { recorded } = mockProfileTx([{ id: 'p1', agents, primary_brand_domain: null }]); + const result = await demotePublicAgentsOnTierDowngrade( + 'org1', 'individual_professional', 'individual_academic', brandDb, + ); expect(result).toBeNull(); - expect(memberDb.updateProfileByOrgId).not.toHaveBeenCalled(); + const sqls = recorded.map(r => r.sql); + expect(sqls).toContain('ROLLBACK'); + expect(sqls.some(q => q.trim().startsWith('UPDATE member_profiles'))).toBe(false); }); it('demotes public agents to members_only on Professional → Explorer', async () => { - memberDb.getProfileByOrgId.mockResolvedValue({ - agents: [ - agent('https://pub.example', 'public'), - agent('https://mem.example', 'members_only'), - agent('https://priv.example', 'private'), - ], - primary_brand_domain: null, - }); - const result = await demote('org1', 'individual_professional', 'individual_academic', memberDb, brandDb); + const agents = [ + agent('https://pub.example', 'public'), + agent('https://mem.example', 'members_only'), + agent('https://priv.example', 'private'), + ]; + const { updateArgs } = mockProfileTx([ + { id: 'p1', agents, primary_brand_domain: null }, + ]); + const result = await demotePublicAgentsOnTierDowngrade( + 'org1', 'individual_professional', 'individual_academic', brandDb, + ); expect(result).toEqual({ orgId: 'org1', demotedCount: 1, brandJsonCleared: false }); - expect(memberDb.updateProfileByOrgId).toHaveBeenCalledWith('org1', { - agents: [ - agent('https://pub.example', 'members_only'), - agent('https://mem.example', 'members_only'), - agent('https://priv.example', 'private'), - ], - }); + expect(updateArgs).toHaveLength(1); + const writtenAgents = JSON.parse(updateArgs[0][0] as string); + expect(writtenAgents).toEqual([ + agent('https://pub.example', 'members_only'), + agent('https://mem.example', 'members_only'), + agent('https://priv.example', 'private'), + ]); }); it('demotes on full cancellation (newTier = null)', async () => { - memberDb.getProfileByOrgId.mockResolvedValue({ - agents: [agent('https://p.example', 'public')], - primary_brand_domain: null, - }); - const result = await demote('org1', 'company_leader', null, memberDb, brandDb); + mockProfileTx([ + { id: 'p1', agents: [agent('https://p.example', 'public')], primary_brand_domain: null }, + ]); + const result = await demotePublicAgentsOnTierDowngrade( + 'org1', 'company_leader', null, brandDb, + ); expect(result?.demotedCount).toBe(1); }); it('clears demoted agents from a community brand.json', async () => { - memberDb.getProfileByOrgId.mockResolvedValue({ - agents: [agent('https://p.example', 'public')], - primary_brand_domain: 'acme.example', - }); + mockProfileTx([ + { id: 'p1', agents: [agent('https://p.example', 'public')], primary_brand_domain: 'acme.example' }, + ]); brandDb.getDiscoveredBrandByDomain.mockResolvedValue({ source_type: 'community', brand_manifest: { @@ -96,7 +207,9 @@ describe('demotePublicAgentsOnTierDowngrade', () => { ], }, }); - const result = await demote('org1', 'individual_professional', null, memberDb, brandDb); + const result = await demotePublicAgentsOnTierDowngrade( + 'org1', 'individual_professional', null, brandDb, + ); expect(result?.brandJsonCleared).toBe(true); expect(brandDb.updateManifestAgents).toHaveBeenCalledWith( 'acme.example', @@ -106,18 +219,37 @@ describe('demotePublicAgentsOnTierDowngrade', () => { }); it('does not touch brand.json for self-hosted brands', async () => { - memberDb.getProfileByOrgId.mockResolvedValue({ - agents: [agent('https://p.example', 'public')], - primary_brand_domain: 'acme.example', - }); + mockProfileTx([ + { id: 'p1', agents: [agent('https://p.example', 'public')], primary_brand_domain: 'acme.example' }, + ]); brandDb.getDiscoveredBrandByDomain.mockResolvedValue({ source_type: 'brand_json', brand_manifest: { agents: [{ url: 'https://p.example', type: 'brand', id: 'p' }], }, }); - const result = await demote('org1', 'individual_professional', null, memberDb, brandDb); + const result = await demotePublicAgentsOnTierDowngrade( + 'org1', 'individual_professional', null, brandDb, + ); expect(result?.brandJsonCleared).toBe(false); expect(brandDb.updateManifestAgents).not.toHaveBeenCalled(); }); + + it('commits the profile tx before touching brand.json (so a failed manifest write does not orphan the JSONB update)', async () => { + const { recorded } = mockProfileTx([ + { id: 'p1', agents: [agent('https://p.example', 'public')], primary_brand_domain: 'acme.example' }, + ]); + brandDb.getDiscoveredBrandByDomain.mockResolvedValue({ + source_type: 'community', + brand_manifest: { agents: [{ url: 'https://p.example', type: 'brand', id: 'p' }] }, + }); + await demotePublicAgentsOnTierDowngrade( + 'org1', 'individual_professional', null, brandDb, + ); + const sqls = recorded.map(r => r.sql); + const commitAt = sqls.indexOf('COMMIT'); + expect(commitAt).toBeGreaterThan(-1); + const updateAt = sqls.findIndex(q => q.trim().startsWith('UPDATE member_profiles')); + expect(updateAt).toBeLessThan(commitAt); + }); }); diff --git a/server/tests/unit/agent-visibility-gate.test.ts b/server/tests/unit/agent-visibility-gate.test.ts new file mode 100644 index 0000000000..2d050a0210 --- /dev/null +++ b/server/tests/unit/agent-visibility-gate.test.ts @@ -0,0 +1,122 @@ +import { describe, it, expect } from 'vitest'; +import { gateAgentVisibilityForCaller } from '../../src/services/agent-visibility-gate.js'; + +/** + * Shared-helper tests for the tier-gate coercion used by both POST and + * PUT of /api/me/member-profile. The reviewer flagged (PR #2793) that + * the PUT path did the gate but the POST path didn't — so an Explorer + * user creating their first profile could land `visibility: 'public'` + * on first write, and subsequent readers filtered strictly on + * `=== 'public'` without a tier re-check. This helper centralizes the + * coercion so neither path can drift. + */ + +describe('gateAgentVisibilityForCaller', () => { + it('returns empty results for a non-array input', () => { + expect(gateAgentVisibilityForCaller(undefined, true)).toEqual({ agents: [], warnings: [] }); + expect(gateAgentVisibilityForCaller(null, false)).toEqual({ agents: [], warnings: [] }); + expect(gateAgentVisibilityForCaller('not an array', false)).toEqual({ agents: [], warnings: [] }); + }); + + it('passes valid visibility values through when caller has API access', () => { + const { agents, warnings } = gateAgentVisibilityForCaller( + [ + { url: 'https://a', visibility: 'public' }, + { url: 'https://b', visibility: 'members_only' }, + { url: 'https://c', visibility: 'private' }, + ], + true, + ); + expect(warnings).toEqual([]); + expect(agents).toEqual([ + { url: 'https://a', visibility: 'public' }, + { url: 'https://b', visibility: 'members_only' }, + { url: 'https://c', visibility: 'private' }, + ]); + }); + + it('downgrades public → members_only and emits a warning when caller lacks API access', () => { + const { agents, warnings } = gateAgentVisibilityForCaller( + [{ url: 'https://a', visibility: 'public' }], + false, + ); + expect(agents).toEqual([{ url: 'https://a', visibility: 'members_only' }]); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatchObject({ + code: 'visibility_downgraded', + agent_url: 'https://a', + requested: 'public', + applied: 'members_only', + reason: 'tier_required', + }); + }); + + it('accepts members_only from a non-API-access caller', () => { + const { agents, warnings } = gateAgentVisibilityForCaller( + [{ url: 'https://a', visibility: 'members_only' }], + false, + ); + expect(agents).toEqual([{ url: 'https://a', visibility: 'members_only' }]); + expect(warnings).toEqual([]); + }); + + it('translates legacy is_public:true to public when visibility is missing', () => { + const { agents } = gateAgentVisibilityForCaller( + [{ url: 'https://a', is_public: true }], + true, + ); + expect(agents[0].visibility).toBe('public'); + }); + + it('translates legacy is_public:false (or missing) to private', () => { + const { agents } = gateAgentVisibilityForCaller( + [{ url: 'https://a' }, { url: 'https://b', is_public: false }], + true, + ); + expect(agents[0].visibility).toBe('private'); + expect(agents[1].visibility).toBe('private'); + }); + + it('downgrades legacy is_public:true → members_only for a non-API-access caller', () => { + // If a legacy client sends is_public:true against a downgraded org, + // we still have to enforce the tier gate — not just for the new + // `visibility:'public'` attack surface. + const { agents, warnings } = gateAgentVisibilityForCaller( + [{ url: 'https://a', is_public: true }], + false, + ); + expect(agents[0].visibility).toBe('members_only'); + expect(warnings).toHaveLength(1); + }); + + it('rejects unknown visibility strings by falling back to legacy is_public (then private)', () => { + const { agents } = gateAgentVisibilityForCaller( + [{ url: 'https://a', visibility: 'admin' }], + true, + ); + expect(agents[0].visibility).toBe('private'); + }); + + it('strips unexpected fields from agent entries', () => { + const { agents } = gateAgentVisibilityForCaller( + [{ url: 'https://a', visibility: 'private', name: 'Agent', type: 'brand', password: 'hunter2' }], + true, + ); + expect(agents[0]).toEqual({ url: 'https://a', visibility: 'private', name: 'Agent', type: 'brand' }); + expect((agents[0] as any).password).toBeUndefined(); + }); + + it('handles a mix of public and non-public agents in one call', () => { + const { agents, warnings } = gateAgentVisibilityForCaller( + [ + { url: 'https://a', visibility: 'public' }, + { url: 'https://b', visibility: 'private' }, + { url: 'https://c', visibility: 'public' }, + ], + false, + ); + expect(agents.map((a) => a.visibility)).toEqual(['members_only', 'private', 'members_only']); + expect(warnings).toHaveLength(2); + expect(warnings.map((w) => w.agent_url)).toEqual(['https://a', 'https://c']); + }); +});