Closed Bug 1704005 Opened 4 years ago Closed 3 years ago

Add SecureContext and Permission Policy to Gamepad API

Categories

(Core :: DOM: Device Interfaces, enhancement)

Product:
Core
Component:
DOM: Device Interfaces
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
91 Branch
Tracking Flags:
Tracking Status
firefox91 --- fixed

People

(Reporter: marcos, Assigned: marcos)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Add SecureContext and Permissions Policy to Gamepad API
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.3 Safari/605.1.15

Steps to reproduce:

Firefox currently exposes the Gamepad API in non-secure contexts and third-party contexts are not restricted to use the API by Permissions Policy.

Expected results:

We should enable SecureContext and Permissions Policy in the appropriate places.

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Device Interfaces' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Component: Untriaged → DOM: Device Interfaces
Product: Firefox → Core
Assignee: nobody → marcos

Hi :jstutte, looks like daoshengmu's account is disabled, so not sure if he is monitoring/triaging gamepad bugs. Do you know if there is someone else I can ping for review?

Flags: needinfo?(jstutte)

Hi Marcos, it seems Johann is helping with review, so clearing the ni. Thanks for your support!

Flags: needinfo?(jstutte)

Sorry to bother you again, Jens, but I've been able to get Baku's attention on that patch (he needs to approve it - but I know he is super busy on other things and probably just fell of his radar). Would you mind either pinging on Slack or maybe finding me another DOM Peer to approve it?

Flags: needinfo?(jstutte)
Pushed by marcos@marcosc.com: https://hg.mozilla.org/integration/autoland/rev/039d90d5ec6b Add SecureContext and Permissions Policy to Gamepad API r=johannh,emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29179 for changes under testing/web-platform/tests

Backed out for causing failures on test_hide_gamepad_info.html

[task 2021-06-02T07:15:17.496Z] 07:15:17 INFO - TEST-START | browser/components/resistfingerprinting/test/mochitest/test_hide_gamepad_info.html
[task 2021-06-02T07:15:18.780Z] 07:15:18 INFO - GECKO(1566) | JavaScript error: http://mochi.test:8888/tests/browser/components/resistfingerprinting/test/mochitest/test_hide_gamepad_info_iframe.html, line 32: TypeError: navigator.getGamepads is not a function
[task 2021-06-02T07:20:37.181Z] 07:20:37 INFO - TEST-INFO | started process screentopng
[task 2021-06-02T07:20:37.358Z] 07:20:37 INFO - TEST-INFO | screentopng: exit 0
[task 2021-06-02T07:20:37.359Z] 07:20:37 INFO - TEST-UNEXPECTED-FAIL | browser/components/resistfingerprinting/test/mochitest/test_hide_gamepad_info.html | Test timed out. -
[task 2021-06-02T07:20:38.191Z] 07:20:38 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-06-02T07:20:38.192Z] 07:20:38 INFO - TEST-UNEXPECTED-FAIL | browser/components/resistfingerprinting/test/mochitest/test_hide_gamepad_info.html | [SimpleTest.finish()] No checks actually run. (You need to call ok(), is(), or similar functions at least once. Make sure you use SimpleTest.waitForExplicitFinish() if you need it.)
[task 2021-06-02T07:20:38.193Z] 07:20:38 INFO - SimpleTest.ok@SimpleTest/SimpleTest.js:417:16
[task 2021-06-02T07:20:38.194Z] 07:20:38 INFO - afterCleanup@SimpleTest/SimpleTest.js:1571:18
[task 2021-06-02T07:20:38.195Z] 07:20:38 INFO - executeCleanupFunction@SimpleTest/SimpleTest.js:1636:7
[task 2021-06-02T07:20:38.196Z] 07:20:38 INFO - SimpleTest.finish@SimpleTest/SimpleTest.js:1656:3
[task 2021-06-02T07:20:38.197Z] 07:20:38 INFO - killTest@SimpleTest/TestRunner.js:194:22
[task 2021-06-02T07:20:38.202Z] 07:20:38 INFO - GECKO(1566) | MEMORY STAT | vsize 20974723MB | residentFast 614MB
[task 2021-06-02T07:20:38.357Z] 07:20:38 INFO - TEST-OK | browser/components/resistfingerprinting/test/mochitest/test_hide_gamepad_info.html | took 320860ms

Flags: needinfo?(marcos)
Upstream PR was closed without merging

Thanks! Will investigate.

Flags: needinfo?(marcos)
Pushed by marcos@marcosc.com: https://hg.mozilla.org/integration/autoland/rev/e4e70f6b0108 Add SecureContext and Permissions Policy to Gamepad API r=johannh,emilio

Thanks, will investigate this one too.

Flags: needinfo?(marcos)

Sent another try run, to be safe:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dc201ee85cd62c0c84608541604f8cd2fafcecf

If it comes back happy, I'll attempt another merge.

Flags: needinfo?(jstutte)
Pushed by marcos@marcosc.com: https://hg.mozilla.org/integration/autoland/rev/77da39e524d6 Add SecureContext and Permissions Policy to Gamepad API r=johannh,emilio

https://hg.mozilla.org/mozilla-central/rev/77da39e524d6

Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
status-firefox91: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
Keywords: dev-doc-needed
Upstream PR merged by moz-wptsync-bot
Regressions: 1714702
Regressions: 1718221

Could we back this out from Nightly now so that beta doesn't end up having all the regressions? Or is there some pref to disable?

Flags: needinfo?(marcos)
Regressions: 1719756

We should just use FeaturePolicyValue::eAll for now (gets us back to a good state without backing everything out), then figure out why FeaturePolicyValue::eSelf is not working.

I'll prepare a patch.

Flags: needinfo?(marcos)

I think it would be better to backout everything in 91.

The SecureContext part shouldn't be needed to be backed out, though.

We did a fix via https://bugzilla.mozilla.org/show_bug.cgi?id=1718221 - no need to back out (🤞).

That one isn't in the beta 91 (yet?).

Not yet. Just confirmed that it should be uplifted to beta: https://bugzilla.mozilla.org/show_bug.cgi?id=1718221#c23

We should probably discuss over there if need be.

Documentation for this can be tracked in https://github.com/mdn/content/issues/6723

My understanding is that from FFv91 there will be two changes:

  1. GamePad will require SecureContext.
  2. Gamepad will be protected by a FeaturePolicy, with default policy of * (i.e. unprotected by default).
  3. Prior to this change you could use it in any context it wasn't subject to feature policy.

Is that correct?

I'm a little confused because it looks like the feature policy was added for "self" in FF81? https://bugzilla.mozilla.org/show_bug.cgi?id=1640086\
Can you clarify?

In terms of what a caller sees, is it still as in this hack - ie if protected the navigator.getGamepads() returns an empty array?

So essentially

Flags: needinfo?(marcos)

(In reply to Hamish Willee from comment #27)

Documentation for this can be tracked in https://github.com/mdn/content/issues/6723

My understanding is that from FFv91 there will be two changes:

  1. GamePad will require SecureContext.
  2. Gamepad will be protected by a FeaturePolicy, with default policy of * (i.e. unprotected by default).
  3. Prior to this change you could use it in any context it wasn't subject to feature policy.

Is that correct?

That is correct. However, the policy of "all" is temporary.

I'm a little confused because it looks like the feature policy was added for "self" in FF81? https://bugzilla.mozilla.org/show_bug.cgi?id=1640086\
Can you clarify?

The intent was that Gamepad should only be available to "self".

However, for some reason that we've not worked out yet, "self" was buggy, so we reverted "all" for now.

There is cross-browser agreement to move to "self".

In terms of what a caller sees, is it still as in this hack - ie if protected the navigator.getGamepads() returns an empty array?

correct (though it's not a hack - it's to prevent breakage as the API is over a decade old).

So essentially

Yes.

-Add policy for gamepad to Feature_Policy in browser compat and MDN docs: (e.g. in https://developer.mozilla.org/en-US/docs/Web/HTTP/Feature_Policy)

  • Anything else obvious?

Yes, but noting that the W3C Spec says this will be "self".

Flags: needinfo?(marcos)

Thanks Marco. I've updated the docs and BCD in https://github.com/mdn/content/issues/6723#issuecomment-886309101. Some more questions/confirmations:

  1. Re the discussion above "navigator.getGamepads() returns an empty array?" you indicated it still returns empty array. However the spec and the code now appear to indicate that a call to navigator.getGamepads() throws an exception instead. Can you clarify which it is?
  2. With respect to noting the spec differences of "self" vs "eall" I have put this in the BCD entry https://github.com/mdn/browser-compat-data/pull/11770 as a note. Still got to discuss it with the BCD people, but that's the right place for spec incompatibilities to be recorded.
  3. There seems to be a lot of overlap between this change and the Permissions API. Can you confirm whether or not this change also lead to a new permission in that API (I have included one in the BCD before reading more closely and realizing they are probably completely separate).
  4. I haven't found evidence of Webkit implementation. Chrome seems to be still behind the preference. Do you happen to know if they have implemented this without a preference? (i.e. am I missing evidence that this is released on those platforms too).
Flags: needinfo?(marcos)

Sorry for the delay, Hamish!

(In reply to Hamish Willee from comment #29)

Thanks Marco. I've updated the docs and BCD in https://github.com/mdn/content/issues/6723#issuecomment-886309101. Some more questions/confirmations:

  1. Re the discussion above "navigator.getGamepads() returns an empty array?" you indicated it still returns empty array. However the spec and the code now appear to indicate that a call to navigator.getGamepads() throws an exception instead. Can you clarify which it is?

In insecure contexts, this will getGamepads() will throw because it's no longer exposed.

In secure contexts, it should work as expected.

  1. With respect to noting the spec differences of "self" vs "eall" I have put this in the BCD entry https://github.com/mdn/browser-compat-data/pull/11770 as a note. Still got to discuss it with the BCD people, but that's the right place for spec incompatibilities to be recorded.

Sounds great. I've subscribed, but please ping me if you need anything (@marcoscaceres on GitHub).

I'll take a look at your comments there too.

  1. There seems to be a lot of overlap between this change and the Permissions API. Can you confirm whether or not this change also lead to a new permission in that API (I have included one in the BCD before reading more closely and realizing they are probably completely separate).

Good point. Actually, I thought I had removed "gamepad' from the Permissions API as it doesn't actually prompt for permission.

I've sent a PR to Permissions API now to remove it.
https://github.com/w3c/permissions/pull/255

  1. I haven't found evidence of Webkit implementation. Chrome seems to be still behind the preference. Do you happen to know if they have implemented this without a preference? (i.e. am I missing evidence that this is released on those platforms too).

I don't think they have. I'm also waiting to when they are planning to remove the pref (it was supposed to be this month).

Flags: needinfo?(marcos)

Thanks Marcos! Docs on this are pretty much done. Still waiting on BCD review of the update - question for you there https://github.com/mdn/browser-compat-data/pull/11770#discussion_r679674364 (essentially where is the spec link I can use?).

We are now in the unfortunate position of shipping this restriction as the only browser even after 6 months.

Regressions: 1870037

Announcement in Hacks:
https://hacks.mozilla.org/2020/07/securing-gamepad-api/

As per comment 32 (but it's now 2.5 years), it's not clear this was a good idea, in any case it basically is just webcompat burn for us at this point:
https://github.com/w3c/gamepad/issues/145

See Also: → 1591329
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: