Make sure that extensions are bundled with the package they claim to be. - #9981
Make sure that extensions are bundled with the package they claim to be.#9981johnpryan wants to merge 1 commit into
Conversation
With this change, extensions are enabled if and only if the package name on disk matches the package name in extension/devtools/config.yaml.
There was a problem hiding this comment.
Code Review
This pull request improves DevTools extension isolation by tracking the providing package name for enablement, deduplication, and asset loading, and adds validation checks for extension names. The review feedback highlights a compilation error in _extensions_api.dart due to invalid map literal syntax, a potential runtime TypeError in _validate.dart when casting the configuration name, and a suggestion to normalize packageRoot in extension_manager.dart for more robust path comparisons.
| ExtensionsApi.extensionPackagePropertyName: ?extensionPackage, | ||
| ExtensionsApi.enabledStatePropertyName: ?enable?.toString(), |
There was a problem hiding this comment.
[MUST-FIX] The syntax '?expression' is invalid in Dart map literals and will cause a compilation error. Use conditional elements ('if (condition)') to conditionally include these query parameters, matching the pattern used previously.
if (extensionPackage != null)
ExtensionsApi.extensionPackagePropertyName: extensionPackage,
if (enable != null)
ExtensionsApi.enabledStatePropertyName: enable.toString(),References
- Prefix every comment with a severity: [MUST-FIX] for logical bugs, [CONCERN] for maintainability issues. (link)
| final configName = configYaml['name'] as String?; | ||
| final underscoresAndLetters = RegExp(r'^[a-z0-9_]*$'); | ||
| if (configName == null || !underscoresAndLetters.hasMatch(configName)) { |
There was a problem hiding this comment.
[CONCERN] Casting configYaml['name'] directly to String? using 'as String?' will throw a TypeError at runtime if the value in config.yaml is of another type (e.g., an integer or boolean). It is safer to perform a type check ('is! String') to throw a descriptive StateError instead.
| final configName = configYaml['name'] as String?; | |
| final underscoresAndLetters = RegExp(r'^[a-z0-9_]*$'); | |
| if (configName == null || !underscoresAndLetters.hasMatch(configName)) { | |
| final configName = configYaml['name']; | |
| final underscoresAndLetters = RegExp(r'^[a-z0-9_]*$'); | |
| if (configName is! String || !underscoresAndLetters.hasMatch(configName)) { |
References
- Prefix every comment with a severity: [MUST-FIX] for logical bugs, [CONCERN] for maintainability issues. (link)
| 'extension', | ||
| 'devtools', | ||
| relativeExtensionLocation, | ||
| final packageRoot = extension.rootUri.toFilePath(); |
There was a problem hiding this comment.
[CONCERN] To ensure that path comparisons using 'path.isWithin' are robust and behave consistently across different platforms and environments, it is safer to normalize 'packageRoot' as well.
| final packageRoot = extension.rootUri.toFilePath(); | |
| final packageRoot = path.normalize(extension.rootUri.toFilePath()); |
References
- Prefix every comment with a severity: [MUST-FIX] for logical bugs, [CONCERN] for maintainability issues. (link)
With this change, extensions are enabled if and only if the package name on disk matches the package name in extension/devtools/config.yaml.
This supersedes #9965