Bug 214448 – Web Share permission policy "web-share" and "self" as the allowlist

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marcos Caceres]

Calling .share() should be restricted by permission policy "web-share" with a "self" allowlist. 

https://github.com/w3c/web-share/pull/166/

[Radar WebKit Bug Importer]

<rdar://problem/65771552>

[Marcos Caceres]

Created attachment 437144 [details]
Patch

[Marcos Caceres]

Created attachment 437154 [details]
Patch

[Devin Rousso]

Comment on attachment 437154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437154&action=review

> Source/WebCore/page/Navigator.cpp:131
> +    if (!frame || !frame->page() || !isFeaturePolicyAllowedByDocumentAndAllOwners(FeaturePolicy::Type::WebShare, *frame->document(), LogFeaturePolicyFailure::Yes))

It's a bit unfortunate that this is here since it means that we'll check `isFeaturePolicyAllowedByDocumentAndAllOwners` twice when calling `Navigator::share`, but I suppose that's not the worst thing 🤔

NIT: I'd put this check on another line

> Source/WebCore/page/Navigator.cpp:148
> +    auto* frame = this->frame();
> +    if (!isFeaturePolicyAllowedByDocumentAndAllOwners(FeaturePolicy::Type::WebShare, *frame->document(), LogFeaturePolicyFailure::Yes)) {

Why not just use the `document` argument?

Also, EWS appears to be crashing in this function.

[Marcos Caceres]

(In reply to Devin Rousso from comment #4)
> Comment on attachment 437154 [details]
> Patch
> > Source/WebCore/page/Navigator.cpp:148
> > +    auto* frame = this->frame();
> > +    if (!isFeaturePolicyAllowedByDocumentAndAllOwners(FeaturePolicy::Type::WebShare, *frame->document(), LogFeaturePolicyFailure::Yes)) {
> 
> Why not just use the `document` argument?

oops, yeah... copy/pasta from the other patch.  
 
> Also, EWS appears to be crashing in this function.

Question: canShare() does this check:

```
    auto* frame = this->frame();
    if (!frame || !frame->page())
        return false;
```

As a followup, I wonder if we should do the same check in .share() and reject with something? The spec doesn't currently to cover this (or maybe this should change to be a "fully active" check?).

[Marcos Caceres]

Created attachment 437200 [details]
Patch

[Devin Rousso]

(In reply to Marcos Caceres from comment #5)
> (In reply to Devin Rousso from comment #4) 
> > Also, EWS appears to be crashing in this function.
> 
> Question: canShare() does this check:
> 
> ```
>     auto* frame = this->frame();
>     if (!frame || !frame->page())
>         return false;
> ```
> 
> As a followup, I wonder if we should do the same check in .share() and
> reject with something? The spec doesn't currently to cover this (or maybe
> this should change to be a "fully active" check?).

`share()` calls `canShare()`, so it should already be checked :)

[Marcos Caceres]

(In reply to Devin Rousso from comment #7) 
> > As a followup, I wonder if we should do the same check in .share() and
> > reject with something? The spec doesn't currently to cover this (or maybe
> > this should change to be a "fully active" check?).
> 
> `share()` calls `canShare()`, so it should already be checked :)

Indeed. Just checked, and all browsers do different things here tho: 

 * WebKit -> TypeError
 * Firefox -> Internal "UNEXPECTED_ERROR"
 * Chrome -> NotAllowedError

https://marcoscaceres.github.io/playground/share-parent.html

[Marcos Caceres]

Created attachment 437223 [details]
Patch

[Devin Rousso]

Comment on attachment 437223 [details]
Patch

r=me once EWS is happy :)

[Devin Rousso]

(In reply to Marcos Caceres from comment #8)
> (In reply to Devin Rousso from comment #7) 
> > > As a followup, I wonder if we should do the same check in .share() and reject with something? The spec doesn't currently to cover this (or maybe this should change to be a "fully active" check?).
> > 
> > `share()` calls `canShare()`, so it should already be checked :)
> 
> Indeed. Just checked, and all browsers do different things here tho: 
> 
>  * WebKit -> TypeError
>  * Firefox -> Internal "UNEXPECTED_ERROR"
>  * Chrome -> NotAllowedError
> 
> https://marcoscaceres.github.io/playground/share-parent.html

Interesting.  `TypeError` feels kinda wrong to me.  Maybe it should be `InvalidStateError`?  🤔

[Marcos Caceres]

(In reply to Devin Rousso from comment #11)
> Interesting.  `TypeError` feels kinda wrong to me.  Maybe it should be
> `InvalidStateError`?  🤔

Coincidently, Kagami from Mozilla said the same thing [1]. I don't mind TypeError, as this is quite an edge-case. Also, using TypeError is "cheap", as the fully-active check happens now in canUse(). 

Alternatively, we can do the check in both share() and canUse(), which is fine too.

Let me know which you prefer and I can update the pull request for the spec [2].   

[1] https://github.com/w3c/web-share/issues/218#issuecomment-912122895
[2] https://github.com/w3c/web-share/pull/219

[Marcos Caceres]

WRT to the tests, could it be that the API is not exposed on "mac-wk1"?
  
I can see from the logs that:

 - "navigator.canShare is undefined".
 - "navigator.share is not a function."

On Windows, looks like the click is not going through. 

Any suggestions?

[Tim Horton]

The API is definitely not exposed (or at least should not be, because the UI parts aren’t implemented) on Mac-wk1. Dunno about windows, but it seems almost certain that it’s not implemented there either

[Marcos Caceres]

(In reply to Tim Horton from comment #14)
> The API is definitely not exposed (or at least should not be, because the UI
> parts aren’t implemented) on Mac-wk1. Dunno about windows, but it seems
> almost certain that it’s not implemented there either

Ok, thanks for confirming. It's ok if I set the expectations to fail on those two platforms? Presumedly yes.

I'll update the patch and wait for a confirmation.

[Marcos Caceres]

Created attachment 437248 [details]
Patch

[Tim Horton]

(In reply to Marcos Caceres from comment #15)
> (In reply to Tim Horton from comment #14)
> > The API is definitely not exposed (or at least should not be, because the UI
> > parts aren’t implemented) on Mac-wk1. Dunno about windows, but it seems
> > almost certain that it’s not implemented there either
> 
> Ok, thanks for confirming. It's ok if I set the expectations to fail on
> those two platforms? Presumedly yes.

Definitely (or skip them, there’s little point running them).

> I'll update the patch and wait for a confirmation.

[Tim Horton]

Comment on attachment 437248 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437248&action=review

> LayoutTests/platform/win/webshare/webshare-allow-attribute-canShare.https-expected.txt:1
> +FAIL: Timed out waiting for notifyDone to be called

You don’t want to do this, I think, because it means the test had to wait for the timeout. Add the directory as a skip to this platform’s TestExpectations file (I’m surprised it’s not already).

[Marcos Caceres]

Created attachment 437259 [details]
Patch

[Marcos Caceres]

(In reply to Tim Horton from comment #18) 
> You don’t want to do this, I think, because it means the test had to wait
> for the timeout. Add the directory as a skip to this platform’s
> TestExpectations file (I’m surprised it’s not already).

thanks! I didn't know about the TestExpectations file. All makes sense now ^_^ 

Hopefully I got it right this time 🤞

[Tim Horton]

Comment on attachment 437259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437259&action=review

Still one failure on ios-wk2 but otherwise seems reasonable.

> LayoutTests/platform/mac-wk1/TestExpectations:1419
> +http/tests/webshare/webshare-allow-attribute-canShare.https.html [ Skip ]
> +http/tests/webshare/webshare-allow-attribute-share.https.html [ Skip ]

Probably just want to skip the whole directory?

> LayoutTests/platform/win/TestExpectations:4694
> +http/tests/webshare/webshare-allow-attribute-canShare.https.html [ Skip ]
> +http/tests/webshare/webshare-allow-attribute-share.https.html [ Skip ]

Ditto.

[Marcos Caceres]

Created attachment 437694 [details]
Patch

[Marcos Caceres]

(In reply to Tim Horton from comment #21)
> Still one failure on ios-wk2 but otherwise seems reasonable.

Investigating that now.

[Marcos Caceres]

Created attachment 437702 [details]
Patch

[Marcos Caceres]

Created attachment 437703 [details]
Patch

[Marcos Caceres]

Created attachment 438206 [details]
Patch

[Marcos Caceres]

(In reply to Marcos Caceres from comment #26)
> Created attachment 438206 [details]
> Patch

The click on "webshare-allow-attribute-share.https.html" tests ended up being racy, which was causing iOS/wk2 to fail. Also, I needed to send a "tap" on iOS, not a click.  

I refactored that test so that each iframe is loaded dynamically, the click/tap is sent, and then each iframe is removed... rinse and repeat. 

Hopefully all good now ^_^

[Marcos Caceres]

Created attachment 438640 [details]
Patch

[Marcos Caceres]

Rebased.

[youenn fablet]

Comment on attachment 438640 [details]
Patch

r=me if canShare implementation is aligning with other browsers and/or ongoing spec work.

View in context: https://bugs.webkit.org/attachment.cgi?id=438640&action=review

> Source/WebCore/page/Navigator.cpp:134
> +        return false;

I am unsure whether we actually want that check.
https://w3c.github.io/web-share/#canshare-data-method does not mention this check. Is the idea to update the spec?
What are doing other browsers?

[Marcos Caceres]

(In reply to youenn fablet from comment #30)
> Comment on attachment 438640 [details]
> Patch
> 
> r=me if canShare implementation is aligning with other browsers and/or
> ongoing spec work.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=438640&action=review
> 
> > Source/WebCore/page/Navigator.cpp:134
> > +        return false;
> 
> I am unsure whether we actually want that check.
> https://w3c.github.io/web-share/#canshare-data-method does not mention this
> check. Is the idea to update the spec?
> What are doing other browsers?

Oh, whoops! Good catch. I thought I added that to spec. I'll send a PR now.  

I have the check as part of the Gecko patch for ::CanShare() - line 1495:
https://phabricator.services.mozilla.com/D90834

[Marcos Caceres]

Sent spec pull request https://github.com/w3c/web-share/pull/220

[youenn fablet]

(In reply to Marcos Caceres from comment #32)
> Sent spec pull request https://github.com/w3c/web-share/pull/220

Looks good then.

[EWS]

Committed r282746 (241883@main): <https://commits.webkit.org/241883@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 438640 [details].

[Tim Horton]

Turns out this change is not web-compatible; Twitter embeds use Web Share but don't have the feature set (and are always cross-origin). Should we roll this out?

[youenn fablet]

We could start with a quirk for now

[Marcos Caceres]

(In reply to youenn fablet from comment #36)
> We could start with a quirk for now

In case it helps, a little more context on the web compat situation: Firefox ships with the policy set to 'self', but web share is only supported on Firefox for Windows.

Alternatively, if we can't get Chrome to change (or it's too late because web compat), we could set the allow list to "all" both in WebKit and in the spec. That would retain web compat with Chrome, while also giving more priv/sec aware sites control over the permissions policy.

The thing to consider is if allowing web share liberally in third party contexts could have significant user privacy or security implications (as happened previously [1]). There is still ongoing work to better secure the API (e.g., [2]). 

Let me know how you would like to proceed. I'm happy to update the spec, Gecko, and WebKit.


[1] https://blog.redteam.pl/2020/08/stealing-local-files-using-safari-web.html
[2] https://github.com/w3c/web-share/issues/178

[youenn fablet]

(In reply to Marcos Caceres from comment #37)
> (In reply to youenn fablet from comment #36)
> > We could start with a quirk for now
> 
> In case it helps, a little more context on the web compat situation: Firefox
> ships with the policy set to 'self', but web share is only supported on
> Firefox for Windows.
> 
> Alternatively, if we can't get Chrome to change (or it's too late because
> web compat), we could set the allow list to "all" both in WebKit and in the
> spec. That would retain web compat with Chrome, while also giving more
> priv/sec aware sites control over the permissions policy.

Is there a bug tracker for Chrome? Do you know their position?

> The thing to consider is if allowing web share liberally in third party
> contexts could have significant user privacy or security implications (as
> happened previously [1]). There is still ongoing work to better secure the
> API (e.g., [2]). 

Twitter is not same origin but same site which is better than arbitrary third-party iframes. Let's add a quirk for now, https://bugs.webkit.org/show_bug.cgi?id=235502.

[youenn fablet]

> Twitter is not same origin but same site which is better than arbitrary
> third-party iframes

Scratch that.