Summary: | Web Share permission policy "web-share" and "self" as the allowlist | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marcos Caceres <marcos> | ||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, changseok, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, thorton, tomac, webkit-bug-importer, youennf | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
URL: | https://github.com/w3c/web-share/pull/166/ | ||||||||||||||||||||||||||
Attachments: |
|
Description
Marcos Caceres
2020-07-17 01:12:47 PDT
Created attachment 437144 [details]
Patch
Created attachment 437154 [details]
Patch
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. (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?). Created attachment 437200 [details]
Patch
(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 :) (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 Created attachment 437223 [details]
Patch
Comment on attachment 437223 [details]
Patch
r=me once EWS is happy :)
(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`? 🤔 (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 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? 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 (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. Created attachment 437248 [details]
Patch
(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 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). Created attachment 437259 [details]
Patch
(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 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. Created attachment 437694 [details]
Patch
(In reply to Tim Horton from comment #21) > Still one failure on ios-wk2 but otherwise seems reasonable. Investigating that now. Created attachment 437702 [details]
Patch
Created attachment 437703 [details]
Patch
Created attachment 438206 [details]
Patch
(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 ^_^ Created attachment 438640 [details]
Patch
Rebased. 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? (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 Sent spec pull request https://github.com/w3c/web-share/pull/220 (In reply to Marcos Caceres from comment #32) > Sent spec pull request https://github.com/w3c/web-share/pull/220 Looks good then. 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]. 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? We could start with a quirk for now (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 (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. > Twitter is not same origin but same site which is better than arbitrary
> third-party iframes
Scratch that.
|