Pass payment-request-ctor-pmi-handling web-platform-tests

RESOLVED FIXED in Firefox 58

Status

()

Core
DOM: Web Payments
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: edenchuang, Assigned: edenchuang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

9 months ago
This bug is used for tracking the w3c spec https://w3c.github.io/payment-method-id implementation.
(Assignee)

Updated

9 months ago
Assignee: nobody → echuang
Blocks: 1380546
Status: NEW → ASSIGNED
(Assignee)

Updated

8 months ago
Summary: [Payment Request API] Payment method identifier validation implementation → Pass payment-request-ctor-pmi-handling web-platform-tests
(Assignee)

Comment 3

8 months ago
Created attachment 8906490 [details] [diff] [review]
Bug 1389418 - Support payment method identifier validation in PaymentRequest API. r?baku

This patch implements the payment method identifier validation algorithm in PaymentRequest API according to https://w3c.github.io/payment-method-id.

    The steps to validate a payment method identifier with a string PMI are given by the following algorithm. It returns true if the PMI is valid.                                                                                       
    
    1. Let URL be the result of running the basic URL parser with PMI.           
    2. If URL parser returns fail, validate a standardized payment method identifier with pmi and return the result.                                                
    3. Otherwise, validate a URL-based payment method identifier passing URL and return the result.
Attachment #8906490 - Flags: review?(amarchesini)
(Assignee)

Comment 4

8 months ago
Created attachment 8906491 [details] [diff] [review]
Bug 1389418 - Mochitest for payment method identifer validation in PaymentRequest API. r?baku

The mochitest for payment method identifier validation algorithm.

    1. Test PaymentRequest construction with valid PMIs
    2. Test PaymentRequest construction with invalid PMIs
    3. Test PaymentRequestUpdateEvent::updateWith() with a valid PMI.
    4. Test PaymentRequestUpdateEvent::updateWith() with an invalid PMI.
Attachment #8906491 - Flags: review?(amarchesini)
Comment on attachment 8906491 [details] [diff] [review]
Bug 1389418 - Mochitest for payment method identifer validation in PaymentRequest API. r?baku

Review of attachment 8906491 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/test/test_pmi_validation.html
@@ +240,5 @@
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +
> +</div>
> +<pre id="test">

Remove these <p>, <div> and <pre>
Attachment #8906491 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 6

7 months ago
Created attachment 8908490 [details] [diff] [review]
Bug 1389418 - Mochitest for payment method identifer validation in PaymentRequest API. r=baku

Update the patch according to comment 5. Remove unnecessary tags in html files.
Attachment #8906491 - Attachment is obsolete: true
Attachment #8908490 - Flags: review+
Comment on attachment 8906490 [details] [diff] [review]
Bug 1389418 - Support payment method identifier validation in PaymentRequest API. r?baku

Review of attachment 8906490 [details] [diff] [review]:
-----------------------------------------------------------------

I would like to see this patch again.

::: dom/payments/PaymentRequest.cpp
@@ +10,5 @@
>  #include "nsContentUtils.h"
>  #include "BasicCardPayment.h"
>  #include "PaymentRequestManager.h"
> +#include "nsIURLParser.h"
> +#include "nsNetCID.h"

alphabetic order.

@@ +65,5 @@
> +   *       stdpmi = part *( "-" part )
> +   *       part = 1loweralpha *( DIGIT / loweralpha )
> +   *       loweralpha =  %x61-7A
> +   */
> +  for (uint32_t idx = 0; idx < aIdentifier.Length(); ++idx) {

I don't like this approach. What about if you do:

nsCString::const_iterator start, end;
aIdentifier.BeginReading(start);
aIdentifier.EndReading(end);

while (start != end) {
  // First char must be in the range %x61-7A
  if (start < 'a' || start > 'z') {
    // error ... return NS_ERROR_RANGE_ERR;
  }

  ++start;

  // The rest can be in the range %x61-7A + DIGITs
  while (start != end && start != '-' &&
         (start >= 'a' && start <= 'z') ||
         (start >= '0' && start <= '9')) {
    ++start;
  }

  if (start != end && start != '-') {
    // error...
    return NS_ERROR_RANGE_ERR; 
  }

  if (start == '-') {
    ++start;
    if (start == end) {
      // error
      return NS_ERROR_RANGE_ERR;
    }
  }
}

something like that... to test, of course :)

@@ +129,5 @@
> +  uint32_t schemePos;
> +  int32_t schemeLen;
> +  uint32_t authorityPos;
> +  int32_t authorityLen;
> +  nsresult rv = urlParser->ParseURL(NS_ConvertUTF16toUTF8(aIdentifier).get(),

Here you create 2 strings. Just do:

NS_ConvertUTF16toUTF8 url(aIdentifier);
nsresult rv = urlParser->ParseURL(url.get(), url.Length(), ...

@@ +139,5 @@
> +  if (schemeLen == -1) {
> +    // The PMI is not a URL-based PMI, check if it is a standardized PMI
> +    return IsValidStandardizedPMI(aIdentifier, aErrorMsg);
> +  }
> +  nsString identifier(aIdentifier);

nsAutoString ?

@@ +142,5 @@
> +  }
> +  nsString identifier(aIdentifier);
> +  nsAutoString scheme;
> +  identifier.Mid(scheme, schemePos, schemeLen);
> +  if (!scheme.EqualsASCII("https")) {

if (!Substring(aIdentifier, schemePos, schemeLen).EqualsASCII("https")) { ...

@@ +150,5 @@
> +    return NS_ERROR_RANGE_ERR;
> +  }
> +  nsAutoString authority;
> +  identifier.Mid(authority, authorityPos, authorityLen);
> +  if (authority.IsEmpty()) {

same here.

@@ +163,5 @@
> +  int32_t passwordLen;
> +  uint32_t hostnamePos;
> +  int32_t hostnameLen;
> +  int32_t port;
> +  rv = urlParser->ParseAuthority(NS_ConvertUTF16toUTF8(authority).get(),

same here.

@@ +175,5 @@
> +    // are used in web-platform-test
> +    // For exmaple:
> +    //     https://:@example.com             // should be considered as valid
> +    //     https://:password@example.com.    // should be considered as invalid
> +    int32_t authorityLen = NS_ConvertUTF16toUTF8(authority).Length();

don't create this again.

@@ +177,5 @@
> +    //     https://:@example.com             // should be considered as valid
> +    //     https://:password@example.com.    // should be considered as invalid
> +    int32_t authorityLen = NS_ConvertUTF16toUTF8(authority).Length();
> +    int32_t atIdx = authorityLen;
> +    for (;atIdx >= 0 && authority.CharAt(atIdx) != '@'; --atIdx) {

int32_t atPos = authority.FindChar('@')
if (atPos ...

@@ +186,5 @@
> +      // only accept the case https://:@xxx
> +      if (atIdx == 1 && authority.CharAt(0) == ':') {
> +        usernamePos = 0;
> +        usernameLen = 0;
> +        passwordPos = 1;

why 1 ? Here we are because of the NS_FAILED(rv). Set usernamePos, usernameLen and so on to 0 when initialized. At line 160.
Attachment #8906490 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 8

7 months ago
Created attachment 8910097 [details] [diff] [review]
Bug 1389418 - Support payment method identifier validation in PaymentRequest API. r?baku

Update the patch according to the comment 7.

    1. using iterator to replace index for string traversing.
    2. reduce string creation.
Attachment #8906490 - Attachment is obsolete: true
Attachment #8910097 - Flags: review?(amarchesini)
Comment on attachment 8910097 [details] [diff] [review]
Bug 1389418 - Support payment method identifier validation in PaymentRequest API. r?baku

Review of attachment 8910097 [details] [diff] [review]:
-----------------------------------------------------------------

tell me more about port and serverInfo. For the rest is r+.

::: dom/payments/PaymentRequest.cpp
@@ +126,5 @@
> +   *  3. Otherwise, return true.
> +   */
> +  nsCOMPtr<nsIURLParser> urlParser = do_GetService(NS_STDURLPARSER_CONTRACTID);
> +  MOZ_ASSERT(urlParser);
> +  uint32_t schemePos;

set a default value. = 0 ? all of them.

@@ +141,5 @@
> +  if (schemeLen == -1) {
> +    // The PMI is not a URL-based PMI, check if it is a standardized PMI
> +    return IsValidStandardizedPMI(aIdentifier, aErrorMsg);
> +  }
> +  nsAutoString identifier(aIdentifier);

why do you need this extra string? I guess you can directly use identifier.

@@ +153,5 @@
> +    aErrorMsg.AssignLiteral("'");
> +    aErrorMsg.Append(aIdentifier);
> +    aErrorMsg.AppendLiteral("' is not valid. hostname can not be empty.");
> +    return NS_ERROR_RANGE_ERR;
> +  }

new line after }

@@ +154,5 @@
> +    aErrorMsg.Append(aIdentifier);
> +    aErrorMsg.AppendLiteral("' is not valid. hostname can not be empty.");
> +    return NS_ERROR_RANGE_ERR;
> +  }
> +  uint32_t usernamePos;

= 0 all

@@ +186,5 @@
> +        passwordLen = 0;
> +      } else {
> +        // for the fail cases, don't care about what the actual length is.
> +        usernamePos = 0;
> +        usernameLen = 1;

set it to INT32_MAX

@@ +188,5 @@
> +        // for the fail cases, don't care about what the actual length is.
> +        usernamePos = 0;
> +        usernameLen = 1;
> +        passwordPos = 0;
> +        passwordLen = 1;

here as well.

@@ +201,5 @@
> +        (passwordLen == 0 || passwordLen == -1)) {
> +      nsAutoCString serverInfo(Substring(authority,
> +                                         atPos + 1,
> +                                         authority.Length() - atPos - 1));
> +      if (serverInfo.IsEmpty()) {

you don't need to do Substring to know that this is empty. Just do:

if (authority.Length() - atPos - 1 == 0) { ...

move nsAutoCString serverInfo = after this block.

@@ +211,5 @@
> +      rv = urlParser->ParseServerInfo(serverInfo.get(),
> +                                      serverInfo.Length(),
> +                                      &hostnamePos, &hostnameLen, &port);
> +      if (NS_FAILED(rv)) {
> +        if ((port != -1) && (port < 0 || port > 65535)) {

what's the relationship between serverInfo and port?

@@ +220,5 @@
> +        return NS_ERROR_RANGE_ERR;
> +      }
> +    }
> +  }
> +  if (((usernameLen != -1) && (usernameLen != 0)) ||

>= 0 ?

@@ +221,5 @@
> +      }
> +    }
> +  }
> +  if (((usernameLen != -1) && (usernameLen != 0)) ||
> +      ((passwordLen != -1) && (passwordLen != 0))) {

>= 0 ?

@@ +227,5 @@
> +    aErrorMsg.Append(aIdentifier);
> +    aErrorMsg.AssignLiteral("' is not valid. Username and password must be empty.");
> +    return NS_ERROR_RANGE_ERR;
> +  }
> +  if ((hostnameLen == -1) || (hostnameLen == 0)) {

>= 0 ?
Attachment #8910097 - Flags: review?(amarchesini)
(Assignee)

Comment 10

7 months ago
(In reply to Andrea Marchesini [:baku] from comment #9)
> Comment on attachment 8910097 [details] [diff] [review]
> Bug 1389418 - Support payment method identifier validation in PaymentRequest
> API. r?baku
> 
> Review of attachment 8910097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> tell me more about port and serverInfo. For the rest is r+.
> 

> @@ +211,5 @@
> > +      rv = urlParser->ParseServerInfo(serverInfo.get(),
> > +                                      serverInfo.Length(),
> > +                                      &hostnamePos, &hostnameLen, &port);
> > +      if (NS_FAILED(rv)) {
> > +        if ((port != -1) && (port < 0 || port > 65535)) {
> 
> what's the relationship between serverInfo and port?

Here we reuse the nsIURLParser::ParseServerInfo to extract the hostname and port information.

Let me use an example to explain.

https:username:password@example.com:12345/abcdef.html
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                          This part is serverInfo.

nsIURLParser::ParseServerInfo extracts example.com as the hostname and 12345 as the port number.

Unfortunately nsIURLParser::ParseServerInfo returns NS_ERROR_MALFORMED_URI in all fail situations, 

https://searchfox.org/mozilla-central/source/netwerk/base/nsURLParsers.cpp#555

To give more debugging information to the merchant, there is one situation that we can tell, the port number is invalid.

https://searchfox.org/mozilla-central/source/netwerk/base/nsURLParsers.cpp#609

According to the code, we can realize that if port != -1 and it returns NS_ERROR_MALFORMED_URI, it must because the port value is not in the range 0 ~ 65535 (UINT16_MAX), and the port value must be saved in the argument we passed in, such that nsIURLParser::ParseServerInfo can check if it is valid. So I write the code 

> if ((port != -1) && (port < 0 || port > 65535)) {

to check the situation. Once it is true, return NS_ERROR_RANGE_ERR with the error message.

> 
> @@ +220,5 @@
> > +        return NS_ERROR_RANGE_ERR;
> > +      }
> > +    }
> > +  }
> > +  if (((usernameLen != -1) && (usernameLen != 0)) ||
> 
> >= 0 ?
> 
> @@ +221,5 @@
> > +      }
> > +    }
> > +  }
> > +  if (((usernameLen != -1) && (usernameLen != 0)) ||
> > +      ((passwordLen != -1) && (passwordLen != 0))) {
> 
> >= 0 ?

  != 0 is correct, -1 means username/password does not exists and 0 means empty username/passward.
  so, if length not equal to -1 or 0, they are invalid.
Flags: needinfo?(amarchesini)
> > tell me more about port and serverInfo. For the rest is r+.

I don't like to check port value when the method returns error. I prefer, instead, adding an extra method or check the port differently. Can you fix this in a follow up and remove this block for now?


> > > +  if (((usernameLen != -1) && (usernameLen != 0)) ||
> > > +      ((passwordLen != -1) && (passwordLen != 0))) {
> > 
> > >= 0 ?
> 
>   != 0 is correct, -1 means username/password does not exists and 0 means
> empty username/passward.
>   so, if length not equal to -1 or 0, they are invalid.

Right. so the check can be: if (usernameLen > 0 || passwordLen > 0) ... + a comment.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 12

7 months ago
Created attachment 8910398 [details] [diff] [review]
Bug 1389418 - Support payment method identifier validation in PaymentRequest API. r?baku

Update the patch according to comment 11.

1. Remove the port value checking block.
2. Simplify the if judgment.
Attachment #8910097 - Attachment is obsolete: true
Attachment #8910398 - Flags: review?(amarchesini)
Attachment #8910398 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 13

7 months ago
Created attachment 8911642 [details] [diff] [review]
Bug 1389418 - Support payment method identifier validation in PaymentRequest API. r=baku
Attachment #8910398 - Attachment is obsolete: true
Attachment #8911642 - Flags: review+
(Assignee)

Comment 14

7 months ago
Created attachment 8911649 [details] [diff] [review]
Bug 1389418 - Mochitest for payment method identifer validation in PaymentRequest API. r=baku

Resolve conflicts.
Attachment #8908490 - Attachment is obsolete: true
Attachment #8911649 - Flags: review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 15

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c4317700a2
Support payment method identifier validation in PaymentRequest API. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff2d7b9381fa
Mochitest for payment method identifier validation support in PaymentRequest API. r=baku
Keywords: checkin-needed

Comment 16

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2c4317700a2
https://hg.mozilla.org/mozilla-central/rev/ff2d7b9381fa
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.