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

validate supportedMethods #464

Closed
marcoscaceres opened this issue Mar 20, 2017 · 16 comments · Fixed by #581
Closed

validate supportedMethods #464

marcoscaceres opened this issue Mar 20, 2017 · 16 comments · Fixed by #581
Assignees
Milestone

Comments

@marcoscaceres
Copy link
Member

marcoscaceres commented Mar 20, 2017

The spec currently specifies:

dictionary PaymentMethodData {
    DOMString supportedMethods;
};

But those are not actually strings: they are either enums (e.g., "basic-card") or USVStrings for URLs. This causes problems when doing URL comparisons as required by the Payment Method Identifiers spec (which calls into the URL spec).

@ianbjacobs
Copy link
Collaborator

+1: they are enums or restricted strings for URLs.

@domenic
Copy link
Collaborator

domenic commented Mar 20, 2017

They need to be parsed into URLs at some point. Does the spec not say when that happens?

@marcoscaceres
Copy link
Member Author

It doesn't look like it. I was expecting it maybe in "Process payment methods" during construction of PaymentRequest.

@marcoscaceres
Copy link
Member Author

Related: w3c/payment-method-id#20

@ianbjacobs
Copy link
Collaborator

@marcoscaceres and @domenic,

Good catch.

The Payment Method Identifer talks about string matching, and distinguishes the enum v. URL case.
In addition, in the URL case, that spec talks about how the URLs are used to fetch payment method manifest files; we are still working those:
https://w3c.github.io/webpayments/proposals/Payment-Manifest-Proposal.html

For the "match" requirements of Payment Request API, would it be possible to point to the definitions of matching in Payment Request API?

The Payment Method Manifest proposal also plays a role in matching - the user agent fetches the file (in the case of a URL PMI) and determines, for example, which origins are authorized to serve payment apps for that payment method.

I suggest that we raise as a separate issue: How does the payment method manifest file change Payment Request API?

Ian

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Mar 20, 2017 via email

@ianbjacobs
Copy link
Collaborator

@marcoscaceres,

You wrote "it needs a little overhaul tho."

That would be great. Can that be done for discussion on Thursday?
At the payment manifest discussion this week I'd like to talk about the
dependencies (PR API and payment method manifest) and whether, for example,
where the additional third party payment app matching semantics implied by the manifest
file are specified.

Ian

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Mar 20, 2017 via email

@ianbjacobs
Copy link
Collaborator

@marcoscaceres,

"We can probably have those discussions without needing any spec changes."

Agreed.

For the other discussion on "should we go to CR" it would be nice to see the changes. However, even if they are not ready yet, we will see them as of the CfC.
Ian

@domenic
Copy link
Collaborator

domenic commented Mar 20, 2017

My main concern is defining when URLs are parsed (and thus when errors are thrown for bad URLs). The actual type in the IDL is not important; DOMString is fine instead of a union, as there will be no observable difference (USVString conversion would need to happen anyway at URL parsing time).

@marcoscaceres
Copy link
Member Author

I concur.

@zkoch zkoch added this to the CR milestone Mar 23, 2017
@zkoch
Copy link
Contributor

zkoch commented Mar 23, 2017

This fees like a CR issue, but feel free to remove if not.

@marcoscaceres marcoscaceres changed the title supportedMethods (URL and enums) supportedMethods (URL and DOMString) Mar 29, 2017
@marcoscaceres
Copy link
Member Author

marcoscaceres commented Mar 29, 2017

Talking to @zkoch, we will use: if not valid payment method identifier, then try URL parser, re-throw exception. I.e., not an "enum".

@marcoscaceres
Copy link
Member Author

Depends on w3c/payment-method-id#35 (we can hook into the validate algorithm I (re)defined there).

@domenic, might be of interest to you. I have Annevk review it (from a URL-spec perspective), and he seemed ok with it and gave a bunch of feedback.

@marcoscaceres marcoscaceres changed the title supportedMethods (URL and DOMString) validate supportedMethods Jul 24, 2017
@marcoscaceres marcoscaceres marked this as a duplicate of #557 Jul 24, 2017
@marcoscaceres
Copy link
Member Author

@domenic, was working on a PR for this but got stuck. What should happen if:

const bad = [
  {
    // Contains Unicode character outside the valid ranges.
    supportedMethods: "💳-card",
  },
  {
    // Contains uppercase characters.
    supportedMethods: "Basic-Card",
  },
  {
    // Contains Unicode characters outside the valid ranges.
    supportedMethods: "¡basic-*-card!",
  },
  {
    // contains completely invalid url.
    supportedMethods: "http://user:[email protected]:12131231123321",
  },
];

const pr = new PaymentRequest(bad, details);

Should we:

  • drop invalid ones, and throw if result is ends up being an empty list?
  • .show() reject on show, because they are all bad?
  • just set computer on fire? 🔥

@zkoch
Copy link
Contributor

zkoch commented Aug 2, 2017

Chatted with @adrianba and we think: we've been moving towards explicitly throwing when encountering malformed data, so the appropriate action here is: reject on show if there is at least 1 bad identifier

a close second is setting computer on 🔥

marcoscaceres added a commit that referenced this issue Aug 10, 2017
* Validate supportedMethods (closes #464)
* Defines behavior for duplicate PMIs in contructor and updateWith() (closes #309)
marcoscaceres added a commit that referenced this issue Aug 18, 2017
* Validate supportedMethods (closes #464)
* Defines behavior for duplicate PMIs in contructor and updateWith() (closes #309)
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 a pull request may close this issue.

4 participants