Bug 197751 - [Apple Pay] Payment APIs should be completely disabled in web views into which clients have injected user scripts
Summary: [Apple Pay] Payment APIs should be completely disabled in web views into whic...
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: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-09 12:24 PDT by Andy Estes
Modified: 2019-05-14 15:50 PDT (History)
13 users (show)

See Also:


Attachments
Patch (48.00 KB, patch)
2019-05-09 12:39 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (48.04 KB, patch)
2019-05-09 12:46 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (50.13 KB, patch)
2019-05-10 16:21 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (50.15 KB, patch)
2019-05-10 16:33 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (50.15 KB, patch)
2019-05-14 13:57 PDT, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2019-05-09 12:24:01 PDT
[Apple Pay] Payment APIs should be disabled in web views that clients have injected user scripts i into
Comment 1 Andy Estes 2019-05-09 12:24:51 PDT
rdar://problem/50631563
Comment 2 Andy Estes 2019-05-09 12:39:22 PDT Comment hidden (obsolete)
Comment 3 Andy Estes 2019-05-09 12:46:35 PDT
Created attachment 369516 [details]
Patch
Comment 4 Sam Weinig 2019-05-09 12:55:48 PDT
Comment on attachment 369516 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        In r243324, when a document has had user agent scripts injected into it, payment APIs were
> +        disabled at runtime by having all entry points return falsy values or throw exceptions
> +        (e.g., ApplePaySession.canMakePayments() returns false).
> +
> +        In the case of user scripts in particular (e.g., WKUserScript), since we know whether these
> +        exist at the time we create a document's DOMWindow, we can do better than r243324 by
> +        completely disabling the payment APIs in the presence of user scripts.

It seems like it will be more confusing and more work for developers to have to deal with with these both (falsy values and completed disabled) cases. Since the developer already has to deal with the first case, why introduce this as well?
Comment 5 Andy Estes 2019-05-09 12:59:50 PDT
(In reply to Sam Weinig from comment #4)
> Comment on attachment 369516 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369516&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        In r243324, when a document has had user agent scripts injected into it, payment APIs were
> > +        disabled at runtime by having all entry points return falsy values or throw exceptions
> > +        (e.g., ApplePaySession.canMakePayments() returns false).
> > +
> > +        In the case of user scripts in particular (e.g., WKUserScript), since we know whether these
> > +        exist at the time we create a document's DOMWindow, we can do better than r243324 by
> > +        completely disabling the payment APIs in the presence of user scripts.
> 
> It seems like it will be more confusing and more work for developers to have
> to deal with with these both (falsy values and completed disabled) cases.
> Since the developer already has to deal with the first case, why introduce
> this as well?

We've found some cases where sites check for API availability but do not use the API properly. For instance, some sites will render Apple Pay buttons whenever window.ApplePaySession exists without first calling canMakePayments().
Comment 6 Alex Christensen 2019-05-09 14:00:29 PDT
Comment on attachment 369516 [details]
Patch

We should add a test that stores a reference to a PaymentRequest, has scripts injected then verify that using that stored reference still gets falsy values.
Comment 7 Geoffrey Garen 2019-05-10 11:49:02 PDT
Is this a bug in the new test?

FAIL: (JS) JSTestEnabledForContext.cpp
--- WebCore/bindings/scripts/test/JS/JSTestEnabledForContext.cpp	2019-05-09 12:47:19.000000000 -0700
+++ /var/folders/19/dls3q4zx3yvd609xbdvv189h0000gn/T/tmpOkWHzN/JSTestEnabledForContext.cpp	2019-05-09 12:47:36.000000000 -0700
@@ -69,7 +69,6 @@
     JSTestEnabledForContextPrototype(JSC::VM& vm, JSC::JSGlobalObject*, JSC::Structure* structure)
         : JSC::JSNonFinalObject(vm, structure)
     {
-        didBecomePrototype();
     }
 
     void finishCreation(JSC::VM&);
Comment 8 Andy Estes 2019-05-10 12:00:15 PDT
(In reply to Geoffrey Garen from comment #7)
> Is this a bug in the new test?
> 
> FAIL: (JS) JSTestEnabledForContext.cpp
> --- WebCore/bindings/scripts/test/JS/JSTestEnabledForContext.cpp	2019-05-09
> 12:47:19.000000000 -0700
> +++
> /var/folders/19/dls3q4zx3yvd609xbdvv189h0000gn/T/tmpOkWHzN/
> JSTestEnabledForContext.cpp	2019-05-09 12:47:36.000000000 -0700
> @@ -69,7 +69,6 @@
>      JSTestEnabledForContextPrototype(JSC::VM& vm, JSC::JSGlobalObject*,
> JSC::Structure* structure)
>          : JSC::JSNonFinalObject(vm, structure)
>      {
> -        didBecomePrototype();
>      }
>  
>      void finishCreation(JSC::VM&);

No, I just need to rebase the result after r245082.
Comment 9 Andy Estes 2019-05-10 16:21:13 PDT Comment hidden (obsolete)
Comment 10 Andy Estes 2019-05-10 16:33:45 PDT
Created attachment 369620 [details]
Patch
Comment 11 Andy Estes 2019-05-10 17:00:18 PDT
(In reply to Alex Christensen from comment #6)
> Comment on attachment 369516 [details]
> Patch
> 
> We should add a test that stores a reference to a PaymentRequest, has
> scripts injected then verify that using that stored reference still gets
> falsy values.

I added the test you suggested to attachment #369620 [details].
Comment 12 Andy Estes 2019-05-14 13:57:53 PDT
Created attachment 369889 [details]
Patch
Comment 13 WebKit Commit Bot 2019-05-14 15:50:27 PDT
Comment on attachment 369889 [details]
Patch

Clearing flags on attachment: 369889

Committed r245314: <https://trac.webkit.org/changeset/245314>
Comment 14 WebKit Commit Bot 2019-05-14 15:50:29 PDT
All reviewed patches have been landed.  Closing bug.