From 6f8e82868ce867f964d6c8b3e8d2e2c5ee491b18 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Wed, 21 Oct 2026 17:06:43 +0100 Subject: [PATCH 1/4] Rework oauth consent flows --- forge/routes/auth/oauth.js | 207 +++++++++++------- frontend/src/pages/account/AccessRequest.vue | 11 +- .../src/pages/account/AccessRequestMCP.vue | 12 +- test/system/201-nr-tools-plugin_spec.js | 8 +- test/unit/forge/routes/auth/oauth_spec.js | 149 ++++++++++++- 5 files changed, 293 insertions(+), 94 deletions(-) diff --git a/forge/routes/auth/oauth.js b/forge/routes/auth/oauth.js index f492f95554..010e8f4e33 100644 --- a/forge/routes/auth/oauth.js +++ b/forge/routes/auth/oauth.js @@ -49,14 +49,26 @@ function redirectUriMatches (registeredURIs, requested) { }) } -function redirectInvalidRequest (reply, redirectURI, error, errorDescription, state) { +function errorRedirectURL (redirectURI, error, errorDescription, state) { const responseUrl = new URL(redirectURI) const response = { error, errorDescription } if (state) { response.state = state } responseUrl.search = querystring.stringify(response) - reply.redirect(responseUrl.toString()) + return responseUrl.toString() +} + +function redirectInvalidRequest (reply, redirectURI, error, errorDescription, state) { + reply.redirect(errorRedirectURL(redirectURI, error, errorDescription, state)) +} + +// The FlowFuse Node-RED plugin and MCP agents receive user-scoped tokens, so +// the user must explicitly approve the request on the consent page. These +// requests can only be completed via the PUT routes, which cannot be triggered +// cross-site; the GET routes are reserved for the editor auth flow. +function requiresApproval (requestObject) { + return requestObject.client_id === 'ff-plugin' || requestObject.mcp } module.exports = async function (app) { @@ -69,6 +81,29 @@ module.exports = async function (app) { } } + // Approving an OAuth request must come from the user's own logged-in browser + // session. Bearer tokens are refused so an existing token cannot be used to + // mint a new, broader one. + async function verifyBrowserSession (request, reply) { + request.session = request.sid ? await app.db.controllers.Session.getValidSession(request.sid) : null + if (!request.session) { + return reply.code(401).send({ code: 'unauthorized', error: 'unauthorized' }) + } + } + + // Issue an authorization code for the user and return the client callback url + async function issueCode (requestObject, user) { + requestObject.userId = user.id + requestObject.code = base64URLEncode(crypto.randomBytes(32)) + await requestCache.set(requestObject.code, requestObject) + const responseUrl = new URL(requestObject.redirect_uri) + responseUrl.search = querystring.stringify({ + code: requestObject.code, + state: requestObject.state + }) + return responseUrl.toString() + } + app.addContentTypeParser('application/x-www-form-urlencoded', { parseAs: 'string' }, function (req, body, done) { try { const json = querystring.parse(body) @@ -222,82 +257,72 @@ module.exports = async function (app) { if (!requestObject) { return badRequest(reply, 'invalid_request', 'Invalid request') } + if (requiresApproval(requestObject)) { + return badRequest(reply, 'invalid_request', 'Invalid request') + } if (request.sid) { request.session = await app.db.controllers.Session.getValidSession(request.sid) if (request.session) { - 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) { - return badRequest(reply, 'invalid_request', 'Invalid client_id') - } - let owner = null - let applicationId - if (authClient.ownerType === 'project') { - owner = await app.db.models.Project.byId(authClient.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)) - // Device.byId does not include the full Application object - if (owner?.ApplicationId) { - applicationId = app.db.models.Application.encodeHashid(owner.ApplicationId) - } + const authClient = await app.db.controllers.AuthClient.getAuthClient(requestObject.client_id) + if (!authClient) { + return badRequest(reply, 'invalid_request', 'Invalid client_id') + } + let owner = null + let applicationId + if (authClient.ownerType === 'project') { + owner = await app.db.models.Project.byId(authClient.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)) + // Device.byId does not include the full Application object + if (owner?.ApplicationId) { + applicationId = app.db.models.Application.encodeHashid(owner.ApplicationId) } - const teamMembership = await request.session.User.getTeamMembership(owner.TeamId) - if (!teamMembership && !request.session.User.admin) { - // This user is neither a team member, nor an admin - reject - return redirectInvalidRequest(reply, requestObject.redirect_uri, 'access_denied', 'Access Denied', requestObject.state) + } + const teamMembership = await request.session.User.getTeamMembership(owner.TeamId) + if (!teamMembership && !request.session.User.admin) { + // This user is neither a team member, nor an admin - reject + return redirectInvalidRequest(reply, requestObject.redirect_uri, 'access_denied', 'Access Denied', requestObject.state) + } + const isEditor = /^editor($|-)/.test(requestObject.scope) + if (isEditor) { + // Allow admin users to have read-access to flows + const protectedInstance = await owner.getSetting(KEY_PROTECTED) + let context + if (applicationId) { + context = { applicationId } } - const isEditor = /^editor($|-)/.test(requestObject.scope) - if (isEditor) { - // Allow admin users to have read-access to flows - const protectedInstance = await owner.getSetting(KEY_PROTECTED) - let context - if (applicationId) { - context = { applicationId } - } - const canReadFlows = request.session.User.admin || app.hasPermission(teamMembership, 'project:flows:view', context) - const canWriteFlows = app.hasPermission(teamMembership, 'project:flows:edit', context) && !protectedInstance?.enabled - const canReadHTTP = app.hasPermission(teamMembership, 'project:flows:http', context) - if (!canReadFlows && !canWriteFlows) { - if (!canReadHTTP) { - return redirectInvalidRequest(reply, requestObject.redirect_uri, 'access_denied', 'Access Denied', requestObject.state) - } else { - // We have to avoid Node-RED autoLogin redirect loops - so bail out with this - // rather ugly error message. - reply.code(400).send('Access Denied: you do not have access to the editor') - return - } - } - if (!canWriteFlows && requestObject.scope === 'editor') { - // Older versions of nr-auth do not know how to apply read-only - // access. We know it is an older version because it set scope to `editor`. - // Versions that support viewer will have a scope of `editor-`. - // This should be sent as plain text as the user will see it in the browser window. - reply.code(400).send('Please ask the team owner to update this project to the latest stack to support viewer access') - return - } - } else { - // This is the httpNode middleware checking access. All - // team members are allowed to access the httpNode routes - if (!teamMembership) { - // This is an admin who isn't a team member - reject the request + const canReadFlows = request.session.User.admin || app.hasPermission(teamMembership, 'project:flows:view', context) + const canWriteFlows = app.hasPermission(teamMembership, 'project:flows:edit', context) && !protectedInstance?.enabled + const canReadHTTP = app.hasPermission(teamMembership, 'project:flows:http', context) + if (!canReadFlows && !canWriteFlows) { + if (!canReadHTTP) { return redirectInvalidRequest(reply, requestObject.redirect_uri, 'access_denied', 'Access Denied', requestObject.state) + } else { + // We have to avoid Node-RED autoLogin redirect loops - so bail out with this + // rather ugly error message. + reply.code(400).send('Access Denied: you do not have access to the editor') + return } } + if (!canWriteFlows && requestObject.scope === 'editor') { + // Older versions of nr-auth do not know how to apply read-only + // access. We know it is an older version because it set scope to `editor`. + // Versions that support viewer will have a scope of `editor-`. + // This should be sent as plain text as the user will see it in the browser window. + reply.code(400).send('Please ask the team owner to update this project to the latest stack to support viewer access') + return + } + } else { + // This is the httpNode middleware checking access. All + // team members are allowed to access the httpNode routes + if (!teamMembership) { + // This is an admin who isn't a team member - reject the request + return redirectInvalidRequest(reply, requestObject.redirect_uri, 'access_denied', 'Access Denied', requestObject.state) + } } - requestObject.userId = request.session.User.id - requestObject.code = base64URLEncode(crypto.randomBytes(32)) - await requestCache.set(requestObject.code, requestObject) - const responseUrl = new URL(requestObject.redirect_uri) - - responseUrl.search = querystring.stringify({ - code: requestObject.code, - state: requestObject.state - }) - reply.redirect(responseUrl.toString()) + reply.redirect(await issueCode(requestObject, request.session.User)) return } } @@ -310,12 +335,42 @@ module.exports = async function (app) { }, async function (request, reply) { const requestId = request.params.code const requestObject = await requestCache.get(requestId) - if (!requestObject) { + if (!requestObject || requiresApproval(requestObject)) { return badRequest(reply, 'invalid_request', 'Invalid request') } return redirectInvalidRequest(reply, requestObject.redirect_uri, 'access_denied', 'Access Denied', requestObject.state) }) + // Consent page approval for the FlowFuse Node-RED plugin and MCP agents. + // Returns the client callback url for the page to redirect to. + app.put('/account/complete/:code', { + preHandler: verifyBrowserSession, + schema: { + tags: ['Authentication', 'X-HIDDEN'] + } + }, async function (request, reply) { + const requestObject = await requestCache.get(request.params.code) + if (!requestObject || !requiresApproval(requestObject)) { + return badRequest(reply, 'invalid_request', 'Invalid request') + } + if (requestObject.mcp && requestObject.approvedBy !== request.session.User.id) { + // The access choices must have been saved by this user via the consent route + return badRequest(reply, 'access_denied', 'Access Denied') + } + reply.send({ redirect: await issueCode(requestObject, request.session.User) }) + }) + app.put('/account/reject/:code', { + schema: { + tags: ['Authentication', 'X-HIDDEN'] + } + }, async function (request, reply) { + const requestObject = await requestCache.get(request.params.code) + if (!requestObject) { + return badRequest(reply, 'invalid_request', 'Invalid request') + } + reply.send({ redirect: errorRedirectURL(requestObject.redirect_uri, 'access_denied', 'Access Denied', requestObject.state) }) + }) + // RFC 7591: Dynamic Client Registration for MCP agents app.post('/account/client', { config: { allowAnonymous: true }, @@ -374,7 +429,7 @@ module.exports = async function (app) { // MCP consent: save the user's access choices before they approve app.put('/account/authorize/:id/consent', { - preHandler: (request, reply) => app.verifySession(request, reply), + preHandler: verifyBrowserSession, schema: { tags: ['Authentication', 'X-HIDDEN'], params: { @@ -447,7 +502,7 @@ module.exports = async function (app) { } } - session.value = { ...requestObject, teamIds, expiresAt, toolPermissions: { default: normalisedDefault, teams: normalisedTeams } } + session.value = { ...requestObject, approvedBy: request.session.User.id, teamIds, expiresAt, toolPermissions: { default: normalisedDefault, teams: normalisedTeams } } await session.save() reply.send({ status: 'ok' }) @@ -524,7 +579,7 @@ module.exports = async function (app) { return } if (requestObject.code_challenge !== URLEncode(sha256(code_verifier))) { - redirectInvalidRequest(reply, 'invalid_request', 'Invalid code_verifier', requestObject.state) + badRequest(reply, 'invalid_request', 'Invalid code_verifier') return } @@ -562,11 +617,15 @@ module.exports = async function (app) { } reply.send(response) } else if (requestObject.mcp) { + // Only issue a token for a request the user approved on the consent page + if (requestObject.approvedBy !== requestObject.userId || !requestObject.expiresAt) { + return badRequest(reply, 'access_denied', 'Access Denied') + } const accessToken = await app.db.controllers.AccessToken.createMCPOAuthToken( requestObject.userId, { teamIds: requestObject.teamIds || [], - grantExpiresAt: requestObject.expiresAt || null, + grantExpiresAt: requestObject.expiresAt, toolPermissions: requestObject.toolPermissions || mcpToolPermissions.fromReadOnly(requestObject.readOnly || false) } ) diff --git a/frontend/src/pages/account/AccessRequest.vue b/frontend/src/pages/account/AccessRequest.vue index d7f725cec0..6f2997e1ab 100644 --- a/frontend/src/pages/account/AccessRequest.vue +++ b/frontend/src/pages/account/AccessRequest.vue @@ -26,6 +26,7 @@ import { ArrowSmallLeftIcon, ArrowSmallRightIcon, KeyIcon, RectangleGroupIcon } from '@heroicons/vue/20/solid' import { mapState } from 'pinia' +import client from '@/api/client.ts' import { useAccountAuthStore } from '@/stores/account-auth.js' export default { @@ -40,11 +41,13 @@ export default { ...mapState(useAccountAuthStore, ['user']) }, methods: { - allowAccess () { - window.location.href = `/account/complete/${this.$router.currentRoute.value.params.id}` + async allowAccess () { + const response = await client.put(`/account/complete/${this.$router.currentRoute.value.params.id}`, {}) + window.location.href = response.data.redirect }, - denyAccess () { - window.location.href = `/account/reject/${this.$router.currentRoute.value.params.id}` + async denyAccess () { + const response = await client.put(`/account/reject/${this.$router.currentRoute.value.params.id}`, {}) + window.location.href = response.data.redirect } } } diff --git a/frontend/src/pages/account/AccessRequestMCP.vue b/frontend/src/pages/account/AccessRequestMCP.vue index 56b84ed064..6c0270bf25 100644 --- a/frontend/src/pages/account/AccessRequestMCP.vue +++ b/frontend/src/pages/account/AccessRequestMCP.vue @@ -271,14 +271,20 @@ export default { expiresAt: Date.parse(this.expiresAt), toolPermissions: { default: this.defaultPermissions, teams: this.teamOverrides } }) - window.location.href = `/account/complete/${this.requestId}` + const response = await client.put(`/account/complete/${this.requestId}`, {}) + window.location.href = response.data.redirect } catch (err) { this.error = err.response?.data?.description || 'Failed to process request. Please try again.' this.submitting = false } }, - denyAccess () { - window.location.href = `/account/reject/${this.requestId}` + async denyAccess () { + try { + const response = await client.put(`/account/reject/${this.requestId}`, {}) + window.location.href = response.data.redirect + } catch (err) { + this.error = err.response?.data?.description || 'Failed to process request. Please try again.' + } } } } diff --git a/test/system/201-nr-tools-plugin_spec.js b/test/system/201-nr-tools-plugin_spec.js index 3f898fce2d..ceb48a6196 100644 --- a/test/system/201-nr-tools-plugin_spec.js +++ b/test/system/201-nr-tools-plugin_spec.js @@ -148,16 +148,16 @@ describe('Node-RED Tools Plugin', function () { const token = parts[parts.length - 1] const response2 = await app.inject({ - method: 'GET', + method: 'PUT', url: `/account/complete/${token}`, + payload: {}, cookies: { sid: TestObjects.accessToken } }) - response2.statusCode.should.equal(302) - response2.headers.should.have.property('location') + response2.statusCode.should.equal(200) // http://example.com/flowfuse-nr-tools/auth/callback?code=eANs21GUv7OqN99S2QxJ4tS1BD5RYEzOEfb-lhLUciw&state=YMbFnV1E_2aPwz_Ktca1DQ - const callbackURL = new URL(response2.headers.location) + const callbackURL = new URL(response2.json().redirect) callbackURL.pathname.should.equal('/flowfuse-nr-tools/auth/callback') callbackURL.host.should.equal('example.com') should.exist(callbackURL.searchParams.get('code')) diff --git a/test/unit/forge/routes/auth/oauth_spec.js b/test/unit/forge/routes/auth/oauth_spec.js index 90583fcbce..cdf63a9933 100644 --- a/test/unit/forge/routes/auth/oauth_spec.js +++ b/test/unit/forge/routes/auth/oauth_spec.js @@ -477,9 +477,9 @@ describe('OAuth', async function () { 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) + const completeResponse = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${requestId}`, payload: {}, cookies: { sid } }) + completeResponse.should.have.property('statusCode', 200) + const callback = new URL(completeResponse.json().redirect) callback.host.should.equal('localhost:9876') const authCode = callback.searchParams.get('code') should.exist(authCode) @@ -533,8 +533,8 @@ describe('OAuth', async function () { const requestId = /\/account\/request\/([^/]+)\/mcp$/.exec(authResponse.headers.location)[1] const expiresAt = Date.now() + 30 * 24 * 60 * 60 * 1000 await mcpApp.inject({ method: 'PUT', url: `/account/authorize/${requestId}/consent`, payload: { teamIds: [], expiresAt, toolPermissions: toolPermissionsPayload(false) }, cookies: { sid } }) - const completeResponse = await mcpApp.inject({ method: 'GET', url: `/account/complete/${requestId}`, cookies: { sid } }) - const authCode = new URL(completeResponse.headers.location).searchParams.get('code') + const completeResponse = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${requestId}`, payload: {}, cookies: { sid } }) + const authCode = new URL(completeResponse.json().redirect).searchParams.get('code') const first = (await mcpApp.inject({ method: 'POST', url: '/account/token', @@ -709,8 +709,8 @@ describe('OAuth', async function () { if (consentResponse.statusCode !== 200) { return { consentResponse } } - const completeResponse = await mcpApp.inject({ method: 'GET', url: `/account/complete/${requestId}`, cookies: { sid } }) - const authCode = new URL(completeResponse.headers.location).searchParams.get('code') + const completeResponse = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${requestId}`, payload: {}, cookies: { sid } }) + const authCode = new URL(completeResponse.json().redirect).searchParams.get('code') const tokenResponse = await mcpApp.inject({ method: 'POST', url: '/account/token', @@ -775,8 +775,8 @@ describe('OAuth', async function () { const { toolPermissions, ...legacyValue } = oauthSession.value oauthSession.value = { ...legacyValue, readOnly: true } await oauthSession.save() - const completeResponse = await mcpApp.inject({ method: 'GET', url: `/account/complete/${requestId}`, cookies: { sid } }) - const authCode = new URL(completeResponse.headers.location).searchParams.get('code') + const completeResponse = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${requestId}`, payload: {}, cookies: { sid } }) + const authCode = new URL(completeResponse.json().redirect).searchParams.get('code') const tokenResponse = await mcpApp.inject({ method: 'POST', url: '/account/token', @@ -840,5 +840,136 @@ describe('OAuth', async function () { response.should.have.property('statusCode', 302) response.headers.location.should.match(/\/account\/request\/[^/]+\/mcp$/) }) + + describe('approval', function () { + const pluginRedirectURI = 'https://example.com/flowfuse-nr-tools/auth/callback' + let otherSid + + before(async function () { + await mcpApp.factory.createUser({ + username: 'mcpother', + name: 'MCP Other', + email: 'mcp-other@example.com', + password: 'mmPassword' + }) + const loginResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/login', + payload: { username: 'mcpother', password: 'mmPassword', remember: false } + }) + otherSid = loginResponse.cookies[0].value + }) + + async function startMCPRequest (cookies = {}) { + const reg = (await register()).json() + const { verifier, challenge } = pkce() + const authResponse = await mcpApp.inject({ method: 'GET', url: authorizeURL(reg.client_id, redirectURI, challenge), cookies }) + const requestId = /\/account\/request\/([^/]+)\/mcp$/.exec(authResponse.headers.location)[1] + return { clientID: reg.client_id, verifier, requestId } + } + + async function startPluginRequest () { + const { challenge } = pkce() + const params = new URLSearchParams({ + client_id: 'ff-plugin', + scope: 'ff-plugin', + response_type: 'code', + redirect_uri: pluginRedirectURI, + state: 'abc', + code_challenge: challenge, + code_challenge_method: 'S256' + }) + const authResponse = await mcpApp.inject({ method: 'GET', url: `/account/authorize?${params}` }) + return /\/account\/request\/([^/]+)$/.exec(authResponse.headers.location)[1] + } + + function consent (requestId, cookies) { + return mcpApp.inject({ + method: 'PUT', + url: `/account/authorize/${requestId}/consent`, + payload: { teamIds: [], expiresAt: Date.now() + 1000 * 60 * 60, toolPermissions: toolPermissionsPayload(false) }, + cookies + }) + } + + it('refuses GET complete for an MCP request, even with consent', async function () { + const { requestId } = await startMCPRequest({ sid }) + ;(await consent(requestId, { sid })).should.have.property('statusCode', 200) + const response = await mcpApp.inject({ method: 'GET', url: `/account/complete/${requestId}`, cookies: { sid } }) + response.should.have.property('statusCode', 400) + should.not.exist(response.headers.location) + }) + + it('refuses GET complete and GET reject for an ff-plugin request', async function () { + const completeResponse = await mcpApp.inject({ method: 'GET', url: `/account/complete/${await startPluginRequest()}`, cookies: { sid } }) + completeResponse.should.have.property('statusCode', 400) + should.not.exist(completeResponse.headers.location) + const rejectResponse = await mcpApp.inject({ method: 'GET', url: `/account/reject/${await startPluginRequest()}`, cookies: { sid } }) + rejectResponse.should.have.property('statusCode', 400) + should.not.exist(rejectResponse.headers.location) + }) + + it('completes an ff-plugin request via PUT', async function () { + const response = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${await startPluginRequest()}`, payload: {}, cookies: { sid } }) + response.should.have.property('statusCode', 200) + const callback = new URL(response.json().redirect) + callback.origin.should.equal('https://example.com') + should.exist(callback.searchParams.get('code')) + callback.searchParams.get('state').should.equal('abc') + }) + + it('rejects a request via PUT', async function () { + const response = await mcpApp.inject({ method: 'PUT', url: `/account/reject/${await startPluginRequest()}`, payload: {} }) + response.should.have.property('statusCode', 200) + const callback = new URL(response.json().redirect) + callback.searchParams.get('error').should.equal('access_denied') + callback.searchParams.get('state').should.equal('abc') + }) + + it('refuses PUT complete for an MCP request without consent', async function () { + const { requestId } = await startMCPRequest() + const response = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${requestId}`, payload: {}, cookies: { sid } }) + response.should.have.property('statusCode', 400) + response.json().should.have.property('error', 'access_denied') + }) + + it('refuses PUT complete when consent was given by a different user', async function () { + // The attacker saves consent with their own session, then has the victim complete it + const { requestId } = await startMCPRequest() + ;(await consent(requestId, { sid: otherSid })).should.have.property('statusCode', 200) + const response = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${requestId}`, payload: {}, cookies: { sid } }) + response.should.have.property('statusCode', 400) + response.json().should.have.property('error', 'access_denied') + }) + + it('refuses consent and PUT complete authenticated with a bearer token', async function () { + const pat = await mcpApp.db.controllers.AccessToken.createPersonalAccessToken(mcpUser, '', null, 'approval-test') + const headers = { authorization: `Bearer ${pat.token}` } + const { requestId } = await startMCPRequest() + const consentResponse = await mcpApp.inject({ + method: 'PUT', + url: `/account/authorize/${requestId}/consent`, + payload: { teamIds: [], expiresAt: Date.now() + 1000 * 60 * 60, toolPermissions: toolPermissionsPayload(false) }, + headers + }) + consentResponse.should.have.property('statusCode', 401) + const completeResponse = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${await startPluginRequest()}`, payload: {}, headers }) + completeResponse.should.have.property('statusCode', 401) + }) + + it('refuses to exchange an MCP code that was not approved', async function () { + const { clientID, verifier, requestId } = await startMCPRequest() + const requestObject = (await mcpApp.db.models.OAuthSession.findOne({ where: { id: requestId } })).value + const code = base64URLEncode(crypto.randomBytes(32)) + await mcpApp.db.models.OAuthSession.create({ id: code, value: { ...requestObject, userId: mcpUser.id, code } }) + const tokenResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/token', + payload: { grant_type: 'authorization_code', code, redirect_uri: redirectURI, client_id: clientID, code_verifier: verifier } + }) + tokenResponse.should.have.property('statusCode', 400) + tokenResponse.json().should.have.property('error', 'access_denied') + }) + }) }) }) From e796196a4b7bd052a33add7f677999f2565fb1b7 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Wed, 21 Oct 2026 18:00:19 +0100 Subject: [PATCH 2/4] Ensure redirect urls are valid http/s urls --- forge/routes/auth/oauth.js | 5 ++++ frontend/src/pages/account/AccessRequest.vue | 5 ++-- .../src/pages/account/AccessRequestMCP.vue | 5 ++-- frontend/src/utils/oauthRedirect.ts | 12 ++++++++++ test/unit/forge/routes/auth/oauth_spec.js | 18 ++++++++++++++ .../unit/frontend/utils/oauthRedirect.spec.js | 24 +++++++++++++++++++ 6 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 frontend/src/utils/oauthRedirect.ts create mode 100644 test/unit/frontend/utils/oauthRedirect.spec.js diff --git a/forge/routes/auth/oauth.js b/forge/routes/auth/oauth.js index 010e8f4e33..60974ebec9 100644 --- a/forge/routes/auth/oauth.js +++ b/forge/routes/auth/oauth.js @@ -158,6 +158,11 @@ module.exports = async function (app) { } catch (err) { return badRequest(reply, 'invalid_request', 'Invalid redirect_uri') } + // Only http(s) callbacks are valid - this rejects schemes such as javascript: + // that would otherwise pass the path checks below + if (redirectURI.protocol !== 'http:' && redirectURI.protocol !== 'https:') { + return badRequest(reply, 'invalid_request', 'Invalid redirect_uri') + } let isMCP = false if (client_id === 'ff-plugin') { // Ensure redirect_uri path component is correct for the tools plugin diff --git a/frontend/src/pages/account/AccessRequest.vue b/frontend/src/pages/account/AccessRequest.vue index 6f2997e1ab..45117f08db 100644 --- a/frontend/src/pages/account/AccessRequest.vue +++ b/frontend/src/pages/account/AccessRequest.vue @@ -28,6 +28,7 @@ import { mapState } from 'pinia' import client from '@/api/client.ts' import { useAccountAuthStore } from '@/stores/account-auth.js' +import { redirectToOAuthClient } from '@/utils/oauthRedirect.ts' export default { name: 'AccessRequest', @@ -43,11 +44,11 @@ export default { methods: { async allowAccess () { const response = await client.put(`/account/complete/${this.$router.currentRoute.value.params.id}`, {}) - window.location.href = response.data.redirect + redirectToOAuthClient(response.data.redirect) }, async denyAccess () { const response = await client.put(`/account/reject/${this.$router.currentRoute.value.params.id}`, {}) - window.location.href = response.data.redirect + redirectToOAuthClient(response.data.redirect) } } } diff --git a/frontend/src/pages/account/AccessRequestMCP.vue b/frontend/src/pages/account/AccessRequestMCP.vue index 6c0270bf25..f367a5bc32 100644 --- a/frontend/src/pages/account/AccessRequestMCP.vue +++ b/frontend/src/pages/account/AccessRequestMCP.vue @@ -121,6 +121,7 @@ import McpToolPermissions from '../../components/mcp/McpToolPermissions.vue' import client from '@/api/client.ts' import teamApi from '@/api/team.ts' import { useAccountAuthStore } from '@/stores/account-auth.js' +import { redirectToOAuthClient } from '@/utils/oauthRedirect.ts' const ONE_DAY = 1000 * 60 * 60 * 24 const ONE_YEAR = ONE_DAY * 365 @@ -272,7 +273,7 @@ export default { toolPermissions: { default: this.defaultPermissions, teams: this.teamOverrides } }) const response = await client.put(`/account/complete/${this.requestId}`, {}) - window.location.href = response.data.redirect + redirectToOAuthClient(response.data.redirect) } catch (err) { this.error = err.response?.data?.description || 'Failed to process request. Please try again.' this.submitting = false @@ -281,7 +282,7 @@ export default { async denyAccess () { try { const response = await client.put(`/account/reject/${this.requestId}`, {}) - window.location.href = response.data.redirect + redirectToOAuthClient(response.data.redirect) } catch (err) { this.error = err.response?.data?.description || 'Failed to process request. Please try again.' } diff --git a/frontend/src/utils/oauthRedirect.ts b/frontend/src/utils/oauthRedirect.ts new file mode 100644 index 0000000000..194e88d8a8 --- /dev/null +++ b/frontend/src/utils/oauthRedirect.ts @@ -0,0 +1,12 @@ +/** + * Navigate the browser to the OAuth client callback url returned by the + * platform. Only http(s) urls are followed, so a callback using a scheme such + * as `javascript:` can never run in the FlowFuse origin. + */ +export function redirectToOAuthClient (redirect: string): void { + const url = new URL(redirect) + if (url.protocol !== 'http:' && url.protocol !== 'https:') { + throw new Error('Invalid redirect url') + } + window.location.href = url.toString() +} diff --git a/test/unit/forge/routes/auth/oauth_spec.js b/test/unit/forge/routes/auth/oauth_spec.js index cdf63a9933..e973818d42 100644 --- a/test/unit/forge/routes/auth/oauth_spec.js +++ b/test/unit/forge/routes/auth/oauth_spec.js @@ -892,6 +892,24 @@ describe('OAuth', async function () { }) } + it('rejects a non-http(s) ff-plugin redirect_uri that matches the callback path', async function () { + for (const scope of ['ff-plugin', 'ff-assistant']) { + const params = new URLSearchParams({ + client_id: 'ff-plugin', + scope, + response_type: 'code', + redirect_uri: `javascript:alert(document.domain)//x/${scope === 'ff-plugin' ? 'flowfuse-nr-tools' : 'nr-assistant'}/auth/callback`, + state: 'abc', + code_challenge: pkce().challenge, + code_challenge_method: 'S256' + }) + const response = await mcpApp.inject({ method: 'GET', url: `/account/authorize?${params}`, cookies: { sid } }) + response.should.have.property('statusCode', 400) + response.json().should.have.property('error', 'invalid_request') + should.not.exist(response.headers.location) + } + }) + it('refuses GET complete for an MCP request, even with consent', async function () { const { requestId } = await startMCPRequest({ sid }) ;(await consent(requestId, { sid })).should.have.property('statusCode', 200) diff --git a/test/unit/frontend/utils/oauthRedirect.spec.js b/test/unit/frontend/utils/oauthRedirect.spec.js new file mode 100644 index 0000000000..2fd333f3c6 --- /dev/null +++ b/test/unit/frontend/utils/oauthRedirect.spec.js @@ -0,0 +1,24 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' + +import { redirectToOAuthClient } from '../../../../frontend/src/utils/oauthRedirect.ts' + +describe('redirectToOAuthClient', () => { + beforeEach(() => { + vi.stubGlobal('window', { location: { href: 'https://app.example.com/account/request/abc' } }) + }) + afterEach(() => { + vi.unstubAllGlobals() + }) + + test('navigates to an http(s) callback', () => { + redirectToOAuthClient('https://example.com/nr-assistant/auth/callback?code=abc') + expect(window.location.href).toBe('https://example.com/nr-assistant/auth/callback?code=abc') + redirectToOAuthClient('http://localhost:1880/flowfuse-nr-tools/auth/callback?code=abc') + expect(window.location.href).toBe('http://localhost:1880/flowfuse-nr-tools/auth/callback?code=abc') + }) + + test('refuses a javascript: callback', () => { + expect(() => redirectToOAuthClient('javascript:alert(document.domain)//x/nr-assistant/auth/callback?code=abc')).toThrow() + expect(window.location.href).toBe('https://app.example.com/account/request/abc') + }) +}) From 4cf759da22a8c1ba8d3c7690a6e0bdc256b7ab67 Mon Sep 17 00:00:00 2001 From: Nick O'Leary Date: Wed, 21 Oct 2026 18:27:06 +0100 Subject: [PATCH 3/4] Fix UI tests on AccessRequestMCP --- .../pages/account/AccessRequestMCP.spec.js | 40 ++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/test/unit/frontend/pages/account/AccessRequestMCP.spec.js b/test/unit/frontend/pages/account/AccessRequestMCP.spec.js index ff50bb3061..05c02a061e 100644 --- a/test/unit/frontend/pages/account/AccessRequestMCP.spec.js +++ b/test/unit/frontend/pages/account/AccessRequestMCP.spec.js @@ -1,5 +1,5 @@ import { flushPromises, mount } from '@vue/test-utils' -import { expect, vi } from 'vitest' +import { beforeEach, expect, vi } from 'vitest' vi.mock('@/stores/account-auth.js', () => ({ useAccountAuthStore: () => ({ user: null }) @@ -14,7 +14,16 @@ vi.mock('@/api/team.ts', () => ({ }) } })) -const { mockPut } = vi.hoisted(() => ({ mockPut: vi.fn().mockResolvedValue({}) })) +// The complete route returns the client callback url. Use a hash change on the +// current page so jsdom can follow it without attempting a real navigation. +const { mockPut } = vi.hoisted(() => ({ + mockPut: vi.fn(async (url) => { + if (/^\/account\/(complete|reject)\//.test(url)) { + return { data: { redirect: `${window.location.origin}${window.location.pathname}#callback` } } + } + return {} + }) +})) vi.mock('@/api/client.ts', () => ({ default: { put: mockPut } })) @@ -23,6 +32,15 @@ vi.mock('@/api/client.ts', () => ({ import AccessRequestMCP from '../../../../../frontend/src/pages/account/AccessRequestMCP.vue' import FFUIComponents from '../../../../../frontend/src/ui-components/index.js' +beforeEach(() => { + mockPut.mockClear() + window.location.hash = '' +}) + +function consentBody () { + return mockPut.mock.calls.find(([url]) => url.endsWith('/consent'))[1] +} + async function mountPage () { const wrapper = mount(AccessRequestMCP, { global: { @@ -130,7 +148,7 @@ describe('AccessRequestMCP', () => { expect(allowButton(wrapper).attributes('disabled')).toBeUndefined() }) - test('sends the default permissions and an empty team override map to /consent', async () => { + test('sends the default permissions and an empty team override map to /consent, then completes', async () => { const wrapper = await mountPage() await findRadio(wrapper, 'All teams').trigger('click') await setExpiry(wrapper, futureDate(30)) @@ -138,9 +156,11 @@ describe('AccessRequestMCP', () => { await wrapper.find('[data-action="allow-access"]').trigger('click') await flushPromises() - expect(mockPut).toHaveBeenCalledTimes(1) + expect(mockPut).toHaveBeenCalledTimes(2) const [url, body] = mockPut.mock.calls[0] expect(url).toBe('/account/authorize/request-id/consent') + expect(mockPut.mock.calls[1][0]).toBe('/account/complete/request-id') + expect(window.location.hash).toBe('#callback') expect(body.teamIds).toEqual([]) expect(body.toolPermissions.default).toEqual({ platform: { read: true, write: true, destructive: false }, @@ -166,13 +186,23 @@ describe('AccessRequestMCP', () => { await wrapper.find('[data-action="allow-access"]').trigger('click') await flushPromises() - const [, bodyWithOverride] = mockPut.mock.calls.at(-1) + const bodyWithOverride = consentBody() expect(bodyWithOverride.toolPermissions.teams['team-1'].platform).toEqual({ read: true, write: true, destructive: true }) await row.find('[data-action="reset-team-default"]').trigger('click') expect(row.text()).toContain('Default') }) + test('deny rejects the request and follows the returned redirect', async () => { + const wrapper = await mountPage() + await wrapper.find('[data-action="deny-access"]').trigger('click') + await flushPromises() + + expect(mockPut).toHaveBeenCalledTimes(1) + expect(mockPut.mock.calls[0][0]).toBe('/account/reject/request-id') + expect(window.location.hash).toBe('#callback') + }) + test('keeps a customised team open until closed, including after a reset', async () => { const wrapper = await mountPage() await findRadio(wrapper, 'All teams').trigger('click') From c0ddf12b3f818e8a6d0dd3cf7f498817e396f646 Mon Sep 17 00:00:00 2001 From: Andrea Palmieri <76187074+andypalmi@users.noreply.github.com> Date: Thu, 22 Oct 2026 10:53:54 +0200 Subject: [PATCH 4/4] OAuth consent flow follow-ups (#9018) Co-authored-by: andypalmi --- forge/routes/auth/oauth.js | 7 +- frontend/src/pages/account/AccessRequest.vue | 21 ++++- test/unit/forge/routes/auth/oauth_spec.js | 77 +++++++++++-------- .../pages/account/AccessRequest.spec.js | 64 +++++++++++++++ 4 files changed, 130 insertions(+), 39 deletions(-) create mode 100644 test/unit/frontend/pages/account/AccessRequest.spec.js diff --git a/forge/routes/auth/oauth.js b/forge/routes/auth/oauth.js index 60974ebec9..acd87e7254 100644 --- a/forge/routes/auth/oauth.js +++ b/forge/routes/auth/oauth.js @@ -365,6 +365,7 @@ module.exports = async function (app) { reply.send({ redirect: await issueCode(requestObject, request.session.User) }) }) app.put('/account/reject/:code', { + preHandler: verifyBrowserSession, schema: { tags: ['Authentication', 'X-HIDDEN'] } @@ -623,15 +624,15 @@ module.exports = async function (app) { reply.send(response) } else if (requestObject.mcp) { // Only issue a token for a request the user approved on the consent page - if (requestObject.approvedBy !== requestObject.userId || !requestObject.expiresAt) { + if (requestObject.approvedBy !== requestObject.userId || !requestObject.expiresAt || !requestObject.toolPermissions) { return badRequest(reply, 'access_denied', 'Access Denied') } const accessToken = await app.db.controllers.AccessToken.createMCPOAuthToken( requestObject.userId, { - teamIds: requestObject.teamIds || [], + teamIds: requestObject.teamIds, grantExpiresAt: requestObject.expiresAt, - toolPermissions: requestObject.toolPermissions || mcpToolPermissions.fromReadOnly(requestObject.readOnly || false) + toolPermissions: requestObject.toolPermissions } ) const response = { diff --git a/frontend/src/pages/account/AccessRequest.vue b/frontend/src/pages/account/AccessRequest.vue index 45117f08db..cbb823d297 100644 --- a/frontend/src/pages/account/AccessRequest.vue +++ b/frontend/src/pages/account/AccessRequest.vue @@ -15,6 +15,7 @@
This application will have access to your teams and instances.
+
{{ error }}
Deny Allow @@ -38,17 +39,29 @@ export default { ArrowSmallRightIcon, ArrowSmallLeftIcon }, + data () { + return { + error: null + } + }, computed: { ...mapState(useAccountAuthStore, ['user']) }, methods: { async allowAccess () { - const response = await client.put(`/account/complete/${this.$router.currentRoute.value.params.id}`, {}) - redirectToOAuthClient(response.data.redirect) + await this.respond('complete') }, async denyAccess () { - const response = await client.put(`/account/reject/${this.$router.currentRoute.value.params.id}`, {}) - redirectToOAuthClient(response.data.redirect) + await this.respond('reject') + }, + async respond (action) { + this.error = null + try { + const response = await client.put(`/account/${action}/${this.$router.currentRoute.value.params.id}`, {}) + redirectToOAuthClient(response.data.redirect) + } catch (err) { + this.error = err.response?.data?.description || 'Failed to process request. Please try again.' + } } } } diff --git a/test/unit/forge/routes/auth/oauth_spec.js b/test/unit/forge/routes/auth/oauth_spec.js index e973818d42..9b28aa466d 100644 --- a/test/unit/forge/routes/auth/oauth_spec.js +++ b/test/unit/forge/routes/auth/oauth_spec.js @@ -760,37 +760,6 @@ describe('OAuth', async function () { issued.MCPGrant.MCPGrantTeamPermissions.should.have.length(0) }) - it('builds a read-only grant at exchange from a session saved with only readOnly', async function () { - const reg = (await register()).json() - const { verifier, challenge } = pkce() - const authResponse = await mcpApp.inject({ method: 'GET', url: authorizeURL(reg.client_id, redirectURI, challenge), cookies: { sid } }) - const requestId = /\/account\/request\/([^/]+)\/mcp$/.exec(authResponse.headers.location)[1] - await mcpApp.inject({ - method: 'PUT', - url: `/account/authorize/${requestId}/consent`, - payload: { teamIds: [], expiresAt: Date.now() + 1000 * 60 * 60, toolPermissions: toolPermissionsPayload(false) }, - cookies: { sid } - }) - const oauthSession = await mcpApp.db.models.OAuthSession.findOne({ where: { id: requestId } }) - const { toolPermissions, ...legacyValue } = oauthSession.value - oauthSession.value = { ...legacyValue, readOnly: true } - await oauthSession.save() - const completeResponse = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${requestId}`, payload: {}, cookies: { sid } }) - const authCode = new URL(completeResponse.json().redirect).searchParams.get('code') - const tokenResponse = await mcpApp.inject({ - method: 'POST', - url: '/account/token', - payload: { grant_type: 'authorization_code', code: authCode, redirect_uri: redirectURI, client_id: reg.client_id, code_verifier: verifier } - }) - const issued = await mcpApp.db.models.AccessToken.byRefreshToken(tokenResponse.json().refresh_token) - issued.readOnly.should.be.true() - const grant = await mcpApp.db.models.MCPGrant.findOne({ where: { AccessTokenId: issued.id } }) - grant.permissions.should.eql({ - platform: { read: true, write: false, destructive: false }, - flow_building: { read: true, write: false, destructive: false } - }) - }) - async function sessionFor (token) { const request = { headers: { authorization: `Bearer ${token}` }, routeOptions: { config: { allowMcpGrant: true } }, requestContext: { set () {} } } await mcpApp.verifySession(request, { code () { return this }, send () {} }) @@ -937,13 +906,40 @@ describe('OAuth', async function () { }) it('rejects a request via PUT', async function () { - const response = await mcpApp.inject({ method: 'PUT', url: `/account/reject/${await startPluginRequest()}`, payload: {} }) + const response = await mcpApp.inject({ method: 'PUT', url: `/account/reject/${await startPluginRequest()}`, payload: {}, cookies: { sid } }) response.should.have.property('statusCode', 200) const callback = new URL(response.json().redirect) callback.searchParams.get('error').should.equal('access_denied') callback.searchParams.get('state').should.equal('abc') }) + it('refuses PUT reject without a session', async function () { + const response = await mcpApp.inject({ method: 'PUT', url: `/account/reject/${await startPluginRequest()}`, payload: {} }) + response.should.have.property('statusCode', 401) + }) + + it('refuses the editor auth shortcut for an MCP client', async function () { + // A logged-in editor-scope request bounces straight to GET complete, + // which must not issue a code for an MCP client + const reg = (await register()).json() + const params = new URLSearchParams({ + client_id: reg.client_id, + scope: 'editor', + response_type: 'code', + redirect_uri: redirectURI, + state: 'abc', + code_challenge: pkce().challenge, + code_challenge_method: 'S256' + }) + const authResponse = await mcpApp.inject({ method: 'GET', url: `/account/authorize?${params}`, cookies: { sid } }) + authResponse.should.have.property('statusCode', 302) + const completePath = new URL(authResponse.headers.location).pathname + completePath.should.match(/^\/account\/complete\//) + const completeResponse = await mcpApp.inject({ method: 'GET', url: completePath, cookies: { sid } }) + completeResponse.should.have.property('statusCode', 400) + should.not.exist(completeResponse.headers.location) + }) + it('refuses PUT complete for an MCP request without consent', async function () { const { requestId } = await startMCPRequest() const response = await mcpApp.inject({ method: 'PUT', url: `/account/complete/${requestId}`, payload: {}, cookies: { sid } }) @@ -988,6 +984,23 @@ describe('OAuth', async function () { tokenResponse.should.have.property('statusCode', 400) tokenResponse.json().should.have.property('error', 'access_denied') }) + + it('refuses to exchange an approved MCP code without tool permissions', async function () { + const { clientID, verifier, requestId } = await startMCPRequest() + const requestObject = (await mcpApp.db.models.OAuthSession.findOne({ where: { id: requestId } })).value + const code = base64URLEncode(crypto.randomBytes(32)) + await mcpApp.db.models.OAuthSession.create({ + id: code, + value: { ...requestObject, userId: mcpUser.id, approvedBy: mcpUser.id, expiresAt: Date.now() + 1000 * 60 * 60, teamIds: [], code } + }) + const tokenResponse = await mcpApp.inject({ + method: 'POST', + url: '/account/token', + payload: { grant_type: 'authorization_code', code, redirect_uri: redirectURI, client_id: clientID, code_verifier: verifier } + }) + tokenResponse.should.have.property('statusCode', 400) + tokenResponse.json().should.have.property('error', 'access_denied') + }) }) }) }) diff --git a/test/unit/frontend/pages/account/AccessRequest.spec.js b/test/unit/frontend/pages/account/AccessRequest.spec.js new file mode 100644 index 0000000000..3ff444410f --- /dev/null +++ b/test/unit/frontend/pages/account/AccessRequest.spec.js @@ -0,0 +1,64 @@ +import { flushPromises, mount } from '@vue/test-utils' +import { beforeEach, expect, vi } from 'vitest' + +vi.mock('@/stores/account-auth.js', () => ({ + useAccountAuthStore: () => ({ user: null }) +})) +// The complete and reject routes return the client callback url. Use a hash change +// on the current page so jsdom can follow it without attempting a real navigation. +const { mockPut } = vi.hoisted(() => ({ + mockPut: vi.fn(async () => ({ data: { redirect: `${window.location.origin}${window.location.pathname}#callback` } })) +})) +vi.mock('@/api/client.ts', () => ({ + default: { put: mockPut } +})) + +// imported after mocks so vi.mock hoisting resolves correctly +import AccessRequest from '../../../../../frontend/src/pages/account/AccessRequest.vue' +import FFUIComponents from '../../../../../frontend/src/ui-components/index.js' + +beforeEach(() => { + mockPut.mockClear() + window.location.hash = '' +}) + +function mountPage () { + return mount(AccessRequest, { + global: { + plugins: [FFUIComponents], + mocks: { + $router: { currentRoute: { value: { params: { id: 'request-id' } } } } + } + } + }) +} + +describe('AccessRequest', () => { + test('allow completes the request and follows the returned redirect', async () => { + const wrapper = mountPage() + await wrapper.find('[data-action="allow-access"]').trigger('click') + await flushPromises() + + expect(mockPut.mock.calls[0][0]).toBe('/account/complete/request-id') + expect(window.location.hash).toBe('#callback') + }) + + test('deny rejects the request and follows the returned redirect', async () => { + const wrapper = mountPage() + await wrapper.find('[data-action="deny-access"]').trigger('click') + await flushPromises() + + expect(mockPut.mock.calls[0][0]).toBe('/account/reject/request-id') + expect(window.location.hash).toBe('#callback') + }) + + test('shows the error when the request can no longer be completed', async () => { + mockPut.mockRejectedValueOnce({ response: { data: { error: 'invalid_request', description: 'Invalid request' } } }) + const wrapper = mountPage() + await wrapper.find('[data-action="allow-access"]').trigger('click') + await flushPromises() + + expect(wrapper.text()).toContain('Invalid request') + expect(window.location.hash).toBe('') + }) +})