diff --git a/src/lib/api-client.ts b/src/lib/api-client.ts index dadf424..7c1d375 100644 --- a/src/lib/api-client.ts +++ b/src/lib/api-client.ts @@ -1,5 +1,6 @@ -import type { AtpSessionData } from '@atproto/api'; +import type { AtpPersistSessionHandler, AtpSessionData } from '@atproto/api'; import { AtpAgent } from '@atproto/api'; +import { isAccessTokenValid } from '../utils/jwt.js'; import { resolvePdsFromIdentifier } from '../utils/pds-resolver.js'; import { clearCurrentSessionMetadata, @@ -101,17 +102,38 @@ export class TangledApiClient { return false; } + // The agent rotates the single-use refresh token on every refresh and + // hands us the new tokens through this callback. Persist them so the next + // CLI invocation presents a valid refresh token instead of a consumed one. + // This fires both for the forced refresh below and for any lazy refresh + // the agent performs mid-command when an access token expires. + let pendingSave: Promise | undefined; + const persistSession: AtpPersistSessionHandler = (event, session) => { + if (session && (event === 'update' || event === 'create')) { + pendingSave = saveSession(session); + } + }; + // Recreate agent pointing at the stored PDS so API calls go to the right server - this.agent = new AtpAgent({ service: metadata.pds }); - await this.agent.resumeSession(sessionData); - - // resumeSession always forces a token refresh, which rotates the - // single-use refresh token and replaces agent.session with the new - // tokens. Persist them so the next CLI invocation presents a valid refresh - // token instead of the consumed one (which would force a re-login). - const refreshed = this.agent.session; - if (refreshed) { - await saveSession(refreshed); + this.agent = new AtpAgent({ service: metadata.pds, persistSession }); + + if (isAccessTokenValid(sessionData.accessJwt)) { + // The stored access token is still good. Install it without forcing a + // refresh so the single-use refresh token is left untouched. Forcing a + // refresh on every command rotates that token constantly; if a command + // is interrupted or overlaps another invocation, the stored token gets + // revoked server-side before its replacement is saved, permanently + // breaking auth until the user logs in again. The agent will refresh + // lazily (and persist via the callback) only when the access token + // actually expires mid-request. + this.agent.sessionManager.session = sessionData; + } else { + // Access token is expired or unreadable: refresh now. resumeSession + // forces a refresh, which rotates the refresh token and fires + // persistSession with the new tokens. Await the resulting save so the + // rotated token is durable before this invocation continues. + await this.agent.resumeSession(sessionData); + await pendingSave; } // Update last used timestamp diff --git a/src/utils/jwt.ts b/src/utils/jwt.ts new file mode 100644 index 0000000..bc00854 --- /dev/null +++ b/src/utils/jwt.ts @@ -0,0 +1,41 @@ +/** + * Minimal JWT helpers for inspecting access tokens without verifying them. + * + * We never trust these tokens for authorization decisions; the PDS does that. + * We only read the `exp` claim to decide whether a stored access token is still + * usable, so we can avoid forcing a token refresh (which rotates the single-use + * refresh token) on every CLI invocation. + */ + +/** + * Decode the `exp` (expiry) claim from a JWT, in seconds since the epoch. + * Returns null if the token is malformed or has no numeric `exp`. + */ +export function getJwtExpiry(jwt: string): number | null { + const parts = jwt.split('.'); + if (parts.length !== 3) { + return null; + } + + try { + const payload = JSON.parse(Buffer.from(parts[1], 'base64url').toString('utf-8')); + return typeof payload.exp === 'number' ? payload.exp : null; + } catch { + return null; + } +} + +/** + * Report whether an access token is still valid with a safety margin. + * + * The margin guards against a token that is technically valid now but would + * expire mid-request. A malformed or unparseable token is treated as invalid so + * the caller falls back to a refresh. + */ +export function isAccessTokenValid(jwt: string, marginSeconds = 60): boolean { + const exp = getJwtExpiry(jwt); + if (exp === null) { + return false; + } + return exp - marginSeconds > Date.now() / 1000; +} diff --git a/tests/lib/api-client.test.ts b/tests/lib/api-client.test.ts index 06d8a0b..177f11e 100644 --- a/tests/lib/api-client.test.ts +++ b/tests/lib/api-client.test.ts @@ -19,6 +19,19 @@ const { mockAgent } = vi.hoisted(() => { return agent._session; }, _session: undefined as AtpSessionData | undefined, + // Mirrors the real CredentialSession: the agent exposes sessionManager, and + // assigning sessionManager.session installs a session without any network + // call. Capturing persistSession lets resumeSession's mock simulate the + // library firing the callback on a token rotation. + persistSession: undefined as ((event: string, session?: AtpSessionData) => void) | undefined, + sessionManager: { + get session() { + return agent._session; + }, + set session(value: AtpSessionData | undefined) { + agent._session = value; + }, + }, login: vi.fn(), resumeSession: vi.fn(), }; @@ -26,7 +39,12 @@ const { mockAgent } = vi.hoisted(() => { }); vi.mock('@atproto/api', () => ({ - AtpAgent: vi.fn().mockReturnValue(mockAgent), + AtpAgent: vi.fn( + (opts?: { persistSession?: (event: string, session?: AtpSessionData) => void }) => { + mockAgent.persistSession = opts?.persistSession; + return mockAgent; + } + ), })); vi.mock('../../src/lib/session.js', async (importOriginal) => { @@ -57,8 +75,11 @@ describe('TangledApiClient', () => { mockAgent._session = mockSessionData; return { success: true, data: mockSessionData }; }); + // The real resumeSession forces a refresh that rotates the tokens and fires + // persistSession('update'). Simulate that so the save path is exercised. vi.mocked(mockAgent.resumeSession).mockImplementation(async (session: AtpSessionData) => { mockAgent._session = session; + mockAgent.persistSession?.('update', session); }); vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(null); @@ -164,7 +185,9 @@ describe('TangledApiClient', () => { await client.resumeSession(); - expect(AtpAgent).toHaveBeenCalledWith({ service: 'https://tngl.sh' }); + expect(AtpAgent).toHaveBeenCalledWith( + expect.objectContaining({ service: 'https://tngl.sh' }) + ); }); it('should return false if no metadata exists', async () => { @@ -190,10 +213,12 @@ describe('TangledApiClient', () => { vi.mocked(sessionModule.loadSession).mockResolvedValue(mockSessionData); // Simulate the agent refreshing and rotating its tokens during resume: it // ends up holding a different session than the stale one loaded from the - // keychain. The rotated session must be written back, awaited, so the next - // CLI invocation can refresh again instead of presenting a consumed token. + // keychain and fires persistSession with it. The rotated session must be + // written back, awaited, so the next CLI invocation can refresh again + // instead of presenting a consumed token. vi.mocked(mockAgent.resumeSession).mockImplementationOnce(async () => { mockAgent._session = mockSessionData2; + mockAgent.persistSession?.('update', mockSessionData2); }); await client.resumeSession(); @@ -201,6 +226,30 @@ describe('TangledApiClient', () => { expect(vi.mocked(sessionModule.saveSession)).toHaveBeenCalledWith(mockSessionData2); }); + it('should reuse a still-valid access token without forcing a refresh', async () => { + // A real JWT whose exp is comfortably in the future. resumeSession should + // install it directly and never call the agent's resumeSession (which + // would rotate the single-use refresh token) or write to the keychain. + const futureExp = Math.floor(Date.now() / 1000) + 3600; + const header = Buffer.from(JSON.stringify({ alg: 'none' })).toString('base64url'); + const body = Buffer.from(JSON.stringify({ exp: futureExp })).toString('base64url'); + const liveSession: AtpSessionData = { + ...mockSessionData, + accessJwt: `${header}.${body}.sig`, + }; + vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(mockSessionMetadata); + vi.mocked(sessionModule.loadSession).mockResolvedValue(liveSession); + + const resumed = await client.resumeSession(); + + expect(resumed).toBe(true); + expect(vi.mocked(mockAgent.resumeSession)).not.toHaveBeenCalled(); + expect(vi.mocked(sessionModule.saveSession)).not.toHaveBeenCalled(); + expect(mockAgent.session).toEqual(liveSession); + // Last-used metadata is still refreshed so status reflects recent activity. + expect(vi.mocked(sessionModule.saveCurrentSessionMetadata)).toHaveBeenCalled(); + }); + it('should return false without clearing metadata on transient resume error', async () => { vi.mocked(sessionModule.getCurrentSessionMetadata).mockResolvedValue(mockSessionMetadata); vi.mocked(sessionModule.loadSession).mockResolvedValue(mockSessionData); diff --git a/tests/lib/auth-token-rotation.test.ts b/tests/lib/auth-token-rotation.test.ts index 578a00c..ae4026c 100644 --- a/tests/lib/auth-token-rotation.test.ts +++ b/tests/lib/auth-token-rotation.test.ts @@ -48,6 +48,14 @@ function jsonResponse(body: unknown): Response { }); } +/** Build a JWT whose exp claim is `secondsFromNow` in the future. */ +function jwtExpiringIn(secondsFromNow: number): string { + const header = Buffer.from(JSON.stringify({ alg: 'none' })).toString('base64url'); + const exp = Math.floor(Date.now() / 1000) + secondsFromNow; + const body = Buffer.from(JSON.stringify({ exp })).toString('base64url'); + return `${header}.${body}.sig`; +} + describe('refresh-token rotation persistence (integration)', () => { const originalFetch = globalThis.fetch; let fetchMock: ReturnType; @@ -118,4 +126,22 @@ describe('refresh-token rotation persistence (integration)', () => { expect(persisted.refreshJwt).toBe('refresh-token-2'); expect(persisted.accessJwt).toBe('access-token-2'); }); + + it('does not touch the refresh token when the access token is still valid', async () => { + // A still-valid access token must be reused as-is. Refreshing here would + // rotate the single-use refresh token needlessly, which is the fragility + // that breaks auth when commands are interrupted or run concurrently. + vi.mocked(sessionModule.loadSession).mockResolvedValue({ + ...storedSession, + accessJwt: jwtExpiringIn(3600), + }); + + const client = new TangledApiClient(); + const resumed = await client.resumeSession(); + + expect(resumed).toBe(true); + // No network call at all: no refresh, so no rotation. + expect(fetchMock).not.toHaveBeenCalled(); + expect(vi.mocked(sessionModule.saveSession)).not.toHaveBeenCalled(); + }); }); diff --git a/tests/utils/jwt.test.ts b/tests/utils/jwt.test.ts new file mode 100644 index 0000000..f8bc80e --- /dev/null +++ b/tests/utils/jwt.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from 'vitest'; +import { getJwtExpiry, isAccessTokenValid } from '../../src/utils/jwt.js'; + +/** + * Build a syntactically valid JWT (header.payload.signature) with the given + * payload. The signature is irrelevant; these helpers never verify it. + */ +function makeJwt(payload: Record): string { + const header = Buffer.from(JSON.stringify({ alg: 'none', typ: 'JWT' })).toString('base64url'); + const body = Buffer.from(JSON.stringify(payload)).toString('base64url'); + return `${header}.${body}.sig`; +} + +const nowSeconds = () => Math.floor(Date.now() / 1000); + +describe('getJwtExpiry', () => { + it('returns the exp claim in seconds', () => { + const exp = nowSeconds() + 3600; + expect(getJwtExpiry(makeJwt({ exp }))).toBe(exp); + }); + + it('returns null when the token is not three parts', () => { + expect(getJwtExpiry('not-a-jwt')).toBeNull(); + expect(getJwtExpiry('only.two')).toBeNull(); + }); + + it('returns null when the payload is not valid JSON', () => { + const header = Buffer.from('{}').toString('base64url'); + expect(getJwtExpiry(`${header}.notjson.sig`)).toBeNull(); + }); + + it('returns null when there is no numeric exp claim', () => { + expect(getJwtExpiry(makeJwt({ sub: 'did:plc:test' }))).toBeNull(); + expect(getJwtExpiry(makeJwt({ exp: 'soon' }))).toBeNull(); + }); +}); + +describe('isAccessTokenValid', () => { + it('returns true for a token expiring comfortably in the future', () => { + expect(isAccessTokenValid(makeJwt({ exp: nowSeconds() + 3600 }))).toBe(true); + }); + + it('returns false for an already-expired token', () => { + expect(isAccessTokenValid(makeJwt({ exp: nowSeconds() - 10 }))).toBe(false); + }); + + it('returns false for a token expiring within the safety margin', () => { + expect(isAccessTokenValid(makeJwt({ exp: nowSeconds() + 30 }), 60)).toBe(false); + }); + + it('returns false for a malformed token', () => { + expect(isAccessTokenValid('garbage')).toBe(false); + }); +});