From ce8f0512d7877773600a1267d9795661435245d2 Mon Sep 17 00:00:00 2001 From: andypalmi Date: Fri, 9 Oct 2026 14:03:59 +0200 Subject: [PATCH] Rely on the shared id guard in the team, pipeline and broker delete tools The id checks in these handlers repeat what the pathId schemas and the guard in invokeTool already refuse, so they are removed along with their handler-level tests. --- forge/ee/lib/mcp/tools/broker.js | 15 ++---------- forge/ee/lib/mcp/tools/pipelines.js | 11 --------- forge/ee/lib/mcp/tools/teams.js | 12 ---------- .../forge/ee/lib/mcp/tools/broker_spec.js | 23 ------------------- .../forge/ee/lib/mcp/tools/pipelines_spec.js | 18 --------------- .../unit/forge/ee/lib/mcp/tools/teams_spec.js | 19 --------------- 6 files changed, 2 insertions(+), 96 deletions(-) diff --git a/forge/ee/lib/mcp/tools/broker.js b/forge/ee/lib/mcp/tools/broker.js index f6a12195c7..8552a111b9 100644 --- a/forge/ee/lib/mcp/tools/broker.js +++ b/forge/ee/lib/mcp/tools/broker.js @@ -1,8 +1,6 @@ const { z } = require('zod') -const { pathId, teamId, basePagination, basePaginationKeys, searchQuery, appendQuery, toolError } = require('../schemas') - -const isHashid = (id) => /^[A-Za-z0-9]+$/.test(id) +const { pathId, teamId, basePagination, basePaginationKeys, searchQuery, appendQuery } = require('../schemas') module.exports = [ { @@ -203,11 +201,6 @@ module.exports = [ topicId: pathId.describe('The hashid of the topic to delete, as returned by platform_list_broker_topics') }, handler: async (args, { inject }) => { - // The ids go into the URL path and inject resolves dot segments, so anything but a plain - // id could send this DELETE to another route (e.g. topicId "../../../../../applications/"). - if (!isHashid(args.teamId) || !(args.brokerId === 'team-broker' || isHashid(args.brokerId)) || !isHashid(args.topicId)) { - return toolError(400, 'invalid_request', 'teamId and topicId must be hashids, and brokerId "team-broker" or a hashid') - } const response = await inject({ method: 'DELETE', url: `/api/v1/teams/${args.teamId}/brokers/${args.brokerId}/topics/${args.topicId}` }) return response } @@ -230,11 +223,7 @@ module.exports = [ }, handler: async (args, { inject }) => { // Usernames are free text, so encode the username to keep a "/" from splitting it into - // more path segments. "." and ".." survive encoding and would still be resolved as dot - // segments, which could send this DELETE to another route. - if (!isHashid(args.teamId) || ['.', '..'].includes(args.username)) { - return toolError(400, 'invalid_request', 'teamId must be a hashid, and username cannot be "." or ".."') - } + // more path segments. const response = await inject({ method: 'DELETE', url: `/api/v1/teams/${args.teamId}/broker/client/${encodeURIComponent(args.username)}` }) return response } diff --git a/forge/ee/lib/mcp/tools/pipelines.js b/forge/ee/lib/mcp/tools/pipelines.js index 63e5b4a7b0..2409836cc1 100644 --- a/forge/ee/lib/mcp/tools/pipelines.js +++ b/forge/ee/lib/mcp/tools/pipelines.js @@ -2,8 +2,6 @@ const { z } = require('zod') const { pathId, teamId, applicationId, toolError, gitStageFields, gitStageFieldKeys } = require('../schemas') -const isHashid = (id) => /^[A-Za-z0-9]+$/.test(id) - const stageAction = z.enum(['create_snapshot', 'use_active_snapshot', 'use_latest_snapshot', 'prompt', 'none']).optional() .describe('How the stage obtains the snapshot it passes on when deployed FROM: create_snapshot makes a new one (the default), use_active_snapshot and use_latest_snapshot reuse existing ones, prompt requires a sourceSnapshotId at deploy time, none makes deploys from this stage a no-op. Not meaningful for git-repo stages') const deployToDevices = z.boolean().optional() @@ -198,11 +196,6 @@ module.exports = [ pipelineId: pathId.describe('The hashid of the pipeline to delete') }, handler: async (args, { inject }) => { - // pipelineId goes into the URL path and inject resolves dot segments, so anything but - // a plain hashid could send this DELETE to another route (e.g. "../applications/"). - if (!isHashid(args.pipelineId)) { - return toolError(400, 'invalid_request', 'pipelineId must be a hashid') - } const response = await inject({ method: 'DELETE', url: `/api/v1/pipelines/${args.pipelineId}` }) return response } @@ -225,10 +218,6 @@ module.exports = [ stageId: pathId.describe('The hashid of the stage to delete') }, handler: async (args, { inject }) => { - // Same as above, for both ids. - if (!isHashid(args.pipelineId) || !isHashid(args.stageId)) { - return toolError(400, 'invalid_request', 'pipelineId and stageId must be hashids') - } const response = await inject({ method: 'DELETE', url: `/api/v1/pipelines/${args.pipelineId}/stages/${args.stageId}` }) return response } diff --git a/forge/ee/lib/mcp/tools/teams.js b/forge/ee/lib/mcp/tools/teams.js index 78ee0cd4bd..25230baaf9 100644 --- a/forge/ee/lib/mcp/tools/teams.js +++ b/forge/ee/lib/mcp/tools/teams.js @@ -2,8 +2,6 @@ const { z } = require('zod') const { pathId, teamId, applicationId, basePagination, basePaginationKeys, searchQuery, searchQueryKeys, auditLogFilters, auditLogFilterKeys, appendQuery, toolError } = require('../schemas') -const isHashid = (id) => /^[A-Za-z0-9]+$/.test(id) - // Audit-log routes accept cursor+limit pagination, free-text query, event // (single name or array) and username. scope narrows which entity levels are // returned; includeChildren pulls in descendant entries within the chosen scope. @@ -318,12 +316,6 @@ module.exports = [ userId: pathId.describe('The hashid of the member to remove, as returned by platform_list_team_members') }, handler: async (args, { inject }) => { - // Both ids go into the URL path and inject resolves dot segments, so anything but - // a plain hashid could send this DELETE to another route (e.g. teamId "../applications" - // with userId "../" deletes an application). - if (!isHashid(args.teamId) || !isHashid(args.userId)) { - return toolError(400, 'invalid_request', 'teamId and userId must be hashids') - } const response = await inject({ method: 'DELETE', url: `/api/v1/teams/${args.teamId}/members/${args.userId}` }) // The route answers 200 for a user who is not a member, with removed: false in the body. if (response.statusCode === 200 && response.json().removed === false) { @@ -348,10 +340,6 @@ module.exports = [ invitationId: pathId.describe('The hashid of the invitation to revoke, as returned by platform_list_team_invitations') }, handler: async (args, { inject }) => { - // Same as above: a crafted invitationId like "../../../applications/" reaches another route. - if (!isHashid(args.teamId) || !isHashid(args.invitationId)) { - return toolError(400, 'invalid_request', 'teamId and invitationId must be hashids') - } const response = await inject({ method: 'DELETE', url: `/api/v1/teams/${args.teamId}/invitations/${args.invitationId}` }) return response } diff --git a/test/unit/forge/ee/lib/mcp/tools/broker_spec.js b/test/unit/forge/ee/lib/mcp/tools/broker_spec.js index b3096b5bc6..f3920b2d41 100644 --- a/test/unit/forge/ee/lib/mcp/tools/broker_spec.js +++ b/test/unit/forge/ee/lib/mcp/tools/broker_spec.js @@ -273,20 +273,6 @@ describe('MCP Broker Tools', function () { response.should.equal(errorResponse) }) - it('refuses ids that would reach another route', async function () { - for (const [teamId, brokerId, topicId] of [ - ['team1', 'team-broker', '../../../../../applications/app1'], - ['team1', '../../applications/app1', 'topic1'], - ['../applications', 'team-broker', 'topic1'], - ['team1', 'team-broker', 'topic1?x=1'] - ]) { - const response = await tool.handler({ teamId, brokerId, topicId }, { inject }) - response.statusCode.should.equal(400) - response.json().should.have.property('code', 'invalid_request') - } - inject.called.should.be.false() - }) - it('accepts a 3rd-party broker hashid', async function () { inject.resolves({ statusCode: 201, json: () => ({}) }) @@ -327,14 +313,5 @@ describe('MCP Broker Tools', function () { await tool.handler({ teamId: 'team1', username: 'rv:client/2' }, { inject }) inject.firstCall.args[0].url.should.equal(`/api/v1/teams/team1/broker/client/${encodeURIComponent('rv:client/2')}`) }) - - it('refuses ids that would reach another route', async function () { - for (const [teamId, username] of [['team1', '..'], ['team1', '.'], ['../applications', 'alice']]) { - const response = await tool.handler({ teamId, username }, { inject }) - response.statusCode.should.equal(400) - response.json().should.have.property('code', 'invalid_request') - } - inject.called.should.be.false() - }) }) }) diff --git a/test/unit/forge/ee/lib/mcp/tools/pipelines_spec.js b/test/unit/forge/ee/lib/mcp/tools/pipelines_spec.js index f8156b5fa1..5de70ba495 100644 --- a/test/unit/forge/ee/lib/mcp/tools/pipelines_spec.js +++ b/test/unit/forge/ee/lib/mcp/tools/pipelines_spec.js @@ -271,15 +271,6 @@ describe('MCP Pipelines Tools', function () { const response = await tool.handler({ pipelineId: 'pipeline1' }, { inject }) response.should.equal(errorResponse) }) - - it('refuses a pipelineId that would reach another route', async function () { - for (const pipelineId of ['../applications/app1', 'pipeline1?x=1', '..']) { - const response = await tool.handler({ pipelineId }, { inject }) - response.statusCode.should.equal(400) - response.json().should.have.property('code', 'invalid_request') - } - inject.called.should.be.false() - }) }) describe('platform_delete_pipeline_stage', function () { @@ -307,14 +298,5 @@ describe('MCP Pipelines Tools', function () { const response = await tool.handler({ pipelineId: 'pipeline1', stageId: 'stage1' }, { inject }) response.should.equal(errorResponse) }) - - it('refuses ids that would reach another route', async function () { - for (const [pipelineId, stageId] of [['pipeline1', '../../../applications/app1'], ['../applications/app1', 'stage1'], ['pipeline1', 'stage1?x=1']]) { - const response = await tool.handler({ pipelineId, stageId }, { inject }) - response.statusCode.should.equal(400) - response.json().should.have.property('code', 'invalid_request') - } - inject.called.should.be.false() - }) }) }) diff --git a/test/unit/forge/ee/lib/mcp/tools/teams_spec.js b/test/unit/forge/ee/lib/mcp/tools/teams_spec.js index aa95d8c32e..f0840fba1f 100644 --- a/test/unit/forge/ee/lib/mcp/tools/teams_spec.js +++ b/test/unit/forge/ee/lib/mcp/tools/teams_spec.js @@ -354,16 +354,6 @@ describe('MCP Teams Tools', function () { response.json().should.have.property('code', 'not_found') }) - it('refuses ids that would reach another route', async function () { - // "../applications" + "../app1" resolves the DELETE to /api/v1/applications/app1 - for (const [teamId, userId] of [['../applications', '../app1'], ['team1', '../../../applications/app1'], ['team1', 'user1?x=1']]) { - const response = await tool.handler({ teamId, userId }, { inject }) - response.statusCode.should.equal(400) - response.json().should.have.property('code', 'invalid_request') - } - inject.called.should.be.false() - }) - it('passes through the 400 for the only owner', async function () { const errorResponse = { statusCode: 400, json: () => ({ code: 'invalid_request', error: 'cannot remove only owner' }) } inject.withArgs({ method: 'DELETE', url: '/api/v1/teams/team1/members/user1' }).resolves(errorResponse) @@ -398,14 +388,5 @@ describe('MCP Teams Tools', function () { const response = await tool.handler({ teamId: 'team1', invitationId: 'invite1' }, { inject }) response.should.equal(errorResponse) }) - - it('refuses ids that would reach another route', async function () { - for (const [teamId, invitationId] of [['team1', '../../../applications/app1'], ['../applications', 'app1'], ['team1', 'invite1?x=1']]) { - const response = await tool.handler({ teamId, invitationId }, { inject }) - response.statusCode.should.equal(400) - response.json().should.have.property('code', 'invalid_request') - } - inject.called.should.be.false() - }) }) })