Bug 214448 - Web Share permission policy "web-share" and "self" as the allowlist
Summary: Web Share permission policy "web-share" and "self" as the allowlist
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: https://github.com/w3c/web-share/pull...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-17 01:12 PDT by Marcos Caceres
Modified: 2021-09-20 07:22 PDT (History)
10 users (show)

See Also:


Attachments
Patch (23.55 KB, patch)
2021-09-02 06:38 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (23.47 KB, patch)
2021-09-02 08:51 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (23.84 KB, patch)
2021-09-02 14:55 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (23.71 KB, patch)
2021-09-02 17:30 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (32.56 KB, patch)
2021-09-03 00:13 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (25.82 KB, patch)
2021-09-03 04:29 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (26.30 KB, patch)
2021-09-08 18:35 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (25.98 KB, patch)
2021-09-08 20:35 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (25.90 KB, patch)
2021-09-08 20:41 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (26.65 KB, patch)
2021-09-14 19:26 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (26.66 KB, patch)
2021-09-19 23:10 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcos Caceres 2020-07-17 01:12:47 PDT
Calling .share() should be restricted by permission policy "web-share" with a "self" allowlist. 

https://github.com/w3c/web-share/pull/166/
Comment 1 Radar WebKit Bug Importer 2020-07-18 11:28:43 PDT
<rdar://problem/65771552>
Comment 2 Marcos Caceres 2021-09-02 06:38:29 PDT
Created attachment 437144 [details]
Patch
Comment 3 Marcos Caceres 2021-09-02 08:51:26 PDT
Created attachment 437154 [details]
Patch
Comment 4 Devin Rousso 2021-09-02 12:26:07 PDT
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.
Comment 5 Marcos Caceres 2021-09-02 14:45:29 PDT
(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?).
Comment 6 Marcos Caceres 2021-09-02 14:55:08 PDT
Created attachment 437200 [details]
Patch
Comment 7 Devin Rousso 2021-09-02 15:38:08 PDT
(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 :)
Comment 8 Marcos Caceres 2021-09-02 15:43:37 PDT
(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
Comment 9 Marcos Caceres 2021-09-02 17:30:54 PDT
Created attachment 437223 [details]
Patch
Comment 10 Devin Rousso 2021-09-02 21:39:58 PDT
Comment on attachment 437223 [details]
Patch

r=me once EWS is happy :)
Comment 11 Devin Rousso 2021-09-02 21:43:10 PDT
(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`?  🤔
Comment 12 Marcos Caceres 2021-09-02 22:01:48 PDT
(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
Comment 13 Marcos Caceres 2021-09-02 23:03:55 PDT
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?
Comment 14 Tim Horton 2021-09-02 23:34:22 PDT
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
Comment 15 Marcos Caceres 2021-09-02 23:49:06 PDT
(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.
Comment 16 Marcos Caceres 2021-09-03 00:13:00 PDT
Created attachment 437248 [details]
Patch
Comment 17 Tim Horton 2021-09-03 01:50:15 PDT
(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.
Comment 18 Tim Horton 2021-09-03 01:51:41 PDT
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).
Comment 19 Marcos Caceres 2021-09-03 04:29:59 PDT
Created attachment 437259 [details]
Patch
Comment 20 Marcos Caceres 2021-09-03 04:32:35 PDT
(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 🤞
Comment 21 Tim Horton 2021-09-03 13:53:29 PDT
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.
Comment 22 Marcos Caceres 2021-09-08 18:35:23 PDT
Created attachment 437694 [details]
Patch
Comment 23 Marcos Caceres 2021-09-08 18:44:44 PDT
(In reply to Tim Horton from comment #21)
> Still one failure on ios-wk2 but otherwise seems reasonable.

Investigating that now.
Comment 24 Marcos Caceres 2021-09-08 20:35:36 PDT
Created attachment 437702 [details]
Patch
Comment 25 Marcos Caceres 2021-09-08 20:41:23 PDT
Created attachment 437703 [details]
Patch
Comment 26 Marcos Caceres 2021-09-14 19:26:04 PDT
Created attachment 438206 [details]
Patch
Comment 27 Marcos Caceres 2021-09-14 19:29:16 PDT
(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 ^_^
Comment 28 Marcos Caceres 2021-09-19 23:10:43 PDT
Created attachment 438640 [details]
Patch
Comment 29 Marcos Caceres 2021-09-19 23:11:31 PDT
Rebased.
Comment 30 youenn fablet 2021-09-20 03:16:49 PDT
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?
Comment 31 Marcos Caceres 2021-09-20 06:33:47 PDT
(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
Comment 32 Marcos Caceres 2021-09-20 06:43:22 PDT
Sent spec pull request https://github.com/w3c/web-share/pull/220
Comment 33 youenn fablet 2021-09-20 07:01:17 PDT
(In reply to Marcos Caceres from comment #32)
> Sent spec pull request https://github.com/w3c/web-share/pull/220

Looks good then.
Comment 34 EWS 2021-09-20 07:22:10 PDT
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].