Bug 197751 – [Apple Pay] Payment APIs should be completely disabled in web views into which clients have injected user scripts

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

[Andy Estes]

[Apple Pay] Payment APIs should be disabled in web views that clients have injected user scripts i into

[Andy Estes]

rdar://problem/50631563

[Andy Estes]

Created attachment 369514 [details]
Patch

[Andy Estes]

Created attachment 369516 [details]
Patch

[Sam Weinig]

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?

[Andy Estes]

(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().

[Alex Christensen]

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.

[Geoffrey Garen]

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&);

[Andy Estes]

(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.

[Andy Estes]

Created attachment 369616 [details]
Patch

[Andy Estes]

Created attachment 369620 [details]
Patch

[Andy Estes]

(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].

[Andy Estes]

Created attachment 369889 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 369889 [details]
Patch

Clearing flags on attachment: 369889

Committed r245314: <https://trac.webkit.org/changeset/245314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.