diff --git a/docs/configuration.md b/docs/configuration.md index 754d818..2ab1756 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -105,6 +105,7 @@ Opt-in support for the Model Context Protocol authorization flow ([issue #86](ht | `mcp.signingAlgorithm` | string | `RS256` | JWT signing algorithm for generated signing keys: `RS256` or `ES256` (EC P-256, [#127](https://github.com/HarperFast/oauth/issues/127)). Changing it on a running deployment rotates in a new key under the new algorithm (once the current key is ≥ 5 minutes old — an age floor so nodes with briefly-disagreeing configs during a rolling rollout don't rotate against each other; update all nodes together); old keys stay in the JWKS until retired, so outstanding tokens keep verifying. Ignored while `signingKeyPem` is set (the pin's key material decides). EdDSA is not supported | | `mcp.accessTokenTtl` | number | `3600` | Access-token lifetime in seconds (default 1 hour) | | `mcp.refreshTokenTtl` | number | `2592000` | Refresh-token (family) lifetime in seconds (default 30 days) | +| `mcp.refreshTokenRequiresOfflineAccess` | boolean | `false` | When `true`, the `authorization_code` exchange only issues a refresh token when the granted scope includes `offline_access` (the [SEP-2207](https://modelcontextprotocol.io/specification/draft/basic/authorization) OIDC-flavored opt-in). Off by default because most MCP clients never request `offline_access` — withholding refresh tokens from them would force re-auth every `accessTokenTtl`. Flipping this on does not revoke existing refresh-token families. The AS metadata always advertises `offline_access` in `scopes_supported` so clients know they may request it | Sensitive leaves inside `mcp` support `${ENV_VAR}` expansion (e.g., `initialAccessToken: ${OAUTH_MCP_REGISTRATION_TOKEN}`), the same way provider credentials do. diff --git a/docs/mcp-oauth.md b/docs/mcp-oauth.md index 8a7326a..9190689 100644 --- a/docs/mcp-oauth.md +++ b/docs/mcp-oauth.md @@ -193,6 +193,15 @@ Refresh tokens rotate on use: presenting an already-used token from a family revokes the whole family (replay defense). Refresh families live for `refreshTokenTtl` (default 30 days). +By default any client whose registered `grant_types` include `refresh_token` +receives a refresh token on the code exchange. The AS metadata advertises +`offline_access` in `scopes_supported` (SEP-2207), so clients that want refresh +tokens may request that scope explicitly; setting +`mcp.refreshTokenRequiresOfflineAccess: true` makes that opt-in mandatory — +refresh tokens are then withheld unless the granted scope carries +`offline_access`. The PRM document never lists `offline_access` (refresh tokens +are not a resource requirement). + --- ## The `withMCPAuth` wrapper diff --git a/src/index.ts b/src/index.ts index 8c3cf46..4b44f52 100644 --- a/src/index.ts +++ b/src/index.ts @@ -164,10 +164,11 @@ export async function handleApplication(scope: Scope): Promise { // sensitive leaves (mcp.dynamicClientRegistration.initialAccessToken) and // a pinned issuer/resource support ${ENV_VAR}. const mcpConfig = options.mcp ? expandEnvVarsDeep(options.mcp) : undefined; - // Coerce documented boolean strings and normalize allowedHosts BEFORE any - // `enabled` check, so an env-expanded "false" can't leave a security switch - // truthy or turn a scalar allowedHosts into substring matching. - if (mcpConfig) normalizeMcpSecurityConfig(mcpConfig); + // Normalize documented booleans (totally — coerce or drop-with-warning) + // and allowedHosts BEFORE any `enabled` check, so an env-expanded "false" + // or an unresolved "${FLAG}" placeholder can't leave a gate string-truthy, + // and a scalar allowedHosts can't become substring matching. + if (mcpConfig) normalizeMcpSecurityConfig(mcpConfig, logger); if (mcpConfig?.enabled && !mcpConfig.issuer) { // Without a pinned issuer, resolveIssuer() (wellKnown.ts) derives it from // the client-controlled Host header — and resolveResource() defaults to diff --git a/src/lib/config.ts b/src/lib/config.ts index b7d395d..d041458 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -72,35 +72,74 @@ export function coerceConfigBoolean(value: unknown): boolean | undefined { return undefined; } +/** + * Normalize one documented-boolean config field in place, TOTALLY: after this + * call the field is either a real boolean or absent. Coercible values + * (booleans, "true"/"false" strings) are coerced; anything else present — + * including an unresolved `${ENV_VAR}` placeholder left by expandEnvVarsDeep + * when the variable is unset — is DELETED with a warning, so the field's + * documented default applies. Without this, a truthy junk value silently + * flips whichever direction the consuming gate happens to test (e.g. + * `refreshTokenRequiresOfflineAccess: ${FLAG}` with FLAG unset would activate + * a documented default-off gate). + */ +function normalizeBooleanField(obj: Record, field: string, path: string, logger?: Logger): void { + const value = obj[field]; + if (value === undefined || value === null) return; // Absent (or bare YAML key) — default applies already. + const coerced = coerceConfigBoolean(value); + if (coerced !== undefined) { + obj[field] = coerced; + return; + } + const isUnresolvedPlaceholder = typeof value === 'string' && /^\$\{[^}]*\}$/.test(value.trim()); + logger?.warn?.( + isUnresolvedPlaceholder + ? `MCP: ${path} is the unresolved env placeholder ${JSON.stringify(value)} (variable unset). ` + + 'Treating the option as absent — its documented default applies.' + : `MCP: ${path} must be a boolean; got ${JSON.stringify(value)}. ` + + 'Treating the option as absent — its documented default applies.' + ); + delete obj[field]; +} + /** * Normalize the security-relevant fields of the `mcp` config block in place, - * so a mis-typed value can never silently fail open: - * - `mcp.enabled` and `mcp.clientIdMetadataDocuments.enabled` are coerced from - * documented boolean strings to real booleans (an env-expanded `"false"` - * would otherwise be truthy and enable the feature the operator disabled). + * so a mis-typed value can never silently flip a gate: + * - Every documented boolean (`mcp.enabled`, + * `mcp.refreshTokenRequiresOfflineAccess`, `mcp.clientCredentials.enabled`, + * `mcp.clientIdMetadataDocuments.enabled`, + * `mcp.dynamicClientRegistration.enabled`) is normalized totally via + * {@link normalizeBooleanField}: coerced to a real boolean, or removed with + * a warning so the documented default applies. Consumers may therefore gate + * on plain truthiness / `!== false` without re-validating types. * - `mcp.clientIdMetadataDocuments.allowedHosts` is normalized to an array of * exact, lowercased hostnames. A scalar string (which `Array.includes` / * `String.includes` would turn into substring matching) is wrapped into a * single-element array; anything that isn't a string or array of strings is * rejected rather than treated as "no restriction". - * - `mcp.clientCredentials.enabled` is coerced the same way — this flag mints - * tokens for headless agents, so a stray truthy string must not enable it - * (it is explicit opt-in, default OFF). */ -export function normalizeMcpSecurityConfig(mcpConfig: Record): void { - const enabled = coerceConfigBoolean(mcpConfig.enabled); - if (enabled !== undefined) mcpConfig.enabled = enabled; +export function normalizeMcpSecurityConfig(mcpConfig: Record, logger?: Logger): void { + normalizeBooleanField(mcpConfig, 'enabled', 'mcp.enabled', logger); + normalizeBooleanField( + mcpConfig, + 'refreshTokenRequiresOfflineAccess', + 'mcp.refreshTokenRequiresOfflineAccess', + logger + ); const clientCredentials = mcpConfig.clientCredentials; if (clientCredentials && typeof clientCredentials === 'object') { - const ccEnabled = coerceConfigBoolean(clientCredentials.enabled); - if (ccEnabled !== undefined) clientCredentials.enabled = ccEnabled; + normalizeBooleanField(clientCredentials, 'enabled', 'mcp.clientCredentials.enabled', logger); + } + + const dcr = mcpConfig.dynamicClientRegistration; + if (dcr && typeof dcr === 'object') { + normalizeBooleanField(dcr, 'enabled', 'mcp.dynamicClientRegistration.enabled', logger); } const cimd = mcpConfig.clientIdMetadataDocuments; if (cimd && typeof cimd === 'object') { - const cimdEnabled = coerceConfigBoolean(cimd.enabled); - if (cimdEnabled !== undefined) cimd.enabled = cimdEnabled; + normalizeBooleanField(cimd, 'enabled', 'mcp.clientIdMetadataDocuments.enabled', logger); if (cimd.allowedHosts !== undefined) { const raw = Array.isArray(cimd.allowedHosts) ? cimd.allowedHosts : [cimd.allowedHosts]; diff --git a/src/lib/mcp/token.ts b/src/lib/mcp/token.ts index 81f0f16..6470ff8 100644 --- a/src/lib/mcp/token.ts +++ b/src/lib/mcp/token.ts @@ -130,6 +130,26 @@ function allowsRefresh(client: MCPClientRecord): boolean { return !client.grant_types || client.grant_types.includes('refresh_token'); } +/** Does a space-delimited scope string include `offline_access` (SEP-2207)? */ +function scopeIncludesOfflineAccess(scope: string | undefined): boolean { + return !!scope && scope.split(/\s+/).includes('offline_access'); +} + +/** + * Should the authorization_code exchange issue a refresh token? Always gated + * on the client's registered grant_types; when + * `mcp.refreshTokenRequiresOfflineAccess` is set, additionally requires the + * granted scope to carry the explicit `offline_access` opt-in (SEP-2207). + * Default policy is grant_types-only — most MCP clients never request + * offline_access, and withholding refresh tokens from them would force + * hourly re-auth. + */ +function shouldIssueRefresh(client: MCPClientRecord, scope: string | undefined, mcpConfig: MCPConfig): boolean { + if (!allowsRefresh(client)) return false; + if (mcpConfig.refreshTokenRequiresOfflineAccess) return scopeIncludesOfflineAccess(scope); + return true; +} + /** Constant-time string compare; length-checks first (timingSafeEqual needs equal length). */ function safeEqual(a: string, b: string): boolean { const ab = Buffer.from(a); @@ -272,7 +292,8 @@ async function mintTokenPair( }; if (grant.scope) responseBody.scope = grant.scope; - // Only issue a refresh token if the client registered the refresh_token grant. + // Refresh issuance is decided by the caller (shouldIssueRefresh: client + // grant_types, plus the offline_access scope opt-in when configured). if (grant.issueRefresh) { const familyId = newFamilyId(); const { token: refreshToken, hash } = makeRefreshToken(familyId); @@ -376,7 +397,7 @@ async function handleAuthorizationCodeGrant( resource: record.resource, scope: record.scope, clientId: client.client_id, - issueRefresh: allowsRefresh(client), + issueRefresh: shouldIssueRefresh(client, record.scope, mcpConfig), }, hookManager, logger diff --git a/src/lib/mcp/wellKnown.ts b/src/lib/mcp/wellKnown.ts index 71a1fdb..5791faa 100644 --- a/src/lib/mcp/wellKnown.ts +++ b/src/lib/mcp/wellKnown.ts @@ -136,6 +136,10 @@ export function protectedResourceMetadataUrl(request: HarperRequest, mcpConfig: export function buildProtectedResourceMetadata(request: HarperRequest, mcpConfig: MCPConfig): Record { const resource = resolveResource(request, mcpConfig); const issuer = resolveIssuer(request, mcpConfig); + // No scopes_supported here by design: SEP-2207 — the protected resource + // must not advertise `offline_access` (refresh tokens are not a resource + // requirement), and MCP token scoping is role-level with no fixed resource + // scope vocabulary to publish. return { resource, authorization_servers: [issuer], @@ -183,6 +187,12 @@ export async function buildAuthorizationServerMetadata( ...(clientCredentialsEnabled ? ['client_credentials'] : []), ], code_challenge_methods_supported: ['S256'], + // SEP-2207: advertising `offline_access` here signals that clients + // wanting refresh tokens MAY add it to their authorization/token scope + // (the MCP spec conditions that MAY on AS metadata listing it). RFC 8414 + // permits a partial list; upstream-provider scopes are opaque + // pass-through and are deliberately not enumerated. + scopes_supported: ['offline_access'], token_endpoint_auth_methods_supported: [ 'none', 'client_secret_basic', diff --git a/src/types.ts b/src/types.ts index c5aec2e..3a99828 100644 --- a/src/types.ts +++ b/src/types.ts @@ -113,6 +113,16 @@ export interface MCPConfig { accessTokenTtl?: number; /** Refresh-token (family) lifetime in seconds. Default: 2592000 (30d). */ refreshTokenTtl?: number; + /** + * When true, the authorization_code exchange only issues a refresh token + * when the granted scope includes `offline_access` (the SEP-2207 + * OIDC-flavored opt-in). Default: false — any client whose registered + * grant_types include refresh_token receives one, since most MCP clients + * never request offline_access and withholding refresh tokens would force + * re-auth every accessTokenTtl. Flipping this on does not revoke existing + * refresh-token families. + */ + refreshTokenRequiresOfflineAccess?: boolean; /** * Client ID Metadata Document (CIMD) resolution settings. * CIMD is enabled by default when `mcp.enabled: true`. Override here diff --git a/test/lib/config.test.js b/test/lib/config.test.js index cd84b24..83b6bab 100644 --- a/test/lib/config.test.js +++ b/test/lib/config.test.js @@ -69,6 +69,49 @@ describe('OAuth Configuration', () => { assert.equal(cfg.enabled, false); assert.equal(cfg.clientIdMetadataDocuments.enabled, false); }); + it('coerces refreshTokenRequiresOfflineAccess (string "false" must not activate the gate)', () => { + const cfg = { refreshTokenRequiresOfflineAccess: 'false' }; + normalizeMcpSecurityConfig(cfg); + assert.equal(cfg.refreshTokenRequiresOfflineAccess, false); + const cfgTrue = { refreshTokenRequiresOfflineAccess: 'true' }; + normalizeMcpSecurityConfig(cfgTrue); + assert.equal(cfgTrue.refreshTokenRequiresOfflineAccess, true); + }); + it('drops an unresolved "${FLAG}" placeholder so a documented-off gate stays off, and warns', () => { + // expandEnvVarsDeep leaves "${FLAG}" intact when FLAG is unset; that + // string is truthy and must not activate the gate (PR #192 review). + const warnings = []; + const logger = { warn: (...args) => warnings.push(args.join(' ')) }; + const cfg = { refreshTokenRequiresOfflineAccess: '${FLAG}' }; + normalizeMcpSecurityConfig(cfg, logger); + assert.equal(cfg.refreshTokenRequiresOfflineAccess, undefined, 'placeholder dropped — default applies'); + assert.equal(warnings.length, 1); + assert.match(warnings[0], /unresolved env placeholder/); + }); + it('drops any other non-boolean value with a warning (total normalization)', () => { + const warnings = []; + const logger = { warn: (...args) => warnings.push(args.join(' ')) }; + const cfg = { + enabled: 'yes', + refreshTokenRequiresOfflineAccess: 1, + clientCredentials: { enabled: {} }, + dynamicClientRegistration: { enabled: 'on' }, + clientIdMetadataDocuments: { enabled: 'nope' }, + }; + normalizeMcpSecurityConfig(cfg, logger); + assert.equal(cfg.enabled, undefined); + assert.equal(cfg.refreshTokenRequiresOfflineAccess, undefined); + assert.equal(cfg.clientCredentials.enabled, undefined); + assert.equal(cfg.dynamicClientRegistration.enabled, undefined); + assert.equal(cfg.clientIdMetadataDocuments.enabled, undefined); + assert.equal(warnings.length, 5, 'one warning per dropped field'); + assert.ok(warnings.every((w) => /must be a boolean/.test(w))); + }); + it('normalizes dynamicClientRegistration.enabled ("false" string must actually disable DCR)', () => { + const cfg = { dynamicClientRegistration: { enabled: 'false' } }; + normalizeMcpSecurityConfig(cfg); + assert.equal(cfg.dynamicClientRegistration.enabled, false); + }); it('coerces clientCredentials.enabled the same way (token-minting switch must not be string-truthy)', () => { const cfg = { clientCredentials: { enabled: 'false' } }; normalizeMcpSecurityConfig(cfg); diff --git a/test/lib/mcp/token.test.js b/test/lib/mcp/token.test.js index 49d6961..2a5d04f 100644 --- a/test/lib/mcp/token.test.js +++ b/test/lib/mcp/token.test.js @@ -15,6 +15,7 @@ import { resetMCPClientsTableCache } from '../../../dist/lib/mcp/clientStore.js' import { resetMCPKeysTableCache, SIGNING_KEY_ID } from '../../../dist/lib/mcp/keyStore.js'; import { resetMCPRefreshFamiliesTableCache, makeRefreshToken } from '../../../dist/lib/mcp/refreshTokenStore.js'; import { verifyAccessToken } from '../../../dist/lib/mcp/tokenIssuer.js'; +import { normalizeMcpSecurityConfig } from '../../../dist/lib/config.js'; function asTrackedObject(plain) { return new Proxy(plain, { @@ -251,6 +252,92 @@ describe('handleToken', () => { assert.equal(families.size, 0, 'no refresh family persisted'); }); + // ---- offline_access opt-in (SEP-2207) ---- + + it('withholds the refresh token when refreshTokenRequiresOfflineAccess is set and the scope lacks offline_access', async () => { + seedCode('code-1'); // scope: 'mcp:read' + const res = await handleToken( + { headers: {} }, + { + grant_type: 'authorization_code', + code: 'code-1', + code_verifier: CODE_VERIFIER, + redirect_uri: REDIRECT, + client_id: 'public-1', + }, + { ...mcpConfig, refreshTokenRequiresOfflineAccess: true } + ); + assert.equal(res.status, 200); + assert.ok(res.body.access_token); + assert.equal(res.body.refresh_token, undefined, 'no refresh token without the offline_access opt-in'); + assert.equal(families.size, 0, 'no refresh family persisted'); + }); + + it('issues the refresh token when refreshTokenRequiresOfflineAccess is set and the scope carries offline_access', async () => { + seedCode('code-1', { scope: 'mcp:read offline_access' }); + const res = await handleToken( + { headers: {} }, + { + grant_type: 'authorization_code', + code: 'code-1', + code_verifier: CODE_VERIFIER, + redirect_uri: REDIRECT, + client_id: 'public-1', + }, + { ...mcpConfig, refreshTokenRequiresOfflineAccess: true } + ); + assert.equal(res.status, 200); + assert.ok(res.body.refresh_token, 'offline_access opt-in honored'); + assert.equal(res.body.scope, 'mcp:read offline_access', 'granted scope echoed back'); + assert.equal(families.size, 1, 'refresh family persisted'); + }); + + it('an unresolved "${FLAG}" placeholder does not activate the gate once config is normalized (PR #192 review)', async () => { + // The startup path always runs normalizeMcpSecurityConfig before any + // handler sees the config; this pins the composed behavior — the truthy + // placeholder string is dropped, so the default (issue refresh) applies. + const cfg = { ...mcpConfig, refreshTokenRequiresOfflineAccess: '${FLAG}' }; + normalizeMcpSecurityConfig(cfg); + seedCode('code-1'); // scope: 'mcp:read' — no offline_access + const res = await handleToken( + { headers: {} }, + { + grant_type: 'authorization_code', + code: 'code-1', + code_verifier: CODE_VERIFIER, + redirect_uri: REDIRECT, + client_id: 'public-1', + }, + cfg + ); + assert.equal(res.status, 200); + assert.ok(res.body.refresh_token, 'refresh token issued — documented default-off gate stayed off'); + }); + + it('still gates on grant_types even when offline_access is requested', async () => { + clients.set('noref-1', { + client_id: 'noref-1', + token_endpoint_auth_method: 'none', + grant_types: JSON.stringify(['authorization_code']), + redirect_uris: JSON.stringify([REDIRECT]), + client_id_issued_at: 1700000000, + }); + seedCode('code-1', { client_id: 'noref-1', scope: 'offline_access' }); + const res = await handleToken( + { headers: {} }, + { + grant_type: 'authorization_code', + code: 'code-1', + code_verifier: CODE_VERIFIER, + redirect_uri: REDIRECT, + client_id: 'noref-1', + }, + { ...mcpConfig, refreshTokenRequiresOfflineAccess: true } + ); + assert.equal(res.status, 200); + assert.equal(res.body.refresh_token, undefined, 'grant_types gate still applies'); + }); + // ---- authorization_code rejection branches ---- it('rejects when code, code_verifier, or redirect_uri is missing', async () => { diff --git a/test/lib/mcp/wellKnown.test.js b/test/lib/mcp/wellKnown.test.js index 447edbf..251f233 100644 --- a/test/lib/mcp/wellKnown.test.js +++ b/test/lib/mcp/wellKnown.test.js @@ -86,6 +86,11 @@ describe('MCP well-known: PRM document (RFC 9728)', () => { }); assert.equal(doc.resource, 'https://my-app.example.com/mcp'); }); + + it('does not advertise scopes_supported — offline_access must stay out of PRM (SEP-2207)', () => { + const doc = buildProtectedResourceMetadata(makeRequest(), { enabled: true }); + assert.ok(!('scopes_supported' in doc), 'PRM must not carry scopes_supported'); + }); }); describe('MCP well-known: AS metadata document (RFC 8414)', () => { @@ -171,6 +176,11 @@ describe('MCP well-known: AS metadata document (RFC 8414)', () => { assert.deepEqual(doc.id_token_signing_alg_values_supported, ['RS256']); }); + it('advertises offline_access in scopes_supported (SEP-2207 refresh-token opt-in)', async () => { + const doc = await buildAuthorizationServerMetadata(makeRequest(), { enabled: true }); + assert.deepEqual(doc.scopes_supported, ['offline_access']); + }); + it('advertises ES256 when mcp.signingAlgorithm is ES256 (#127)', async () => { const doc = await buildAuthorizationServerMetadata(makeRequest(), { enabled: true, signingAlgorithm: 'ES256' }); assert.deepEqual(doc.id_token_signing_alg_values_supported, ['ES256']);