diff --git a/package-lock.json b/package-lock.json index 9135c175..26d57f12 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12300,6 +12300,7 @@ }, "node_modules/react-native-url-polyfill": { "version": "3.0.0", + "dev": true, "license": "MIT", "dependencies": { "whatwg-url-without-unicode": "8.0.0-3" @@ -15149,12 +15150,6 @@ "vitest": "^4.0.16" } }, - "packages/atxp-base/node_modules/@atxp/mpp": { - "version": "0.11.7", - "resolved": "https://registry.npmjs.org/@atxp/mpp/-/mpp-0.11.7.tgz", - "integrity": "sha512-VnZcFAxlE1T8F9ssSRVk2BYP2DTPQbjmlNln1pPkbDk5fVhgQ9Il/yqkgo2tEPT8wX9CCBLd1v6YYLTAmh7Wlw==", - "license": "MIT" - }, "packages/atxp-client": { "name": "@atxp/client", "version": "0.11.8", @@ -15187,25 +15182,6 @@ "react-native-url-polyfill": "^3.0.0" } }, - "packages/atxp-client/node_modules/@atxp/common": { - "version": "0.11.7", - "resolved": "https://registry.npmjs.org/@atxp/common/-/common-0.11.7.tgz", - "integrity": "sha512-9iAOCsjU3ztzKm75uQlEFF9lch7xpFf9ySJQ58mYyCE2x6MB7SuMnGJ9hlGB5J/PFbqdyzeW9YffG4hJDu6kng==", - "license": "MIT", - "dependencies": { - "bignumber.js": "^9.3.0", - "jose": "^6.0.11", - "oauth4webapi": "^3.8.3", - "tweetnacl": "^1.0.3", - "tweetnacl-util": "^0.15.1" - } - }, - "packages/atxp-client/node_modules/@atxp/mpp": { - "version": "0.11.7", - "resolved": "https://registry.npmjs.org/@atxp/mpp/-/mpp-0.11.7.tgz", - "integrity": "sha512-VnZcFAxlE1T8F9ssSRVk2BYP2DTPQbjmlNln1pPkbDk5fVhgQ9Il/yqkgo2tEPT8wX9CCBLd1v6YYLTAmh7Wlw==", - "license": "MIT" - }, "packages/atxp-cloudflare": { "name": "@atxp/cloudflare", "version": "0.11.8", @@ -15410,12 +15386,6 @@ "vitest": "^4.0.16" } }, - "packages/atxp-tempo/node_modules/@atxp/mpp": { - "version": "0.11.7", - "resolved": "https://registry.npmjs.org/@atxp/mpp/-/mpp-0.11.7.tgz", - "integrity": "sha512-VnZcFAxlE1T8F9ssSRVk2BYP2DTPQbjmlNln1pPkbDk5fVhgQ9Il/yqkgo2tEPT8wX9CCBLd1v6YYLTAmh7Wlw==", - "license": "MIT" - }, "packages/atxp-worldchain": { "name": "@atxp/worldchain", "version": "0.11.8", diff --git a/packages/atxp-client/src/oAuth.ts b/packages/atxp-client/src/oAuth.ts index ed4a3ee1..d2cfded2 100644 --- a/packages/atxp-client/src/oAuth.ts +++ b/packages/atxp-client/src/oAuth.ts @@ -307,8 +307,8 @@ export class OAuthClient extends OAuthResourceClient { resourceUrl, accessToken: result.access_token, refreshToken: result.refresh_token, - expiresAt: result.expires_in - ? Date.now() + result.expires_in * 1000 + expiresAt: result.expires_in + ? Math.floor(Date.now() / 1000) + result.expires_in : undefined }); @@ -359,8 +359,8 @@ export class OAuthClient extends OAuthResourceClient { resourceUrl: token.resourceUrl, accessToken: result.access_token, refreshToken: result.refresh_token, - expiresAt: result.expires_in - ? Date.now() + result.expires_in * 1000 + expiresAt: result.expires_in + ? Math.floor(Date.now() / 1000) + result.expires_in : undefined }; await this.db.saveAccessToken(this.userId, url, at); diff --git a/packages/atxp-client/src/oauth.test.ts b/packages/atxp-client/src/oauth.test.ts index 6e0ddebf..a8401dfc 100644 --- a/packages/atxp-client/src/oauth.test.ts +++ b/packages/atxp-client/src/oauth.test.ts @@ -100,7 +100,7 @@ describe('oauthClient', () => { db.saveAccessToken('bdj', 'https://example.com/mcp', { resourceUrl: 'https://example.com/mcp', accessToken: 'test-access-token', - expiresAt: Date.now() + 1000 * 60 * 60 * 24 * 30 + expiresAt: Math.floor(Date.now() / 1000) + 60 * 60 * 24 * 30 }); const f = fetchMock.createInstance().getOnce('https://example.com/mcp', 401); mockResourceServer(f, 'https://example.com', '/mcp'); @@ -121,7 +121,7 @@ describe('oauthClient', () => { db.saveAccessToken('bdj', 'https://example.com', { resourceUrl: 'https://example.com', accessToken: 'test-access-token', - expiresAt: Date.now() + 1000 * 60 * 60 * 24 * 30 + expiresAt: Math.floor(Date.now() / 1000) + 60 * 60 * 24 * 30 }); const f = fetchMock.createInstance().getOnce('https://example.com/mcp', 401); mockResourceServer(f, 'https://example.com', '/mcp'); @@ -158,7 +158,7 @@ describe('oauthClient', () => { // Expires in the future, but the server can invalidate tokens whenever it wants // regardless. Set the time in the future so future changes to the client don't // pre-emptively refresh the token and break this test case - expiresAt: Date.now() + 1000, + expiresAt: Math.floor(Date.now() / 1000) + 3600, refreshToken: 'oldRefreshToken' }; db.saveAccessToken('bdj', 'https://example.com/mcp', oldToken); @@ -200,7 +200,7 @@ describe('oauthClient', () => { expect(token).not.toBeNull(); expect(token?.accessToken).toEqual('newAccessToken'); expect(token?.refreshToken).toEqual('newRefreshToken'); - expect(token?.expiresAt).toBeGreaterThan(Date.now()); + expect(token?.expiresAt).toBeGreaterThan(Math.floor(Date.now() / 1000)); }); it('should throw if the token refresh fails', async () => { @@ -211,7 +211,7 @@ describe('oauthClient', () => { // Expires in the future, but the server can invalidate tokens whenever it wants // regardless. Set the time in the future so future changes to the client don't // pre-emptively refresh the token and break this test case - expiresAt: Date.now() + 1000, + expiresAt: Math.floor(Date.now() / 1000) + 3600, refreshToken: 'oldRefreshToken' }; db.saveAccessToken('bdj', 'https://example.com/mcp', oldToken); @@ -423,7 +423,7 @@ describe('oauthClient', () => { const token = await db.getAccessToken('bdj', 'https://example.com/mcp'); expect(token).not.toBeNull(); expect(token?.accessToken).toEqual('testAccessToken'); - expect(token?.expiresAt).toBeGreaterThan(Date.now()); + expect(token?.expiresAt).toBeGreaterThan(Math.floor(Date.now() / 1000)); }); it('should throw if no PKCE values found for state', async () => { diff --git a/packages/atxp-common/src/memoryOAuthDb.test.ts b/packages/atxp-common/src/memoryOAuthDb.test.ts new file mode 100644 index 00000000..4caf6b7d --- /dev/null +++ b/packages/atxp-common/src/memoryOAuthDb.test.ts @@ -0,0 +1,30 @@ +import { describe, it, expect } from 'vitest'; +import { MemoryOAuthDb } from './memoryOAuthDb.js'; +import type { AccessToken } from './types.js'; + +// AccessToken.expiresAt is Unix epoch SECONDS. These tests pin that contract, +// which the in-memory store previously got wrong (it compared seconds against +// Date.now() milliseconds, so valid tokens looked expired and vice versa). +describe('MemoryOAuthDb expiry (epoch seconds)', () => { + const token = (expiresAt?: number): AccessToken => ({ + accessToken: 'a', + resourceUrl: 'https://example.com', + expiresAt + }); + + it('returns a token whose expiresAt is in the future (seconds)', async () => { + const db = new MemoryOAuthDb(); + const t = token(Math.floor(Date.now() / 1000) + 3600); + await db.saveAccessToken('u', 'https://example.com', t); + expect(await db.getAccessToken('u', 'https://example.com')).toEqual(t); + await db.close(); + }); + + it('evicts a token whose expiresAt is in the past (seconds)', async () => { + const db = new MemoryOAuthDb(); + const t = token(Math.floor(Date.now() / 1000) - 60); + await db.saveAccessToken('u', 'https://example.com', t); + expect(await db.getAccessToken('u', 'https://example.com')).toBeNull(); + await db.close(); + }); +}); diff --git a/packages/atxp-common/src/memoryOAuthDb.ts b/packages/atxp-common/src/memoryOAuthDb.ts index c8f88622..b20843a7 100644 --- a/packages/atxp-common/src/memoryOAuthDb.ts +++ b/packages/atxp-common/src/memoryOAuthDb.ts @@ -58,7 +58,7 @@ export class MemoryOAuthDb implements OAuthDb { } // Check if token has expired - if (token.expiresAt && token.expiresAt < Date.now()) { + if (token.expiresAt && token.expiresAt * 1000 < Date.now()) { this.logger.info(`Access token expired for user: ${userId}, url: ${url}`); this.accessTokens.delete(key); return null; @@ -101,7 +101,7 @@ export class MemoryOAuthDb implements OAuthDb { let cleaned = 0; for (const [key, token] of this.accessTokens.entries()) { - if (token.expiresAt && token.expiresAt < now) { + if (token.expiresAt && token.expiresAt * 1000 < now) { this.accessTokens.delete(key); cleaned++; } diff --git a/packages/atxp-common/src/oAuthResource.ts b/packages/atxp-common/src/oAuthResource.ts index fa4f2813..ffa91ca5 100644 --- a/packages/atxp-common/src/oAuthResource.ts +++ b/packages/atxp-common/src/oAuthResource.ts @@ -146,7 +146,8 @@ export class OAuthResourceClient { active: tokenData.active, scope: tokenData.scope, sub: tokenData.sub, - aud: tokenData.aud + aud: tokenData.aud, + exp: tokenData.exp }; } diff --git a/packages/atxp-common/src/types.ts b/packages/atxp-common/src/types.ts index 308d588e..4ef57678 100644 --- a/packages/atxp-common/src/types.ts +++ b/packages/atxp-common/src/types.ts @@ -137,7 +137,7 @@ export type PKCEValues = { export type AccessToken = { accessToken: string, refreshToken?: string, - expiresAt?: number, + expiresAt?: number, // Unix epoch SECONDS (not milliseconds) resourceUrl: string }; @@ -159,6 +159,7 @@ export type TokenData = { scope?: string, sub?: string, aud?: string|string[], + exp?: number, // Token expiry as Unix epoch SECONDS (RFC 7662 introspection `exp`), when the authorization server provides it } // This should match MCP SDK's version, however they don't export it diff --git a/packages/atxp-redis/src/index.ts b/packages/atxp-redis/src/index.ts index a1664394..663cac30 100644 --- a/packages/atxp-redis/src/index.ts +++ b/packages/atxp-redis/src/index.ts @@ -1,6 +1,16 @@ import { ConsoleLogger } from '@atxp/common'; import type { AccessToken, ClientCredentials, Logger, OAuthDb, PKCEValues } from '@atxp/common'; +/** + * Default TTL (seconds) applied to access tokens that have neither an explicit + * `expiresAt` nor a configured `ttl`. Such tokens were previously stored with + * no expiry (plain `SET`), so they accumulated forever and grew shared Redis + * instances without bound. 24h is a safe cap: callers (e.g. the ATXP server + * middleware) re-save tokens on every request, so active sessions stay cached + * while idle entries age out. + */ +export const DEFAULT_ACCESS_TOKEN_TTL_SECONDS = 24 * 60 * 60; + export interface RedisClient { get(key: string): Promise; set(key: string, value: string): Promise; @@ -16,6 +26,7 @@ export interface RedisOAuthDbConfig { logger?: Logger; keyPrefix?: string; ttl?: number; // TTL in seconds for tokens + defaultTtl?: number; // Fallback TTL (seconds) for access tokens with no explicit expiry. Defaults to DEFAULT_ACCESS_TOKEN_TTL_SECONDS. } export class RedisOAuthDb implements OAuthDb { @@ -25,6 +36,7 @@ export class RedisOAuthDb implements OAuthDb { private logger: Logger; private keyPrefix: string; private ttl?: number; + private defaultTtl: number; constructor({ redis, @@ -32,7 +44,8 @@ export class RedisOAuthDb implements OAuthDb { decrypt = (data: string) => data, logger = new ConsoleLogger(), keyPrefix = 'oauth:', - ttl + ttl, + defaultTtl = DEFAULT_ACCESS_TOKEN_TTL_SECONDS }: RedisOAuthDbConfig) { if (typeof redis === 'string') { // Dynamic import to avoid bundling issues @@ -46,6 +59,7 @@ export class RedisOAuthDb implements OAuthDb { this.logger = logger; this.keyPrefix = keyPrefix; this.ttl = ttl; + this.defaultTtl = defaultTtl; } private async createRedisClient(redisUrl: string): Promise { @@ -187,8 +201,15 @@ export class RedisOAuthDb implements OAuthDb { const ttlSeconds = Math.max(1, token.expiresAt - Math.floor(Date.now() / 1000)); await redis.setex(key, ttlSeconds, data); } else { - // No expiration, store indefinitely - await redis.set(key, data); + // No explicit expiry: apply a bounded default TTL so access tokens + // cannot accumulate indefinitely. Previously this stored the token with + // no expiry, which caused unbounded key growth in shared Redis instances. + // Note: with no expiresAt, expires_at is stored null and the read-path + // expiry check is skipped, so this TTL is the only bound — a token could be + // served for up to defaultTtl after it actually expires upstream. Callers + // that can supply expiresAt (the server middleware sets it from the + // introspection `exp`) get exact expiry instead of this backstop. + await redis.setex(key, this.defaultTtl, data); } } diff --git a/packages/atxp-redis/src/redisOAuthDb.test.ts b/packages/atxp-redis/src/redisOAuthDb.test.ts index 7ba715e3..b16d0e86 100644 --- a/packages/atxp-redis/src/redisOAuthDb.test.ts +++ b/packages/atxp-redis/src/redisOAuthDb.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { RedisOAuthDb } from './index.js'; +import { RedisOAuthDb, DEFAULT_ACCESS_TOKEN_TTL_SECONDS } from './index.js'; import type { AccessToken, ClientCredentials, PKCEValues } from '@atxp/common'; // Mock Redis client for unit tests @@ -187,6 +187,75 @@ describe('RedisOAuthDb', () => { }); }); + describe('Default TTL (no explicit expiry)', () => { + const userId = 'test-user'; + const url = 'https://example.com'; + const tokenWithoutExpiry: AccessToken = { + accessToken: 'no-expiry-token', + resourceUrl: 'https://example.com' + }; + + it('applies a bounded default TTL instead of storing indefinitely', async () => { + // Regression guard: this branch previously called redis.set (no TTL), so + // tokens without an expiry accumulated forever and bloated shared Redis. + await db.saveAccessToken(userId, url, tokenWithoutExpiry); + + expect(mockRedis.set).not.toHaveBeenCalled(); + expect(mockRedis.setex).toHaveBeenCalledWith( + 'test:oauth:access_token:test-user:https://example.com', + DEFAULT_ACCESS_TOKEN_TTL_SECONDS, + expect.any(String) + ); + }); + + it('honors a configured defaultTtl', async () => { + const customDb = new RedisOAuthDb({ + redis: mockRedis, + keyPrefix: 'test:oauth:', + defaultTtl: 3600 + }); + + await customDb.saveAccessToken(userId, url, tokenWithoutExpiry); + + expect(mockRedis.setex).toHaveBeenCalledWith( + 'test:oauth:access_token:test-user:https://example.com', + 3600, + expect.any(String) + ); + + await customDb.close(); + }); + + it('still prefers an explicit token expiry over the default TTL', async () => { + const expiresAt = Math.floor(Date.now() / 1000) + 120; + await db.saveAccessToken(userId, url, { + accessToken: 'expiring-token', + resourceUrl: 'https://example.com', + expiresAt + }); + + const call = mockRedis.setex.mock.calls.find( + c => c[0] === 'test:oauth:access_token:test-user:https://example.com' + ); + expect(call).toBeDefined(); + expect(call![1]).toBeGreaterThan(0); + expect(call![1]).toBeLessThanOrEqual(120); + }); + + it('refreshes the TTL on every re-save (sliding window)', async () => { + // The whole fix relies on this: withATXPContext re-saves on each request, + // so an active session keeps re-arming the TTL while idle keys expire. + await db.saveAccessToken(userId, url, tokenWithoutExpiry); + await db.saveAccessToken(userId, url, tokenWithoutExpiry); + + const calls = mockRedis.setex.mock.calls.filter( + c => c[0] === 'test:oauth:access_token:test-user:https://example.com' + ); + expect(calls).toHaveLength(2); + expect(calls.every(c => c[1] === DEFAULT_ACCESS_TOKEN_TTL_SECONDS)).toBe(true); + }); + }); + describe('Encryption Support', () => { it('should support custom encryption/decryption functions', async () => { const encryptedDb = new RedisOAuthDb({ @@ -259,24 +328,26 @@ describe('RedisOAuthDb', () => { }); }); - describe('TTL Configuration', () => { - it('should use configured default TTL for tokens without expiration', async () => { + describe('Unconditional ttl option', () => { + it('applies the configured `ttl` to all tokens, overriding any explicit expiresAt', async () => { const ttlDb = new RedisOAuthDb({ redis: mockRedis, keyPrefix: 'test:ttl:', - ttl: 1800 // 30 minutes + ttl: 1800 // 30 minutes, applied unconditionally to every token }); - const tokenWithoutExpiry: AccessToken = { - accessToken: 'no-expiry-token', - resourceUrl: 'https://example.com' + // Token WITH an explicit far-future expiry — the unconditional ttl still wins. + const token: AccessToken = { + accessToken: 'tok', + resourceUrl: 'https://example.com', + expiresAt: Math.floor(Date.now() / 1000) + 86400 // 24h }; - await ttlDb.saveAccessToken('user', 'https://example.com', tokenWithoutExpiry); + await ttlDb.saveAccessToken('user', 'https://example.com', token); expect(mockRedis.setex).toHaveBeenCalledWith( 'test:ttl:access_token:user:https://example.com', - 1800, // Configured TTL + 1800, // the unconditional ttl, NOT the ~86400 derived from expiresAt expect.any(String) ); diff --git a/packages/atxp-server/src/atxpContext.test.ts b/packages/atxp-server/src/atxpContext.test.ts index 27551c0b..ba85c2df 100644 --- a/packages/atxp-server/src/atxpContext.test.ts +++ b/packages/atxp-server/src/atxpContext.test.ts @@ -75,4 +75,15 @@ describe('continueWithATXPContext', () => { const fromDb = await cfg.oAuthDb.getAccessToken(tokenCheck.data!.sub!, ''); expect(fromDb).toBeNull(); }); + + it('writes the introspected token expiry (exp) to the oauth DB', async () => { + const cfg = TH.config(); + const exp = Math.floor(Date.now() / 1000) + 3600; + const tokenCheck = TH.tokenCheck({ data: { ...TH.tokenData(), exp } }); + await withATXPContext(cfg, new URL('https://example.com'), tokenCheck, () => {}); + const fromDb = await cfg.oAuthDb.getAccessToken(tokenCheck.data!.sub!, ''); + // Root fix: the cached token carries the introspection exp (epoch seconds) so + // every OAuthDb backend can bound/evict it instead of storing it forever. + expect(fromDb?.expiresAt).toBe(exp); + }); }); diff --git a/packages/atxp-server/src/atxpContext.ts b/packages/atxp-server/src/atxpContext.ts index bee52526..5442e61c 100644 --- a/packages/atxp-server/src/atxpContext.ts +++ b/packages/atxp-server/src/atxpContext.ts @@ -107,7 +107,11 @@ export async function withATXPContext(config: ATXPConfig, resource: URL, tokenIn if(tokenInfo.token) { const dbData = { accessToken: tokenInfo.token!, - resourceUrl: '' + resourceUrl: '', + // Bound the cached token to the introspected token's lifetime (epoch seconds). + // Without this the OAuthDb backends store it with no expiry and accumulate + // forever (and could hand back an already-expired token). + expiresAt: tokenInfo.data.exp }; // Save the token to the oAuthDB so that other users of the DB can access it // if needed (ie, for token-exchange for downstream services) diff --git a/packages/atxp-sqlite/src/sqliteOAuthDb.test.ts b/packages/atxp-sqlite/src/sqliteOAuthDb.test.ts index 04b88e21..676cc824 100644 --- a/packages/atxp-sqlite/src/sqliteOAuthDb.test.ts +++ b/packages/atxp-sqlite/src/sqliteOAuthDb.test.ts @@ -142,6 +142,19 @@ describe('SqliteOAuthDb', () => { expect(retrieved?.expiresAt).toBeUndefined(); }); + it('returns null for an expired token (read-path expiry)', async () => { + const expired: AccessToken = { + accessToken: 'expired-token', + expiresAt: Math.floor(Date.now() / 1000) - 60, // 1 minute ago + resourceUrl: 'https://example.com' + }; + await db.saveAccessToken('test-user', 'https://example.com', expired); + + // Previously sqlite selected expires_at but never compared it, so it returned + // the expired token as valid and never evicted it. + expect(await db.getAccessToken('test-user', 'https://example.com')).toBeNull(); + }); + it('should update existing access tokens', async () => { await db.saveAccessToken(userId, url, accessToken); diff --git a/packages/atxp-sqlite/src/sqliteOAuthDb.ts b/packages/atxp-sqlite/src/sqliteOAuthDb.ts index 3b187e2b..c2074829 100644 --- a/packages/atxp-sqlite/src/sqliteOAuthDb.ts +++ b/packages/atxp-sqlite/src/sqliteOAuthDb.ts @@ -154,18 +154,38 @@ export class SqliteOAuthDb implements OAuthDb { const statement = await this.db.prepareAsync( 'SELECT resource_url, encrypted_access_token, encrypted_refresh_token, expires_at FROM oauth_access_tokens WHERE user_id = ? AND url = ?' ); + let row: { resource_url: string; encrypted_access_token: string; encrypted_refresh_token: string | null; expires_at: string | null } | null = null; try { const result = await statement.executeAsync<{ resource_url: string; encrypted_access_token: string; encrypted_refresh_token: string | null; expires_at: string | null }>(userId, url); - const row = await result.getFirstAsync(); + row = await result.getFirstAsync(); + } finally { + await statement.finalizeAsync(); + } + + if (!row) return null; - if (!row) return null; + const expiresAt = row.expires_at ? parseInt(row.expires_at) : undefined; + // Read-path expiry: never hand back an expired token, and delete the row so the + // table doesn't retain dead tokens. expiresAt is Unix epoch seconds. + if (expiresAt !== undefined && Date.now() >= expiresAt * 1000) { + await this.deleteAccessToken(userId, url); + return null; + } - return { - accessToken: this.decrypt(row.encrypted_access_token), - refreshToken: row.encrypted_refresh_token ? this.decrypt(row.encrypted_refresh_token) : undefined, - expiresAt: row.expires_at ? parseInt(row.expires_at) : undefined, - resourceUrl: row.resource_url - }; + return { + accessToken: this.decrypt(row.encrypted_access_token), + refreshToken: row.encrypted_refresh_token ? this.decrypt(row.encrypted_refresh_token) : undefined, + expiresAt, + resourceUrl: row.resource_url + }; + } + + private deleteAccessToken = async (userId: string, url: string): Promise => { + const statement = await this.db.prepareAsync( + 'DELETE FROM oauth_access_tokens WHERE user_id = ? AND url = ?' + ); + try { + await statement.executeAsync(userId, url); } finally { await statement.finalizeAsync(); }