Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict more disurptive features #45

Merged
merged 4 commits into from
Feb 25, 2021
Merged

Restrict more disurptive features #45

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5fdc6b
Restrict more disruptive features
domenic Feb 19, 2021
e2f27c6
Web NFC
domenic Feb 23, 2021
b6012e3
Battery status and header rejiggering
domenic Feb 23, 2021
1f7a61d
Review comments
domenic Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 82 additions & 2 deletions index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ spec: html; urlPrefix: https://html.spec.whatwg.org/multipage/
text: resulting URL record
urlPrefix: webappapis.html
text: script; url: concept-script
spec: page-visibility; urlPrefix: https://w3c.github.io/page-visibility/
type: dfn
text: hidden; for: Document; url: dfn-hidden
spec: geolocation; urlPrefix: https://w3c.github.io/geolocation-api/
type: method; for: Geolocation
text: getCurrentPosition(successCallback, errorCallback, options); url: getcurrentposition-method
Expand All @@ -68,6 +71,24 @@ spec: idle-detection; urlPrefix: https://wicg.github.io/idle-detection/
type: method; for: IdleDetector
text: requestPermission(); url: api-idledetector-requestpermission
text: start(); url: api-idledetector-start
spec: clipboard-apis; urlPrefix: https://w3c.github.io/clipboard-apis/
type: method; for: Clipboard
text: read(); url: dom-clipboard-read
text: readText(); url: dom-clipboard-readtext
text: write(); url: dom-clipboard-write
text: writeText(); url: dom-clipboard-writetext
spec: screen-wake-lock; urlPrefix: https://w3c.github.io/screen-wake-lock/
type: method; for: WakeLock
text: request(); url: the-request-method
spec: web-nfc; urlPrefix: https://w3c.github.io/web-nfc/
type: method; for: NDEFReader
text: write(); url: dom-ndefreader-write
text: scan(); url: dom-ndefreader-scan
spec: battery; urlPrefix: https://w3c.github.io/battery/
type: method; for: Navigator
text: getBattery(); url: dom-navigator-getbattery
type: dfn
text: battery promise; url: dfn-battery-promise
</pre>
<pre class="biblio">
{
Expand Down Expand Up @@ -548,7 +569,7 @@ Modify the <a spec=HTML>downloads a hyperlink</a> algorithm to ensure that downl
1. If [=this=]'s [=Window/browsing context=] is a [=prerendering browsing context=], then return.
</div>

<h3 id="permissions">Waiting before permissions checks</h3>
<h3 id="delay-async-apis">Delaying async API results</h3>

Many specifications need to be patched so that, if a given algorithm invoked in a [=prerendering browsing context=], most of its work is deferred until the browsing context is [=prerendering browsing context/activated=]. This is tricky to do uniformly, as many of these specifications do not have great hygeine around <a href="https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-for-spec-authors">using the event loop</a>. Nevertheless, the following sections give our best attempt.

Expand Down Expand Up @@ -627,7 +648,64 @@ TODO: what about the service worker API? Depends on what we're doing for service

<p class="note">The other interesting method, {{IdleDetector/requestPermission()|IdleDetector.requestPermission()}}, is gated on [=transient activation=]. However, even if permission was previously granted for the origin in question, we delay starting any idle detectors while prerendering.

<h4 id="activation-gated">Activation-gated APIs</h4>
<h4 id="patch-clipboard-apis">Clipboard APIs</h4>

<div algorithm="Clipboard write patch">
Modify the {{Clipboard/write()}} method steps by inserting the following steps after the initial creation of |p|:

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 |p|.
</div>

<div algorithm="Clipboard writeText patch">
Modify the {{Clipboard/writeText()}} method steps by inserting the following steps after the initial creation of |p|:

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 |p|.
</div>

<h4 id="patch-wake-lock">Screen Wake Lock API</h4>

<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|.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@domenic domenic Feb 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

</div>

<p class="note">Prerendered documents do <em>not</em> count as [=Document/hidden=], so the part of the method steps which throw an exception in that case will not trigger.</p>

<h4 id="patch-generic-sensor">Generic Sensor API</h4>

<div algorithm="Sensor start patch">
Modify the {{Sensor/start()}} method steps by inserting the following steps after the state is set to "`activating`":

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

<h4 id="patch-web-nfc">Web NFC</h4>

<div algorithm="NDEFReader write patch">
Modify the {{NDEFReader/write()}} method steps by inserting the following steps after the initial creation of |p|:

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 |p|.
</div>

<div algorithm="NDEFReader scan patch">
Modify the {{NDEFReader/scan()}} method steps by inserting the following steps after the initial creation of |p|:

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 |p|.
</div>

<h4 id="patch-battery">Battery Status API</h4>

<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=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's always defer all checks. On the spec side that'll depend on w3c/battery#30, but I'll make that clear.

</div>

<h3 id="activation-gated">Activation-gated APIs</h3>

The following APIs do not need modifications, because they will automatically fail or no-op without [=transient activation=] or [=sticky activation=], which a [=prerendering browsing context=]'s [=browsing context/active window=] will never have. We list them here for completeness, to show which API surfaces we've audited.

Expand All @@ -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]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for adding this. I was initially not understanding why write() is patched and read() isn't.

- {{Navigator/share()|navigator.share()}} [[WEB-SHARE]]

<h2 id="todo">TODO</h2>

Expand Down