Bug 229406 - Add "payment" permissions policy
Summary: Add "payment" permissions policy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari 14
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 167417 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-08-23 03:55 PDT by Marcos Caceres
Modified: 2021-10-01 17:57 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.98 KB, patch)
2021-08-23 04:06 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (16.85 KB, patch)
2021-09-01 05:03 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (16.60 KB, patch)
2021-09-01 13:54 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 2021-08-23 03:55:20 PDT
The Payment Request API [1] defines defines a policy-controlled feature identified by the string "payment" [permissions-policy]. Its default allowlist is 'self'.

However, WebKit currently doesn't include "payment" as policy controlled feature. 


[1] https://www.w3.org/TR/payment-request/#permissions-policy
Comment 1 Marcos Caceres 2021-08-23 04:06:38 PDT
Created attachment 436180 [details]
Patch
Comment 2 Marcos Caceres 2021-08-24 17:33:08 PDT
Lacking tests right now, for reasons we discussed on Slack... with a little guidance, I should be able to add them.
Comment 3 youenn fablet 2021-08-26 00:05:50 PDT
We have some WebKit specific tests, for instance LayoutTests/http/tests/media/media-stream/enumerate-devices-iframe-allow-attribute.html.
We also have WPT LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-default-feature-policy.https.html for instance.
Comment 4 Marcos Caceres 2021-08-26 00:07:06 PDT
(In reply to youenn fablet from comment #3)
> We have some WebKit specific tests, for instance
> LayoutTests/http/tests/media/media-stream/enumerate-devices-iframe-allow-
> attribute.html.
> We also have WPT
> LayoutTests/imported/w3c/web-platform-tests/mediacapture-streams/MediaStream-
> default-feature-policy.https.html for instance.

Thanks Youenn! I'll create some tests off those.
Comment 5 Devin Rousso 2021-08-26 14:33:30 PDT
Comment on attachment 436180 [details]
Patch

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

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:294
> +    if (!isFeaturePolicyAllowedByDocumentAndAllOwners(FeaturePolicy::Type::Payment, document, LogFeaturePolicyFailure::Yes))

Rather than doing this here, I wonder if this should be moved to `PaymentSession::canCreateSession` so that stuff like `ApplePaySession.canMakePayments` would also throw unless the `"payment"` feature policy is allowed.  This would also cover someone trying to use the Apple Pay JS API <https://developer.apple.com/documentation/apple_pay_on_the_web/apple_pay_js_api>, which I'd imagine should also be covered by this.
Comment 6 Sam Sneddon [:gsnedders] 2021-08-29 14:22:09 PDT
is this just a dupe of Bug 167417? though given work has been happening here, maybe we should just forward-dupe that to here
Comment 7 Marcos Caceres 2021-08-29 21:01:03 PDT
(In reply to Sam Sneddon [:gsnedders] from comment #6)
> is this just a dupe of Bug 167417? 

Yes, indeed. Although the scope of this one is a little bigger (it's the iframe and respecting the http header).  

> though given work has been happening
> here, maybe we should just forward-dupe that to here

Yes, that be great.
Comment 8 Radar WebKit Bug Importer 2021-08-30 03:56:26 PDT
<rdar://problem/82518272>
Comment 9 Marcos Caceres 2021-09-01 05:03:28 PDT
Created attachment 437018 [details]
Patch
Comment 10 Sam Sneddon [:gsnedders] 2021-09-01 06:57:23 PDT
*** Bug 167417 has been marked as a duplicate of this bug. ***
Comment 11 Devin Rousso 2021-09-01 12:10:20 PDT
Comment on attachment 437018 [details]
Patch

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

r=me

> Source/WebCore/Modules/applepay/PaymentSession.cpp:54
> +        return Exception { SecurityError, "Payment Request API is disabled Permissions Policy."_s };

Can we make this error into an actual sentence?  Maybe `"Third-party iframes are not allowed to request payments unless explicitly allowed via Feature-Policy (payment)"`?

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:34
> +#include "FeaturePolicy.h"

NIT: I don't think this is needed anymore
Comment 12 Marcos Caceres 2021-09-01 13:54:22 PDT
Created attachment 437065 [details]
Patch
Comment 13 EWS 2021-09-01 15:54:49 PDT
Committed r281885 (241215@main): <https://commits.webkit.org/241215@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437065 [details].
Comment 14 Brad 2021-09-13 16:15:51 PDT
Hi glad to see progress here! This doesn't seem to resolve this duplicate issue: https://bugs.webkit.org/show_bug.cgi?id=226345.

The issue is that https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/applepay/PaymentSession.cpp?rev=281885#L66 still requires the origins of ancestors frames to match. This seems reasonable to remove now that payments policy is enforced (and is not available to third parties by default).
Comment 15 Marcos Caceres 2021-09-14 17:15:46 PDT
(In reply to Brad from comment #14)
> Hi glad to see progress here! This doesn't seem to resolve this duplicate
> issue: https://bugs.webkit.org/show_bug.cgi?id=226345.
> 
> The issue is that
> https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/Modules/applepay/
> PaymentSession.cpp?rev=281885#L66 still requires the origins of ancestors
> frames to match. This seems reasonable to remove now that payments policy is
> enforced (and is not available to third parties by default).

Yes, that's definitely true. I'm guessing that's a product decision and not a bug? 

It would be great to get input from Apple Pay folks on the above, as there may be technical or security issues around the same origin enforcement.
Comment 16 Brad 2021-10-01 17:57:07 PDT
Sure is there an update from them? I reopened the other ticket.

From the developer perspective, it makes it difficult to build a safe Apple Pay integration when using third party vendors, which need to be trusted to process payments but not to receive access to sensitive user data and cookies on the top level origin.

This top level origin restriction made sense from a security perspective before the "payment" permissions policy was implemented, to prevent arbitrary iframes from asking for payments that the top origin didn't intend. But now that this attribute works and is default denied, it seems safe to allow top origins to delegate the ability to their own payment vendors in specific iframes.