Decentralize extension Mojo interface IPC reviews [chromium/src : main]
Moe Ahmadi (Gerrit)
New activity on the change![]( "Open in Gerrit")
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 7 comments![]( "Open in Gerrit")
Thanks, Moe! Generally looks good, but a couple comments here
nit: generally, no \n between override methods (possible exception for the dtor)
// Additions to these overloads require review from IPC_SECURITY_OWNERS.extend:
```
// These effectively allow exposing additional mojo interfaces to
// different renderers. As such, additions to these overloads require
// review from IPC_SECURITY_OWNERS.
// New BinderProvider files must also have a corresponding OWNERS files
// restricting reviews to IPC_SECURITY_OWNERS.
```
using PassKey = base::PassKey<ExtensionMojoBinderRegistry>;unused
#include <string>I think we can clean up some of these includes
if (!extension || !Manifest::IsComponentLocation(extension->location())) {semi-related to the comment below, can we also add a call to IsMojoJsEnabledForExtension() here? (Unless the intention is to remove that and fold it into this class)
I know right now it's superfluous, but I'd like to make sure they stay in sync
if (!extension || !Manifest::IsComponentLocation(extension->location())) {
return;
}
if (render_frame_host &&
util::GetExtensionIdForSiteInstance(
*render_frame_host->GetSiteInstance()) != extension->id()) {
return;
}
auto it = providers_.find(extension->id());
if (it == providers_.end()) {
return;
}these checks are quite similar between this method and PopulateServiceWorkerBinders. Can we extract (the non-RFH-based) checks to a helper method?
Related details
- Moe Ahmadi
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. |
Moe Ahmadi (Gerrit)
Moe Ahmadi voted and added 7 comments![]( "Open in Gerrit")
Votes added by Moe Ahmadi
| Commit-Queue | +1 |
Thanks, Moe! Generally looks good, but a couple comments here
Thanks for the comments!
nit: generally, no \n between override methods (possible exception for the dtor)
Done
// Additions to these overloads require review from IPC_SECURITY_OWNERS.extend:
```
// These effectively allow exposing additional mojo interfaces to
// different renderers. As such, additions to these overloads require
// review from IPC_SECURITY_OWNERS.
// New BinderProvider files must also have a corresponding OWNERS files
// restricting reviews to IPC_SECURITY_OWNERS.
```
Done. Added your suggested comment.
using PassKey = base::PassKey<ExtensionMojoBinderRegistry>;Moe Ahmadiunused
Done
I think we can clean up some of these includes
Done. Removed unused headers
if (!extension || !Manifest::IsComponentLocation(extension->location())) {semi-related to the comment below, can we also add a call to IsMojoJsEnabledForExtension() here? (Unless the intention is to remove that and fold it into this class)
I know right now it's superfluous, but I'd like to make sure they stay in sync
Excellent point. Folded `IsMojoJsEnabledForExtension()` into the registry and updated extension_util.cc to delegate to it. lmk what you think!
if (!extension || !Manifest::IsComponentLocation(extension->location())) {
return;
}
if (render_frame_host &&
util::GetExtensionIdForSiteInstance(
*render_frame_host->GetSiteInstance()) != extension->id()) {
return;
}
auto it = providers_.find(extension->id());
if (it == providers_.end()) {
return;
}these checks are quite similar between this method and PopulateServiceWorkerBinders. Can we extract (the non-RFH-based) checks to a helper method?
- Chromium IPC Reviews
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ort...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ort...@chromium.org
Reviewer source(s):
ort...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Devlin Cronin
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
Is there anyone from IPC reviewers that is more familiar with this effort that would be more appropriate to review this CL? Looks like Nasko might have reviewed the precursor CL? If not, any design docs or previous discussions?
Related details
- Devlin Cronin
- Moe Ahmadi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 1 comment![]( "Open in Gerrit")
Is there anyone from IPC reviewers that is more familiar with this effort that would be more appropriate to review this CL? Looks like Nasko might have reviewed the precursor CL? If not, any design docs or previous discussions?
Hey Giovanni! Nasko has more context on what we are doing here but is OOO. Please see:
- http://shortn/_I93mjko7vd, DD for the specific project that requires this
- http://shortn/_lj7Rhh3aae, high-level direction why we are enabling Mojo for extensions.
- crrev.com/c/8092669, preceding CL for additional context.
Related details
- Devlin Cronin
- Giovanni Ortuno Urquidi
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
| Code-Review | +1 |
1 comment
- Giovanni Ortuno Urquidi
- Moe Ahmadi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 8 comments![]( "Open in Gerrit")
per-file *binder_provider*.*=set noparent
per-file *binder_provider*.*=file://ipc/SECURITY_OWNERSI think it's too easy to forget to add this. Can we add `_extension_binder_provider.h/.cc` to https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;l=4720;drc=01c9ad7fe24638f468ec728736fd4cd74abcd9b4 ?
base::BindRepeating(optional: Can you add a static method to AimEligibilityExtensionBridge to avoid the big inlined lambda?
auto* bridge = AimEligibilityExtensionBridge::Get(When would there be an interface request for this interface where `bridge` is nullptr? I feel like we should crash the renderer in that case, but I could be missing a case?
Actually, can we avoid registering the provider if there's no AimEligibilityExtensionBridge? Then we can CHECK here.
void RegisterProvider(
base::PassKey<AimEligibilityExtensionBinderProvider> passkey,
std::unique_ptr<ExtensionMojoBinderProvider> provider);
void RegisterProvider(base::PassKey<ExtensionMojoBinderRegistryTest> passkey,
std::unique_ptr<ExtensionMojoBinderProvider> provider);I *think* you could do the following here:
```
template <typename T>
void RegisterProvider(base::PassKey<T> passkey,
std::unique_ptr<ExtensionMojoBinderProvider> provider)
```
And then in the .cc:
```
template <>
void ExtensionMojoBinderRegistry::RegisterProvider(
base::PassKey<AimEligibilityExtensionBinderProvider>,
std::unique_ptr<ExtensionMojoBinderProvider> provider) {
...
}
```
That way developers only need to modify one file. If this works, maybe worth moving the comment to the .cc.
// New BinderProvider files must also have a corresponding OWNERS files
// restricting reviews to IPC_SECURITY_OWNERS.If we add _extension_binder_provider to the presubmit, then that would enforce this. Maybe we can change this to mention that the provider should end with _extension_binder_provider.h/cc?
DCHECK(binder_map);nit: Prefer CHECKs over DCHECKs.
DCHECK(binder_map);nit: Prefer CHECK over DCHECK for new code: https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md#invariant_verification-mechanisms
EXPECT_FALSE(registry()->IsMojoJsEnabled(component_extension.get()));nit: `IsMojoEnabled(nullptr)`
Related details
- Moe Ahmadi
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. |
Moe Ahmadi (Gerrit)
Moe Ahmadi added 8 comments![]( "Open in Gerrit")
per-file *binder_provider*.*=set noparent
per-file *binder_provider*.*=file://ipc/SECURITY_OWNERSI think it's too easy to forget to add this. Can we add `_extension_binder_provider.h/.cc` to https://source.chromium.org/chromium/chromium/src/+/main:PRESUBMIT.py;l=4720;drc=01c9ad7fe24638f468ec728736fd4cd74abcd9b4 ?
Good idea! Done.
optional: Can you add a static method to AimEligibilityExtensionBridge to avoid the big inlined lambda?
Added helpers to the anonymous namespace inside aim_eligibility_extension_binder_provider.cc instead of static methods on `AimEligibilityExtensionBridge` given that's the file reviewed by IPC_SECURITY_OWNERS
auto* bridge = AimEligibilityExtensionBridge::Get(When would there be an interface request for this interface where `bridge` is nullptr? I feel like we should crash the renderer in that case, but I could be missing a case?
Actually, can we avoid registering the provider if there's no AimEligibilityExtensionBridge? Then we can CHECK here.
We can't avoid conditional registration or add a CHECK here.
`ExtensionMojoBinderRegistry` is a singleton, whereas `AimEligibilityExtensionBridge` is a ProfileKeyedService and can be nullptr. For example, if the connection comes from an OTR profile. `ExtensionMojoBinderProvider` instances are also global singletons and owned by the registry. Both the registry and the provider need to remain global singletons and can't be per-profile instances because `PopulateChromeFrameBindersForExtension()` and `PopulateChromeServiceWorkerBindersForExtension()` are called globally by ChromeContentBrowserClient whenever any extension document or service worker connects across the entire browser process.
Dropping the receiver seems like the right way to handle those cases to me. Please reopen if you disagree.
void RegisterProvider(
base::PassKey<AimEligibilityExtensionBinderProvider> passkey,
std::unique_ptr<ExtensionMojoBinderProvider> provider);
void RegisterProvider(base::PassKey<ExtensionMojoBinderRegistryTest> passkey,
std::unique_ptr<ExtensionMojoBinderProvider> provider);I *think* you could do the following here:
```
template <typename T>
void RegisterProvider(base::PassKey<T> passkey,
std::unique_ptr<ExtensionMojoBinderProvider> provider)
```
And then in the .cc:
```
template <>
void ExtensionMojoBinderRegistry::RegisterProvider(
base::PassKey<AimEligibilityExtensionBinderProvider>,
std::unique_ptr<ExtensionMojoBinderProvider> provider) {
...
}
```That way developers only need to modify one file. If this works, maybe worth moving the comment to the .cc.
It could work and would reduce the edits to one file. However, I lean towards keeping explicit overloads because (1) it matches the pattern in `services/metrics/public/cpp/ukm_recorder.h` which Devlin pointed to as the precedent for his suggestion to use passkeys; and (2) it's easier to see the allowlist of approved providers at one glance looking at either file.
touching both files when adding a new provider seems like a minor trade-off for better visibility. We can revisit that if this pattern proliferates and becomes cumbersome. Please reopen if you still prefer moving it to the .cc!
// New BinderProvider files must also have a corresponding OWNERS files
// restricting reviews to IPC_SECURITY_OWNERS.If we add _extension_binder_provider to the presubmit, then that would enforce this. Maybe we can change this to mention that the provider should end with _extension_binder_provider.h/cc?
good idea! adjusted the comment here to reflect the naming convention.
nit: Prefer CHECKs over DCHECKs.
Done
nit: Prefer CHECK over DCHECK for new code: https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/checks.md#invariant_verification-mechanisms
Done
EXPECT_FALSE(registry()->IsMojoJsEnabled(component_extension.get()));Moe Ahmadinit: `IsMojoEnabled(nullptr)`
Done
Related details
- Devlin Cronin
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |