Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spec] Remove user-identifiable information from canMakePayment #404

Merged

Conversation

stephenmcgruer
Copy link
Collaborator

@stephenmcgruer stephenmcgruer commented Nov 15, 2022

The “canmakepayment” service worker event lets the merchant know whether the user has a card on file in an installed payment app. It silently passes the merchant's origin and arbitrary data to a service worker from payment app origin. This cross-origin communication happens on PaymentRequest construction in JavaScript, does not require a user gesture, and does not show any user interface. As such, it is a potential source of silent user tracking in a post-3p cookies world.

See #401 for discussions around use-cases for the canmakepayment event.

The following tasks have been completed:

  • web platform tests (link) - N/A (tests are sadly currently in a poor enough state that the entire test suite needs rewritten)
  • MDN Docs added - N/A? (I cannot locate PaymentHandler docs on MDN...)

Implementation commitment:

  • Safari - N/A, does not ship PaymentHandler
  • Chrome (link to issue)
  • FirefoxN/A, does not ship PaymentRequest or PaymentHandler
  • Edge (public signal)

Preview | Diff

The “canmakepayment” service worker event lets the merchant know whether the
user has a card on file in an installed payment app. It silently passes the
merchant's origin and arbitrary data to a service worker from payment app
origin. This cross-origin communication happens on PaymentRequest construction
in JavaScript, does not require a user gesture, and does not show any user
interface. As such, it is a potential source of silent user tracking in a
post-3p cookies world.
@stephenmcgruer
Copy link
Collaborator Author

(I do not appear to be able to set reviewers in this repository, so cc @rsolomakhin @ianbjacobs for review).

Copy link
Member

@romandev romandev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@stephenmcgruer
Copy link
Collaborator Author

This was discussed in the WPWG today (minutes). We heard no objections and therefore I believe it is clear to land - once I update the WPT tests. (Any day now... 🤣 )

@stephenmcgruer
Copy link
Collaborator Author

stephenmcgruer commented Dec 14, 2022

@ianbjacobs @rsolomakhin - ok, after a few days of looking into the tests, I'm sorry to say I think I need to ask that we merge this without updating the WPT tests. The reason is that the test suite is in such a sorry state that it is currently so far from passing even without this change today, which makes testing this fairly impossible without re-writing the entire test suite to fix all the other problems.

A short list of problem I found so far:

  • Tests treat supportedMethods as a list still, not a string
  • Tests assert that additionalDisplayItems and total are not in modifiers for "canmakepaymentevent", which they are in Chrome's implementation
  • Tests assert that modifier.data exists for "canmakepaymentevent", and it doesn't seem to for Chrome
  • Tests presume that "canmakepaymentevent" affects canMakePayment(), not hasEnrolledInstrument()
  • Tests presume that if "canmakepaymentevent" returns false, then show() will throw a NotSupportedError
  • Tests try to reject the "canmakepaymentevent" respondWith with Error objects that have actual messages, which is not supported afaik (maybe they silently become false?)

Some of these may be actual bugs in Chrome, whilst some reflect how both the PaymentHandler spec and test suite have diverged from changes in both PaymentRequest spec and in the reality of the Chrome implementation. In all worlds, the test suite (and spec) desperately needs some TLC to at least reflect the shipping reality. But for now, I'd like to just merge this change and address that larger concern later.

(I will open a follow-on issue about the test suite)

@rsolomakhin
Copy link
Collaborator

rsolomakhin commented Dec 14, 2022

merge this without updating the WPT tests

This sounds reasonable given your explanation. If the other reviewers are OK, too, then I can merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants