Closed Bug 1653199 Opened 4 years ago Closed 4 years ago

Web Share should be controlled by permission policy

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: marcosc, Assigned: marcosc)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

Web Share should be controlled by permission policy, called "web-share" and restricted to 'self'.

Do we have some meta bug about Web Share? If so, could you add dependency?

Severity: -- → S3
Flags: needinfo?(mcaceres)
Priority: -- → P3

We don't... I'll create one.

Flags: needinfo?(mcaceres)
Blocks: webshare
Assignee: nobody → mcaceres

Restrict navigator.share() to web-share permission policy

Adds "web-share" permissions policy

Attachment #9164261 - Attachment is obsolete: true
Pushed by mcaceres@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4b17772c446
Add web-share permissions policy r=ckerschb

Backed out changeset e4b17772c446 (bug 1653199) for test_featureList.html failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=dVlyjVTtS8qXiomPMM3JXg.0&searchStr=mochitest&fromchange=dbd8cf9a9c829672446f1b98e529d5c5d53ab8e7&tochange=f5bfa5ea6f045557603e3029d8f79112dda201e6

Backout link: https://hg.mozilla.org/integration/autoland/rev/f5bfa5ea6f045557603e3029d8f79112dda201e6

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310465787&repo=autoland&lineNumber=11983

[task 2020-07-20T23:03:59.999Z] 23:03:59     INFO - TEST-START | dom/security/featurepolicy/test/mochitest/test_featureList.html
[task 2020-07-20T23:04:01.218Z] 23:04:01     INFO - TEST-INFO | started process screenshot
[task 2020-07-20T23:04:01.285Z] 23:04:01     INFO - TEST-INFO | screenshot: exit 0
[task 2020-07-20T23:04:01.286Z] 23:04:01     INFO - Buffered messages logged at 23:04:01
[task 2020-07-20T23:04:01.286Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | We have document.featurePolicy 
[task 2020-07-20T23:04:01.286Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: camera 
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: geolocation 
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: microphone 
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: display-capture 
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: fullscreen 
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO - Buffered messages finished
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO - TEST-UNEXPECTED-FAIL | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: web-share 
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO -     SimpleTest.ok@SimpleTest/SimpleTest.js:412:16
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO -     checkFeatures/<@dom/security/featurepolicy/test/mochitest/test_featureList.html:29:7
[task 2020-07-20T23:04:01.287Z] 23:04:01     INFO -     checkFeatures@dom/security/featurepolicy/test/mochitest/test_featureList.html:28:12
[task 2020-07-20T23:04:01.288Z] 23:04:01     INFO -     @dom/security/featurepolicy/test/mochitest/test_featureList.html:34:14
[task 2020-07-20T23:04:01.288Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | We have HTMLIFrameElement.featurePolicy 
[task 2020-07-20T23:04:01.288Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: camera 
[task 2020-07-20T23:04:01.288Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: geolocation 
[task 2020-07-20T23:04:01.288Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: microphone 
[task 2020-07-20T23:04:01.288Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: display-capture 
[task 2020-07-20T23:04:01.288Z] 23:04:01     INFO - TEST-PASS | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: fullscreen 
[task 2020-07-20T23:04:01.288Z] 23:04:01     INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-07-20T23:04:01.289Z] 23:04:01     INFO - TEST-UNEXPECTED-FAIL | dom/security/featurepolicy/test/mochitest/test_featureList.html | Feature: web-share 
[task 2020-07-20T23:04:01.289Z] 23:04:01     INFO -     SimpleTest.ok@SimpleTest/SimpleTest.js:412:16
[task 2020-07-20T23:04:01.289Z] 23:04:01     INFO -     checkFeatures/<@dom/security/featurepolicy/test/mochitest/test_featureList.html:29:7
[task 2020-07-20T23:04:01.289Z] 23:04:01     INFO -     checkFeatures@dom/security/featurepolicy/test/mochitest/test_featureList.html:28:12
[task 2020-07-20T23:04:01.289Z] 23:04:01     INFO -     @dom/security/featurepolicy/test/mochitest/test_featureList.html:38:14
[task 2020-07-20T23:04:01.289Z] 23:04:01     INFO - GECKO(6020) | MEMORY STAT | vsize 543MB | vsizeMaxContiguous 830MB | residentFast 80MB | heapAllocated 8MB
[task 2020-07-20T23:04:01.309Z] 23:04:01     INFO - TEST-OK | dom/security/featurepolicy/test/mochitest/test_featureList.html | took 1308ms
Flags: needinfo?(mcaceres)

Oops, thanks! Will fix.

Flags: needinfo?(mcaceres)
Attachment #9164261 - Attachment is obsolete: false
Pushed by mcaceres@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce3679646f67
Add web-share permission policy r=ckerschb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Attachment #9164637 - Attachment is obsolete: true

Hi, I have an action to document this fix.

I think this change just adds web-share to the list of Feature-Policy types that are supported, but only for self. From the MDN docs on web-share:

'self': The feature will be allowed in this document, and in all nested browsing contexts (iframes) in the same origin.

A couple of questions:

  1. Does that mean the browser will now respect the header: Feature-Policy: web-share 'self';?
  2. What will it do if a server sets: Feature-Policy: web-share '*'; (or src, or none, or another domain)?
  3. Feature-Policy has a docs note that it is experimental. Is that correct? Looking at the BCD it might be more correct to say that Feature-Policy is not experimental but that individual features are?
  4. I understand that Feature-Policy is going to be deprecated in favour of Permissions-Policy.
    • Do you know we support Permissions Policy yet?
    • When we support it, what happens with Feature-Policy - does it continue to be updated with new features or would be just do a complete replace with "Permission policy"?

I know some of that is out of scope of this particular issue - if you're not sure on this would would you recommend I talk to? FYI :marcosc :smaug

I'm attempting to confirm what this fix means for users, and in particular that is a partial implementation which allows only the "self" feature policy. Please see question above ^^^^

Flags: needinfo?(marcos)
Flags: needinfo?(bugs)

(In reply to Hamish Willee from comment #11)

Hi, I have an action to document this fix.

I think this change just adds web-share to the list of Feature-Policy types that are supported, but only for self. From the MDN docs on web-share:

'self': The feature will be allowed in this document, and in all nested browsing contexts (iframes) in the same origin.

A couple of questions:

  1. Does that mean the browser will now respect the header: Feature-Policy: web-share 'self';?

Yes. Though please be aware that the HTTP header is now called Permissions-Policy: and I think the syntax is slightly differently.

  1. What will it do if a server sets: Feature-Policy: web-share '*'; (or src, or none, or another domain)?

The behavior will be whatever the Permissions Policy says it will be. Presumedly, "*", allow it for any remote third-party. "None" should block it on all, etc. However, please confirm against the Permissions Policy spec.

  1. Feature-Policy has a docs note that it is experimental. Is that correct? Looking at the BCD it might be more correct to say that Feature-Policy is not experimental but that individual features are?

Permissions Policy is indeed experimental. Note what I mentioned above about both the header and syntax changing.

  1. I understand that Feature-Policy is going to be deprecated in favour of Permissions-Policy.

Correct. Heh, should have read all the way through before writing the above.

  • Do you know we support Permissions Policy yet?

No.

  • When we support it, what happens with Feature-Policy - does it continue to be updated with new features or would be just do a complete replace with "Permission policy"?

Replaced completely. Feature Policy is behind a pref. At some point, someone will need to go in and do a big find/replace to update all the things.

I know some of that is out of scope of this particular issue - if you're not sure on this would would you recommend I talk to? FYI :marcosc :smaug

Baku is the right person to confirm all of the above with. I've NI'ed him for you.

Flags: needinfo?(marcos) → needinfo?(amarchesini)

Thanks Marcos (&Baku)- awesome.

I think I've confused you (or maybe just me!) on points 1 and 2. Let me try again.

  • Looking at the code change you made it looks like we support Feature-Policy: web-share 'self'; - so we suppor self but not the other options - *, src.
  • So that is why I ask question 2 - if we only support the self option then presumably the browser has to do "something" if it gets the other options. I was wondering if we know what?

Based on the rest of your response I guess Permissions-Policy isn't yet supported so I document this as Feature-Policy. Baku do you know approximate timeframe when we will need to do the MDN cleanup on the headers?

(In reply to Hamish Willee from comment #14)

Thanks Marcos (&Baku)- awesome.

I think I've confused you (or maybe just me!) on points 1 and 2. Let me try again.

  • Looking at the code change you made it looks like we support Feature-Policy: web-share 'self'; - so we suppor self but not the other options - *, src.

Oh, that would be a bug. We should support "self" by default, but should allow the others.

Baku, did I miss something in the patch?

  • So that is why I ask question 2 - if we only support the self option then presumably the browser has to do "something" if it gets the other options. I was wondering if we know what?

It should behave like any other Permission Policy controlled feature.

Based on the rest of your response I guess Permissions-Policy isn't yet supported so I document this as Feature-Policy. Baku do you know approximate timeframe when we will need to do the MDN cleanup on the headers?

... I have the same question :) Baku, let me know if I can be of help with the refactor.

@Hamish, just wondering, what lead you to conclude the other options are not supported?

@marcos - the linked patch https://hg.mozilla.org/integration/autoland/rev/ce3679646f67#l2.12

Shows addition of {"web-share", FeaturePolicyUtils::FeaturePolicyValue::eSelf} to static FeatureMap sSupportedFeatures[]. Which I interpretted as supporting only the policy self.

Flags: needinfo?(bugs)

@marcos - the linked patch https://hg.mozilla.org/integration/autoland/rev/ce3679646f67#l2.12

Shows addition of {"web-share", FeaturePolicyUtils::FeaturePolicyValue::eSelf} to static FeatureMap sSupportedFeatures[]. Which I interpreted as supporting only the policy self.

Though perhaps that just means I can't read code and means "self is the default"?

(In reply to Hamish Willee from comment #18)

Though perhaps that just means I can't read code and means "self is the default"?

No, I think you've raised a good point. It's probably something we need to clarify in the code, even if we just add a comment above sSupportedFeatures making it a bit more clear. Let's wait for Baku to confirm :)

Thanks @marcos. I assume Baku is amarchesini in the Needinfo request? I'm hoping to get this all closed down soon for the FF82 release in a couple of weeks.

Clearing request from Baku.

Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: