Validate extension names and warn in devtools_app for server-side ext…

…ension errors (#6666)
flutter · Nov 7, 2023 · a935fb3 · a935fb3
1 parent 0d225b2
commit a935fb3
Show file tree

Hide file tree

Showing 16 changed files with 136 additions and 42 deletions.
diff --git a/packages/devtools_app/lib/src/extensions/extension_screen.dart b/packages/devtools_app/lib/src/extensions/extension_screen.dart
@@ -11,7 +11,6 @@ import '../shared/analytics/analytics.dart' as ga;
 import '../shared/analytics/constants.dart' as gac;
 import '../shared/common_widgets.dart';
 import '../shared/globals.dart';
-import '../shared/primitives/utils.dart';
 import '../shared/screen.dart';
 import 'embedded/controller.dart';
 import 'embedded/view.dart';
@@ -22,7 +21,7 @@ class ExtensionScreen extends Screen {
       : super.conditional(
           // TODO(kenz): we may need to ensure this is a unique id.
           id: '${extensionConfig.name}_ext',
-          title: extensionConfig.screenTitle,
+          title: extensionConfig.name,
           icon: extensionConfig.icon,
           // TODO(kenz): support static DevTools extensions.
           requiresConnection: true,
@@ -135,6 +134,4 @@ extension ExtensionConfigExtension on DevToolsExtensionConfig {
         materialIconCodePoint,
         fontFamily: 'MaterialIcons',
       );
-
-  String get screenTitle => name.toSentenceCase();
 }
diff --git a/packages/devtools_app/lib/src/service/vm_service_wrapper.dart b/packages/devtools_app/lib/src/service/vm_service_wrapper.dart
@@ -114,7 +114,7 @@ class VmServiceWrapper extends VmService {
     String isolateId, {
     bool? reset,
     bool? gc,
-  }) async {
+  }) {
     return callMethod(
       // TODO(bkonyi): add _new and _old to public response.
       '_getAllocationProfile',
@@ -131,7 +131,7 @@ class VmServiceWrapper extends VmService {
     String isolateId,
     int timeOriginMicros,
     int timeExtentMicros,
-  ) async {
+  ) {
     return callMethod(
       'getCpuSamples',
       isolateId: isolateId,

diff --git a/...ls_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart b/...ls_app/lib/src/shared/config_specific/framework_initialize/_framework_initialize_web.dart
@@ -86,12 +86,12 @@ class ServerConnectionStorage implements Storage {
   final DevToolsServerConnection connection;
 
   @override
-  Future<String> getValue(String key) async {
+  Future<String> getValue(String key) {
     return connection.getPreferenceValue(key);
   }
 
   @override
-  Future setValue(String key, String value) async {
+  Future<void> setValue(String key, String value) async {
     await connection.setPreferenceValue(key, value);
   }
 }
@@ -103,7 +103,7 @@ class BrowserStorage implements Storage {
   }
 
   @override
-  Future setValue(String key, String value) async {
+  Future<void> setValue(String key, String value) async {
     window.localStorage[key] = value;
   }
 }
diff --git a/packages/devtools_app/lib/src/shared/config_specific/server/_server_stub.dart b/packages/devtools_app/lib/src/shared/config_specific/server/_server_stub.dart
@@ -18,64 +18,64 @@ const unsupportedMessage =
 bool get isDevToolsServerAvailable => false;
 
 // This is used in g3.
-Future<Object?> request(String url) async {
+Future<Object?> request(String url) {
   throw Exception(unsupportedMessage);
 }
 
-Future<bool> isFirstRun() async {
+Future<bool> isFirstRun() {
   throw Exception(unsupportedMessage);
 }
 
-Future<bool> isAnalyticsEnabled() async {
+Future<bool> isAnalyticsEnabled() {
   throw Exception(unsupportedMessage);
 }
 
-Future<bool> setAnalyticsEnabled([bool value = true]) async {
+Future<bool> setAnalyticsEnabled([bool value = true]) {
   throw Exception(unsupportedMessage);
 }
 
-Future<String> flutterGAClientID() async {
+Future<String> flutterGAClientID() {
   throw Exception(unsupportedMessage);
 }
 
-Future<bool> setActiveSurvey(String value) async {
+Future<bool> setActiveSurvey(String value) {
   throw Exception(unsupportedMessage);
 }
 
-Future<bool> surveyActionTaken() async {
+Future<bool> surveyActionTaken() {
   throw Exception(unsupportedMessage);
 }
 
-Future<void> setSurveyActionTaken() async {
+Future<void> setSurveyActionTaken() {
   throw Exception(unsupportedMessage);
 }
 
-Future<int> surveyShownCount() async {
+Future<int> surveyShownCount() {
   throw Exception(unsupportedMessage);
 }
 
-Future<int> incrementSurveyShownCount() async {
+Future<int> incrementSurveyShownCount() {
   throw Exception(unsupportedMessage);
 }
 
-Future<String> getLastShownReleaseNotesVersion() async {
+Future<String> getLastShownReleaseNotesVersion() {
   throw Exception(unsupportedMessage);
 }
 
-Future<String> setLastShownReleaseNotesVersion(String version) async {
+Future<String> setLastShownReleaseNotesVersion(String version) {
   throw Exception(unsupportedMessage);
 }
 
 // currently unused
-Future<void> resetDevToolsFile() async {
+Future<void> resetDevToolsFile() {
   throw Exception(unsupportedMessage);
 }
 
-Future<DevToolsJsonFile?> requestBaseAppSizeFile(String path) async {
+Future<DevToolsJsonFile?> requestBaseAppSizeFile(String path) {
   throw Exception(unsupportedMessage);
 }
 
-Future<DevToolsJsonFile?> requestTestAppSizeFile(String path) async {
+Future<DevToolsJsonFile?> requestTestAppSizeFile(String path) {
   throw Exception(unsupportedMessage);
 }
 

diff --git a/packages/devtools_app/lib/src/shared/config_specific/server/_server_web.dart b/packages/devtools_app/lib/src/shared/config_specific/server/_server_web.dart
@@ -360,6 +360,13 @@ Future<List<DevToolsExtensionConfig>> refreshAvailableExtensions(
                   as List<Object?>)
               .whereNotNull()
               .cast<Map<String, Object?>>();
+
+      final warningMessage =
+          parsedResult[ExtensionsApi.extensionsResultWarningPropertyName];
+      if (warningMessage != null) {
+        _log.warning(warningMessage);
+      }
+
       return extensionsAsJson
           .map((p) => DevToolsExtensionConfig.parse(p))
           .toList();

diff --git a/packages/devtools_app/lib/src/shared/primitives/storage.dart b/packages/devtools_app/lib/src/shared/primitives/storage.dart
@@ -11,5 +11,5 @@ abstract class Storage {
   Future<String?> getValue(String key);
 
   /// Set a value for the given key.
-  Future setValue(String key, String value);
+  Future<void> setValue(String key, String value);
 }
diff --git a/packages/devtools_app/test/extensions/extension_analytics_test.dart b/packages/devtools_app/test/extensions/extension_analytics_test.dart
@@ -53,7 +53,7 @@ void main() {
       () {
         final private = fooExtension;
         expect(private.isPubliclyHosted, false);
-        expect(private.name, 'Foo');
+        expect(private.name, 'foo');
         expect(private.analyticsSafeName, 'private');
         expect(
           DevToolsExtensionEvents.extensionScreenName(private),

diff --git a/packages/devtools_app/test/extensions/extension_screen_test.dart b/packages/devtools_app/test/extensions/extension_screen_test.dart
@@ -38,15 +38,15 @@ void main() {
 
     testWidgets('builds its tab', (WidgetTester tester) async {
       await tester.pumpWidget(wrap(Builder(builder: fooScreen.buildTab)));
-      expect(find.text('Foo'), findsOneWidget);
+      expect(find.text('foo'), findsOneWidget);
       expect(find.byIcon(fooExtension.icon), findsOneWidget);
 
       await tester.pumpWidget(wrap(Builder(builder: barScreen.buildTab)));
-      expect(find.text('Bar'), findsOneWidget);
+      expect(find.text('bar'), findsOneWidget);
       expect(find.byIcon(barExtension.icon), findsOneWidget);
 
       await tester.pumpWidget(wrap(Builder(builder: providerScreen.buildTab)));
-      expect(find.text('Provider'), findsOneWidget);
+      expect(find.text('provider'), findsOneWidget);
       expect(find.byIcon(providerExtension.icon), findsOneWidget);
     });
 

diff --git a/packages/devtools_app/test/test_infra/test_data/dart_tooling_api/mock_api.dart b/packages/devtools_app/test/test_infra/test_data/dart_tooling_api/mock_api.dart
@@ -136,7 +136,7 @@ class MockDartToolingApi extends DartToolingApiImpl {
   }
 
   /// Simulates executing a VS Code command requested by the embedded panel.
-  Future<Object?> executeCommand(json_rpc_2.Parameters parameters) async {
+  Future<Object?> executeCommand(json_rpc_2.Parameters parameters) {
     final params = parameters.asMap;
     final command = params['command'];
     switch (command) {

diff --git a/packages/devtools_app/test/test_infra/test_data/extensions.dart b/packages/devtools_app/test/test_infra/test_data/extensions.dart
@@ -7,7 +7,7 @@ import 'package:devtools_shared/src/extensions/extension_model.dart';
 final testExtensions = [fooExtension, barExtension, providerExtension];
 
 final fooExtension = DevToolsExtensionConfig.parse({
-  DevToolsExtensionConfig.nameKey: 'Foo',
+  DevToolsExtensionConfig.nameKey: 'foo',
   DevToolsExtensionConfig.issueTrackerKey: 'www.google.com',
   DevToolsExtensionConfig.versionKey: '1.0.0',
   DevToolsExtensionConfig.pathKey: '/path/to/foo',

diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md
@@ -1,5 +1,9 @@
 # 6.0.1
 * Add field `isPublic` to `DevToolsExtensionConfig`.
+* Add validation for `DevToolsExtensionConfig.name` field to ensure it is a valid
+Dart package name.
+* Pass warnings and errors for DevTools extension APIs from the DevTools server to
+DevTools app.
 
 # 6.0.0
 * Bump `package:vm_service` dependency to ^13.0.0.

diff --git a/packages/devtools_shared/lib/src/devtools_api.dart b/packages/devtools_shared/lib/src/devtools_api.dart
@@ -77,6 +77,11 @@ abstract class ExtensionsApi {
   /// receiving a [apiServeAvailableExtensions] request.
   static const extensionsResultPropertyName = 'extensions';
 
+  /// The property name for an optional warning message field in the response
+  /// that the server sends back upon receiving a [apiServeAvailableExtensions]
+  /// request.
+  static const extensionsResultWarningPropertyName = 'warning';
+
   /// Returns and optionally sets the enabled state for a DevTools extension.
   static const apiExtensionEnabledState = '${apiPrefix}extensionEnabledState';
 

diff --git a/packages/devtools_shared/lib/src/extensions/extension_manager.dart b/packages/devtools_shared/lib/src/extensions/extension_manager.dart
@@ -15,10 +15,6 @@ import 'extension_model.dart';
 /// DevTools assets are served (build/).
 const extensionRequestPath = 'devtools_extensions';
 
-/// The location for the DevTools extension, relative to the parent package's
-/// root.
-const extensionLocation = 'extension/devtools';
-
 /// The default location for the DevTools extension, relative to
 /// `<parent_package_root>/extension/devtools/`.
 const extensionBuildDefault = 'build';
@@ -58,6 +54,7 @@ class ExtensionsManager {
   /// DevTools server is serving.
   Future<void> serveAvailableExtensions(String? rootPath) async {
     devtoolsExtensions.clear();
+    final parsingErrors = StringBuffer();
 
     if (rootPath != null) {
       late final List<Extension> extensions;
@@ -73,9 +70,10 @@ class ExtensionsManager {
           ),
         );
       } catch (e) {
-        print('[ERROR] `findExtensions` failed: $e');
         extensions = <Extension>[];
+        rethrow;
       }
+
       for (final extension in extensions) {
         final config = extension.config;
         // TODO(https://github.com/dart-lang/pub/issues/4042): make this check
@@ -91,7 +89,8 @@ class ExtensionsManager {
 
         final location = path.join(
           extension.rootUri.toFilePath(),
-          extensionLocation,
+          'extension',
+          'devtools',
           relativeExtensionLocation,
         );
 
@@ -103,7 +102,7 @@ class ExtensionsManager {
           });
           devtoolsExtensions.add(extensionConfig);
         } on StateError catch (e) {
-          print(e.message);
+          parsingErrors.writeln(e.message);
           continue;
         }
       }
@@ -114,6 +113,13 @@ class ExtensionsManager {
       for (final extension in devtoolsExtensions)
         _moveToServedExtensionsDir(extension.name, extension.path),
     ]);
+
+    if (parsingErrors.isNotEmpty) {
+      throw ExtensionParsingException(
+        'Encountered errors while parsing extension config.yaml '
+        'files:\n$parsingErrors',
+      );
+    }
   }
 
   void _resetServedPluginsDir() {
@@ -174,3 +180,9 @@ Future<void> copyPath(String from, String to) async {
     }
   }
 }
+
+/// Exception type for errors encountered while parsing DevTools extension
+/// config.yaml files.
+class ExtensionParsingException extends FormatException {
+  const ExtensionParsingException(super.message);
+}
diff --git a/packages/devtools_shared/lib/src/extensions/extension_model.dart b/packages/devtools_shared/lib/src/extensions/extension_model.dart
@@ -36,6 +36,15 @@ class DevToolsExtensionConfig implements Comparable {
           versionKey: final String version,
           isPubliclyHostedKey: final String isPubliclyHosted,
         }) {
+      final underscoresAndLetters = RegExp(r'^[a-z0-9_]*$');
+      if (!underscoresAndLetters.hasMatch(name)) {
+        throw StateError(
+          'The "name" field in the extension config.yaml should only contain '
+          'lowercase letters, numbers, and underscores but instead was '
+          '"$name". This should be a valid Dart package name that matches the '
+          'package name this extension belongs to.',
+        );
+      }
       return DevToolsExtensionConfig._(
         name: name,
         path: path,

diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart
@@ -351,12 +351,21 @@ abstract class _ExtensionsApiHandler {
 
     final rootPath = queryParams[ExtensionsApi.extensionRootPathPropertyName];
 
-    await extensionsManager.serveAvailableExtensions(rootPath);
+    final result = <String, Object?>{};
+    try {
+      await extensionsManager.serveAvailableExtensions(rootPath);
+    } on ExtensionParsingException catch (e) {
+      // For [ExtensionParsingException]s, we should return a success response
+      // with a warning message.
+      result[ExtensionsApi.extensionsResultWarningPropertyName] = e.message;
+    } catch (e) {
+      // For all other exceptions, return an error response.
+      return api.serverError('$e');
+    }
+
     final extensions =
         extensionsManager.devtoolsExtensions.map((p) => p.toJson()).toList();
-    final result = {
-      ExtensionsApi.extensionsResultPropertyName: extensions,
-    };
+    result[ExtensionsApi.extensionsResultPropertyName] = extensions;
     return ServerApi._encodeResponse(result, api: api);
   }