[<install> Element] Add manifest attribute support and plumbing [chromium/src : main]
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
Hi Rob! Looking for internal review here as well.
Related details
- Robert Paveza
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Paveza (Gerrit)
Robert Paveza added 3 comments![]( "Open in Gerrit")
NOTIMPLEMENTED();Is this being done in another CL?
I'm trying to understand why the WebInstallServiceImpl would need to know why the install action is coming from an element vs why the element wouldn't just call the InstallFromManifest function. This is how we've discussed speccing the behavior ("the install action is the same irrespective of whether it comes from n.i() or the element"), right? Is the delta concerning the permission?
If the delta is whether the UA should raise a permission prompt, I don't think that's really a significant point. The bottom line IMO is "a compromised renderer is a compromised renderer" -- which is to say, if the renderer is compromised, then the renderer telling the UA "no it's okay to bypass the permission check" by calling "ElementInstallFromManifest" instead of "InstallFromManifest" is questionable.
I think we've discussed when an <install> element is constructed, it should obtain a token from the browser process (I think the internal name of this is UnguessableToken) which is associated with that element. This would require a bit of negotiation at bootstrapping time:
renderer: "heyyyyyy I have an <install> element"
browser: "ok here's a token, I'll remember - don't try to trick me later!"
renderer: "yo, my person clicked my install element, fr fr, here's the token"
Now it's not clear to me that EVEN THIS is really that "trustworthy," but at least with an n.i() call, it would be challenging to find that specific token in the memory structure (or if you're not using the <install> element at all, there would be no such token in the browser process).
Then, the ManifestInstallOptions mojo structure can include an optional token. For n.i(), you don't include the token; for the element, you do. If the token is included but not valid, it's rejected (maybe we even actively tear down the renderer because it seems compromised). If it's not included, that gives you the logic of whether to present the permissions prompt.
// manifestid alone is not supported; silent no-op.Hmm, a silent no-op feels wrong. "Incorrect input parameters" is a developer error -- we should invoke the manifest install result handler with an error condition (probably a DataError or TypeError).
NOTIMPLEMENTED();Is this coming soon or coming later?
Related details
- Lia Hiscock
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. |
Lia Hiscock (Gerrit)
Lia Hiscock added 2 comments![]( "Open in Gerrit")
Hmm, a silent no-op feels wrong. "Incorrect input parameters" is a developer error -- we should invoke the manifest install result handler with an error condition (probably a DataError or TypeError).
Yeah, that's fair...This case was handled inadvertently in the old world by `GetCheckedInstallOptions`, but changed with the logic to handle both installurl/manifestid and manifest/manifestid.
I've added back explicit handling here - using `HandleInstallDataError` + `PromptDismiss`, as was used before.
note - this is part of the error handling that will change anyway with Kristin's CL
Is this coming soon or coming later?
Not coming at all!
This is the `navigator_web_install test`, so the `ElementInstall...` is not expected to be used. And vice versa in the `html_install_element_test`.
Related details
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. |
Lia Hiscock (Gerrit)
Lia Hiscock added 2 comments![]( "Open in Gerrit")
I've rebased so the ElementInstallFromManifest should be clearer now!
NOTIMPLEMENTED();Is this being done in another CL?
I'm trying to understand why the WebInstallServiceImpl would need to know why the install action is coming from an element vs why the element wouldn't just call the InstallFromManifest function. This is how we've discussed speccing the behavior ("the install action is the same irrespective of whether it comes from n.i() or the element"), right? Is the delta concerning the permission?
If the delta is whether the UA should raise a permission prompt, I don't think that's really a significant point. The bottom line IMO is "a compromised renderer is a compromised renderer" -- which is to say, if the renderer is compromised, then the renderer telling the UA "no it's okay to bypass the permission check" by calling "ElementInstallFromManifest" instead of "InstallFromManifest" is questionable.
I think we've discussed when an <install> element is constructed, it should obtain a token from the browser process (I think the internal name of this is UnguessableToken) which is associated with that element. This would require a bit of negotiation at bootstrapping time:
renderer: "heyyyyyy I have an <install> element"
browser: "ok here's a token, I'll remember - don't try to trick me later!"
renderer: "yo, my person clicked my install element, fr fr, here's the token"
Now it's not clear to me that EVEN THIS is really that "trustworthy," but at least with an n.i() call, it would be challenging to find that specific token in the memory structure (or if you're not using the <install> element at all, there would be no such token in the browser process).Then, the ManifestInstallOptions mojo structure can include an optional token. For n.i(), you don't include the token; for the element, you do. If the token is included but not valid, it's rejected (maybe we even actively tear down the renderer because it seems compromised). If it's not included, that gives you the logic of whether to present the permissions prompt.
Discussed offline, recapping for the record -
### Re. ElementInstallFromManifest/InstallFromManifest
The split follows the convention we set with the install_url path - the 2 mojo functions are just thin wrappers that both call into the same WebInstallServiceImpl internal function, to avoid adding a load bearing bool to the mojo signatures -
- `InstallFromManifest` / `ElementInstallFromManifest` -> `InstallFromManifestInternal`
- `Install` / `InstallFromElement` --> `InstallInternal`
The `Element` versions pass an additional bool `triggered_from_element_` (already in `InstallInternal`, coming soon to `InstallFromManifestInternal`) to the `Install[FromManifest]Internal` that's used primarily for metrics, but also to decide whether to skip the permission prompt.
Granted, "skip the permission prompt" has been a source of AI-identified bugs (having a renderer path that allows skipping the prompt), but the entire premise of the PEPC elements is that no additional permission prompt is needed, so for well-behaving renderers, there must be a path to "show the install dialog directly, no permission prompt"
### Re. base::UnguessableToken -
The PEPC elements already provide some degree of "identification with the browser" guarantee - [HTMLCapabilityElementBase::is_registered_in_browser_process_](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_capability_element_base.cc;l=1070?q=is_registered_in_browser_process_&ss=chromium).
This bool is only `true` if the page is under the allowed count of same-type elements - [kMaxPEPCPerPage or kMaxInstallPerPage](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/permissions/embedded_permission_control_checker.cc;l=25-26?q=kMaxPEPCPerPage&ss=chromium) - this count is enforced by the browser.
**<install> is already compliant with this count limit - https://chromium-review.googlesource.com/c/chromium/src/+/7256736**
Related details
- Robert Paveza
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. |