Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 1 addition & 31 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions packages/atxp-client/src/oAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
});

Expand Down Expand Up @@ -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);
Expand Down
12 changes: 6 additions & 6 deletions packages/atxp-client/src/oauth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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);
Expand Down Expand Up @@ -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 () => {
Expand Down
30 changes: 30 additions & 0 deletions packages/atxp-common/src/memoryOAuthDb.test.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
4 changes: 2 additions & 2 deletions packages/atxp-common/src/memoryOAuthDb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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++;
}
Expand Down
3 changes: 2 additions & 1 deletion packages/atxp-common/src/oAuthResource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
}

Expand Down
3 changes: 2 additions & 1 deletion packages/atxp-common/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

Expand All @@ -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
Expand Down
27 changes: 24 additions & 3 deletions packages/atxp-redis/src/index.ts
Original file line number Diff line number Diff line change
@@ -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<string | null>;
set(key: string, value: string): Promise<unknown>;
Expand All @@ -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 {
Expand All @@ -25,14 +36,16 @@ export class RedisOAuthDb implements OAuthDb {
private logger: Logger;
private keyPrefix: string;
private ttl?: number;
private defaultTtl: number;

constructor({
redis,
encrypt = (data: string) => data,
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
Expand All @@ -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<RedisClient> {
Expand Down Expand Up @@ -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);
}
}

Expand Down
89 changes: 80 additions & 9 deletions packages/atxp-redis/src/redisOAuthDb.test.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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)
);

Expand Down
Loading
Loading