* fix(mcp): grace window + idempotent refresh-token replay for OAuth (#710) OAuth refresh rotated BOTH the refresh token and the access key in one zero-grace CAS. Claude Code's MCP OAuth client fails to persist the rotated refresh token (or fires concurrent refreshes), re-presents the stale one, the CAS matches 0 rows, and the grant dies with invalid_grant — forcing a full re-authorization roughly every 60s in a loop. Regression from #392. Keep rotation (RFC 9700 §4.14.2 requires it for public clients) but add a bounded grace window with idempotent replay, atomic in one SECURITY DEFINER RPC: - Migration adds previous_key_hash / previous_refresh_token_hash (+ *_expires_at) shadow columns. validate_and_increment_api_key accepts the current OR an unexpired previous key_hash, with the rate-limit increment keyed off the resolved row id. - New rotate_mcp_refresh_token RPC: rotated | replayed | reuse_revoked | revoked | invalid. In-grace replay re-issues a fresh pair and slides the window so an actively-refreshing client that cannot persist the rotated token keeps working; reuse after the window revokes the grant family (RFC 9700 4.14.2 reuse detection preserved). - The refresh grant now calls the one RPC, closing the old SELECT-then-CAS TOCTOU gap. All previous_* columns default NULL, so existing keys are unaffected and the RPC return shape is unchanged (callers untouched). Tests: rewired the token-route unit tests to the RPC and replaced the test that codified the bug with a #710 regression (in-grace replay returns 200, not 400); added tests/pg/mcp-oauth-rotation-grace.pg.test.ts (grace accept/expire, revoke-never-graced, rotate->demote, idempotent replay, reuse-after-grace->revoke). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: retrigger checks for #714 No code change — re-running CI. The Supabase Preview check fails on a pre-existing main-branch migration-history drift ("Remote migration versions not found in local migrations directory"), not this PR; pg-real (full migration replay) passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
b7f60b23f5
commit
5078b4e02d
@@ -147,12 +147,10 @@ describe('POST /api/mcp-oauth/token', () => {
|
||||
it('rotates both tokens and returns a fresh access_token', async () => {
|
||||
const { token: refreshToken } = generateRefreshToken()
|
||||
|
||||
const { supabase, enqueueMany } = createQueuedMockSupabase()
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueueMany([
|
||||
{ data: { id: 'key-1', revoked_at: null }, error: null }, // SELECT
|
||||
{ data: [{ id: 'key-1' }], error: null }, // UPDATE ... RETURNING
|
||||
])
|
||||
// rotate_mcp_refresh_token RPC → normal rotation
|
||||
enqueue({ data: [{ outcome: 'rotated', scopes: null }], error: null })
|
||||
|
||||
const res = await POST(
|
||||
formRequest({
|
||||
@@ -179,7 +177,7 @@ describe('POST /api/mcp-oauth/token', () => {
|
||||
it('returns 400 when refresh_token is unknown', async () => {
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueue({ data: null, error: null }) // SELECT — no row
|
||||
enqueue({ data: [{ outcome: 'invalid', scopes: null }], error: null })
|
||||
|
||||
const res = await POST(
|
||||
formRequest({
|
||||
@@ -195,10 +193,7 @@ describe('POST /api/mcp-oauth/token', () => {
|
||||
it('returns 400 when the api_key is revoked', async () => {
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueue({
|
||||
data: { id: 'key-1', revoked_at: '2026-05-01T00:00:00Z' },
|
||||
error: null,
|
||||
})
|
||||
enqueue({ data: [{ outcome: 'revoked', scopes: null }], error: null })
|
||||
|
||||
const res = await POST(
|
||||
formRequest({
|
||||
@@ -212,7 +207,7 @@ describe('POST /api/mcp-oauth/token', () => {
|
||||
expect(body.error_description).toContain('revoked')
|
||||
})
|
||||
|
||||
it('returns 500 when the lookup fails with a DB error', async () => {
|
||||
it('returns 500 when the rotation RPC fails with a DB error', async () => {
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueue({ data: null, error: { message: 'connection reset' } })
|
||||
@@ -228,32 +223,12 @@ describe('POST /api/mcp-oauth/token', () => {
|
||||
expect(body.error).toBe('server_error')
|
||||
})
|
||||
|
||||
it('returns 500 when the rotation update fails with a DB error', async () => {
|
||||
const { supabase, enqueueMany } = createQueuedMockSupabase()
|
||||
it('returns 400 invalid_grant when a refresh token is reused after its grace window (reuse_revoked)', async () => {
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueueMany([
|
||||
{ data: { id: 'key-1', revoked_at: null }, error: null }, // SELECT
|
||||
{ data: null, error: { message: 'deadlock detected' } }, // UPDATE — DB error
|
||||
])
|
||||
|
||||
const res = await POST(
|
||||
formRequest({
|
||||
grant_type: 'refresh_token',
|
||||
refresh_token: 'gnubok_rt_anything',
|
||||
})
|
||||
)
|
||||
expect(res.status).toBe(500)
|
||||
const body = await res.json()
|
||||
expect(body.error).toBe('server_error')
|
||||
})
|
||||
|
||||
it('returns 400 when the CAS update affects 0 rows (concurrent reuse)', async () => {
|
||||
const { supabase, enqueueMany } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueueMany([
|
||||
{ data: { id: 'key-1', revoked_at: null }, error: null }, // SELECT
|
||||
{ data: [], error: null }, // UPDATE — 0 rows (lost the CAS race)
|
||||
])
|
||||
// The RPC detected reuse of a previous refresh token past its grace
|
||||
// window and already revoked the grant family (RFC 9700 §4.14.2).
|
||||
enqueue({ data: [{ outcome: 'reuse_revoked', scopes: null }], error: null })
|
||||
|
||||
const res = await POST(
|
||||
formRequest({
|
||||
@@ -264,7 +239,29 @@ describe('POST /api/mcp-oauth/token', () => {
|
||||
expect(res.status).toBe(400)
|
||||
const body = await res.json()
|
||||
expect(body.error).toBe('invalid_grant')
|
||||
expect(body.error_description).toContain('already used')
|
||||
})
|
||||
|
||||
it('returns a fresh pair on idempotent in-grace replay instead of 400 (issue #710 regression)', async () => {
|
||||
// A retried / mis-persisted / concurrent refresh presents the previous
|
||||
// refresh token within the grace window. The old code returned 400
|
||||
// "already used", stranding Claude Code into a re-auth loop; the RPC now
|
||||
// replays idempotently and the client gets a working pair.
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueue({ data: [{ outcome: 'replayed', scopes: ['transactions:read'] }], error: null })
|
||||
|
||||
const res = await POST(
|
||||
formRequest({
|
||||
grant_type: 'refresh_token',
|
||||
refresh_token: 'gnubok_rt_anything',
|
||||
})
|
||||
)
|
||||
expect(res.status).toBe(200)
|
||||
const body = await res.json()
|
||||
expect(body.access_token).toMatch(/^gnubok_sk_/)
|
||||
expect(body.refresh_token).toMatch(/^gnubok_rt_/)
|
||||
expect(body.expires_in).toBe(3600)
|
||||
expect(body.scope).toBe('transactions:read')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -387,19 +384,17 @@ describe('POST /api/mcp-oauth/token', () => {
|
||||
it('returns the granular scopes the api_key was minted with', async () => {
|
||||
// Greptile P1 — refresh response previously hardcoded scope:'mcp',
|
||||
// causing OAuth 2.1 clients to think they had lost their grant.
|
||||
const { supabase, enqueueMany } = createQueuedMockSupabase()
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueueMany([
|
||||
{
|
||||
data: {
|
||||
id: 'key-1',
|
||||
revoked_at: null,
|
||||
enqueue({
|
||||
data: [
|
||||
{
|
||||
outcome: 'rotated',
|
||||
scopes: ['transactions:read', 'invoices:read', 'invoices:write'],
|
||||
},
|
||||
error: null,
|
||||
}, // SELECT
|
||||
{ data: [{ id: 'key-1' }], error: null }, // UPDATE
|
||||
])
|
||||
],
|
||||
error: null,
|
||||
})
|
||||
|
||||
const res = await POST(
|
||||
formRequest({
|
||||
@@ -415,12 +410,9 @@ describe('POST /api/mcp-oauth/token', () => {
|
||||
})
|
||||
|
||||
it('falls back to read-only DEFAULT_OAUTH_SCOPES for legacy keys with null scopes', async () => {
|
||||
const { supabase, enqueueMany } = createQueuedMockSupabase()
|
||||
const { supabase, enqueue } = createQueuedMockSupabase()
|
||||
mocks.supabaseFactory.mockReturnValue(supabase)
|
||||
enqueueMany([
|
||||
{ data: { id: 'key-1', revoked_at: null, scopes: null }, error: null },
|
||||
{ data: [{ id: 'key-1' }], error: null },
|
||||
])
|
||||
enqueue({ data: [{ outcome: 'rotated', scopes: null }], error: null })
|
||||
|
||||
const res = await POST(
|
||||
formRequest({
|
||||
|
||||
@@ -13,6 +13,13 @@ import { requireCompanyId } from '@/lib/company/context'
|
||||
|
||||
const ACCESS_TOKEN_TTL_SECONDS = 3600
|
||||
|
||||
// Grace window (seconds) during which a just-superseded access token and refresh
|
||||
// token stay valid after a rotation. Lets a client that cannot reliably persist
|
||||
// the rotated refresh token — or that fires concurrent refreshes — recover via
|
||||
// idempotent replay instead of being forced into a re-auth loop (issue #710).
|
||||
// Reuse of a previous refresh token AFTER this window revokes the grant.
|
||||
const REFRESH_GRACE_SECONDS = 120
|
||||
|
||||
/**
|
||||
* OAuth 2.0 Token Endpoint.
|
||||
*
|
||||
@@ -184,75 +191,54 @@ async function handleRefreshTokenGrant(params: URLSearchParams) {
|
||||
const supabase = createServiceClientNoCookies()
|
||||
const presentedHash = hashRefreshToken(refreshToken)
|
||||
|
||||
// Look up the api_key row by refresh_token_hash. The hash is unique among
|
||||
// non-null values, so there's at most one match. We pull `scopes` so the
|
||||
// rotated token response advertises the same granular grant the key
|
||||
// already carries (otherwise OAuth 2.1 clients would re-authorize on
|
||||
// every refresh).
|
||||
const { data: row, error: lookupError } = await supabase
|
||||
.from('api_keys')
|
||||
.select('id, revoked_at, scopes')
|
||||
.eq('refresh_token_hash', presentedHash)
|
||||
.maybeSingle()
|
||||
|
||||
if (lookupError) {
|
||||
return NextResponse.json(
|
||||
{ error: 'server_error', error_description: 'Failed to look up refresh token' },
|
||||
{ status: 500 }
|
||||
)
|
||||
}
|
||||
|
||||
if (!row) {
|
||||
return NextResponse.json(
|
||||
{ error: 'invalid_grant', error_description: 'Invalid refresh token' },
|
||||
{ status: 400 }
|
||||
)
|
||||
}
|
||||
|
||||
if (row.revoked_at) {
|
||||
return NextResponse.json(
|
||||
{ error: 'invalid_grant', error_description: 'Refresh token revoked' },
|
||||
{ status: 400 }
|
||||
)
|
||||
}
|
||||
|
||||
// Rotate both tokens atomically. OAuth 2.1 §6.1 recommends rotating the
|
||||
// refresh token; we also rotate the api_key because key_hash is one-way
|
||||
// and we cannot recover the original plaintext to return to the client.
|
||||
// The .eq('refresh_token_hash', presentedHash) guard makes this a CAS:
|
||||
// a concurrent refresh with the same token will affect 0 rows.
|
||||
// Pre-generate the candidate credentials; the RPC decides whether to use them
|
||||
// (rotate / idempotent replay) or ignore them (reuse / revoked / invalid).
|
||||
// Doing lookup + rotate + demote in ONE SECURITY DEFINER RPC closes the
|
||||
// TOCTOU gap the old SELECT-then-CAS had, and lets a just-superseded refresh
|
||||
// token stay valid for a grace window so a client that cannot persist the
|
||||
// rotated token — or fires concurrent refreshes — recovers instead of being
|
||||
// forced into a re-auth loop (issue #710). Reuse AFTER the grace window
|
||||
// revokes the grant (RFC 9700 §4.14.2 reuse detection).
|
||||
const rotated = generateRefreshToken()
|
||||
const { key: newKey, hash: newKeyHash, prefix: newKeyPrefix } = generateApiKey()
|
||||
|
||||
const { data: updated, error: updateError } = await supabase
|
||||
.from('api_keys')
|
||||
.update({
|
||||
refresh_token_hash: rotated.hash,
|
||||
key_hash: newKeyHash,
|
||||
key_prefix: newKeyPrefix,
|
||||
})
|
||||
.eq('id', row.id)
|
||||
.eq('refresh_token_hash', presentedHash)
|
||||
.select('id')
|
||||
const { data, error } = await supabase.rpc('rotate_mcp_refresh_token', {
|
||||
p_presented_hash: presentedHash,
|
||||
p_new_refresh_hash: rotated.hash,
|
||||
p_new_key_hash: newKeyHash,
|
||||
p_new_key_prefix: newKeyPrefix,
|
||||
p_grace_seconds: REFRESH_GRACE_SECONDS,
|
||||
})
|
||||
|
||||
if (updateError) {
|
||||
if (error) {
|
||||
return NextResponse.json(
|
||||
{ error: 'server_error', error_description: 'Failed to rotate refresh token' },
|
||||
{ status: 500 }
|
||||
)
|
||||
}
|
||||
|
||||
if (!updated || updated.length === 0) {
|
||||
const result = (Array.isArray(data) ? data[0] : data) as
|
||||
| { outcome: string; scopes: string[] | null }
|
||||
| undefined
|
||||
const outcome = result?.outcome
|
||||
|
||||
// 'rotated' (normal) and 'replayed' (idempotent in-grace retry/concurrent)
|
||||
// both succeed and return the freshly minted pair. Everything else maps to
|
||||
// invalid_grant; for 'reuse_revoked' the grant was already revoked in the RPC.
|
||||
if (outcome !== 'rotated' && outcome !== 'replayed') {
|
||||
return NextResponse.json(
|
||||
{ error: 'invalid_grant', error_description: 'Refresh token already used' },
|
||||
{
|
||||
error: 'invalid_grant',
|
||||
error_description:
|
||||
outcome === 'revoked' ? 'Refresh token revoked' : 'Invalid or expired refresh token',
|
||||
},
|
||||
{ status: 400 }
|
||||
)
|
||||
}
|
||||
|
||||
// Return the granular scopes the key was originally minted with. Falling
|
||||
// back to the read-only OAuth defaults preserves the pre-scope-plumbing
|
||||
// behaviour for legacy keys whose scopes column is null.
|
||||
const persistedScopes = validateScopes(row.scopes) ?? DEFAULT_OAUTH_SCOPES
|
||||
// Carry the granular scopes the key was minted with (read-only OAuth defaults
|
||||
// for legacy keys with null scopes) so clients don't re-authorize on refresh.
|
||||
const persistedScopes = validateScopes(result?.scopes ?? null) ?? DEFAULT_OAUTH_SCOPES
|
||||
|
||||
return NextResponse.json({
|
||||
access_token: newKey,
|
||||
|
||||
Reference in New Issue
Block a user