mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 06:28:59 +02:00
fix(browse): revokeToken deletes ALL tokens for a clientId, not the first Map hit
revokeToken deleted the first Map entry matching the clientId and returned true. After a normal pairing, two entries share one clientId: the spent setup key (kept by exchangeSetupKey for idempotent re-exchange) and the session token, in that insertion order. Revoke ate the setup key, reported success, and the live session survived: DELETE /token/<id> returned a false 200 while /agents kept listing the agent. Worse, an unspent setup key created after the session survived revoke, so a "revoked" agent could POST /connect and mint a fresh session within the key's 5-minute validity window. revokeToken now deletes every matching entry and returns the delete count (truthy-compatible with the old boolean). The DELETE /token handler logs "Revoked N token(s)" and returns tokens_deleted so the multi-token class stays visible; revokeSkillToken wraps Boolean() to keep its documented contract. Regression tests pin shapes a (spent-key shadowing), b (re-grant hole), c (multiple pending keys), and bystander isolation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
9da6692930
commit
b9eb108f16
@@ -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' },
|
||||
});
|
||||
}
|
||||
|
||||
@@ -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)));
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user