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 7 commits 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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
| return DevToolsExtensionConfig._( | ||
| // These values are required fields in the extension's config.yaml file. | ||
| name: name, | ||
| packageName: packageName, |
There was a problem hiding this comment.
the comment above name is intended for all keys until the next comment on line 82, so package should not go here unless we are expecting users to manually enter this in the extension config.yaml file.
|
|
||
| // Spoofed package does not inherit provider's enablement | ||
| expect( | ||
| options.lookupExtensionEnabledState( |
There was a problem hiding this comment.
is this going to be backwards compatible for existing devtools_options.yaml entries from before this change?
There was a problem hiding this comment.
Yes, for most packages (like patrol or provider), the packageName field (from .dart_tool/package_config.json') matches the name` field (the package name the extension is declared to before in config.yaml).
There was a problem hiding this comment.
If they don't match, then users will need to re-enable the extension after this change lands, because DevToolsExtensionConfig.identifier will be put in devtools_options.yaml file instead.
| // Write the new enabled state to the map. | ||
| final extension = extensions.firstWhereOrNull( | ||
| (e) => e.keys.first == extensionName, | ||
| (e) => e.keys.first == targetKey, |
There was a problem hiding this comment.
won't we end up in a situation where users who already have an extension enabled in their devtools options file, call if foo_ext provided by package foo will now have two keys in their devtools_options.yaml file:
foo.foo_ext and foo_ext since targetKey is now foo.foo_ext?
There was a problem hiding this comment.
Yes, that's possible.
| throw FileSystemException('${packageDirectory.path} directory not found'); | ||
| } | ||
|
|
||
| final pubspecFile = File(path.join(packageDirectory.path, 'pubspec.yaml')); |
There was a problem hiding this comment.
the additional validations added to this file seem useful, but they also seem out of scope of this PR. Are these somehow related to resolving the security bug?
There was a problem hiding this comment.
I think AGY added this because extensions need to be included in a Dart package, right?
| codePoint = codePointFromJson as int; | ||
| } | ||
|
|
||
| final packageName = json[packageNameKey] as String? ?? name; |
There was a problem hiding this comment.
where is the packageName getting set in the json? Maybe I'm missing it in this PR.
There was a problem hiding this comment.
In extension_manager.dart L191, the ExtensionsManager class puts this key into DevToolsExtensionConfig manually when detecting the available extensions.
| /// This value is parsed from the package name in | ||
| /// `.dart_tool/package_config.json`. | ||
| final String packageName; |
There was a problem hiding this comment.
related to my last comment. Where is this parsing code?
There was a problem hiding this comment.
My understanding is that this is parsed from the package_config.json file in the extension_discovery package, and then ExtensionsManager puts this value into DevToolsExtensionConfig.
2 participants
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