diff --git a/pkg/dds/CHANGELOG.md b/pkg/dds/CHANGELOG.md index 9c2353ee128..0335b394000 100644 --- a/pkg/dds/CHANGELOG.md +++ b/pkg/dds/CHANGELOG.md @@ -1,3 +1,6 @@ +# 2.5.1 +- [DAP] Supplying incorrect types of arguments in `launch`/`attach` requests will now result in a clear error message in an error response instead of terminating the adapter. + # 2.5.0 - [DAP] `variables` requests now treat lists from `dart:typed_data` (such as `Uint8List`) like standard `List` instances and return their elements instead of class fields. - [DAP] `variables` requests now return information about the number of items in lists to allow the client to page through them. diff --git a/pkg/dds/lib/src/dap/adapters/dart.dart b/pkg/dds/lib/src/dap/adapters/dart.dart index 7cc20b487ed..985f0891ee5 100644 --- a/pkg/dds/lib/src/dap/adapters/dart.dart +++ b/pkg/dds/lib/src/dap/adapters/dart.dart @@ -102,6 +102,10 @@ class DartAttachRequestArguments extends DartCommonLaunchAttachRequestArguments /// Either this or [vmServiceUri] must be supplied. final String? vmServiceInfoFile; + /// A reader for protocol arguments that throws detailed exceptions if + /// arguments aren't of the correct type. + static final arg = DebugAdapterArgumentReader('attach'); + DartAttachRequestArguments({ this.vmServiceUri, this.vmServiceInfoFile, @@ -131,8 +135,8 @@ class DartAttachRequestArguments extends DartCommonLaunchAttachRequestArguments ); DartAttachRequestArguments.fromMap(Map obj) - : vmServiceUri = obj['vmServiceUri'] as String?, - vmServiceInfoFile = obj['vmServiceInfoFile'] as String?, + : vmServiceUri = arg.read(obj, 'vmServiceUri'), + vmServiceInfoFile = arg.read(obj, 'vmServiceInfoFile'), super.fromMap(obj); @override @@ -149,6 +153,10 @@ class DartAttachRequestArguments extends DartCommonLaunchAttachRequestArguments /// A common base for [DartLaunchRequestArguments] and /// [DartAttachRequestArguments] for fields that are common to both. class DartCommonLaunchAttachRequestArguments extends RequestArguments { + /// A reader for protocol arguments that throws detailed exceptions if + /// arguments aren't of the correct type. + static final arg = DebugAdapterArgumentReader('launch/attach'); + /// Optional data from the previous, restarted session. /// The data is sent as the 'restart' attribute of the 'terminated' event. /// The client should leave the data intact. @@ -226,21 +234,22 @@ class DartCommonLaunchAttachRequestArguments extends RequestArguments { }); DartCommonLaunchAttachRequestArguments.fromMap(Map obj) - : restart = obj['restart'], - name = obj['name'] as String?, - cwd = obj['cwd'] as String?, - env = (obj['env'] as Map?)?.cast(), + : restart = arg.read(obj, 'restart'), + name = arg.read(obj, 'name'), + cwd = arg.read(obj, 'cwd'), + env = arg.readOptionalMap(obj, 'env'), additionalProjectPaths = - (obj['additionalProjectPaths'] as List?)?.cast(), - debugSdkLibraries = obj['debugSdkLibraries'] as bool?, + arg.readOptionalList(obj, 'additionalProjectPaths'), + debugSdkLibraries = arg.read(obj, 'debugSdkLibraries'), debugExternalPackageLibraries = - obj['debugExternalPackageLibraries'] as bool?, + arg.read(obj, 'debugExternalPackageLibraries'), evaluateGettersInDebugViews = - obj['evaluateGettersInDebugViews'] as bool?, + arg.read(obj, 'evaluateGettersInDebugViews'), evaluateToStringInDebugViews = - obj['evaluateToStringInDebugViews'] as bool?, - sendLogsToClient = obj['sendLogsToClient'] as bool?, - sendCustomProgressEvents = obj['sendCustomProgressEvents'] as bool?; + arg.read(obj, 'evaluateToStringInDebugViews'), + sendLogsToClient = arg.read(obj, 'sendLogsToClient'), + sendCustomProgressEvents = + arg.read(obj, 'sendCustomProgressEvents'); Map toJson() => { if (restart != null) 'restart': restart, @@ -2213,6 +2222,10 @@ abstract class DartDebugAdapter obj) - : noDebug = obj['noDebug'] as bool?, - program = obj['program'] as String, - args = (obj['args'] as List?)?.cast(), - toolArgs = (obj['toolArgs'] as List?)?.cast(), - vmAdditionalArgs = (obj['vmAdditionalArgs'] as List?)?.cast(), - vmServicePort = obj['vmServicePort'] as int?, - console = obj['console'] as String?, - customTool = obj['customTool'] as String?, - customToolReplacesArgs = obj['customToolReplacesArgs'] as int?, + : noDebug = arg.read(obj, 'noDebug'), + program = arg.read(obj, 'program'), + args = arg.readOptionalList(obj, 'args'), + toolArgs = arg.readOptionalList(obj, 'toolArgs'), + vmAdditionalArgs = + arg.readOptionalList(obj, 'vmAdditionalArgs'), + vmServicePort = arg.read(obj, 'vmServicePort'), + console = arg.read(obj, 'console'), + customTool = arg.read(obj, 'customTool'), + customToolReplacesArgs = arg.read(obj, 'customToolReplacesArgs'), super.fromMap(obj); @override diff --git a/pkg/dds/lib/src/dap/base_debug_adapter.dart b/pkg/dds/lib/src/dap/base_debug_adapter.dart index bd67bb61bb4..32f1e7286b4 100644 --- a/pkg/dds/lib/src/dap/base_debug_adapter.dart +++ b/pkg/dds/lib/src/dap/base_debug_adapter.dart @@ -108,31 +108,31 @@ abstract class BaseDebugAdapter handler, TArg Function(Map) fromJson, ) async { - final args = request.arguments != null - ? fromJson(request.arguments as Map) - // arguments are only valid to be null then TArg is nullable. - : null as TArg; - - // Because handlers may need to send responses before they have finished - // executing (for example, initializeRequest needs to send its response - // before sending InitializedEvent()), we pass in a function `sendResponse` - // rather than using a return value. - var sendResponseCalled = false; - void sendResponse(TResp responseBody) { - assert(!sendResponseCalled, - 'sendResponse was called multiple times by ${request.command}'); - sendResponseCalled = true; - final response = Response( - success: true, - requestSeq: request.seq, - seq: _sequence++, - command: request.command, - body: responseBody, - ); - _channel.sendResponse(response); - } - try { + final args = request.arguments != null + ? fromJson(request.arguments as Map) + // arguments are only valid to be null then TArg is nullable. + : null as TArg; + + // Because handlers may need to send responses before they have finished + // executing (for example, initializeRequest needs to send its response + // before sending InitializedEvent()), we pass in a function `sendResponse` + // rather than using a return value. + var sendResponseCalled = false; + void sendResponse(TResp responseBody) { + assert(!sendResponseCalled, + 'sendResponse was called multiple times by ${request.command}'); + sendResponseCalled = true; + final response = Response( + success: true, + requestSeq: request.seq, + seq: _sequence++, + command: request.command, + body: responseBody, + ); + _channel.sendResponse(response); + } + await handler(request, args, sendResponse); assert(sendResponseCalled, 'sendResponse was not called in ${request.command}'); diff --git a/pkg/dds/lib/src/dap/exceptions.dart b/pkg/dds/lib/src/dap/exceptions.dart index ec05ca5f537..81431dbc988 100644 --- a/pkg/dds/lib/src/dap/exceptions.dart +++ b/pkg/dds/lib/src/dap/exceptions.dart @@ -12,3 +12,42 @@ class DebugAdapterException implements Exception { String toString() => 'DebugAdapterException: $message'; } + +/// Exception thrown when failing to read arguments supplied by the user because +/// they are not the correct type. +/// +/// This is usually because a user customised their launch configuration (for +/// example in `.vscode/launch.json` for VS Code) with values that are not +/// valid, such as putting a `String` in a field intended to be a `Map`: +/// +/// ``` +/// // Bad. +/// "env": "foo" +/// +/// // Good. +/// "env": { +/// "FLUTTER_ROOT": "foo", +/// } +/// ``` +class DebugAdapterInvalidArgumentException implements DebugAdapterException { + final String requestName; + final String argumentName; + final Type expectedType; + final Type actualType; + final Object? actualValue; + + DebugAdapterInvalidArgumentException({ + required this.requestName, + required this.argumentName, + required this.expectedType, + required this.actualType, + required this.actualValue, + }); + + @override + String get message => + '"$argumentName" argument in $requestName configuration must be a ' + '$expectedType but provided value was a $actualType ($actualValue)'; + + String toString() => 'DebugAdapterInvalidArgumentException: $message'; +} diff --git a/pkg/dds/lib/src/dap/protocol_common.dart b/pkg/dds/lib/src/dap/protocol_common.dart index de38f0dc8a1..d06266d65f5 100644 --- a/pkg/dds/lib/src/dap/protocol_common.dart +++ b/pkg/dds/lib/src/dap/protocol_common.dart @@ -4,6 +4,8 @@ import 'dart:convert'; +import 'exceptions.dart'; + /// A base class for (spec-generated) classes that represent the `body` of a an /// event. abstract class EventBody { @@ -49,3 +51,81 @@ class RawRequestArguments extends RequestArguments { abstract class RequestArguments { static bool canParse(Object? obj) => obj is Map?; } + +/// A helper for reading arguments for DAP requests from the client. +class DebugAdapterArgumentReader { + final String request; + + DebugAdapterArgumentReader(this.request); + + /// Reads a value of type [T] from [field] in [obj]. + T read( + Map obj, + String field, + ) { + final value = obj[field]; + if (value is! T) { + throw DebugAdapterInvalidArgumentException( + requestName: request, + argumentName: field, + expectedType: T, + actualType: value.runtimeType, + actualValue: value, + ); + } + return obj[field] as T; + } + + /// Reads a List of values of type [T] from [field] in [obj]. + List readList( + Map obj, + String field, + ) { + final value = obj[field]; + if (value is! List || !value.every((element) => element is T)) { + throw DebugAdapterInvalidArgumentException( + requestName: request, + argumentName: field, + expectedType: List, + actualType: value.runtimeType, + actualValue: value, + ); + } + return (obj[field] as List).cast(); + } + + /// Reads an optional List of values of type [T] from [field] in [obj]. + List? readOptionalList( + Map obj, + String field, + ) { + return obj.containsKey(field) ? readList(obj, field) : null; + } + + /// Reads an optional Map of types [K],[V] from [field] in [obj]. + Map? readOptionalMap( + Map obj, + String field, + ) { + return obj.containsKey(field) ? readMap(obj, field) : null; + } + + /// Reads a Map of types [K],[V] from [field] in [obj]. + Map readMap( + Map obj, + String field, + ) { + final value = obj[field]; + if (value is! Map || + !value.entries.every((entry) => entry.key is K && entry.value is V)) { + throw DebugAdapterInvalidArgumentException( + requestName: request, + argumentName: field, + expectedType: Map, + actualType: value.runtimeType, + actualValue: value, + ); + } + return (obj[field] as Map).cast(); + } +} diff --git a/pkg/dds/pubspec.yaml b/pkg/dds/pubspec.yaml index f3f2b4197e1..c101786691b 100644 --- a/pkg/dds/pubspec.yaml +++ b/pkg/dds/pubspec.yaml @@ -1,5 +1,5 @@ name: dds -version: 2.5.0 +version: 2.5.1 description: >- A library used to spawn the Dart Developer Service, used to communicate with a Dart VM Service instance. diff --git a/pkg/dds/test/dap/integration/args_test.dart b/pkg/dds/test/dap/integration/args_test.dart new file mode 100644 index 00000000000..d06688ae120 --- /dev/null +++ b/pkg/dds/test/dap/integration/args_test.dart @@ -0,0 +1,220 @@ +// Copyright (c) 2022, 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:test/test.dart'; + +import 'test_support.dart'; + +main() { + group('validates arguments', () { + group('for Dart CLI adapter', () { + late DapTestSession dap; + setUp(() async { + dap = await DapTestSession.setUp(); + }); + tearDown(() => dap.tearDown()); + + String errorMessage( + String field, + String request, + String expectedType, + Object? actual, + ) => + '"$field" argument in $request configuration must be a $expectedType ' + 'but provided value was a ${actual.runtimeType} ($actual)'; + + group('for launchRequest', () { + Future expectError( + Map args, + String expectedError, + ) async { + final response = await dap.client.sendRequest( + args, + overrideCommand: 'launch', + allowFailure: true, + ); + + expect(response.success, isFalse); + expect(response.message, expectedError); + } + + test('when a non-String is supplied for a String', () async { + await expectError( + { + 'program': true, + }, + errorMessage('program', 'launch', 'String', true), + ); + }); + + test('when a non-Map is supplied for a Map', () async { + await expectError( + { + 'program': '', + 'env': 'test', + }, + errorMessage('env', 'launch/attach', 'Map', 'test'), + ); + }); + + test('when an invalid type is supplied for a Map value', () async { + await expectError( + { + 'program': '', + 'env': {'FOO': true} + }, + errorMessage('env', 'launch/attach', 'Map', + {'FOO': true}), + ); + }); + }); + + group('for attachRequest', () { + Future expectError( + Map args, + String expectedError, + ) async { + final response = await dap.client.sendRequest( + args, + overrideCommand: 'attach', + allowFailure: true, + ); + + expect(response.success, isFalse); + expect(response.message, expectedError); + } + + test('when a non-String is supplied for a String?', () async { + await expectError( + { + 'vmServiceUri': true, + }, + errorMessage('vmServiceUri', 'attach', 'String?', true), + ); + }); + + test('when a non-Map is supplied for a Map', () async { + await expectError( + { + 'program': '', + 'env': 'test', + }, + errorMessage('env', 'launch/attach', 'Map', 'test'), + ); + }); + + test('when an invalid type is supplied for a Map value', () async { + await expectError( + { + 'program': '', + 'env': {'FOO': true} + }, + errorMessage('env', 'launch/attach', 'Map', + {'FOO': true}), + ); + }); + }); + }); + + group('for Dart Test adapter', () { + late DapTestSession dap; + setUp(() async { + dap = await DapTestSession.setUp(additionalArgs: ['--test']); + }); + tearDown(() => dap.tearDown()); + + group('for launchRequest', () { + Future expectError( + Map args, + String expectedError, + ) async { + final response = await dap.client.sendRequest( + args, + overrideCommand: 'launch', + allowFailure: true, + ); + + expect(response.success, isFalse); + expect(response.message, expectedError); + } + + test('when a non-String is supplied for a String', () async { + await expectError( + { + 'program': true, + }, + '"program" argument in launch configuration must be a String but provided value was a bool (true)', + ); + }); + + test('when a non-Map is supplied for a Map', () async { + await expectError( + { + 'program': '', + 'env': 'test', + }, + '"env" argument in launch/attach configuration must be a Map but provided value was a String (test)', + ); + }); + + test('when an invalid type is supplied for a Map value', () async { + await expectError( + { + 'program': '', + 'env': {'FOO': true} + }, + '"env" argument in launch/attach configuration must be a Map but provided value was a _Map ({FOO: true})', + ); + }); + }); + + group('for attachRequest', () { + Future expectError( + Map args, + String expectedError, + ) async { + final response = await dap.client.sendRequest( + args, + overrideCommand: 'attach', + allowFailure: true, + ); + + expect(response.success, isFalse); + expect(response.message, expectedError); + } + + test('when a non-String is supplied for a String?', () async { + await expectError( + { + 'vmServiceUri': true, + }, + '"vmServiceUri" argument in attach configuration must be a String? but provided value was a bool (true)', + ); + }); + + test('when a non-Map is supplied for a Map', () async { + await expectError( + { + 'program': '', + 'env': 'test', + }, + '"env" argument in launch/attach configuration must be a Map but provided value was a String (test)', + ); + }); + + test('when an invalid type is supplied for a Map value', () async { + await expectError( + { + 'program': '', + 'env': {'FOO': true} + }, + '"env" argument in launch/attach configuration must be a Map but provided value was a _Map ({FOO: true})', + ); + }); + }); + }); + + // These tests can be slow due to starting up the external server process. + }, timeout: Timeout.none); +}