From 24123c0ceb054be3ddd4dbf2b7649b4722712194 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Thu, 24 Sep 2026 21:18:31 +0200 Subject: [PATCH] fix(mcp): decouple access from REST API settings --- routes/ai.php | 3 +-- routes/api.php | 5 ++--- tests/Feature/AdvisorySecurityRegressionTest.php | 8 ++++---- tests/Feature/LoginAndMcpAccessTest.php | 10 ++++++---- tests/Feature/Mcp/McpEndpointTest.php | 9 ++++++--- tests/Feature/Mcp/McpToggleApiTest.php | 10 +++++----- 6 files changed, 24 insertions(+), 21 deletions(-) diff --git a/routes/ai.php b/routes/ai.php index ac7aec4da1..d79ff0ebae 100644 --- a/routes/ai.php +++ b/routes/ai.php @@ -1,8 +1,7 @@ middleware(['mcp.enabled', 'auth:sanctum', 'api.token.team', ApiAllowed::class, 'mcp.team.enabled']); + ->middleware(['mcp.enabled', 'auth:sanctum', 'api.token.team', 'mcp.team.enabled']); diff --git a/routes/api.php b/routes/api.php index f9407d04f9..d1ce47dfaa 100644 --- a/routes/api.php +++ b/routes/api.php @@ -58,15 +58,14 @@ Route::group([ Route::get('/disable', [OtherController::class, 'post_required']); Route::post('/enable', [OtherController::class, 'enable_api']); Route::post('/disable', [OtherController::class, 'disable_api']); + Route::post('/mcp/enable', [OtherController::class, 'enable_mcp']); + Route::post('/mcp/disable', [OtherController::class, 'disable_mcp']); }); Route::group([ 'middleware' => ['auth:sanctum', 'api.token.team', ApiAllowed::class, 'api.sensitive'], 'prefix' => 'v1', ], function () { - Route::post('/mcp/enable', [OtherController::class, 'enable_mcp'])->middleware('api.ability:write'); - Route::post('/mcp/disable', [OtherController::class, 'disable_mcp'])->middleware('api.ability:write'); - Route::get('/version', [OtherController::class, 'version'])->middleware(['api.ability:read']); Route::get('/audit-events', [AuditEventsController::class, 'index'])->middleware(['api.ability:read']); diff --git a/tests/Feature/AdvisorySecurityRegressionTest.php b/tests/Feature/AdvisorySecurityRegressionTest.php index 7f1349bf5f..d35602ea4b 100644 --- a/tests/Feature/AdvisorySecurityRegressionTest.php +++ b/tests/Feature/AdvisorySecurityRegressionTest.php @@ -49,15 +49,15 @@ it('does not require email verification for protected web routes', function () { $this->actingAs($user)->get('/analytics')->assertOk(); }); -it('applies the API allowlist to MCP and MCP switch routes', function () { +it('does not apply the REST API allowlist to MCP and MCP switch routes', function () { $mcp = Route::getRoutes()->match(Request::create('/mcp', 'POST')); $enable = Route::getRoutes()->match(Request::create('/api/v1/mcp/enable', 'POST')); $disable = Route::getRoutes()->match(Request::create('/api/v1/mcp/disable', 'POST')); expect($mcp)->not->toBeNull() - ->and($mcp->gatherMiddleware())->toContain(ApiAllowed::class) - ->and($enable->gatherMiddleware())->toContain(ApiAllowed::class) - ->and($disable->gatherMiddleware())->toContain(ApiAllowed::class); + ->and($mcp->gatherMiddleware())->not->toContain(ApiAllowed::class) + ->and($enable->gatherMiddleware())->not->toContain(ApiAllowed::class) + ->and($disable->gatherMiddleware())->not->toContain(ApiAllowed::class); }); it('throttles every manual webhook route', function (string $provider) { diff --git a/tests/Feature/LoginAndMcpAccessTest.php b/tests/Feature/LoginAndMcpAccessTest.php index 59a8b7e8a3..5d0325f291 100644 --- a/tests/Feature/LoginAndMcpAccessTest.php +++ b/tests/Feature/LoginAndMcpAccessTest.php @@ -18,12 +18,14 @@ it('limits login attempts by normalized email independent of IP', function () { ->and($firstLimits[1]->key)->toBe($secondLimits[1]->key); }); -it('applies the API access check to MCP and its switch routes', function () { +it('keeps MCP independent from the REST API access check', function () { $mcp = Route::getRoutes()->match(Request::create('/mcp', 'POST')); $enable = Route::getRoutes()->match(Request::create('/api/v1/mcp/enable', 'POST')); $disable = Route::getRoutes()->match(Request::create('/api/v1/mcp/disable', 'POST')); - expect($mcp->gatherMiddleware())->toContain(ApiAllowed::class) - ->and($enable->gatherMiddleware())->toContain(ApiAllowed::class, 'api.ability:write') - ->and($disable->gatherMiddleware())->toContain(ApiAllowed::class, 'api.ability:write'); + expect($mcp->gatherMiddleware())->not->toContain(ApiAllowed::class) + ->and($enable->gatherMiddleware())->not->toContain(ApiAllowed::class) + ->and($disable->gatherMiddleware())->not->toContain(ApiAllowed::class) + ->and($enable->gatherMiddleware())->toContain('auth:sanctum', 'api.token.team', 'api.ability:write') + ->and($disable->gatherMiddleware())->toContain('auth:sanctum', 'api.token.team', 'api.ability:write'); }); diff --git a/tests/Feature/Mcp/McpEndpointTest.php b/tests/Feature/Mcp/McpEndpointTest.php index d290013cc9..43f300a0fe 100644 --- a/tests/Feature/Mcp/McpEndpointTest.php +++ b/tests/Feature/Mcp/McpEndpointTest.php @@ -117,12 +117,15 @@ test('MCP endpoint rejects unauthenticated requests', function () { $response->assertStatus(401); }); -test('MCP endpoint rejects a token from an IP outside the API allow-list', function () { - InstanceSettings::query()->where('id', 0)->update(['allowed_ips' => '192.0.2.10']); +test('MCP endpoint works when the REST API is disabled and its IP allow-list excludes the client', function () { + InstanceSettings::query()->where('id', 0)->update(['is_api_enabled' => false, 'allowed_ips' => '192.0.2.10']); Once::flush(); $token = $this->user->createToken('mcp-read', ['read'])->plainTextToken; - mcpListTools($token)->assertForbidden(); + mcpListTools($token)->assertOk(); + test()->withHeader('Authorization', 'Bearer '.$token) + ->getJson('/api/v1/version') + ->assertForbidden(); }); test('MCP endpoint lists tools for an authenticated token', function () { diff --git a/tests/Feature/Mcp/McpToggleApiTest.php b/tests/Feature/Mcp/McpToggleApiTest.php index 1048617ada..0de306cd01 100644 --- a/tests/Feature/Mcp/McpToggleApiTest.php +++ b/tests/Feature/Mcp/McpToggleApiTest.php @@ -58,7 +58,7 @@ test('POST /api/v1/mcp/enable enables MCP server with root token', function () { }); test('POST /api/v1/mcp/disable disables MCP server with root token', function () { - InstanceSettings::query()->where('id', 0)->update(['is_mcp_server_enabled' => true]); + InstanceSettings::query()->where('id', 0)->update(['is_mcp_server_enabled' => true, 'is_api_enabled' => false]); $token = makeRootMcpToken($this->user); $response = test()->withHeaders([ @@ -93,15 +93,15 @@ test('non-root token cannot disable MCP server', function () { expect(InstanceSettings::find(0)->is_mcp_server_enabled)->toBeTrue(); }); -test('root token cannot enable MCP server from an IP outside the API allow-list', function () { - InstanceSettings::query()->where('id', 0)->update(['allowed_ips' => '192.0.2.10']); +test('root token can enable MCP server when the REST API is disabled', function () { + InstanceSettings::query()->where('id', 0)->update(['is_api_enabled' => false, 'allowed_ips' => '192.0.2.10']); $token = makeRootMcpToken($this->user); test()->withHeaders(['Authorization' => 'Bearer '.$token]) ->postJson('/api/v1/mcp/enable') - ->assertForbidden(); + ->assertOk(); - expect(InstanceSettings::find(0)->is_mcp_server_enabled)->toBeFalse(); + expect(InstanceSettings::find(0)->is_mcp_server_enabled)->toBeTrue(); }); test('unauthenticated request to /api/v1/mcp/enable returns 401', function () {