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);