diff --git a/packages/devtools_app/lib/src/extensions/extension_screen.dart b/packages/devtools_app/lib/src/extensions/extension_screen.dart index 7102119f487..3d0bb61dae1 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 @@ class ExtensionView extends StatelessWidget { const SizedBox(height: intermediateSpacing), Expanded( child: ValueListenableBuilder( - 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 5635266ba6b..813e183999b 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 @@ class EmbeddedExtensionHeader extends StatelessWidget { @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 @@ class EmbeddedExtensionHeader extends StatelessWidget { 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 @@ class _ExtensionContextMenuButton extends StatelessWidget { @override Widget build(BuildContext context) { return ValueListenableBuilder( - 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 @@ class DisableExtensionDialog extends StatelessWidget { 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 @@ class EnableExtensionPrompt extends StatelessWidget { 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 ae49a3b7022..2ba343c34cd 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 @@ class ExtensionService extends DisposableController final _ignoredStaticExtensionsByHashCode = {}; /// Returns the [ValueListenable] that stores the [ExtensionEnabledState] for - /// the DevTools Extension with [extensionName]. + /// the DevTools Extension provided by [extensionPackageName]. ValueListenable enabledStateListenable( - String extensionName, + String extensionPackageName, ) { return _extensionEnabledStates.putIfAbsent( - extensionName.toLowerCase(), + extensionPackageName.toLowerCase(), () => ValueNotifier(ExtensionEnabledState.none), ); } @@ -232,7 +232,7 @@ class ExtensionService extends DisposableController // 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 @@ class ExtensionService extends DisposableController final stateFromOptionsFile = await server.extensionEnabledState( devtoolsOptionsFileUri: extension.devtoolsOptionsUri, extensionName: extension.name, + extensionPackage: extension.packageName, ); final stateNotifier = _extensionEnabledStates.putIfAbsent( - extension.name, + extension.packageName.toLowerCase(), () => ValueNotifier(stateFromOptionsFile), ); stateNotifier.value = stateFromOptionsFile; @@ -295,12 +296,13 @@ class ExtensionService extends DisposableController 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 1e5f54379cf..3133ac7a795 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 @@ void deduplicateExtensionsAndTakeLatest( }) { final deduped = {}; 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 @@ void deduplicateExtensionsAndTakeLatest( ); } 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 4ebfdfae6d9..fa2dd8aa16f 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 @@ class ExtensionSetting extends StatelessWidget { ), ]; 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 @@ class ExtensionSetting extends StatelessWidget { 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 072e8049905..e514582db5f 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 @@ extension StubDevToolsExtensions on Never { /// 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 @@ extension StubDevToolsExtensions on Never { /// 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 @@ extension StubDevToolsExtensions on Never { /// 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 @@ extension StubDevToolsExtensions on Never { /// 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 StubDevToolsExtensions on Never { /// 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 @@ extension StubDevToolsExtensions on Never { /// 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 @@ extension StubDevToolsExtensions on Never { /// 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 5933894a6e3..290abeacd49 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> refreshAvailableExtensions( Future 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 @@ Future extensionEnabledState({ 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 ca70b2cd925..9d13816220c 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 @@ TODO: Remove this section if there are not any updates. [#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 306a0dc16c7..f0f9cc056de 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 @@ void main() { 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 @@ void main() { 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 52f99d9d310..fbf0129b6f4 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 @@ void main() { // 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 @@ void main() { }); 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 @@ void main() { // 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 @@ void main() { }); 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 @@ void main() { // 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 @@ void main() { }); 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 @@ void main() { expect(takeLatestExtension(a, b), a); }); }); + + group('deduplicateExtensionsAndTakeLatest', () { + test('deduplicates matching packageName', () { + final ignored = {}; + 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 = {}; + 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 d8f61cb6f61..fea1cc26ace 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 @@ void main() { 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 83756dfe4eb..9115a73747c 100644 --- a/packages/devtools_app_shared/CHANGELOG.md +++ b/packages/devtools_app_shared/CHANGELOG.md @@ -10,6 +10,7 @@ found in the LICENSE file or at https://developers.google.com/open-source/licens * 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 10437f6534b..9f41284e3dd 100644 --- a/packages/devtools_app_shared/pubspec.yaml +++ b/packages/devtools_app_shared/pubspec.yaml @@ -15,7 +15,7 @@ resolution: workspace 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 2d412aec0ac..5123faeaeb9 100644 --- a/packages/devtools_extensions/CHANGELOG.md +++ b/packages/devtools_extensions/CHANGELOG.md @@ -6,6 +6,8 @@ found in the LICENSE file or at https://developers.google.com/open-source/licens ## 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 a0542f3c6e7..30f36cc1b66 100644 --- a/packages/devtools_extensions/bin/_validate.dart +++ b/packages/devtools_extensions/bin/_validate.dart @@ -53,6 +53,7 @@ class ValidateExtensionCommand extends Command { ..._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 @@ void _validateDirectoryContents(String packagePath) { 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 @@ An extension/devtools/config.yaml file is required, but none was found. 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 _configAsMap(String packagePath) { diff --git a/packages/devtools_extensions/pubspec.yaml b/packages/devtools_extensions/pubspec.yaml index df7ce763e48..4639c9cbcc4 100644 --- a/packages/devtools_extensions/pubspec.yaml +++ b/packages/devtools_extensions/pubspec.yaml @@ -18,7 +18,7 @@ executables: 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 8efcb5e0849..3616638041d 100644 --- a/packages/devtools_extensions/test/validate_test.dart +++ b/packages/devtools_extensions/test/validate_test.dart @@ -53,4 +53,44 @@ void main() { }); } }); + + 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 132ea2a0590..f5c4069e1bc 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -3,6 +3,11 @@ Copyright 2025 The Flutter Authors 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 f28ffbbf0a5..fd2ca9338ec 100644 --- a/packages/devtools_shared/lib/src/devtools_api.dart +++ b/packages/devtools_shared/lib/src/devtools_api.dart @@ -136,6 +136,11 @@ abstract class ExtensionsApi { /// 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 d6a12670ce4..d86c03ca05d 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 @@ $_documentationKey: https://docs.flutter.dev/tools/devtools/extensions#configure $_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 @@ $_extensionsKey: 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 @@ $_extensionsKey: ?.cast>(); 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 @@ $_extensionsKey: ExtensionEnabledState setExtensionEnabledState({ required Uri devtoolsOptionsUri, required String extensionName, + String? packageName, required bool enable, }) { final options = _optionsAsMap(optionsUri: devtoolsOptionsUri); @@ -73,14 +77,16 @@ $_extensionsKey: extensions = options[_extensionsKey] as List>; } + 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 @@ $_extensionsKey: 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 92ad6016d22..90da58f2f21 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 @@ class ExtensionsManager { 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 @@ class ExtensionsManager { 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 8d669e0134c..4cb5b22998a 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 @@ import 'package:collection/collection.dart'; class DevToolsExtensionConfig implements Comparable { DevToolsExtensionConfig._({ required this.name, + required this.packageName, required this.issueTrackerLink, required this.version, required this.materialIconCodePoint, @@ -42,6 +43,7 @@ class DevToolsExtensionConfig implements Comparable { // 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 @@ class DevToolsExtensionConfig implements Comparable { // 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 @@ class DevToolsExtensionConfig implements Comparable { // 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 @@ class DevToolsExtensionConfig implements Comparable { 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 toJson() => { nameKey: name, + packageNameKey: packageName, issueTrackerKey: issueTrackerLink, versionKey: version, materialIconCodePointKey: materialIconCodePoint, @@ -248,11 +262,14 @@ class DevToolsExtensionConfig implements Comparable { @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 @@ class DevToolsExtensionConfig implements Comparable { 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 @@ class DevToolsExtensionConfig implements Comparable { @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 22dded4394a..035ccaa4600 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 @@ extension _ExtensionsApiHandler on Never { } 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 @@ extension _ExtensionsApiHandler on Never { .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 ff121888ba3..4507c5e3cfc 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 36984b308d6..db841b76b34 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 @@ extensions: 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 2bf4236ca6f..3a98e0445b8 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 @@ void main() { 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 @@ void main() { }); 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 @@ void main() { 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 @@ void main() { }); 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 @@ void main() { 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 @@ void main() { }); 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 @@ void main() { 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 @@ void main() { }); 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 @@ void main() { 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 @@ void main() { test('name', () { expect(() { DevToolsExtensionConfig.parse({ + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -131,6 +170,7 @@ void main() { expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, 'extensionAssetsPath': '/absolute/path/to/foo/extension', @@ -146,6 +186,7 @@ void main() { expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'materialIconCodePoint': 0xf012, 'extensionAssetsPath': '/absolute/path/to/foo/extension', @@ -161,6 +202,7 @@ void main() { 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 @@ void main() { }); }, 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 @@ void main() { expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -204,6 +265,7 @@ void main() { expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -218,6 +280,7 @@ void main() { expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -245,6 +308,7 @@ void main() { DevToolsExtensionConfig.parse({ // Expects a String here. 'name': 23, + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -258,6 +322,7 @@ void main() { expect(() { DevToolsExtensionConfig.parse({ 'name': 'foo', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -284,6 +349,7 @@ void main() { expect(() { DevToolsExtensionConfig.parse({ 'name': 'name with spaces', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -297,6 +363,7 @@ void main() { expect(() { DevToolsExtensionConfig.parse({ 'name': 'Name_With_Capital_Letters', + 'packageName': 'foo', 'issueTracker': 'www.google.com', 'version': '1.0.0', 'materialIconCodePoint': 0xf012, @@ -310,6 +377,7 @@ void main() { 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 323ab819c83..00add39d1ca 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 @@ class ExtensionTestManager { Future 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, + includeSpoofedExtension: includeSpoofedExtension, ); - _setupExtensions(includeBadExtension: includeBadExtension); // Generate the .dart_tool/package_config.json file for each Dart package. final testDirectoryContents = testDirectory.listSync(); @@ -144,10 +149,19 @@ class ExtensionTestManager { 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 @@ resolution: workspace /// 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 @@ resolution: workspace _setupExtension(newerStaticExtension1Package); if (includeBadExtension) _setupExtension(badExtensionPackage); + if (includeSpoofedExtension) _setupExtension(spoofedExtensionPackage); } void _setupPackage(TestPackage package, {bool isRuntimeRoot = false}) { @@ -304,6 +322,10 @@ final myAppPackageWithBadExtension = TestPackage( 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 workspaceMember = TestPackage( 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 driftPackage = TestPackageWithExtension( ); 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 providerPackage = TestPackageWithExtension( ); 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 staticExtension1Package = TestPackageWithExtension( ); 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 staticExtension2Package = TestPackageWithExtension( ); 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 newerStaticExtension1Package = TestPackageWithExtension( 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 @@ final badExtensionPackage = TestPackageWithExtension( 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 @@ class TestPackageWithExtension { 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 @@ ${!requiresConnection ? 'requiresConnection: false' : ''} String get pubspecContent => ''' -name: ${name.toLowerCase()} +name: $packageName environment: sdk: ">=3.4.0-282.1.beta <4.0.0" '''; @@ -442,7 +481,7 @@ ${_dependenciesAsString()} 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 e8101458fd9..c49d6e7b4c1 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 @@ void main() { 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 @@ void main() { test('$providerPackage', () { expect(providerPackage.name, 'provider'); + expect(providerPackage.packageName, 'provider'); expect( providerPackage.issueTracker, 'https://github.com/rrousselGit/provider/issues', @@ -41,6 +43,7 @@ void main() { 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 @@ requiresConnection: false 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 @@ requiresConnection: false 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 @@ requiresConnection: false 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 @@ requiresConnection: false expect(badExtensionPackage.packageVersion, null); expect( badExtensionPackage.relativePathFromExtensions, - badExtensionPackage.name.toLowerCase(), + badExtensionPackage.packageName, ); expect(badExtensionPackage.pubspecContent, ''' name: bad_extension @@ -143,6 +149,33 @@ issueTracker: https://www.google.com/ version: 1.0.0 materialIconCodePoint: 58634 +'''); + }); + + 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 + '''); }); }); 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 af0530ed512..ca0d9059dd1 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 @@ void main() { Future 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 @@ void main() { ); }); + 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 @@ void _verifyExtension( 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);