836215 - [B2G RIL] Validate the numbers that are passed to nsIDOMTelephony.dial()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

This is the Gecko counter part of bug 835750

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Noming since this is part of  bug 835750 which is TEF+ already

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking status-b2g18 and status-b2g18-v1.0.0 as affected, please update the status to fixed once this is verified landed on v1-train/mozilla-b2g18 and v1.0.0/mozilla-b2g18_v_1_0_0

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1/3: add PhoneNumberUtils.isViablePhoneNumber

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2/3: validate number in nsIDOMTelephony.dial()

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Part 3 is marionette test cases. Waiting for feedback+.

Comment 6

[Andrew Overholt [:overholt]]

Comment on attachment 709006 [details] [diff] [review]
Part 1/3: add PhoneNumberUtils.isViablePhoneNumber

Gregor is probably a better reviewer here.

Comment 7

[Gregor Wagner [:gwagner]]

Comment on attachment 709006 [details] [diff] [review]
Part 1/3: add PhoneNumberUtils.isViablePhoneNumber

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

We usually patch https://github.com/andreasgal/PhoneNumber.js and merge it here.
Please also add some plain PhoneNumberJS tests for the final version.
We already have an IsViable function with the current country code? Why doesn't this work?

::: dom/phonenumberutils/PhoneNumber.jsm
@@ +18,3 @@
>    const NON_ALPHA_CHARS = /[^a-zA-Z]/g;
>    const NON_DIALABLE_CHARS = /[^,#+\*\d]/g;
> +  const PLUS_CHARS = "+\uFF0B";

You can also use PLUS_CHARS.source if you don't want to change this.
Maybe add _SOURCE instead of _PATTERN if you want to define the regular string in the reminder of the patch?

@@ +23,5 @@
>  
> +  const VALID_PUNCTUATION = "-x\u2010-\u2015\u2212\u30FC\uFF0D-\uFF0F \u00A0"
> +                          + "\u200B\u2060\u3000()\uFF08\uFF09\uFF3B\uFF3D."
> +                          + "\\[\\]/~\u2053\u223C\uFF5E";
> +  const VALID_DIGITS = "0-9\uFF10-\uFF19\u0660-\u0669\u06F0-\u06F9";

Maybe add UNICODE_DIGITS.source?

@@ +29,5 @@
> +  const VALID_PHONE_NUMBER = "[" + PLUS_CHARS + "]*(?:[" + VALID_PUNCTUATION
> +                           + "]*[" + VALID_DIGITS + "]){3,}["
> +                           + VALID_PUNCTUATION + VALID_ALPHA + VALID_DIGITS
> +                           + "]*";
> +  const RFC3966_EXTN_PREFIX = ";ext=";

That's only used once.

@@ +37,5 @@
> +                            + "(?:ext(?:ensi(?:o\u0301?|\u00F3))?n?|\uFF45\uFF58\uFF54\uFF4E?|"
> +                            + "[,x\uFF58#\uFF03~\uFF5E]|int|anexo|\uFF49\uFF4E\uFF54)"
> +                            + "[:\\.\uFF0E]?[ \u00A0\\t,-]*"
> +                            + CAPTURING_EXTN_DIGITS + "#?|[- ]+(["
> +                            + VALID_DIGITS + "]{1,5})#";

Any explanation what's going on here?

@@ +385,5 @@
>  
> +  function isViablePhoneNumber(number) {
> +    if (number == null || number.length < 3) {
> +      return false;
> +    }

What about 2 digit numbers without punctuation? I think they are also valid.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gregor Wagner [:gwagner] from comment #7)
> Please also add some plain PhoneNumberJS tests for the final
> version.

Sure, if this is a feedback+.

> We already have an IsViable function with the current country
> code? Why doesn't this work?

If you mean |IsValidNumber(number, md)|, then it takes an additional parameter md, and we have to look up meta data array by some country code.  Since we only loosely validate the number here, we don't need country code or per country possible patterns.

> ::: dom/phonenumberutils/PhoneNumber.jsm
> Any explanation what's going on here?

I'll add some comments.

> What about 2 digit numbers without punctuation? I think they
> are also valid.

"Most PBXs support variable-length dial plans that use 3 to 11 digits."[1] Do you have any known, working 2 digit numbers?

[1]: http://en.wikipedia.org/wiki/Numbering_plan

Comment 9

[Anshul]

Vicamo, why have a check in RadioInterfaceLayer.js and not in Gaia?

Comment 10

[Gregor Wagner [:gwagner]]

Comment on attachment 709006 [details] [diff] [review]
Part 1/3: add PhoneNumberUtils.isViablePhoneNumber

Yeah looks good to me.

Comment 11

[Julien Wajsberg [:julienw]]

Can someone move this forward while Taipei is in holiday ?

Comment 12

[David Scravaglieri [:scravag]]

Andrew, please, could you assign someone on this ?

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Anshul from comment #9)
> Vicamo, why have a check in RadioInterfaceLayer.js and not in
> Gaia?

Isn't this the purpose of this bug?

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gregor Wagner [:gwagner] from comment #7)
> Comment on attachment 709006 [details] [diff] [review]
> ::: dom/phonenumberutils/PhoneNumber.jsm
> @@ +18,3 @@
> >    const NON_ALPHA_CHARS = /[^a-zA-Z]/g;
> >    const NON_DIALABLE_CHARS = /[^,#+\*\d]/g;
> > +  const PLUS_CHARS = "+\uFF0B";
> 
> You can also use PLUS_CHARS.source if you don't want to change
> this. Maybe add _SOURCE instead of _PATTERN if you want to
> define the regular string in the reminder of the patch?

PLUS_CHARS.source is "^[+\uFF0B]+", but in other places I want only "+\uFF0B" or at most "[+\uFF0B]". VALID_ALPHA and VALID_DIGITS both have the same situation.

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> (In reply to Anshul from comment #9)
> > Vicamo, why have a check in RadioInterfaceLayer.js and not in
> > Gaia?
> 
> Isn't this the purpose of this bug?

Yep, it is. Even if the check is done on Gaia also this is something that belongs to the platform.

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1/2

Github pull request: https://github.com/andreasgal/PhoneNumber.js/pull/10
try https://tbpl.mozilla.org/?tree=Try&rev=ac33c14c22e8

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2/2

Comment 19

[Gregor Wagner [:gwagner]]

Comment on attachment 714315 [details] [diff] [review]
Part 2/2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1985,5 @@
> +      this.handleCallError({
> +        callIndex: -1,
> +        error: RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER]
> +      });
> +      return;

Maybe add a dump here that prints if isViablePhoneNumber returns false? We might event want to see this in a non debug build. Or do we have any gaia feedback here?

Comment 20

[Andrew Overholt [:overholt]]

Is this now ready to land?

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1/2: v2

fix marionette test failures.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2/2: v2

Add debug message for non-viable phone number passed.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd0daca35d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5afa0f17822

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cdd0daca35d1
https://hg.mozilla.org/mozilla-central/rev/d5afa0f17822

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ab6027d9159d
https://hg.mozilla.org/releases/mozilla-b2g18/rev/8e0db1c10cbf

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/35194af77dc6
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5d344067e0e7