From 1c9d477a327cdb22f3335379d6b93987579275a8 Mon Sep 17 00:00:00 2001 From: John Ryan Date: Wed, 26 Aug 2026 14:38:33 -0700 Subject: [PATCH] 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. --- .../lib/src/extensions/extension_service.dart | 16 ++-- .../extensions/extension_service_helpers.dart | 9 +- .../src/shared/server/_extensions_api.dart | 7 +- .../release_notes/NEXT_RELEASE_NOTES.md | 3 + .../extension_service_helpers_test.dart | 90 +++++++++++++++++++ packages/devtools_app_shared/CHANGELOG.md | 1 + packages/devtools_app_shared/pubspec.yaml | 2 +- packages/devtools_extensions/CHANGELOG.md | 1 + .../devtools_extensions/bin/_validate.dart | 19 ++++ packages/devtools_extensions/pubspec.yaml | 2 +- .../test/validate_test.dart | 40 +++++++++ packages/devtools_shared/CHANGELOG.md | 6 +- .../devtools_shared/lib/src/devtools_api.dart | 5 ++ .../src/extensions/extension_enablement.dart | 56 ++++++++++-- .../lib/src/extensions/extension_manager.dart | 29 ++++-- .../lib/src/extensions/extension_model.dart | 35 ++++++-- .../server/handlers/_devtools_extensions.dart | 4 + packages/devtools_shared/pubspec.yaml | 2 +- .../extensions/extension_enablement_test.dart | 45 ++++++++++ .../test/extensions/extension_model_test.dart | 37 ++++++++ .../test/helpers/extension_test_manager.dart | 49 ++++++++-- .../server/devtools_extensions_api_test.dart | 38 ++++++++ 22 files changed, 456 insertions(+), 40 deletions(-) diff --git a/packages/devtools_app/lib/src/extensions/extension_service.dart b/packages/devtools_app/lib/src/extensions/extension_service.dart index ae49a3b7022..ff60f690394 100644 --- a/packages/devtools_app/lib/src/extensions/extension_service.dart +++ b/packages/devtools_app/lib/src/extensions/extension_service.dart @@ -232,7 +232,9 @@ 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 && + ext.name == staticExtension.name, ); if (isRuntimeDuplicate) { _log.fine( @@ -256,6 +258,7 @@ class ExtensionService extends DisposableController final stateFromOptionsFile = await server.extensionEnabledState( devtoolsOptionsFileUri: extension.devtoolsOptionsUri, extensionName: extension.name, + extensionPackage: extension.packageName, ); final stateNotifier = _extensionEnabledStates.putIfAbsent( extension.name, @@ -292,15 +295,18 @@ class ExtensionService extends DisposableController // Set the enabled state for all matching extensions, even if some are // marked as ignored due to being a duplicate. This ensures that // devtools_options.yaml files are kept in sync across the project. - final allMatchingExtensions = [ - ...runtimeExtensions, - ...staticExtensions, - ].where((e) => e.name == extension.name); + final allMatchingExtensions = [...runtimeExtensions, ...staticExtensions] + .where( + (e) => + e.packageName == extension.packageName && + e.name == extension.name, + ); 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..f439a89effc 100644 --- a/packages/devtools_app/lib/src/extensions/extension_service_helpers.dart +++ b/packages/devtools_app/lib/src/extensions/extension_service_helpers.dart @@ -19,11 +19,14 @@ void deduplicateExtensionsAndTakeLatest( }) { final deduped = {}; for (final ext in extensions) { - if (deduped.contains(ext.name)) continue; - deduped.add(ext.name); + final dedupKey = '${ext.packageName}:${ext.name}'; + if (deduped.contains(dedupKey)) continue; + deduped.add(dedupKey); // This includes [ext] itself. - final matchingExtensions = extensions.where((e) => e.name == ext.name); + final matchingExtensions = extensions.where( + (e) => e.packageName == ext.packageName && e.name == ext.name, + ); if (matchingExtensions.length > 1) { logger?.fine( 'detected duplicate $extensionType extensions for ${ext.name}', 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..f20ffcc1de6 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, + 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,8 +93,8 @@ Future extensionEnabledState({ queryParameters: { ExtensionsApi.devtoolsOptionsUriPropertyName: devtoolsOptionsFileUri, ExtensionsApi.extensionNamePropertyName: extensionName, - if (enable != null) - ExtensionsApi.enabledStatePropertyName: enable.toString(), + ExtensionsApi.extensionPackagePropertyName: ?extensionPackage, + ExtensionsApi.enabledStatePropertyName: ?enable?.toString(), }, ); final resp = await request(uri.toString()); diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index 01ad0a7d9e2..f8027918973 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md @@ -86,6 +86,9 @@ TODO: Remove this section if there are not any updates. * Hide the DevTools extensions menu button in single-screen embedded mode (`EmbedMode.embedOne`) on standard screens. [#8507](https://github.com/flutter/devtools/issues/8507) +* Improved DevTools extension isolation by tracking the providing package name for + enablement, deduplication, and asset loading. + [#9965](https://github.com/flutter/devtools/pull/9965) ## Advanced developer mode updates 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..c3222414ab8 100644 --- a/packages/devtools_app/test/extensions/extension_service_helpers_test.dart +++ b/packages/devtools_app/test/extensions/extension_service_helpers_test.dart @@ -101,4 +101,94 @@ void main() { expect(takeLatestExtension(a, b), a); }); }); + + group('deduplicateExtensionsAndTakeLatest', () { + test('deduplicates matching packageName and name', () { + 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_shared/CHANGELOG.md b/packages/devtools_app_shared/CHANGELOG.md index 7dadf83d6ea..11bbf51bc00 100644 --- a/packages/devtools_app_shared/CHANGELOG.md +++ b/packages/devtools_app_shared/CHANGELOG.md @@ -9,6 +9,7 @@ found in the LICENSE file or at https://developers.google.com/open-source/licens * Fix garbage collection issues with the result list in `asyncEval` on both native VM and web. * 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 8afa9443e83..706aa2c1f64 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..61730a27689 100644 --- a/packages/devtools_extensions/CHANGELOG.md +++ b/packages/devtools_extensions/CHANGELOG.md @@ -6,6 +6,7 @@ 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`. ## 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..04e4668e7c6 100644 --- a/packages/devtools_extensions/bin/_validate.dart +++ b/packages/devtools_extensions/bin/_validate.dart @@ -78,6 +78,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 +117,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'] as String?; + final underscoresAndLetters = RegExp(r'^[a-z0-9_]*$'); + if (configName == null || !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..1d16d6dbcdb 100644 --- a/packages/devtools_shared/CHANGELOG.md +++ b/packages/devtools_shared/CHANGELOG.md @@ -1,8 +1,12 @@ +# 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..f251c544d11 100644 --- a/packages/devtools_shared/lib/src/extensions/extension_enablement.dart +++ b/packages/devtools_shared/lib/src/extensions/extension_enablement.dart @@ -30,9 +30,17 @@ $_extensionsKey: /// with an empty set of extensions. /// /// [devtoolsOptionsUri] is expected to be a file:// URI. + /// Returns the current enabled state for [extensionName] (and optionally + /// [packageName]) 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. + /// + /// [devtoolsOptionsUri] is expected to be a file:// URI. ExtensionEnabledState lookupExtensionEnabledState({ required Uri devtoolsOptionsUri, required String extensionName, + String? packageName, }) { final options = _optionsAsMap(optionsUri: devtoolsOptionsUri); if (options == null) return ExtensionEnabledState.error; @@ -41,19 +49,26 @@ $_extensionsKey: ?.cast>(); if (extensions == null) return ExtensionEnabledState.none; + final lookupKeys = _enablementLookupKeys( + extensionName: extensionName, + packageName: packageName, + ); + 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]); + for (final key in lookupKeys) { + if (e.keys.first == key) { + return _extensionStateForValue(e[key]); + } } } return ExtensionEnabledState.none; } - /// Sets the enabled state for [extensionName] in the - /// 'devtools_options.yaml' file at [devtoolsOptionsUri]. + /// Sets the enabled state for [extensionName] (and optionally [packageName]) + /// in the 'devtools_options.yaml' file at [devtoolsOptionsUri]. /// /// If the 'devtools_options.yaml' file does not exist, it will be created. /// @@ -61,6 +76,7 @@ $_extensionsKey: ExtensionEnabledState setExtensionEnabledState({ required Uri devtoolsOptionsUri, required String extensionName, + String? packageName, required bool enable, }) { final options = _optionsAsMap(optionsUri: devtoolsOptionsUri); @@ -73,14 +89,19 @@ $_extensionsKey: extensions = options[_extensionsKey] as List>; } + final targetKey = _primaryEnablementKey( + extensionName: extensionName, + packageName: packageName, + ); + // 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,9 +111,30 @@ $_extensionsKey: return lookupExtensionEnabledState( devtoolsOptionsUri: devtoolsOptionsUri, extensionName: extensionName, + packageName: packageName, ); } + static String _primaryEnablementKey({ + required String extensionName, + String? packageName, + }) { + if (packageName == null || packageName == extensionName) { + return extensionName; + } + return '$packageName.$extensionName'; + } + + static List _enablementLookupKeys({ + required String extensionName, + String? packageName, + }) { + if (packageName == null || packageName == extensionName) { + return [extensionName]; + } + return ['$packageName.$extensionName', packageName]; + } + /// Returns the content of the `devtools_options.yaml` file at [optionsUri] /// as a Map. Map? _optionsAsMap({required Uri optionsUri}) { diff --git a/packages/devtools_shared/lib/src/extensions/extension_manager.dart b/packages/devtools_shared/lib/src/extensions/extension_manager.dart index 92ad6016d22..fc4b59c6ab4 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 = 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..6deb9c0df95 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, @@ -69,9 +70,12 @@ class DevToolsExtensionConfig implements Comparable { codePoint = codePointFromJson as int; } + final packageName = json[packageNameKey] as String? ?? name; + return DevToolsExtensionConfig._( // These values are required fields in the extension's config.yaml file. name: name, + packageName: packageName, issueTrackerLink: issueTracker, version: version, materialIconCodePoint: codePoint, @@ -130,22 +134,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 +242,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 +263,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 +280,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 +294,7 @@ class DevToolsExtensionConfig implements Comparable { @override int get hashCode => Object.hash( name, + packageName, issueTrackerLink, version, materialIconCodePoint, @@ -288,6 +308,9 @@ class DevToolsExtensionConfig implements Comparable { static void _assertGeneratedKeysPresent(Map json) { final missingKeys = []; for (final key in _serverGeneratedKeys) { + if (key == packageNameKey) { + continue; // Optional for backwards compatibility. + } if (!json.containsKey(key)) { missingKeys.add(key); } 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..58ff48cc1f6 100644 --- a/packages/devtools_shared/test/extensions/extension_enablement_test.dart +++ b/packages/devtools_shared/test/extensions/extension_enablement_test.dart @@ -103,5 +103,50 @@ 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 + 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, + ); + }); }); } diff --git a/packages/devtools_shared/test/extensions/extension_model_test.dart b/packages/devtools_shared/test/extensions/extension_model_test.dart index 2bf4236ca6f..a8a9dd62688 100644 --- a/packages/devtools_shared/test/extensions/extension_model_test.dart +++ b/packages/devtools_shared/test/extensions/extension_model_test.dart @@ -84,6 +84,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 +92,42 @@ 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': 'provider', + 'packageName': 'provider_devtools_extension', + '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, + 'provider_devtools_extension', + ); + expect( + configWithDistinctPackage.identifier, + 'provider_devtools_extension_provider_1.0.0', + ); + }); + group('parse throws when missing required field', () { Matcher throwsMissingRequiredFieldsError() { return throwsA( diff --git a/packages/devtools_shared/test/helpers/extension_test_manager.dart b/packages/devtools_shared/test/helpers/extension_test_manager.dart index 323ab819c83..54a0dba9d31 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], @@ -374,10 +396,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, + String? packageName, required this.issueTracker, required this.version, required this.materialIconCodePoint, @@ -385,11 +418,13 @@ class TestPackageWithExtension { required this.isPubliclyHosted, required this.packageVersion, String? relativePathFromExtensions, - }) : assert(isPubliclyHosted == (packageVersion != null)), + }) : packageName = packageName ?? name.toLowerCase(), + assert(isPubliclyHosted == (packageVersion != null)), relativePathFromExtensions = - relativePathFromExtensions ?? name.toLowerCase(); + relativePathFromExtensions ?? (packageName ?? name.toLowerCase()); final String name; + final String packageName; final String issueTracker; final String version; final Object? materialIconCodePoint; @@ -413,7 +448,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 +477,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/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);