feat: rewrite all the things #35

marcoscaceres · 2017-04-27T06:10:56Z

Removed wording about overloading identifiers (closes Reference manifest spec #33)
Defined "standardized payment method identifier" (closes "W3C String" is overly generic #32)
Cited URL standard (closes citation to dated version of since-abandoned url spec fork is unusual #30)
Dropped dependency on [SECURE-CONTEXTS] (closes [SECURE-CONTEXTS] should be a normative rather than informative reference #29)
defined "Standardized Payment Method Identifiers" (closes Payment Method Identifier needs dfn #23)
Removed circular dependency (closes Circular dependency between PMI and PR specs #22)
Added proper URL parsing and specific validation with error (closes Identifier equivalence does not handle failure #8 Check what happens if URL parsing throws.... #27 use of 'must' in section on Syntax of URLs as Payment Method Identifiers is suspicious #28)
Added Security considerations sections (closes Add Security Considerations and Privacy Considerations sections #7)
Added Marcos as editor + cleans up respecConfig
Removes "Conditional Matching Beyond Payment Methods" (closes Remove "Conditional Matching Beyond Payment Method Identifier Matching" #34)

marcoscaceres · 2017-04-27T06:16:15Z

index.html

+        Security consideration
+      </h2>
+      <p>
+        As this specification is reliant on <a>URLs</a>, implementers need to


@annevk, just a 👍 if this is ok with you.

Depends, are these URLs exposed to users? Which parts of the URL are exposed to users?

They are never exposed to the user, presumedly (but I couldn't be 100% sure it would never happen - but people do crazy things). I was going to make a note of that.

If they're not exposed than most of the security considerations don't really apply I think. What did you think would apply?

Like you said, if they were ever displayed then I was concerned the considerations would apply. I'll just say that.

ianbjacobs · 2017-04-27T06:29:52Z

index.html

+        payments, or credit transfers). The <a href="https://w3.org">W3C</a>
+        expects these payment methods will be implemented in a wide variety of
+        third party payment handlers. The W3C identifies these payment methods
+        with (short) strings (individual referred to as a <dfn>standardized


s/individual/individually

annevk · 2017-04-27T06:56:35Z

index.html

+        <ol class="algorithm">
+          <li>Let <var>url</var> be the result of trying to <a data-lt=
+          "URL Parser">parse</a> <var>potential URL</var>. If failure,
+          <a>throw</a> a <code>TypeError</code> exception.


Just make the If statement a new step. url would be failure in that case.

"If url is failure, then throw a TypeError."

annevk · 2017-04-27T06:57:12Z

index.html

+          "URL Parser">parse</a> <var>potential URL</var>. If failure,
+          <a>throw</a> a <code>TypeError</code> exception.
+          </li>
+          <li>If <a>scheme</a> is not "https", <a>throw</a> a


"If url's scheme ..."

annevk · 2017-04-27T06:57:28Z

index.html

+          <li>If <a>scheme</a> is not "https", <a>throw</a> a
+          <code>TypeError</code> exception.
+          </li>
+          <li>If <a>query</a> is not null or <a>fragment</a> is not null,


Again, need to specify where the query and fragment come from.

annevk · 2017-04-27T06:58:01Z

index.html

-          <li>Let <em>urlA</em> be the result from the <a>URL parser</a> when
-          parsing <em>A</em> with no <a>base URL</a>.
+        <ol class="algorithm">
+          <li>Try to <a>parse</a> <var>A</var> and let <var>urlA</var> be the


Shouldn't this reference the URL parser again?

<a>parse</a> links to URL Parser. Is that what you mean? I can be more clear.

Okay. You used "URL parser" as well earlier. Seems weird to use different referencing terms within the same document.

Yeah, I see what you mean. I'll mirror more what you have in the URL spec.

annevk · 2017-04-27T06:58:57Z

index.html

-          <em>null</em> terminate the algorithm with a result of <em>no
-          match</em> and discard <em>B</em> from further matching.
+          <li>Try to <a>validate</a> <var>urlA</var> and <var>urlB</var>. If
+          either throws, return false.


This seems wrong since validate takes strings, not URLs.

Ah, good point.

annevk · 2017-04-27T07:40:30Z

index.html

+          <li>Let <var>url</var> be the result of trying to <a data-lt=
+          "URL Parser">parse</a> <var>potential URL</var>.
+          </li>
+          <li>If failure, <a>throw</a> a <code>TypeError</code> exception.


"If url is failure"

annevk · 2017-04-27T07:41:36Z

index.html

+          </li>
+          <li>If running <a>potentially trustworthy URL</a> with <var>url</var>
+          returns <code>"Not Trustworthy"</code>, <a>throw</a> a
+          <code>SecurityError</code> exception.


Different exception really necessary? You'll have to write some tests to make sure it's thrown before a non-null query...

Ok, will just use TypeError here too.

(also noted about the test order)

annevk · 2017-04-27T07:42:21Z

index.html

-          match</em> and discard <em>B</em> from further matching.
+          <li>
+            <a>Serialize</a> <var>urlA</var> and <var>urlB</var> and attempt to
+            <a>validate</a> each. If either throws, return false.


I was hoping you'd abstract validate a bit somehow instead. Parse -> serialize -> parse seems really hacky.

annevk · 2017-04-27T07:42:49Z

index.html

        </h2>
        <p>
-          For W3C strings, user agents MUST use exact string matching.
+          For <a>standardized payment method identifier</a>, user agents MUST


identifiers*

annevk · 2017-04-27T07:43:43Z

index.html

+        rendered or passed around by end-users. However, in the off-chance that
+        they are, implementers need to be aware of the <a data-cite=
+        "!URL#security-considerations">security considerations</a> from the
+        [[!URL]] specification.


Surely you should know whether they ever are? This seems a little weird.

You are right. Can definitely write this "as the world is today" - where there are no known instances of these URLs being rendered. Will fix.

annevk · 2017-04-27T09:05:54Z

Note that "I was hoping you'd abstract validate a bit somehow instead. Parse -> serialize -> parse seems really hacky." got lost due to a commit. One of the downsides of not addressing all comments in a single commit.

domenic · 2017-04-27T20:08:16Z

index.html

+          </li>
+          <li>If failure, <a>throw</a> a <code>TypeError</code> exception.
+          </li>
+          <li>If running <a>potentially trustworthy URL</a> with <var>url</var>


Do you really want to allow about:blank, about:srcdoc, wss: URLs, file: URLs, etc.? (All are potentially trustworthy.)

Do you want to allow usernames or passwords in the URL?

What is wrong with queries?

We probably need to synchronize https://w3c.github.io/payment-method-manifest/#fetch-payment-method-manifests step 2 with this.

I'm still of the opinion that we should:

restrict this to "https" (because Payment Manifest is the only serious consumer of the end points)

Recommend "best practice" to developers to use paths and avoid query, fragments, username, and password.

That would mean that any URL with a https:// scheme is a valid PMI-URL - and would make everything super simple.

Path and query seem equivalent to me: if you allow one you should allow the other.

I don't think we want to add another resource to the platform hidden behind HTTP auth, so I'd suggest disallowing username and password.

Ok, let's ban username and password. @zkoch has opinions about query/path - but hopefully the above can convince him to allow query.

I'll make preemptive changes to match.

Query strings just feel more complicated to me and much easier for developers to get wrong. The whole goal was to give us the flexibility of URLs (to enable things like method manifests) but also make it as tightly scoped as possible to prevent developer error.

There's a lot of discussion on this at #9

Thanks @zkoch for the pointer.

As Anne states at the end of #9, the overloading of this URL as an identifier AND a resource could become a real issue.

@domenic, crazy thought... but what if, to enable payment manifests, a developer instead had to serve the link relationship (instead of bobpay):

So, https://merchant.com/checkout:

Link: <https://bobpay.com/manifest>; rel="payment-method-manifest"; pmi="https://bobpay.com" Link: <https://alicepay.com/pay-manifest>; rel="payment-method-manifest"; pmi="https://alicepay.com"

Then, so long as Link and referenced PMI are same origin, then "https://bobpay.com/manifest" can be fetched.

This also has the benefit of allowing prefetching of the manifests.

(Added additional Link: above, to show how you could have multiple payment methods.)

How does the developer know where the manifest file is?

This puts the burden on 1 million developers instead of 1 time on the origin. Or am I missing something?

Ian

I don't really understand the Link proposal. @zkoch, I think your feeling about queries is off-base. They are equivalent to paths (and indeed, some server software uses them instead of paths). If we want to disallow paths and queries, that's fine, but we should not disallow just queries.

domenic · 2017-04-27T20:09:43Z

index.html

-          let <em>B</em> be the second payment method identifier string.
+        <ol class="algorithm">
+          <li>Let <var>urlA</var> be the result of running the <a>URL
+          parser</a> on <var>inputA</var>.


Generally since you don't want to get access to the blob URL store you probably want to use the basic URL parser

I actually think we should drop this section entirely. To compare two PMI URLs, you need to have first gone through the validation steps to get urlA and urlB. If we make that an invariant, then URL spec's "equals" algorithm can just be used without needing any of this.

That sounds great to me; let's get rid of it.

The larger point about using the basic URL parser throughout this spec stands though.

domenic · 2017-04-27T20:12:54Z

index.html

        </p>
+        <ol class="algorithm">


This algorithm would be better operating on parsed URLs, not on strings. Most places that call it seem to serialize the parsed URL before calling it, which is pretty pointless since it's just going to parse it again.

This is currently not the case in the Payment Request spec. In the "Process payment methods:", I was going to call "validate" just before step 4.2.2:

Let serializedData be the result of JSON-serializing paymentMethod.data into a string, if the data member of paymentMethod is present, or null if it is not. Rethrow any exceptions.

I can call the URL Parser in the Payment Request spec (re-throwing any errors), and then pass the URL into validate.

I can try to take another look at the callers tomorrow. What I remember is that all the callers in this document had to go through parse+serialize+parse. But maybe taking URLs as input is better anyway, not sure yet.

OK, after removing the matching algorithm, I see your perspective. But, I think that it's still a better algorithm if it's more "strongly typed", i.e. operates on URLs.

I have a separate issue which is that I think it should return true or false, instead of returning true or throwing a TypeError. E.g. in the payment-method-manifest spec we aren't in a context where throwing makes sense, so we'd have to "catch" the exception, even though we're not even in a JavaScript context where exceptions are sensible to talk about.

Agree. I'll make the change to only return true/false and move the exception handling to the PR spec.

domenic · 2017-04-27T20:13:17Z

index.html

+        The <a href="https://w3.org">W3C</a> expects to publish a small number
+        of payment method specifications that define abstractions across
+        similar payment methods (such as credit card payments, tokenized
+        payments, or credit transfers). The <a href="https://w3.org">W3C</a>


Probably only the first "W3C" should be linked

domenic · 2017-04-27T20:13:48Z

index.html

        </h2>
        <p>
-          For W3C strings, user agents MUST use exact string matching.
+          For a<a>standardized payment method identifiers</a>, user agents MUST


missing space; mismatched pluralness

domenic · 2017-04-27T20:14:18Z

index.html

-          For W3C strings, user agents MUST use exact string matching.
+          For a<a>standardized payment method identifiers</a>, user agents MUST
+          compare strings in a <a data-cite=
+          "HTML#case-sensitive">case-sensitive</a> manner (code point for code


/cc @annevk we should probably move this to Infra (no action item for now on this document though)

domenic · 2017-04-27T20:15:01Z

index.html

+        The URLs relied upon by this specification are only expected to be used
+        by APIs, or internally by the user agent. As such, there is no
+        expectation that URLs defined in this specification would ever be
+        rendered or passed around by end-users.


I'm not sure this is quite accurate, given the existence of https://w3c.github.io/payment-method-manifest/... It's not very clear in general, and it's not clear what it has to do with security considerations.

Does payment manifest somehow expose these URLs to end users?

FWIW, I don't mind removing this section in its entirety, or just stating: "There are no known privacy or security issues to be considered at this time. This is subject to change as this specification evolves." or something like that.

Won't end users end up seeing something like "Pay with https://android.google.com/pay"?

I'd be OK with removing it too.

domenic · 2017-04-27T20:17:34Z

index.html

+        method specific data for those methods they claim to accept. This
+        allows the API to abstract away the details of specific payment methods
+        and to add new ones over time without changing the <abbr title=
+        "application programming interface">API</abbr>.


Probably the first use of API in the document should get this treatment, not one middle-way through the document. Although IMO it's not necessary at all.

ianbjacobs · 2017-04-27T20:38:26Z

@domenic wrote:

Do you really want to allow about:blank, about:srcdoc, wss: URLs, file: URLs, etc.? (All are potentially trustworthy.)

I think the answer is "no." However, I wonder if there are some use cases for them (e.g., local testing of a payment method?) We've not really discussed those, and I think the intended use cases are for published payment method manifest files.

Do you want to allow usernames or passwords in the URL?

Again, I think the answer is "no" but there might be use cases we've not considered.

What is wrong with queries?

I don't recall the rationale exactly, but I think it had to do with simplicity.

These are good questions.

Ian

marcoscaceres · 2017-06-20T05:57:55Z

This is ready for final review.

zkoch · 2017-06-21T22:12:44Z

index.html

        <p>
-          The syntax of W3C strings used as payment method identifiers is
-          constrained by this regular expression:
+          User agents MUST NOT enforce validity or well-formedness of


This seems a bit confusing to me at first read. All we're trying to say is "methods either match (great) or they don't (ignored)." It took me a bit to understand what this meant. Example might help?

Adding as a separate bug: #37

zkoch

Minor nit I commented on, otherwise LGTM

marcoscaceres mentioned this pull request Apr 27, 2017

If URL parsing fails then always fail to match URLs. #31

Closed

marcoscaceres commented Apr 27, 2017

View reviewed changes

marcoscaceres requested a review from adrianba April 27, 2017 06:17

ianbjacobs reviewed Apr 27, 2017

View reviewed changes

marcoscaceres requested a review from mollyd16 April 27, 2017 06:31

marcoscaceres mentioned this pull request Apr 27, 2017

Payment Method Identifier needs dfn #23

Closed

annevk reviewed Apr 27, 2017

View reviewed changes

marcoscaceres mentioned this pull request Apr 27, 2017

validate supportedMethods w3c/payment-request#464

Closed

domenic reviewed Apr 27, 2017

View reviewed changes

marcoscaceres added 14 commits May 5, 2017 13:37

feat: rewrite spec and algos

07bc246

feat(Security): add security section (closes #7)

ba781ff

feat: formally define 'Payment method identifiers' (closes #23)

35cecc1

editorial: typo

cceab9f

fix: bring back use of potentially trustworthy URL

d092148

Clear up URL Security + serializer URLs for validation

0eafd25

fix: add missing word

d6c3550

fix: typo

356d549

Use URL spec's formalisms

2ff6413

cleanup pre markup for

6e65ca0

Throw TypeError instead of SecurityError

bbc1045

fix typo, clarify security section

acbae01

remove API abbr

8755c62

Fixed nits found by domenic

060b0d8

remove matching algorihtm

41f06e3

marcoscaceres force-pushed the validation branch from 93bcec6 to 41f06e3 Compare May 5, 2017 03:37

marcoscaceres added 2 commits June 20, 2017 15:27

further cleanup

00f45a0

cleanup registry

150eddc

marcoscaceres mentioned this pull request Jun 20, 2017

editorial: rm 'Conditional Matching Beyond Payment Methods' section #36

Closed

marcoscaceres removed request for adrianba and mollyd16 June 20, 2017 06:00

marcoscaceres added 2 commits June 20, 2017 16:03

use 'github' option for ReSpec config

9a0942f

small nits

6c87b4e

zkoch reviewed Jun 21, 2017

View reviewed changes

zkoch approved these changes Jun 21, 2017

View reviewed changes

marcoscaceres merged commit 816309b into gh-pages Jun 23, 2017

marcoscaceres deleted the validation branch June 23, 2017 09:00

ianbjacobs mentioned this pull request Jun 23, 2017

Reference manifest spec #33

Closed

feat: rewrite all the things #35

feat: rewrite all the things #35

Conversation

marcoscaceres commented Apr 27, 2017 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianbjacobs commented Apr 27, 2017

marcoscaceres commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkoch left a comment

Choose a reason for hiding this comment

marcoscaceres commented Apr 27, 2017 •

edited by pr-preview bot

Loading

marcoscaceres Apr 28, 2017 •

edited

Loading