diff --git a/pkgs/dart_tooling_mcp_server/lib/src/mixins/analyzer.dart b/pkgs/dart_tooling_mcp_server/lib/src/mixins/analyzer.dart index 98faf3ba..953fccd1 100644 --- a/pkgs/dart_tooling_mcp_server/lib/src/mixins/analyzer.dart +++ b/pkgs/dart_tooling_mcp_server/lib/src/mixins/analyzer.dart @@ -271,7 +271,7 @@ base mixin DartAnalyzerSupport return CallToolResult(content: [TextContent(text: jsonEncode(result))]); } - /// Ensures that all prerequites for any analysis task are met. + /// Ensures that all prerequisites for any analysis task are met. /// /// Returns an error response if any prerequisite is not met, otherwise /// returns `null`. diff --git a/pkgs/dart_tooling_mcp_server/lib/src/mixins/dart_cli.dart b/pkgs/dart_tooling_mcp_server/lib/src/mixins/dart_cli.dart index 844a1dbf..7960483d 100644 --- a/pkgs/dart_tooling_mcp_server/lib/src/mixins/dart_cli.dart +++ b/pkgs/dart_tooling_mcp_server/lib/src/mixins/dart_cli.dart @@ -7,6 +7,7 @@ import 'dart:async'; import 'package:dart_mcp/server.dart'; import '../utils/cli_utils.dart'; +import '../utils/constants.dart'; import '../utils/process_manager.dart'; // TODO: migrate the analyze files tool to use this mixin and run the @@ -17,13 +18,19 @@ import '../utils/process_manager.dart'; /// /// The MCPServer must already have the [ToolsSupport] and [LoggingSupport] /// mixins applied. -base mixin DartCliSupport on ToolsSupport, LoggingSupport +base mixin DartCliSupport on ToolsSupport, LoggingSupport, RootsTrackingSupport implements ProcessManagerSupport { @override FutureOr initialize(InitializeRequest request) { - registerTool(dartFixTool, _runDartFixTool); - registerTool(dartFormatTool, _runDartFormatTool); - return super.initialize(request); + try { + return super.initialize(request); + } finally { + // Can't call this until after `super.initialize`. + if (supportsRoots) { + registerTool(dartFixTool, _runDartFixTool); + registerTool(dartFormatTool, _runDartFormatTool); + } + } } /// Implementation of the [dartFixTool]. @@ -33,6 +40,7 @@ base mixin DartCliSupport on ToolsSupport, LoggingSupport command: ['dart', 'fix', '--apply'], commandDescription: 'dart fix', processManager: processManager, + knownRoots: await roots, ); } @@ -44,6 +52,7 @@ base mixin DartCliSupport on ToolsSupport, LoggingSupport commandDescription: 'dart format', processManager: processManager, defaultPaths: ['.'], + knownRoots: await roots, ); } @@ -52,22 +61,7 @@ base mixin DartCliSupport on ToolsSupport, LoggingSupport description: 'Runs `dart fix --apply` for the given project roots.', annotations: ToolAnnotations(title: 'Dart fix', destructiveHint: true), inputSchema: Schema.object( - properties: { - 'roots': Schema.list( - title: 'All projects roots to run dart fix in.', - description: - 'These must match a root returned by a call to "listRoots".', - items: Schema.object( - properties: { - 'root': Schema.string( - title: 'The URI of the project root to run `dart fix` in.', - ), - }, - required: ['root'], - ), - ), - }, - required: ['roots'], + properties: {ParameterNames.roots: rootsSchema()}, ), ); @@ -76,29 +70,7 @@ base mixin DartCliSupport on ToolsSupport, LoggingSupport description: 'Runs `dart format .` for the given project roots.', annotations: ToolAnnotations(title: 'Dart format', destructiveHint: true), inputSchema: Schema.object( - properties: { - 'roots': Schema.list( - title: 'All projects roots to run dart format in.', - description: - 'These must match a root returned by a call to "listRoots".', - items: Schema.object( - properties: { - 'root': Schema.string( - title: 'The URI of the project root to run `dart format` in.', - ), - 'paths': Schema.list( - title: - 'Relative or absolute paths to analyze under the ' - '"root". Paths must correspond to files and not ' - 'directories.', - items: Schema.string(), - ), - }, - required: ['root'], - ), - ), - }, - required: ['roots'], + properties: {ParameterNames.roots: rootsSchema(supportsPaths: true)}, ), ); } diff --git a/pkgs/dart_tooling_mcp_server/lib/src/mixins/pub.dart b/pkgs/dart_tooling_mcp_server/lib/src/mixins/pub.dart index 69390cd8..1bd46a4e 100644 --- a/pkgs/dart_tooling_mcp_server/lib/src/mixins/pub.dart +++ b/pkgs/dart_tooling_mcp_server/lib/src/mixins/pub.dart @@ -7,6 +7,7 @@ import 'dart:async'; import 'package:dart_mcp/server.dart'; import '../utils/cli_utils.dart'; +import '../utils/constants.dart'; import '../utils/process_manager.dart'; /// Mix this in to any MCPServer to add support for running Pub commands like @@ -16,17 +17,22 @@ import '../utils/process_manager.dart'; /// /// The MCPServer must already have the [ToolsSupport] and [LoggingSupport] /// mixins applied. -base mixin PubSupport on ToolsSupport, LoggingSupport +base mixin PubSupport on ToolsSupport, LoggingSupport, RootsTrackingSupport implements ProcessManagerSupport { @override FutureOr initialize(InitializeRequest request) { - registerTool(pubTool, _runDartPubTool); - return super.initialize(request); + try { + return super.initialize(request); + } finally { + if (supportsRoots) { + registerTool(pubTool, _runDartPubTool); + } + } } /// Implementation of the [pubTool]. Future _runDartPubTool(CallToolRequest request) async { - final command = request.arguments?['command'] as String?; + final command = request.arguments?[ParameterNames.command] as String?; if (command == null) { return CallToolResult( content: [TextContent(text: 'Missing required argument `command`.')], @@ -48,7 +54,8 @@ base mixin PubSupport on ToolsSupport, LoggingSupport ); } - final packageName = request.arguments?['packageName'] as String?; + final packageName = + request.arguments?[ParameterNames.packageName] as String?; if (matchingCommand.requiresPackageName && packageName == null) { return CallToolResult( content: [ @@ -69,6 +76,7 @@ base mixin PubSupport on ToolsSupport, LoggingSupport command: ['dart', 'pub', command, if (packageName != null) packageName], commandDescription: 'dart pub $command', processManager: processManager, + knownRoots: await roots, ); } @@ -80,34 +88,20 @@ base mixin PubSupport on ToolsSupport, LoggingSupport annotations: ToolAnnotations(title: 'pub', readOnlyHint: false), inputSchema: Schema.object( properties: { - 'command': Schema.string( + ParameterNames.command: Schema.string( title: 'The dart pub command to run.', description: 'Currently only ${SupportedPubCommand.listAll} are supported.', ), - 'packageName': Schema.string( + ParameterNames.packageName: Schema.string( title: 'The package name to run the command for.', description: 'This is required for the ' '${SupportedPubCommand.listAllThatRequirePackageName} commands.', ), - 'roots': Schema.list( - title: 'All projects roots to run the dart pub command in.', - description: - 'These must match a root returned by a call to "listRoots".', - items: Schema.object( - properties: { - 'root': Schema.string( - title: - 'The URI of the project root to run the dart pub command ' - 'in.', - ), - }, - required: ['root'], - ), - ), + ParameterNames.roots: rootsSchema(), }, - required: ['command', 'roots'], + required: [ParameterNames.command], ), ); } diff --git a/pkgs/dart_tooling_mcp_server/lib/src/utils/cli_utils.dart b/pkgs/dart_tooling_mcp_server/lib/src/utils/cli_utils.dart index 4e2d5f22..9d426a62 100644 --- a/pkgs/dart_tooling_mcp_server/lib/src/utils/cli_utils.dart +++ b/pkgs/dart_tooling_mcp_server/lib/src/utils/cli_utils.dart @@ -5,8 +5,11 @@ import 'dart:io'; import 'package:dart_mcp/server.dart'; +import 'package:path/path.dart' as p; import 'package:process/process.dart'; +import 'constants.dart'; + /// Runs [command] in each of the project roots specified in the [request]. /// /// The [command] should be a list of strings that can be passed directly to @@ -16,6 +19,10 @@ import 'package:process/process.dart'; /// being run. For example, if the command is `['dart', 'fix', '--apply']`, the /// command description might be `dart fix`. /// +/// The [knownRoots] are used by default if no roots are provided as an +/// argument on the [request]. Otherwise, all roots provided in the request +/// arguments must still be encapsulated by the [knownRoots]. +/// /// [defaultPaths] may be specified if one or more path arguments are required /// for the command (e.g. `dart format `). The paths can be /// absolute or relative paths that point to the directories on which the @@ -29,20 +36,23 @@ Future runCommandInRoots( required List command, required String commandDescription, required ProcessManager processManager, + required List knownRoots, List defaultPaths = const [], }) async { - final rootConfigs = - (request.arguments?['roots'] as List?)?.cast>(); - if (rootConfigs == null) { - return CallToolResult( - content: [TextContent(text: 'Missing required argument `roots`.')], - isError: true, - ); + var rootConfigs = + (request.arguments?[ParameterNames.roots] as List?) + ?.cast>(); + + // Default to use the known roots if none were specified. + if (rootConfigs == null || rootConfigs.isEmpty) { + rootConfigs = [ + for (final root in knownRoots) {ParameterNames.root: root.uri}, + ]; } final outputs = []; for (var rootConfig in rootConfigs) { - final rootUriString = rootConfig['root'] as String?; + final rootUriString = rootConfig[ParameterNames.root] as String?; if (rootUriString == null) { // This shouldn't happen based on the schema, but handle defensively. return CallToolResult( @@ -53,6 +63,19 @@ Future runCommandInRoots( ); } + if (!_isAllowedRoot(rootUriString, knownRoots)) { + return CallToolResult( + content: [ + TextContent( + text: + 'Invalid root $rootUriString, must be under one of the ' + 'registered project roots:\n\n${knownRoots.join('\n')}', + ), + ], + isError: true, + ); + } + final rootUri = Uri.parse(rootUriString); if (rootUri.scheme != 'file') { return CallToolResult( @@ -68,9 +91,28 @@ Future runCommandInRoots( } final projectRoot = Directory(rootUri.toFilePath()); - final commandWithPaths = List.from(command); - final paths = (rootConfig['paths'] as List?)?.cast(); - commandWithPaths.addAll(paths ?? defaultPaths); + final commandWithPaths = List.of(command); + final paths = + (rootConfig[ParameterNames.paths] as List?)?.cast() ?? + defaultPaths; + final invalidPaths = paths.where((path) { + final resolvedPath = rootUri.resolve(path).toString(); + return rootUriString != resolvedPath && + !p.isWithin(rootUriString, resolvedPath); + }); + if (invalidPaths.isNotEmpty) { + return CallToolResult( + content: [ + TextContent( + text: + 'Paths are not allowed to escape their project root:\n' + '${invalidPaths.join('\n')}', + ), + ], + isError: true, + ); + } + commandWithPaths.addAll(paths); final result = await processManager.run( commandWithPaths, @@ -102,3 +144,35 @@ Future runCommandInRoots( } return CallToolResult(content: outputs); } + +/// Returns whether or not [rootUri] is an allowed root, either exactly matching +/// or under on of the [knownRoots]. +bool _isAllowedRoot(String rootUri, List knownRoots) => + knownRoots.any((knownRoot) { + final knownRootUri = Uri.parse(knownRoot.uri); + final resolvedRoot = knownRootUri.resolve(rootUri).toString(); + return knownRoot.uri == resolvedRoot || + p.isWithin(knownRoot.uri, resolvedRoot); + }); + +/// The schema for the `roots` parameter for any tool that accepts it. +ListSchema rootsSchema({bool supportsPaths = false}) => Schema.list( + title: 'All projects roots to run this tool in.', + items: Schema.object( + properties: { + ParameterNames.root: Schema.string( + title: 'The URI of the project root to run this tool in.', + description: + 'This must be equal to or a subdirectory of one of the roots ' + 'returned by a call to "listRoots".', + ), + if (supportsPaths) + ParameterNames.paths: Schema.list( + title: + 'Paths to run this tool on. Must resolve to a path that is ' + 'within the "root".', + ), + }, + required: [ParameterNames.root], + ), +); diff --git a/pkgs/dart_tooling_mcp_server/lib/src/utils/constants.dart b/pkgs/dart_tooling_mcp_server/lib/src/utils/constants.dart new file mode 100644 index 00000000..acd5a699 --- /dev/null +++ b/pkgs/dart_tooling_mcp_server/lib/src/utils/constants.dart @@ -0,0 +1,12 @@ +// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +/// A namespace for all the parameter names. +extension ParameterNames on Never { + static const command = 'command'; + static const packageName = 'packageName'; + static const paths = 'paths'; + static const root = 'root'; + static const roots = 'roots'; +} diff --git a/pkgs/dart_tooling_mcp_server/test/tools/dart_cli_test.dart b/pkgs/dart_tooling_mcp_server/test/tools/dart_cli_test.dart index 843a0e59..79968764 100644 --- a/pkgs/dart_tooling_mcp_server/test/tools/dart_cli_test.dart +++ b/pkgs/dart_tooling_mcp_server/test/tools/dart_cli_test.dart @@ -4,6 +4,7 @@ import 'package:dart_mcp/server.dart'; import 'package:dart_tooling_mcp_server/src/mixins/dart_cli.dart'; +import 'package:dart_tooling_mcp_server/src/utils/constants.dart'; import 'package:test/test.dart'; import '../test_harness.dart'; @@ -47,14 +48,14 @@ void main() { final request = CallToolRequest( name: dartFixTool.name, arguments: { - 'roots': [ - {'root': testRoot.uri}, + ParameterNames.roots: [ + {ParameterNames.root: testRoot.uri}, ], }, ); final result = await testHarness.callToolWithRetry(request); - // Verify the command was sent to the process maanger without error. + // Verify the command was sent to the process manager without error. expect(result.isError, isNot(true)); expect(testProcessManager.commandsRan, [ ['dart', 'fix', '--apply'], @@ -65,14 +66,14 @@ void main() { final request = CallToolRequest( name: dartFormatTool.name, arguments: { - 'roots': [ - {'root': testRoot.uri}, + ParameterNames.roots: [ + {ParameterNames.root: testRoot.uri}, ], }, ); final result = await testHarness.callToolWithRetry(request); - // Verify the command was sent to the process maanger without error. + // Verify the command was sent to the process manager without error. expect(result.isError, isNot(true)); expect(testProcessManager.commandsRan, [ ['dart', 'format', '.'], @@ -83,17 +84,17 @@ void main() { final request = CallToolRequest( name: dartFormatTool.name, arguments: { - 'roots': [ + ParameterNames.roots: [ { - 'root': testRoot.uri, - 'paths': ['foo.dart', 'bar.dart'], + ParameterNames.root: testRoot.uri, + ParameterNames.paths: ['foo.dart', 'bar.dart'], }, ], }, ); final result = await testHarness.callToolWithRetry(request); - // Verify the command was sent to the process maanger without error. + // Verify the command was sent to the process manager without error. expect(result.isError, isNot(true)); expect(testProcessManager.commandsRan, [ ['dart', 'format', 'foo.dart', 'bar.dart'], diff --git a/pkgs/dart_tooling_mcp_server/test/tools/pub_test.dart b/pkgs/dart_tooling_mcp_server/test/tools/pub_test.dart index 3fa1b962..9cd2ccb0 100644 --- a/pkgs/dart_tooling_mcp_server/test/tools/pub_test.dart +++ b/pkgs/dart_tooling_mcp_server/test/tools/pub_test.dart @@ -4,6 +4,7 @@ import 'package:dart_mcp/server.dart'; import 'package:dart_tooling_mcp_server/src/mixins/pub.dart'; +import 'package:dart_tooling_mcp_server/src/utils/constants.dart'; import 'package:test/test.dart'; import '../test_harness.dart'; @@ -39,10 +40,10 @@ void main() { final request = CallToolRequest( name: dartPubTool.name, arguments: { - 'command': 'add', - 'packageName': 'foo', - 'roots': [ - {'root': testRoot.uri}, + ParameterNames.command: 'add', + ParameterNames.packageName: 'foo', + ParameterNames.roots: [ + {ParameterNames.root: testRoot.uri}, ], }, ); @@ -59,10 +60,10 @@ void main() { final request = CallToolRequest( name: dartPubTool.name, arguments: { - 'command': 'remove', - 'packageName': 'foo', - 'roots': [ - {'root': testRoot.uri}, + ParameterNames.command: 'remove', + ParameterNames.packageName: 'foo', + ParameterNames.roots: [ + {ParameterNames.root: testRoot.uri}, ], }, ); @@ -79,9 +80,9 @@ void main() { final request = CallToolRequest( name: dartPubTool.name, arguments: { - 'command': 'get', - 'roots': [ - {'root': testRoot.uri}, + ParameterNames.command: 'get', + ParameterNames.roots: [ + {ParameterNames.root: testRoot.uri}, ], }, ); @@ -98,9 +99,9 @@ void main() { final request = CallToolRequest( name: dartPubTool.name, arguments: { - 'command': 'upgrade', - 'roots': [ - {'root': testRoot.uri}, + ParameterNames.command: 'upgrade', + ParameterNames.roots: [ + {ParameterNames.root: testRoot.uri}, ], }, ); @@ -131,7 +132,7 @@ void main() { test('for unsupported command', () async { final request = CallToolRequest( name: dartPubTool.name, - arguments: {'command': 'publish'}, + arguments: {ParameterNames.command: 'publish'}, ); final result = await testHarness.callToolWithRetry( request, @@ -151,7 +152,7 @@ void main() { test('for missing package name: $command', () async { final request = CallToolRequest( name: dartPubTool.name, - arguments: {'command': command.name}, + arguments: {ParameterNames.command: command.name}, ); final result = await testHarness.callToolWithRetry( request, @@ -166,23 +167,6 @@ void main() { expect(testProcessManager.commandsRan, isEmpty); }); } - - test('for missing roots', () async { - final request = CallToolRequest( - name: dartPubTool.name, - arguments: {'command': 'get'}, - ); - final result = await testHarness.callToolWithRetry( - request, - expectError: true, - ); - - expect( - (result.content.single as TextContent).text, - 'Missing required argument `roots`.', - ); - expect(testProcessManager.commandsRan, isEmpty); - }); }); }); } diff --git a/pkgs/dart_tooling_mcp_server/test/utils/cli_utils_test.dart b/pkgs/dart_tooling_mcp_server/test/utils/cli_utils_test.dart new file mode 100644 index 00000000..4781f2ad --- /dev/null +++ b/pkgs/dart_tooling_mcp_server/test/utils/cli_utils_test.dart @@ -0,0 +1,140 @@ +// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:dart_mcp/server.dart'; +import 'package:dart_tooling_mcp_server/src/utils/cli_utils.dart'; +import 'package:dart_tooling_mcp_server/src/utils/constants.dart'; +import 'package:process/process.dart'; +import 'package:test/fake.dart'; +import 'package:test/test.dart'; + +import '../test_harness.dart'; + +void main() { + group('can run commands', () { + late TestProcessManager processManager; + setUp(() async { + processManager = TestProcessManager(); + }); + + test('can run commands with exact matches for roots', () async { + final result = await runCommandInRoots( + CallToolRequest( + name: 'foo', + arguments: { + ParameterNames.roots: [ + {ParameterNames.root: 'file:///bar/'}, + ], + }, + ), + command: ['testCommand'], + commandDescription: '', + processManager: processManager, + knownRoots: [Root(uri: 'file:///bar/')], + ); + expect(result.isError, isNot(true)); + expect(processManager.commandsRan, [ + ['testCommand'], + ]); + }); + + test( + 'can run commands with roots that are subdirectories of known roots', + () async { + final result = await runCommandInRoots( + CallToolRequest( + name: 'foo', + arguments: { + ParameterNames.roots: [ + {ParameterNames.root: 'file:///bar/baz/'}, + ], + }, + ), + command: ['testCommand'], + commandDescription: '', + processManager: processManager, + knownRoots: [Root(uri: 'file:///bar/')], + ); + expect(result.isError, isNot(true)); + expect(processManager.commandsRan, [ + ['testCommand'], + ]); + }, + ); + }); + + group('cannot run commands', () { + final processManager = FakeProcessManager(); + + test('with roots outside of known roots', () async { + for (var invalidRoot in ['file:///bar/', 'file:///foo/../bar/']) { + final result = await runCommandInRoots( + CallToolRequest( + name: 'foo', + arguments: { + ParameterNames.roots: [ + {ParameterNames.root: invalidRoot}, + ], + }, + ), + command: ['fake'], + commandDescription: '', + processManager: processManager, + knownRoots: [Root(uri: 'file:///foo/')], + ); + expect(result.isError, isTrue); + expect( + result.content.single, + isA().having( + (t) => t.text, + 'text', + allOf(contains('Invalid root $invalidRoot')), + ), + ); + } + }); + + test('with paths outside of known roots', () async { + final result = await runCommandInRoots( + CallToolRequest( + name: 'foo', + arguments: { + ParameterNames.roots: [ + { + ParameterNames.root: 'file:///foo/', + ParameterNames.paths: [ + 'file:///bar/', + '../baz/', + 'zip/../../zap/', + 'ok.dart', + ], + }, + ], + }, + ), + command: ['fake'], + commandDescription: '', + processManager: processManager, + knownRoots: [Root(uri: 'file:///foo/')], + ); + expect(result.isError, isTrue); + expect( + result.content.single, + isA().having( + (t) => t.text, + 'text', + allOf( + contains('Paths are not allowed to escape their project root'), + contains('bar/'), + contains('baz/'), + contains('zap/'), + isNot(contains('ok.dart')), + ), + ), + ); + }); + }); +} + +class FakeProcessManager extends Fake implements ProcessManager {}