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

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Eden Chuang[:edenchuang]]

This bug is used for tracking the w3c spec https://w3c.github.io/payment-method-id implementation.

Comment 1

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e6561b65b0bbb2487071ff6b69e40b91cdcb405

Comment 2

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dae3da09b072f7e341c934e7e69bc35a2757506

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: 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.

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: 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.

Comment 5

[Andrea Marchesini [:baku]]

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>

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: 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.

Comment 7

[Andrea Marchesini [:baku]]

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://:[e-mail].    // 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://:[e-mail].    // 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.

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: 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.

Comment 9

[Andrea Marchesini [:baku]]

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 ?

Comment 10

[Eden Chuang[:edenchuang]]

(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:[e-mail]: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.

Comment 11

[Andrea Marchesini [:baku]]

> > 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.

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: 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.

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Bug 1389418 - Support payment method identifier validation in PaymentRequest API. r=baku

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Bug 1389418 - Mochitest for payment method identifer validation in PaymentRequest API. r=baku

Resolve conflicts.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a2c4317700a2
https://hg.mozilla.org/mozilla-central/rev/ff2d7b9381fa