[WebInstallAPI] Re-verify user activation in WebInstallService [chromium/src : main]
Lia Hiscock (Gerrit)
Lia Hiscock added 4 comments![]( "Open in Gerrit")
// Both renderer entry points (NavigatorWebInstall::InstallImpl and
// HTMLInstallElement::OnActivated) require user activation before sendingLia HiscockNot really my wheelhouse but AI review surfaced this comment nit:
HTMLInstallElement::OnActivated doesn't check transient activation — the element path gates on HTMLCapabilityElementBase::HandleActivation via event.IsFullyTrusted() (a trusted activation event) plus a debounce. A trusted DOMActivate normally coincides with transient activation, but they can diverge (activation consumed/expired by other in-flight work while the trusted event is processed), in which case this new check could reject a legitimately user-initiated element activation. Suggest rewording, e.g. "The JS path checks transient activation; the <install> element path requires a trusted activation event. Re-verify transient activation here as the authoritative browser-side gate — and the only one before the dialog for the element path, which skips the permission prompt."
lots to unpack here. concretely - i've updated the comment to explain the divergence and potential risk.
re. the potential risk (of rejecting legit <install> invocations) -
1 - risk of expiry - Tl;DR uncommon but not impossible (depends on how much work is between activation and this check)
2 - consumed by someone else - sneakier (apparently) - activation is frame-wide and single-shot - requires the page to deliberately (or accidentally) wire 2 activation-gated actions to 1 gesture
3 - realistically, this should be rare to never under normal use - we have no network or async UI between the trusted <install> event and this check.
`vr_service_impl` takes this same risk.
```
if (!has_user_activation) {
// User activation is verified blink-side, so this should never fail
// (everything that happens up to this point should not take enough time for
// the user activation to expire). Treat lack of user activation as unknown
// failure:
RejectSession(std::move(callback), options->trace_id,
```
There are 2 established patterns for browser verification
1 - CHECK, dont consume (this CL approach) - used by frame_sensor_provider_proxy, vr_service_impl, file_system_access_file_handle_impl, eye_dropper_chooser_impl)
2 - snapshot the activation bit early, never recheck - used for flows that span slow, async work and must not false-reject a legitimate gesture
- FedCM stores `has_transient_user_activation` at entry and reuses it throughout.
**For us, pattern A is the right tradeoff. since we have a UMA now, we can monitor for spikes and reevaluate.**
std::move(callback).Run(blink::mojom::WebInstallServiceResult::kAbortError,Lia HiscockPotential inconsistency: renderer-side missing activation throws NotAllowedError, but this browser-side path (the expiry/race case) returns kAbortError, which OnInstallResponse maps to JS AbortError. Same condition → two different errors depending on timing. Consider adding an enum value to disambiguate?
ah good catch. added `WebInstallServiceResult::kMissingActivationError`
if (!render_frame_host().HasTransientUserActivation()) {
std::move(callback).Run(blink::mojom::WebInstallServiceResult::kAbortError,
GURL());
return;
}Lia HiscockIs it true that now neither side consumes the activation? Within the transient-activation window (~5s) a page can call `navigator.install()` repeatedly?
yes, true. I also called that out to my agent. Because we're checking on both sides now, trying to consume activation anywhere would likely cause a race between checking/consuming. Agent said it's worth the tradeoff to get the browser guarantee that activation was actually there.
And yes, technically a page could spam. We have a 2nd layer of defense with the `IsInstallInProgress` guard though.
Given this, are you still wanting additional protections?
EmitInstallResultUma(triggered_from_element,Lia HiscockThe new rejection is placed before EmitInstallTypeUma , so it's invisible to metrics (the new tests assert zero UMA). Do we need to know how often it's happening?
might as well! missing user activation doesn't *technically* feel like it's a WebInstall*Service*Result, but not worth making a new enum, so I threw it on the end
Related details
- Lu Huang
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. |
Lu Huang (Gerrit)
Lu Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lu Huang
| Code-Review | +1 |
1 comment
if (!render_frame_host().HasTransientUserActivation()) {
std::move(callback).Run(blink::mojom::WebInstallServiceResult::kAbortError,
GURL());
return;
}Lia HiscockIs it true that now neither side consumes the activation? Within the transient-activation window (~5s) a page can call `navigator.install()` repeatedly?
yes, true. I also called that out to my agent. Because we're checking on both sides now, trying to consume activation anywhere would likely cause a race between checking/consuming. Agent said it's worth the tradeoff to get the browser guarantee that activation was actually there.
And yes, technically a page could spam. We have a 2nd layer of defense with the `IsInstallInProgress` guard though.
Given this, are you still wanting additional protections?
IsInstallInProgress helps but doesn't prevent a site from calling Install multiple times within the duration of the transient activation if the activation is never consumed, if they allow each call to complete.
I'm ok with this but I don't think this should be my call to make.
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 1 comment![]( "Open in Gerrit")
if (!render_frame_host().HasTransientUserActivation()) {
std::move(callback).Run(blink::mojom::WebInstallServiceResult::kAbortError,
GURL());
return;
}Lia HiscockIs it true that now neither side consumes the activation? Within the transient-activation window (~5s) a page can call `navigator.install()` repeatedly?
Lu Huangyes, true. I also called that out to my agent. Because we're checking on both sides now, trying to consume activation anywhere would likely cause a race between checking/consuming. Agent said it's worth the tradeoff to get the browser guarantee that activation was actually there.
And yes, technically a page could spam. We have a 2nd layer of defense with the `IsInstallInProgress` guard though.
Given this, are you still wanting additional protections?
IsInstallInProgress helps but doesn't prevent a site from calling Install multiple times within the duration of the transient activation if the activation is never consumed, if they allow each call to complete.
I'm ok with this but I don't think this should be my call to make.
No, it doesn't prevent that. I guess what's the risk though? The service impl will reject all but the first, right?
Related details
- Lu Huang
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. |