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