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.

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

Improve getBattery() algorithm #30

Merged
merged 3 commits into from
Feb 26, 2021
Merged

Conversation

domenic
Copy link
Contributor

@domenic domenic commented Feb 25, 2021

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. Closes Update the reference to Feature Policy #26.
  • 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 Restrict more disurptive features WICG/nav-speculation#45.

Preview | Diff

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

@anssiko anssiko left a comment

Choose a reason for hiding this comment

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

@domenic Much thanks!

PTAL @mounirlamouri

index.html Outdated
</h2>
<p data-link-for="Navigator">
The Battery Status API is a <a>policy-controlled feature</a> identified
by the string "<code>battery</code>". It's default allowlist is
by the string "<code>battery</code>". It's <a>default allowlist</a> is
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Its

(To be fixed on the same pass.)

@mounirlamouri mounirlamouri merged commit e1d1aeb into w3c:gh-pages Feb 26, 2021
@domenic domenic deleted the improvements branch February 26, 2021 19:16
rakuco added a commit to rakuco/wpt that referenced this pull request Jan 26, 2022
This test does not reflect either the current spec or any implementation.

w3c/battery#30 removed the bits from the spec that mandated throwing a
SecurityError when the API was used in an insecure origin. Blink does not
perform any checks for a security context anyway, although the plan is to
switch spec and implementation to using [SecureContext] in the future, as
tracked in w3c/battery#15.
rakuco added a commit to web-platform-tests/wpt that referenced this pull request Jan 28, 2022
This test does not reflect either the current spec or any implementation.

w3c/battery#30 removed the bits from the spec that mandated throwing a
SecurityError when the API was used in an insecure origin. Blink does not
perform any checks for a security context anyway, although the plan is to
switch spec and implementation to using [SecureContext] in the future, as
tracked in w3c/battery#15.
rakuco added a commit that referenced this pull request Jan 31, 2022
…face.

In other words, stop exposing this API to insecure origins.

This has been discussed since at least 2016 (see #5). #11 made access from
an insecure origin throw a SecurityError at a time when the
`[SecureContext]` Web IDL extended attribute was not widespread.
Unfortunately, the spec change was not accompanied by a change in the
implementation(s) and, to this day, Blink's implementation (the only
remaining one) continues to expose the API to insecure origins.

Years later, #30 was added in the context of the discussions in #15 where it
was noted that the spec was still manually throwing a SecurityError when
checking if it was called by a secure origin. Besides stopping throwing a
SecurityError (which was never implemented), #30 also recognized the current
situation by noting that the API _should_ use `[SecureContext]` but did not
because it only made sense to do so when the implementation was updated
accordingly.

This change finally adds `[SecureContext]` to the spec's Web IDL because I
am also handling the Blink implementation [1][2]: starting with Chrome 99,
users will be warned that using the Battery Status API in an insecure origin
is deprecated, and starting with Chrome 103 doing so will no longer be
possible.

[1] https://chromestatus.com/feature/4878376799043584
[2] https://groups.google.com/a/chromium.org/g/blink-dev/c/w80tJL8uEV8/m/PfrHlvtgAgAJ

From a testing perspective, there isn't much to be done:
- web-platform-tests/wpt#5871 changed the existing tests to run in HTTPS.
- web-platform-tests/wpt#32556 removed the test for the SecurityError
  exception that never passed.

Fixes #15
rakuco added a commit that referenced this pull request Feb 1, 2022
…face.

In other words, stop exposing this API to insecure origins. Even though this
API is not new, it provides user information that, transmitted insecurely,
can pose a risk to user privacy. See
https://w3ctag.github.io/design-principles/#secure-context for more
information on the guidelines we are trying to follow.

This has been discussed since at least 2016 (see #5). #11 made access from
an insecure origin throw a SecurityError at a time when the
`[SecureContext]` Web IDL extended attribute was not widespread.
Unfortunately, the spec change was not accompanied by a change in the
implementation(s) and, to this day, Blink's implementation (the only
remaining one) continues to expose the API to insecure origins.

Years later, #30 was added in the context of the discussions in #15 where it
was noted that the spec was still manually throwing a SecurityError when
checking if it was called by a secure origin. Besides stopping throwing a
SecurityError (which was never implemented), #30 also recognized the current
situation by noting that the API _should_ use `[SecureContext]` but did not
because it only made sense to do so when the implementation was updated
accordingly.

This change finally adds `[SecureContext]` to the spec's Web IDL because I
am also handling the Blink implementation [1][2]: starting with Chrome 99,
users will be warned that using the Battery Status API in an insecure origin
is deprecated, and starting with Chrome 103 doing so will no longer be
possible.

[1] https://chromestatus.com/feature/4878376799043584
[2] https://groups.google.com/a/chromium.org/g/blink-dev/c/w80tJL8uEV8/m/PfrHlvtgAgAJ

From a testing perspective, there isn't much to be done:
- web-platform-tests/wpt#5871 changed the existing tests to run in HTTPS.
- web-platform-tests/wpt#32556 removed the test for the SecurityError
  exception that never passed.

Fixes #15
rakuco added a commit that referenced this pull request Feb 1, 2022
…face.

In other words, stop exposing this API to insecure origins. Even though this
API is not new, it provides user information that, transmitted insecurely,
can pose a risk to user privacy. See
https://w3ctag.github.io/design-principles/#secure-context for more
information on the guidelines we are trying to follow.

This has been discussed since at least 2016 (see #5). #11 made access from
an insecure origin throw a SecurityError at a time when the
`[SecureContext]` Web IDL extended attribute was not widespread.
Unfortunately, the spec change was not accompanied by a change in the
implementation(s) and, to this day, Blink's implementation (the only
remaining one) continues to expose the API to insecure origins.

Years later, #30 was added in the context of the discussions in #15 where it
was noted that the spec was still manually throwing a SecurityError when
checking if it was called by a secure origin. Besides stopping throwing a
SecurityError (which was never implemented), #30 also recognized the current
situation by noting that the API _should_ use `[SecureContext]` but did not
because it only made sense to do so when the implementation was updated
accordingly.

This change finally adds `[SecureContext]` to the spec's Web IDL because I
am also handling the Blink implementation [1][2]: starting with Chrome 99,
users will be warned that using the Battery Status API in an insecure origin
is deprecated, and starting with Chrome 103 doing so will no longer be
possible.

[1] https://chromestatus.com/feature/4878376799043584
[2] https://groups.google.com/a/chromium.org/g/blink-dev/c/w80tJL8uEV8/m/PfrHlvtgAgAJ

From a testing perspective, there isn't much to be done:
- web-platform-tests/wpt#5871 changed the existing tests to run in HTTPS.
- web-platform-tests/wpt#32556 removed the test for the SecurityError
  exception that never passed.

Fixes #15
rakuco added a commit that referenced this pull request Feb 1, 2022
…face. (#51)

In other words, stop exposing this API to insecure origins. Even though this
API is not new, it provides user information that, transmitted insecurely,
can pose a risk to user privacy. See
https://w3ctag.github.io/design-principles/#secure-context for more
information on the guidelines we are trying to follow.

This has been discussed since at least 2016 (see #5). #11 made access from
an insecure origin throw a SecurityError at a time when the
`[SecureContext]` Web IDL extended attribute was not widespread.
Unfortunately, the spec change was not accompanied by a change in the
implementation(s) and, to this day, Blink's implementation (the only
remaining one) continues to expose the API to insecure origins.

Years later, #30 was added in the context of the discussions in #15 where it
was noted that the spec was still manually throwing a SecurityError when
checking if it was called by a secure origin. Besides stopping throwing a
SecurityError (which was never implemented), #30 also recognized the current
situation by noting that the API _should_ use `[SecureContext]` but did not
because it only made sense to do so when the implementation was updated
accordingly.

This change finally adds `[SecureContext]` to the spec's Web IDL because I
am also handling the Blink implementation [1][2]: starting with Chrome 99,
users will be warned that using the Battery Status API in an insecure origin
is deprecated, and starting with Chrome 103 doing so will no longer be
possible.

[1] https://chromestatus.com/feature/4878376799043584
[2] https://groups.google.com/a/chromium.org/g/blink-dev/c/w80tJL8uEV8/m/PfrHlvtgAgAJ

From a testing perspective, there isn't much to be done:
- web-platform-tests/wpt#5871 changed the existing tests to run in HTTPS.
- web-platform-tests/wpt#32556 removed the test for the SecurityError
  exception that never passed.

Fixes #15
mattwoodrow pushed a commit to mattwoodrow/wpt that referenced this pull request Feb 15, 2022
…ests#32556)

This test does not reflect either the current spec or any implementation.

w3c/battery#30 removed the bits from the spec that mandated throwing a
SecurityError when the API was used in an insecure origin. Blink does not
perform any checks for a security context anyway, although the plan is to
switch spec and implementation to using [SecureContext] in the future, as
tracked in w3c/battery#15.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 26, 2022
…context.html., a=testonly

Automatic update from web-platform-tests
battery status: Remove battery-insecure-context.html. (#32556)

This test does not reflect either the current spec or any implementation.

w3c/battery#30 removed the bits from the spec that mandated throwing a
SecurityError when the API was used in an insecure origin. Blink does not
perform any checks for a security context anyway, although the plan is to
switch spec and implementation to using [SecureContext] in the future, as
tracked in w3c/battery#15.
--

wpt-commits: aa8ded0ca6ef536aa74719a73d7175499f0a2cd8
wpt-pr: 32556
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this pull request Feb 28, 2022
…ests#32556)

This test does not reflect either the current spec or any implementation.

w3c/battery#30 removed the bits from the spec that mandated throwing a
SecurityError when the API was used in an insecure origin. Blink does not
perform any checks for a security context anyway, although the plan is to
switch spec and implementation to using [SecureContext] in the future, as
tracked in w3c/battery#15.
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Mar 1, 2022
…context.html., a=testonly

Automatic update from web-platform-tests
battery status: Remove battery-insecure-context.html. (#32556)

This test does not reflect either the current spec or any implementation.

w3c/battery#30 removed the bits from the spec that mandated throwing a
SecurityError when the API was used in an insecure origin. Blink does not
perform any checks for a security context anyway, although the plan is to
switch spec and implementation to using [SecureContext] in the future, as
tracked in w3c/battery#15.
--

wpt-commits: aa8ded0ca6ef536aa74719a73d7175499f0a2cd8
wpt-pr: 32556
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 8, 2022
…context.html., a=testonly

Automatic update from web-platform-tests
battery status: Remove battery-insecure-context.html. (#32556)

This test does not reflect either the current spec or any implementation.

w3c/battery#30 removed the bits from the spec that mandated throwing a
SecurityError when the API was used in an insecure origin. Blink does not
perform any checks for a security context anyway, although the plan is to
switch spec and implementation to using [SecureContext] in the future, as
tracked in w3c/battery#15.
--

wpt-commits: aa8ded0ca6ef536aa74719a73d7175499f0a2cd8
wpt-pr: 32556
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Mar 8, 2022
…context.html., a=testonly

Automatic update from web-platform-tests
battery status: Remove battery-insecure-context.html. (#32556)

This test does not reflect either the current spec or any implementation.

w3c/battery#30 removed the bits from the spec that mandated throwing a
SecurityError when the API was used in an insecure origin. Blink does not
perform any checks for a security context anyway, although the plan is to
switch spec and implementation to using [SecureContext] in the future, as
tracked in w3c/battery#15.
--

wpt-commits: aa8ded0ca6ef536aa74719a73d7175499f0a2cd8
wpt-pr: 32556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the reference to Feature Policy
3 participants