From 2a37c87e3e5e31041f9e7463fc409b9a678d996d Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 28 Jul 2026 14:26:30 +0200 Subject: [PATCH 01/15] feat(mcp-server): let clients shorten the OAuth token lifetimes Spendesk asked for short-lived OAuth tokens on the MCP server to force periodic re-authentication and limit the blast radius of a leaked token. Nothing was configurable: the access token mirrored Forest Admin's 1h and the refresh token its 8 days. agent.mountAiMcpServer({ tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, }); Both values are upper bounds, applied as a min() against what Forest Admin granted, so a client can only ever shorten a lifetime. The cap is applied where the TTL is computed rather than in the sign() options, which keeps the advertised expires_in in step with the lifetime signed into the JWT. The refresh cap is anchored on the interactive login through a sessionStartedAt claim that is carried across refreshes and never re-stamped. Forest Admin mints a brand-new full-lifetime refresh token on every refresh grant, so a plain per-refresh cap would slide forever: an active client would never re-authenticate and the option would only be an idle timeout. Refresh tokens predating the claim fall back to their issue date. Once the window elapses the refresh is rejected before any call to Forest Admin. Values below 60s are raised to it: the MCP SDK rate-limits /oauth/token to 50 requests per 15 minutes, so a shorter lifetime makes a client exhaust its own quota and take 429s. Invalid values (zero, negative, fractional) fail at startup instead of silently leaving the tokens uncapped. Available embedded and standalone (FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS, FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS), since standalone is the only mode available to non-Node back-ends. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/agent.ts | 15 +- packages/agent/test/agent.test.ts | 14 ++ packages/mcp-server/CLAUDE.md | 2 +- packages/mcp-server/README.md | 27 +++ packages/mcp-server/src/cli.ts | 5 + .../mcp-server/src/forest-oauth-provider.ts | 84 ++++++-- packages/mcp-server/src/index.ts | 1 + packages/mcp-server/src/server.ts | 11 + .../mcp-server/src/utils/parse-token-ttl.ts | 15 ++ packages/mcp-server/src/utils/token-ttl.ts | 79 +++++++ packages/mcp-server/test/cli.test.ts | 24 +++ .../test/forest-oauth-provider.test.ts | 204 +++++++++++++++++- packages/mcp-server/test/server.test.ts | 89 ++++++++ .../mcp-server/test/utils/token-ttl.test.ts | 133 ++++++++++++ 14 files changed, 683 insertions(+), 20 deletions(-) create mode 100644 packages/mcp-server/src/utils/parse-token-ttl.ts create mode 100644 packages/mcp-server/src/utils/token-ttl.ts create mode 100644 packages/mcp-server/test/utils/token-ttl.test.ts diff --git a/packages/agent/src/agent.ts b/packages/agent/src/agent.ts index a8e3b2397d..4e20fcbd91 100644 --- a/packages/agent/src/agent.ts +++ b/packages/agent/src/agent.ts @@ -18,7 +18,7 @@ import type { } from '@forestadmin/datasource-customizer'; import type { DataSource, DataSourceFactory } from '@forestadmin/datasource-toolkit'; import type { ForestSchema } from '@forestadmin/forestadmin-client'; -import type { ToolName } from '@forestadmin/mcp-server'; +import type { TokenTtlOptions, ToolName } from '@forestadmin/mcp-server'; import { DataSourceCustomizer } from '@forestadmin/datasource-customizer'; import bodyParser from '@koa/bodyparser'; @@ -57,6 +57,7 @@ export default class Agent extends FrameworkMounter private mcpEnabled = false; private mcpEnabledTools?: ToolName[]; private mcpBasePath?: string; + private mcpTokenTtl?: TokenTtlOptions; /** In-process workflow executor, created only when addWorkflowExecutor() is called. */ private embeddedExecutor: EmbeddedWorkflowExecutor | null = null; @@ -257,11 +258,20 @@ export default class Agent extends FrameworkMounter * // OAuth discovery metadata stays at the origin root (prefix-suffixed), so root `.well-known` * // traffic must still reach the agent. * agent.mountAiMcpServer({ basePath: '/ai' }); + * // Example: shorten the OAuth token lifetimes. Both values are upper bounds — they can only + * // shorten what Forest Admin granted, so a value above its own TTL has no effect. + * // `refreshTokenSeconds` bounds the time between two interactive logins. + * agent.mountAiMcpServer({ tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 } }); */ - mountAiMcpServer(options?: { enabledTools?: ToolName[]; basePath?: string }): this { + mountAiMcpServer(options?: { + enabledTools?: ToolName[]; + basePath?: string; + tokenTtl?: TokenTtlOptions; + }): this { this.mcpEnabled = true; this.mcpEnabledTools = options?.enabledTools; this.mcpBasePath = options?.basePath; + this.mcpTokenTtl = options?.tokenTtl; return this; } @@ -429,6 +439,7 @@ export default class Agent extends FrameworkMounter forestServerClient, enabledTools: this.mcpEnabledTools, basePath: this.mcpBasePath, + tokenTtl: this.mcpTokenTtl, agentDispatcher: this.getInProcessDispatcher(), }); diff --git a/packages/agent/test/agent.test.ts b/packages/agent/test/agent.test.ts index ffe113ff7b..64fcafc67d 100644 --- a/packages/agent/test/agent.test.ts +++ b/packages/agent/test/agent.test.ts @@ -591,6 +591,20 @@ describe('Agent', () => { expect(mcpServerSpy).toHaveBeenCalledWith(expect.objectContaining({ basePath: '/mcp' })); }); + test('should pass tokenTtl to ForestMCPServer', async () => { + const options = factories.forestAdminHttpDriverOptions.build(); + const agent = new Agent(options); + + agent.mountAiMcpServer({ tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 } }); + await agent.start(); + + expect(mcpServerSpy).toHaveBeenCalledWith( + expect.objectContaining({ + tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, + }), + ); + }); + test('passes an in-process agentDispatcher to ForestMCPServer', async () => { const options = factories.forestAdminHttpDriverOptions.build(); const agent = new Agent(options); diff --git a/packages/mcp-server/CLAUDE.md b/packages/mcp-server/CLAUDE.md index 552db3aa83..261030b522 100644 --- a/packages/mcp-server/CLAUDE.md +++ b/packages/mcp-server/CLAUDE.md @@ -13,7 +13,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Key flows that only make sense across files: - **Per-request MCP server.** `handleMcpRequest` builds a *fresh* `McpServer` + `StreamableHTTPServerTransport` per request (`sessionIdGenerator: undefined`, stateless) and closes the transport on response end. Tool registration in `createMcpServer()` therefore re-runs on every call. -- **OAuth = pass-through to Forest Admin.** `ForestOAuthProvider` (`src/forest-oauth-provider.ts`) does not store tokens. `/oauth/authorize` redirects to the Forest app; `exchange*` calls relay to the Forest server's `/oauth/token`, then `generateAccessToken` re-signs a JWT with `authSecret` carrying the Forest token under the `serverToken` claim. `verifyAccessToken` verifies that JWT and builds `AuthInfo.extra` with `forestServerToken: decoded.serverToken` plus `environmentApiEndpoint` — the latter is **not** in the JWT, it's read from the provider's own `this.environmentApiEndpoint` field (set during env discovery). +- **OAuth = pass-through to Forest Admin.** `ForestOAuthProvider` (`src/forest-oauth-provider.ts`) does not store tokens. `/oauth/authorize` redirects to the Forest app; `exchange*` calls relay to the Forest server's `/oauth/token`, then `generateAccessToken` re-signs a JWT with `authSecret` carrying the Forest token under the `serverToken` claim. `verifyAccessToken` verifies that JWT and builds `AuthInfo.extra` with `forestServerToken: decoded.serverToken` plus `environmentApiEndpoint` — the latter is **not** in the JWT, it's read from the provider's own `this.environmentApiEndpoint` field (set during env discovery). Both lifetimes mirror the Forest token's own `exp`, optionally shortened by the `tokenTtl` option (`src/utils/token-ttl.ts`) — always a `min`, never a `max`, and capped at the point the TTL is computed so the advertised `expires_in` stays in step with the signed JWT. The refresh cap is anchored on a `sessionStartedAt` claim carried across refreshes and never re-stamped: Forest grants a full refresh lifetime on every refresh, so a per-refresh cap would slide forever and never force a re-login. - **Tools call the live agent, not this server.** Each tool in `src/tools/*` is a `declareXxxTool(mcpServer, forestServerClient, logger, collectionNames)` factory. At call time `buildClient(extra)` (`src/utils/agent-caller.ts`) reads `extra.authInfo` (`forestServerToken` + `environmentApiEndpoint` from `AuthInfo.extra`) and builds a `createRemoteAgentClient` from `@forestadmin/agent-client` — i.e. the tool RPCs into the user's actual running agent. `forestServerClient` (`src/http-client`, wrapping `@forestadmin/forestadmin-client`'s `SchemaService`/`ActivityLogsService`) is used only for schema fetch and activity logging, not data. - **Two cross-cutting wrappers, always used together.** `registerToolWithLogging` (`src/utils/tool-with-logging.ts`) registers the tool and converts thrown errors into `{ isError: true }` results (per MCP spec) instead of protocol errors. Inside the handler, `withActivityLog` (`src/utils/with-activity-log.ts`) brackets the operation with a pending→succeeded/failed Forest activity log and runs `parseAgentError` + optional `errorEnhancer` (e.g. `list` appends sortable field names on "Invalid sort"). - **`collectionNames` → `z.enum`.** `fetchCollectionNames()` populates the schema's collection list; tools turn it into a `z.enum` for `collectionName` so the LLM gets autocomplete/validation. If schema fetch fails the server logs a warning and runs "degraded" with `z.string()`. diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index e2a2faf975..b87e745a7c 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -64,6 +64,8 @@ yarn start:dev # Development (loads .env file automatically) | `MCP_SERVER_PORT` | No | `3931` | Port for the HTTP server | | `FOREST_MCP_ENABLED_TOOLS` | No | - | Comma-separated list of tools to enable (allowlist) | | `FOREST_AGENT_URL` | No | your environment's back-end URL | URL the MCP server uses to reach the back-end's data layer. Set it when the server runs next to a self-hosted back-end at an internal address (e.g. `http://localhost:3310`), instead of the public URL registered in Forest | +| `FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS` | No | Forest Admin's own lifetime | Maximum lifetime of the OAuth access tokens the server issues (`tokenTtl.accessTokenSeconds`). Minimum `60` | +| `FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS` | No | Forest Admin's own lifetime | Maximum time between two interactive logins (`tokenTtl.refreshTokenSeconds`). Minimum `60` | #### Example Configuration @@ -109,6 +111,31 @@ When `enabledTools` is not set, all tools are enabled by default. See [Available Tools](#available-tools) for the full list. `describeCollection` is always enabled as it is required for the MCP server to function properly. +## Shorten Token Lifetimes + +**Both values are upper bounds: they can only shorten the lifetime Forest Admin granted, never extend it.** A value above Forest Admin's own lifetime therefore has no effect. + +```typescript +// With Forest Admin Agent +agent.mountAiMcpServer({ + tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, +}); +``` + +```bash +# Standalone +export FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS=900 +export FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS=86400 +npx forest-mcp-server +``` + +The two settings differ in what the user notices: + +- `accessTokenSeconds` shortens how long a leaked access token stays usable. It is transparent — the AI assistant silently obtains a new one. +- `refreshTokenSeconds` bounds the time between two **interactive logins**: once it elapses, the assistant can no longer refresh and the user signs in through the browser again. It is measured from the login itself, so an active assistant cannot keep extending its session. + +The minimum for either value is 60 seconds; anything lower is raised to it. An invalid value (zero, negative, fractional) fails at startup rather than silently leaving the tokens uncapped. + ## API Endpoints Once running, the MCP server exposes the following endpoints: diff --git a/packages/mcp-server/src/cli.ts b/packages/mcp-server/src/cli.ts index 16dc12e1dd..ed0bef7691 100644 --- a/packages/mcp-server/src/cli.ts +++ b/packages/mcp-server/src/cli.ts @@ -1,6 +1,7 @@ #!/usr/bin/env node import ForestMCPServer from './server'; +import parseTokenTtl from './utils/parse-token-ttl'; import parseToolList from './utils/parse-tool-list'; // Start the server when run directly as CLI @@ -11,6 +12,10 @@ const server = new ForestMCPServer({ authSecret: process.env.FOREST_AUTH_SECRET, enabledTools: parseToolList(process.env.FOREST_MCP_ENABLED_TOOLS), agentUrl: process.env.FOREST_AGENT_URL, + tokenTtl: parseTokenTtl( + process.env.FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS, + process.env.FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS, + ), }); server.run().catch(error => { diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index a351cd8811..9e91ce2db7 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -1,4 +1,5 @@ import type { Logger } from './server'; +import type { TokenTtlOptions } from './utils/token-ttl'; import type { ForestAdminClient } from '@forestadmin/forestadmin-client'; import type { OAuthRegisteredClientsStore } from '@modelcontextprotocol/sdk/server/auth/clients.js'; import type { @@ -24,6 +25,8 @@ import { } from '@modelcontextprotocol/sdk/server/auth/errors.js'; import jsonwebtoken from 'jsonwebtoken'; +import { capAccessTokenTtl, capRefreshTokenTtl } from './utils/token-ttl'; + export interface ForestOAuthProviderOptions { forestServerUrl: string; forestAppUrl: string; @@ -32,6 +35,7 @@ export interface ForestOAuthProviderOptions { logger: Logger; // Pre-normalized by the caller (ForestMCPServer); not re-validated here. agentUrl?: string; + tokenTtl?: TokenTtlOptions; } /** @@ -46,6 +50,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { private environmentId?: number; private environmentApiEndpoint?: string; private agentUrl?: string; + private tokenTtl?: TokenTtlOptions; private logger: Logger; constructor({ @@ -55,6 +60,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { authSecret, logger, agentUrl, + tokenTtl, }: ForestOAuthProviderOptions) { this.forestServerUrl = forestServerUrl; this.forestAppUrl = forestAppUrl; @@ -62,6 +68,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { this.authSecret = authSecret; this.logger = logger; this.agentUrl = agentUrl; + this.tokenTtl = tokenTtl; this.forestClient = createForestAdminClient({ forestServerUrl: this.forestServerUrl, envSecret: this.envSecret, @@ -205,13 +212,18 @@ export default class ForestOAuthProvider implements OAuthServerProvider { redirectUri?: string, ): Promise { try { - return await this.generateAccessToken(client, { - grant_type: 'authorization_code', - code: authorizationCode, - redirect_uri: redirectUri, - client_id: client.client_id, - code_verifier: codeVerifier, - }); + return await this.generateAccessToken( + client, + { + grant_type: 'authorization_code', + code: authorizationCode, + redirect_uri: redirectUri, + client_id: client.client_id, + code_verifier: codeVerifier, + }, + // The only interactive entry point, so this is when the session starts. + Math.floor(Date.now() / 1000), + ); } catch (error) { const message = error instanceof Error ? error.message : String(error); throw new InvalidRequestError(`Failed to exchange authorization code: ${message}`); @@ -230,6 +242,8 @@ export default class ForestOAuthProvider implements OAuthServerProvider { userId: number; renderingId: number; serverRefreshToken: string; + sessionStartedAt?: number; + iat?: number; }; try { @@ -263,14 +277,36 @@ export default class ForestOAuthProvider implements OAuthServerProvider { throw new InvalidClientError('Token was not issued to this client'); } + // Carried over from the interactive login, never re-stamped. Tokens issued before this claim + // existed fall back to their issue date, which closes their window one refresh later. + const nowInSeconds = Math.floor(Date.now() / 1000); + const sessionStartedAt = decoded.sessionStartedAt ?? decoded.iat ?? nowInSeconds; + const { refreshTokenSeconds } = this.tokenTtl ?? {}; + + if ( + refreshTokenSeconds !== undefined && + sessionStartedAt + refreshTokenSeconds <= nowInSeconds + ) { + this.logger( + 'Error', + `[ForestOAuthProvider] Session exceeded tokenTtl.refreshTokenSeconds=${refreshTokenSeconds}: ` + + `sessionStartedAt=${sessionStartedAt}, clientId=${client.client_id}, userId=${decoded.userId}`, + ); + throw new InvalidGrantError('Refresh token has expired'); + } + // Exchange the Forest refresh token for new tokens try { - return await this.generateAccessToken(client, { - grant_type: 'refresh_token', - refresh_token: decoded.serverRefreshToken, - client_id: client.client_id, - scopes, - }); + return await this.generateAccessToken( + client, + { + grant_type: 'refresh_token', + refresh_token: decoded.serverRefreshToken, + client_id: client.client_id, + scopes, + }, + sessionStartedAt, + ); } catch (error) { const message = error instanceof Error ? error.message : String(error); const errorName = error instanceof Error ? error.name : 'Unknown'; @@ -286,6 +322,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { private async generateAccessToken( client: OAuthClientInformationFull, tokenPayload: Record, + sessionStartedAt: number, ): Promise { const response = await fetch(`${this.forestServerUrl}/oauth/token`, { method: 'POST', @@ -346,8 +383,13 @@ export default class ForestOAuthProvider implements OAuthServerProvider { forestServerAccessToken, ); - // Create new access token - const expiresIn = expirationDate - Math.floor(Date.now() / 1000); + // Create new access token. Capping here rather than in the sign() options keeps the advertised + // `expires_in` below in step with the lifetime actually signed into the JWT. + const nowInSeconds = Math.floor(Date.now() / 1000); + const expiresIn = capAccessTokenTtl( + expirationDate - nowInSeconds, + this.tokenTtl?.accessTokenSeconds, + ); const tokenScopes = scope ? scope.split(' ') : ['mcp:read', 'mcp:write', 'mcp:action']; const accessToken = jsonwebtoken.sign( { @@ -370,14 +412,24 @@ export default class ForestOAuthProvider implements OAuthServerProvider { userId: user.id, renderingId, serverRefreshToken: forestServerRefreshToken, + sessionStartedAt, }, this.authSecret, - { expiresIn: refreshTokenExpirationDate - Math.floor(Date.now() / 1000) }, + { + expiresIn: capRefreshTokenTtl({ + upstreamTtlSeconds: refreshTokenExpirationDate - nowInSeconds, + capSeconds: this.tokenTtl?.refreshTokenSeconds, + sessionStartedAt, + nowInSeconds, + }), + }, ); return { access_token: accessToken, token_type: 'Bearer', + // Only reachable when the Forest token is already expired: a cap can shorten a positive + // lifetime but never drive it to zero. expires_in: expiresIn > 0 ? expiresIn : 3600, refresh_token: refreshToken, scope: scope || client.scope, diff --git a/packages/mcp-server/src/index.ts b/packages/mcp-server/src/index.ts index 15af70f4b8..164da65311 100644 --- a/packages/mcp-server/src/index.ts +++ b/packages/mcp-server/src/index.ts @@ -1,6 +1,7 @@ // Library exports only - no side effects export { default as ForestMCPServer } from './server'; export type { ForestMCPServerOptions, HttpCallback, ToolName } from './server'; +export type { TokenTtlOptions } from './utils/token-ttl'; export type { InProcessAgentDispatcher, InProcessDispatchRequest, diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index 449ba3a9c3..99d995285a 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -5,6 +5,7 @@ import './polyfills'; import type { ForestServerClient } from './http-client'; import type { InProcessAgentDispatcher } from './in-process-agent-dispatcher'; import type { ToolContext } from './tool-context'; +import type { TokenTtlOptions } from './utils/token-ttl'; import type { Express } from 'express'; import { authorizationHandler } from '@modelcontextprotocol/sdk/server/auth/handlers/authorize.js'; @@ -38,6 +39,7 @@ import declareUpdateTool from './tools/update'; import normalizeAgentUrl from './utils/normalize-agent-url'; import { fetchForestSchema, getCollectionNames } from './utils/schema-fetcher'; import interceptResponseForErrorLogging from './utils/sse-error-logger'; +import normalizeTokenTtl from './utils/token-ttl'; import { NAME, VERSION } from './version'; export const LOGO_URL = 'https://forest-assets.s3.us-east-1.amazonaws.com/logo-green.png'; @@ -139,6 +141,12 @@ export interface ForestMCPServerOptions { * instead of reaching its public `api_endpoint` over HTTP. */ agentDispatcher?: InProcessAgentDispatcher; + /** + * Upper bounds on the OAuth token lifetimes this server issues. Each value can only shorten the + * lifetime Forest Admin granted, never extend it, so a value above Forest Admin's own TTL has no + * effect. `refreshTokenSeconds` bounds the time between two interactive logins. + */ + tokenTtl?: TokenTtlOptions; } /** @@ -163,6 +171,7 @@ export default class ForestMCPServer { private basePath: string; private agentUrl?: string; private agentDispatcher?: InProcessAgentDispatcher; + private tokenTtl?: TokenTtlOptions; constructor(options?: ForestMCPServerOptions) { this.forestServerUrl = options?.forestServerUrl || 'https://api.forestadmin.com'; @@ -174,6 +183,7 @@ export default class ForestMCPServer { this.basePath = normalizeMountPath(options?.basePath); this.agentUrl = normalizeAgentUrl(options?.agentUrl); this.agentDispatcher = options?.agentDispatcher; + this.tokenTtl = normalizeTokenTtl(options?.tokenTtl, this.logger); // Use injected forestServerClient or create default this.forestServerClient = options?.forestServerClient ?? this.createDefaultForestServerClient(); @@ -430,6 +440,7 @@ export default class ForestMCPServer { authSecret, logger: this.logger, agentUrl: this.agentUrl, + tokenTtl: this.tokenTtl, }); await oauthProvider.initialize(); diff --git a/packages/mcp-server/src/utils/parse-token-ttl.ts b/packages/mcp-server/src/utils/parse-token-ttl.ts new file mode 100644 index 0000000000..0268fda93b --- /dev/null +++ b/packages/mcp-server/src/utils/parse-token-ttl.ts @@ -0,0 +1,15 @@ +import type { TokenTtlOptions } from './token-ttl'; + +// Coercion only — normalizeTokenTtl owns validation, so a bad env var and a bad embedded option +// fail with the same message. `Number('abc')` yields NaN, which it rejects. +export default function parseTokenTtl( + accessTokenSeconds?: string, + refreshTokenSeconds?: string, +): TokenTtlOptions | undefined { + if (!accessTokenSeconds && !refreshTokenSeconds) return undefined; + + return { + accessTokenSeconds: accessTokenSeconds ? Number(accessTokenSeconds) : undefined, + refreshTokenSeconds: refreshTokenSeconds ? Number(refreshTokenSeconds) : undefined, + }; +} diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts new file mode 100644 index 0000000000..f5a4fc71c8 --- /dev/null +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -0,0 +1,79 @@ +import type { Logger } from '../server'; + +// The MCP SDK rate-limits /oauth/token to 50 requests per 15 minutes and server.ts does not +// override it, so a lifetime under ~18s makes a client exhaust its own token quota and get 429s. +// 60s also stays clear of clock drift between agent instances: verifyAccessToken and the SDK's +// bearer middleware both compare timestamps with a zero clock tolerance. +export const MIN_TOKEN_TTL_SECONDS = 60; + +export type TokenTtlOptions = { + accessTokenSeconds?: number; + refreshTokenSeconds?: number; +}; + +function normalizeSeconds( + field: string, + value: number | undefined, + logger: Logger, +): number | undefined { + if (value === undefined) return undefined; + + // Fail closed rather than falling back to a default: a silently ignored cap would leave the + // customer believing their tokens are short-lived when they are not. + if (!Number.isInteger(value) || value <= 0) { + throw new Error( + `Invalid tokenTtl.${field} "${value}": it must be a positive integer number of seconds.`, + ); + } + + if (value < MIN_TOKEN_TTL_SECONDS) { + logger('Warn', `ignoring tokenTtl.${field}: minimum value is ${MIN_TOKEN_TTL_SECONDS} seconds`); + + return MIN_TOKEN_TTL_SECONDS; + } + + return value; +} + +export default function normalizeTokenTtl( + tokenTtl: TokenTtlOptions | undefined, + logger: Logger, +): TokenTtlOptions | undefined { + const accessTokenSeconds = normalizeSeconds( + 'accessTokenSeconds', + tokenTtl?.accessTokenSeconds, + logger, + ); + const refreshTokenSeconds = normalizeSeconds( + 'refreshTokenSeconds', + tokenTtl?.refreshTokenSeconds, + logger, + ); + + if (accessTokenSeconds === undefined && refreshTokenSeconds === undefined) return undefined; + + return { accessTokenSeconds, refreshTokenSeconds }; +} + +export function capAccessTokenTtl(upstreamTtlSeconds: number, capSeconds?: number): number { + return capSeconds === undefined ? upstreamTtlSeconds : Math.min(upstreamTtlSeconds, capSeconds); +} + +// Anchored on the interactive login rather than on the current refresh: the Forest server grants a +// full refresh lifetime on every refresh, so a per-refresh cap would slide forever and never force +// a re-login. +export function capRefreshTokenTtl({ + upstreamTtlSeconds, + capSeconds, + sessionStartedAt, + nowInSeconds, +}: { + upstreamTtlSeconds: number; + capSeconds?: number; + sessionStartedAt: number; + nowInSeconds: number; +}): number { + if (capSeconds === undefined) return upstreamTtlSeconds; + + return Math.min(upstreamTtlSeconds, sessionStartedAt + capSeconds - nowInSeconds); +} diff --git a/packages/mcp-server/test/cli.test.ts b/packages/mcp-server/test/cli.test.ts index 23dfde7fff..2a8b8d1c88 100644 --- a/packages/mcp-server/test/cli.test.ts +++ b/packages/mcp-server/test/cli.test.ts @@ -1,3 +1,4 @@ +import parseTokenTtl from '../src/utils/parse-token-ttl'; import parseToolList from '../src/utils/parse-tool-list'; describe('parseToolList', () => { @@ -25,3 +26,26 @@ describe('parseToolList', () => { expect(parseToolList('delete')).toEqual(['delete']); }); }); + +describe('parseTokenTtl', () => { + it.each([ + [undefined, undefined], + ['', ''], + ])('returns undefined for (%p, %p) so nothing is capped', (access, refresh) => { + expect(parseTokenTtl(access, refresh)).toBeUndefined(); + }); + + it.each([ + ['900', undefined, { accessTokenSeconds: 900, refreshTokenSeconds: undefined }], + [undefined, '86400', { accessTokenSeconds: undefined, refreshTokenSeconds: 86400 }], + ['900', '86400', { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }], + ])('parses (%p, %p) to seconds', (access, refresh, expected) => { + expect(parseTokenTtl(access, refresh)).toEqual(expected); + }); + + // Validation is deliberately deferred to normalizeTokenTtl, so an env var and an embedded option + // fail with the same message. + it('leaves a non-numeric value as NaN for the normalizer to reject', () => { + expect(parseTokenTtl('abc')?.accessTokenSeconds).toBeNaN(); + }); +}); diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index cd11f7cdfb..53299c7d69 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -1,3 +1,4 @@ +import type { TokenTtlOptions } from '../src/utils/token-ttl'; import type { OAuthClientInformationFull } from '@modelcontextprotocol/sdk/shared/auth'; import type { Response } from 'express'; @@ -20,7 +21,11 @@ const TEST_ENV_SECRET = 'test-env-secret'; const TEST_AUTH_SECRET = 'test-auth-secret'; const TEST_FOREST_APP_URL = 'https://app.forestadmin.com'; -function createProvider(forestServerUrl = 'https://api.forestadmin.com', agentUrl?: string) { +function createProvider( + forestServerUrl = 'https://api.forestadmin.com', + agentUrl?: string, + tokenTtl?: TokenTtlOptions, +) { return new ForestOAuthProvider({ forestServerUrl, forestAppUrl: TEST_FOREST_APP_URL, @@ -28,6 +33,7 @@ function createProvider(forestServerUrl = 'https://api.forestadmin.com', agentUr authSecret: TEST_AUTH_SECRET, logger: console.info, agentUrl, + tokenTtl, }); } @@ -955,4 +961,200 @@ describe('ForestOAuthProvider', () => { }); }); }); + + describe('tokenTtl', () => { + const NOW_MS = Date.UTC(2026, 0, 1); + const nowInSeconds = Math.floor(NOW_MS / 1000); + const FOREST_ACCESS_TTL = 3600; + const FOREST_REFRESH_TTL = 604800; + + let mockClient: OAuthClientInformationFull; + + function primeForestTokens() { + mockJwtDecode + .mockReturnValueOnce({ + meta: { renderingId: 456 }, + exp: nowInSeconds + FOREST_ACCESS_TTL, + iat: nowInSeconds, + scope: 'mcp:read', + }) + .mockReturnValueOnce({ + exp: nowInSeconds + FOREST_REFRESH_TTL, + iat: nowInSeconds, + }); + mockJwtSign.mockReturnValue('mocked-jwt-token'); + + mockServer + .get('/liana/environment', { + data: { id: '12345', attributes: { api_endpoint: 'https://api.example.com' } }, + }) + .post('/oauth/token', { + access_token: 'forest-access-token', + refresh_token: 'forest-refresh-token', + expires_in: FOREST_ACCESS_TTL, + token_type: 'Bearer', + scope: 'mcp:read', + }); + global.fetch = mockServer.fetch; + } + + const signedTtl = (nthCall: number) => mockJwtSign.mock.calls[nthCall - 1][2].expiresIn; + const signedPayload = (nthCall: number) => mockJwtSign.mock.calls[nthCall - 1][0]; + + beforeEach(() => { + jest.useFakeTimers().setSystemTime(NOW_MS); + + mockClient = { + client_id: 'test-client-id', + redirect_uris: ['https://example.com/callback'], + scope: 'mcp:read mcp:write', + } as OAuthClientInformationFull; + + mockCreateForestAdminClient.mockReturnValue({ + authService: { + getUserInfo: jest.fn().mockResolvedValue({ + id: 123, + email: 'user@example.com', + firstName: 'Test', + lastName: 'User', + team: 'Operations', + role: 'Admin', + tags: {}, + renderingId: 456, + permissionLevel: 'admin', + }), + }, + } as unknown as ReturnType); + + primeForestTokens(); + }); + + afterEach(() => { + jest.useRealTimers(); + jest.clearAllMocks(); + }); + + it('shortens both token lifetimes to the configured caps', async () => { + const provider = createProvider(undefined, undefined, { + accessTokenSeconds: 900, + refreshTokenSeconds: 86400, + }); + + const result = await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(signedTtl(1)).toBe(900); + expect(signedTtl(2)).toBe(86400); + expect(result.expires_in).toBe(900); + }); + + it('never extends beyond the lifetimes Forest Admin granted', async () => { + const provider = createProvider(undefined, undefined, { + accessTokenSeconds: 7200, + refreshTokenSeconds: 30 * 24 * 3600, + }); + + const result = await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(signedTtl(1)).toBe(FOREST_ACCESS_TTL); + expect(signedTtl(2)).toBe(FOREST_REFRESH_TTL); + expect(result.expires_in).toBe(FOREST_ACCESS_TTL); + }); + + it('leaves both lifetimes untouched when no cap is configured', async () => { + const provider = createProvider(); + + const result = await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(signedTtl(1)).toBe(FOREST_ACCESS_TTL); + expect(signedTtl(2)).toBe(FOREST_REFRESH_TTL); + expect(result.expires_in).toBe(FOREST_ACCESS_TTL); + }); + + it('stamps the session start on the refresh token issued at login', async () => { + const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); + + await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(signedPayload(2)).toEqual( + expect.objectContaining({ type: 'refresh', sessionStartedAt: nowInSeconds }), + ); + }); + + // Forest Admin grants a full refresh lifetime on every refresh, so without the anchor the + // window would slide forever and never force a re-login. + it('does not slide the refresh window when refreshing', async () => { + (jsonwebtoken.verify as jest.Mock).mockReturnValue({ + type: 'refresh', + clientId: 'test-client-id', + userId: 123, + renderingId: 456, + serverRefreshToken: 'forest-refresh-token', + sessionStartedAt: nowInSeconds - 80000, + }); + const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); + + await provider.exchangeRefreshToken(mockClient, 'our-refresh-token'); + + expect(signedTtl(2)).toBe(6400); + expect(signedPayload(2)).toEqual( + expect.objectContaining({ sessionStartedAt: nowInSeconds - 80000 }), + ); + }); + + it('rejects a refresh once the session window has elapsed, without calling Forest Admin', async () => { + (jsonwebtoken.verify as jest.Mock).mockReturnValue({ + type: 'refresh', + clientId: 'test-client-id', + userId: 123, + renderingId: 456, + serverRefreshToken: 'forest-refresh-token', + sessionStartedAt: nowInSeconds - 90000, + }); + const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); + + await expect(provider.exchangeRefreshToken(mockClient, 'our-refresh-token')).rejects.toThrow( + 'Refresh token has expired', + ); + expect(mockServer.fetch).not.toHaveBeenCalledWith( + expect.stringContaining('/oauth/token'), + expect.anything(), + ); + }); + + it('falls back to the issue date for a refresh token minted before the anchor existed', async () => { + (jsonwebtoken.verify as jest.Mock).mockReturnValue({ + type: 'refresh', + clientId: 'test-client-id', + userId: 123, + renderingId: 456, + serverRefreshToken: 'forest-refresh-token', + iat: nowInSeconds - 1000, + }); + const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); + + await provider.exchangeRefreshToken(mockClient, 'our-refresh-token'); + + expect(signedTtl(2)).toBe(85400); + }); + + // Pre-existing behaviour, deliberately untouched: a cap shortens a positive lifetime but can + // never drive it to zero, so it cannot make this branch reachable. + it('still advertises the 3600 fallback when the Forest token is already expired', async () => { + mockJwtDecode.mockReset(); + mockJwtDecode + .mockReturnValueOnce({ + meta: { renderingId: 456 }, + exp: nowInSeconds - 10, + iat: nowInSeconds - 3610, + scope: 'mcp:read', + }) + .mockReturnValueOnce({ exp: nowInSeconds + FOREST_REFRESH_TTL, iat: nowInSeconds }); + const provider = createProvider(); + + const result = await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(signedTtl(1)).toBe(-10); + expect(result.expires_in).toBe(3600); + }); + }); }); diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index 73e96e6798..3f6d7b3723 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -2825,6 +2825,95 @@ describe('agentUrl option', () => { }); }); +describe('tokenTtl option', () => { + const originalFetch = global.fetch; + const FOREST_SECRET = 'forest-signing-secret'; + let mockFetchServer: MockServer; + + afterEach(() => { + global.fetch = originalFetch; + mockFetchServer?.restoreSuperagent(); + }); + + function stubForestServer() { + mockFetchServer = new MockServer(); + mockFetchServer + .get('/liana/environment', { + data: { id: '1', attributes: { api_endpoint: 'https://api.example.com' } }, + }) + .get(/\/oauth\/register\//, { + client_id: 'test-client', + redirect_uris: ['https://example.com/callback'], + }) + .post('/oauth/token', { + access_token: jsonwebtoken.sign( + { meta: { renderingId: 456 }, scope: 'mcp:read' }, + FOREST_SECRET, + { expiresIn: 3600 }, + ), + refresh_token: jsonwebtoken.sign({}, FOREST_SECRET, { expiresIn: '7d' }), + expires_in: 3600, + token_type: 'Bearer', + scope: 'mcp:read', + }) + .get(/\/liana\/v2\/renderings\/\d+\/authorization/, { + data: { + id: '123', + attributes: { + email: 'user@example.com', + first_name: 'Test', + last_name: 'User', + teams: ['Operations'], + role: 'Admin', + permission_level: 'admin', + tags: [], + }, + }, + }); + global.fetch = mockFetchServer.fetch; + mockFetchServer.setupSuperagentMock(); + } + + it('shortens the issued access token down to the configured cap', async () => { + stubForestServer(); + const server = new ForestMCPServer({ + envSecret: 'ENV_SECRET', + authSecret: 'AUTH_SECRET', + forestServerClient: createMockForestServerClient(), + tokenTtl: { accessTokenSeconds: 120 }, + }); + const app = await server.buildExpressApp(new URL('http://localhost:3000')); + + const response = await request(app).post('/oauth/token').type('form').send({ + grant_type: 'authorization_code', + code: 'auth-code', + code_verifier: 'code-verifier', + client_id: 'test-client', + }); + + expect(response.status).toBe(200); + expect(response.body.expires_in).toBe(120); + + const { exp, iat } = jsonwebtoken.decode(response.body.access_token) as { + exp: number; + iat: number; + }; + expect(exp - iat).toBe(120); + }); + + it('rejects an invalid value at construction rather than issuing uncapped tokens', () => { + expect( + () => + new ForestMCPServer({ + envSecret: 'ENV_SECRET', + authSecret: 'AUTH_SECRET', + forestServerClient: createMockForestServerClient(), + tokenTtl: { accessTokenSeconds: 0 }, + }), + ).toThrow(/Invalid tokenTtl\.accessTokenSeconds/); + }); +}); + describe('handleMcpRequest cleanup', () => { const originalFetch = global.fetch; let cleanupServer: ForestMCPServer; diff --git a/packages/mcp-server/test/utils/token-ttl.test.ts b/packages/mcp-server/test/utils/token-ttl.test.ts new file mode 100644 index 0000000000..31850ea02f --- /dev/null +++ b/packages/mcp-server/test/utils/token-ttl.test.ts @@ -0,0 +1,133 @@ +import normalizeTokenTtl, { + MIN_TOKEN_TTL_SECONDS, + capAccessTokenTtl, + capRefreshTokenTtl, +} from '../../src/utils/token-ttl'; + +describe('normalizeTokenTtl', () => { + let logger: jest.Mock; + + beforeEach(() => { + logger = jest.fn(); + }); + + it.each([undefined, {}])('returns undefined for %p so nothing is capped', input => { + expect(normalizeTokenTtl(input, logger)).toBeUndefined(); + expect(logger).not.toHaveBeenCalled(); + }); + + it('returns valid values unchanged', () => { + expect( + normalizeTokenTtl({ accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, logger), + ).toEqual({ accessTokenSeconds: 900, refreshTokenSeconds: 86400 }); + expect(logger).not.toHaveBeenCalled(); + }); + + it('accepts a value far above the Forest lifetime, which the cap neutralizes', () => { + expect(normalizeTokenTtl({ accessTokenSeconds: 1e12 }, logger)).toEqual({ + accessTokenSeconds: 1e12, + refreshTokenSeconds: undefined, + }); + expect(logger).not.toHaveBeenCalled(); + }); + + it.each([0, -1, -3600, 3600.5, NaN, Infinity, -Infinity, '900' as unknown as number, null])( + 'throws for accessTokenSeconds %p', + value => { + expect(() => normalizeTokenTtl({ accessTokenSeconds: value }, logger)).toThrow( + /Invalid tokenTtl\.accessTokenSeconds/, + ); + }, + ); + + it.each([0, -1, -3600, 3600.5, NaN, Infinity, -Infinity, '900' as unknown as number, null])( + 'throws for refreshTokenSeconds %p', + value => { + expect(() => normalizeTokenTtl({ refreshTokenSeconds: value }, logger)).toThrow( + /Invalid tokenTtl\.refreshTokenSeconds/, + ); + }, + ); + + it('clamps a value below the minimum and warns', () => { + expect(normalizeTokenTtl({ accessTokenSeconds: 30 }, logger)).toEqual({ + accessTokenSeconds: MIN_TOKEN_TTL_SECONDS, + refreshTokenSeconds: undefined, + }); + expect(logger).toHaveBeenCalledWith( + 'Warn', + `ignoring tokenTtl.accessTokenSeconds: minimum value is ${MIN_TOKEN_TTL_SECONDS} seconds`, + ); + }); + + it('clamps at the boundary but leaves the minimum itself alone', () => { + expect(normalizeTokenTtl({ accessTokenSeconds: 59 }, logger)?.accessTokenSeconds).toBe(60); + expect(normalizeTokenTtl({ accessTokenSeconds: 60 }, logger)?.accessTokenSeconds).toBe(60); + expect(logger).toHaveBeenCalledTimes(1); + }); +}); + +describe('capAccessTokenTtl', () => { + it('returns the Forest lifetime when no cap is configured', () => { + expect(capAccessTokenTtl(3600)).toBe(3600); + }); + + it('shortens the lifetime to the cap', () => { + expect(capAccessTokenTtl(3600, 900)).toBe(900); + }); + + it('never extends beyond the Forest lifetime', () => { + expect(capAccessTokenTtl(900, 3600)).toBe(900); + }); + + it.each([0, -5])('does not rescue an already-expired Forest lifetime of %p', upstream => { + expect(capAccessTokenTtl(upstream, 900)).toBe(upstream); + }); +}); + +describe('capRefreshTokenTtl', () => { + const nowInSeconds = 1_000_000; + + it('returns the Forest lifetime when no cap is configured', () => { + expect( + capRefreshTokenTtl({ + upstreamTtlSeconds: 604800, + sessionStartedAt: nowInSeconds, + nowInSeconds, + }), + ).toBe(604800); + }); + + it('grants the full cap at the start of the session', () => { + expect( + capRefreshTokenTtl({ + upstreamTtlSeconds: 604800, + capSeconds: 86400, + sessionStartedAt: nowInSeconds, + nowInSeconds, + }), + ).toBe(86400); + }); + + it('shrinks as the session ages instead of sliding', () => { + expect( + capRefreshTokenTtl({ + upstreamTtlSeconds: 604800, + capSeconds: 86400, + sessionStartedAt: nowInSeconds - 80000, + nowInSeconds, + }), + ).toBe(6400); + }); + + it('never extends beyond the Forest lifetime', () => { + expect( + capRefreshTokenTtl({ + upstreamTtlSeconds: 3600, + capSeconds: 86400, + sessionStartedAt: nowInSeconds, + nowInSeconds, + }), + ).toBe(3600); + }); +}); From 5d85f21cc14727a4217c50289f8b4a669f554585 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 28 Jul 2026 14:48:19 +0200 Subject: [PATCH 02/15] fix(mcp-server): keep every credential inside the session window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both review findings, which share one cause: the session deadline was enforced at a single point in time and against a single token. An access token could outlive the session. generateAccessToken bounded it only by accessTokenSeconds and the Forest lifetime, so refreshing near the deadline handed out a token valid well past sessionStartedAt + refreshTokenSeconds — with a 24h session and a 1h access token, up to an hour of access without an interactive login. The deadline could also lapse mid-flight. exchangeRefreshToken checked it before the Forest round trips, then the TTL was recomputed after them, so a refresh started just before expiry signed a refresh token with a non-positive lifetime: an already-dead token returned as a success instead of a clean InvalidGrantError. The remaining session lifetime is now measured once, after those round trips, and bounds both tokens; a non-positive value rejects the grant. capRefreshTokenTtl collapses into sessionRemainingSeconds so the anchor arithmetic lives in one place, and capAccessTokenTtl becomes the generic capTtl. Also warns once per process when a configured value exceeds the lifetime Forest Admin granted. The min() silently neutralizes it, which reads as "my setting is ignored" — the same confusion raised while the option was being designed. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/agent.ts | 5 +- .../mcp-server/src/forest-oauth-provider.ts | 68 ++++++++--- .../mcp-server/src/utils/parse-token-ttl.ts | 3 +- packages/mcp-server/src/utils/token-ttl.ts | 27 ++--- packages/mcp-server/test/cli.test.ts | 2 - .../test/forest-oauth-provider.test.ts | 107 +++++++++++++++++- .../mcp-server/test/utils/token-ttl.test.ts | 51 ++++----- 7 files changed, 187 insertions(+), 76 deletions(-) diff --git a/packages/agent/src/agent.ts b/packages/agent/src/agent.ts index 4e20fcbd91..5ce4b30a53 100644 --- a/packages/agent/src/agent.ts +++ b/packages/agent/src/agent.ts @@ -258,9 +258,8 @@ export default class Agent extends FrameworkMounter * // OAuth discovery metadata stays at the origin root (prefix-suffixed), so root `.well-known` * // traffic must still reach the agent. * agent.mountAiMcpServer({ basePath: '/ai' }); - * // Example: shorten the OAuth token lifetimes. Both values are upper bounds — they can only - * // shorten what Forest Admin granted, so a value above its own TTL has no effect. - * // `refreshTokenSeconds` bounds the time between two interactive logins. + * // Example: shorten the OAuth token lifetimes. Both values can only shorten what Forest Admin + * // granted; `refreshTokenSeconds` bounds the time between two interactive logins. * agent.mountAiMcpServer({ tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 } }); */ mountAiMcpServer(options?: { diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index 9e91ce2db7..33a7038f02 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -25,7 +25,7 @@ import { } from '@modelcontextprotocol/sdk/server/auth/errors.js'; import jsonwebtoken from 'jsonwebtoken'; -import { capAccessTokenTtl, capRefreshTokenTtl } from './utils/token-ttl'; +import { capTtl, sessionRemainingSeconds } from './utils/token-ttl'; export interface ForestOAuthProviderOptions { forestServerUrl: string; @@ -51,6 +51,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { private environmentApiEndpoint?: string; private agentUrl?: string; private tokenTtl?: TokenTtlOptions; + private readonly fieldsWarnedAsInert = new Set(); private logger: Logger; constructor({ @@ -211,6 +212,8 @@ export default class ForestOAuthProvider implements OAuthServerProvider { codeVerifier?: string, redirectUri?: string, ): Promise { + const sessionStartedAt = Math.floor(Date.now() / 1000); + try { return await this.generateAccessToken( client, @@ -221,8 +224,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { client_id: client.client_id, code_verifier: codeVerifier, }, - // The only interactive entry point, so this is when the session starts. - Math.floor(Date.now() / 1000), + sessionStartedAt, ); } catch (error) { const message = error instanceof Error ? error.message : String(error); @@ -277,8 +279,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { throw new InvalidClientError('Token was not issued to this client'); } - // Carried over from the interactive login, never re-stamped. Tokens issued before this claim - // existed fall back to their issue date, which closes their window one refresh later. + // Carried over from the interactive login, never re-stamped. const nowInSeconds = Math.floor(Date.now() / 1000); const sessionStartedAt = decoded.sessionStartedAt ?? decoded.iat ?? nowInSeconds; const { refreshTokenSeconds } = this.tokenTtl ?? {}; @@ -319,6 +320,23 @@ export default class ForestOAuthProvider implements OAuthServerProvider { } } + private warnIfCapIsInert( + field: string, + capSeconds: number | undefined, + upstreamTtlSeconds: number, + ): void { + if (capSeconds === undefined || upstreamTtlSeconds <= 0) return; + if (capSeconds <= upstreamTtlSeconds || this.fieldsWarnedAsInert.has(field)) return; + + this.fieldsWarnedAsInert.add(field); + this.logger( + 'Warn', + `[ForestOAuthProvider] tokenTtl.${field}=${capSeconds} exceeds the ${upstreamTtlSeconds}s ` + + `lifetime granted by Forest Admin and has no effect: this option can only shorten a token ` + + `lifetime, never extend it.`, + ); + } + private async generateAccessToken( client: OAuthClientInformationFull, tokenPayload: Record, @@ -383,11 +401,33 @@ export default class ForestOAuthProvider implements OAuthServerProvider { forestServerAccessToken, ); - // Create new access token. Capping here rather than in the sign() options keeps the advertised - // `expires_in` below in step with the lifetime actually signed into the JWT. const nowInSeconds = Math.floor(Date.now() / 1000); - const expiresIn = capAccessTokenTtl( + this.warnIfCapIsInert( + 'accessTokenSeconds', + this.tokenTtl?.accessTokenSeconds, expirationDate - nowInSeconds, + ); + this.warnIfCapIsInert( + 'refreshTokenSeconds', + this.tokenTtl?.refreshTokenSeconds, + refreshTokenExpirationDate - nowInSeconds, + ); + + // Re-measured after the Forest round trips, which can themselves outlast the session. Bounds + // both tokens: an access token outliving the session would extend it by its own lifetime. + const sessionRemaining = sessionRemainingSeconds({ + refreshTokenSeconds: this.tokenTtl?.refreshTokenSeconds, + sessionStartedAt, + nowInSeconds, + }); + + if (sessionRemaining <= 0) { + throw new InvalidGrantError('Refresh token has expired'); + } + + // Capped here, not in the sign() options, so the `expires_in` returned below cannot drift. + const expiresIn = capTtl( + Math.min(expirationDate - nowInSeconds, sessionRemaining), this.tokenTtl?.accessTokenSeconds, ); const tokenScopes = scope ? scope.split(' ') : ['mcp:read', 'mcp:write', 'mcp:action']; @@ -415,21 +455,13 @@ export default class ForestOAuthProvider implements OAuthServerProvider { sessionStartedAt, }, this.authSecret, - { - expiresIn: capRefreshTokenTtl({ - upstreamTtlSeconds: refreshTokenExpirationDate - nowInSeconds, - capSeconds: this.tokenTtl?.refreshTokenSeconds, - sessionStartedAt, - nowInSeconds, - }), - }, + { expiresIn: Math.min(refreshTokenExpirationDate - nowInSeconds, sessionRemaining) }, ); return { access_token: accessToken, token_type: 'Bearer', - // Only reachable when the Forest token is already expired: a cap can shorten a positive - // lifetime but never drive it to zero. + // Only reachable when the Forest token is already expired; a cap cannot reach zero. expires_in: expiresIn > 0 ? expiresIn : 3600, refresh_token: refreshToken, scope: scope || client.scope, diff --git a/packages/mcp-server/src/utils/parse-token-ttl.ts b/packages/mcp-server/src/utils/parse-token-ttl.ts index 0268fda93b..2fd2b52a72 100644 --- a/packages/mcp-server/src/utils/parse-token-ttl.ts +++ b/packages/mcp-server/src/utils/parse-token-ttl.ts @@ -1,7 +1,6 @@ import type { TokenTtlOptions } from './token-ttl'; -// Coercion only — normalizeTokenTtl owns validation, so a bad env var and a bad embedded option -// fail with the same message. `Number('abc')` yields NaN, which it rejects. +// Coercion only: normalizeTokenTtl owns validation, so an env var and an embedded option fail alike. export default function parseTokenTtl( accessTokenSeconds?: string, refreshTokenSeconds?: string, diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts index f5a4fc71c8..5f4a035941 100644 --- a/packages/mcp-server/src/utils/token-ttl.ts +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -1,9 +1,7 @@ import type { Logger } from '../server'; -// The MCP SDK rate-limits /oauth/token to 50 requests per 15 minutes and server.ts does not -// override it, so a lifetime under ~18s makes a client exhaust its own token quota and get 429s. -// 60s also stays clear of clock drift between agent instances: verifyAccessToken and the SDK's -// bearer middleware both compare timestamps with a zero clock tolerance. +// The MCP SDK rate-limits /oauth/token to 50 requests per 15 minutes (one per 18s) and server.ts +// does not override it, so a shorter lifetime makes a client exhaust its own quota and take 429s. export const MIN_TOKEN_TTL_SECONDS = 60; export type TokenTtlOptions = { @@ -18,8 +16,6 @@ function normalizeSeconds( ): number | undefined { if (value === undefined) return undefined; - // Fail closed rather than falling back to a default: a silently ignored cap would leave the - // customer believing their tokens are short-lived when they are not. if (!Number.isInteger(value) || value <= 0) { throw new Error( `Invalid tokenTtl.${field} "${value}": it must be a positive integer number of seconds.`, @@ -55,25 +51,22 @@ export default function normalizeTokenTtl( return { accessTokenSeconds, refreshTokenSeconds }; } -export function capAccessTokenTtl(upstreamTtlSeconds: number, capSeconds?: number): number { +export function capTtl(upstreamTtlSeconds: number, capSeconds?: number): number { return capSeconds === undefined ? upstreamTtlSeconds : Math.min(upstreamTtlSeconds, capSeconds); } -// Anchored on the interactive login rather than on the current refresh: the Forest server grants a -// full refresh lifetime on every refresh, so a per-refresh cap would slide forever and never force -// a re-login. -export function capRefreshTokenTtl({ - upstreamTtlSeconds, - capSeconds, +// Anchored on the interactive login, not on the current refresh: Forest grants a full refresh +// lifetime on every refresh, so a per-refresh bound would slide forever and never force a re-login. +export function sessionRemainingSeconds({ + refreshTokenSeconds, sessionStartedAt, nowInSeconds, }: { - upstreamTtlSeconds: number; - capSeconds?: number; + refreshTokenSeconds?: number; sessionStartedAt: number; nowInSeconds: number; }): number { - if (capSeconds === undefined) return upstreamTtlSeconds; + if (refreshTokenSeconds === undefined) return Number.POSITIVE_INFINITY; - return Math.min(upstreamTtlSeconds, sessionStartedAt + capSeconds - nowInSeconds); + return sessionStartedAt + refreshTokenSeconds - nowInSeconds; } diff --git a/packages/mcp-server/test/cli.test.ts b/packages/mcp-server/test/cli.test.ts index 2a8b8d1c88..91e77777ab 100644 --- a/packages/mcp-server/test/cli.test.ts +++ b/packages/mcp-server/test/cli.test.ts @@ -43,8 +43,6 @@ describe('parseTokenTtl', () => { expect(parseTokenTtl(access, refresh)).toEqual(expected); }); - // Validation is deliberately deferred to normalizeTokenTtl, so an env var and an embedded option - // fail with the same message. it('leaves a non-numeric value as NaN for the normalizer to reject', () => { expect(parseTokenTtl('abc')?.accessTokenSeconds).toBeNaN(); }); diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index 53299c7d69..ca8107128f 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -1,3 +1,4 @@ +import type { Logger } from '../src/server'; import type { TokenTtlOptions } from '../src/utils/token-ttl'; import type { OAuthClientInformationFull } from '@modelcontextprotocol/sdk/shared/auth'; import type { Response } from 'express'; @@ -25,13 +26,14 @@ function createProvider( forestServerUrl = 'https://api.forestadmin.com', agentUrl?: string, tokenTtl?: TokenTtlOptions, + logger: Logger = console.info, ) { return new ForestOAuthProvider({ forestServerUrl, forestAppUrl: TEST_FOREST_APP_URL, envSecret: TEST_ENV_SECRET, authSecret: TEST_AUTH_SECRET, - logger: console.info, + logger, agentUrl, tokenTtl, }); @@ -1060,6 +1062,70 @@ describe('ForestOAuthProvider', () => { expect(result.expires_in).toBe(FOREST_ACCESS_TTL); }); + it('warns for each cap that exceeds the lifetime Forest Admin granted', async () => { + const logger = jest.fn(); + const provider = createProvider( + undefined, + undefined, + { accessTokenSeconds: 7200, refreshTokenSeconds: 30 * 24 * 3600 }, + logger, + ); + + await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(logger).toHaveBeenCalledWith( + 'Warn', + expect.stringContaining(`tokenTtl.accessTokenSeconds=7200 exceeds the 3600s`), + ); + expect(logger).toHaveBeenCalledWith( + 'Warn', + expect.stringContaining(`tokenTtl.refreshTokenSeconds=2592000 exceeds the 604800s`), + ); + }); + + it('warns once per field instead of on every token issuance', async () => { + const logger = jest.fn(); + const provider = createProvider(undefined, undefined, { accessTokenSeconds: 7200 }, logger); + + await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + primeForestTokens(); + await provider.exchangeAuthorizationCode(mockClient, 'auth-code-456'); + + expect(logger).toHaveBeenCalledTimes(1); + }); + + it('stays silent when the caps are effective', async () => { + const logger = jest.fn(); + const provider = createProvider( + undefined, + undefined, + { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, + logger, + ); + + await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(logger).not.toHaveBeenCalled(); + }); + + it('stays silent when the Forest token is already expired', async () => { + const logger = jest.fn(); + mockJwtDecode.mockReset(); + mockJwtDecode + .mockReturnValueOnce({ + meta: { renderingId: 456 }, + exp: nowInSeconds - 10, + iat: nowInSeconds - 3610, + scope: 'mcp:read', + }) + .mockReturnValueOnce({ exp: nowInSeconds + FOREST_REFRESH_TTL, iat: nowInSeconds }); + const provider = createProvider(undefined, undefined, { accessTokenSeconds: 7200 }, logger); + + await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(logger).not.toHaveBeenCalled(); + }); + it('leaves both lifetimes untouched when no cap is configured', async () => { const provider = createProvider(); @@ -1080,8 +1146,6 @@ describe('ForestOAuthProvider', () => { ); }); - // Forest Admin grants a full refresh lifetime on every refresh, so without the anchor the - // window would slide forever and never force a re-login. it('does not slide the refresh window when refreshing', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', @@ -1101,6 +1165,41 @@ describe('ForestOAuthProvider', () => { ); }); + it('keeps the access token inside the session window', async () => { + (jsonwebtoken.verify as jest.Mock).mockReturnValue({ + type: 'refresh', + clientId: 'test-client-id', + userId: 123, + renderingId: 456, + serverRefreshToken: 'forest-refresh-token', + sessionStartedAt: nowInSeconds - 86340, + }); + const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); + + const result = await provider.exchangeRefreshToken(mockClient, 'our-refresh-token'); + + expect(signedTtl(1)).toBe(60); + expect(result.expires_in).toBe(60); + }); + + it('rejects when the session elapses while talking to Forest Admin', async () => { + (jsonwebtoken.verify as jest.Mock).mockReturnValue({ + type: 'refresh', + clientId: 'test-client-id', + userId: 123, + renderingId: 456, + serverRefreshToken: 'forest-refresh-token', + sessionStartedAt: nowInSeconds - 86399, + }); + const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); + + const pending = provider.exchangeRefreshToken(mockClient, 'our-refresh-token'); + jest.setSystemTime(NOW_MS + 5000); + + await expect(pending).rejects.toThrow('Refresh token has expired'); + expect(mockJwtSign).not.toHaveBeenCalled(); + }); + it('rejects a refresh once the session window has elapsed, without calling Forest Admin', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', @@ -1137,8 +1236,6 @@ describe('ForestOAuthProvider', () => { expect(signedTtl(2)).toBe(85400); }); - // Pre-existing behaviour, deliberately untouched: a cap shortens a positive lifetime but can - // never drive it to zero, so it cannot make this branch reachable. it('still advertises the 3600 fallback when the Forest token is already expired', async () => { mockJwtDecode.mockReset(); mockJwtDecode diff --git a/packages/mcp-server/test/utils/token-ttl.test.ts b/packages/mcp-server/test/utils/token-ttl.test.ts index 31850ea02f..d5842df4db 100644 --- a/packages/mcp-server/test/utils/token-ttl.test.ts +++ b/packages/mcp-server/test/utils/token-ttl.test.ts @@ -1,7 +1,7 @@ import normalizeTokenTtl, { MIN_TOKEN_TTL_SECONDS, - capAccessTokenTtl, - capRefreshTokenTtl, + capTtl, + sessionRemainingSeconds, } from '../../src/utils/token-ttl'; describe('normalizeTokenTtl', () => { @@ -67,42 +67,37 @@ describe('normalizeTokenTtl', () => { }); }); -describe('capAccessTokenTtl', () => { +describe('capTtl', () => { it('returns the Forest lifetime when no cap is configured', () => { - expect(capAccessTokenTtl(3600)).toBe(3600); + expect(capTtl(3600)).toBe(3600); }); it('shortens the lifetime to the cap', () => { - expect(capAccessTokenTtl(3600, 900)).toBe(900); + expect(capTtl(3600, 900)).toBe(900); }); it('never extends beyond the Forest lifetime', () => { - expect(capAccessTokenTtl(900, 3600)).toBe(900); + expect(capTtl(900, 3600)).toBe(900); }); it.each([0, -5])('does not rescue an already-expired Forest lifetime of %p', upstream => { - expect(capAccessTokenTtl(upstream, 900)).toBe(upstream); + expect(capTtl(upstream, 900)).toBe(upstream); }); }); -describe('capRefreshTokenTtl', () => { +describe('sessionRemainingSeconds', () => { const nowInSeconds = 1_000_000; - it('returns the Forest lifetime when no cap is configured', () => { - expect( - capRefreshTokenTtl({ - upstreamTtlSeconds: 604800, - sessionStartedAt: nowInSeconds, - nowInSeconds, - }), - ).toBe(604800); + it('is unbounded when no refresh cap is configured', () => { + expect(sessionRemainingSeconds({ sessionStartedAt: nowInSeconds, nowInSeconds })).toBe( + Infinity, + ); }); - it('grants the full cap at the start of the session', () => { + it('is the full cap at the start of the session', () => { expect( - capRefreshTokenTtl({ - upstreamTtlSeconds: 604800, - capSeconds: 86400, + sessionRemainingSeconds({ + refreshTokenSeconds: 86400, sessionStartedAt: nowInSeconds, nowInSeconds, }), @@ -111,23 +106,21 @@ describe('capRefreshTokenTtl', () => { it('shrinks as the session ages instead of sliding', () => { expect( - capRefreshTokenTtl({ - upstreamTtlSeconds: 604800, - capSeconds: 86400, + sessionRemainingSeconds({ + refreshTokenSeconds: 86400, sessionStartedAt: nowInSeconds - 80000, nowInSeconds, }), ).toBe(6400); }); - it('never extends beyond the Forest lifetime', () => { + it('goes non-positive once the session is over', () => { expect( - capRefreshTokenTtl({ - upstreamTtlSeconds: 3600, - capSeconds: 86400, - sessionStartedAt: nowInSeconds, + sessionRemainingSeconds({ + refreshTokenSeconds: 86400, + sessionStartedAt: nowInSeconds - 90000, nowInSeconds, }), - ).toBe(3600); + ).toBe(-3600); }); }); From cb4afc4f585295d93529b734c1d2067337cc7945 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 28 Jul 2026 16:13:36 +0200 Subject: [PATCH 03/15] fix(mcp-server): return invalid_grant when a session lapses mid-refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up, three defects. An expired session raised the wrong OAuth error code depending on timing. The in-flight check throws InvalidGrantError from inside generateAccessToken, but exchangeRefreshToken's catch rewrapped everything as InvalidRequestError, so the same decision surfaced as invalid_grant when caught pre-flight and invalid_request when the Forest round trips outlasted the session. Only invalid_grant makes the client drop its refresh token and re-run the interactive login, so in the window this check exists to close the user got a dead end instead of the re-login the option promises. The passthrough is narrowed to InvalidGrantError rather than OAuthError, leaving Forest's own error surface untouched. The test asserted the message by substring and passed on the wrong class; it now asserts the class. `refreshTokenSeconds` was reported as having no effect when it exceeded one Forest refresh lifetime. It has no inert case: it bounds the whole session, which Forest re-extends on every refresh, so any value binds however large. Warning about it told operators to delete a working control. Only the access cap can be inert. The clamp log said "ignoring" while applying the 60s minimum — the opposite reading, since the result is a 60s lifetime rather than an uncapped one. Also pins the anchor's precedence: flipping `sessionStartedAt ?? iat` passed the whole suite because each test supplied only one claim, while every real refresh token carries both. That mutation is the silent no-op the anchor exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) --- packages/mcp-server/README.md | 2 +- .../mcp-server/src/forest-oauth-provider.ts | 29 +++++++------ .../mcp-server/src/utils/parse-token-ttl.ts | 1 - packages/mcp-server/src/utils/token-ttl.ts | 10 +++-- .../test/forest-oauth-provider.test.ts | 43 ++++++++++++------- .../mcp-server/test/utils/token-ttl.test.ts | 3 +- 6 files changed, 53 insertions(+), 35 deletions(-) diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index b87e745a7c..e23f58c264 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -113,7 +113,7 @@ See [Available Tools](#available-tools) for the full list. `describeCollection` ## Shorten Token Lifetimes -**Both values are upper bounds: they can only shorten the lifetime Forest Admin granted, never extend it.** A value above Forest Admin's own lifetime therefore has no effect. +**Both values are upper bounds: they can only shorten what Forest Admin granted, never extend it.** For `accessTokenSeconds` that means a value above Forest Admin's own token lifetime has no effect. `refreshTokenSeconds` bounds the whole session, which Forest Admin otherwise re-extends on every refresh, so any value shortens it however large it is. ```typescript // With Forest Admin Agent diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index 33a7038f02..79cd191812 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -227,6 +227,8 @@ export default class ForestOAuthProvider implements OAuthServerProvider { sessionStartedAt, ); } catch (error) { + if (error instanceof InvalidGrantError) throw error; + const message = error instanceof Error ? error.message : String(error); throw new InvalidRequestError(`Failed to exchange authorization code: ${message}`); } @@ -279,15 +281,14 @@ export default class ForestOAuthProvider implements OAuthServerProvider { throw new InvalidClientError('Token was not issued to this client'); } - // Carried over from the interactive login, never re-stamped. const nowInSeconds = Math.floor(Date.now() / 1000); + // `sessionStartedAt` is stamped at the interactive login; a token predating the claim falls back + // to its own issue date. Never re-stamped, or the window would slide and never force a re-login. const sessionStartedAt = decoded.sessionStartedAt ?? decoded.iat ?? nowInSeconds; const { refreshTokenSeconds } = this.tokenTtl ?? {}; - if ( - refreshTokenSeconds !== undefined && - sessionStartedAt + refreshTokenSeconds <= nowInSeconds - ) { + // Checked again after the Forest round trips; this one only spares them when already expired. + if (sessionRemainingSeconds({ refreshTokenSeconds, sessionStartedAt, nowInSeconds }) <= 0) { this.logger( 'Error', `[ForestOAuthProvider] Session exceeded tokenTtl.refreshTokenSeconds=${refreshTokenSeconds}: ` + @@ -309,6 +310,10 @@ export default class ForestOAuthProvider implements OAuthServerProvider { sessionStartedAt, ); } catch (error) { + // Only `invalid_grant` tells the client to drop its refresh token and re-run the interactive + // login, so a session that lapsed mid-flight must keep its own error code. + if (error instanceof InvalidGrantError) throw error; + const message = error instanceof Error ? error.message : String(error); const errorName = error instanceof Error ? error.name : 'Unknown'; this.logger( @@ -402,19 +407,16 @@ export default class ForestOAuthProvider implements OAuthServerProvider { ); const nowInSeconds = Math.floor(Date.now() / 1000); + // Only the access cap can be inert. `refreshTokenSeconds` bounds the whole session, which Forest + // otherwise re-extends on every refresh, so any value binds however large it is. this.warnIfCapIsInert( 'accessTokenSeconds', this.tokenTtl?.accessTokenSeconds, expirationDate - nowInSeconds, ); - this.warnIfCapIsInert( - 'refreshTokenSeconds', - this.tokenTtl?.refreshTokenSeconds, - refreshTokenExpirationDate - nowInSeconds, - ); - // Re-measured after the Forest round trips, which can themselves outlast the session. Bounds - // both tokens: an access token outliving the session would extend it by its own lifetime. + // Re-read after the Forest round trips: they can outlast what is left of the session. Bounds the + // access token too, which would otherwise extend the session by its own lifetime. const sessionRemaining = sessionRemainingSeconds({ refreshTokenSeconds: this.tokenTtl?.refreshTokenSeconds, sessionStartedAt, @@ -425,7 +427,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { throw new InvalidGrantError('Refresh token has expired'); } - // Capped here, not in the sign() options, so the `expires_in` returned below cannot drift. + // Capped here rather than in the sign() options, so `expires_in` below reports the same value. const expiresIn = capTtl( Math.min(expirationDate - nowInSeconds, sessionRemaining), this.tokenTtl?.accessTokenSeconds, @@ -461,7 +463,6 @@ export default class ForestOAuthProvider implements OAuthServerProvider { return { access_token: accessToken, token_type: 'Bearer', - // Only reachable when the Forest token is already expired; a cap cannot reach zero. expires_in: expiresIn > 0 ? expiresIn : 3600, refresh_token: refreshToken, scope: scope || client.scope, diff --git a/packages/mcp-server/src/utils/parse-token-ttl.ts b/packages/mcp-server/src/utils/parse-token-ttl.ts index 2fd2b52a72..36963ed6c0 100644 --- a/packages/mcp-server/src/utils/parse-token-ttl.ts +++ b/packages/mcp-server/src/utils/parse-token-ttl.ts @@ -1,6 +1,5 @@ import type { TokenTtlOptions } from './token-ttl'; -// Coercion only: normalizeTokenTtl owns validation, so an env var and an embedded option fail alike. export default function parseTokenTtl( accessTokenSeconds?: string, refreshTokenSeconds?: string, diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts index 5f4a035941..d07d4494e4 100644 --- a/packages/mcp-server/src/utils/token-ttl.ts +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -1,7 +1,7 @@ import type { Logger } from '../server'; -// The MCP SDK rate-limits /oauth/token to 50 requests per 15 minutes (one per 18s) and server.ts -// does not override it, so a shorter lifetime makes a client exhaust its own quota and take 429s. +// The MCP SDK rate-limits /oauth/token to 50 requests per 15 minutes (one per 18s) per IP and +// server.ts does not override it. 60s leaves margin for clients sharing an egress IP. export const MIN_TOKEN_TTL_SECONDS = 60; export type TokenTtlOptions = { @@ -23,7 +23,11 @@ function normalizeSeconds( } if (value < MIN_TOKEN_TTL_SECONDS) { - logger('Warn', `ignoring tokenTtl.${field}: minimum value is ${MIN_TOKEN_TTL_SECONDS} seconds`); + logger( + 'Warn', + `[ForestOAuthProvider] raising tokenTtl.${field} from ${value} to the ` + + `${MIN_TOKEN_TTL_SECONDS}s minimum`, + ); return MIN_TOKEN_TTL_SECONDS; } diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index ca8107128f..07480bd579 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -4,6 +4,7 @@ import type { OAuthClientInformationFull } from '@modelcontextprotocol/sdk/share import type { Response } from 'express'; import createForestAdminClient from '@forestadmin/forestadmin-client'; +import { InvalidGrantError } from '@modelcontextprotocol/sdk/server/auth/errors.js'; import jsonwebtoken from 'jsonwebtoken'; import MockServer from './test-utils/mock-server'; @@ -1062,14 +1063,9 @@ describe('ForestOAuthProvider', () => { expect(result.expires_in).toBe(FOREST_ACCESS_TTL); }); - it('warns for each cap that exceeds the lifetime Forest Admin granted', async () => { + it('warns when the access cap exceeds the lifetime Forest Admin granted', async () => { const logger = jest.fn(); - const provider = createProvider( - undefined, - undefined, - { accessTokenSeconds: 7200, refreshTokenSeconds: 30 * 24 * 3600 }, - logger, - ); + const provider = createProvider(undefined, undefined, { accessTokenSeconds: 7200 }, logger); await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); @@ -1077,10 +1073,22 @@ describe('ForestOAuthProvider', () => { 'Warn', expect.stringContaining(`tokenTtl.accessTokenSeconds=7200 exceeds the 3600s`), ); - expect(logger).toHaveBeenCalledWith( - 'Warn', - expect.stringContaining(`tokenTtl.refreshTokenSeconds=2592000 exceeds the 604800s`), + }); + + // A refresh cap bounds the whole session, which Forest re-extends on every refresh, so a value + // above one token's lifetime still forces a re-login and must not be reported as inert. + it('stays silent when the refresh cap exceeds one Forest refresh lifetime', async () => { + const logger = jest.fn(); + const provider = createProvider( + undefined, + undefined, + { refreshTokenSeconds: 30 * 24 * 3600 }, + logger, ); + + await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(logger).not.toHaveBeenCalled(); }); it('warns once per field instead of on every token issuance', async () => { @@ -1146,7 +1154,9 @@ describe('ForestOAuthProvider', () => { ); }); - it('does not slide the refresh window when refreshing', async () => { + // Every real refresh token carries both claims, `iat` being the mint time of that refresh. The + // anchor must be the stamped session start, or the window slides on every refresh. + it('anchors on the stamped session start, not on the refresh token issue date', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', clientId: 'test-client-id', @@ -1154,6 +1164,7 @@ describe('ForestOAuthProvider', () => { renderingId: 456, serverRefreshToken: 'forest-refresh-token', sessionStartedAt: nowInSeconds - 80000, + iat: nowInSeconds - 300, }); const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); @@ -1193,10 +1204,12 @@ describe('ForestOAuthProvider', () => { }); const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); + // The pre-flight check passes with 1s left, then the round trips outlast the session. const pending = provider.exchangeRefreshToken(mockClient, 'our-refresh-token'); jest.setSystemTime(NOW_MS + 5000); - await expect(pending).rejects.toThrow('Refresh token has expired'); + // Only invalid_grant makes the client re-run the interactive login. + await expect(pending).rejects.toBeInstanceOf(InvalidGrantError); expect(mockJwtSign).not.toHaveBeenCalled(); }); @@ -1211,9 +1224,9 @@ describe('ForestOAuthProvider', () => { }); const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); - await expect(provider.exchangeRefreshToken(mockClient, 'our-refresh-token')).rejects.toThrow( - 'Refresh token has expired', - ); + await expect( + provider.exchangeRefreshToken(mockClient, 'our-refresh-token'), + ).rejects.toBeInstanceOf(InvalidGrantError); expect(mockServer.fetch).not.toHaveBeenCalledWith( expect.stringContaining('/oauth/token'), expect.anything(), diff --git a/packages/mcp-server/test/utils/token-ttl.test.ts b/packages/mcp-server/test/utils/token-ttl.test.ts index d5842df4db..dee90c508c 100644 --- a/packages/mcp-server/test/utils/token-ttl.test.ts +++ b/packages/mcp-server/test/utils/token-ttl.test.ts @@ -56,7 +56,8 @@ describe('normalizeTokenTtl', () => { }); expect(logger).toHaveBeenCalledWith( 'Warn', - `ignoring tokenTtl.accessTokenSeconds: minimum value is ${MIN_TOKEN_TTL_SECONDS} seconds`, + `[ForestOAuthProvider] raising tokenTtl.accessTokenSeconds from 30 to the ` + + `${MIN_TOKEN_TTL_SECONDS}s minimum`, ); }); From de43f319ed42bb99a2ddac9475113efbd1eea482 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 28 Jul 2026 16:18:29 +0200 Subject: [PATCH 04/15] docs(mcp-server): state the lifetimes Forest Admin grants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without the numbers a reader cannot tell whether their value will bind. Added to the JSDoc on both the option and the type — that is the copy people read in their IDE — plus the env table and the README section. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/agent.ts | 5 +++-- packages/mcp-server/README.md | 8 +++++--- packages/mcp-server/src/server.ts | 7 ++++--- packages/mcp-server/src/utils/token-ttl.ts | 2 ++ 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/agent/src/agent.ts b/packages/agent/src/agent.ts index 5ce4b30a53..d25c58e833 100644 --- a/packages/agent/src/agent.ts +++ b/packages/agent/src/agent.ts @@ -258,8 +258,9 @@ export default class Agent extends FrameworkMounter * // OAuth discovery metadata stays at the origin root (prefix-suffixed), so root `.well-known` * // traffic must still reach the agent. * agent.mountAiMcpServer({ basePath: '/ai' }); - * // Example: shorten the OAuth token lifetimes. Both values can only shorten what Forest Admin - * // granted; `refreshTokenSeconds` bounds the time between two interactive logins. + * // Example: shorten the OAuth token lifetimes. Forest Admin grants 1h for an access token and + * // 8 days for a refresh token; these only shorten that. `refreshTokenSeconds` bounds the time + * // between two interactive logins. * agent.mountAiMcpServer({ tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 } }); */ mountAiMcpServer(options?: { diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index e23f58c264..4c46222c07 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -64,8 +64,8 @@ yarn start:dev # Development (loads .env file automatically) | `MCP_SERVER_PORT` | No | `3931` | Port for the HTTP server | | `FOREST_MCP_ENABLED_TOOLS` | No | - | Comma-separated list of tools to enable (allowlist) | | `FOREST_AGENT_URL` | No | your environment's back-end URL | URL the MCP server uses to reach the back-end's data layer. Set it when the server runs next to a self-hosted back-end at an internal address (e.g. `http://localhost:3310`), instead of the public URL registered in Forest | -| `FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS` | No | Forest Admin's own lifetime | Maximum lifetime of the OAuth access tokens the server issues (`tokenTtl.accessTokenSeconds`). Minimum `60` | -| `FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS` | No | Forest Admin's own lifetime | Maximum time between two interactive logins (`tokenTtl.refreshTokenSeconds`). Minimum `60` | +| `FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS` | No | `3600` (1 hour) | Maximum lifetime of the OAuth access tokens the server issues (`tokenTtl.accessTokenSeconds`). Minimum `60` | +| `FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS` | No | `691200` (8 days) | Maximum time between two interactive logins (`tokenTtl.refreshTokenSeconds`). Minimum `60` | #### Example Configuration @@ -113,7 +113,9 @@ See [Available Tools](#available-tools) for the full list. `describeCollection` ## Shorten Token Lifetimes -**Both values are upper bounds: they can only shorten what Forest Admin granted, never extend it.** For `accessTokenSeconds` that means a value above Forest Admin's own token lifetime has no effect. `refreshTokenSeconds` bounds the whole session, which Forest Admin otherwise re-extends on every refresh, so any value shortens it however large it is. +Forest Admin grants **1 hour** (3600s) for an access token and **8 days** (691200s) for a refresh token. + +**Both values are upper bounds: they can only shorten that, never extend it.** For `accessTokenSeconds` a value above 3600 therefore has no effect. `refreshTokenSeconds` bounds the whole session, which Forest Admin otherwise re-extends on every refresh, so any value shortens it however large it is. ```typescript // With Forest Admin Agent diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index 99d995285a..be45fcf228 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -142,9 +142,10 @@ export interface ForestMCPServerOptions { */ agentDispatcher?: InProcessAgentDispatcher; /** - * Upper bounds on the OAuth token lifetimes this server issues. Each value can only shorten the - * lifetime Forest Admin granted, never extend it, so a value above Forest Admin's own TTL has no - * effect. `refreshTokenSeconds` bounds the time between two interactive logins. + * Upper bounds on the OAuth token lifetimes this server issues. Forest Admin grants 1 hour + * (3600s) for an access token and 8 days (691200s) for a refresh token; each value can only + * shorten that, never extend it. `refreshTokenSeconds` bounds the time between two interactive + * logins. Minimum 60s for either. */ tokenTtl?: TokenTtlOptions; } diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts index d07d4494e4..0ad7e56776 100644 --- a/packages/mcp-server/src/utils/token-ttl.ts +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -5,7 +5,9 @@ import type { Logger } from '../server'; export const MIN_TOKEN_TTL_SECONDS = 60; export type TokenTtlOptions = { + /** Caps the access token lifetime. Forest Admin grants 3600s (1 hour). */ accessTokenSeconds?: number; + /** Caps the time between two interactive logins. Forest Admin grants 691200s (8 days). */ refreshTokenSeconds?: number; }; From 3c1a4ee3cd862779a2e6a9c7b8ccf15deeb0b61f Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 28 Jul 2026 16:30:27 +0200 Subject: [PATCH 05/15] fix(mcp-server): propagate OAuth codes and reject empty TTL env vars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to @matthv's review. Widened the error passthrough from InvalidGrantError to OAuthError. The catch was flattening every Forest error into invalid_request, the one code the SDK client treats as unrecoverable (client/auth.js:150-162 retries only on invalid_grant, invalid_client and unauthorized_client, and maps the wire code back to a class). So a session Forest itself revoked — user removed from the project, token revoked — hard-failed forever without ever offering a login. Six tests asserted that flattening; they now assert the propagated code. Kept InvalidRequestError for the non-OAuth residue: ServerError reads better per RFC but the client recovers from neither, so it buys nothing here. An env var set to an empty string was indistinguishable from unset, silently leaving that token uncapped — worst when only one of the two is empty, since the operator sees short access tokens working and assumes both took effect. It now coerces to NaN and is rejected, matching what the README already promised. parseTokenTtl takes an object: swapping two adjacent positional strings compiled, type-checked and applied the refresh cap to access tokens. The refresh claims are now a named type used by both the sign() payload and the verify() cast, so renaming one side is a compile error instead of a silent fallback to iat — which is re-stamped on every rotation and would slide the window forever. Log levels were inverted: a session reaching its limit is this option working, not an error, and with the documented 86400 it fired once per user per day. It drops to Info, while the genuinely anomalous branch (round trips outlasting the session) now warns with the elapsed time, the only way to tell it from a plain expiry. Docs: the refresh default is unbounded, not 8 days — Forest re-grants its lifetime on every refresh, so unset means never signing in again. Also documents that tokens predating the option anchor on their last refresh for one longer session. Co-Authored-By: Claude Opus 5 (1M context) --- packages/mcp-server/README.md | 6 +- packages/mcp-server/src/cli.ts | 8 +- .../mcp-server/src/forest-oauth-provider.ts | 76 +++++++++++-------- .../mcp-server/src/utils/parse-token-ttl.ts | 26 +++++-- packages/mcp-server/src/utils/token-ttl.ts | 2 +- packages/mcp-server/test/cli.test.ts | 40 ++++++---- .../test/forest-oauth-provider.test.ts | 11 +-- packages/mcp-server/test/server.test.ts | 30 +++++--- 8 files changed, 124 insertions(+), 75 deletions(-) diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index 4c46222c07..5f943279c8 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -65,7 +65,7 @@ yarn start:dev # Development (loads .env file automatically) | `FOREST_MCP_ENABLED_TOOLS` | No | - | Comma-separated list of tools to enable (allowlist) | | `FOREST_AGENT_URL` | No | your environment's back-end URL | URL the MCP server uses to reach the back-end's data layer. Set it when the server runs next to a self-hosted back-end at an internal address (e.g. `http://localhost:3310`), instead of the public URL registered in Forest | | `FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS` | No | `3600` (1 hour) | Maximum lifetime of the OAuth access tokens the server issues (`tokenTtl.accessTokenSeconds`). Minimum `60` | -| `FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS` | No | `691200` (8 days) | Maximum time between two interactive logins (`tokenTtl.refreshTokenSeconds`). Minimum `60` | +| `FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS` | No | unbounded | Maximum time between two interactive logins (`tokenTtl.refreshTokenSeconds`). Unset, a client that keeps refreshing never signs in again. Minimum `60` | #### Example Configuration @@ -113,7 +113,7 @@ See [Available Tools](#available-tools) for the full list. `describeCollection` ## Shorten Token Lifetimes -Forest Admin grants **1 hour** (3600s) for an access token and **8 days** (691200s) for a refresh token. +Forest Admin grants **1 hour** (3600s) for an access token and **8 days** (691200s) for a refresh token — but it re-grants those 8 days on *every* refresh, so without `refreshTokenSeconds` a client that keeps working is never asked to sign in again. **Both values are upper bounds: they can only shorten that, never extend it.** For `accessTokenSeconds` a value above 3600 therefore has no effect. `refreshTokenSeconds` bounds the whole session, which Forest Admin otherwise re-extends on every refresh, so any value shortens it however large it is. @@ -134,7 +134,7 @@ npx forest-mcp-server The two settings differ in what the user notices: - `accessTokenSeconds` shortens how long a leaked access token stays usable. It is transparent — the AI assistant silently obtains a new one. -- `refreshTokenSeconds` bounds the time between two **interactive logins**: once it elapses, the assistant can no longer refresh and the user signs in through the browser again. It is measured from the login itself, so an active assistant cannot keep extending its session. +- `refreshTokenSeconds` bounds the time between two **interactive logins**: once it elapses, the assistant can no longer refresh and the user signs in through the browser again. It is measured from the login itself, so an active assistant cannot keep extending its session. Refresh tokens issued before you enabled the option carry no login timestamp, so their window is measured from their last refresh instead — one longer session each, then bounded. The minimum for either value is 60 seconds; anything lower is raised to it. An invalid value (zero, negative, fractional) fails at startup rather than silently leaving the tokens uncapped. diff --git a/packages/mcp-server/src/cli.ts b/packages/mcp-server/src/cli.ts index ed0bef7691..f0ec7c2cde 100644 --- a/packages/mcp-server/src/cli.ts +++ b/packages/mcp-server/src/cli.ts @@ -12,10 +12,10 @@ const server = new ForestMCPServer({ authSecret: process.env.FOREST_AUTH_SECRET, enabledTools: parseToolList(process.env.FOREST_MCP_ENABLED_TOOLS), agentUrl: process.env.FOREST_AGENT_URL, - tokenTtl: parseTokenTtl( - process.env.FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS, - process.env.FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS, - ), + tokenTtl: parseTokenTtl({ + accessTokenSeconds: process.env.FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS, + refreshTokenSeconds: process.env.FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS, + }), }); server.run().catch(error => { diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index 79cd191812..69ffcb5031 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -21,12 +21,25 @@ import { InvalidGrantError, InvalidRequestError, InvalidTokenError, + OAuthError, UnsupportedTokenTypeError, } from '@modelcontextprotocol/sdk/server/auth/errors.js'; import jsonwebtoken from 'jsonwebtoken'; import { capTtl, sessionRemainingSeconds } from './utils/token-ttl'; +// Used by both the sign() payload and the verify() cast, so renaming a claim on one side is a +// compile error rather than a silent fallback to `iat` — which is re-stamped on every rotation and +// would slide the session window forever. +interface RefreshTokenClaims { + type: 'refresh'; + clientId: string; + userId: number; + renderingId: number; + serverRefreshToken: string; + sessionStartedAt?: number; +} + export interface ForestOAuthProviderOptions { forestServerUrl: string; forestAppUrl: string; @@ -227,7 +240,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { sessionStartedAt, ); } catch (error) { - if (error instanceof InvalidGrantError) throw error; + if (error instanceof OAuthError) throw error; const message = error instanceof Error ? error.message : String(error); throw new InvalidRequestError(`Failed to exchange authorization code: ${message}`); @@ -240,15 +253,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { scopes?: string[], ): Promise { // Verify and decode the refresh token - let decoded: { - type: string; - clientId: string; - userId: number; - renderingId: number; - serverRefreshToken: string; - sessionStartedAt?: number; - iat?: number; - }; + let decoded: RefreshTokenClaims & { iat?: number }; try { decoded = jsonwebtoken.verify(refreshToken, this.authSecret) as typeof decoded; @@ -289,10 +294,13 @@ export default class ForestOAuthProvider implements OAuthServerProvider { // Checked again after the Forest round trips; this one only spares them when already expired. if (sessionRemainingSeconds({ refreshTokenSeconds, sessionStartedAt, nowInSeconds }) <= 0) { + // Info, not Error: the session reaching its configured limit is this option working. With the + // documented 86400 it fires once per user per day. this.logger( - 'Error', - `[ForestOAuthProvider] Session exceeded tokenTtl.refreshTokenSeconds=${refreshTokenSeconds}: ` + - `sessionStartedAt=${sessionStartedAt}, clientId=${client.client_id}, userId=${decoded.userId}`, + 'Info', + `[ForestOAuthProvider] Session reached tokenTtl.refreshTokenSeconds=${refreshTokenSeconds}, ` + + `re-authentication required: sessionStartedAt=${sessionStartedAt}, ` + + `clientId=${client.client_id}, userId=${decoded.userId}`, ); throw new InvalidGrantError('Refresh token has expired'); } @@ -310,9 +318,9 @@ export default class ForestOAuthProvider implements OAuthServerProvider { sessionStartedAt, ); } catch (error) { - // Only `invalid_grant` tells the client to drop its refresh token and re-run the interactive - // login, so a session that lapsed mid-flight must keep its own error code. - if (error instanceof InvalidGrantError) throw error; + // Preserve the OAuth code: the client only re-authenticates on invalid_grant (and + // invalid_client), and invalid_request is the one code it treats as unrecoverable. + if (error instanceof OAuthError) throw error; const message = error instanceof Error ? error.message : String(error); const errorName = error instanceof Error ? error.name : 'Unknown'; @@ -326,7 +334,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { } private warnIfCapIsInert( - field: string, + field: keyof TokenTtlOptions, capSeconds: number | undefined, upstreamTtlSeconds: number, ): void { @@ -424,10 +432,19 @@ export default class ForestOAuthProvider implements OAuthServerProvider { }); if (sessionRemaining <= 0) { + // Unlike the caller's pre-flight check, reaching here means the round trips themselves + // outlasted the session — the elapsed time is the only way to tell that from a plain expiry. + this.logger( + 'Warn', + `[ForestOAuthProvider] Session lapsed while calling Forest Admin: ` + + `sessionStartedAt=${sessionStartedAt}, roundTripSeconds=${ + nowInSeconds - sessionStartedAt + }`, + ); throw new InvalidGrantError('Refresh token has expired'); } - // Capped here rather than in the sign() options, so `expires_in` below reports the same value. + // Computed before signing so the `expires_in` returned below reports the same value. const expiresIn = capTtl( Math.min(expirationDate - nowInSeconds, sessionRemaining), this.tokenTtl?.accessTokenSeconds, @@ -447,18 +464,17 @@ export default class ForestOAuthProvider implements OAuthServerProvider { ); // Create new refresh token (token rotation for security) - const refreshToken = jsonwebtoken.sign( - { - type: 'refresh', - clientId: client.client_id, - userId: user.id, - renderingId, - serverRefreshToken: forestServerRefreshToken, - sessionStartedAt, - }, - this.authSecret, - { expiresIn: Math.min(refreshTokenExpirationDate - nowInSeconds, sessionRemaining) }, - ); + const refreshClaims: RefreshTokenClaims = { + type: 'refresh', + clientId: client.client_id, + userId: user.id, + renderingId, + serverRefreshToken: forestServerRefreshToken, + sessionStartedAt, + }; + const refreshToken = jsonwebtoken.sign(refreshClaims, this.authSecret, { + expiresIn: Math.min(refreshTokenExpirationDate - nowInSeconds, sessionRemaining), + }); return { access_token: accessToken, diff --git a/packages/mcp-server/src/utils/parse-token-ttl.ts b/packages/mcp-server/src/utils/parse-token-ttl.ts index 36963ed6c0..7121b06457 100644 --- a/packages/mcp-server/src/utils/parse-token-ttl.ts +++ b/packages/mcp-server/src/utils/parse-token-ttl.ts @@ -1,13 +1,25 @@ import type { TokenTtlOptions } from './token-ttl'; -export default function parseTokenTtl( - accessTokenSeconds?: string, - refreshTokenSeconds?: string, -): TokenTtlOptions | undefined { - if (!accessTokenSeconds && !refreshTokenSeconds) return undefined; +// An env var set to an empty string is a misconfiguration, not an opt-out: the operator meant to +// cap something. Coercing it to NaN makes normalizeTokenTtl reject it instead of leaving that token +// silently uncapped — which matters most when only one of the two is empty. +function toSeconds(value: string | undefined): number | undefined { + if (value === undefined) return undefined; + + return Number(value.trim() === '' ? NaN : value.trim()); +} + +export default function parseTokenTtl({ + accessTokenSeconds, + refreshTokenSeconds, +}: { + accessTokenSeconds?: string; + refreshTokenSeconds?: string; +}): TokenTtlOptions | undefined { + if (accessTokenSeconds === undefined && refreshTokenSeconds === undefined) return undefined; return { - accessTokenSeconds: accessTokenSeconds ? Number(accessTokenSeconds) : undefined, - refreshTokenSeconds: refreshTokenSeconds ? Number(refreshTokenSeconds) : undefined, + accessTokenSeconds: toSeconds(accessTokenSeconds), + refreshTokenSeconds: toSeconds(refreshTokenSeconds), }; } diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts index 0ad7e56776..792f0b1053 100644 --- a/packages/mcp-server/src/utils/token-ttl.ts +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -12,7 +12,7 @@ export type TokenTtlOptions = { }; function normalizeSeconds( - field: string, + field: keyof TokenTtlOptions, value: number | undefined, logger: Logger, ): number | undefined { diff --git a/packages/mcp-server/test/cli.test.ts b/packages/mcp-server/test/cli.test.ts index 91e77777ab..816bffcb7a 100644 --- a/packages/mcp-server/test/cli.test.ts +++ b/packages/mcp-server/test/cli.test.ts @@ -28,22 +28,34 @@ describe('parseToolList', () => { }); describe('parseTokenTtl', () => { - it.each([ - [undefined, undefined], - ['', ''], - ])('returns undefined for (%p, %p) so nothing is capped', (access, refresh) => { - expect(parseTokenTtl(access, refresh)).toBeUndefined(); + it('returns undefined when neither variable is set, so nothing is capped', () => { + expect(parseTokenTtl({})).toBeUndefined(); }); it.each([ - ['900', undefined, { accessTokenSeconds: 900, refreshTokenSeconds: undefined }], - [undefined, '86400', { accessTokenSeconds: undefined, refreshTokenSeconds: 86400 }], - ['900', '86400', { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }], - ])('parses (%p, %p) to seconds', (access, refresh, expected) => { - expect(parseTokenTtl(access, refresh)).toEqual(expected); - }); - - it('leaves a non-numeric value as NaN for the normalizer to reject', () => { - expect(parseTokenTtl('abc')?.accessTokenSeconds).toBeNaN(); + [{ accessTokenSeconds: '900' }, { accessTokenSeconds: 900, refreshTokenSeconds: undefined }], + [ + { refreshTokenSeconds: '86400' }, + { accessTokenSeconds: undefined, refreshTokenSeconds: 86400 }, + ], + [ + { accessTokenSeconds: '900', refreshTokenSeconds: '86400' }, + { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, + ], + [{ accessTokenSeconds: ' 900 ' }, { accessTokenSeconds: 900, refreshTokenSeconds: undefined }], + ])('parses %p to seconds', (env, expected) => { + expect(parseTokenTtl(env)).toEqual(expected); + }); + + // An unrendered template or a missing ConfigMap key must not silently leave a token uncapped. + it.each(['', ' ', 'abc', '15m'])('yields NaN for %p so the normalizer rejects it', value => { + expect(parseTokenTtl({ accessTokenSeconds: value })?.accessTokenSeconds).toBeNaN(); + }); + + it('rejects an empty variable even when the other one is valid', () => { + expect(parseTokenTtl({ accessTokenSeconds: '900', refreshTokenSeconds: '' })).toEqual({ + accessTokenSeconds: 900, + refreshTokenSeconds: NaN, + }); }); }); diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index 07480bd579..9f81bec315 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -4,7 +4,6 @@ import type { OAuthClientInformationFull } from '@modelcontextprotocol/sdk/share import type { Response } from 'express'; import createForestAdminClient from '@forestadmin/forestadmin-client'; -import { InvalidGrantError } from '@modelcontextprotocol/sdk/server/auth/errors.js'; import jsonwebtoken from 'jsonwebtoken'; import MockServer from './test-utils/mock-server'; @@ -795,7 +794,9 @@ describe('ForestOAuthProvider', () => { ).rejects.toThrow('Token was not issued to this client'); }); - it('should throw error when Forest Admin refresh fails', async () => { + // Forest revoking the session (removed user, revoked token) must reach the client as + // invalid_grant, the only code that makes it re-run the interactive login. + it('propagates the OAuth code when Forest Admin rejects the refresh', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', clientId: 'test-client-id', @@ -811,7 +812,7 @@ describe('ForestOAuthProvider', () => { await expect( provider.exchangeRefreshToken(mockClient, 'valid-refresh-token'), - ).rejects.toThrow('Failed to refresh token'); + ).rejects.toMatchObject({ errorCode: 'invalid_grant' }); }); }); @@ -1209,7 +1210,7 @@ describe('ForestOAuthProvider', () => { jest.setSystemTime(NOW_MS + 5000); // Only invalid_grant makes the client re-run the interactive login. - await expect(pending).rejects.toBeInstanceOf(InvalidGrantError); + await expect(pending).rejects.toMatchObject({ errorCode: 'invalid_grant' }); expect(mockJwtSign).not.toHaveBeenCalled(); }); @@ -1226,7 +1227,7 @@ describe('ForestOAuthProvider', () => { await expect( provider.exchangeRefreshToken(mockClient, 'our-refresh-token'), - ).rejects.toBeInstanceOf(InvalidGrantError); + ).rejects.toMatchObject({ errorCode: 'invalid_grant' }); expect(mockServer.fetch).not.toHaveBeenCalledWith( expect.stringContaining('/oauth/token'), expect.anything(), diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index 3f6d7b3723..4cb405ed37 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -763,8 +763,10 @@ describe('ForestMCPServer Instance', () => { }); expect(response.status).toBe(400); - expect(response.body.error).toBe('invalid_request'); - expect(response.body.error_description).toContain('Failed to exchange authorization code'); + expect(response.body.error).toBe('invalid_grant'); + expect(response.body.error_description).toContain( + 'The authorization code has expired or is invalid', + ); }); it('should return error when client authentication fails', async () => { @@ -785,8 +787,8 @@ describe('ForestMCPServer Instance', () => { }); expect(response.status).toBe(400); - expect(response.body.error).toBe('invalid_request'); - expect(response.body.error_description).toContain('Failed to exchange authorization code'); + expect(response.body.error).toBe('invalid_client'); + expect(response.body.error_description).toContain('Client authentication failed'); }); it('should return error when requested scope is invalid', async () => { @@ -807,8 +809,10 @@ describe('ForestMCPServer Instance', () => { }); expect(response.status).toBe(400); - expect(response.body.error).toBe('invalid_request'); - expect(response.body.error_description).toContain('Failed to exchange authorization code'); + expect(response.body.error).toBe('invalid_scope'); + expect(response.body.error_description).toContain( + 'The requested scope is invalid or unknown', + ); }); it('should return error when client is not authorized', async () => { @@ -829,8 +833,10 @@ describe('ForestMCPServer Instance', () => { }); expect(response.status).toBe(400); - expect(response.body.error).toBe('invalid_request'); - expect(response.body.error_description).toContain('Failed to exchange authorization code'); + expect(response.body.error).toBe('unauthorized_client'); + expect(response.body.error_description).toContain( + 'The client is not authorized to use this grant type', + ); }); it('should return error when Forest Admin server has internal error', async () => { @@ -851,8 +857,10 @@ describe('ForestMCPServer Instance', () => { }); expect(response.status).toBe(400); - expect(response.body.error).toBe('invalid_request'); - expect(response.body.error_description).toContain('Failed to exchange authorization code'); + expect(response.body.error).toBe('server_error'); + expect(response.body.error_description).toContain( + 'An unexpected error occurred on the server', + ); }); it('should use default error description when not provided by Forest server', async () => { @@ -883,7 +891,7 @@ describe('ForestMCPServer Instance', () => { }); expect(response.status).toBe(400); - expect(response.body.error).toBe('invalid_request'); + expect(response.body.error).toBe('server_error'); expect(response.body.error_description).toContain('Failed to exchange authorization code'); }); }); From 4ba19d960af737384d012a9e739f304ab246d523 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 28 Jul 2026 16:33:50 +0200 Subject: [PATCH 06/15] docs(mcp-server): say Forest, not Forest Admin The company renamed. Scoped to the lines this branch adds; the rest of the repo still carries the old name and needs its own sweep. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/agent.ts | 2 +- packages/mcp-server/README.md | 6 +++--- packages/mcp-server/src/forest-oauth-provider.ts | 4 ++-- packages/mcp-server/src/server.ts | 2 +- packages/mcp-server/src/utils/token-ttl.ts | 4 ++-- packages/mcp-server/test/forest-oauth-provider.test.ts | 10 +++++----- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/agent/src/agent.ts b/packages/agent/src/agent.ts index d25c58e833..20537cc615 100644 --- a/packages/agent/src/agent.ts +++ b/packages/agent/src/agent.ts @@ -258,7 +258,7 @@ export default class Agent extends FrameworkMounter * // OAuth discovery metadata stays at the origin root (prefix-suffixed), so root `.well-known` * // traffic must still reach the agent. * agent.mountAiMcpServer({ basePath: '/ai' }); - * // Example: shorten the OAuth token lifetimes. Forest Admin grants 1h for an access token and + * // Example: shorten the OAuth token lifetimes. Forest grants 1h for an access token and * // 8 days for a refresh token; these only shorten that. `refreshTokenSeconds` bounds the time * // between two interactive logins. * agent.mountAiMcpServer({ tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 } }); diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index 5f943279c8..6a52cda0bc 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -113,12 +113,12 @@ See [Available Tools](#available-tools) for the full list. `describeCollection` ## Shorten Token Lifetimes -Forest Admin grants **1 hour** (3600s) for an access token and **8 days** (691200s) for a refresh token — but it re-grants those 8 days on *every* refresh, so without `refreshTokenSeconds` a client that keeps working is never asked to sign in again. +Forest grants **1 hour** (3600s) for an access token and **8 days** (691200s) for a refresh token — but it re-grants those 8 days on *every* refresh, so without `refreshTokenSeconds` a client that keeps working is never asked to sign in again. -**Both values are upper bounds: they can only shorten that, never extend it.** For `accessTokenSeconds` a value above 3600 therefore has no effect. `refreshTokenSeconds` bounds the whole session, which Forest Admin otherwise re-extends on every refresh, so any value shortens it however large it is. +**Both values are upper bounds: they can only shorten that, never extend it.** For `accessTokenSeconds` a value above 3600 therefore has no effect. `refreshTokenSeconds` bounds the whole session, which Forest otherwise re-extends on every refresh, so any value shortens it however large it is. ```typescript -// With Forest Admin Agent +// With Forest Agent agent.mountAiMcpServer({ tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, }); diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index 69ffcb5031..ee8ab3c07f 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -345,7 +345,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { this.logger( 'Warn', `[ForestOAuthProvider] tokenTtl.${field}=${capSeconds} exceeds the ${upstreamTtlSeconds}s ` + - `lifetime granted by Forest Admin and has no effect: this option can only shorten a token ` + + `lifetime granted by Forest and has no effect: this option can only shorten a token ` + `lifetime, never extend it.`, ); } @@ -436,7 +436,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { // outlasted the session — the elapsed time is the only way to tell that from a plain expiry. this.logger( 'Warn', - `[ForestOAuthProvider] Session lapsed while calling Forest Admin: ` + + `[ForestOAuthProvider] Session lapsed while calling Forest: ` + `sessionStartedAt=${sessionStartedAt}, roundTripSeconds=${ nowInSeconds - sessionStartedAt }`, diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index be45fcf228..7510bda4d4 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -142,7 +142,7 @@ export interface ForestMCPServerOptions { */ agentDispatcher?: InProcessAgentDispatcher; /** - * Upper bounds on the OAuth token lifetimes this server issues. Forest Admin grants 1 hour + * Upper bounds on the OAuth token lifetimes this server issues. Forest grants 1 hour * (3600s) for an access token and 8 days (691200s) for a refresh token; each value can only * shorten that, never extend it. `refreshTokenSeconds` bounds the time between two interactive * logins. Minimum 60s for either. diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts index 792f0b1053..f6afa73c70 100644 --- a/packages/mcp-server/src/utils/token-ttl.ts +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -5,9 +5,9 @@ import type { Logger } from '../server'; export const MIN_TOKEN_TTL_SECONDS = 60; export type TokenTtlOptions = { - /** Caps the access token lifetime. Forest Admin grants 3600s (1 hour). */ + /** Caps the access token lifetime. Forest grants 3600s (1 hour). */ accessTokenSeconds?: number; - /** Caps the time between two interactive logins. Forest Admin grants 691200s (8 days). */ + /** Caps the time between two interactive logins. Forest grants 691200s (8 days). */ refreshTokenSeconds?: number; }; diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index 9f81bec315..1e184a84a3 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -796,7 +796,7 @@ describe('ForestOAuthProvider', () => { // Forest revoking the session (removed user, revoked token) must reach the client as // invalid_grant, the only code that makes it re-run the interactive login. - it('propagates the OAuth code when Forest Admin rejects the refresh', async () => { + it('propagates the OAuth code when Forest rejects the refresh', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', clientId: 'test-client-id', @@ -1051,7 +1051,7 @@ describe('ForestOAuthProvider', () => { expect(result.expires_in).toBe(900); }); - it('never extends beyond the lifetimes Forest Admin granted', async () => { + it('never extends beyond the lifetimes Forest granted', async () => { const provider = createProvider(undefined, undefined, { accessTokenSeconds: 7200, refreshTokenSeconds: 30 * 24 * 3600, @@ -1064,7 +1064,7 @@ describe('ForestOAuthProvider', () => { expect(result.expires_in).toBe(FOREST_ACCESS_TTL); }); - it('warns when the access cap exceeds the lifetime Forest Admin granted', async () => { + it('warns when the access cap exceeds the lifetime Forest granted', async () => { const logger = jest.fn(); const provider = createProvider(undefined, undefined, { accessTokenSeconds: 7200 }, logger); @@ -1194,7 +1194,7 @@ describe('ForestOAuthProvider', () => { expect(result.expires_in).toBe(60); }); - it('rejects when the session elapses while talking to Forest Admin', async () => { + it('rejects when the session elapses while talking to Forest', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', clientId: 'test-client-id', @@ -1214,7 +1214,7 @@ describe('ForestOAuthProvider', () => { expect(mockJwtSign).not.toHaveBeenCalled(); }); - it('rejects a refresh once the session window has elapsed, without calling Forest Admin', async () => { + it('rejects a refresh once the session window has elapsed, without calling Forest', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', clientId: 'test-client-id', From 637517a7b952400ee425f026b01335800f41bf93 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 28 Jul 2026 16:44:06 +0200 Subject: [PATCH 07/15] docs(mcp-server): scope what accessTokenSeconds actually protects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The JWT the server signs is not encrypted and carries the Forest token under a serverToken claim, so a leaked MCP access token yields a Forest credential usable against the agent for its own remaining lifetime — bypassing the scopes and the audit log the MCP path enforces. Claiming the cap shortens how long a leaked token stays usable promised more than the code delivers. Also drops the JSDoc over-generalisation: only accessTokenSeconds is bounded by a Forest lifetime, refreshTokenSeconds bounds an otherwise unbounded session. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/agent.ts | 6 +++--- packages/mcp-server/README.md | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/agent/src/agent.ts b/packages/agent/src/agent.ts index 20537cc615..6e7d852483 100644 --- a/packages/agent/src/agent.ts +++ b/packages/agent/src/agent.ts @@ -258,9 +258,9 @@ export default class Agent extends FrameworkMounter * // OAuth discovery metadata stays at the origin root (prefix-suffixed), so root `.well-known` * // traffic must still reach the agent. * agent.mountAiMcpServer({ basePath: '/ai' }); - * // Example: shorten the OAuth token lifetimes. Forest grants 1h for an access token and - * // 8 days for a refresh token; these only shorten that. `refreshTokenSeconds` bounds the time - * // between two interactive logins. + * // Example: shorten the OAuth token lifetimes. `accessTokenSeconds` cannot exceed the 1h Forest + * // grants; `refreshTokenSeconds` bounds the time between two interactive logins, which is + * // otherwise unbounded since Forest re-grants its refresh lifetime on every refresh. * agent.mountAiMcpServer({ tokenTtl: { accessTokenSeconds: 900, refreshTokenSeconds: 86400 } }); */ mountAiMcpServer(options?: { diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index 6a52cda0bc..a8ad563e7c 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -133,7 +133,7 @@ npx forest-mcp-server The two settings differ in what the user notices: -- `accessTokenSeconds` shortens how long a leaked access token stays usable. It is transparent — the AI assistant silently obtains a new one. +- `accessTokenSeconds` shortens how long a leaked access token can drive **this server**: the MCP path closes, its scopes stop applying and its calls stop being audited. It does **not** shorten the Forest token carried inside that JWT — the JWT is signed, not encrypted, so treat a leak as a Forest token leak and revoke at the source. It is transparent to users — the assistant silently obtains a new one. - `refreshTokenSeconds` bounds the time between two **interactive logins**: once it elapses, the assistant can no longer refresh and the user signs in through the browser again. It is measured from the login itself, so an active assistant cannot keep extending its session. Refresh tokens issued before you enabled the option carry no login timestamp, so their window is measured from their last refresh instead — one longer session each, then bounded. The minimum for either value is 60 seconds; anything lower is raised to it. An invalid value (zero, negative, fractional) fails at startup rather than silently leaving the tokens uncapped. From f685a7c5ffaf4e0a1f3c9e2aafb73ada34d1bc1e Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Tue, 28 Jul 2026 16:56:56 +0200 Subject: [PATCH 08/15] fix(mcp-server): make roundTripSeconds measure the round trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round from @matthv. roundTripSeconds was `nowInSeconds - sessionStartedAt`, i.e. the session age. Since that branch is only reached when the session has lapsed, the field always printed at least the configured cap — 86404 on a request whose round trips took 5s — so it could never do the one job the comment claimed. It now diffs against a timestamp captured at the top of generateAccessToken, before the Forest calls. RefreshTokenClaims caught a rename but not an omission: `sessionStartedAt?` was optional on the shared interface, so deleting it from the signed payload still typechecked and `?? iat` took over silently. It is now required on the write side, with a separate read type accepting it missing for tokens minted before the claim. Verified: omitting it is TS2741. Adds the cross-field warn two reviewers asked for — a session shorter than one access token is almost always the two values swapped. Drops a stale note in server.test.ts that contradicted the six assertions under it, restores the comment explaining the 3600 fallback (accurate, and the line had no explanation left), scopes the CLAUDE.md expires_in claim to exclude the already-expired branch, and drops the [ForestOAuthProvider] prefix from the clamp warning — that one is emitted from the ForestMCPServer constructor. Co-Authored-By: Claude Opus 5 (1M context) --- packages/mcp-server/CLAUDE.md | 2 +- .../mcp-server/src/forest-oauth-provider.ts | 28 ++++++++++++------- packages/mcp-server/src/utils/token-ttl.ts | 18 ++++++++++-- packages/mcp-server/test/server.test.ts | 3 -- .../mcp-server/test/utils/token-ttl.test.ts | 15 ++++++++-- 5 files changed, 48 insertions(+), 18 deletions(-) diff --git a/packages/mcp-server/CLAUDE.md b/packages/mcp-server/CLAUDE.md index 261030b522..518e2cf714 100644 --- a/packages/mcp-server/CLAUDE.md +++ b/packages/mcp-server/CLAUDE.md @@ -13,7 +13,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Key flows that only make sense across files: - **Per-request MCP server.** `handleMcpRequest` builds a *fresh* `McpServer` + `StreamableHTTPServerTransport` per request (`sessionIdGenerator: undefined`, stateless) and closes the transport on response end. Tool registration in `createMcpServer()` therefore re-runs on every call. -- **OAuth = pass-through to Forest Admin.** `ForestOAuthProvider` (`src/forest-oauth-provider.ts`) does not store tokens. `/oauth/authorize` redirects to the Forest app; `exchange*` calls relay to the Forest server's `/oauth/token`, then `generateAccessToken` re-signs a JWT with `authSecret` carrying the Forest token under the `serverToken` claim. `verifyAccessToken` verifies that JWT and builds `AuthInfo.extra` with `forestServerToken: decoded.serverToken` plus `environmentApiEndpoint` — the latter is **not** in the JWT, it's read from the provider's own `this.environmentApiEndpoint` field (set during env discovery). Both lifetimes mirror the Forest token's own `exp`, optionally shortened by the `tokenTtl` option (`src/utils/token-ttl.ts`) — always a `min`, never a `max`, and capped at the point the TTL is computed so the advertised `expires_in` stays in step with the signed JWT. The refresh cap is anchored on a `sessionStartedAt` claim carried across refreshes and never re-stamped: Forest grants a full refresh lifetime on every refresh, so a per-refresh cap would slide forever and never force a re-login. +- **OAuth = pass-through to Forest Admin.** `ForestOAuthProvider` (`src/forest-oauth-provider.ts`) does not store tokens. `/oauth/authorize` redirects to the Forest app; `exchange*` calls relay to the Forest server's `/oauth/token`, then `generateAccessToken` re-signs a JWT with `authSecret` carrying the Forest token under the `serverToken` claim. `verifyAccessToken` verifies that JWT and builds `AuthInfo.extra` with `forestServerToken: decoded.serverToken` plus `environmentApiEndpoint` — the latter is **not** in the JWT, it's read from the provider's own `this.environmentApiEndpoint` field (set during env discovery). Both lifetimes mirror the Forest token's own `exp`, optionally shortened by the `tokenTtl` option (`src/utils/token-ttl.ts`) — always a `min`, never a `max`, and capped at the point the TTL is computed so the advertised `expires_in` matches the signed JWT (except on the already-expired-upstream branch, which advertises 3600). The refresh cap is anchored on a `sessionStartedAt` claim carried across refreshes and never re-stamped: Forest grants a full refresh lifetime on every refresh, so a per-refresh cap would slide forever and never force a re-login. - **Tools call the live agent, not this server.** Each tool in `src/tools/*` is a `declareXxxTool(mcpServer, forestServerClient, logger, collectionNames)` factory. At call time `buildClient(extra)` (`src/utils/agent-caller.ts`) reads `extra.authInfo` (`forestServerToken` + `environmentApiEndpoint` from `AuthInfo.extra`) and builds a `createRemoteAgentClient` from `@forestadmin/agent-client` — i.e. the tool RPCs into the user's actual running agent. `forestServerClient` (`src/http-client`, wrapping `@forestadmin/forestadmin-client`'s `SchemaService`/`ActivityLogsService`) is used only for schema fetch and activity logging, not data. - **Two cross-cutting wrappers, always used together.** `registerToolWithLogging` (`src/utils/tool-with-logging.ts`) registers the tool and converts thrown errors into `{ isError: true }` results (per MCP spec) instead of protocol errors. Inside the handler, `withActivityLog` (`src/utils/with-activity-log.ts`) brackets the operation with a pending→succeeded/failed Forest activity log and runs `parseAgentError` + optional `errorEnhancer` (e.g. `list` appends sortable field names on "Invalid sort"). - **`collectionNames` → `z.enum`.** `fetchCollectionNames()` populates the schema's collection list; tools turn it into a `z.enum` for `collectionName` so the LLM gets autocomplete/validation. If schema fetch fails the server logs a warning and runs "degraded" with `z.string()`. diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index ee8ab3c07f..41bdab9dfc 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -28,18 +28,25 @@ import jsonwebtoken from 'jsonwebtoken'; import { capTtl, sessionRemainingSeconds } from './utils/token-ttl'; -// Used by both the sign() payload and the verify() cast, so renaming a claim on one side is a -// compile error rather than a silent fallback to `iat` — which is re-stamped on every rotation and -// would slide the session window forever. +// Shape of the refresh token this server signs. Required on the write side so that dropping or +// renaming `sessionStartedAt` is a compile error rather than a silent fallback to `iat` — which is +// re-stamped on every rotation and would slide the session window forever. interface RefreshTokenClaims { type: 'refresh'; clientId: string; userId: number; renderingId: number; serverRefreshToken: string; - sessionStartedAt?: number; + sessionStartedAt: number; } +// Tokens minted before the claim existed carry no `sessionStartedAt`, so the read side accepts it +// missing and falls back to `iat`. +type DecodedRefreshToken = Omit & { + sessionStartedAt?: number; + iat?: number; +}; + export interface ForestOAuthProviderOptions { forestServerUrl: string; forestAppUrl: string; @@ -253,7 +260,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { scopes?: string[], ): Promise { // Verify and decode the refresh token - let decoded: RefreshTokenClaims & { iat?: number }; + let decoded: DecodedRefreshToken; try { decoded = jsonwebtoken.verify(refreshToken, this.authSecret) as typeof decoded; @@ -355,6 +362,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { tokenPayload: Record, sessionStartedAt: number, ): Promise { + const requestStartedAt = Math.floor(Date.now() / 1000); const response = await fetch(`${this.forestServerUrl}/oauth/token`, { method: 'POST', headers: { @@ -432,14 +440,13 @@ export default class ForestOAuthProvider implements OAuthServerProvider { }); if (sessionRemaining <= 0) { - // Unlike the caller's pre-flight check, reaching here means the round trips themselves - // outlasted the session — the elapsed time is the only way to tell that from a plain expiry. + // The caller's pre-flight check passed, so the round trips above are what consumed the last + // of the window — their duration is the only way to tell this from a plain expiry. this.logger( 'Warn', `[ForestOAuthProvider] Session lapsed while calling Forest: ` + - `sessionStartedAt=${sessionStartedAt}, roundTripSeconds=${ - nowInSeconds - sessionStartedAt - }`, + `sessionStartedAt=${sessionStartedAt}, ` + + `roundTripSeconds=${nowInSeconds - requestStartedAt}`, ); throw new InvalidGrantError('Refresh token has expired'); } @@ -479,6 +486,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { return { access_token: accessToken, token_type: 'Bearer', + // Only reachable when the Forest token is already expired; a cap cannot reach zero. expires_in: expiresIn > 0 ? expiresIn : 3600, refresh_token: refreshToken, scope: scope || client.scope, diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts index f6afa73c70..a2f6508601 100644 --- a/packages/mcp-server/src/utils/token-ttl.ts +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -27,8 +27,7 @@ function normalizeSeconds( if (value < MIN_TOKEN_TTL_SECONDS) { logger( 'Warn', - `[ForestOAuthProvider] raising tokenTtl.${field} from ${value} to the ` + - `${MIN_TOKEN_TTL_SECONDS}s minimum`, + `raising tokenTtl.${field} from ${value} to the ${MIN_TOKEN_TTL_SECONDS}s minimum`, ); return MIN_TOKEN_TTL_SECONDS; @@ -54,6 +53,21 @@ export default function normalizeTokenTtl( if (accessTokenSeconds === undefined && refreshTokenSeconds === undefined) return undefined; + // Legal, and the session bound rightly wins — but a session shorter than one access token is + // almost always the two values swapped, which forces a browser login on the access-token cadence. + if ( + accessTokenSeconds !== undefined && + refreshTokenSeconds !== undefined && + refreshTokenSeconds < accessTokenSeconds + ) { + logger( + 'Warn', + `tokenTtl.refreshTokenSeconds=${refreshTokenSeconds} is shorter than ` + + `tokenTtl.accessTokenSeconds=${accessTokenSeconds}: users will re-authenticate every ` + + `${refreshTokenSeconds}s. Did you swap the two values?`, + ); + } + return { accessTokenSeconds, refreshTokenSeconds }; } diff --git a/packages/mcp-server/test/server.test.ts b/packages/mcp-server/test/server.test.ts index 4cb405ed37..e21a4d7e8d 100644 --- a/packages/mcp-server/test/server.test.ts +++ b/packages/mcp-server/test/server.test.ts @@ -742,9 +742,6 @@ describe('ForestMCPServer Instance', () => { .post('/oauth/token', errorResponse, statusCode); }; - // Note: The implementation wraps all OAuth errors in InvalidRequestError, - // so the error code is always 'invalid_request' with the original error in the description - it('should return error when authorization code is invalid', async () => { setupErrorMock( { diff --git a/packages/mcp-server/test/utils/token-ttl.test.ts b/packages/mcp-server/test/utils/token-ttl.test.ts index dee90c508c..3e85691a79 100644 --- a/packages/mcp-server/test/utils/token-ttl.test.ts +++ b/packages/mcp-server/test/utils/token-ttl.test.ts @@ -56,11 +56,22 @@ describe('normalizeTokenTtl', () => { }); expect(logger).toHaveBeenCalledWith( 'Warn', - `[ForestOAuthProvider] raising tokenTtl.accessTokenSeconds from 30 to the ` + - `${MIN_TOKEN_TTL_SECONDS}s minimum`, + `raising tokenTtl.accessTokenSeconds from 30 to the ${MIN_TOKEN_TTL_SECONDS}s minimum`, ); }); + it('warns when the session is shorter than one access token, the classic swap', () => { + expect( + normalizeTokenTtl({ accessTokenSeconds: 3600, refreshTokenSeconds: 60 }, logger), + ).toEqual({ accessTokenSeconds: 3600, refreshTokenSeconds: 60 }); + expect(logger).toHaveBeenCalledWith('Warn', expect.stringContaining('Did you swap the two')); + }); + + it('stays silent when the session is longer than the access token', () => { + normalizeTokenTtl({ accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, logger); + expect(logger).not.toHaveBeenCalled(); + }); + it('clamps at the boundary but leaves the minimum itself alone', () => { expect(normalizeTokenTtl({ accessTokenSeconds: 59 }, logger)?.accessTokenSeconds).toBe(60); expect(normalizeTokenTtl({ accessTokenSeconds: 60 }, logger)?.accessTokenSeconds).toBe(60); From 9bb8e63608790f0fe42f3346cc2da024d9b8b0ed Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 29 Jul 2026 09:44:33 +0200 Subject: [PATCH 09/15] fix(mcp-server): parse token payloads with zod instead of casting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit zod is already a runtime dependency of this package (^4.3.5, used by every tool schema), so this adds no dependency — and it follows the boundary-validation pattern workflow-executor documents for anything crossing a trust boundary. Three casts were unchecked. The two Forest tokens are only `decode`d, never verified — we hold no key — so their shape was entirely unvalidated: a missing or non-numeric `exp` put NaN into the TTL arithmetic and then into sign(). Our own refresh token is signature-verified but its payload shape was not, which was the one hole the required `sessionStartedAt` claim could not close: the compiler now rejects a rename or an omission, but not a verify() returning a different shape at runtime. The refresh schema makes `type: z.literal('refresh')` part of the shape, so the parse subsumes the old type check and a signature-valid token we cannot read is refused before any call to Forest. Drops the hand-written DecodedRefreshToken, now redundant with the schema. Co-Authored-By: Claude Opus 5 (1M context) --- .../mcp-server/src/forest-oauth-provider.ts | 82 ++++++++++++------- .../test/forest-oauth-provider.test.ts | 56 +++++++++++++ 2 files changed, 110 insertions(+), 28 deletions(-) diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index 41bdab9dfc..b2d92717cb 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -25,9 +25,33 @@ import { UnsupportedTokenTypeError, } from '@modelcontextprotocol/sdk/server/auth/errors.js'; import jsonwebtoken from 'jsonwebtoken'; +import { z } from 'zod'; import { capTtl, sessionRemainingSeconds } from './utils/token-ttl'; +// The SaaS tokens are only `decode`d — we hold no key to verify them — so their shape is entirely +// unchecked. A missing or non-numeric `exp` would put NaN into the TTL arithmetic and then into +// sign(), so parse rather than cast. +const ForestAccessTokenSchema = z.object({ + meta: z.object({ renderingId: z.number() }), + exp: z.number(), + scope: z.string().optional(), +}); + +const ForestRefreshTokenSchema = z.object({ exp: z.number() }); + +// Our own refresh token: signature-verified, but the payload shape still isn't — an older token +// from a previous release can legitimately differ, which is why `sessionStartedAt` is optional. +const DecodedRefreshTokenSchema = z.object({ + type: z.literal('refresh'), + clientId: z.string(), + userId: z.number(), + renderingId: z.number(), + serverRefreshToken: z.string(), + sessionStartedAt: z.number().optional(), + iat: z.number().optional(), +}); + // Shape of the refresh token this server signs. Required on the write side so that dropping or // renaming `sessionStartedAt` is a compile error rather than a silent fallback to `iat` — which is // re-stamped on every rotation and would slide the session window forever. @@ -40,13 +64,6 @@ interface RefreshTokenClaims { sessionStartedAt: number; } -// Tokens minted before the claim existed carry no `sessionStartedAt`, so the read side accepts it -// missing and falls back to `iat`. -type DecodedRefreshToken = Omit & { - sessionStartedAt?: number; - iat?: number; -}; - export interface ForestOAuthProviderOptions { forestServerUrl: string; forestAppUrl: string; @@ -260,10 +277,10 @@ export default class ForestOAuthProvider implements OAuthServerProvider { scopes?: string[], ): Promise { // Verify and decode the refresh token - let decoded: DecodedRefreshToken; + let verified: unknown; try { - decoded = jsonwebtoken.verify(refreshToken, this.authSecret) as typeof decoded; + verified = jsonwebtoken.verify(refreshToken, this.authSecret); } catch (error) { if (error instanceof jsonwebtoken.TokenExpiredError) { this.logger( @@ -283,11 +300,20 @@ export default class ForestOAuthProvider implements OAuthServerProvider { throw new InvalidGrantError('Invalid refresh token'); } - // Validate token type - if (decoded.type !== 'refresh') { + const parsed = DecodedRefreshTokenSchema.safeParse(verified); + + // A signature-valid token whose payload we cannot read is not usable as a refresh token, and + // `type` is part of that shape — so the parse subsumes the old type check. + if (!parsed.success) { + this.logger( + 'Error', + `[ForestOAuthProvider] Unexpected refresh token payload: ${parsed.error.message}`, + ); throw new UnsupportedTokenTypeError('Invalid token type'); } + const decoded = parsed.data; + // Validate client_id matches if (decoded.clientId !== client.client_id) { throw new InvalidClientError('Token was not issued to this client'); @@ -390,33 +416,33 @@ export default class ForestOAuthProvider implements OAuthServerProvider { }; // Get updated user info - const decodedAccessToken = jsonwebtoken.decode(forestServerAccessToken) as { - meta: { renderingId: number }; - exp: number; - iat: number; - scope: string; - } | null; - - if (!decodedAccessToken) { - throw new Error('Failed to decode access token from Forest Admin server'); + const decodedAccessToken = ForestAccessTokenSchema.safeParse( + jsonwebtoken.decode(forestServerAccessToken), + ); + + if (!decodedAccessToken.success) { + throw new Error( + `Unexpected access token payload from Forest: ${decodedAccessToken.error.message}`, + ); } const { meta: { renderingId }, exp: expirationDate, scope, - } = decodedAccessToken; + } = decodedAccessToken.data; - const decodedRefreshToken = jsonwebtoken.decode(forestServerRefreshToken) as { - exp: number; - iat: number; - } | null; + const decodedRefreshToken = ForestRefreshTokenSchema.safeParse( + jsonwebtoken.decode(forestServerRefreshToken), + ); - if (!decodedRefreshToken) { - throw new Error('Failed to decode refresh token from Forest Admin server'); + if (!decodedRefreshToken.success) { + throw new Error( + `Unexpected refresh token payload from Forest: ${decodedRefreshToken.error.message}`, + ); } - const { exp: refreshTokenExpirationDate } = decodedRefreshToken; + const { exp: refreshTokenExpirationDate } = decodedRefreshToken.data; const user = await this.forestClient.authService.getUserInfo( renderingId, forestServerAccessToken, diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index 1e184a84a3..30bf868d52 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -1250,6 +1250,62 @@ describe('ForestOAuthProvider', () => { expect(signedTtl(2)).toBe(85400); }); + // The Forest tokens are only decoded, never verified, so their shape is unchecked. A missing or + // non-numeric `exp` used to put NaN straight into the TTL arithmetic and then into sign(). + it.each([ + ['a missing exp', { meta: { renderingId: 456 }, scope: 'mcp:read' }], + ['a non-numeric exp', { meta: { renderingId: 456 }, exp: 'soon', scope: 'mcp:read' }], + ['a missing renderingId', { meta: {}, exp: 1_000_000 }], + ])('rejects a Forest access token with %s', async (_label, payload) => { + mockJwtDecode.mockReset(); + mockJwtDecode + .mockReturnValueOnce(payload) + .mockReturnValueOnce({ exp: nowInSeconds + FOREST_REFRESH_TTL }); + const provider = createProvider(); + + await expect(provider.exchangeAuthorizationCode(mockClient, 'auth-code-123')).rejects.toThrow( + /Unexpected access token payload from Forest/, + ); + expect(mockJwtSign).not.toHaveBeenCalled(); + }); + + it('rejects a Forest refresh token with a non-numeric exp', async () => { + mockJwtDecode.mockReset(); + mockJwtDecode + .mockReturnValueOnce({ + meta: { renderingId: 456 }, + exp: nowInSeconds + FOREST_ACCESS_TTL, + scope: 'mcp:read', + }) + .mockReturnValueOnce({ exp: null }); + const provider = createProvider(); + + await expect(provider.exchangeAuthorizationCode(mockClient, 'auth-code-123')).rejects.toThrow( + /Unexpected refresh token payload from Forest/, + ); + expect(mockJwtSign).not.toHaveBeenCalled(); + }); + + // Our own refresh token is signature-verified, but its payload shape is not. + it('rejects our refresh token when the payload shape is unusable', async () => { + (jsonwebtoken.verify as jest.Mock).mockReturnValue({ + type: 'refresh', + clientId: 'test-client-id', + userId: 'not-a-number', + renderingId: 456, + serverRefreshToken: 'forest-refresh-token', + }); + const provider = createProvider(); + + await expect(provider.exchangeRefreshToken(mockClient, 'our-refresh-token')).rejects.toThrow( + 'Invalid token type', + ); + expect(mockServer.fetch).not.toHaveBeenCalledWith( + expect.stringContaining('/oauth/token'), + expect.anything(), + ); + }); + it('still advertises the 3600 fallback when the Forest token is already expired', async () => { mockJwtDecode.mockReset(); mockJwtDecode From 5f21e994556e25888f92272ef54b9ff18f48608b Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 29 Jul 2026 09:59:01 +0200 Subject: [PATCH 10/15] refactor(mcp-server): drop parse-token-ttl and capTtl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither earned its keep. parse-token-ttl existed to coerce env strings, but Number() already trims (' 900 ' → 900) and turns '' and ' ' into 0, which the positive-integer check already rejects. So the trim was dead and the empty-string-to-NaN trick only changed the rejected value printed in the message from "0" to "NaN" — both values nobody typed. Its "both undefined → undefined" guard also duplicated normalizeTokenTtl's own. Replaced by one expression at the only call site. capTtl was a one-line Math.min wrapper with a single production call site, and it introduced a second convention for "no bound" (undefined) alongside the Infinity that sessionRemainingSeconds already returns. Inlining it unifies the sentinel and keeps the property that matters — expiresIn computed once, before signing. Its unit tests go with it; the same property is asserted with exact values at the provider level ("shortens both token lifetimes", "never extends beyond the lifetimes Forest granted"). Co-Authored-By: Claude Opus 5 (1M context) --- packages/mcp-server/src/cli.ts | 13 ++++--- .../mcp-server/src/forest-oauth-provider.ts | 9 ++--- .../mcp-server/src/utils/parse-token-ttl.ts | 25 -------------- packages/mcp-server/src/utils/token-ttl.ts | 4 --- packages/mcp-server/test/cli.test.ts | 34 ------------------- .../mcp-server/test/utils/token-ttl.test.ts | 19 ----------- 6 files changed, 13 insertions(+), 91 deletions(-) delete mode 100644 packages/mcp-server/src/utils/parse-token-ttl.ts diff --git a/packages/mcp-server/src/cli.ts b/packages/mcp-server/src/cli.ts index f0ec7c2cde..e5918065a8 100644 --- a/packages/mcp-server/src/cli.ts +++ b/packages/mcp-server/src/cli.ts @@ -1,9 +1,10 @@ #!/usr/bin/env node import ForestMCPServer from './server'; -import parseTokenTtl from './utils/parse-token-ttl'; import parseToolList from './utils/parse-tool-list'; +const toSeconds = (value?: string) => (value === undefined ? undefined : Number(value)); + // Start the server when run directly as CLI const server = new ForestMCPServer({ forestServerUrl: process.env.FOREST_SERVER_URL || 'https://api.forestadmin.com', @@ -12,10 +13,12 @@ const server = new ForestMCPServer({ authSecret: process.env.FOREST_AUTH_SECRET, enabledTools: parseToolList(process.env.FOREST_MCP_ENABLED_TOOLS), agentUrl: process.env.FOREST_AGENT_URL, - tokenTtl: parseTokenTtl({ - accessTokenSeconds: process.env.FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS, - refreshTokenSeconds: process.env.FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS, - }), + // normalizeTokenTtl rejects NaN and non-positive values, so a malformed or empty variable fails + // at startup rather than leaving that token uncapped. + tokenTtl: { + accessTokenSeconds: toSeconds(process.env.FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS), + refreshTokenSeconds: toSeconds(process.env.FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS), + }, }); server.run().catch(error => { diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index b2d92717cb..c111e1ae44 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -27,7 +27,7 @@ import { import jsonwebtoken from 'jsonwebtoken'; import { z } from 'zod'; -import { capTtl, sessionRemainingSeconds } from './utils/token-ttl'; +import { sessionRemainingSeconds } from './utils/token-ttl'; // The SaaS tokens are only `decode`d — we hold no key to verify them — so their shape is entirely // unchecked. A missing or non-numeric `exp` would put NaN into the TTL arithmetic and then into @@ -478,9 +478,10 @@ export default class ForestOAuthProvider implements OAuthServerProvider { } // Computed before signing so the `expires_in` returned below reports the same value. - const expiresIn = capTtl( - Math.min(expirationDate - nowInSeconds, sessionRemaining), - this.tokenTtl?.accessTokenSeconds, + const expiresIn = Math.min( + expirationDate - nowInSeconds, + sessionRemaining, + this.tokenTtl?.accessTokenSeconds ?? Infinity, ); const tokenScopes = scope ? scope.split(' ') : ['mcp:read', 'mcp:write', 'mcp:action']; const accessToken = jsonwebtoken.sign( diff --git a/packages/mcp-server/src/utils/parse-token-ttl.ts b/packages/mcp-server/src/utils/parse-token-ttl.ts deleted file mode 100644 index 7121b06457..0000000000 --- a/packages/mcp-server/src/utils/parse-token-ttl.ts +++ /dev/null @@ -1,25 +0,0 @@ -import type { TokenTtlOptions } from './token-ttl'; - -// An env var set to an empty string is a misconfiguration, not an opt-out: the operator meant to -// cap something. Coercing it to NaN makes normalizeTokenTtl reject it instead of leaving that token -// silently uncapped — which matters most when only one of the two is empty. -function toSeconds(value: string | undefined): number | undefined { - if (value === undefined) return undefined; - - return Number(value.trim() === '' ? NaN : value.trim()); -} - -export default function parseTokenTtl({ - accessTokenSeconds, - refreshTokenSeconds, -}: { - accessTokenSeconds?: string; - refreshTokenSeconds?: string; -}): TokenTtlOptions | undefined { - if (accessTokenSeconds === undefined && refreshTokenSeconds === undefined) return undefined; - - return { - accessTokenSeconds: toSeconds(accessTokenSeconds), - refreshTokenSeconds: toSeconds(refreshTokenSeconds), - }; -} diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts index a2f6508601..8ef0f6531e 100644 --- a/packages/mcp-server/src/utils/token-ttl.ts +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -71,10 +71,6 @@ export default function normalizeTokenTtl( return { accessTokenSeconds, refreshTokenSeconds }; } -export function capTtl(upstreamTtlSeconds: number, capSeconds?: number): number { - return capSeconds === undefined ? upstreamTtlSeconds : Math.min(upstreamTtlSeconds, capSeconds); -} - // Anchored on the interactive login, not on the current refresh: Forest grants a full refresh // lifetime on every refresh, so a per-refresh bound would slide forever and never force a re-login. export function sessionRemainingSeconds({ diff --git a/packages/mcp-server/test/cli.test.ts b/packages/mcp-server/test/cli.test.ts index 816bffcb7a..23dfde7fff 100644 --- a/packages/mcp-server/test/cli.test.ts +++ b/packages/mcp-server/test/cli.test.ts @@ -1,4 +1,3 @@ -import parseTokenTtl from '../src/utils/parse-token-ttl'; import parseToolList from '../src/utils/parse-tool-list'; describe('parseToolList', () => { @@ -26,36 +25,3 @@ describe('parseToolList', () => { expect(parseToolList('delete')).toEqual(['delete']); }); }); - -describe('parseTokenTtl', () => { - it('returns undefined when neither variable is set, so nothing is capped', () => { - expect(parseTokenTtl({})).toBeUndefined(); - }); - - it.each([ - [{ accessTokenSeconds: '900' }, { accessTokenSeconds: 900, refreshTokenSeconds: undefined }], - [ - { refreshTokenSeconds: '86400' }, - { accessTokenSeconds: undefined, refreshTokenSeconds: 86400 }, - ], - [ - { accessTokenSeconds: '900', refreshTokenSeconds: '86400' }, - { accessTokenSeconds: 900, refreshTokenSeconds: 86400 }, - ], - [{ accessTokenSeconds: ' 900 ' }, { accessTokenSeconds: 900, refreshTokenSeconds: undefined }], - ])('parses %p to seconds', (env, expected) => { - expect(parseTokenTtl(env)).toEqual(expected); - }); - - // An unrendered template or a missing ConfigMap key must not silently leave a token uncapped. - it.each(['', ' ', 'abc', '15m'])('yields NaN for %p so the normalizer rejects it', value => { - expect(parseTokenTtl({ accessTokenSeconds: value })?.accessTokenSeconds).toBeNaN(); - }); - - it('rejects an empty variable even when the other one is valid', () => { - expect(parseTokenTtl({ accessTokenSeconds: '900', refreshTokenSeconds: '' })).toEqual({ - accessTokenSeconds: 900, - refreshTokenSeconds: NaN, - }); - }); -}); diff --git a/packages/mcp-server/test/utils/token-ttl.test.ts b/packages/mcp-server/test/utils/token-ttl.test.ts index 3e85691a79..39069c8013 100644 --- a/packages/mcp-server/test/utils/token-ttl.test.ts +++ b/packages/mcp-server/test/utils/token-ttl.test.ts @@ -1,6 +1,5 @@ import normalizeTokenTtl, { MIN_TOKEN_TTL_SECONDS, - capTtl, sessionRemainingSeconds, } from '../../src/utils/token-ttl'; @@ -79,24 +78,6 @@ describe('normalizeTokenTtl', () => { }); }); -describe('capTtl', () => { - it('returns the Forest lifetime when no cap is configured', () => { - expect(capTtl(3600)).toBe(3600); - }); - - it('shortens the lifetime to the cap', () => { - expect(capTtl(3600, 900)).toBe(900); - }); - - it('never extends beyond the Forest lifetime', () => { - expect(capTtl(900, 3600)).toBe(900); - }); - - it.each([0, -5])('does not rescue an already-expired Forest lifetime of %p', upstream => { - expect(capTtl(upstream, 900)).toBe(upstream); - }); -}); - describe('sessionRemainingSeconds', () => { const nowInSeconds = 1_000_000; From 371315a80d42a37d48f97c952712f91a918d439e Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 29 Jul 2026 10:11:21 +0200 Subject: [PATCH 11/15] fix(mcp-server): coerce the numeric claims instead of requiring numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The zod schemas were stricter than the tokens they parse, which on the login path means nobody signs in. private-api's auth-token.ts types the payload as `meta?: { renderingId?: number }` — optional both ways — and the cast this replaced tolerated a stringified id, since it only ever fed String(renderingId) and getUserInfo. A strict z.number() would have rejected a token the previous code accepted. Coerced with int().positive() rather than plain coercion: Number(null) is 0, and an exp of 0 means expired in 1970, so bare coercion would have let the NaN class of bug back in through a different door. Pins that a valid token still passes, including one carrying renderingId as a string. Co-Authored-By: Claude Opus 5 (1M context) --- .../mcp-server/src/forest-oauth-provider.ts | 12 +++++++----- .../test/forest-oauth-provider.test.ts | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index c111e1ae44..1192eead05 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -30,15 +30,17 @@ import { z } from 'zod'; import { sessionRemainingSeconds } from './utils/token-ttl'; // The SaaS tokens are only `decode`d — we hold no key to verify them — so their shape is entirely -// unchecked. A missing or non-numeric `exp` would put NaN into the TTL arithmetic and then into -// sign(), so parse rather than cast. +// unchecked, and a missing or non-numeric `exp` put NaN into the TTL arithmetic and then into +// sign(). Coerced rather than strict: the SaaS types both `meta` and `renderingId` as optional +// (private-api auth-token.ts), and the previous cast tolerated a stringified id, so rejecting one +// here would break every login for no gain. const ForestAccessTokenSchema = z.object({ - meta: z.object({ renderingId: z.number() }), - exp: z.number(), + meta: z.object({ renderingId: z.coerce.number().int().positive() }), + exp: z.coerce.number().int().positive(), scope: z.string().optional(), }); -const ForestRefreshTokenSchema = z.object({ exp: z.number() }); +const ForestRefreshTokenSchema = z.object({ exp: z.coerce.number().int().positive() }); // Our own refresh token: signature-verified, but the payload shape still isn't — an older token // from a previous release can legitimately differ, which is why `sessionStartedAt` is optional. diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index 30bf868d52..1be2df3c5c 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -1250,6 +1250,25 @@ describe('ForestOAuthProvider', () => { expect(signedTtl(2)).toBe(85400); }); + // The SaaS types meta and renderingId as optional and the previous cast tolerated a stringified + // id, so the parse must not reject a token the old code accepted. + it('accepts a Forest access token whose renderingId is a string', async () => { + mockJwtDecode.mockReset(); + mockJwtDecode + .mockReturnValueOnce({ + meta: { renderingId: '456' }, + exp: nowInSeconds + FOREST_ACCESS_TTL, + scope: 'mcp:read', + }) + .mockReturnValueOnce({ exp: nowInSeconds + FOREST_REFRESH_TTL }); + const provider = createProvider(); + + const result = await provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'); + + expect(result.expires_in).toBe(FOREST_ACCESS_TTL); + expect(signedPayload(1)).toEqual(expect.objectContaining({ rendering_id: '456' })); + }); + // The Forest tokens are only decoded, never verified, so their shape is unchecked. A missing or // non-numeric `exp` used to put NaN straight into the TTL arithmetic and then into sign(). it.each([ From 8326551c8a153e523f9f76ba905c940470744749 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 29 Jul 2026 10:16:19 +0200 Subject: [PATCH 12/15] refactor(mcp-server): only parse the token whose failure is silent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I justified parsing the SaaS tokens with "a NaN reaches sign()". That is true and harmless: jsonwebtoken.sign rejects a NaN expiresIn, a missing meta already throws on destructuring, and a stringified exp already worked through JS coercion. Every malformed shape failed loudly, so a schema there bought a nicer message — against the risk of rejecting a token the SaaS legitimately sends, which breaks login. That risk materialised twice while writing it. Reverted to the cast. Our own refresh token is the opposite case and keeps its schema: if verify() returns a shape without `sessionStartedAt`, the `?? iat` fallback takes over with a timestamp re-stamped on every rotation, so the session window slides forever with no error and no log. Silent, and a security bound. That is what the parse is for. The Forest-token rejection tests go too: jsonwebtoken is mocked in that suite, so the loud failure they claimed to pin is invisible there. A test that cannot observe its property is worse than none. The tolerance test stays — it guards against someone re-adding a strict schema. Co-Authored-By: Claude Opus 5 (1M context) --- .../mcp-server/src/forest-oauth-provider.ts | 48 +++++++------------ .../test/forest-oauth-provider.test.ts | 36 -------------- 2 files changed, 18 insertions(+), 66 deletions(-) diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index 1192eead05..180dbf3cd1 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -29,19 +29,6 @@ import { z } from 'zod'; import { sessionRemainingSeconds } from './utils/token-ttl'; -// The SaaS tokens are only `decode`d — we hold no key to verify them — so their shape is entirely -// unchecked, and a missing or non-numeric `exp` put NaN into the TTL arithmetic and then into -// sign(). Coerced rather than strict: the SaaS types both `meta` and `renderingId` as optional -// (private-api auth-token.ts), and the previous cast tolerated a stringified id, so rejecting one -// here would break every login for no gain. -const ForestAccessTokenSchema = z.object({ - meta: z.object({ renderingId: z.coerce.number().int().positive() }), - exp: z.coerce.number().int().positive(), - scope: z.string().optional(), -}); - -const ForestRefreshTokenSchema = z.object({ exp: z.coerce.number().int().positive() }); - // Our own refresh token: signature-verified, but the payload shape still isn't — an older token // from a previous release can legitimately differ, which is why `sessionStartedAt` is optional. const DecodedRefreshTokenSchema = z.object({ @@ -418,33 +405,34 @@ export default class ForestOAuthProvider implements OAuthServerProvider { }; // Get updated user info - const decodedAccessToken = ForestAccessTokenSchema.safeParse( - jsonwebtoken.decode(forestServerAccessToken), - ); - - if (!decodedAccessToken.success) { - throw new Error( - `Unexpected access token payload from Forest: ${decodedAccessToken.error.message}`, - ); + // Cast, not parsed: every malformed shape already fails loudly downstream (a missing `meta` + // throws here, a missing `exp` makes jsonwebtoken.sign reject the NaN), while a schema strict enough to add + // anything would risk rejecting a token the SaaS legitimately sends — and that breaks login. + const decodedAccessToken = jsonwebtoken.decode(forestServerAccessToken) as { + meta: { renderingId: number }; + exp: number; + scope: string; + } | null; + + if (!decodedAccessToken) { + throw new Error('Failed to decode access token from Forest'); } const { meta: { renderingId }, exp: expirationDate, scope, - } = decodedAccessToken.data; + } = decodedAccessToken; - const decodedRefreshToken = ForestRefreshTokenSchema.safeParse( - jsonwebtoken.decode(forestServerRefreshToken), - ); + const decodedRefreshToken = jsonwebtoken.decode(forestServerRefreshToken) as { + exp: number; + } | null; - if (!decodedRefreshToken.success) { - throw new Error( - `Unexpected refresh token payload from Forest: ${decodedRefreshToken.error.message}`, - ); + if (!decodedRefreshToken) { + throw new Error('Failed to decode refresh token from Forest'); } - const { exp: refreshTokenExpirationDate } = decodedRefreshToken.data; + const { exp: refreshTokenExpirationDate } = decodedRefreshToken; const user = await this.forestClient.authService.getUserInfo( renderingId, forestServerAccessToken, diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index 1be2df3c5c..03b5d16c9c 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -1269,42 +1269,6 @@ describe('ForestOAuthProvider', () => { expect(signedPayload(1)).toEqual(expect.objectContaining({ rendering_id: '456' })); }); - // The Forest tokens are only decoded, never verified, so their shape is unchecked. A missing or - // non-numeric `exp` used to put NaN straight into the TTL arithmetic and then into sign(). - it.each([ - ['a missing exp', { meta: { renderingId: 456 }, scope: 'mcp:read' }], - ['a non-numeric exp', { meta: { renderingId: 456 }, exp: 'soon', scope: 'mcp:read' }], - ['a missing renderingId', { meta: {}, exp: 1_000_000 }], - ])('rejects a Forest access token with %s', async (_label, payload) => { - mockJwtDecode.mockReset(); - mockJwtDecode - .mockReturnValueOnce(payload) - .mockReturnValueOnce({ exp: nowInSeconds + FOREST_REFRESH_TTL }); - const provider = createProvider(); - - await expect(provider.exchangeAuthorizationCode(mockClient, 'auth-code-123')).rejects.toThrow( - /Unexpected access token payload from Forest/, - ); - expect(mockJwtSign).not.toHaveBeenCalled(); - }); - - it('rejects a Forest refresh token with a non-numeric exp', async () => { - mockJwtDecode.mockReset(); - mockJwtDecode - .mockReturnValueOnce({ - meta: { renderingId: 456 }, - exp: nowInSeconds + FOREST_ACCESS_TTL, - scope: 'mcp:read', - }) - .mockReturnValueOnce({ exp: null }); - const provider = createProvider(); - - await expect(provider.exchangeAuthorizationCode(mockClient, 'auth-code-123')).rejects.toThrow( - /Unexpected refresh token payload from Forest/, - ); - expect(mockJwtSign).not.toHaveBeenCalled(); - }); - // Our own refresh token is signature-verified, but its payload shape is not. it('rejects our refresh token when the payload shape is unusable', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ From 06b5a40ee0fbea23e7d6f7f069d17958d8f45ce0 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 29 Jul 2026 10:24:42 +0200 Subject: [PATCH 13/15] docs(mcp-server): cut the comments that were not carrying their weight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removed the three that repeated a fact stated elsewhere in the same file (the sessionStartedAt rollout note, duplicated on the type; the anchor note, same) or that paraphrased visible code (the parse subsuming the type check, when z.literal('refresh') sits two lines above). Shortened five others. Deleted all seven in the test file — the test names already carry the intent. What survives is the set where the reason is not deducible from the code and where a future edit would silently break something: the measured rate limit behind the 60s floor, the SaaS re-granting its refresh lifetime (the whole reason for the anchor), why the SaaS tokens are cast rather than parsed, why only the access cap can be inert, why the OAuth code must pass through, why the TTL is computed before signing, and why the 3600 branch is unreachable. Co-Authored-By: Claude Opus 5 (1M context) --- packages/mcp-server/src/cli.ts | 3 +-- .../mcp-server/src/forest-oauth-provider.ts | 26 ++++++------------- packages/mcp-server/src/utils/token-ttl.ts | 3 +-- .../test/forest-oauth-provider.test.ts | 11 -------- 4 files changed, 10 insertions(+), 33 deletions(-) diff --git a/packages/mcp-server/src/cli.ts b/packages/mcp-server/src/cli.ts index e5918065a8..ec5d4832ff 100644 --- a/packages/mcp-server/src/cli.ts +++ b/packages/mcp-server/src/cli.ts @@ -13,8 +13,7 @@ const server = new ForestMCPServer({ authSecret: process.env.FOREST_AUTH_SECRET, enabledTools: parseToolList(process.env.FOREST_MCP_ENABLED_TOOLS), agentUrl: process.env.FOREST_AGENT_URL, - // normalizeTokenTtl rejects NaN and non-positive values, so a malformed or empty variable fails - // at startup rather than leaving that token uncapped. + // normalizeTokenTtl rejects NaN and non-positive values, so a bad variable fails at startup. tokenTtl: { accessTokenSeconds: toSeconds(process.env.FOREST_MCP_ACCESS_TOKEN_TTL_SECONDS), refreshTokenSeconds: toSeconds(process.env.FOREST_MCP_REFRESH_TOKEN_TTL_SECONDS), diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index 180dbf3cd1..ba98e9d9b5 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -29,8 +29,6 @@ import { z } from 'zod'; import { sessionRemainingSeconds } from './utils/token-ttl'; -// Our own refresh token: signature-verified, but the payload shape still isn't — an older token -// from a previous release can legitimately differ, which is why `sessionStartedAt` is optional. const DecodedRefreshTokenSchema = z.object({ type: z.literal('refresh'), clientId: z.string(), @@ -41,9 +39,8 @@ const DecodedRefreshTokenSchema = z.object({ iat: z.number().optional(), }); -// Shape of the refresh token this server signs. Required on the write side so that dropping or -// renaming `sessionStartedAt` is a compile error rather than a silent fallback to `iat` — which is -// re-stamped on every rotation and would slide the session window forever. +// `sessionStartedAt` is required here so dropping or renaming it is a compile error, not a silent +// fallback to `iat` — which is re-stamped on every rotation and would slide the window forever. interface RefreshTokenClaims { type: 'refresh'; clientId: string; @@ -291,8 +288,6 @@ export default class ForestOAuthProvider implements OAuthServerProvider { const parsed = DecodedRefreshTokenSchema.safeParse(verified); - // A signature-valid token whose payload we cannot read is not usable as a refresh token, and - // `type` is part of that shape — so the parse subsumes the old type check. if (!parsed.success) { this.logger( 'Error', @@ -309,15 +304,12 @@ export default class ForestOAuthProvider implements OAuthServerProvider { } const nowInSeconds = Math.floor(Date.now() / 1000); - // `sessionStartedAt` is stamped at the interactive login; a token predating the claim falls back - // to its own issue date. Never re-stamped, or the window would slide and never force a re-login. const sessionStartedAt = decoded.sessionStartedAt ?? decoded.iat ?? nowInSeconds; const { refreshTokenSeconds } = this.tokenTtl ?? {}; // Checked again after the Forest round trips; this one only spares them when already expired. if (sessionRemainingSeconds({ refreshTokenSeconds, sessionStartedAt, nowInSeconds }) <= 0) { - // Info, not Error: the session reaching its configured limit is this option working. With the - // documented 86400 it fires once per user per day. + // Info, not Error: a session reaching its limit is this option working. this.logger( 'Info', `[ForestOAuthProvider] Session reached tokenTtl.refreshTokenSeconds=${refreshTokenSeconds}, ` + @@ -405,9 +397,8 @@ export default class ForestOAuthProvider implements OAuthServerProvider { }; // Get updated user info - // Cast, not parsed: every malformed shape already fails loudly downstream (a missing `meta` - // throws here, a missing `exp` makes jsonwebtoken.sign reject the NaN), while a schema strict enough to add - // anything would risk rejecting a token the SaaS legitimately sends — and that breaks login. + // Cast, not parsed: malformed shapes already fail loudly (missing `meta` throws here, missing + // `exp` makes sign() reject the NaN), and a stricter schema risks refusing a valid token. const decodedAccessToken = jsonwebtoken.decode(forestServerAccessToken) as { meta: { renderingId: number }; exp: number; @@ -447,8 +438,8 @@ export default class ForestOAuthProvider implements OAuthServerProvider { expirationDate - nowInSeconds, ); - // Re-read after the Forest round trips: they can outlast what is left of the session. Bounds the - // access token too, which would otherwise extend the session by its own lifetime. + // Re-read after the Forest round trips, which can outlast the session. Bounds the access token + // too, which would otherwise extend the session by its own lifetime. const sessionRemaining = sessionRemainingSeconds({ refreshTokenSeconds: this.tokenTtl?.refreshTokenSeconds, sessionStartedAt, @@ -456,8 +447,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { }); if (sessionRemaining <= 0) { - // The caller's pre-flight check passed, so the round trips above are what consumed the last - // of the window — their duration is the only way to tell this from a plain expiry. + // The pre-flight check passed, so the round trips consumed the last of the window. this.logger( 'Warn', `[ForestOAuthProvider] Session lapsed while calling Forest: ` + diff --git a/packages/mcp-server/src/utils/token-ttl.ts b/packages/mcp-server/src/utils/token-ttl.ts index 8ef0f6531e..528a01fa59 100644 --- a/packages/mcp-server/src/utils/token-ttl.ts +++ b/packages/mcp-server/src/utils/token-ttl.ts @@ -53,8 +53,7 @@ export default function normalizeTokenTtl( if (accessTokenSeconds === undefined && refreshTokenSeconds === undefined) return undefined; - // Legal, and the session bound rightly wins — but a session shorter than one access token is - // almost always the two values swapped, which forces a browser login on the access-token cadence. + // Legal — the session bound wins — but almost always the two values swapped. if ( accessTokenSeconds !== undefined && refreshTokenSeconds !== undefined && diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index 03b5d16c9c..4e279173cb 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -794,8 +794,6 @@ describe('ForestOAuthProvider', () => { ).rejects.toThrow('Token was not issued to this client'); }); - // Forest revoking the session (removed user, revoked token) must reach the client as - // invalid_grant, the only code that makes it re-run the interactive login. it('propagates the OAuth code when Forest rejects the refresh', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', @@ -1076,8 +1074,6 @@ describe('ForestOAuthProvider', () => { ); }); - // A refresh cap bounds the whole session, which Forest re-extends on every refresh, so a value - // above one token's lifetime still forces a re-login and must not be reported as inert. it('stays silent when the refresh cap exceeds one Forest refresh lifetime', async () => { const logger = jest.fn(); const provider = createProvider( @@ -1155,8 +1151,6 @@ describe('ForestOAuthProvider', () => { ); }); - // Every real refresh token carries both claims, `iat` being the mint time of that refresh. The - // anchor must be the stamped session start, or the window slides on every refresh. it('anchors on the stamped session start, not on the refresh token issue date', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', @@ -1205,11 +1199,9 @@ describe('ForestOAuthProvider', () => { }); const provider = createProvider(undefined, undefined, { refreshTokenSeconds: 86400 }); - // The pre-flight check passes with 1s left, then the round trips outlast the session. const pending = provider.exchangeRefreshToken(mockClient, 'our-refresh-token'); jest.setSystemTime(NOW_MS + 5000); - // Only invalid_grant makes the client re-run the interactive login. await expect(pending).rejects.toMatchObject({ errorCode: 'invalid_grant' }); expect(mockJwtSign).not.toHaveBeenCalled(); }); @@ -1250,8 +1242,6 @@ describe('ForestOAuthProvider', () => { expect(signedTtl(2)).toBe(85400); }); - // The SaaS types meta and renderingId as optional and the previous cast tolerated a stringified - // id, so the parse must not reject a token the old code accepted. it('accepts a Forest access token whose renderingId is a string', async () => { mockJwtDecode.mockReset(); mockJwtDecode @@ -1269,7 +1259,6 @@ describe('ForestOAuthProvider', () => { expect(signedPayload(1)).toEqual(expect.objectContaining({ rendering_id: '456' })); }); - // Our own refresh token is signature-verified, but its payload shape is not. it('rejects our refresh token when the payload shape is unusable', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh', From 53a442dec129f7a389cecec293cb035a24de1c96 Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 29 Jul 2026 10:38:20 +0200 Subject: [PATCH 14/15] refactor(mcp-server): derive the refresh claims from the zod schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The schema and the interface declared the same five claims independently, so renaming `sessionStartedAt` in the schema alone compiled fine while the parse silently stopped finding the claim and `?? iat` slid the window — only a runtime test stood in the way. The write type is now derived from the schema (Required>), so both a schema rename and a payload omission are compile errors; verified as TS errors by mutation. Also collapses warnIfCapIsInert's per-field Set to a boolean: since the refresh warning was removed there is one call site and one field, and the keyof parameter was generality for a case that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) --- .../mcp-server/src/forest-oauth-provider.ts | 39 +++++++------------ 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/packages/mcp-server/src/forest-oauth-provider.ts b/packages/mcp-server/src/forest-oauth-provider.ts index ba98e9d9b5..a76e2d6edf 100644 --- a/packages/mcp-server/src/forest-oauth-provider.ts +++ b/packages/mcp-server/src/forest-oauth-provider.ts @@ -39,16 +39,11 @@ const DecodedRefreshTokenSchema = z.object({ iat: z.number().optional(), }); -// `sessionStartedAt` is required here so dropping or renaming it is a compile error, not a silent -// fallback to `iat` — which is re-stamped on every rotation and would slide the window forever. -interface RefreshTokenClaims { - type: 'refresh'; - clientId: string; - userId: number; - renderingId: number; - serverRefreshToken: string; - sessionStartedAt: number; -} +// `sessionStartedAt` is required on the write side so dropping or renaming it — in the schema or in +// the signed payload — is a compile error, not a silent fallback to `iat`, which is re-stamped on +// every rotation and would slide the window forever. +type RefreshTokenClaims = Omit, 'iat'> & + Required, 'sessionStartedAt'>>; export interface ForestOAuthProviderOptions { forestServerUrl: string; @@ -74,7 +69,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { private environmentApiEndpoint?: string; private agentUrl?: string; private tokenTtl?: TokenTtlOptions; - private readonly fieldsWarnedAsInert = new Set(); + private warnedAccessCapInert = false; private logger: Logger; constructor({ @@ -347,20 +342,16 @@ export default class ForestOAuthProvider implements OAuthServerProvider { } } - private warnIfCapIsInert( - field: keyof TokenTtlOptions, - capSeconds: number | undefined, - upstreamTtlSeconds: number, - ): void { + private warnIfAccessCapIsInert(capSeconds: number | undefined, upstreamTtlSeconds: number): void { if (capSeconds === undefined || upstreamTtlSeconds <= 0) return; - if (capSeconds <= upstreamTtlSeconds || this.fieldsWarnedAsInert.has(field)) return; + if (capSeconds <= upstreamTtlSeconds || this.warnedAccessCapInert) return; - this.fieldsWarnedAsInert.add(field); + this.warnedAccessCapInert = true; this.logger( 'Warn', - `[ForestOAuthProvider] tokenTtl.${field}=${capSeconds} exceeds the ${upstreamTtlSeconds}s ` + - `lifetime granted by Forest and has no effect: this option can only shorten a token ` + - `lifetime, never extend it.`, + `[ForestOAuthProvider] tokenTtl.accessTokenSeconds=${capSeconds} exceeds the ` + + `${upstreamTtlSeconds}s lifetime granted by Forest and has no effect: this option can ` + + `only shorten a token lifetime, never extend it.`, ); } @@ -432,11 +423,7 @@ export default class ForestOAuthProvider implements OAuthServerProvider { const nowInSeconds = Math.floor(Date.now() / 1000); // Only the access cap can be inert. `refreshTokenSeconds` bounds the whole session, which Forest // otherwise re-extends on every refresh, so any value binds however large it is. - this.warnIfCapIsInert( - 'accessTokenSeconds', - this.tokenTtl?.accessTokenSeconds, - expirationDate - nowInSeconds, - ); + this.warnIfAccessCapIsInert(this.tokenTtl?.accessTokenSeconds, expirationDate - nowInSeconds); // Re-read after the Forest round trips, which can outlast the session. Bounds the access token // too, which would otherwise extend the session by its own lifetime. From db9e8f0b0cc7e7006e26108d4ed4e6489bb32d3b Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 29 Jul 2026 10:52:23 +0200 Subject: [PATCH 15/15] test(mcp-server): cover the Forest token decode guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qlty flagged the diff at 96.7% against a 98% threshold, on lines 400-414 — the two 'not a decodable JWT' guards. They predate this branch but count as modified because the rename touched their messages, and they were never covered. The zod rejection tests I removed had been covering them incidentally. Co-Authored-By: Claude Opus 5 (1M context) --- .../test/forest-oauth-provider.test.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/mcp-server/test/forest-oauth-provider.test.ts b/packages/mcp-server/test/forest-oauth-provider.test.ts index 4e279173cb..ed0bb58ea9 100644 --- a/packages/mcp-server/test/forest-oauth-provider.test.ts +++ b/packages/mcp-server/test/forest-oauth-provider.test.ts @@ -1259,6 +1259,26 @@ describe('ForestOAuthProvider', () => { expect(signedPayload(1)).toEqual(expect.objectContaining({ rendering_id: '456' })); }); + it.each([ + ['access', [null, { exp: 1_000_000 }], /Failed to decode access token from Forest/], + [ + 'refresh', + [{ meta: { renderingId: 456 }, exp: 1_000_000, scope: 'mcp:read' }, null], + /Failed to decode refresh token from Forest/, + ], + ])( + 'throws when the Forest %s token is not a decodable JWT', + async (_label, decoded, message) => { + mockJwtDecode.mockReset(); + mockJwtDecode.mockReturnValueOnce(decoded[0]).mockReturnValueOnce(decoded[1]); + const provider = createProvider(); + + await expect( + provider.exchangeAuthorizationCode(mockClient, 'auth-code-123'), + ).rejects.toThrow(message); + }, + ); + it('rejects our refresh token when the payload shape is unusable', async () => { (jsonwebtoken.verify as jest.Mock).mockReturnValue({ type: 'refresh',