From 0f32529370a83184fbc533bd54bf57a16d023e3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Guti=C3=A9rrez?= <35082514+alezmad@users.noreply.github.com> Date: Sat, 2 May 2026 18:39:25 +0100 Subject: [PATCH] fix(apikey): revoke must verify a row was actually updated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claudemesh apikey revoke reported success even when the input didn't match any row in mesh.api_key. The CLI's `apikey list` shows truncated 8-char prefixes; users naturally paste those; broker did exact-id match against meshApiKey.id; UPDATE affected 0 rows; old revokeApiKey returned void so the CLI couldn't tell. Discovered via end-to-end CLI smoke test against prod (roadmap validation pass). Three-part fix: - broker.revokeApiKey now returns { status: "revoked"|"not_found"|"not_unique"; id?, matches? } and accepts either the full id or a unique prefix (>=6 chars). Prefix matching is bounded to the caller's mesh and only succeeds if exactly one row matches; ambiguous prefixes return not_unique so we never silently revoke the wrong key. - New WSApiKeyRevokeResponseMessage carries the structured status back to the CLI. Old apikey_revoke_ok type removed before being released — never shipped to users. The error path is no longer used for not_found/not_unique cases; the unified response carries both outcomes. - CLI's apiKeyRevoke now resolves with { ok, id } | { ok: false, code, message }. runApiKeyRevoke surfaces the code/message and exits non-zero on failure (NOT_FOUND for missing, INVALID_ARGS for ambiguous prefix). Net effect: pasting `claudemesh apikey revoke vq0fwjdX` now actually revokes the key whose id starts with vq0fwjdX (or fails loud if 0 or >1 keys match). Verified against prod via the new branch's CLI binary before commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/broker/src/broker.ts | 51 ++++++++++++++++++++--- apps/broker/src/index.ts | 14 +++++-- apps/broker/src/types.ts | 11 +++++ apps/cli/src/commands/apikey.ts | 18 ++++++-- apps/cli/src/services/broker/ws-client.ts | 42 +++++++++++++++++-- 5 files changed, 121 insertions(+), 15 deletions(-) diff --git a/apps/broker/src/broker.ts b/apps/broker/src/broker.ts index 1f5f9dc..13a8809 100644 --- a/apps/broker/src/broker.ts +++ b/apps/broker/src/broker.ts @@ -901,12 +901,51 @@ export async function listApiKeys(meshId: string): Promise< })); } -/** Revoke an API key. Idempotent. */ -export async function revokeApiKey(args: { meshId: string; id: string }): Promise { - await db - .update(meshApiKey) - .set({ revokedAt: new Date() }) - .where(and(eq(meshApiKey.meshId, args.meshId), eq(meshApiKey.id, args.id))); +/** + * Revoke an API key. Returns "revoked" with the matched id, or a + * structured error. + * + * Accepts either the full id or a unique prefix (length >= 6) — the + * CLI's `apikey list` truncates ids to 8 chars for display, so users + * naturally paste the truncated form. Prefix matching is bounded to + * the caller's mesh and only succeeds if exactly one key matches; + * ambiguous prefixes return `not_unique` so we never silently revoke + * the wrong key. + * + * Idempotent for already-revoked keys (returns "revoked" with the + * prior revoked_at). + */ +export async function revokeApiKey(args: { + meshId: string; + id: string; +}): Promise< + | { status: "revoked"; id: string } + | { status: "not_found" } + | { status: "not_unique"; matches: number } +> { + const candidates = await db + .select({ id: meshApiKey.id, revokedAt: meshApiKey.revokedAt }) + .from(meshApiKey) + .where( + and( + eq(meshApiKey.meshId, args.meshId), + // Try exact match first; fall back to prefix. + sql`(${meshApiKey.id} = ${args.id} OR ${meshApiKey.id} LIKE ${args.id + "%"})`, + ), + ) + .limit(2); + if (candidates.length === 0) return { status: "not_found" }; + if (candidates.length > 1) { + return { status: "not_unique", matches: candidates.length }; + } + const matched = candidates[0]!; + if (!matched.revokedAt) { + await db + .update(meshApiKey) + .set({ revokedAt: new Date() }) + .where(eq(meshApiKey.id, matched.id)); + } + return { status: "revoked", id: matched.id }; } /** diff --git a/apps/broker/src/index.ts b/apps/broker/src/index.ts index 60551ff..bbabcbc 100644 --- a/apps/broker/src/index.ts +++ b/apps/broker/src/index.ts @@ -2523,9 +2523,17 @@ function handleConnection(ws: WebSocket): void { case "apikey_revoke": { const ar = msg as Extract; - if (!ar.id) { sendError(ws, "invalid_args", "id required", _reqId); break; } - await revokeApiKey({ meshId: conn.meshId, id: ar.id }); - log.info("ws apikey_revoke", { presence_id: presenceId, key_id: ar.id }); + if (!ar.id) { sendError(ws, "invalid_args", "id required", undefined, _reqId); break; } + const result = await revokeApiKey({ meshId: conn.meshId, id: ar.id }); + log.info("ws apikey_revoke", { presence_id: presenceId, key_id: ar.id, status: result.status }); + const resp: WSServerMessage = { + type: "apikey_revoke_response", + status: result.status, + ...(result.status === "revoked" ? { id: result.id } : {}), + ...(result.status === "not_unique" ? { matches: result.matches } : {}), + ...(_reqId ? { _reqId } : {}), + }; + conn.ws.send(JSON.stringify(resp)); break; } diff --git a/apps/broker/src/types.ts b/apps/broker/src/types.ts index 82147a8..b510874 100644 --- a/apps/broker/src/types.ts +++ b/apps/broker/src/types.ts @@ -233,6 +233,16 @@ export interface WSApiKeyListResponseMessage { _reqId?: string; } +export interface WSApiKeyRevokeResponseMessage { + type: "apikey_revoke_response"; + status: "revoked" | "not_found" | "not_unique"; + /** Full id of the revoked key on success (may differ from input if a prefix was sent). */ + id?: string; + /** How many keys matched on not_unique. */ + matches?: number; + _reqId?: string; +} + // ── Topics (v0.2.0) ───────────────────────────────────────────────── // Topics complement groups: groups are identity tags, topics are // conversation scopes. targetSpec for topic-tagged messages is @@ -1484,6 +1494,7 @@ export type WSServerMessage = | WSTopicHistoryResponseMessage | WSApiKeyCreatedMessage | WSApiKeyListResponseMessage + | WSApiKeyRevokeResponseMessage | WSStateChangeMessage | WSStateResultMessage | WSStateListMessage diff --git a/apps/cli/src/commands/apikey.ts b/apps/cli/src/commands/apikey.ts index e2ab0a5..20da57b 100644 --- a/apps/cli/src/commands/apikey.ts +++ b/apps/cli/src/commands/apikey.ts @@ -112,9 +112,21 @@ export async function runApiKeyRevoke(id: string, flags: ApiKeyFlags): Promise { - await client.apiKeyRevoke(id); - if (flags.json) console.log(JSON.stringify({ revoked: id })); - else render.ok("revoked", clay(id.slice(0, 8))); + const result = await client.apiKeyRevoke(id); + if (!result.ok) { + if (flags.json) { + console.log(JSON.stringify({ ok: false, code: result.code, message: result.message })); + } else { + render.err(`${result.code}: ${result.message}`); + } + return result.code === "not_found" + ? EXIT.NOT_FOUND + : result.code === "not_unique" + ? EXIT.INVALID_ARGS + : EXIT.INTERNAL_ERROR; + } + if (flags.json) console.log(JSON.stringify({ revoked: result.id })); + else render.ok("revoked", clay(result.id.slice(0, 8))); return EXIT.SUCCESS; }); } diff --git a/apps/cli/src/services/broker/ws-client.ts b/apps/cli/src/services/broker/ws-client.ts index fb291e6..c11e35e 100644 --- a/apps/cli/src/services/broker/ws-client.ts +++ b/apps/cli/src/services/broker/ws-client.ts @@ -169,6 +169,7 @@ export class BrokerClient { // ── API keys (v0.2.0) ── private apiKeyCreatedResolvers = new Map; topicScopes: string[] | null; createdAt: string } | null) => void; timer: NodeJS.Timeout }>(); private apiKeyListResolvers = new Map; topicScopes: string[] | null; createdAt: string; lastUsedAt: string | null; revokedAt: string | null; expiresAt: string | null }>) => void; timer: NodeJS.Timeout }>(); + private apiKeyRevokeResolvers = new Map void; timer: NodeJS.Timeout }>(); /** Directories from which this peer serves files. Default: [process.cwd()]. */ private sharedDirs: string[] = [process.cwd()]; private _serviceCatalog: Array<{ name: string; description: string; status: string; tools: Array<{ name: string; description: string; inputSchema: object }>; deployed_by: string }> = []; @@ -699,9 +700,22 @@ export class BrokerClient { }); } - async apiKeyRevoke(id: string): Promise { - if (!this.ws || this.ws.readyState !== this.ws.OPEN) return; - this.ws.send(JSON.stringify({ type: "apikey_revoke", id })); + async apiKeyRevoke(id: string): Promise<{ ok: true; id: string } | { ok: false; code: string; message: string }> { + if (!this.ws || this.ws.readyState !== this.ws.OPEN) { + return { ok: false, code: "not_connected", message: "broker not connected" }; + } + return new Promise((resolve) => { + const reqId = this.makeReqId(); + this.apiKeyRevokeResolvers.set(reqId, { + resolve, + timer: setTimeout(() => { + if (this.apiKeyRevokeResolvers.delete(reqId)) { + resolve({ ok: false, code: "timeout", message: "broker did not respond within 5s" }); + } + }, 5_000), + }); + this.ws!.send(JSON.stringify({ type: "apikey_revoke", id, _reqId: reqId })); + }); } // --- State --- @@ -1909,6 +1923,28 @@ export class BrokerClient { this.resolveFromMap(this.apiKeyListResolvers, msgReqId, (msg.keys as any[]) ?? []); return; } + if (msg.type === "apikey_revoke_response") { + const status = String(msg.status ?? ""); + if (status === "revoked") { + this.resolveFromMap(this.apiKeyRevokeResolvers, msgReqId, { + ok: true as const, + id: String(msg.id ?? ""), + }); + } else if (status === "not_found") { + this.resolveFromMap(this.apiKeyRevokeResolvers, msgReqId, { + ok: false as const, + code: "not_found", + message: "no api key matches that id in this mesh", + }); + } else if (status === "not_unique") { + this.resolveFromMap(this.apiKeyRevokeResolvers, msgReqId, { + ok: false as const, + code: "not_unique", + message: `prefix matches ${Number(msg.matches ?? 0)} keys; use the full id`, + }); + } + return; + } if (msg.type === "push") { this._statsCounters.messagesIn++; const nonce = String(msg.nonce ?? "");