diff --git a/browse/src/server.ts b/browse/src/server.ts index f42af1a45..7efaf13c0 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -2337,8 +2337,8 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { status: 404, headers: { 'Content-Type': 'application/json' }, }); } - console.log(`[browse] Revoked token for: ${clientId}`); - return new Response(JSON.stringify({ revoked: clientId }), { + console.log(`[browse] Revoked ${revoked} token(s) for: ${clientId}`); + return new Response(JSON.stringify({ revoked: clientId, tokens_deleted: revoked }), { status: 200, headers: { 'Content-Type': 'application/json' }, }); } diff --git a/browse/src/skill-token.ts b/browse/src/skill-token.ts index e58f2f619..4cf876081 100644 --- a/browse/src/skill-token.ts +++ b/browse/src/skill-token.ts @@ -87,5 +87,5 @@ export function mintSkillToken(opts: MintSkillTokenOptions): TokenInfo { * token returns false but is not an error. */ export function revokeSkillToken(skillName: string, spawnId: string): boolean { - return revokeToken(skillClientId(skillName, spawnId)); + return Boolean(revokeToken(skillClientId(skillName, spawnId))); } diff --git a/browse/src/token-registry.ts b/browse/src/token-registry.ts index 161b26b6d..439a11a3e 100644 --- a/browse/src/token-registry.ts +++ b/browse/src/token-registry.ts @@ -417,17 +417,23 @@ export function recordCommand(token: string): void { } /** - * Revoke a token by client ID. Returns true if found and revoked. + * Revoke ALL tokens for a client ID — the session token and every setup key, + * spent or unspent. Deleting only the first match left two holes: a spent + * setup key (kept for idempotent re-exchange) shadowed the session token, so + * revoke reported success while the live session survived; and an unspent + * setup key surviving revoke let a "revoked" agent POST /connect into a + * fresh session. Returns the number of tokens deleted (0 = nothing found). */ -export function revokeToken(clientId: string): boolean { +export function revokeToken(clientId: string): number { + let deleted = 0; for (const [token, info] of tokens) { if (info.clientId === clientId) { - tokens.delete(token); - rateBuckets.delete(clientId); - return true; + tokens.delete(token); // Map tolerates delete during for...of iteration + deleted++; } } - return false; + if (deleted > 0) rateBuckets.delete(clientId); + return deleted; } /** diff --git a/browse/test/token-registry.test.ts b/browse/test/token-registry.test.ts index 0435aa335..9df4f5d83 100644 --- a/browse/test/token-registry.test.ts +++ b/browse/test/token-registry.test.ts @@ -298,12 +298,52 @@ describe('token-registry', () => { describe('revokeToken', () => { it('revokes existing token', () => { const info = createToken({ clientId: 'to-revoke' }); - expect(revokeToken('to-revoke')).toBe(true); + // revokeToken returns the delete count, not a boolean (truthy for callers) + expect(revokeToken('to-revoke')).toBe(1); expect(validateToken(info.token)).toBeNull(); }); - it('returns false for non-existent client', () => { - expect(revokeToken('no-such-client')).toBe(false); + it('returns 0 for non-existent client', () => { + expect(revokeToken('no-such-client')).toBe(0); + }); + + // Regression: revokeToken deleted only the FIRST matching Map entry. The + // spent setup key (kept for idempotent re-exchange) is inserted before the + // session token, so it shadowed the session: revoke reported success while + // the live session survived and DELETE /token returned a false 200. + it('revokes the session even when a spent setup key precedes it (shape a)', () => { + const setup = createSetupKey({ clientId: 'shadowed' }); + const session = exchangeSetupKey(setup.token)!; + expect(revokeToken('shadowed')).toBe(2); + expect(validateToken(session.token)).toBeNull(); + expect(exchangeSetupKey(setup.token)).toBeNull(); + }); + + // Regression: an UNSPENT setup key created after the session survived the + // old first-match revoke, so a "revoked" agent could POST /connect and + // mint a brand-new session within the key's 5-minute validity window. + it('closes the re-grant hole: unspent setup key dies with the revoke (shape b)', () => { + const first = createSetupKey({ clientId: 'regrant' }); + exchangeSetupKey(first.token); + const second = createSetupKey({ clientId: 'regrant' }); + expect(revokeToken('regrant')).toBe(3); + expect(exchangeSetupKey(second.token)).toBeNull(); + expect(listTokens().filter(t => t.clientId === 'regrant')).toHaveLength(0); + }); + + it('revokes multiple pending setup keys for one clientId in a single call (shape c)', () => { + const keys = [1, 2, 3].map(() => createSetupKey({ clientId: 'multi' })); + expect(revokeToken('multi')).toBe(3); + for (const k of keys) expect(exchangeSetupKey(k.token)).toBeNull(); + expect(revokeToken('multi')).toBe(0); // idempotent: second call finds nothing + }); + + it('does not touch other clients\' tokens', () => { + const bystander = createToken({ clientId: 'bystander' }); + createSetupKey({ clientId: 'target' }); + createToken({ clientId: 'target' }); + expect(revokeToken('target')).toBe(2); + expect(validateToken(bystander.token)).not.toBeNull(); }); });