Make sure that extensions are bundled with the package they claim to be. (#9981) * Make sure that extensions are bundled with the package they claim to be. With this change, extensions are enabled if and only if the package name on disk matches the package name in extension/devtools/config.yaml. * Check string type in if expression * Normalize package root * Fix typo * Revert missing line in CHANGELOG.md * Fix presubmits * Use the package name to verify extension permissions and deduplication * Use non-nullable parameters Make extensionPackage a required non-nullable parameter in extensionEnabledState Make packageName non-nullable parameter in TestPackageWithExtension * fix PR link * Add pubspec verification to CHANGELOG * Fix doc comment in lookupExtensionEnabledState and setExtensionEnabledState * Format * Move packageName to correct section in constructor
diff --git a/packages/devtools_app/lib/src/extensions/extension_screen.dart b/packages/devtools_app/lib/src/extensions/extension_screen.dart index 7102119..3d0bb61 100644 --- a/packages/devtools_app/lib/src/extensions/extension_screen.dart +++ b/packages/devtools_app/lib/src/extensions/extension_screen.dart
@@ -112,7 +112,9 @@ const SizedBox(height: intermediateSpacing), Expanded( child: ValueListenableBuilder<ExtensionEnabledState>( - valueListenable: extensionService.enabledStateListenable(ext.name), + valueListenable: extensionService.enabledStateListenable( + ext.packageName, + ), builder: (context, activationState, _) { if (activationState == ExtensionEnabledState.enabled) { return KeepAliveWrapper(
diff --git a/packages/devtools_app/lib/src/extensions/extension_screen_controls.dart b/packages/devtools_app/lib/src/extensions/extension_screen_controls.dart index 5635266..813e183 100644 --- a/packages/devtools_app/lib/src/extensions/extension_screen_controls.dart +++ b/packages/devtools_app/lib/src/extensions/extension_screen_controls.dart
@@ -29,7 +29,7 @@ @override Widget build(BuildContext context) { final theme = Theme.of(context); - final extensionName = ext.displayName; + final extensionPackage = ext.packageName; return SizedBox( width: double.infinity, child: Wrap( @@ -40,7 +40,7 @@ padding: const EdgeInsets.only(left: borderPadding), child: RichText( text: TextSpan( - text: 'package:$extensionName extension', + text: 'package:$extensionPackage extension', style: theme.regularTextStyle.copyWith( fontWeight: FontWeight.bold, ), @@ -96,7 +96,7 @@ @override Widget build(BuildContext context) { return ValueListenableBuilder<ExtensionEnabledState>( - valueListenable: extensionService.enabledStateListenable(ext.displayName), + valueListenable: extensionService.enabledStateListenable(ext.packageName), builder: (context, activationState, _) { if (activationState != ExtensionEnabledState.enabled) { return const SizedBox.shrink(); @@ -168,7 +168,10 @@ text: 'Are you sure you want to disable the ', style: theme.regularTextStyle, children: [ - TextSpan(text: ext.displayName, style: theme.fixedFontStyle), + TextSpan( + text: 'package:${ext.packageName}', + style: theme.fixedFontStyle, + ), const TextSpan(text: ' extension?'), ], ), @@ -233,7 +236,10 @@ text: 'The ', style: theme.regularTextStyle, children: [ - TextSpan(text: ext.name, style: theme.fixedFontStyle), + TextSpan( + text: 'package:${ext.packageName}', + style: theme.fixedFontStyle, + ), const TextSpan( text: ' extension has not been enabled. Do you want to enable'
diff --git a/packages/devtools_app/lib/src/extensions/extension_service.dart b/packages/devtools_app/lib/src/extensions/extension_service.dart index ae49a3b..2ba343c 100644 --- a/packages/devtools_app/lib/src/extensions/extension_service.dart +++ b/packages/devtools_app/lib/src/extensions/extension_service.dart
@@ -111,12 +111,12 @@ final _ignoredStaticExtensionsByHashCode = <int>{}; /// Returns the [ValueListenable] that stores the [ExtensionEnabledState] for - /// the DevTools Extension with [extensionName]. + /// the DevTools Extension provided by [extensionPackageName]. ValueListenable<ExtensionEnabledState> enabledStateListenable( - String extensionName, + String extensionPackageName, ) { return _extensionEnabledStates.putIfAbsent( - extensionName.toLowerCase(), + extensionPackageName.toLowerCase(), () => ValueNotifier<ExtensionEnabledState>(ExtensionEnabledState.none), ); } @@ -232,7 +232,7 @@ // not always be true for extensions that are not published on pub or // extensions that do not follow best practices for naming. final isRuntimeDuplicate = runtimeExtensions.any( - (ext) => ext.name == staticExtension.name, + (ext) => ext.packageName == staticExtension.packageName, ); if (isRuntimeDuplicate) { _log.fine( @@ -256,9 +256,10 @@ final stateFromOptionsFile = await server.extensionEnabledState( devtoolsOptionsFileUri: extension.devtoolsOptionsUri, extensionName: extension.name, + extensionPackage: extension.packageName, ); final stateNotifier = _extensionEnabledStates.putIfAbsent( - extension.name, + extension.packageName.toLowerCase(), () => ValueNotifier<ExtensionEnabledState>(stateFromOptionsFile), ); stateNotifier.value = stateFromOptionsFile; @@ -295,12 +296,13 @@ final allMatchingExtensions = [ ...runtimeExtensions, ...staticExtensions, - ].where((e) => e.name == extension.name); + ].where((e) => e.packageName == extension.packageName); await [ for (final ext in allMatchingExtensions) server.extensionEnabledState( devtoolsOptionsFileUri: ext.devtoolsOptionsUri, extensionName: ext.name, + extensionPackage: ext.packageName, enable: enable, ), ].wait;
diff --git a/packages/devtools_app/lib/src/extensions/extension_service_helpers.dart b/packages/devtools_app/lib/src/extensions/extension_service_helpers.dart index 1e5f543..3133ac7 100644 --- a/packages/devtools_app/lib/src/extensions/extension_service_helpers.dart +++ b/packages/devtools_app/lib/src/extensions/extension_service_helpers.dart
@@ -19,14 +19,17 @@ }) { final deduped = <String>{}; for (final ext in extensions) { - if (deduped.contains(ext.name)) continue; - deduped.add(ext.name); + final dedupeKey = ext.packageName; + if (deduped.contains(dedupeKey)) continue; + deduped.add(dedupeKey); // This includes [ext] itself. - final matchingExtensions = extensions.where((e) => e.name == ext.name); + final matchingExtensions = extensions.where( + (e) => e.packageName == ext.packageName, + ); if (matchingExtensions.length > 1) { logger?.fine( - 'detected duplicate $extensionType extensions for ${ext.name}', + 'detected duplicate $extensionType extensions for package:${ext.packageName}', ); // Ignore all matching extensions and then mark the [latest] as @@ -45,7 +48,7 @@ ); } else { logger?.fine( - 'no duplicates found for $extensionType extension ${ext.name}', + 'no duplicates found for $extensionType extension package:${ext.packageName}', ); } }
diff --git a/packages/devtools_app/lib/src/extensions/extension_settings.dart b/packages/devtools_app/lib/src/extensions/extension_settings.dart index 4ebfdfa..fa2dd8a 100644 --- a/packages/devtools_app/lib/src/extensions/extension_settings.dart +++ b/packages/devtools_app/lib/src/extensions/extension_settings.dart
@@ -172,9 +172,9 @@ ), ]; final theme = Theme.of(context); - final extensionName = extension.name.toLowerCase(); + final packageName = extension.packageName.toLowerCase(); return ValueListenableBuilder( - valueListenable: extensionService.enabledStateListenable(extensionName), + valueListenable: extensionService.enabledStateListenable(packageName), builder: (context, enabledState, _) { return Padding( padding: const EdgeInsets.only(bottom: denseSpacing), @@ -182,7 +182,7 @@ mainAxisAlignment: MainAxisAlignment.spaceBetween, children: [ Text( - 'package:$extensionName', + 'package:$packageName', overflow: TextOverflow.ellipsis, style: theme.fixedFontStyle, ),
diff --git a/packages/devtools_app/lib/src/shared/development_helpers.dart b/packages/devtools_app/lib/src/shared/development_helpers.dart index 072e804..e514582 100644 --- a/packages/devtools_app/lib/src/shared/development_helpers.dart +++ b/packages/devtools_app/lib/src/shared/development_helpers.dart
@@ -91,6 +91,7 @@ /// connected app. static final fooExtension = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'foo', + DevToolsExtensionConfig.packageNameKey: 'foo', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: '1.0.0', DevToolsExtensionConfig.materialIconCodePointKey: '0xe0b1', @@ -105,6 +106,7 @@ /// connected app. static final providerExtension = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'provider', + DevToolsExtensionConfig.packageNameKey: 'provider', DevToolsExtensionConfig.issueTrackerKey: 'https://github.com/rrousselGit/provider/issues', DevToolsExtensionConfig.versionKey: '3.0.0', @@ -121,6 +123,7 @@ /// not require a connected app. static final someToolExtension = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'some_tool', + DevToolsExtensionConfig.packageNameKey: 'some_tool', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: '1.0.0', DevToolsExtensionConfig.materialIconCodePointKey: '0xe00c', @@ -137,6 +140,7 @@ /// require a connected app. static final barExtension = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'bar', + DevToolsExtensionConfig.packageNameKey: 'bar', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: '2.0.0', DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, @@ -153,6 +157,7 @@ /// extension. static final newerBarExtension = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'bar', + DevToolsExtensionConfig.packageNameKey: 'bar', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: '2.1.0', // Newer version. DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, @@ -168,6 +173,7 @@ /// connected app. static final bazExtension = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'baz', + DevToolsExtensionConfig.packageNameKey: 'baz', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: '1.0.0', DevToolsExtensionConfig.materialIconCodePointKey: 0xe716, @@ -182,6 +188,7 @@ /// of a runtime extension [fooExtension], which requires a connected app. static final duplicateFooExtension = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'foo', + DevToolsExtensionConfig.packageNameKey: 'foo', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: '1.0.0', DevToolsExtensionConfig.materialIconCodePointKey: '0xe0b1',
diff --git a/packages/devtools_app/lib/src/shared/server/_extensions_api.dart b/packages/devtools_app/lib/src/shared/server/_extensions_api.dart index 5933894..290abea 100644 --- a/packages/devtools_app/lib/src/shared/server/_extensions_api.dart +++ b/packages/devtools_app/lib/src/shared/server/_extensions_api.dart
@@ -74,11 +74,12 @@ Future<ExtensionEnabledState> extensionEnabledState({ required String devtoolsOptionsFileUri, required String extensionName, + required String extensionPackage, bool? enable, }) async { _log.fine( '${enable != null ? 'setting' : 'getting'} extensionEnabledState for ' - '$extensionName in options file ($devtoolsOptionsFileUri)', + '$extensionName (package: $extensionPackage) in options file ($devtoolsOptionsFileUri)', ); if (debugDevToolsExtensions) { return debugHandleExtensionEnabledState( @@ -92,6 +93,7 @@ queryParameters: { ExtensionsApi.devtoolsOptionsUriPropertyName: devtoolsOptionsFileUri, ExtensionsApi.extensionNamePropertyName: extensionName, + ExtensionsApi.extensionPackagePropertyName: extensionPackage, if (enable != null) ExtensionsApi.enabledStatePropertyName: enable.toString(), },
diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index ca70b2c..9d13816 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md
@@ -93,6 +93,9 @@ [#8507](https://github.com/flutter/devtools/issues/8507) * Added iframe sandboxing for embedded DevTools extensions to enforce origin isolation. [#9967](https://github.com/flutter/devtools/pull/9967) +* Improved DevTools extension isolation by tracking the providing package name for + enablement, deduplication, and asset loading. + [#9981](https://github.com/flutter/devtools/pull/9981) ## Advanced developer mode updates
diff --git a/packages/devtools_app/test/extensions/extension_screen_test.dart b/packages/devtools_app/test/extensions/extension_screen_test.dart index 306a0dc..f0f9cc0 100644 --- a/packages/devtools_app/test/extensions/extension_screen_test.dart +++ b/packages/devtools_app/test/extensions/extension_screen_test.dart
@@ -79,34 +79,76 @@ await tester.pumpWidget(wrap(Builder(builder: fooScreen.build))); expect(find.byType(ExtensionView), findsOneWidget); expect(find.byType(EmbeddedExtensionHeader), findsOneWidget); - expect(find.richTextContaining('package:foo extension'), findsOneWidget); + expect( + find.descendant( + of: find.byType(EmbeddedExtensionHeader), + matching: find.richTextContaining('package:foo extension'), + ), + findsOneWidget, + ); expect(find.richTextContaining('(v1.0.0)'), findsOneWidget); expect(find.richTextContaining('Report an issue'), findsOneWidget); expect(_extensionContextMenuFinder, findsNothing); expect(find.byType(EnableExtensionPrompt), findsOneWidget); + expect( + find.descendant( + of: find.byType(EnableExtensionPrompt), + matching: find.richTextContaining( + 'The package:foo extension has not been enabled', + ), + ), + findsOneWidget, + ); expect(find.byType(EmbeddedExtensionView), findsNothing); await tester.pumpWidget(wrap(Builder(builder: barScreen.build))); expect(find.byType(ExtensionView), findsOneWidget); expect(find.byType(EmbeddedExtensionHeader), findsOneWidget); - expect(find.richTextContaining('package:bar extension'), findsOneWidget); + expect( + find.descendant( + of: find.byType(EmbeddedExtensionHeader), + matching: find.richTextContaining('package:bar extension'), + ), + findsOneWidget, + ); expect(find.richTextContaining('(v2.0.0)'), findsOneWidget); expect(find.richTextContaining('Report an issue'), findsOneWidget); expect(_extensionContextMenuFinder, findsNothing); expect(find.byType(EnableExtensionPrompt), findsOneWidget); + expect( + find.descendant( + of: find.byType(EnableExtensionPrompt), + matching: find.richTextContaining( + 'The package:bar extension has not been enabled', + ), + ), + findsOneWidget, + ); expect(find.byType(EmbeddedExtensionView), findsNothing); await tester.pumpWidget(wrap(Builder(builder: providerScreen.build))); expect(find.byType(ExtensionView), findsOneWidget); expect(find.byType(EmbeddedExtensionHeader), findsOneWidget); expect( - find.richTextContaining('package:provider extension'), + find.descendant( + of: find.byType(EmbeddedExtensionHeader), + matching: find.richTextContaining('package:provider extension'), + ), findsOneWidget, ); expect(find.richTextContaining('(v3.0.0)'), findsOneWidget); expect(find.richTextContaining('Report an issue'), findsOneWidget); expect(_extensionContextMenuFinder, findsNothing); expect(find.byType(EnableExtensionPrompt), findsOneWidget); + expect( + find.descendant( + of: find.byType(EnableExtensionPrompt), + matching: find.richTextContaining( + 'The package:provider extension has not been enabled', + ), + ), + findsOneWidget, + ); expect(find.byType(EmbeddedExtensionView), findsNothing); }); @@ -141,11 +183,26 @@ await tester.pumpWidget(wrap(Builder(builder: fooScreen.build))); expect(find.byType(ExtensionView), findsOneWidget); expect(find.byType(EmbeddedExtensionHeader), findsOneWidget); - expect(find.richTextContaining('package:foo extension'), findsOneWidget); + expect( + find.descendant( + of: find.byType(EmbeddedExtensionHeader), + matching: find.richTextContaining('package:foo extension'), + ), + findsOneWidget, + ); expect(find.richTextContaining('(v1.0.0)'), findsOneWidget); expect(find.richTextContaining('Report an issue'), findsOneWidget); expect(_extensionContextMenuFinder, findsNothing); expect(find.byType(EnableExtensionPrompt), findsOneWidget); + expect( + find.descendant( + of: find.byType(EnableExtensionPrompt), + matching: find.richTextContaining( + 'The package:foo extension has not been enabled', + ), + ), + findsOneWidget, + ); expect(find.byType(EmbeddedExtensionView), findsNothing); });
diff --git a/packages/devtools_app/test/extensions/extension_service_helpers_test.dart b/packages/devtools_app/test/extensions/extension_service_helpers_test.dart index 52f99d9..fbf0129 100644 --- a/packages/devtools_app/test/extensions/extension_service_helpers_test.dart +++ b/packages/devtools_app/test/extensions/extension_service_helpers_test.dart
@@ -23,6 +23,7 @@ // Returns 'b' when 'a' has parsing errors. var a = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'bar', + DevToolsExtensionConfig.packageNameKey: 'bar', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: 'this-will-not-parse', DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, @@ -35,6 +36,7 @@ }); var b = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'bar', + DevToolsExtensionConfig.packageNameKey: 'bar', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: '2.1.0', DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, @@ -50,6 +52,7 @@ // Returns 'a' when 'b' has parsing errors. a = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'bar', + DevToolsExtensionConfig.packageNameKey: 'bar', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: '2.1.0', DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, @@ -62,6 +65,7 @@ }); b = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'bar', + DevToolsExtensionConfig.packageNameKey: 'bar', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: 'this-will-not-parse', DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, @@ -76,6 +80,7 @@ // Returns 'a' when both 'a' and 'b' have parsing errors. a = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'bar', + DevToolsExtensionConfig.packageNameKey: 'bar', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: 'this-will-not-parse', DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, @@ -88,6 +93,7 @@ }); b = DevToolsExtensionConfig.parse({ DevToolsExtensionConfig.nameKey: 'bar', + DevToolsExtensionConfig.packageNameKey: 'bar', DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', DevToolsExtensionConfig.versionKey: 'this-will-not-parse', DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, @@ -101,4 +107,94 @@ expect(takeLatestExtension(a, b), a); }); }); + + group('deduplicateExtensionsAndTakeLatest', () { + test('deduplicates matching packageName', () { + final ignored = <DevToolsExtensionConfig>{}; + final ext1 = DevToolsExtensionConfig.parse({ + DevToolsExtensionConfig.nameKey: 'provider', + DevToolsExtensionConfig.packageNameKey: 'provider', + DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', + DevToolsExtensionConfig.versionKey: '1.0.0', + DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, + DevToolsExtensionConfig.requiresConnectionKey: 'false', + DevToolsExtensionConfig.extensionAssetsPathKey: '/path/to/provider_1', + DevToolsExtensionConfig.devtoolsOptionsUriKey: + 'file:///path/to/options', + DevToolsExtensionConfig.isPubliclyHostedKey: 'false', + DevToolsExtensionConfig.detectedFromStaticContextKey: 'true', + }); + final ext2 = DevToolsExtensionConfig.parse({ + DevToolsExtensionConfig.nameKey: 'provider', + DevToolsExtensionConfig.packageNameKey: 'provider', + DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', + DevToolsExtensionConfig.versionKey: '2.0.0', + DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, + DevToolsExtensionConfig.requiresConnectionKey: 'false', + DevToolsExtensionConfig.extensionAssetsPathKey: '/path/to/provider_2', + DevToolsExtensionConfig.devtoolsOptionsUriKey: + 'file:///path/to/options', + DevToolsExtensionConfig.isPubliclyHostedKey: 'false', + DevToolsExtensionConfig.detectedFromStaticContextKey: 'true', + }); + + deduplicateExtensionsAndTakeLatest( + [ext1, ext2], + onSetIgnored: (ext, {required ignore}) { + if (ignore) { + ignored.add(ext); + } else { + ignored.remove(ext); + } + }, + ); + + expect(ignored, contains(ext1)); + expect(ignored, isNot(contains(ext2))); + }); + + test('does not deduplicate across different packageNames', () { + final ignored = <DevToolsExtensionConfig>{}; + final providerExt = DevToolsExtensionConfig.parse({ + DevToolsExtensionConfig.nameKey: 'provider', + DevToolsExtensionConfig.packageNameKey: 'provider', + DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', + DevToolsExtensionConfig.versionKey: '1.0.0', + DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, + DevToolsExtensionConfig.requiresConnectionKey: 'false', + DevToolsExtensionConfig.extensionAssetsPathKey: '/path/to/provider', + DevToolsExtensionConfig.devtoolsOptionsUriKey: + 'file:///path/to/options', + DevToolsExtensionConfig.isPubliclyHostedKey: 'false', + DevToolsExtensionConfig.detectedFromStaticContextKey: 'true', + }); + final spoofedExt = DevToolsExtensionConfig.parse({ + DevToolsExtensionConfig.nameKey: 'provider', + DevToolsExtensionConfig.packageNameKey: 'bad_pkg', + DevToolsExtensionConfig.issueTrackerKey: 'www.google.com', + DevToolsExtensionConfig.versionKey: '999.0.0', + DevToolsExtensionConfig.materialIconCodePointKey: 0xe638, + DevToolsExtensionConfig.requiresConnectionKey: 'false', + DevToolsExtensionConfig.extensionAssetsPathKey: '/path/to/bad_pkg', + DevToolsExtensionConfig.devtoolsOptionsUriKey: + 'file:///path/to/options', + DevToolsExtensionConfig.isPubliclyHostedKey: 'false', + DevToolsExtensionConfig.detectedFromStaticContextKey: 'true', + }); + + deduplicateExtensionsAndTakeLatest( + [providerExt, spoofedExt], + onSetIgnored: (ext, {required ignore}) { + if (ignore) { + ignored.add(ext); + } else { + ignored.remove(ext); + } + }, + ); + + // Neither should be ignored because they come from different packages. + expect(ignored, isEmpty); + }); + }); }
diff --git a/packages/devtools_app/test/extensions/extension_service_test.dart b/packages/devtools_app/test/extensions/extension_service_test.dart index d8f61cb..fea1cc2 100644 --- a/packages/devtools_app/test/extensions/extension_service_test.dart +++ b/packages/devtools_app/test/extensions/extension_service_test.dart
@@ -100,6 +100,7 @@ return await server.extensionEnabledState( devtoolsOptionsFileUri: ext.devtoolsOptionsUri, extensionName: ext.name, + extensionPackage: ext.packageName, ); }
diff --git a/packages/devtools_app_shared/CHANGELOG.md b/packages/devtools_app_shared/CHANGELOG.md index 83756df..9115a73 100644 --- a/packages/devtools_app_shared/CHANGELOG.md +++ b/packages/devtools_app_shared/CHANGELOG.md
@@ -10,6 +10,7 @@ * Safely handle RPC errors and unexpected exceptions when calling service extensions in `ServiceExtensionManager`. * The minimum Dart SDK version is bumped to 3.11.0. * The minimum Flutter SDK version is bumped to 3.41.0. +* Updates `devtools_shared` constraint to `^14.0.1`. ## 0.5.1 * Add DevTools-styled text field `DevToolsTextField`.
diff --git a/packages/devtools_app_shared/pubspec.yaml b/packages/devtools_app_shared/pubspec.yaml index 10437f6..9f41284 100644 --- a/packages/devtools_app_shared/pubspec.yaml +++ b/packages/devtools_app_shared/pubspec.yaml
@@ -15,7 +15,7 @@ dependencies: collection: ^1.15.0 dds_service_extensions: ^2.0.0 - devtools_shared: ^14.0.0 + devtools_shared: ^14.0.1 dtd: ^4.0.0 flutter: sdk: flutter
diff --git a/packages/devtools_extensions/CHANGELOG.md b/packages/devtools_extensions/CHANGELOG.md index 2d412ae..5123fae 100644 --- a/packages/devtools_extensions/CHANGELOG.md +++ b/packages/devtools_extensions/CHANGELOG.md
@@ -6,6 +6,8 @@ ## 0.5.2-wip * The minimum Dart SDK version is bumped to 3.11.0. * The minimum Flutter SDK version is bumped to 3.41.0. +* Updates `devtools_shared` constraint to `^14.0.1`. +* Add validation for `pubspec.yaml` existence and valid characters in the `config.yaml` `name` field to the `validate` command. ## 0.5.1 * Updates `devtools_app_shared` constraint to `^0.5.1`.
diff --git a/packages/devtools_extensions/bin/_validate.dart b/packages/devtools_extensions/bin/_validate.dart index a0542f3..30f36cc 100644 --- a/packages/devtools_extensions/bin/_validate.dart +++ b/packages/devtools_extensions/bin/_validate.dart
@@ -53,6 +53,7 @@ ..._configAsMap(packagePath), // These are generated on the DevTools server, so pass in stubbed // values for the sake of validation. + DevToolsExtensionConfig.packageNameKey: '', DevToolsExtensionConfig.extensionAssetsPathKey: '', DevToolsExtensionConfig.devtoolsOptionsUriKey: '', DevToolsExtensionConfig.isPubliclyHostedKey: 'false', @@ -78,6 +79,14 @@ throw FileSystemException('${packageDirectory.path} directory not found'); } + final pubspecFile = File(path.join(packageDirectory.path, 'pubspec.yaml')); + if (!pubspecFile.existsSync()) { + throw const FileSystemException(''' +A pubspec.yaml file is required, but none was found. +See ${ValidateExtensionCommand.docUrl}. +'''); + } + final devtoolsExtensionDir = Directory( path.join(packageDirectory.path, 'extension', 'devtools'), ); @@ -109,6 +118,17 @@ See ${ValidateExtensionCommand.docUrl}. '''); } + + // Ensure the extension's name is a valid identifier. + final configYaml = _configAsMap(packagePath); + final configName = configYaml['name']; + final underscoresAndLetters = RegExp(r'^[a-z0-9_]*$'); + if (configName is! String || !underscoresAndLetters.hasMatch(configName)) { + throw StateError( + 'The "name" field in config.yaml should only contain lowercase letters, ' + 'numbers, and underscores but instead was "$configName".', + ); + } } Map<String, Object?> _configAsMap(String packagePath) {
diff --git a/packages/devtools_extensions/pubspec.yaml b/packages/devtools_extensions/pubspec.yaml index df7ce76..4639c9c 100644 --- a/packages/devtools_extensions/pubspec.yaml +++ b/packages/devtools_extensions/pubspec.yaml
@@ -18,7 +18,7 @@ dependencies: args: ^2.4.2 - devtools_shared: ^14.0.0 + devtools_shared: ^14.0.1 devtools_app_shared: ^0.5.1 flutter: sdk: flutter
diff --git a/packages/devtools_extensions/test/validate_test.dart b/packages/devtools_extensions/test/validate_test.dart index 8efcb5e..3616638 100644 --- a/packages/devtools_extensions/test/validate_test.dart +++ b/packages/devtools_extensions/test/validate_test.dart
@@ -53,4 +53,44 @@ }); } }); + + group('devtools_extensions validate command fails', () { + test('when config.yaml name contains invalid characters', () async { + final tempDir = Directory.systemTemp.createTempSync(); + try { + final extDir = Directory(p.join(tempDir.path, 'extension', 'devtools')) + ..createSync(recursive: true); + Directory(p.join(extDir.path, 'build')).createSync(recursive: true); + File(p.join(extDir.path, 'build', 'index.html')).writeAsStringSync(''); + File(p.join(extDir.path, 'config.yaml')).writeAsStringSync(''' +name: invalid-name-with-hyphens +issueTracker: https://www.google.com/ +version: 1.0.0 +materialIconCodePoint: "0xe50a" +'''); + File(p.join(tempDir.path, 'pubspec.yaml')).writeAsStringSync(''' +name: actual_package_name +environment: + sdk: ^3.2.0 +'''); + + final process = await Process.run('dart', [ + 'run', + 'devtools_extensions', + 'validate', + '-p', + tempDir.path, + ]); + expect( + process.stderr, + contains( + 'Validation error: The "name" field in config.yaml should only ' + 'contain lowercase letters, numbers, and underscores', + ), + ); + } finally { + tempDir.deleteSync(recursive: true); + } + }); + }); }
diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md index 132ea2a..f5c4069 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md
@@ -3,6 +3,11 @@ Use of this source code is governed by a BSD-style license that can be found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. --> +# 14.0.1 + +* Track providing package name for DevTools extensions to isolate extension enablement, + deduplication, and asset loading. + # 14.0.0 * **Breaking changes**: `LocalFileSystem`, an extension which provided some handy
diff --git a/packages/devtools_shared/lib/src/devtools_api.dart b/packages/devtools_shared/lib/src/devtools_api.dart index f28ffbb..fd2ca93 100644 --- a/packages/devtools_shared/lib/src/devtools_api.dart +++ b/packages/devtools_shared/lib/src/devtools_api.dart
@@ -136,6 +136,11 @@ /// name of the extension whose state is being queried. static const extensionNamePropertyName = 'name'; + /// The property name for the query parameter optionally passed along with + /// [apiExtensionEnabledState] requests to the server that describes the + /// package name providing the extension. + static const extensionPackagePropertyName = 'package'; + /// The property name for the query parameter that is optionally passed along /// with [apiExtensionEnabledState] requests to the server to set the /// enabled state for the extension.
diff --git a/packages/devtools_shared/lib/src/extensions/extension_enablement.dart b/packages/devtools_shared/lib/src/extensions/extension_enablement.dart index d6a1267..d86c03c 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_enablement.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_enablement.dart
@@ -23,8 +23,9 @@ $_extensionsKey: '''; - /// Returns the current enabled state for [extensionName] in the - /// 'devtools_options.yaml' file at [devtoolsOptionsUri]. + /// Returns the current enabled state of [packageName] (falls back to + /// [extensionName]) in the 'devtools_options.yaml' file at + /// [devtoolsOptionsUri]. /// /// If the 'devtools_options.yaml' file does not exist, it will be created /// with an empty set of extensions. @@ -33,6 +34,7 @@ ExtensionEnabledState lookupExtensionEnabledState({ required Uri devtoolsOptionsUri, required String extensionName, + String? packageName, }) { final options = _optionsAsMap(optionsUri: devtoolsOptionsUri); if (options == null) return ExtensionEnabledState.error; @@ -41,19 +43,20 @@ ?.cast<Map<String, Object?>>(); if (extensions == null) return ExtensionEnabledState.none; + final targetKey = packageName ?? extensionName; for (final e in extensions) { // Each entry should only have one key / value pair (e.g. '- foo: true'). assert(e.keys.length == 1); - if (e.keys.first == extensionName) { - return _extensionStateForValue(e[extensionName]); + if (e.keys.first == targetKey) { + return _extensionStateForValue(e[targetKey]); } } return ExtensionEnabledState.none; } - /// Sets the enabled state for [extensionName] in the - /// 'devtools_options.yaml' file at [devtoolsOptionsUri]. + /// Sets the enabled state of [packageName] (falls back to [extensionName]) + /// in the 'devtools_options.yaml' file at [devtoolsOptionsUri]. /// /// If the 'devtools_options.yaml' file does not exist, it will be created. /// @@ -61,6 +64,7 @@ ExtensionEnabledState setExtensionEnabledState({ required Uri devtoolsOptionsUri, required String extensionName, + String? packageName, required bool enable, }) { final options = _optionsAsMap(optionsUri: devtoolsOptionsUri); @@ -73,14 +77,16 @@ extensions = options[_extensionsKey] as List<Map<String, Object?>>; } + final targetKey = packageName ?? extensionName; + // Write the new enabled state to the map. final extension = extensions.firstWhereOrNull( - (e) => e.keys.first == extensionName, + (e) => e.keys.first == targetKey, ); if (extension == null) { - extensions.add({extensionName: enable}); + extensions.add({targetKey: enable}); } else { - extension[extensionName] = enable; + extension[targetKey] = enable; } _writeToOptionsFile(optionsUri: devtoolsOptionsUri, options: options); @@ -90,6 +96,7 @@ return lookupExtensionEnabledState( devtoolsOptionsUri: devtoolsOptionsUri, extensionName: extensionName, + packageName: packageName, ); }
diff --git a/packages/devtools_shared/lib/src/extensions/extension_manager.dart b/packages/devtools_shared/lib/src/extensions/extension_manager.dart index 92ad601..90da58f 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_manager.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_manager.dart
@@ -148,6 +148,7 @@ for (final extension in extensions) { final config = extension.config; + // TODO(https://github.com/dart-lang/pub/issues/4042): make this check // more robust. final isPubliclyHosted = @@ -160,16 +161,34 @@ final relativeExtensionLocation = config['buildLocation'] as String? ?? 'build'; - final location = path.join( - extension.rootUri.toFilePath(), - 'extension', - 'devtools', - relativeExtensionLocation, + final packageRoot = path.normalize(extension.rootUri.toFilePath()); + final location = path.normalize( + path.join( + packageRoot, + 'extension', + 'devtools', + relativeExtensionLocation, + ), ); + // Verify that this extension's build location is a subdirectory of the + // package providing the extension. Usually this is + // $HOME/.pub-cache/hosted/pub.dev/foo-1.0.0/extension/devtools/build. + // + // This prevents packages from declaring a build location outside of + // their package directory (e.g. ../../../../.ssh/) + if (!path.isWithin(packageRoot, location)) { + parsingErrors.writeln( + 'Ignoring extension from package "${extension.package}": invalid ' + 'buildLocation "$relativeExtensionLocation" outside package root.', + ); + continue; + } + try { final extensionConfig = DevToolsExtensionConfig.parse({ ...config, + DevToolsExtensionConfig.packageNameKey: extension.package, DevToolsExtensionConfig.extensionAssetsPathKey: location, // The [packageConfigPath] will look like // 'pkg/.dart_tool/package_config.json' so we will store the
diff --git a/packages/devtools_shared/lib/src/extensions/extension_model.dart b/packages/devtools_shared/lib/src/extensions/extension_model.dart index 8d669e0..4cb5b22 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_model.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_model.dart
@@ -16,6 +16,7 @@ class DevToolsExtensionConfig implements Comparable<DevToolsExtensionConfig> { DevToolsExtensionConfig._({ required this.name, + required this.packageName, required this.issueTrackerLink, required this.version, required this.materialIconCodePoint, @@ -42,6 +43,7 @@ // The expected keys below are not from the extension's config.yaml // file; they are generated during the extension detection mechanism // in the DevTools server. + packageNameKey: final String packageName, extensionAssetsPathKey: final String extensionAssetsPath, devtoolsOptionsUriKey: final String devtoolsOptionsUri, isPubliclyHostedKey: final String isPubliclyHosted, @@ -79,6 +81,7 @@ // and will use default values if not specified. requiresConnection: requiresConnection, // These values are generated by the DevTools server. + packageName: packageName, extensionAssetsPath: extensionAssetsPath, devtoolsOptionsUri: devtoolsOptionsUri, isPubliclyHosted: bool.parse(isPubliclyHosted), @@ -130,22 +133,30 @@ // The following keys are never expected to be in the extension's config.yaml // file. They are generated during the extension detection mechanism in the // DevTools server. + static const packageNameKey = 'packageName'; static const extensionAssetsPathKey = 'extensionAssetsPath'; static const devtoolsOptionsUriKey = 'devtoolsOptionsUri'; static const isPubliclyHostedKey = 'isPubliclyHosted'; static const detectedFromStaticContextKey = 'detectedFromStaticContext'; static const _serverGeneratedKeys = [ + packageNameKey, extensionAssetsPathKey, devtoolsOptionsUriKey, isPubliclyHostedKey, detectedFromStaticContextKey, ]; - /// The package name that this extension is for. + /// The name that this extension is for. /// - /// This value should be defined by the extension's config.yaml file. + /// This value is defined by the extension's config.yaml file. final String name; + /// The Dart package that provides this DevTools extension. + /// + /// This value is parsed from the package name in + /// `.dart_tool/package_config.json`. + final String packageName; + // TODO(kenz): we might want to add validation to these issue tracker // links to ensure they don't point to the DevTools repo or flutter repo. // If an invalid issue tracker link is provided, we can default to @@ -230,12 +241,15 @@ String get displayName => name.toLowerCase(); - String get identifier => '${displayName}_$version'; + String get identifier => packageName == displayName + ? '${displayName}_$version' + : '${packageName}_${displayName}_$version'; String get analyticsSafeName => isPubliclyHosted ? name : 'private'; Map<String, Object?> toJson() => { nameKey: name, + packageNameKey: packageName, issueTrackerKey: issueTrackerLink, versionKey: version, materialIconCodePointKey: materialIconCodePoint, @@ -248,11 +262,14 @@ @override int compareTo(DevToolsExtensionConfig other) { - var compare = name.compareTo(other.name); + var compare = packageName.compareTo(other.packageName); if (compare == 0) { - compare = extensionAssetsPath.compareTo(other.extensionAssetsPath); + compare = name.compareTo(other.name); if (compare == 0) { - return devtoolsOptionsUri.compareTo(other.devtoolsOptionsUri); + compare = extensionAssetsPath.compareTo(other.extensionAssetsPath); + if (compare == 0) { + return devtoolsOptionsUri.compareTo(other.devtoolsOptionsUri); + } } } return compare; @@ -262,6 +279,7 @@ bool operator ==(Object other) { return other is DevToolsExtensionConfig && other.name == name && + other.packageName == packageName && other.issueTrackerLink == issueTrackerLink && other.version == version && other.materialIconCodePoint == materialIconCodePoint && @@ -275,6 +293,7 @@ @override int get hashCode => Object.hash( name, + packageName, issueTrackerLink, version, materialIconCodePoint,
diff --git a/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart b/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart index 22dded4..035ccaa 100644 --- a/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart +++ b/packages/devtools_shared/lib/src/server/handlers/_devtools_extensions.dart
@@ -104,12 +104,15 @@ } final extensionName = queryParams[ExtensionsApi.extensionNamePropertyName]!; + final extensionPackage = + queryParams[ExtensionsApi.extensionPackagePropertyName]; final activate = queryParams[ExtensionsApi.enabledStatePropertyName]; if (activate != null) { final newState = ServerApi._devToolsOptions.setExtensionEnabledState( devtoolsOptionsUri: devtoolsOptionsFileUri, extensionName: extensionName, + packageName: extensionPackage, enable: bool.parse(activate), ); return ServerApi._encodeResponse(newState.name, api: api); @@ -118,6 +121,7 @@ .lookupExtensionEnabledState( devtoolsOptionsUri: devtoolsOptionsFileUri, extensionName: extensionName, + packageName: extensionPackage, ); return ServerApi._encodeResponse(activationState.name, api: api); }
diff --git a/packages/devtools_shared/pubspec.yaml b/packages/devtools_shared/pubspec.yaml index ff12188..4507c5e 100644 --- a/packages/devtools_shared/pubspec.yaml +++ b/packages/devtools_shared/pubspec.yaml
@@ -4,7 +4,7 @@ name: devtools_shared description: Package of shared Dart structures between devtools_app, dds, and other tools. -version: 14.0.0 +version: 14.0.1 repository: https://github.com/flutter/devtools/tree/master/packages/devtools_shared
diff --git a/packages/devtools_shared/test/extensions/extension_enablement_test.dart b/packages/devtools_shared/test/extensions/extension_enablement_test.dart index 36984b3..db841b7 100644 --- a/packages/devtools_shared/test/extensions/extension_enablement_test.dart +++ b/packages/devtools_shared/test/extensions/extension_enablement_test.dart
@@ -103,5 +103,58 @@ ExtensionEnabledState.none, ); }); + + test('isolates enablement state by packageName', () { + options.setExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'provider', + packageName: 'provider', + enable: true, + ); + + // Legitimate provider matches + expect( + options.lookupExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'provider', + packageName: 'provider', + ), + ExtensionEnabledState.enabled, + ); + + // Spoofed package does not inherit provider's enablement + expect( + options.lookupExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'provider', + packageName: 'bad_pkg', + ), + ExtensionEnabledState.none, + ); + + // Custom package with custom tool name writes and reads cleanly using packageName + options.setExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'custom_tool', + packageName: 'custom_pkg', + enable: true, + ); + expect( + options.lookupExtensionEnabledState( + devtoolsOptionsUri: optionsUri, + extensionName: 'custom_tool', + packageName: 'custom_pkg', + ), + ExtensionEnabledState.enabled, + ); + + final file = optionsFileFromTmp(); + expect(file.readAsStringSync(), ''' +description: This file stores settings for Dart & Flutter DevTools. +documentation: https://docs.flutter.dev/tools/devtools/extensions#configure-extension-enablement-states +extensions: + - provider: true + - custom_pkg: true'''); + }); }); }
diff --git a/packages/devtools_shared/test/extensions/extension_model_test.dart b/packages/devtools_shared/test/extensions/extension_model_test.dart index 2bf4236..3a98e04 100644 --- a/packages/devtools_shared/test/extensions/extension_model_test.dart +++ b/packages/devtools_shared/test/extensions/extension_model_test.dart
@@ -10,6 +10,7 @@ test('parses with a String materialIconCodePoint field', () { final config = DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': '0xf012', @@ -21,6 +22,7 @@ }); expect(config.name, 'foo'); + expect(config.packageName, 'foo'); expect(config.extensionAssetsPath, '/absolute/path/to/foo/extension'); expect(config.issueTrackerLink, 'www.google.com'); expect(config.version, '1.0.0'); @@ -31,6 +33,7 @@ test('parses with an int materialIconCodePoint field', () { final config = DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -42,6 +45,7 @@ }); expect(config.name, 'foo'); + expect(config.packageName, 'foo'); expect(config.extensionAssetsPath, '/absolute/path/to/foo/extension'); expect(config.issueTrackerLink, 'www.google.com'); expect(config.version, '1.0.0'); @@ -52,6 +56,7 @@ test('parses with a String requiresConnection field', () { final config = DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': '0xf012', @@ -63,6 +68,7 @@ }); expect(config.name, 'foo'); + expect(config.packageName, 'foo'); expect(config.extensionAssetsPath, '/absolute/path/to/foo/extension'); expect(config.issueTrackerLink, 'www.google.com'); expect(config.version, '1.0.0'); @@ -73,6 +79,7 @@ test('parses with a bool requiresConnection field', () { final config = DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -84,6 +91,7 @@ }); expect(config.name, 'foo'); + expect(config.packageName, 'foo'); expect(config.extensionAssetsPath, '/absolute/path/to/foo/extension'); expect(config.issueTrackerLink, 'www.google.com'); expect(config.version, '1.0.0'); @@ -91,6 +99,36 @@ expect(config.requiresConnection, false); }); + test('parses with a packageName field and computes identifier', () { + final configWithMatchingPackage = DevToolsExtensionConfig.parse({ + 'name': 'foo', + 'packageName': 'foo', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + 'materialIconCodePoint': 0xf012, + 'extensionAssetsPath': '/absolute/path/to/foo/extension', + 'devtoolsOptionsUri': 'file:///path/to/package/devtools_options.yaml', + 'isPubliclyHosted': 'false', + 'detectedFromStaticContext': 'false', + }); + expect(configWithMatchingPackage.packageName, 'foo'); + expect(configWithMatchingPackage.identifier, 'foo_1.0.0'); + + final configWithDistinctPackage = DevToolsExtensionConfig.parse({ + 'name': 'bar', + 'packageName': 'foo', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + 'materialIconCodePoint': 0xf012, + 'extensionAssetsPath': '/absolute/path/to/foo/extension', + 'devtoolsOptionsUri': 'file:///path/to/package/devtools_options.yaml', + 'isPubliclyHosted': 'false', + 'detectedFromStaticContext': 'false', + }); + expect(configWithDistinctPackage.packageName, 'foo'); + expect(configWithDistinctPackage.identifier, 'foo_bar_1.0.0'); + }); + group('parse throws when missing required field', () { Matcher throwsMissingRequiredFieldsError() { return throwsA( @@ -115,6 +153,7 @@ test('name', () { expect(() { DevToolsExtensionConfig.parse({ + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -131,6 +170,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, 'extensionAssetsPath': '/absolute/path/to/foo/extension', @@ -146,6 +186,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'materialIconCodePoint': 0xf012, 'extensionAssetsPath': '/absolute/path/to/foo/extension', @@ -161,6 +202,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'extensionAssetsPath': '/absolute/path/to/foo/extension', @@ -171,10 +213,28 @@ }); }, throwsMissingRequiredFieldsError()); }); + + test('packageName', () { + expect(() { + DevToolsExtensionConfig.parse({ + 'name': 'foo', + 'issueTracker': 'www.google.com', + 'version': '1.0.0', + 'materialIconCodePoint': 0xf012, + 'extensionAssetsPath': '/absolute/path/to/foo/extension', + 'devtoolsOptionsUri': + 'file:///path/to/package/devtools_options.yaml', + 'isPubliclyHosted': 'false', + 'detectedFromStaticContext': 'false', + }); + }, throwsMissingGeneratedKeysError()); + }); + test('extensionAssetsPath', () { expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -190,6 +250,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -204,6 +265,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -218,6 +280,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -245,6 +308,7 @@ DevToolsExtensionConfig.parse({ // Expects a String here. 'name': 23, + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -258,6 +322,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -284,6 +349,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'name with spaces', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -297,6 +363,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'Name_With_Capital_Letters', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -310,6 +377,7 @@ expect(() { DevToolsExtensionConfig.parse({ 'name': 'name.with\'specialchars/', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012,
diff --git a/packages/devtools_shared/test/helpers/extension_test_manager.dart b/packages/devtools_shared/test/helpers/extension_test_manager.dart index 323ab81..00add39 100644 --- a/packages/devtools_shared/test/helpers/extension_test_manager.dart +++ b/packages/devtools_shared/test/helpers/extension_test_manager.dart
@@ -82,14 +82,19 @@ Future<void> setupTestDirectoryStructure({ bool includeDependenciesWithExtensions = true, bool includeBadExtension = false, + bool includeSpoofedExtension = false, }) async { _testDirectory = Directory.systemTemp.createTempSync(); _setupPackages( includeDependenciesWithExtensions: includeDependenciesWithExtensions, includeBadExtension: includeBadExtension, + includeSpoofedExtension: includeSpoofedExtension, ); - _setupExtensions(includeBadExtension: includeBadExtension); + _setupExtensions( + includeBadExtension: includeBadExtension, + includeSpoofedExtension: includeSpoofedExtension, + ); // Generate the .dart_tool/package_config.json file for each Dart package. final testDirectoryContents = testDirectory.listSync(); @@ -144,10 +149,19 @@ void _setupPackages({ required bool includeDependenciesWithExtensions, required bool includeBadExtension, + bool includeSpoofedExtension = false, }) { + final TestPackage myApp; + if (includeSpoofedExtension) { + myApp = myAppPackageWithSpoofedExtension; + } else if (includeBadExtension) { + myApp = myAppPackageWithBadExtension; + } else { + myApp = myAppPackage; + } _setupPackage( createTestPackageFrom( - includeBadExtension ? myAppPackageWithBadExtension : myAppPackage, + myApp, includeDependenciesWithExtensions: includeDependenciesWithExtensions, ), isRuntimeRoot: true, @@ -228,7 +242,10 @@ /// devtools/ /// build/ /// config.yaml - void _setupExtensions({required bool includeBadExtension}) { + void _setupExtensions({ + required bool includeBadExtension, + bool includeSpoofedExtension = false, + }) { _setupExtension(staticExtension1Package); _setupExtension(staticExtension2Package); @@ -237,6 +254,7 @@ _setupExtension(newerStaticExtension1Package); if (includeBadExtension) _setupExtension(badExtensionPackage); + if (includeSpoofedExtension) _setupExtension(spoofedExtensionPackage); } void _setupPackage(TestPackage package, {bool isRuntimeRoot = false}) { @@ -304,6 +322,10 @@ name: myAppPackage.name, dependencies: [...myAppPackage.dependencies, badExtensionPackage], ); +final myAppPackageWithSpoofedExtension = TestPackage( + name: myAppPackage.name, + dependencies: [...myAppPackage.dependencies, spoofedExtensionPackage], +); final otherRoot1Package = TestPackage( name: 'other_root_1', dependencies: [staticExtension1Package, staticExtension2Package], @@ -320,6 +342,7 @@ final driftPackage = TestPackageWithExtension( name: 'drift', + packageName: 'drift', issueTracker: 'https://github.com/simolus3/drift/issues', version: '0.0.1', materialIconCodePoint: 62494, @@ -329,6 +352,7 @@ ); final providerPackage = TestPackageWithExtension( name: 'provider', + packageName: 'provider', issueTracker: 'https://github.com/rrousselGit/provider/issues', version: '0.0.1', materialIconCodePoint: 57521, @@ -338,6 +362,7 @@ ); final staticExtension1Package = TestPackageWithExtension( name: 'static_extension_1', + packageName: 'static_extension_1', issueTracker: 'https://www.google.com/', version: '1.0.0', materialIconCodePoint: 0xe50a, @@ -347,6 +372,7 @@ ); final staticExtension2Package = TestPackageWithExtension( name: 'static_extension_2', + packageName: 'static_extension_2', issueTracker: 'https://www.google.com/', version: '2.0.0', materialIconCodePoint: 0xe50a, @@ -356,6 +382,7 @@ ); final newerStaticExtension1Package = TestPackageWithExtension( name: 'static_extension_1', + packageName: 'static_extension_1', issueTracker: 'https://www.google.com/', version: '2.0.0', materialIconCodePoint: 0xe50a, @@ -367,6 +394,7 @@ final badExtensionPackage = TestPackageWithExtension( // Extension names must be only lowercase letters and underscores. name: 'BAD_EXTENSION', + packageName: 'bad_extension', issueTracker: 'https://www.google.com/', version: '1.0.0', materialIconCodePoint: 0xe50a, @@ -374,10 +402,21 @@ isPubliclyHosted: false, packageVersion: null, ); +final spoofedExtensionPackage = TestPackageWithExtension( + name: 'provider', + packageName: 'bad_pkg', + issueTracker: 'https://www.google.com/', + version: '999.0.0', + materialIconCodePoint: 0xe50a, + requiresConnection: true, + isPubliclyHosted: false, + packageVersion: null, +); class TestPackageWithExtension { TestPackageWithExtension({ required this.name, + required this.packageName, required this.issueTracker, required this.version, required this.materialIconCodePoint, @@ -386,10 +425,10 @@ required this.packageVersion, String? relativePathFromExtensions, }) : assert(isPubliclyHosted == (packageVersion != null)), - relativePathFromExtensions = - relativePathFromExtensions ?? name.toLowerCase(); + relativePathFromExtensions = relativePathFromExtensions ?? packageName; final String name; + final String packageName; final String issueTracker; final String version; final Object? materialIconCodePoint; @@ -413,7 +452,7 @@ String get pubspecContent => ''' -name: ${name.toLowerCase()} +name: $packageName environment: sdk: ">=3.4.0-282.1.beta <4.0.0" '''; @@ -442,7 +481,7 @@ String _dependenciesAsString() { final sb = StringBuffer(); for (final dep in dependencies) { - sb.write(' ${dep.name.toLowerCase()}:'); + sb.write(' ${dep.packageName}:'); if (dep.isPubliclyHosted) { sb.writeln(' ${dep.packageVersion!}'); } else {
diff --git a/packages/devtools_shared/test/helpers/extension_test_manager_test.dart b/packages/devtools_shared/test/helpers/extension_test_manager_test.dart index e810145..c49d6e7 100644 --- a/packages/devtools_shared/test/helpers/extension_test_manager_test.dart +++ b/packages/devtools_shared/test/helpers/extension_test_manager_test.dart
@@ -13,6 +13,7 @@ group('$TestPackageWithExtension', () { test('$driftPackage', () { expect(driftPackage.name, 'drift'); + expect(driftPackage.packageName, 'drift'); expect( driftPackage.issueTracker, 'https://github.com/simolus3/drift/issues', @@ -27,6 +28,7 @@ test('$providerPackage', () { expect(providerPackage.name, 'provider'); + expect(providerPackage.packageName, 'provider'); expect( providerPackage.issueTracker, 'https://github.com/rrousselGit/provider/issues', @@ -41,6 +43,7 @@ test('$staticExtension1Package', () { expect(staticExtension1Package.name, 'static_extension_1'); + expect(staticExtension1Package.packageName, 'static_extension_1'); expect(staticExtension1Package.issueTracker, 'https://www.google.com/'); expect(staticExtension1Package.version, '1.0.0'); expect(staticExtension1Package.materialIconCodePoint, 0xe50a); @@ -67,6 +70,7 @@ test('$staticExtension2Package', () { expect(staticExtension2Package.name, 'static_extension_2'); + expect(staticExtension2Package.packageName, 'static_extension_2'); expect(staticExtension2Package.issueTracker, 'https://www.google.com/'); expect(staticExtension2Package.version, '2.0.0'); expect(staticExtension2Package.materialIconCodePoint, 0xe50a); @@ -93,6 +97,7 @@ test('$newerStaticExtension1Package', () { expect(newerStaticExtension1Package.name, 'static_extension_1'); + expect(newerStaticExtension1Package.packageName, 'static_extension_1'); expect( newerStaticExtension1Package.issueTracker, 'https://www.google.com/', @@ -122,6 +127,7 @@ test('$badExtensionPackage', () { expect(badExtensionPackage.name, 'BAD_EXTENSION'); + expect(badExtensionPackage.packageName, 'bad_extension'); expect(badExtensionPackage.issueTracker, 'https://www.google.com/'); expect(badExtensionPackage.version, '1.0.0'); expect(badExtensionPackage.materialIconCodePoint, 0xe50a); @@ -130,7 +136,7 @@ expect(badExtensionPackage.packageVersion, null); expect( badExtensionPackage.relativePathFromExtensions, - badExtensionPackage.name.toLowerCase(), + badExtensionPackage.packageName, ); expect(badExtensionPackage.pubspecContent, ''' name: bad_extension @@ -145,6 +151,33 @@ '''); }); + + test('$spoofedExtensionPackage', () { + expect(spoofedExtensionPackage.name, 'provider'); + expect(spoofedExtensionPackage.packageName, 'bad_pkg'); + expect(spoofedExtensionPackage.issueTracker, 'https://www.google.com/'); + expect(spoofedExtensionPackage.version, '999.0.0'); + expect(spoofedExtensionPackage.materialIconCodePoint, 0xe50a); + expect(spoofedExtensionPackage.requiresConnection, true); + expect(spoofedExtensionPackage.isPubliclyHosted, false); + expect(spoofedExtensionPackage.packageVersion, null); + expect( + spoofedExtensionPackage.relativePathFromExtensions, + spoofedExtensionPackage.packageName, + ); + expect(spoofedExtensionPackage.pubspecContent, ''' +name: bad_pkg +environment: + sdk: ">=3.4.0-282.1.beta <4.0.0" +'''); + expect(spoofedExtensionPackage.configYamlContent, ''' +name: provider +issueTracker: https://www.google.com/ +version: 999.0.0 +materialIconCodePoint: 58634 + +'''); + }); }); group('$TestPackage', () {
diff --git a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart index af0530e..ca0d905 100644 --- a/packages/devtools_shared/test/server/devtools_extensions_api_test.dart +++ b/packages/devtools_shared/test/server/devtools_extensions_api_test.dart
@@ -5,6 +5,7 @@ import 'dart:convert'; import 'dart:io'; +import 'package:collection/collection.dart'; import 'package:devtools_shared/devtools_extensions.dart'; import 'package:devtools_shared/devtools_shared.dart'; import 'package:devtools_shared/src/extensions/extension_manager.dart'; @@ -44,10 +45,12 @@ Future<void> initializeTestDirectory({ bool includeDependenciesWithExtensions = true, bool includeBadExtension = false, + bool includeSpoofedExtension = false, }) async { await extensionTestManager.setupTestDirectoryStructure( includeDependenciesWithExtensions: includeDependenciesWithExtensions, includeBadExtension: includeBadExtension, + includeSpoofedExtension: includeSpoofedExtension, ); await testDtdConnection!.setIDEWorkspaceRoots(dtd!.info!.secret!, [ extensionTestManager.packagesRootUri, @@ -112,6 +115,40 @@ ); }); + test( + 'spoofed extension is isolated by packageName and cannot overwrite legitimate extension', + () async { + await initializeTestDirectory(includeSpoofedExtension: true); + final response = await serveExtensions(extensionsManager); + expect(response.statusCode, HttpStatus.ok); + + // Verify that the legitimate provider extension is present. + final providerExtension = extensionsManager.devtoolsExtensions + .firstWhereOrNull((e) => e.packageName == 'provider'); + expect(providerExtension, isNotNull); + expect(providerExtension!.name, 'provider'); + expect(providerExtension.version, providerPackage.version); + + // Verify that the spoofed extension from bad_pkg is isolated under packageName 'bad_pkg'. + final spoofedExtension = extensionsManager.devtoolsExtensions + .firstWhereOrNull((e) => e.packageName == 'bad_pkg'); + expect(spoofedExtension, isNotNull); + expect(spoofedExtension!.name, 'provider'); + expect(spoofedExtension.version, '999.0.0'); + expect(spoofedExtension.identifier, 'bad_pkg_provider_999.0.0'); + + // Verify lookupLocationFor returns the respective paths without collision. + expect( + extensionsManager.lookupLocationFor(providerExtension.identifier), + providerExtension.extensionAssetsPath, + ); + expect( + extensionsManager.lookupLocationFor(spoofedExtension.identifier), + spoofedExtension.extensionAssetsPath, + ); + }, + ); + test('succeeds for valid extensions when an exception is thrown', () async { await initializeTestDirectory(); extensionsManager = _TestExtensionsManager(); @@ -399,6 +436,7 @@ required bool fromStaticContext, }) { expect(ext.name, extensionPackage.name); + expect(ext.packageName, extensionPackage.packageName); expect(ext.issueTrackerLink, extensionPackage.issueTracker); expect(ext.version, extensionPackage.version); expect(ext.materialIconCodePoint, extensionPackage.materialIconCodePoint);