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