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

Allow use in same-origin children, add Feature Policy integration #13

Open
wants to merge 7 commits into
base: gh-pages
from

Conversation

Projects
None yet
8 participants
@anssiko
Copy link
Member

commented Aug 31, 2017

Per @clelland's suggestion in #10 (comment) this change allows Battery Status API use in same-origin children, and adds Feature Policy integration.

PTAL @clelland @RByers @mounirlamouri @riju

Fixes #10.


Preview | Diff

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2017

@clelland @RByers @mounirlamouri, any concerns?

If I don't hear any, I'll merge this and @riju will update the Chrome's implementation accordingly.

@RByers

This comment has been minimized.

Copy link

commented Sep 19, 2017

Looks reasonable to me. Is the plan to add a feature control hook later?

@clelland

This comment has been minimized.

Copy link

commented Sep 19, 2017

The only forwards-incompatibility I can see with Feature Policy here is that if a deeply nested frame happens to be same-origin with the top-document, then it will be granted access to the API by default, while feature policy would disallow that.

That is, in an A->B->A embedding scenario, both the top-level and the innermost frame would be able to use the battery API. With feature policy, the inner frame wouldn't be allowed to use it, unless the top-level doc granted it to the middle frame, which then further granted it to the inner one.

(That is, feature policy would allow access to same-origin children -- and their same-origin children, and so on -- but not to arbitrary same-origin descendent frames)

@mounirlamouri

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

Do we have any idea of how common it is to have x-origin iframe usage of the Battery API? Said otherwise, are we sure we are not breaking a real use case? Are we sure this change is even web compatible?

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

Thanks for your comments!

Is the plan to add a feature control hook later?

@RByers, that was my initial plan. That said, we could consider adding the hook on the same pass, there's just one procedural issue that need to be cleared:

The Feature Policy spec is currently in incubation, and having an unstable normative reference might be an issue if we want to advance the Battery Status API to Candidate Recommendation and beyond.

@dontcallmedom to confirm whether we could make an exception here.

Do we have any idea of how common it is to have x-origin iframe usage of the Battery API?

@mounirlamouri, I'd like to know as well, but I don't have any data.

If we care strongly about this, would Chrome be fine with us adding telemetry to figure out how common x-origin iframe usage is?

A->B->A embedding scenario

@clelland, I'd expect many other platform features be similarly impacted? If so, I wouldn't be concerned about this case at hand specifically.

Couple of questions to @clelland et al.:

Feature Policy "v1" shipped in Chrome M62. Does it have all the features implemented we'd need for this spec to integrate with it?

Do you have established conventions for spec authors on how to hook into the Feature Policy spec the right way. I'm looking for normative spec language I could borrow similarly to e.g. https://w3c.github.io/webappsec-secure-contexts/#new This helps keep specs consistent.

Thanks!

@dontcallmedom

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

re CR transition with a reference to an unstable spec - this won't block transition to CR (although we would want to highlight it in the transition request), but may block transition to PR. That being said, transition to PR needs a 2nd implementation we don't have yet.

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2017

Thanks for the confirmation @dontcallmedom.

With that, I'd be happy to hook this spec into the Feature Policy spec when shown the preferred conventions in doing so.

@clelland

This comment has been minimized.

Copy link

commented Sep 21, 2017

@anssiko -- I think that we do have all of the features implemented in Chromium to do that. As long as the plan is just to reject the promise when disallowed, then it should be pretty simple.

I've written up a spec integration guide at https://github.com/WICG/feature-policy/blob/gh-pages/integration.md -- it's not strictly normative, but take a look and see if gives you enough guidance for the wording. If there's anything that I can add to that to help you out, I'm glad to do it.

@anssiko anssiko force-pushed the issue-10-secure-top-level branch from a2761ae to c2083ad Sep 21, 2017

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2017

@clelland, thanks, the integration guide is very helpful.

I added Feature Policy integration in c2083ad, PTAL. See also the preview.

(I assumed the integration guide means "SecurityError" DOMException.)

@clelland

This comment has been minimized.

Copy link

commented Sep 22, 2017

@anssiko: Yes, that should totally read SecurityError DOMException, thanks :) PR fired. The actual error, though, should be whatever makes sense for the API -- whether rejecting a promise, or returning an error code, the important thing is just defining some way for the API to fail when disabled.

This change LGTM, though -- thanks!

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2017

@mounirlamouri PTAL and merge this PR if you're happy.

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2017

@mounirlamouri, gentle ping.

Others on this PR seem to be fine with this change, would like to get your explicit OK prior to merging.

@anssiko anssiko changed the title Allow use in same-origin children Allow use in same-origin children, add Feature Policy integration Oct 9, 2017

@anssiko anssiko referenced this pull request Oct 9, 2017

Closed

Relation to Feature Policy #133

@anssiko anssiko requested a review from mounirlamouri Oct 18, 2017

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2017

@mounirlamouri, we'll merge this PR unless we hear concerns from you by next Tuesday 24 Oct, to be followed by a corresponding patch to Chromium.

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2017

@Honry, as we prepare for this PR to land, could you evaluate the gaps in the battery-status test suite (in the spirit of test as you commit, soon to be adopted for all deliverables), and update the test suite accordingly? Even if I see my name in OWNERS, I may not have the cycles to pull off the test suite update at this time, and I'd +1 you to become an owner for the battery status test suite.

On a quick glimpse, I think we need to update the iframe tests and add new tests the Feature Policy integration, and test interesting edge cases such as #13 (comment)

@foolip, this is another API that needs a bunch of manual tests due to its inherent nature, similarly to Vibration API. Is the following guide still considered the state of the art in manual w-p-t testing? :-)

http://web-platform-tests.org/writing-tests/manual.html

@Honry

This comment has been minimized.

Copy link

commented Oct 23, 2017

@anssiko, according to the test as you commit policy, would you please add a label "needs tests" to this PR?

I will add a corresponding PR in w-p-t to reflect the changes and involve my name in OWNERS file.
Then revisit current test coverage. :)

@anssiko anssiko added the needs tests label Oct 23, 2017

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2017

would you please add a label "needs tests" to this PR?

Done. Thanks for your help updating this test suite too!

Honry added a commit to Honry/web-platform-tests that referenced this pull request Oct 23, 2017

Add feature-policy to battery-status
 - Add feature-policy tests
 - Disallow using battery in cross-origin iframe
 - Allow using battery in same-origin iframe
 w3c/battery#13
@Honry

This comment has been minimized.

Copy link

commented Oct 23, 2017

I will add a corresponding PR in w-p-t to reflect the changes and involve my name in OWNERS file.

Done at web-platform-tests/wpt#7944, PTAL.

@mounirlamouri

This comment has been minimized.

Copy link
Member

commented Oct 23, 2017

Sorry for the delay Anssi :( I still disagree with this change as I think it's curing thy symptoms, not the disease. The API should have an implementation that is privacy friendly.

This said, in practice, my main concern is whether this will break web pages. Was a use counter added into Chrome to discover how large the impact of this change would be and whether the deprecation is web-compatible? I don't think we can disable this in Chrome without this information.

@clelland

This comment has been minimized.

Copy link

commented Oct 23, 2017

@mounirlamouri, I don't think we've added a use counter for that into Chrome.

When we were looking at a similar behaviour change for the Fullscreen API, I think we added one that fired every time a same-origin page was blocked from entering fullscreen.

We could add a similar counter here for battery-status, as well as one that fires when a same-origin-as-root-but-embedded-cross-origin page successfully uses the API. If we do it quickly enough, we should be be able to get that change merged for M63, to get some actual data before the end of the year.

@mounirlamouri

This comment has been minimized.

Copy link
Member

commented Oct 23, 2017

I think something that checks the usage in x-origin iframe and another for same origin should be enough.

@clelland

This comment has been minimized.

Copy link

commented Oct 23, 2017

So Chrome doesn't currently restrict getBattery at all. Fully complying with this change would block the API in Chrome:

  • in all insecure contexts
  • in any frame which is at a different origin than its parent (without an appropriate feature policy)
  • in any frame which is at a different origin than the top-level document (again, without an appropriate feature policy)

If we add counters for those situations, then we should have a good idea how much usage would be broken by adopting this change (and also adopting the current spec as-is).

(Also, this is Chrome-specific, but both caniuse and the API confluence dashboard suggest that the API is not implemented anywhere else -- if that's wrong, it could change the 'web-compatible' calculation quite a bit)

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2018

For FP, what would be the plan? Would the API be denied by default or allowed?

This PR proposes that the default allowlist for the Battery Status API is ["self"] and that the battery promise will be rejected with a SecurityError if not allowed to use the policy-controlled feature.

Do you know what's the idea for sensors?

Concrete sensors that extend Sensor throw at construction if they're not allowed to use the policy-controlled feature, see https://w3c.github.io/sensors/#construct-sensor-object

The default allowlist for sensors is similarly ["self"], see https://w3c.github.io/sensors/#feature-policy-api

@clelland

This comment has been minimized.

Copy link

commented Jan 10, 2018

For FP, what would be the plan? Would the API be denied by default or allowed?

This PR proposes that the default allowlist for the Battery Status API is ["self"] and that the battery promise will be rejected with a SecurityError if not allowed to use the policy-controlled feature.

That matches my understanding -- so all usage would be allowed on the top-level document, and that document could allow access in cross-origin frames by embedding them with <iframe allow="battery"></iframe>, or with an HTTP header that allows it in that origin. Other usage would be denied.

@RByers

This comment has been minimized.

Copy link

commented Mar 15, 2018

Ideally, it would be great to have an idea of which websites are using the API but there is no good tools for this :(

There are some tools. After the UseCounters have been in stable for a bit, we could search HTTP Archive for their usage. Also it's now possible for a Googler to use CrUX data to get an even clearer view of usage in the wild.

@RByers

This comment has been minimized.

Copy link

commented Mar 15, 2018

FWIW, my hunch is that the severity of breakage is likely to be very small. If there's a compelling reason to lock this down, then (as long as we have the FP opt-in) I suspect we could get away with it.

All that said, I have heard of people using coarse grained battery status data to do A/B tests for power-saving optimizations. If that's what a lot of this usage is actually doing in practice (eg. some ad network script), then it seems like a pretty beneficial use case and argument for NOT locking this down (at least until there's a good WebPerf API around power usage).

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2018

Thanks @RByers!

I'm hearing we want to wait until we have more UseCounters data to be able to do a search on a bigger data set.

If everyone is happy with that plan, I suggest we park this issue for couple more months and revisit.

@anssiko anssiko force-pushed the issue-10-secure-top-level branch from c2083ad to e9f5442 May 22, 2018

index.html Outdated
The following concepts, terms, and interfaces are defined in
[[!HTML5]], [[!ECMASCRIPT]], [[!WEBIDL]], and [[!SECURE-CONTEXTS]]:
The following concepts, terms, and interfaces are defined in [[!HTML]],
[[!HTML5]], [[!ECMASCRIPT]], [[!WEBIDL]], [[!SECURE-CONTEXTS]], and

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Aug 1, 2018

Member

What's HTML5 still used for? Everything used should come out of WHATWG's version of the spec.

index.html Outdated
The Battery Status API is a <a>policy-controlled feature</a>, as
defined by Feature Policy [[!FEATURE-POLICY]]. The <a>feature name</a>
for the Battery Status API is "<code>battery</code>". The <a>default
allowlist</a> for the Battery Status API is <code>["self"]</code>. When

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Aug 1, 2018

Member

The allowlist is defined as an ordered set, which is a list. So instead of ["self"], « "self" ».

index.html Outdated
steps.
<li>If the <a>top-level browsing context</a>'s <a>active document</a>'s
<a>origin</a> and the <a>origin</a> specified by the <a>current
settings object</a> is not <a>same-origin domain</a>, then reject this

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Aug 1, 2018

Member

same origin-domain

index.html Outdated
<li>
<a href=
"https://html.spec.whatwg.org/multipage/origin.html#same-origin-domain">
<dfn>same-origin domain</dfn></a>

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Aug 1, 2018

Member

same origin-domain

index.html Outdated
promise</a> with a "<a>SecurityError</a>" <a>DOMException</a>, return
this <a>Navigator</a> object's <a>battery promise</a> and abort these
steps.
<li>If the <a>top-level browsing context</a>'s <a>active document</a>'s

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Aug 1, 2018

Member

#19 doesn't seem to be fixed here.

Probably you want, hmm, this Navigator object's relevant global object's associated Document's browsing context?

index.html Outdated
@@ -255,17 +286,18 @@ <h2>
the following steps:
</p>
<ol>
<li>If the <a>incumbent settings object</a> is not a <a>secure
<li>If the <a>current settings object</a> is not a <a>secure

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Aug 1, 2018

Member

Things in the web platform generally use "relevant settings object of this Navigator object." We generally don't want to use "current". See https://html.spec.whatwg.org/multipage/webappapis.html#realms-settings-objects-global-objects, the example starting with "One reason why the relevant concept is generally a better default choice than the current concept"

@mounirlamouri

This comment has been minimized.

Copy link
Member

commented Aug 6, 2018

@RByers I don't know about the severity of breakage because it will mostly depend on how the scripts will handle a failing attempts. Though, I'm more worried about shutting down valid use cases.

Fix settings object and global object prose to match the latest HTML
- Clarify which active document's origin is tested for same origin-domain-ness
- Use 'relevant settings object' over 'current settings object' where applicable
- Update 'allowlist' to match the convention
- Use WHATWG HTML references (except for firing a simple event that's gone)
- Fix typos

Fix #19

@anssiko anssiko force-pushed the issue-10-secure-top-level branch from 9954e99 to d2458d8 Aug 24, 2018

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2018

@TimothyGu, thanks a lot for your comments! I updated the PR, PTAL.

Honry added a commit to Honry/web-platform-tests that referenced this pull request Sep 4, 2018

Add feature-policy to battery-status
 - Add feature-policy tests
 - Disallow using battery in cross-origin iframe
 - Allow using battery in same-origin iframe
 w3c/battery#13
@anssiko

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

(Here's an HTML diff comparing this branch with gh-pages to ease review, e.g. at F2F. I enabled pr-preview to automate this for any subsequent PRs.)

promise</a> with a "<a>SecurityError</a>" <a>DOMException</a>, return
this <a>Navigator</a> object's <a>battery promise</a> and abort these
steps.
<li>If this <a>Navigator</a> object's <a>relevant global object</a>'s

This comment has been minimized.

Copy link
@clelland

clelland Sep 18, 2018

With feature policy integration, I think you can replace this entire paragraph with something like

If this Navigator object's relevant global object's associated Document is not allowed to use the battery feature, then reject...

Because the default allowlist is 'self', that will automatically take care of the same-origin embed case, while allowing cross-origin usage only if explicitly enabled by the embedding document (which this paragraph still prohibits, I think)

This comment has been minimized.

Copy link
@anssiko

anssiko Oct 2, 2018

Author Member

I pushed an update in an attempt to make use of "allowed to use". Please take a look. (I'm happy to see "allowed to use" abstracted out and reusable.)

index.html Outdated
The Battery Status API is a <a>policy-controlled feature</a>, as
defined by Feature Policy [[!FEATURE-POLICY]]. The <a>feature name</a>
for the Battery Status API is "<code>battery</code>". The <a>default
allowlist</a> for the Battery Status API is <code>« "self" »</code>.

This comment has been minimized.

Copy link
@clelland

clelland Sep 18, 2018

Minor nit: the value for the default allowlist would be 'self', with single quotes included

You can just say now

The Battery Status API is a policy-controlled-feature identified by the string "battery". It's default allowlist is 'self'.

This comment has been minimized.

Copy link
@anssiko

anssiko Oct 2, 2018

Author Member

Ditto, pushed an update per your proposal.

Use 'allowed to use' abstraction, reword Feature Policy integration
- Use 'allowed to use' as a condition for rejection, move the old
  text to a note
- Align Feature Policy integration section with the latest prose
- Fix allowlist notation
@clelland
Copy link

left a comment

One small comment about the non-normative explanation; this looks great otherwise.

index.html Outdated
<code>Document</code></a>'s <a>browsing context</a>'s <a>active
document</a>'s <a>origin</a> is not <a>same origin-domain</a> with
the <a>origin</a> of the <a>current settings object</a> of this
<a>Navigator</a> object.

This comment has been minimized.

Copy link
@clelland

clelland Oct 2, 2018

You could add something like "unless specifically allowed by the document's feature policy." to this, if you wanted to be clear that it's possible to grant access to cross-origin subframes, but you have to be deliberate about it. The default situation is exactly as you describe here.

Clarify note on rejection condition
Note it is possible to explicitly grant access to cross-origin
subframes using feature policy, in which case the promise is
not rejected.
@anssiko

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2018

@clelland, thanks for your comments! I integrated your suggestion and pushed an update. At F2F, we can gauge whether we have consensus to land this and update the implementation(s) accordingly. I invited @mounirlamouri to join as well.

</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

This comment has been minimized.

Copy link
@anssiko

anssiko Oct 21, 2018

Author Member

(Note to self: s/It’s/Its/)

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Oct 22, 2018

@anssiko

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

(Rebased with gh-pages and fixed merge conflicts in a9e3c94.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.