[Extensions] Disable browser namespace for extensions with devtools [chromium/src : main]
Justin Lulejian (Gerrit)
Justin Lulejian added 1 comment![]( "Open in Gerrit")
Hi Devlin! Keeping it consistent with devtools related CLs 😄 I use the manifest key in the extension to detect if the extension could use the devtools API. Do you think that is robust enough of a check?
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 5 comments![]( "Open in Gerrit")
Thanks, Justin! This mostly looks good, but a couple comments
bool IsExtensionBrowserNamespaceAndPolyfillSupportEnabledForExtension(I think this would be cleaner putting it more proximal to the code that uses it; I worry that otherwise, extension_features will explode in size (if everything puts logic related to the _application_ of the feature here)
!manifest_values.Find(extensions::manifest_keys::kDevToolsPage);I think using `!GetDevToolsPage(extension).is_empty()` is maybe more robust. That would validate that the value is present in the manifest, can be used by the extension (i.e., is available), and is valid.
return script_context && script_context->extension() &&this means it's not enabled for non-extension contexts -- this controls the promise-based returns of messaging, right? If so, I think we also want that in web contexts?
bool IsMessagePolyfillSupportEnabled(ScriptContext& script_context) {nit: we should probably just update callers to adapt them to use the variant above
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian added 4 comments![]( "Open in Gerrit")
bool IsExtensionBrowserNamespaceAndPolyfillSupportEnabledForExtension(I think this would be cleaner putting it more proximal to the code that uses it; I worry that otherwise, extension_features will explode in size (if everything puts logic related to the _application_ of the feature here)
Done
!manifest_values.Find(extensions::manifest_keys::kDevToolsPage);I think using `!GetDevToolsPage(extension).is_empty()` is maybe more robust. That would validate that the value is present in the manifest, can be used by the extension (i.e., is available), and is valid.
Done
return script_context && script_context->extension() &&this means it's not enabled for non-extension contexts -- this controls the promise-based returns of messaging, right? If so, I think we also want that in web contexts?
Done
bool IsMessagePolyfillSupportEnabled(ScriptContext& script_context) {nit: we should probably just update callers to adapt them to use the variant above
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
if (extension) {
return IsExtensionBrowserNamespaceAndPolyfillSupportEnabledForExtension(
extension);
}
// For example a non-extension webpage that can communicate with extensions.
return base::FeatureList::IsEnabled(
extensions_features::kExtensionBrowserNamespaceAndPolyfillSupport);optional: I'd use a ternary here to save a couple lines, but up to you
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 4 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
4 comments
url::Origin::Create(
chrome_manifest_urls::GetDevToolsPage(extension)) &&nit: prefer just using extension->origin() (it should be guaranteed to be the same origin as the devtools page)
.SetManifestVersion(3)nit: unnecessary (this is the default)
EXPECT_TRUE(browser->IsUndefined());optional: could verify that `chrome` is available as a safety check
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 5 comments![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Auto-Submit | +1 |
5 comments
Hi Devlin! Unfortunately the upstream changes seem to have subtracted the +1, but there's little significant diff between patchset 7 and 9.
if (extension) {
return IsExtensionBrowserNamespaceAndPolyfillSupportEnabledForExtension(
extension);
}
// For example a non-extension webpage that can communicate with extensions.
return base::FeatureList::IsEnabled(
extensions_features::kExtensionBrowserNamespaceAndPolyfillSupport);optional: I'd use a ternary here to save a couple lines, but up to you
I love a ternary but curiously enough it ends up adding a couple lines due to the length of the method/feature names and format wrapping (at least when I tried `return extension ? ...;`) so I'll stick with it as-is in this case.
url::Origin::Create(
chrome_manifest_urls::GetDevToolsPage(extension)) &&nit: prefer just using extension->origin() (it should be guaranteed to be the same origin as the devtools page)
Fix in upstream https://crrev.com/c/7499734
nit: unnecessary (this is the default)
Removed this test upstream since it wasn't going to work as a unit test after I made the new devtools context check.
optional: could verify that `chrome` is available as a safety check
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
9 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[Extensions] Disable browser namespace for extensions with devtools
Before this change the kExtensionBrowserNamespaceAndPolyfillSupport
feature, when enabled, applied to all extensions. This caused an issue
because some extensions using the (webextension
polyfill](https://github.com/mozilla/webextension-polyfill) were using
it to allow promise returns from devtools API calls that are not
supported by the API itself. When the
kExtensionBrowserNamespaceAndPolyfillSupport turned on it would no-op
the webextension-polyfill and these calls would be syntax errors.
After this change we use
IsExtensionBrowserNamespaceAndPolyfillSupportEnabledForExtension() to
disable the feature for extension script contexts when the extension
has a devtools_page (and could use the devtools API). This restricts
the browser namespace and polyfill support from being enabled for all
script contexts for these specific extensions.
Also:
* updates NativeExtensionBindingsSystem and OneTimeMessageHandler to
use this extension-specific check.
* updates tests to reflect that browser is undefined in these
contexts and disables the DevToolsApiAliasing test since it's not
relevant until this exception is eventually removed.
Bug: 401226626,439644930,40753031
in prog devtools bypass
- M chrome/browser/extensions/native_bindings_apitest.cc
- M extensions/renderer/BUILD.gn
- M extensions/renderer/api/messaging/one_time_message_handler.cc
- M extensions/renderer/native_extension_bindings_system.cc
- A extensions/renderer/polyfill_util.cc
- A extensions/renderer/polyfill_util.h
Code-Review: +1 by Devlin Cronin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |