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

Changes to address Action #40 #21

Merged
merged 5 commits into from
Feb 23, 2017
Merged

Conversation

adrianhopebailie
Copy link
Contributor

@adrianhopebailie adrianhopebailie commented Jan 26, 2017

The spec has been updated to refer to Secure Contexts concept of a potentially secure URL.
https://www.w3.org/Payments/WG/track/actions/40
and #17

The inline issue regarding locating the manifest (related to #19) has been updated to indicate that the mechanism for locating the manifest will be defined in the manifest specification.
https://www.w3.org/Payments/WG/track/actions/50

@ianbjacobs
Copy link
Contributor

Hi @adrianhopebailie,

Several comments:

OLD:
<div class="issue" title="Payment method manifest specification in progress">The Web Payments Working Group is developing a payment method manifest specification for the information a payment method owner would want to publish. Payment method identifiers that are URLs will help user agents locate these manifest files using a mechanism defined in that specification. <a href="https://github.com/zkoch/zkoch.github.io/blob/master/payment-manifest.md">Payment Manifest Proposal</a>.</div>

NEW:
<div class="issue" title="Payment method manifest specification in progress">The Web Payments Working Group is developing a Payment Method Manifest specification for machine readable information that describes how that method participates in the PaymentRequest ecosystem. That specification will define a mechanism to discover a payment method manifest file given a payment method identifier.</div>

=========

Your new text around syntax is overly constraining:

"When URLs are used for payment method identifiers they MUST be absolute URLs including a scheme, host, port and path. 

The text is overly constraining because some of those parts are optional anyway. Suggested alternative:

When URLs are used for payment method identifiers they MUST be absolute URLs that include at most the following URL components: scheme, host, port, and path.

=======

Please delete this:

The port MAY be excluded if the scheme is a special scheme.

It is no longer necessary given the previous edit, and we should also not be defining general URL rules; that's out of our scope.

==========

IMPORTANT: You implemented something other than what the WG resolved. The WG resolved that URLs that do not conform to the syntax limitations will be thrown out during matching. Instead, you implemented an algorithm that ignores the query and fragment, but does not throw out the entire URL. Please fix that.

This sentence will also need to change:

Identifier URLs SHOULD have null query and fragment values as these will be ignored when matching identifiers.

I like your idea of updating the algorithm. The algorithm should say something like "If either URL includes any components other than those permitted by this specification, the test returns false."

If you want to keep a sentence about the impact of the syntax restriction on matching, you could
say this (taking all the edits into account):

When URLs are used for payment method identifiers they MUST be absolute URLs that include at most the following URL components: scheme, host, port, and path. URLs that include any other components MUST be ignored during matching. The URL MUST be a potentially trustworthy URL as defined in the [[SECURE-CONTEXTS]] specification.

@adrianhopebailie
Copy link
Contributor Author

The WG resolved that URLs that do not conform to the syntax limitations will be thrown out during matching. Instead, you implemented an algorithm that ignores the query and fragment, but does not throw out the entire URL.

That was not my understanding of the WG resolution so perhaps we need to raise this on the next call?

@ianbjacobs
Copy link
Contributor

Hi Adrian,

Here's the suggestion from Rouslan that I thought captured the requirement [1]:

"I suggest we preserve the original intent of requiring no fragment identifiers or query string. I suggest that the user agent ignore any URL that has a fragment / query string rather than ignoring the offending parts... that reduces the cost of parsing... browser can do a match on the URL and reject it if not well-formed"

Thus, the UA would reject the entire URL (and not match with it).

+1 to double checking at the next call.

Ian

[1] https://www.w3.org/2017/01/26-wpwg-minutes#item01

@adrianhopebailie
Copy link
Contributor Author

You're right, but even though I was on the call I didn't pick up that subtle difference. Let's take to the group and get a final resolution.

@adrianhopebailie
Copy link
Contributor Author

Updated per WG resolution to discard URL's that contain a query string or fragment.

@ianbjacobs can you re-review please

@adrianhopebailie adrianhopebailie merged commit 14c0254 into gh-pages Feb 23, 2017
@adrianhopebailie adrianhopebailie deleted the adrianhopebailie-patch-1 branch February 23, 2017 15:13
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.

None yet

2 participants