OAuth consent flow follow-ups

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 = {

frontend/src/pages/account/AccessRequest.vue

@@ -15,6 +15,7 @@
         <div class="my-4">
             This application will have access to your teams and instances.
         </div>
+        <div v-if="error" class="text-red-600 text-sm mb-4">{{ error }}</div>
         <div class="ff-actions flex flex-row">
             <ff-button class="mx-8" data-action="deny-access" @click="denyAccess">Deny</ff-button>
             <ff-button class="mx-8" data-action="allow-access" @click="allowAccess">Allow</ff-button>
@@ -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.'
+            }
         }
     }
 }

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')
+            })
         })
     })
 })

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('')
+    })
+})