Restrict more disurptive features #45
Conversation
This batch includes delaying clipboard, screen wake lock, and generic sensor APIs. It also notes that clipboard events and navigator.share() are activation-gated.
domenic
force-pushed
the
more-permissions
branch
from
ef0cdb9
to
e5fdc6b
Compare
| <div algorithm="WakeLock request patch"> | ||
| Modify the {{WakeLock/request()}} method steps by inserting the following steps after the mid-algorithm creation of |promise|: | ||
|
|
||
| 1. If [=this=]'s [=relevant global object=]'s [=Window/browsing context=] is a [=prerendering browsing context=], then append the following steps to [=this=]'s [=platform object/post-prerendering activation steps list=] and return |promise|. |
There was a problem hiding this comment.
Just curious: The algorithm request() algorithm uses the browsing context of the "responsible document of the current settings object". Is this another way to get the same browsing context as "this's relevant global object's browsing context"? When we patch these specs should we make these consistent?
There was a problem hiding this comment.
Following the definitions, they are indeed the same, although it's not obvious. I can work on fixing the spec to use this. (We'd eventually like to remove "responsible document" per whatwg/html#4335.)
| 1. If [=this=]'s [=relevant global object=]'s [=Window/browsing context=] is a [=prerendering browsing context=], then append the following steps to [=this=]'s [=platform object/post-prerendering activation steps list=] and return. | ||
| 1. If [=this=].{{Sensor/[[state]]}} is "`idle`", then return. | ||
|
|
||
| <p class="note">This ensures that if this portion of the algorithm was delayed due to prerendering, and in the meantime {{Sensor/stop()}} was called, we do nothing upon activating the prerender. |
There was a problem hiding this comment.
I thought this may break if start() is called twice, but I think it works too. Should we add an ASSERT(state is 'activating') after the idle check?
| <div algorithm="Navigator getBattery patch"> | ||
| Modify the {{Navigator/getBattery()}} method steps by inserting the following after the initial secure context check: | ||
|
|
||
| 1. If [=this=]'s [=relevant global object=]'s [=Window/browsing context=] is a [=prerendering browsing context=], then append the following steps to [=this=]'s [=platform object/post-prerendering activation steps list=] and return [=this=]'s [=battery promise=]. |
There was a problem hiding this comment.
Check for understanding: We do the secure context check during prerendering and delay the "allowed to use" check because secure context is immutable while "allowed to use" is mutable? In the Geolocation spec I think we are delaying all checks until after prerendering.
There was a problem hiding this comment.
This is a good point. My reasoning was indeed something like "if it's going to fail always (because of a non-secure context), we might as well fail early". But indeed, in geolocation we delay all the checks until afterward. This is mainly because the existing geolocation spec bundles up secure context and "allowed to use" into a single step, so it would have been more awkward to split them.
I see a few approaches:
- Split out all immutable checks. This requires more case-by-case analysis.
- Split out secure context checks specifically. These should be rare as most APIs use
[SecureContext]these days; it's only very old APIs that have explicit secure context checks. - Always defer all checks. Nice and simple and consistent.
To be clear, the first two options here would involve updating geolocation to make it more like getBattery().
Any thoughts? I think any of them is fine, personally.
There was a problem hiding this comment.
Hm, I agree any of them seem fine. If I were to choose, I'd go with "Always defer all checks", just because it seems the most straightforward and least error-prone.
There was a problem hiding this comment.
OK, let's always defer all checks. On the spec side that'll depend on w3c/battery#30, but I'll make that clear.
| @@ -638,6 +716,8 @@ The following APIs do not need modifications, because they will automatically fa | |||
| - {{PresentationRequest/start()|presentationRequest.start()}} [[PRESENTATION-API]] | |||
| - {{Window/showOpenFilePicker()}}, {{Window/showSaveFilePicker()}}, and {{Window/showDirectoryPicker()}} [[FILE-SYSTEM-ACCESS]] | |||
| - {{IdleDetector/requestPermission()|IdleDetector.requestPermission()}} [[IDLE-DETECTION]] | |||
| - Firing of clipboard events, as well as {{Clipboard/read()|navigator.clipboard.read()}} and {{Clipboard/readText()|navigator.clipboard.readText()}} [[CLIPBOARD-APIS]] | |||
There was a problem hiding this comment.
Ah, thanks for adding this. I was initially not understanding why write() is patched and read() isn't.
These changes are mostly editorial or minor correctness improvements. Notable ones: * Move to the modern definition of secure context * Move to permissions policy instead of feature policy * Avoid "return and continue asynchronously" pattern. It's fine to create the BatteryManager as part of the algorithm (and that in fact matches implementations). * Always have a battery promise available, instead of having it sometimes be null. This is important for integration with prerendering in WICG/nav-speculation#45.
This batch includes delaying clipboard, screen wake lock, and generic sensor APIs. It also notes that clipboard events and navigator.share() are activation-gated.
Preview | Diff