From 56950cf0f60f72f56b4237844f173e90778ce43e Mon Sep 17 00:00:00 2001 From: Minxi Hou Date: Fri, 2 Oct 2026 23:36:07 -0400 Subject: [PATCH] fix(auth): revoke one access token by prefix and stop rewriting last use Revoking by a display prefix updated every live token that shared it, while the route reported a single success. A prefix that matches more than one live token now revokes none and the route returns 409. An exact id still revokes that one token, and revoking an already-revoked token returns success so a retry after a lost response does not 404. Every successful verification also rewrote last_used_at, and one request verifies the same token more than once. The stamp is now reused for 60 seconds, so a repeat check inside that window does not write. Signed-off-by: Minxi Hou --- docs/openapi.yaml | 6 +- public/openapi.yaml | 6 +- src/app/api/cli/tokens/[id]/route.ts | 25 +++--- src/lib/db/accessTokens.ts | 82 +++++++++++++---- tests/unit/access-tokens-db.test.ts | 102 +++++++++++++++++++++- tests/unit/cli-token-delete-route.test.ts | 75 ++++++++++++++++ 6 files changed, 261 insertions(+), 35 deletions(-) create mode 100644 tests/unit/cli-token-delete-route.test.ts diff --git a/docs/openapi.yaml b/docs/openapi.yaml index ed4fafcb442d..5a445171d9ac 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -4037,11 +4037,13 @@ paths: schema: { type: string } responses: "200": - description: Token revoked + description: Token revoked (alreadyRevoked is true when it was already revoked) "403": description: Insufficient scope "404": - description: Token not found or already revoked + description: Token not found + "409": + description: Prefix matches more than one live token; nothing revoked # ─── CLI Tools ───────────────────────────────────────────────── diff --git a/public/openapi.yaml b/public/openapi.yaml index 7a9f771cb298..a17d5602325f 100644 --- a/public/openapi.yaml +++ b/public/openapi.yaml @@ -2812,11 +2812,13 @@ paths: schema: { type: string } responses: "200": - description: Token revoked + description: Token revoked (alreadyRevoked is true when it was already revoked) "403": description: Insufficient scope "404": - description: Token not found or already revoked + description: Token not found + "409": + description: Prefix matches more than one live token; nothing revoked # ─── CLI Tools ───────────────────────────────────────────────── diff --git a/src/app/api/cli/tokens/[id]/route.ts b/src/app/api/cli/tokens/[id]/route.ts index b227a7a4b555..c8129a3ee8fb 100644 --- a/src/app/api/cli/tokens/[id]/route.ts +++ b/src/app/api/cli/tokens/[id]/route.ts @@ -4,20 +4,25 @@ import { revokeAccessToken } from "@/lib/db/accessTokens"; /** * DELETE /api/cli/tokens/:id — revoke an access token (by id or display prefix). - * Admin-only (same enforcement as the collection route). Idempotent: revoking - * an unknown/already-revoked token returns 404. + * Admin-only (same enforcement as the collection route). Revoking an + * already-revoked token returns 200 with alreadyRevoked so a retry after a + * lost response succeeds. An unknown token is 404. A prefix that matches more + * than one live token revokes nothing and returns 409. */ -export async function DELETE( - request: Request, - { params }: { params: Promise<{ id: string }> } -) { +export async function DELETE(request: Request, { params }: { params: Promise<{ id: string }> }) { const authError = await requireManagementAuth(request); if (authError) return authError; const { id } = await params; - const revoked = revokeAccessToken(id); - if (!revoked) { - return NextResponse.json({ error: "Token not found or already revoked" }, { status: 404 }); + const result = revokeAccessToken(id); + if (result.ambiguous) { + return NextResponse.json( + { error: "Token prefix matches more than one token" }, + { status: 409 } + ); } - return NextResponse.json({ success: true, id }); + if (!result.revoked) { + return NextResponse.json({ error: "Token not found" }, { status: 404 }); + } + return NextResponse.json({ success: true, id, alreadyRevoked: result.alreadyRevoked }); } diff --git a/src/lib/db/accessTokens.ts b/src/lib/db/accessTokens.ts index 6fbf4e977bb1..a8512003da1b 100644 --- a/src/lib/db/accessTokens.ts +++ b/src/lib/db/accessTokens.ts @@ -16,6 +16,9 @@ const TOKEN_SECRET_PREFIX = "oma_live_"; /** How many leading chars of the secret are kept for display (prefix). */ const DISPLAY_PREFIX_LEN = TOKEN_SECRET_PREFIX.length + 6; +/** How long a successful verify may reuse the stored last_used_at stamp. */ +const LAST_USED_WRITE_INTERVAL_MS = 60_000; + export interface AccessTokenRecord { id: string; name: string; @@ -34,6 +37,15 @@ export interface VerifiedAccessToken { scope: AccessScope; } +/** Outcome of a revoke. A retry of an already-revoked token is still a success. */ +export interface RevokeAccessTokenResult { + /** True when the token is revoked after this call, including a prior revoke. */ + revoked: boolean; + alreadyRevoked: boolean; + /** True when a prefix matched more than one live token and nothing was revoked. */ + ambiguous: boolean; +} + interface AccessTokenRow { id: string; token_hash: string; @@ -135,14 +147,20 @@ export function verifyAccessToken(secret: string | null | undefined): VerifiedAc if (row.revoked_at) return null; if (isExpired(row.expires_at)) return null; - // Best-effort usage stamp; never block validation on the write. - try { - db.prepare("UPDATE cli_access_tokens SET last_used_at = ? WHERE id = ?").run( - new Date().toISOString(), - row.id - ); - } catch { - /* non-fatal */ + // A management request verifies the same token at the policy and the route. + // Skip the write while the stored stamp is still inside the window. + const lastUsedMs = row.last_used_at ? Date.parse(row.last_used_at) : Number.NaN; + const stampIsFresh = + Number.isFinite(lastUsedMs) && Date.now() - lastUsedMs < LAST_USED_WRITE_INTERVAL_MS; + if (!stampIsFresh) { + try { + db.prepare("UPDATE cli_access_tokens SET last_used_at = ? WHERE id = ?").run( + new Date().toISOString(), + row.id + ); + } catch { + /* non-fatal */ + } } return { id: row.id, name: row.name, scope: normalizeScope(row.scope) }; @@ -161,23 +179,53 @@ export function listAccessTokens(): AccessTokenRecord[] { export function getAccessToken(id: string): AccessTokenRecord | null { const db = getDbInstance(); const row = db.prepare("SELECT * FROM cli_access_tokens WHERE id = ?").get(id) as - | AccessTokenRow - | undefined; + AccessTokenRow | undefined; return row ? rowToRecord(row) : null; } /** - * Revoke a token by id or by its display prefix. Idempotent: revoking an - * already-revoked token is a no-op. Returns true when a row was newly revoked. + * Revoke a token by id, or by a display prefix that matches exactly one live + * token. A prefix shared by two or more live tokens revokes nothing. + * Revoking an already-revoked token succeeds and reports alreadyRevoked. */ -export function revokeAccessToken(idOrPrefix: string): boolean { - if (!idOrPrefix) return false; +export function revokeAccessToken(idOrPrefix: string): RevokeAccessTokenResult { + const none = { revoked: false, alreadyRevoked: false, ambiguous: false }; + if (!idOrPrefix) return none; const db = getDbInstance(); + + const matches = db + .prepare( + `SELECT id, revoked_at FROM cli_access_tokens + WHERE id = ? OR token_prefix = ?` + ) + .all(idOrPrefix, idOrPrefix) as Array<{ id: string; revoked_at: string | null }>; + if (matches.length === 0) return none; + + const exact = matches.find((row) => row.id === idOrPrefix); + // Revoked siblings do not make the one remaining live prefix ambiguous. + const live = matches.filter((row) => row.revoked_at == null); + let target = exact ?? null; + if (!target) { + if (live.length > 1) { + return { revoked: false, alreadyRevoked: false, ambiguous: true }; + } + if (live.length === 1) target = live[0]; + else if (matches.length === 1) target = matches[0]; + else return none; + } + if (target.revoked_at) { + return { revoked: true, alreadyRevoked: true, ambiguous: false }; + } + const res = db .prepare( `UPDATE cli_access_tokens SET revoked_at = ? - WHERE (id = ? OR token_prefix = ?) AND revoked_at IS NULL` + WHERE id = ? AND revoked_at IS NULL` ) - .run(new Date().toISOString(), idOrPrefix, idOrPrefix); - return res.changes > 0; + .run(new Date().toISOString(), target.id); + if (res.changes > 0) { + return { revoked: true, alreadyRevoked: false, ambiguous: false }; + } + // Lost the race to another revoke of this same id. + return { revoked: true, alreadyRevoked: true, ambiguous: false }; } diff --git a/tests/unit/access-tokens-db.test.ts b/tests/unit/access-tokens-db.test.ts index d0d6c4c89005..fd42623ad52e 100644 --- a/tests/unit/access-tokens-db.test.ts +++ b/tests/unit/access-tokens-db.test.ts @@ -70,21 +70,115 @@ test("verifyAccessToken stamps last_used_at", () => { assert.notEqual(at.getAccessToken(record.id)?.lastUsedAt, null); }); +test("verifyAccessToken writes last_used_at once per window", () => { + const { secret, record } = at.createAccessToken({ name: "throttle", scope: "read" }); + const db = core.getDbInstance(); + let writes = 0; + const originalPrepare = db.prepare.bind(db); + db.prepare = ((sql: string) => { + const statement = originalPrepare(sql); + if (/UPDATE\s+cli_access_tokens\s+SET\s+last_used_at/i.test(sql)) { + const originalRun = statement.run.bind(statement); + statement.run = ((...args: unknown[]) => { + writes += 1; + return originalRun(...args); + }) as typeof statement.run; + } + return statement; + }) as typeof db.prepare; + + try { + assert.ok(at.verifyAccessToken(secret)); + assert.ok(at.verifyAccessToken(secret)); + assert.equal(writes, 1, "a repeat verify inside the window must not write again"); + assert.notEqual(at.getAccessToken(record.id)?.lastUsedAt, null); + + const stale = new Date(Date.now() - 61_000).toISOString(); + originalPrepare("UPDATE cli_access_tokens SET last_used_at = ? WHERE id = ?").run( + stale, + record.id + ); + assert.ok(at.verifyAccessToken(secret)); + assert.equal(writes, 2, "a stamp older than the window must be refreshed"); + assert.notEqual(at.getAccessToken(record.id)?.lastUsedAt, stale); + } finally { + db.prepare = originalPrepare; + } +}); + test("revoked tokens fail verification", () => { const { secret, record } = at.createAccessToken({ name: "to-revoke", scope: "write" }); assert.ok(at.verifyAccessToken(secret)); - assert.equal(at.revokeAccessToken(record.id), true); + const first = at.revokeAccessToken(record.id); + assert.equal(first.revoked, true); + assert.equal(first.alreadyRevoked, false); assert.equal(at.verifyAccessToken(secret), null); - // idempotent: revoking again is a no-op - assert.equal(at.revokeAccessToken(record.id), false); + // A retry after a lost response must not look like a miss. + const second = at.revokeAccessToken(record.id); + assert.equal(second.revoked, true); + assert.equal(second.alreadyRevoked, true); + assert.notEqual(at.getAccessToken(record.id)?.revokedAt, null); }); test("revokeAccessToken works by display prefix too", () => { const { secret, record } = at.createAccessToken({ name: "by-prefix", scope: "read" }); - assert.equal(at.revokeAccessToken(record.tokenPrefix), true); + const result = at.revokeAccessToken(record.tokenPrefix); + assert.equal(result.revoked, true); + assert.equal(result.alreadyRevoked, false); assert.equal(at.verifyAccessToken(secret), null); }); +test("revoke by a shared prefix refuses instead of revoking every match", () => { + const first = at.createAccessToken({ name: "shared-a", scope: "read" }); + const second = at.createAccessToken({ name: "shared-b", scope: "write" }); + const db = core.getDbInstance(); + db.prepare("UPDATE cli_access_tokens SET token_prefix = ? WHERE id = ?").run( + first.record.tokenPrefix, + second.record.id + ); + + const result = at.revokeAccessToken(first.record.tokenPrefix); + assert.equal(result.revoked, false); + assert.equal(result.ambiguous, true); + assert.equal(at.getAccessToken(first.record.id)?.revokedAt, null); + assert.equal(at.getAccessToken(second.record.id)?.revokedAt, null); + // The id is unique, so the same collision does not block an id revoke. + assert.equal(at.revokeAccessToken(first.record.id).revoked, true); + assert.equal(at.getAccessToken(second.record.id)?.revokedAt, null); +}); + +test("revoke by prefix ignores already-revoked siblings", () => { + const live = at.createAccessToken({ name: "live-sibling", scope: "read" }); + const dead = at.createAccessToken({ name: "dead-sibling", scope: "write" }); + const db = core.getDbInstance(); + db.prepare("UPDATE cli_access_tokens SET token_prefix = ?, revoked_at = ? WHERE id = ?").run( + live.record.tokenPrefix, + new Date().toISOString(), + dead.record.id + ); + + const result = at.revokeAccessToken(live.record.tokenPrefix); + assert.equal(result.revoked, true); + assert.equal(result.ambiguous, false); + assert.notEqual(at.getAccessToken(live.record.id)?.revokedAt, null); +}); + +test("revoke by prefix of an already-revoked unique prefix reports alreadyRevoked", () => { + const { record } = at.createAccessToken({ name: "prefix-retry", scope: "read" }); + assert.equal(at.revokeAccessToken(record.tokenPrefix).alreadyRevoked, false); + const retry = at.revokeAccessToken(record.tokenPrefix); + assert.equal(retry.revoked, true); + assert.equal(retry.alreadyRevoked, true); + assert.equal(retry.ambiguous, false); +}); + +test("revoke of an unknown id is not a success", () => { + const result = at.revokeAccessToken("tok_does_not_exist"); + assert.equal(result.revoked, false); + assert.equal(result.alreadyRevoked, false); + assert.equal(result.ambiguous, false); +}); + test("expired tokens fail verification", () => { const past = new Date(Date.now() - 60_000).toISOString(); const { secret } = at.createAccessToken({ name: "expired", scope: "admin", expiresAt: past }); diff --git a/tests/unit/cli-token-delete-route.test.ts b/tests/unit/cli-token-delete-route.test.ts new file mode 100644 index 000000000000..1a418ed1cc43 --- /dev/null +++ b/tests/unit/cli-token-delete-route.test.ts @@ -0,0 +1,75 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +// DELETE /api/cli/tokens/:id. Auth is forced on so a retry of an already-revoked +// token is the behavior under test, not an open management endpoint. +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-cli-token-delete-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.DISABLE_SQLITE_AUTO_BACKUP = "true"; +process.env.INITIAL_PASSWORD = "test-pass"; + +const core = await import("../../src/lib/db/core.ts"); +const at = await import("../../src/lib/db/accessTokens.ts"); +const { DELETE } = await import("../../src/app/api/cli/tokens/[id]/route.ts"); + +test.after(() => { + try { + core.resetDbInstance(); + } catch {} + try { + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); + } catch {} + delete process.env.INITIAL_PASSWORD; +}); + +function deleteToken(id: string, bearer: string): Promise { + return DELETE( + new Request(`http://localhost:20128/api/cli/tokens/${id}`, { + method: "DELETE", + headers: { authorization: `Bearer ${bearer}` }, + }), + { params: Promise.resolve({ id }) } + ); +} + +test("DELETE of an already-revoked token succeeds so a lost response can be retried", async () => { + const admin = at.createAccessToken({ name: "admin", scope: "admin" }); + const target = at.createAccessToken({ name: "target", scope: "read" }); + + const first = await deleteToken(target.record.id, admin.secret); + assert.equal(first.status, 200); + const firstBody = await first.json(); + assert.equal(firstBody.success, true); + assert.equal(firstBody.alreadyRevoked, false); + + const retry = await deleteToken(target.record.id, admin.secret); + assert.equal(retry.status, 200); + const retryBody = await retry.json(); + assert.equal(retryBody.success, true); + assert.equal(retryBody.alreadyRevoked, true); + assert.equal(retryBody.id, target.record.id); +}); + +test("DELETE of an unknown token is still 404", async () => { + const admin = at.createAccessToken({ name: "admin-miss", scope: "admin" }); + const missing = await deleteToken("tok_missing", admin.secret); + assert.equal(missing.status, 404); +}); + +test("DELETE by a prefix shared by two live tokens refuses both", async () => { + const admin = at.createAccessToken({ name: "admin-amb", scope: "admin" }); + const first = at.createAccessToken({ name: "amb-a", scope: "read" }); + const second = at.createAccessToken({ name: "amb-b", scope: "write" }); + core + .getDbInstance() + .prepare("UPDATE cli_access_tokens SET token_prefix = ? WHERE id = ?") + .run(first.record.tokenPrefix, second.record.id); + + const response = await deleteToken(first.record.tokenPrefix, admin.secret); + assert.equal(response.status, 409); + assert.equal(at.getAccessToken(first.record.id)?.revokedAt, null); + assert.equal(at.getAccessToken(second.record.id)?.revokedAt, null); +});