From 6747cbd6d3369669ae4f54792fbccec4227003b7 Mon Sep 17 00:00:00 2001 From: andypalmi Date: Mon, 24 Aug 2026 19:47:54 +0200 Subject: [PATCH 1/6] feat(mcp): OAuth2 PKCE flow with dynamic client registration for MCP agents Extend the OAuth2 flow so external MCP agents (Claude, Cursor, etc.) can authenticate. Rather than a single hardcoded client id, agents register dynamically per RFC 7591: POST /account/client persists a public AuthClient (type 'mcp', no secret) with its approved redirect URIs and returns a generated client id. The authorize, complete, and token endpoints recognise these clients by looking them up, skip the project/device ownership checks (MCP is user-scoped), and drive an MCP consent step that records the read-only and team selection before issuing a scoped personal access token. Redirect URIs must be loopback http (RFC 8252, port-flexible) or https for hosted clients; token issuance and refresh require no client secret. Also forward the caller scope through the platform automation handler and add a platform_get_active_user tool that reports the calling token's scope. Adds AuthClient.type/name/redirectURIs (migration + model), an AuthClient.createMCPClient controller, and AccessToken.createMCPOAuthToken. Ref FlowFuse/flowfuse#7432 --- forge/comms/platformAutomation.js | 10 +- forge/db/controllers/AccessToken.js | 31 ++ forge/db/controllers/AuthClient.js | 14 + .../20260824-01-add-mcp-authclient-fields.js | 39 +++ forge/db/models/AuthClient.js | 15 +- forge/ee/lib/mcp/tools/users.js | 34 ++ forge/routes/auth/oauth.js | 297 ++++++++++++++---- .../forge/comms/platformAutomation_spec.js | 39 ++- .../unit/forge/ee/lib/mcp/tools/users_spec.js | 54 ++++ test/unit/forge/routes/auth/oauth_spec.js | 157 +++++++++ 10 files changed, 620 insertions(+), 70 deletions(-) create mode 100644 forge/db/migrations/20260824-01-add-mcp-authclient-fields.js create mode 100644 forge/ee/lib/mcp/tools/users.js create mode 100644 test/unit/forge/ee/lib/mcp/tools/users_spec.js diff --git a/forge/comms/platformAutomation.js b/forge/comms/platformAutomation.js index 1dc14fe6d8..9a054d4f93 100644 --- a/forge/comms/platformAutomation.js +++ b/forge/comms/platformAutomation.js @@ -104,7 +104,7 @@ class PlatformAutomationHandler { this.client.on('request/platform-automation:forge', this.eventHandler) } - eventHandler = async ({ userId, mcpSessionId, command, data, meta } = {}, onSuccess, onError) => { + eventHandler = async ({ userId, mcpSessionId, command, data, meta, scope } = {}, onSuccess, onError) => { try { let result = {} this.app.log.info(`platform-automation request: userId=${userId} mcpSessionId=${mcpSessionId} command=${command} tool=${data?.name || 'n/a'}`) @@ -123,6 +123,12 @@ class PlatformAutomationHandler { const toolName = data?.name const args = data?.input || {} + // The caller scope (readOnly plus any team restriction from the + // session token) rides with the request so tools can report and + // enforce what the session is allowed to do. Prefer the top-level + // value, falling back to meta for callers that attach it there. + const callerScope = scope ?? meta?.scope + // TODO: Probably sensible to verify that toolDefinition matches the tool to ensure no tampering has occurred const { toolDefinition } = meta || {} @@ -163,7 +169,7 @@ class PlatformAutomationHandler { } const { formatResponse } = require('../ee/lib/mcp/toolLoader') - const response = await tool.handler(args, { inject, app: this.app, user, mcpSessionId }) + const response = await tool.handler(args, { inject, app: this.app, user, mcpSessionId, scope: callerScope }) result = formatResponse(response) } break diff --git a/forge/db/controllers/AccessToken.js b/forge/db/controllers/AccessToken.js index 38111550a4..064e83d1df 100644 --- a/forge/db/controllers/AccessToken.js +++ b/forge/db/controllers/AccessToken.js @@ -265,6 +265,37 @@ module.exports = { await app.settings.set('platform:stats:token', false) }, + createMCPOAuthToken: async function (app, userId, { readOnly = false, teamIds = [] } = {}) { + const token = generateToken(32, 'ffpat') + const refreshToken = generateToken(32, 'ffpat') + const expiresAt = Date.now() + DEFAULT_TOKEN_SESSION_EXPIRY + + await app.db.sequelize.transaction(async (t) => { + const tok = await app.db.models.AccessToken.create({ + name: 'MCP Agent', + token, + refreshToken, + scope: '', + expiresAt, + readOnly, + adminOptIn: false, + ownerId: '' + userId, + ownerType: 'user' + }, { transaction: t }) + + if (teamIds.length > 0) { + const scopes = teamIds.map(teamId => ({ + AccessTokenId: tok.id, + TeamId: app.db.models.Team.decodeHashid(teamId), + UserId: userId + })) + await app.db.models.AccessTokenTeamScope.bulkCreate(scopes, { transaction: t }) + } + }) + + return { token, expiresAt, refreshToken } + }, + createPersonalAccessToken: async function (app, user, scope, expiresAt, name, { readOnly = false, adminOptIn = false, teamIds = [] } = {}) { const userId = typeof user === 'number' ? user : user.id const token = generateToken(32, 'ffpat') diff --git a/forge/db/controllers/AuthClient.js b/forge/db/controllers/AuthClient.js index ede14db8d1..adb0d06cca 100644 --- a/forge/db/controllers/AuthClient.js +++ b/forge/db/controllers/AuthClient.js @@ -42,6 +42,20 @@ module.exports = { return client }, + /** + * Register a public MCP client (RFC 7591 Dynamic Client Registration). + * MCP clients have no owner and no secret - they authenticate with PKCE. + * The generated clientID is the only credential returned to the client. + */ + createMCPClient: async function (app, { name, redirectURIs } = {}) { + return app.db.models.AuthClient.create({ + clientID: generateToken(32, 'ffmcp'), + type: 'mcp', + name: name || 'MCP Agent', + redirectURIs: redirectURIs || [] + }) + }, + removeClientForDevice: async function (app, device) { const existingAuthClient = await device.getAuthClient() if (existingAuthClient) { diff --git a/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js b/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js new file mode 100644 index 0000000000..df7fa920b8 --- /dev/null +++ b/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js @@ -0,0 +1,39 @@ +/** + * Add fields to AuthClients so MCP agents can register dynamically (RFC 7591). + * + * Existing clients (project/device editor auth) are owned by a resource via + * ownerType/ownerId and authenticate with a clientSecret. MCP clients have no + * owner and are public (PKCE, no secret), so they need somewhere to record the + * client type, a display name, and the redirect URIs approved at registration. + * + * type - 'mcp' for dynamically registered MCP clients, null otherwise + * name - the client_name supplied at registration + * redirectURIs - JSON array of redirect URIs the client may use + */ + +const { DataTypes } = require('sequelize') + +module.exports = { + up: async (context) => { + await context.addColumn('AuthClients', 'type', { + type: DataTypes.STRING, + allowNull: true, + defaultValue: null + }) + await context.addColumn('AuthClients', 'name', { + type: DataTypes.STRING, + allowNull: true, + defaultValue: null + }) + await context.addColumn('AuthClients', 'redirectURIs', { + type: DataTypes.TEXT, + allowNull: true, + defaultValue: null + }) + }, + down: async (context) => { + await context.removeColumn('AuthClients', 'type') + await context.removeColumn('AuthClients', 'name') + await context.removeColumn('AuthClients', 'redirectURIs') + } +} diff --git a/forge/db/models/AuthClient.js b/forge/db/models/AuthClient.js index dfbb602b1a..17def3bb02 100644 --- a/forge/db/models/AuthClient.js +++ b/forge/db/models/AuthClient.js @@ -18,7 +18,20 @@ module.exports = { } }, ownerId: { type: DataTypes.STRING }, - ownerType: { type: DataTypes.STRING } + ownerType: { type: DataTypes.STRING }, + // 'mcp' for dynamically registered MCP clients (public, no secret), null otherwise + type: { type: DataTypes.STRING, allowNull: true }, + name: { type: DataTypes.STRING, allowNull: true }, + redirectURIs: { + type: DataTypes.TEXT, + get () { + const rawValue = this.getDataValue('redirectURIs') + return rawValue ? JSON.parse(rawValue) : [] + }, + set (value) { + this.setDataValue('redirectURIs', JSON.stringify(value || [])) + } + } }, associations: function (M) { this.belongsTo(M.Project, { foreignKey: 'ownerId', constraints: false }) diff --git a/forge/ee/lib/mcp/tools/users.js b/forge/ee/lib/mcp/tools/users.js new file mode 100644 index 0000000000..01df71d48f --- /dev/null +++ b/forge/ee/lib/mcp/tools/users.js @@ -0,0 +1,34 @@ +module.exports = [ + { + name: 'platform_get_active_user', + title: 'Get Active User', + description: `FlowFuse platform automation tool: + Get the profile of the user this MCP session is authenticated as, along with what the session's token is allowed to do. + Returns the user's ID (hashid), username, name, email and admin flag, plus a token object: + token.readOnly - when true, write and delete tools are rejected, so only read tools can be used. + token.allTeams - when true, the token is not restricted to a subset of teams and every team the user belongs to is in reach. + token.teams - when allTeams is false, the IDs of the only teams the token may act on. Calls against any other team will fail. + Use this to resolve the current user's ID before calling tools that need a userId, such as listing browser sessions, + and to check up front whether an action the user asked for is within the session's access.`, + annotations: { readOnlyHint: true, destructiveHint: false }, + inputSchema: {}, + handler: async (args, { inject, scope }) => { + const response = await inject({ method: 'GET', url: '/api/v1/user' }) + if (response.statusCode >= 400) { + return response + } + + // No scope means the tool was not called through a scoped token (the FlowFuse + // Expert path), so the session has the user's full access. + const teams = Array.isArray(scope?.teams) ? scope.teams : [] + return { + ...response.json(), + token: { + readOnly: scope?.readOnly === true, + allTeams: teams.length === 0, + teams + } + } + } + } +] diff --git a/forge/routes/auth/oauth.js b/forge/routes/auth/oauth.js index df128ee715..bfb3a04cd6 100644 --- a/forge/routes/auth/oauth.js +++ b/forge/routes/auth/oauth.js @@ -14,6 +14,37 @@ function badRequest (reply, error, description) { }) } +// A loopback redirect_uri may vary its port between registration and use +// (RFC 8252 Section 7.3), and localhost/127.0.0.1 are interchangeable. Every +// other redirect must match a registered value exactly. +function isLoopbackHost (hostname) { + return hostname === 'localhost' || hostname === '127.0.0.1' +} + +function redirectUriMatches (registeredURIs, requested) { + if (registeredURIs.includes(requested)) { + return true + } + let req + try { + req = new URL(requested) + } catch (err) { + return false + } + if (!isLoopbackHost(req.hostname)) { + return false + } + return registeredURIs.some((uri) => { + let reg + try { + reg = new URL(uri) + } catch (err) { + return false + } + return isLoopbackHost(reg.hostname) && reg.protocol === req.protocol && reg.pathname === req.pathname + }) +} + function redirectInvalidRequest (reply, redirectURI, error, errorDescription, state) { const responseUrl = new URL(redirectURI) const response = { error, errorDescription } @@ -59,7 +90,7 @@ module.exports = async function (app) { code_challenge_method: { type: 'string' } }, // client_id and redirect_uri are handled manually - required: ['response_type', 'scope', 'code_challenge', 'code_challenge_method'] + required: ['response_type', 'code_challenge', 'code_challenge_method'] } }, attachValidation: true @@ -88,25 +119,8 @@ module.exports = async function (app) { } catch (err) { return badRequest(reply, 'invalid_request', 'Invalid redirect_uri') } - if (client_id !== 'ff-plugin') { - // Check client_id is valid. Note - no client_secret provided at this point - const authClient = await app.db.controllers.AuthClient.getAuthClient(client_id) - if (!authClient) { - return badRequest(reply, 'invalid_request', 'Invalid client_id') - } - // Ensure redirect_uri path component is correct - if ( - // HTTP Auth callback - !/\/_ffAuth\/callback$/.test(redirectURI.pathname) && - // Admin Auth callback - !/\/auth\/strategy\/callback$/.test(redirectURI.pathname) - ) { - return badRequest(reply, 'invalid_request', 'Invalid redirect_uri') - } - if (!/^(editor($|-))|httpAuth-/.test(scope)) { - return redirectInvalidRequest(reply, redirect_uri, 'invalid_request', "Invalid scope '" + scope + "'. Only 'editor[-version]' is supported", state) - } - } else { + let isMCP = false + if (client_id === 'ff-plugin') { // Ensure redirect_uri path component is correct for the tools plugin if (!/\/(flow(fuse|forge)-nr-tools|nr-assistant)\/auth\/callback$/.test(redirectURI.pathname)) { return badRequest(reply, 'invalid_request', 'Invalid redirect_uri') @@ -114,6 +128,34 @@ module.exports = async function (app) { if (scope !== 'ff-plugin' && scope !== 'ff-assistant') { return redirectInvalidRequest(reply, redirect_uri, 'invalid_request', "Invalid scope '" + scope + "'", state) } + } else { + const authClient = await app.db.controllers.AuthClient.getAuthClient(client_id) + if (!authClient) { + return badRequest(reply, 'invalid_request', 'Invalid client_id') + } + if (authClient.type === 'mcp') { + // MCP agent (dynamically registered): redirect_uri must match one + // approved at registration. Scope is not validated here - the access + // level is chosen by the user on the consent page (readOnly + teamIds). + if (!redirectUriMatches(authClient.redirectURIs, redirect_uri)) { + return badRequest(reply, 'invalid_request', 'Invalid redirect_uri') + } + isMCP = true + } else { + // Dynamic client (project/device editor auth) + // Ensure redirect_uri path component is correct + if ( + // HTTP Auth callback + !/\/_ffAuth\/callback$/.test(redirectURI.pathname) && + // Admin Auth callback + !/\/auth\/strategy\/callback$/.test(redirectURI.pathname) + ) { + return badRequest(reply, 'invalid_request', 'Invalid redirect_uri') + } + if (!/^(editor($|-))|httpAuth-/.test(scope)) { + return redirectInvalidRequest(reply, redirect_uri, 'invalid_request', "Invalid scope '" + scope + "'. Only 'editor[-version]' is supported", state) + } + } } // If anything else missing, redirect with details if (request.validationError) { @@ -137,7 +179,8 @@ module.exports = async function (app) { redirect_uri, state, code_challenge, - code_challenge_method + code_challenge_method, + mcp: isMCP } const requestId = base64URLEncode(crypto.randomBytes(32)) await requestCache.set(requestId, requestObject) @@ -159,6 +202,11 @@ module.exports = async function (app) { reply.redirect(`${app.config.base_url}/account/request/${requestId}/editor`) return } + if (isMCP) { + // Redirect to MCP-specific consent page + reply.redirect(`${app.config.base_url}/account/request/${requestId}/mcp`) + return + } // Redirect to login page with requestId in url - to bounce to an approve page reply.redirect(`${app.config.base_url}/account/request/${requestId}`) }) @@ -176,8 +224,8 @@ module.exports = async function (app) { if (request.sid) { request.session = await app.db.controllers.Session.getOrExpire(request.sid) if (request.session) { - if (requestObject.client_id === 'ff-plugin') { - // This is the FlowFuse Node-RED plugin. + if (requestObject.client_id === 'ff-plugin' || requestObject.mcp) { + // FlowFuse Node-RED plugin or MCP agent: user-scoped, no resource ownership checks } else { const authClient = await app.db.controllers.AuthClient.getAuthClient(requestObject.client_id) if (!authClient) { @@ -267,6 +315,106 @@ module.exports = async function (app) { return redirectInvalidRequest(reply, requestObject.redirect_uri, 'access_denied', 'Access Denied', requestObject.state) }) + // RFC 7591: Dynamic Client Registration for MCP agents + app.post('/account/client', { + config: { allowAnonymous: true }, + schema: { + tags: ['Authentication', 'X-HIDDEN'], + body: { + type: 'object', + properties: { + redirect_uris: { type: 'array', items: { type: 'string' } }, + client_name: { type: 'string' }, + grant_types: { type: 'array', items: { type: 'string' } }, + response_types: { type: 'array', items: { type: 'string' } }, + token_endpoint_auth_method: { type: 'string' } + }, + required: ['redirect_uris'] + } + } + }, async function (request, reply) { + const { redirect_uris, client_name, grant_types, response_types } = request.body + + if (!Array.isArray(redirect_uris) || redirect_uris.length === 0) { + return badRequest(reply, 'invalid_redirect_uri', 'At least one redirect_uri is required') + } + // A redirect_uri must either be a loopback http address (local dev tools, + // RFC 8252 Section 7.3) or a secure https address (a hosted client, e.g. a + // provider connector). Anything else is rejected. + for (const uri of redirect_uris) { + let parsed + try { + parsed = new URL(uri) + } catch (err) { + return badRequest(reply, 'invalid_redirect_uri', `Invalid redirect_uri: ${uri}`) + } + const loopbackHttp = isLoopbackHost(parsed.hostname) && parsed.protocol === 'http:' + const secure = parsed.protocol === 'https:' + if (!loopbackHttp && !secure) { + return badRequest(reply, 'invalid_redirect_uri', 'redirect_uri must be a loopback http address or an https address') + } + } + + const client = await app.db.controllers.AuthClient.createMCPClient({ + name: client_name, + redirectURIs: redirect_uris + }) + + // MCP clients are public (PKCE, no secret), so no client_secret is issued. + reply.code(201).send({ + client_id: client.clientID, + client_id_issued_at: Math.floor(client.createdAt.getTime() / 1000), + client_name: client.name, + redirect_uris, + grant_types: grant_types || ['authorization_code', 'refresh_token'], + response_types: response_types || ['code'], + token_endpoint_auth_method: 'none' + }) + }) + + // MCP consent: save the user's access choices before they approve + app.put('/account/authorize/:id/consent', { + preHandler: (request, reply) => app.verifySession(request, reply), + schema: { + tags: ['Authentication', 'X-HIDDEN'], + params: { + type: 'object', + properties: { + id: { type: 'string' } + }, + required: ['id'] + }, + body: { + type: 'object', + properties: { + readOnly: { type: 'boolean' }, + teamIds: { type: 'array', items: { type: 'string' } } + } + } + } + }, async function (request, reply) { + const requestId = request.params.id + const { readOnly = false, teamIds = [] } = request.body + + const session = await app.db.models.OAuthSession.findOne({ where: { id: requestId } }) + if (!session) { + return badRequest(reply, 'invalid_request', 'Invalid or expired request') + } + if (Date.now() - session.createdAt.getTime() >= 1000 * 60 * 5) { + await session.destroy() + return badRequest(reply, 'invalid_request', 'Request has expired') + } + const requestObject = session.value + if (!requestObject.mcp) { + return badRequest(reply, 'invalid_request', 'Invalid request') + } + + session.value = { ...requestObject, readOnly, teamIds } + await session.save() + + reply.send({ status: 'ok' }) + }) + app.post('/account/token', { config: { rateLimit: false // never rate limit this route @@ -342,7 +490,56 @@ module.exports = async function (app) { return } - if (client_id !== 'ff-plugin') { + if (client_id === 'ff-plugin') { + const scope = { + 'ff-plugin': [ + 'user:read', + 'user:team:list', + 'team:read', + 'team:projects:list', + 'project:read', + 'project:snapshot:list', + 'project:snapshot:create', + 'device:snapshot:list', + 'device:snapshot:create' + ], + 'ff-assistant': [ + 'user:read', + 'assistant:call' + ] + }[requestObject.scope] + if (!scope) { + return badRequest(reply, 'access_denied', 'Access Denied') + } + const accessToken = await app.db.controllers.AccessToken.createTokenForUser(requestObject.userId, + null, + scope, + true + ) + const response = { + access_token: accessToken.token, + expires_in: Math.floor((accessToken.expiresAt - Date.now()) / 1000), + refresh_token: accessToken.refreshToken, + state: requestObject.state + } + reply.send(response) + } else if (requestObject.mcp) { + const accessToken = await app.db.controllers.AccessToken.createMCPOAuthToken( + requestObject.userId, + { + readOnly: requestObject.readOnly || false, + teamIds: requestObject.teamIds || [] + } + ) + const response = { + access_token: accessToken.token, + token_type: 'bearer', + expires_in: Math.floor((accessToken.expiresAt - Date.now()) / 1000), + refresh_token: accessToken.refreshToken, + state: requestObject.state + } + reply.send(response) + } else { const authClient = await app.db.controllers.AuthClient.getAuthClient(client_id, client_secret) if (!authClient) { return badRequest(reply, 'invalid_request', 'Invalid client_id') @@ -404,39 +601,6 @@ module.exports = async function (app) { scope } reply.send(response) - } else { - const scope = { - 'ff-plugin': [ - 'user:read', - 'user:team:list', - 'team:read', - 'team:projects:list', - 'project:read', - 'project:snapshot:list', - 'project:snapshot:create', - 'device:snapshot:list', - 'device:snapshot:create' - ], - 'ff-assistant': [ - 'user:read', - 'assistant:call' - ] - }[requestObject.scope] - if (!scope) { - return badRequest(reply, 'access_denied', 'Access Denied') - } - const accessToken = await app.db.controllers.AccessToken.createTokenForUser(requestObject.userId, - null, - scope, - true - ) - const response = { - access_token: accessToken.token, - expires_in: Math.floor((accessToken.expiresAt - Date.now()) / 1000), - refresh_token: accessToken.refreshToken, - state: requestObject.state - } - reply.send(response) } } else if (grant_type === 'refresh_token') { const existingToken = await app.db.models.AccessToken.byRefreshToken(refresh_token) @@ -444,23 +608,26 @@ module.exports = async function (app) { badRequest(reply, 'invalid_request', 'Invalid refresh_token') return } + // ff-plugin and MCP clients are user-scoped; only project/device + // clients need their resource ownership re-checked on refresh. + let refreshAuthClient = null if (client_id !== 'ff-plugin') { - const authClient = await app.db.controllers.AuthClient.getAuthClient(client_id, client_secret) - if (!authClient) { + refreshAuthClient = await app.db.controllers.AuthClient.getAuthClient(client_id, client_secret) + if (!refreshAuthClient) { return badRequest(reply, 'invalid_request', 'Invalid client_id') } - // We have validated client_id and client_secret by this point. - + } + if (refreshAuthClient && refreshAuthClient.type !== 'mcp') { // Check the owner of the existing session still has access to the project // this client is owned by let owner = null let applicationId - if (authClient.ownerType === 'project') { - owner = await app.db.models.Project.byId(authClient.ownerId) + if (refreshAuthClient.ownerType === 'project') { + owner = await app.db.models.Project.byId(refreshAuthClient.ownerId) // Project.byId will include the full Application object applicationId = owner?.Application.hashid - } else if (authClient.ownerType === 'device') { - owner = await app.db.models.Device.byId(parseInt(authClient.ownerId)) + } else if (refreshAuthClient.ownerType === 'device') { + owner = await app.db.models.Device.byId(parseInt(refreshAuthClient.ownerId)) // Device.byId does not include the full Application object if (owner?.ApplicationId) { applicationId = app.db.models.Application.encodeHashid(owner.ApplicationId) diff --git a/test/unit/forge/comms/platformAutomation_spec.js b/test/unit/forge/comms/platformAutomation_spec.js index 01972422ad..a24bfd7f3b 100644 --- a/test/unit/forge/comms/platformAutomation_spec.js +++ b/test/unit/forge/comms/platformAutomation_spec.js @@ -24,7 +24,7 @@ describe('PlatformAutomationHandler', function () { } } - function invokeToolCall ({ userId, toolName, args, meta }) { + function invokeToolCall ({ userId, toolName, args, meta, scope }) { return new Promise((resolve) => { const onSuccess = (result) => resolve({ ok: true, result }) const onError = (message, code, err) => resolve({ ok: false, message, code, err }) @@ -33,7 +33,8 @@ describe('PlatformAutomationHandler', function () { userId, command: 'mcp-call-tool', data: { name: toolName, input: args || {} }, - meta + meta, + scope }, onSuccess, onError @@ -192,6 +193,40 @@ describe('PlatformAutomationHandler', function () { }) }) + describe('caller scope', function () { + function callActiveUser (message) { + const tool = handler.findTool('platform_get_active_user') + return invokeToolCall({ + ...message, + userId: app.adminUser.hashid, + toolName: 'platform_get_active_user', + meta: { toolDefinition: { annotations: tool.annotations }, ...message.meta } + }) + } + + it('passes the caller scope through to the tool handler', async function () { + const res = await callActiveUser({ scope: { readOnly: true, teams: [app.team.hashid] } }) + + res.ok.should.be.true() + res.result.should.have.property('username', app.adminUser.username) + res.result.token.should.eql({ readOnly: true, allTeams: false, teams: [app.team.hashid] }) + }) + + it('accepts the caller scope on meta', async function () { + const res = await callActiveUser({ meta: { scope: { readOnly: true, teams: [] } } }) + + res.ok.should.be.true() + res.result.token.should.eql({ readOnly: true, allTeams: true, teams: [] }) + }) + + it('treats a call with no scope as full user access', async function () { + const res = await callActiveUser({}) + + res.ok.should.be.true() + res.result.token.should.eql({ readOnly: false, allTeams: true, teams: [] }) + }) + }) + describe('mcp-get-features', function () { it('returns the tool list along with a catalogHash', async function () { const res = await invokeGetFeatures() diff --git a/test/unit/forge/ee/lib/mcp/tools/users_spec.js b/test/unit/forge/ee/lib/mcp/tools/users_spec.js new file mode 100644 index 0000000000..f832a1febd --- /dev/null +++ b/test/unit/forge/ee/lib/mcp/tools/users_spec.js @@ -0,0 +1,54 @@ +const should = require('should') // eslint-disable-line no-unused-vars +const sinon = require('sinon') + +const tools = require('../../../../../../../forge/ee/lib/mcp/tools/users') + +function getTool (name) { + return tools.find(tool => tool.name === name) +} + +describe('MCP Users Tools', function () { + let inject + + beforeEach(function () { + inject = sinon.stub() + }) + + describe('platform_get_active_user', function () { + const tool = getTool('platform_get_active_user') + + function userResponse (body = { id: 'user1', username: 'alice' }) { + return { statusCode: 200, json: () => body } + } + + it('calls the user endpoint and returns the profile with token metadata', async function () { + inject.resolves(userResponse()) + const response = await tool.handler({}, { inject, scope: { readOnly: true, teams: ['team1', 'team2'] } }) + inject.firstCall.args[0].should.eql({ method: 'GET', url: '/api/v1/user' }) + response.should.eql({ + id: 'user1', + username: 'alice', + token: { readOnly: true, allTeams: false, teams: ['team1', 'team2'] } + }) + }) + + it('reports full access when the token is not restricted to any team', async function () { + inject.resolves(userResponse()) + const response = await tool.handler({}, { inject, scope: { readOnly: false, teams: [] } }) + response.token.should.eql({ readOnly: false, allTeams: true, teams: [] }) + }) + + it('reports full access when no scope is provided', async function () { + inject.resolves(userResponse()) + const response = await tool.handler({}, { inject }) + response.token.should.eql({ readOnly: false, allTeams: true, teams: [] }) + }) + + it('returns the response unmodified when the request fails', async function () { + const injectResponse = { statusCode: 401, json: () => ({ code: 'unauthorized' }) } + inject.resolves(injectResponse) + const response = await tool.handler({}, { inject, scope: { readOnly: true, teams: [] } }) + response.should.equal(injectResponse) + }) + }) +}) diff --git a/test/unit/forge/routes/auth/oauth_spec.js b/test/unit/forge/routes/auth/oauth_spec.js index b403c90a21..2f0fbc7b20 100644 --- a/test/unit/forge/routes/auth/oauth_spec.js +++ b/test/unit/forge/routes/auth/oauth_spec.js @@ -319,4 +319,161 @@ describe('OAuth', async function () { scope.should.equal('read') }) }) + + describe('MCP agent auth (DCR + PKCE)', async function () { + let mcpApp + let sid + const redirectURI = 'http://localhost:9876/oauth/callback' + + function pkce () { + const verifier = base64URLEncode(crypto.randomBytes(32)) + const challenge = base64URLEncode(crypto.createHash('sha256').update(verifier).digest()) + return { verifier, challenge } + } + + async function register (redirectURIs = [redirectURI]) { + return mcpApp.inject({ + method: 'POST', + url: '/account/client', + payload: { redirect_uris: redirectURIs, client_name: 'Test MCP Client' } + }) + } + + function authorizeURL (clientID, redirect, challenge, state = '') { + const params = new URLSearchParams({ + client_id: clientID, + response_type: 'code', + redirect_uri: redirect, + state, + code_challenge: challenge, + code_challenge_method: 'S256' + }) + return `/account/authorize?${params}` + } + + before(async function () { + mcpApp = await setup({ + license: 'eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJGbG93Rm9yZ2UgSW5jLiIsInN1YiI6IkZsb3dGb3JnZSBJbmMuIERldmVsb3BtZW50IiwibmJmIjoxNjYyNDIyNDAwLCJleHAiOjc5ODY5MDIzOTksIm5vdGUiOiJEZXZlbG9wbWVudC1tb2RlIE9ubHkuIE5vdCBmb3IgcHJvZHVjdGlvbiIsInVzZXJzIjoxNTAsInRlYW1zIjo1MCwicHJvamVjdHMiOjUwLCJkZXZpY2VzIjo1MCwiZGV2Ijp0cnVlLCJpYXQiOjE2NjI0ODI5ODd9.e8Jeppq4aURwWYz-rEpnXs9RY2Y7HF7LJ6rMtMZWdw2Xls6-iyaiKV1TyzQw5sUBAhdUSZxgtiFH5e_cNJgrUg' + }) + const user = await mcpApp.factory.createUser({ + username: 'mcpuser', + name: 'MCP User', + email: 'mcp@example.com', + password: 'mmPassword' + }) + await mcpApp.team.addUser(user, { through: { role: mcpApp.factory.Roles.Roles.Owner } }) + const loginResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/login', + payload: { username: 'mcpuser', password: 'mmPassword', remember: false } + }) + sid = loginResponse.cookies[0].value + }) + after(async function () { + await mcpApp.close() + }) + + it('registers a public MCP client via DCR (RFC 7591)', async function () { + const response = await register() + response.should.have.property('statusCode', 201) + const body = response.json() + body.client_id.should.be.a.String().and.startWith('ffmcp') + body.should.have.property('token_endpoint_auth_method', 'none') + body.should.not.have.property('client_secret') + body.redirect_uris.should.eql([redirectURI]) + + const client = await mcpApp.db.controllers.AuthClient.getAuthClient(body.client_id) + client.should.have.property('type', 'mcp') + client.redirectURIs.should.eql([redirectURI]) + }) + + it('rejects a redirect_uri that is neither loopback-http nor https', async function () { + const response = await register(['http://example.com/callback']) + response.should.have.property('statusCode', 400) + response.json().should.have.property('error', 'invalid_redirect_uri') + }) + + it('accepts an https redirect_uri for hosted clients', async function () { + const response = await register(['https://claude.ai/api/mcp/callback']) + response.should.have.property('statusCode', 201) + }) + + it('completes the full flow: register -> authorize -> consent -> complete -> token -> refresh', async function () { + const reg = (await register()).json() + const clientID = reg.client_id + const { verifier, challenge } = pkce() + + // authorize -> redirected to the MCP consent page + const authResponse = await mcpApp.inject({ method: 'GET', url: authorizeURL(clientID, redirectURI, challenge, 'xyz'), cookies: { sid } }) + authResponse.should.have.property('statusCode', 302) + const m = /\/account\/request\/([^/]+)\/mcp$/.exec(authResponse.headers.location) + should.exist(m, 'expected redirect to MCP consent page: ' + authResponse.headers.location) + const requestId = m[1] + + // consent - user chooses read-only, scoped to their team + const consentResponse = await mcpApp.inject({ + method: 'PUT', + url: `/account/authorize/${requestId}/consent`, + payload: { readOnly: true, teamIds: [mcpApp.team.hashid] }, + cookies: { sid } + }) + consentResponse.should.have.property('statusCode', 200) + + // complete - issues the authorization code and redirects back to the client + const completeResponse = await mcpApp.inject({ method: 'GET', url: `/account/complete/${requestId}`, cookies: { sid } }) + completeResponse.should.have.property('statusCode', 302) + const callback = new URL(completeResponse.headers.location) + callback.host.should.equal('localhost:9876') + const authCode = callback.searchParams.get('code') + should.exist(authCode) + callback.searchParams.get('state').should.equal('xyz') + + // token - exchange the code for a scoped PAT + const tokenResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/token', + payload: { + grant_type: 'authorization_code', + code: authCode, + redirect_uri: redirectURI, + client_id: clientID, + code_verifier: verifier + } + }) + tokenResponse.should.have.property('statusCode', 200) + const token = tokenResponse.json() + token.access_token.should.be.a.String().and.startWith('ffpat') + token.should.have.property('token_type', 'bearer') + token.should.have.property('refresh_token') + + // the issued token reflects the consent choices + const issued = await mcpApp.db.models.AccessToken.byRefreshToken(token.refresh_token) + issued.should.have.property('readOnly', true) + + // refresh - a public MCP client refreshes without a client secret + const refreshResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/token', + payload: { grant_type: 'refresh_token', client_id: clientID, refresh_token: token.refresh_token } + }) + refreshResponse.should.have.property('statusCode', 200) + refreshResponse.json().access_token.should.be.a.String().and.startWith('ffpat') + }) + + it('rejects an authorize redirect_uri that was not registered', async function () { + const reg = (await register(['http://localhost:9876/oauth/callback'])).json() + const { challenge } = pkce() + const response = await mcpApp.inject({ method: 'GET', url: authorizeURL(reg.client_id, 'http://localhost:9876/evil', challenge), cookies: { sid } }) + response.should.have.property('statusCode', 400) + response.json().should.have.property('error', 'invalid_request') + }) + + it('accepts a loopback redirect on a different port than registered (RFC 8252)', async function () { + const reg = (await register(['http://localhost:1111/cb'])).json() + const { challenge } = pkce() + const response = await mcpApp.inject({ method: 'GET', url: authorizeURL(reg.client_id, 'http://127.0.0.1:2222/cb', challenge), cookies: { sid } }) + response.should.have.property('statusCode', 302) + response.headers.location.should.match(/\/account\/request\/[^/]+\/mcp$/) + }) + }) }) From d07d5056a519a44261f3539637182e45da82142e Mon Sep 17 00:00:00 2001 From: andypalmi Date: Tue, 25 Aug 2026 10:56:06 +0200 Subject: [PATCH 2/6] fix(mcp): keep the refresh token alive after access-token expiry An AccessToken row holds both the access token and its refresh token, and getOrExpire destroyed the row when the access token expired, taking the refresh token with it so a client could never refresh (RFC 6749 1.5). Add a separate refreshTokenExpiresAt lifetime: reject an expired access token but keep the row while its refresh token is still valid. The MCP refresh token is stable rather than rotating, and concurrent refreshes coalesce through a shared cache so they reuse the most recently minted access token instead of overwriting the row. --- forge/db/controllers/AccessToken.js | 63 ++++++++++++-- ...d-refreshTokenExpiresAt-to-AccessTokens.js | 29 +++++++ forge/db/models/AccessToken.js | 1 + .../forge/db/controllers/AccessToken_spec.js | 87 +++++++++++++++++++ 4 files changed, 175 insertions(+), 5 deletions(-) create mode 100644 forge/db/migrations/20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js diff --git a/forge/db/controllers/AccessToken.js b/forge/db/controllers/AccessToken.js index 064e83d1df..47caaaa105 100644 --- a/forge/db/controllers/AccessToken.js +++ b/forge/db/controllers/AccessToken.js @@ -4,6 +4,15 @@ const { generateToken, generateNumericToken, sha256, randomPhrase } = require('. const DEFAULT_TOKEN_SESSION_EXPIRY = 1000 * 60 * 30 // 30 mins session - with refresh token support +const DEFAULT_REFRESH_TOKEN_EXPIRY = 1000 * 60 * 60 * 24 * 30 // 30 days - sliding refresh token lifetime + +// Concurrent refreshes of the same stable refresh token reuse the most recently +// minted access token from this shared cache (Valkey in production) rather than +// each minting a new one and overwriting the row. Re-mint once the cached token +// is within this window of expiry so a client is never handed a token about to die. +const MCP_ACCESS_TOKEN_CACHE = 'mcp-oauth-access-token' +const MCP_ACCESS_TOKEN_REMAINING_LIMIT = 1000 * 60 // 60 seconds + const DEFAULT_DEVICE_OTC_EXPIRY = 1000 * 60 * 60 * 24 // 24 hours /* @@ -269,6 +278,7 @@ module.exports = { const token = generateToken(32, 'ffpat') const refreshToken = generateToken(32, 'ffpat') const expiresAt = Date.now() + DEFAULT_TOKEN_SESSION_EXPIRY + const refreshTokenExpiresAt = Date.now() + DEFAULT_REFRESH_TOKEN_EXPIRY await app.db.sequelize.transaction(async (t) => { const tok = await app.db.models.AccessToken.create({ @@ -277,6 +287,7 @@ module.exports = { refreshToken, scope: '', expiresAt, + refreshTokenExpiresAt, readOnly, adminOptIn: false, ownerId: '' + userId, @@ -433,8 +444,14 @@ module.exports = { refreshToken: async function (app, refreshToken) { const existingToken = await app.db.models.AccessToken.byRefreshToken(refreshToken) - if (existingToken) { - const [prefix] = refreshToken.split('_') + if (!existingToken) { + return null + } + const [prefix] = refreshToken.split('_') + + // Tokens without their own refresh lifetime (e.g. editor sessions) rotate + // the refresh token on each use. + if (!existingToken.refreshTokenExpiresAt) { const tokenUpdates = { token: generateToken(32, prefix), refreshToken: generateToken(32, prefix), @@ -443,7 +460,35 @@ module.exports = { await app.db.models.AccessToken.update(tokenUpdates, { where: { refreshToken: existingToken.refreshToken } }) return tokenUpdates } - return null + + // A refresh token with its own lifetime is no longer valid once that + // lifetime has passed - remove it rather than issuing a new access token. + if (existingToken.refreshTokenExpiresAt.getTime() < Date.now()) { + await existingToken.destroy() + return null + } + + // These tokens keep a stable refresh token, so concurrent refreshes can + // reuse the access token most recently minted for it: the first mint + // populates the shared cache and the rest return the same token instead of + // overwriting the row. If a truly simultaneous mint slips past the cache, + // the stale side simply refreshes again with its unchanged refresh token. + const cache = app.caches?.getCache?.(MCP_ACCESS_TOKEN_CACHE, { ttl: DEFAULT_TOKEN_SESSION_EXPIRY, max: 10000 }) + const cacheKey = sha256(refreshToken) + const cached = await cache?.get(cacheKey) + if (cached && cached.expiresAt - Date.now() > MCP_ACCESS_TOKEN_REMAINING_LIMIT) { + return { token: cached.token, expiresAt: cached.expiresAt, refreshToken } + } + + const token = generateToken(32, prefix) + const expiresAt = Date.now() + DEFAULT_TOKEN_SESSION_EXPIRY + await app.db.models.AccessToken.update( + { token, expiresAt, refreshTokenExpiresAt: Date.now() + DEFAULT_REFRESH_TOKEN_EXPIRY }, + { where: { refreshToken: existingToken.refreshToken } } + ) + await cache?.set(cacheKey, { token, expiresAt }) + // The refresh token is unchanged, so hand back the one the client presented. + return { token, expiresAt, refreshToken } }, /** @@ -465,8 +510,16 @@ module.exports = { }) if (accessToken) { if (accessToken.expiresAt && accessToken.expiresAt.getTime() < Date.now()) { - await accessToken.destroy() - accessToken = null + const refreshTokenValid = accessToken.refreshTokenExpiresAt && accessToken.refreshTokenExpiresAt.getTime() > Date.now() + if (refreshTokenValid) { + // The access token has expired but the refresh token is still + // valid. Reject the access token without destroying the row so + // the client can obtain a new one via refresh (RFC 6749 §1.5). + accessToken = null + } else { + await accessToken.destroy() + accessToken = null + } } } return accessToken diff --git a/forge/db/migrations/20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js b/forge/db/migrations/20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js new file mode 100644 index 0000000000..1962bae2a2 --- /dev/null +++ b/forge/db/migrations/20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js @@ -0,0 +1,29 @@ +/** + * Give refresh tokens a lifetime independent of the access token. + * + * An AccessToken row holds both the access token and its refresh token. The + * access token is short-lived (expiresAt); the refresh token is meant to + * outlive it so a client can obtain a new access token after expiry + * (RFC 6749 §1.5). Without a separate expiry the refresh token's lifetime was + * tied to the access token's, so expiring the access token also discarded the + * refresh token and made refresh impossible. + * + * refreshTokenExpiresAt - when the refresh token itself expires. Null for + * tokens that do not use a refresh lifetime, which + * keep the previous behaviour. + */ + +const { DataTypes } = require('sequelize') + +module.exports = { + up: async (context) => { + await context.addColumn('AccessTokens', 'refreshTokenExpiresAt', { + type: DataTypes.DATE, + allowNull: true, + defaultValue: null + }) + }, + down: async (context) => { + await context.removeColumn('AccessTokens', 'refreshTokenExpiresAt') + } +} diff --git a/forge/db/models/AccessToken.js b/forge/db/models/AccessToken.js index d92d15d686..f1a6c0e254 100644 --- a/forge/db/models/AccessToken.js +++ b/forge/db/models/AccessToken.js @@ -45,6 +45,7 @@ module.exports = { } } }, + refreshTokenExpiresAt: { type: DataTypes.DATE }, name: { type: DataTypes.STRING }, readOnly: { type: DataTypes.BOOLEAN, defaultValue: false, allowNull: false }, adminOptIn: { type: DataTypes.BOOLEAN, defaultValue: false, allowNull: false } diff --git a/test/unit/forge/db/controllers/AccessToken_spec.js b/test/unit/forge/db/controllers/AccessToken_spec.js index 1a0b273e80..d07c96d080 100644 --- a/test/unit/forge/db/controllers/AccessToken_spec.js +++ b/test/unit/forge/db/controllers/AccessToken_spec.js @@ -335,6 +335,93 @@ describe('AccessToken controller', function () { }) }) + describe('MCP OAuth Tokens', function () { + function createToken (opts = {}) { + return app.db.controllers.AccessToken.createMCPOAuthToken(TestObjects.alice.id, opts) + } + + // Move the row's access and/or refresh token expiry, so the refresh + // behaviour can be exercised without waiting for real time to pass. + async function setRowExpiry (refreshToken, { accessMs, refreshMs } = {}) { + const row = await app.db.models.AccessToken.byRefreshToken(refreshToken) + const updates = {} + if (accessMs !== undefined) { + updates.expiresAt = new Date(Date.now() + accessMs) + } + if (refreshMs !== undefined) { + updates.refreshTokenExpiresAt = new Date(Date.now() + refreshMs) + } + await app.db.models.AccessToken.update(updates, { where: { id: row.id } }) + } + + it('creates a user token with a refresh token that outlives the access token', async function () { + const result = await createToken({ readOnly: true }) + result.token.should.be.a.String().and.startWith('ffpat') + result.refreshToken.should.be.a.String().and.startWith('ffpat') + should.exist(result.expiresAt) + + const row = await app.db.models.AccessToken.byRefreshToken(result.refreshToken) + row.should.have.property('readOnly', true) + should.exist(row.refreshTokenExpiresAt) + row.refreshTokenExpiresAt.getTime().should.be.greaterThan(row.expiresAt.getTime()) + }) + + it('rejects an expired access token but keeps the row so it can still be refreshed', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { accessMs: -5000 }) + + // The access token is rejected... + should.not.exist(await app.db.controllers.AccessToken.getOrExpire(original.token)) + // ...but the row survives (RFC 6749 §1.5) so refresh still works. + ;(await app.db.models.AccessToken.count()).should.equal(1) + + const refreshed = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) + should.exist(refreshed) + should.exist(await app.db.controllers.AccessToken.getOrExpire(refreshed.token)) + }) + + it('destroys the row once the refresh token has also expired', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { accessMs: -5000, refreshMs: -1000 }) + + should.not.exist(await app.db.controllers.AccessToken.getOrExpire(original.token)) + ;(await app.db.models.AccessToken.count()).should.equal(0) + }) + + it('keeps the refresh token stable and slides its expiry on refresh', async function () { + const original = await createToken() + const before = await app.db.models.AccessToken.byRefreshToken(original.refreshToken) + // Bring the refresh expiry close so the slide forward is observable. + await setRowExpiry(original.refreshToken, { accessMs: -5000, refreshMs: 60000 }) + + const refreshed = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) + refreshed.token.should.not.equal(original.token) + // The refresh token is not rotated. + refreshed.refreshToken.should.equal(original.refreshToken) + + const after = await app.db.models.AccessToken.byRefreshToken(original.refreshToken) + after.refreshTokenExpiresAt.getTime().should.be.greaterThan(before.refreshTokenExpiresAt.getTime()) + }) + + it('reuses the same access token for a repeat refresh within its lifetime', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { accessMs: -5000 }) + + const first = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) + const second = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) + second.token.should.equal(first.token) + should.exist(await app.db.controllers.AccessToken.getOrExpire(first.token)) + }) + + it('fails to refresh once the refresh token lifetime has passed', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { refreshMs: -1000 }) + + should.not.exist(await app.db.controllers.AccessToken.refreshToken(original.refreshToken)) + ;(await app.db.models.AccessToken.count()).should.equal(0) + }) + }) + describe('getOrExpire', function () { it('does not return expired tokens', async function () { ;(await app.db.models.AccessToken.count()).should.equal(0) From 3ce33b31f2364eb11c41c933b041a2729d583a8c Mon Sep 17 00:00:00 2001 From: andypalmi Date: Tue, 25 Aug 2026 11:35:45 +0200 Subject: [PATCH 3/6] test(mcp): make the refresh-expiry slide assertion deterministic Capture the pre-refresh expiry after lowering it, so the assertion compares against the shortened lifetime rather than the original one and no longer ties when the refresh lands in the same millisecond. --- test/unit/forge/db/controllers/AccessToken_spec.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/unit/forge/db/controllers/AccessToken_spec.js b/test/unit/forge/db/controllers/AccessToken_spec.js index d07c96d080..8424a09a1a 100644 --- a/test/unit/forge/db/controllers/AccessToken_spec.js +++ b/test/unit/forge/db/controllers/AccessToken_spec.js @@ -390,9 +390,10 @@ describe('AccessToken controller', function () { it('keeps the refresh token stable and slides its expiry on refresh', async function () { const original = await createToken() - const before = await app.db.models.AccessToken.byRefreshToken(original.refreshToken) - // Bring the refresh expiry close so the slide forward is observable. + // Bring the refresh expiry close so the slide back out to the full + // lifetime is observable rather than a same-millisecond tie. await setRowExpiry(original.refreshToken, { accessMs: -5000, refreshMs: 60000 }) + const before = await app.db.models.AccessToken.byRefreshToken(original.refreshToken) const refreshed = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) refreshed.token.should.not.equal(original.token) From 39e94a7f521b98c117aff8d39f4889d4afc84d82 Mon Sep 17 00:00:00 2001 From: andypalmi Date: Tue, 25 Aug 2026 15:30:58 +0200 Subject: [PATCH 4/6] fix(mcp): include token_type in the refresh token response The refresh_token grant returned access_token, expires_in and refresh_token but omitted token_type, which RFC 6749 section 5.1 requires. A spec-compliant client rejects the response and falls back to re-authorization even though the server rotated the token successfully. Return token_type: bearer to match the authorization_code response. --- forge/routes/auth/oauth.js | 1 + test/unit/forge/routes/auth/oauth_spec.js | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/forge/routes/auth/oauth.js b/forge/routes/auth/oauth.js index bfb3a04cd6..81681fc0a7 100644 --- a/forge/routes/auth/oauth.js +++ b/forge/routes/auth/oauth.js @@ -655,6 +655,7 @@ module.exports = async function (app) { const response = { access_token: accessToken.token, + token_type: 'bearer', expires_in: Math.floor((accessToken.expiresAt - Date.now()) / 1000), refresh_token: accessToken.refreshToken } diff --git a/test/unit/forge/routes/auth/oauth_spec.js b/test/unit/forge/routes/auth/oauth_spec.js index 2f0fbc7b20..847ac2dd6d 100644 --- a/test/unit/forge/routes/auth/oauth_spec.js +++ b/test/unit/forge/routes/auth/oauth_spec.js @@ -457,7 +457,10 @@ describe('OAuth', async function () { payload: { grant_type: 'refresh_token', client_id: clientID, refresh_token: token.refresh_token } }) refreshResponse.should.have.property('statusCode', 200) - refreshResponse.json().access_token.should.be.a.String().and.startWith('ffpat') + const refreshed = refreshResponse.json() + refreshed.access_token.should.be.a.String().and.startWith('ffpat') + refreshed.should.have.property('token_type', 'bearer') + refreshed.should.have.property('refresh_token') }) it('rejects an authorize redirect_uri that was not registered', async function () { From 3174417d8966d9dd48ca952467f247b97ce816a7 Mon Sep 17 00:00:00 2001 From: andypalmi Date: Tue, 25 Aug 2026 15:43:30 +0200 Subject: [PATCH 5/6] chore(mcp): make migration down a no-op to match convention --- .../db/migrations/20260824-01-add-mcp-authclient-fields.js | 6 +----- ...20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js | 4 +--- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js b/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js index df7fa920b8..21ffaff4f2 100644 --- a/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js +++ b/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js @@ -31,9 +31,5 @@ module.exports = { defaultValue: null }) }, - down: async (context) => { - await context.removeColumn('AuthClients', 'type') - await context.removeColumn('AuthClients', 'name') - await context.removeColumn('AuthClients', 'redirectURIs') - } + down: async (context) => {} } diff --git a/forge/db/migrations/20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js b/forge/db/migrations/20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js index 1962bae2a2..536414ab40 100644 --- a/forge/db/migrations/20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js +++ b/forge/db/migrations/20260825-01-add-refreshTokenExpiresAt-to-AccessTokens.js @@ -23,7 +23,5 @@ module.exports = { defaultValue: null }) }, - down: async (context) => { - await context.removeColumn('AccessTokens', 'refreshTokenExpiresAt') - } + down: async (context) => {} } From 7209333d53eedb9b1a1c8ff29523267d93432b30 Mon Sep 17 00:00:00 2001 From: andypalmi Date: Tue, 25 Aug 2026 21:43:53 +0200 Subject: [PATCH 6/6] refactor(mcp): identify MCP clients via ownerType and widen refresh cache margin Reuse the AuthClients ownerType column with ownerType='mcp' for dynamically registered MCP clients instead of a dedicated type column. Raise the refresh re-mint margin to 5 minutes so the coalescing cache stays effective against the 30 minute access-token lifetime. --- forge/db/controllers/AccessToken.js | 28 +++++++------------ forge/db/controllers/AuthClient.js | 9 ++---- .../20260824-01-add-mcp-authclient-fields.js | 11 ++------ forge/db/models/AuthClient.js | 3 +- forge/routes/auth/oauth.js | 25 +++++++---------- test/unit/forge/routes/auth/oauth_spec.js | 2 +- 6 files changed, 28 insertions(+), 50 deletions(-) diff --git a/forge/db/controllers/AccessToken.js b/forge/db/controllers/AccessToken.js index 47caaaa105..151d753430 100644 --- a/forge/db/controllers/AccessToken.js +++ b/forge/db/controllers/AccessToken.js @@ -6,12 +6,11 @@ const DEFAULT_TOKEN_SESSION_EXPIRY = 1000 * 60 * 30 // 30 mins session - with re const DEFAULT_REFRESH_TOKEN_EXPIRY = 1000 * 60 * 60 * 24 * 30 // 30 days - sliding refresh token lifetime -// Concurrent refreshes of the same stable refresh token reuse the most recently -// minted access token from this shared cache (Valkey in production) rather than -// each minting a new one and overwriting the row. Re-mint once the cached token -// is within this window of expiry so a client is never handed a token about to die. +// Concurrent refreshes of the same refresh token reuse the cached access token +// rather than each minting a new one and overwriting the row. Re-mint once the +// cached token is within this window of expiry. const MCP_ACCESS_TOKEN_CACHE = 'mcp-oauth-access-token' -const MCP_ACCESS_TOKEN_REMAINING_LIMIT = 1000 * 60 // 60 seconds +const MCP_ACCESS_TOKEN_REMAINING_LIMIT = 1000 * 60 * 5 // 5 minutes const DEFAULT_DEVICE_OTC_EXPIRY = 1000 * 60 * 60 * 24 // 24 hours @@ -449,8 +448,7 @@ module.exports = { } const [prefix] = refreshToken.split('_') - // Tokens without their own refresh lifetime (e.g. editor sessions) rotate - // the refresh token on each use. + // Editor sessions have no refresh lifetime: rotate the refresh token each use. if (!existingToken.refreshTokenExpiresAt) { const tokenUpdates = { token: generateToken(32, prefix), @@ -461,18 +459,14 @@ module.exports = { return tokenUpdates } - // A refresh token with its own lifetime is no longer valid once that - // lifetime has passed - remove it rather than issuing a new access token. + // Past its lifetime the refresh token is dead: remove the row. if (existingToken.refreshTokenExpiresAt.getTime() < Date.now()) { await existingToken.destroy() return null } - // These tokens keep a stable refresh token, so concurrent refreshes can - // reuse the access token most recently minted for it: the first mint - // populates the shared cache and the rest return the same token instead of - // overwriting the row. If a truly simultaneous mint slips past the cache, - // the stale side simply refreshes again with its unchanged refresh token. + // Stable refresh token: concurrent refreshes reuse the cached access token + // instead of each minting one and overwriting the row. const cache = app.caches?.getCache?.(MCP_ACCESS_TOKEN_CACHE, { ttl: DEFAULT_TOKEN_SESSION_EXPIRY, max: 10000 }) const cacheKey = sha256(refreshToken) const cached = await cache?.get(cacheKey) @@ -487,7 +481,6 @@ module.exports = { { where: { refreshToken: existingToken.refreshToken } } ) await cache?.set(cacheKey, { token, expiresAt }) - // The refresh token is unchanged, so hand back the one the client presented. return { token, expiresAt, refreshToken } }, @@ -512,9 +505,8 @@ module.exports = { if (accessToken.expiresAt && accessToken.expiresAt.getTime() < Date.now()) { const refreshTokenValid = accessToken.refreshTokenExpiresAt && accessToken.refreshTokenExpiresAt.getTime() > Date.now() if (refreshTokenValid) { - // The access token has expired but the refresh token is still - // valid. Reject the access token without destroying the row so - // the client can obtain a new one via refresh (RFC 6749 §1.5). + // Refresh token still valid: reject the access token but keep the + // row so the client can refresh (RFC 6749 §1.5). accessToken = null } else { await accessToken.destroy() diff --git a/forge/db/controllers/AuthClient.js b/forge/db/controllers/AuthClient.js index adb0d06cca..7e407a34e5 100644 --- a/forge/db/controllers/AuthClient.js +++ b/forge/db/controllers/AuthClient.js @@ -42,15 +42,12 @@ module.exports = { return client }, - /** - * Register a public MCP client (RFC 7591 Dynamic Client Registration). - * MCP clients have no owner and no secret - they authenticate with PKCE. - * The generated clientID is the only credential returned to the client. - */ + // Register a public MCP client (RFC 7591). No owner, no secret: PKCE only, + // and the generated clientID is the sole credential. createMCPClient: async function (app, { name, redirectURIs } = {}) { return app.db.models.AuthClient.create({ clientID: generateToken(32, 'ffmcp'), - type: 'mcp', + ownerType: 'mcp', name: name || 'MCP Agent', redirectURIs: redirectURIs || [] }) diff --git a/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js b/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js index 21ffaff4f2..4f05007381 100644 --- a/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js +++ b/forge/db/migrations/20260824-01-add-mcp-authclient-fields.js @@ -3,10 +3,10 @@ * * Existing clients (project/device editor auth) are owned by a resource via * ownerType/ownerId and authenticate with a clientSecret. MCP clients have no - * owner and are public (PKCE, no secret), so they need somewhere to record the - * client type, a display name, and the redirect URIs approved at registration. + * owning resource and are public (PKCE, no secret): they reuse ownerType with + * ownerType='mcp' and add a display name and the redirect URIs approved at + * registration. * - * type - 'mcp' for dynamically registered MCP clients, null otherwise * name - the client_name supplied at registration * redirectURIs - JSON array of redirect URIs the client may use */ @@ -15,11 +15,6 @@ const { DataTypes } = require('sequelize') module.exports = { up: async (context) => { - await context.addColumn('AuthClients', 'type', { - type: DataTypes.STRING, - allowNull: true, - defaultValue: null - }) await context.addColumn('AuthClients', 'name', { type: DataTypes.STRING, allowNull: true, diff --git a/forge/db/models/AuthClient.js b/forge/db/models/AuthClient.js index 17def3bb02..16b2d954ee 100644 --- a/forge/db/models/AuthClient.js +++ b/forge/db/models/AuthClient.js @@ -18,9 +18,8 @@ module.exports = { } }, ownerId: { type: DataTypes.STRING }, + // 'project'/'device' for editor auth clients; 'mcp' for dynamically registered MCP clients (public, no secret) ownerType: { type: DataTypes.STRING }, - // 'mcp' for dynamically registered MCP clients (public, no secret), null otherwise - type: { type: DataTypes.STRING, allowNull: true }, name: { type: DataTypes.STRING, allowNull: true }, redirectURIs: { type: DataTypes.TEXT, diff --git a/forge/routes/auth/oauth.js b/forge/routes/auth/oauth.js index 81681fc0a7..8519c35e58 100644 --- a/forge/routes/auth/oauth.js +++ b/forge/routes/auth/oauth.js @@ -15,8 +15,7 @@ function badRequest (reply, error, description) { } // A loopback redirect_uri may vary its port between registration and use -// (RFC 8252 Section 7.3), and localhost/127.0.0.1 are interchangeable. Every -// other redirect must match a registered value exactly. +// (RFC 8252 Section 7.3); localhost and 127.0.0.1 are interchangeable. function isLoopbackHost (hostname) { return hostname === 'localhost' || hostname === '127.0.0.1' } @@ -133,17 +132,15 @@ module.exports = async function (app) { if (!authClient) { return badRequest(reply, 'invalid_request', 'Invalid client_id') } - if (authClient.type === 'mcp') { - // MCP agent (dynamically registered): redirect_uri must match one - // approved at registration. Scope is not validated here - the access - // level is chosen by the user on the consent page (readOnly + teamIds). + if (authClient.ownerType === 'mcp') { + // redirect_uri must match one approved at registration; scope is + // not validated here (chosen by the user on the consent page). if (!redirectUriMatches(authClient.redirectURIs, redirect_uri)) { return badRequest(reply, 'invalid_request', 'Invalid redirect_uri') } isMCP = true } else { - // Dynamic client (project/device editor auth) - // Ensure redirect_uri path component is correct + // Dynamic client (project/device editor auth): validate callback path if ( // HTTP Auth callback !/\/_ffAuth\/callback$/.test(redirectURI.pathname) && @@ -203,7 +200,6 @@ module.exports = async function (app) { return } if (isMCP) { - // Redirect to MCP-specific consent page reply.redirect(`${app.config.base_url}/account/request/${requestId}/mcp`) return } @@ -338,9 +334,8 @@ module.exports = async function (app) { if (!Array.isArray(redirect_uris) || redirect_uris.length === 0) { return badRequest(reply, 'invalid_redirect_uri', 'At least one redirect_uri is required') } - // A redirect_uri must either be a loopback http address (local dev tools, - // RFC 8252 Section 7.3) or a secure https address (a hosted client, e.g. a - // provider connector). Anything else is rejected. + // A redirect_uri must be a loopback http address (local dev tools, RFC 8252 + // Section 7.3) or an https address; anything else is rejected. for (const uri of redirect_uris) { let parsed try { @@ -608,8 +603,8 @@ module.exports = async function (app) { badRequest(reply, 'invalid_request', 'Invalid refresh_token') return } - // ff-plugin and MCP clients are user-scoped; only project/device - // clients need their resource ownership re-checked on refresh. + // Only project/device clients re-check resource ownership on refresh; + // ff-plugin and MCP clients are user-scoped. let refreshAuthClient = null if (client_id !== 'ff-plugin') { refreshAuthClient = await app.db.controllers.AuthClient.getAuthClient(client_id, client_secret) @@ -617,7 +612,7 @@ module.exports = async function (app) { return badRequest(reply, 'invalid_request', 'Invalid client_id') } } - if (refreshAuthClient && refreshAuthClient.type !== 'mcp') { + if (refreshAuthClient && refreshAuthClient.ownerType !== 'mcp') { // Check the owner of the existing session still has access to the project // this client is owned by let owner = null diff --git a/test/unit/forge/routes/auth/oauth_spec.js b/test/unit/forge/routes/auth/oauth_spec.js index 847ac2dd6d..52eff92c3e 100644 --- a/test/unit/forge/routes/auth/oauth_spec.js +++ b/test/unit/forge/routes/auth/oauth_spec.js @@ -383,7 +383,7 @@ describe('OAuth', async function () { body.redirect_uris.should.eql([redirectURI]) const client = await mcpApp.db.controllers.AuthClient.getAuthClient(body.client_id) - client.should.have.property('type', 'mcp') + client.should.have.property('ownerType', 'mcp') client.redirectURIs.should.eql([redirectURI]) })