846499 - [B2G RIL] don't reject so many numbers in nsIDOMTelephony.dial()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[David Flanagan [:djf]]

Bug 836215 added an isViable() function to dom/phonenumberutils/PhoneNumber.jsm and added calls to that function in the gonk telephony code so that it would reject strings that do not appear to be possible phone numbers.

This lead to the regression in bug 845539 where we were no longer allowing users to dial *2 or *8, which are valid numbers in Venezuela. The patch for that bug fixes the regression in a very narrow way by expanding the definition of a viable number to include * followed by a single digit.

I believe that our filter is still much too strict and that we're asking for trouble here with similar regressions in the future. It turns out that *8 is a valid number in the US on AT&T, for example, but no one seemed to know that. I just don't trust that we can know what numbers are supported currently and we can't predict what number patterns will be added by carriers in the future.

I believe that the original intent of Bug 836215 was to add a layer of security and prevent injection attacks through tel:// phone numbers and numbers dialed through the contacts app.  So we need to reject some set of strings that are malformed or malicious.

For future compatibility with the world's phone networks, I think we should allow any string of numbers plus * and # (subject to some maximum length limit) that the user could dial from a standard telephone keypad. We shouldn't be trying to decide whether '***' is a valid number or not. Let the carrier do that on their network.  Maybe some day, some carrier will want to make that string do something. It would suck if we wouldn't even allow the phone to dial that number.

Comment 1

[David Flanagan [:djf]]

cc'ing some people who were involved with bug 836215

Comment 2

[David Flanagan [:djf]]

Nominating this as tracking-b2g18 because I believe that shipping with the current strict filtering is not future proof and is going to come back and bite us hard.

As a point of comparison, my android phone allows me to dial numbers like '1', '***', and '#*#'.  They don't do anything, of course, but if Verizon decided to make them do something, my Android phone wouldn't prevent me from calling them the way my FirefoxOS phone does.

Comment 3

[David Flanagan [:djf]]

Actually, I'm going to go ahead and nominate this for tef because it is a spinoff of bug 845539 which is a tef+ bug.

Comment 4

[Andrew Overholt [:overholt]]

Vicamo, should we give this to someone else?

Comment 5

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

(In reply to Andrew Overholt [:overholt] from comment #4)
> Vicamo, should we give this to someone else?

I don't know why we have to fix this bug.  Normal users can only dial numbers with only characters shown on Dialer keypad, so actually this bug means reverting works done in bug 845539 and bug 836215.

So, I guess this will be easier.  Please re-open this bug if you want revert both 845539 and 836215.

Comment 6

[David Flanagan [:djf]]

Vicamo and I have already disagreed about this in bug 845539, so I'd prefer to have someone other than he and I weigh in on the technical merits of this bug before we close this as WONTFIX.

As it stands now, a FirefoxOS phone will never dial a 1 digit number or a weird number like *8*.  I don't know why an operator would want to make numbers like that do anything on their cell network. But if they did, our phones would refuse to dial those numbers: we are filtering them out. I don't think there is any need to filter them, and I worry that sometime in the future there will be a cell network with a number that we will not dial.

The last sentence of comment 5 is quite misleading. We are not talking about reverting those two bugs. Bug 836215 instituted this filtering. IIUC, this was done to prevent injection attacks in tel:// URLs. I believe that the filter that was put in place was too aggressive and filtered out too many numbers, including those that clearly could not be security issues.  Bug 845539 was itself a regression caused by the overly strict filter. In that bug I proposed a patch made the filtering much more liberal, but Vicamo instead landed a patch that very narrowly fixed the regression.  I opened this bug to continue the discussion because I continue to believe that we're asking for trouble by not actually dialing anything the user types of the dialer keypad.

I do not want to revert 836215. I want to modify the filter it uses so that it accepts any number from the keypad: that is it accepts any (not too long) sequence of digits * and #.  I propose that we retain current filtering for numbers that contain any other characters.

If someone can tell me that there is a telephone number standard that all mobile operators abide by that guarantees that the numbers we allow are the only numbers that will ever be valid, I'll be happy to drop this. But I doubt that such a standard exists, and the fact that we've already been bitten by this (in bug 845539) makes me very nervous about shipping the phone with this filter in place.

I'd be happy to take this bug if someone who knows more about this area of telephony will tell me that my proposed fix is the right thing to do. I suspect that the fact that mvines made this a tef+ bug is validation that we should fix it, but since he's on vacation maybe we can get someone else to offer an opinion.

Comment 7

[David Flanagan [:djf]]

I'm taking the bug, but am not going to work on it until someone who knows about telephony tells me to proceed.

Comment 8

[Julien Wajsberg [:julienw]]

Numbers with # are actually desirable as it lets the user query the network for eg the account balance. I don't know much more but I know this is wanted.

Comment 9

[Julien Wajsberg [:julienw]]

Beatriz might now more about this.

Comment 10

[Julien Wajsberg [:julienw]]

(*know)

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Numbers with # are actually desirable as it lets the user query the network
> for eg the account balance. I don't know much more but I know this is wanted.

If I am not wrong, numbers containing '#' are considered as MMI codes as specified in 3GPP TS 22.030 (http://www.quintillion.co.jp/3GPP/Specs/22030-340.pdf).

Comment 12

[David Flanagan [:djf]]

Attachment: allow RIL to dial any # of the form /^[0-9*#]{1,64}$/ or any viable number.

This patch retains the call to isViableNumber(), but only uses that function for phone numbers that include characters other than 0-9, * and # that appear on a standard telephone keypad.

If the number passed to RIL is a string of 1 to 64 characters, and all of those characters are ASCII digits, * or #, then we will attempt to dial the number.

Otherwise, we'll call the isViableNumber() function to apply additional filtering and sanity checking.

This might improve performance by preventing the RIL code from loading the phone number utils library in almost all cases.

Also, and I think importantly, this patch adds filtering to emergency numbers as well. Previously these were not filtered which may have been a security issue. 

I'm asking for Gregor's review because he reviewed the original patch in bug 836215.

Comment 13

[Gregor Wagner [:gwagner]]

This is more a product/UX decision. The patch looks good to me but I am not the right person to decide if this is the correct way of doing it.

Comment 14

[David Flanagan [:djf]]

I don't actually think it is a product or UX decision. Its an technical issue. Ideally someone who understands how the RIL works and how phone numbers are dialed on cell networks can weigh in here.  I don't know who that person is.

My main assertion is that we have no business rejecting strings of keypad digits and symbols before they are sent through the RIL to the carrier. We should not be deciding, for example, that single digits are not valid phone numbers.

My patch for this bug makes us accept any string of 0-9, * and # as a valid phone number.  Strings that contain any other characters are passed to the IsViable() function in dom/phonenumberutils/PhoneNumber.jsm to determine whether we'll dial them.

Since I filed this bug, I've taken a look at the RIL code, and am now wondering if the IsViable() function bears any relationship to the kinds of numbers we should be passing to the RIL...

If asked to dial the number "+1-212-PE6-5000;ext=123", we'll treat that as a viable number and pass it through to the RIL. I don't see any code in the RIL for converting "PE" to digits or for handling the extension, so I think we send the string unmodified out to the network. Is that actually expected to work?

Similarly, our current filtering accepts unicode characters for arabic digits like ٣٣ -٣٣٣٣

We'd pass that string of characters to the RIL as well. Will cellular networks be able to dial that as 33-3333 or will they choke?

(I'm not certain whether gaia will ever pass numbers of those forms to gecko. If so, it would require a tel:// URL or clever use of the contacts API, but it could be that the dialer app would reject the number before it got to gecko.)

Comment 15

[Julien Wajsberg [:julienw]]

Normally, the conversion of letters to digit is done by the PhoneNumber library. It converts each letter to the number that we used to press to type this letter on pre-qwerty phones (eg: 'a', 'b', 'c' all are converted to 2, 'd', 'e', 'f' are converted to 3, etc).

We're supposed to check the resulting number after the PhoneNumber parsed it, not before (is it what's done here ?).

Therefore the number should not have anything else than digits and possibily other control characters (like +, #) whose we need to know the valid list.

Comment 16

[Julien Wajsberg [:julienw]]

Also, I don't think we should do any filter in the Gaia code. Rather we need to react to a possible error from the RIL. This code should be done in another bug as we'll probably need new l10n strings and therefore it won't probably not land in v1.0.1.

Comment 17

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

Comment on attachment 721403 [details] [diff] [review]
allow RIL to dial any # of the form /^[0-9*#]{1,64}$/ or any viable number.

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

Please don't put any phone number validation stuff in RIL. Move to dom/phonenumberutils instead.

Comment 18

[Julien Wajsberg [:julienw]]

For the record, a similar bug in Maemo : https://bugs.maemo.org/show_bug.cgi?id=5357
This comes with loads of example of valid and useful numbers with # and *, that are used for querying the network.

If I understand correctly, the current isViable method is used before using PhoneNumberUtils' parse method, so that the RIL can send an error to Gaia. Is that it ?

Then, we only need to fix the current VALID_PHONE_NUMBER regexp ?

To me, we're to restrictive in validating. For example, there is a special regexp for venezuela numbers "*" followed by only one digit, but why a "*" followed by 2 digits should be forbidden ? I mean, what value does this bring to the user ? I don't think this brings us any more safety (as we're normalizing the phone numbers before sending them to the modem anyway).

Comment 19

[David Flanagan [:djf]]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> Normally, the conversion of letters to digit is done by the PhoneNumber
> library. It converts each letter to the number that we used to press to type
> this letter on pre-qwerty phones (eg: 'a', 'b', 'c' all are converted to 2,
> 'd', 'e', 'f' are converted to 3, etc).

As far as I can tell, the number that Gaia passes to the dial() method is never parsed.  If it passes validation through isViable(), then it is sent verbatim to the modem, unicode characters, extensions and all. I've traced the code through dom/telephony/Telephony.cpp to dom/system/gonk/RILContentHelper.js to dom/system/gonk/RadioInterfaceLayer.js to dom/system/gonk/ril_worker.js  Nothing uses PhoneNumberUtils (except for the isViable() call) and nothing parses or modifies the number that gaia passes in in any way.

Comment 20

[David Flanagan [:djf]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #17)
> Comment on attachment 721403 [details] [diff] [review]
> allow RIL to dial any # of the form /^[0-9*#]{1,64}$/ or any viable number.
> 
> Review of attachment 721403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please don't put any phone number validation stuff in RIL. Move to
> dom/phonenumberutils instead.

Vicamo,

You had already indicated that you didn't think this bug should be fixed, so I purposely didn't ask for your review. I really need someone else who is knowledgeable about this part of gecko and gonk to review this. By offering a r- on Gregor's pending review, you've unset the flag and now no one is going to look at it.

As to the content of the your review, I disagree strongly. PhoneNumberUtils is lazy loaded, so it doesnt' cost us anything until we actually use it. It is over 500 lines long, and we shouldn't load it if we don't need it.  If I move my [0-9*#] regexp to PhoneNumberUtils, then we'll always load that module. If I leave it in RIL then we will almost never load that module.   The validation is happening in RIL, so I don't see anything wrong with putting a regexp in RIL.

Comment 21

[Julien Wajsberg [:julienw]]

Right, it seems this is only used for SMS. I don't know why...

Vicamo, any idea why the Telephony API doens't use PhoneNumberUtils.parse ? Is this done on purpose or is it something we forgot ?

Comment 22

[David Flanagan [:djf]]

Comment on attachment 721403 [details] [diff] [review]
allow RIL to dial any # of the form /^[0-9*#]{1,64}$/ or any viable number.

Asking Lucas for a review since this all started out as a security concern.

As a general thing, I would appreciate it if someone cc'ed on this bug would offer an opinion whether this is a bug that should be fixed.  

Vicamo doesn't think it needs to be fixed. I think it is a freaking three alarm emergency. But I know practically nothing about this area of the codebase.  Someone please back me up or talk me down!

Comment 23

[Julien Wajsberg [:julienw]]

David, as I said before, I think you're right in fixing this bug, but I don't think having different regexp in different places is a good solution. Also, PhoneNumberUtils.parse should be called at one moment imho.

Comment 24

[David Flanagan [:djf]]

(In reply to Julien Wajsberg [:julienw] from comment #18)

> If I understand correctly, the current isViable method is used before using
> PhoneNumberUtils' parse method, so that the RIL can send an error to Gaia.
> Is that it ?
> 
> Then, we only need to fix the current VALID_PHONE_NUMBER regexp ?

Now that I understand that there is a missing parsing step, I suspect that we don't need isViable() at all.

> 
> To me, we're to restrictive in validating. For example, there is a special
> regexp for venezuela numbers "*" followed by only one digit, but why a "*"
> followed by 2 digits should be forbidden ? I mean, what value does this
> bring to the user ? I don't think this brings us any more safety (as we're
> normalizing the phone numbers before sending them to the modem anyway).

I agree, and that is exactly why I filed this bug as a followup to bug 845539.

Now that we've realized there is a missing parse() step, I suspect that the proper fix is:

1) Revert Bug 836215 and remove the isViable() function entirely

2) Insert the missing call to parse. I don't know where we do it for SMS, but presumably something parallel to that code.  If parsing fails we should trigger the same error that a failure in isViable() currently triggers.

3) Add a much, much simpler sanity/security check of the parsed number to the RIL before we send the number out over the modem.

Julien: does that sound like the right approach to you? (And do you want to take the bug from me?)

Comment 25

[Lucas Adamski [:ladamski]]

I think Antonio is probably in the best position to answer this question. :)

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think this entirely comes down to what we are able to safely send to the network. Surely the GSM standard defines which phone numbers you can call?

We should make the API match as closely as possible what the GSM standard supports, and make the UI match the subset of that that you can type with a numeric keyboard.

Comment 27

[Antonio Manuel Amaya Calvo (:amac)]

Format of international phone numbers is defined on ITU-T E.164, which is also used to define MSISDNs. And that format is +?\d{15} basically. But E.164 doesn't define anything that's not a international phone number, like local numbers, special purpose numbers and such.

There are some items I've found while testing this. For one, both iOS and Android let you enter huge numbers on the keypads but they don't necessarily pass those numbers to the network. 

On our own network I've found a long digit-only combination that when dialed on Firefox OS gets me a "Because of network congestion the number can't be dialed at this moment" locution from the network but when dialed on both iOS and Android just seem to drop the call on the device without bothering the network. And that number is accepted on Firefox OS even with the current, more restrictive patten (which, BTW, doesn't have a maximum length restriction and it should).

For the *device* security reasons, which is why I opened the related bugs and also bug 847896, I think an approach similar to what Dave is defending here should suffice (although the regexp should be /^+?[0-9*#]{1,64}$/) since international numbers can start by + according to ITU-T E.164). Oh, and both the Gaia dialer and the web activity only let you pass 50 chars, not 64.

But I don't know if for the *network* it'll be enough with that or if the device will have to apply some kind of 'network-protecting' logic to get certified. As I said, both iOS and Android seem to have some under-the-sheet number filtering.

And I see the right person to clarify that is already on need-info :)

Comment 28

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

Comment on attachment 721403 [details] [diff] [review]
allow RIL to dial any # of the form /^[0-9*#]{1,64}$/ or any viable number.

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

We can have this all day. DO NOT push phone number parsing code in RadioInterfaceLayer.js.

Comment 29

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

(In reply to Julien Wajsberg [:julienw] from comment #21)
> Right, it seems this is only used for SMS. I don't know why...
> 
> Vicamo, any idea why the Telephony API doens't use PhoneNumberUtils.parse ?
> Is this done on purpose or is it something we forgot ?

parse() is for making international number from national number and mcc, or making national number from an international number.  It can't be used for parsing short codes like '*8' or '12345'.

Comment 30

[David Flanagan [:djf]]

Vicamo,

Thank you for the feedback. It would be more helpful if you would say what you think I should do instead of just telling me what not to do.  In particular you did not comment on the significant performance implications of moving my simple regexp into the large (and usually unused) PhoneNumberUtils.jsm module.

Also, twice now, I have asked others to review my patch, and you have done the review for them. Because you and I have disagreed about whether this is even a valid bug, I'm trying to get a third party to settle the dispute. But if you keep doing the reviews that I've asked others to do, it takes the bug out of their review queue and off their radar. I am interested in what Gregor and Lucas think about this bug, but I do not know if they will ever return to it, since neither of them has a pending review request on it anymore.  Surely there is a way in Bugzilla for you to offer your feedback without setting flags intended for someone else, isn't there?

In any case, the scope of this bug has increased since I attached that patch, which is why Julien is talking about parse()...

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #29)
> (In reply to Julien Wajsberg [:julienw] from comment #21)
> > Right, it seems this is only used for SMS. I don't know why...
> > 
> > Vicamo, any idea why the Telephony API doens't use PhoneNumberUtils.parse ?
> > Is this done on purpose or is it something we forgot ?
> 
> parse() is for making international number from national number and mcc, or
> making national number from an international number.  It can't be used for
> parsing short codes like '*8' or '12345'.

Please see comment 14. Right now, it looks like we can send letters and non-ASCII digits through the RIL to the modem.  Julien suggested that we should be using parse() to convert all phone numbers into valid strings of ASCII digits (plus *#+) before passing them to the modem. He says that the SMS code does this and wonders if we should also be doing it somewhere in the telephony code as well. What do you think? Is it okay to pass Arabic digits (for example) to the modem?

If you agree that we should be calling parse() somewhere, then please see the outline of a fix I propose in comment 24. Does that sound like what we should do here?

Comment 31

[David Flanagan [:djf]]

Maybe we don't need to call parse... Perhaps just using the NormalizeNumber() function in PhoneNumber.jsm would be enough.

Comment 32

[David Flanagan [:djf]]

Julien,

Are you sure that SMS uses PhoneNumberUtils.parse()?  I can't find that in the code either.

Comment 33

[Julien Wajsberg [:julienw]]

Yep, it's used in [1], which is used in several places in the same file.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/ril/MobileMessageDatabaseService.js#730

Vicamo, I don't see the problem in that, maybe it's possible to keep the number if parse returns null.

David, we should not backout Bug 836215 completely as it made valuable changes to NormalizeNumber's regexps too :) The isViable function is good too, we only need to make it better.

And imho we really need to add correctly the parse call, without it the user can't call a phone-number-with-letters which I think is common in some countries.

I'm not sure yet if isViable should be called before or after the call to parse. Doing it after make it simpler but also it may silently hide errors to the user, as |parse| silently strips unwanted characters. Also I saw that isViable checks for ";ext=" stuff, that I don't really know, but if it's there it is certainly important.

IMHO the only important thing is that we don't send unwanted control characters to the modem, cf [1].

David, if you don't mind, and because you suggested it in Comment 24, I'll try to come up with a patch today with my approach.

[1] http://en.wikibooks.org/wiki/Serial_Programming/Modems_and_AT_Commands/Commands_A_-_M#D:_Dial_Command

Comment 34

[David Flanagan [:djf]]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> Yep, it's used in [1], which is used in several places in the same file.
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/ril/
> MobileMessageDatabaseService.js#730
> 

But it looks like that code is only parsing the number when saving it to or reading it from the database.  The number we pass to Send() in dom/sms/src/SmsManager.cpp nver gets parsed or filtered at all. Any string we pass appears to go right to the modem.  Massive security hole, just like for the telephone case.

> Vicamo, I don't see the problem in that, maybe it's possible to keep the
> number if parse returns null.
> 
> David, we should not backout Bug 836215 completely as it made valuable
> changes to NormalizeNumber's regexps too :) The isViable function is good
> too, we only need to make it better.
> 
> And imho we really need to add correctly the parse call, without it the user
> can't call a phone-number-with-letters which I think is common in some
> countries.

Note that NormalizeNumber is broken. So phone numbers with letters do not currently work right, even if you do call parse() See bug 847896 which I just filed.

> 
> I'm not sure yet if isViable should be called before or after the call to
> parse. 

I don't think we need isViable() if you add a call to parse(). And if isViable is unused, it should be taken back out so we're not big unnecessary regexps that just sit around in memory.

Doing it after make it simpler but also it may silently hide errors
> to the user, as |parse| silently strips unwanted characters. Also I saw that
> isViable checks for ";ext=" stuff, that I don't really know, but if it's
> there it is certainly important.
> 
> IMHO the only important thing is that we don't send unwanted control
> characters to the modem, cf [1].
> 

Yes, that's the most important thing. But don't forget the original purpose of this bug: to not filter numbers unnecessarily.

> David, if you don't mind, and because you suggested it in Comment 24, I'll
> try to come up with a patch today with my approach.

Please do!  Will you fix both the telephony and the sms apis as part of this bug? You should probably either edit the description of this bug to reflect the new scope of the issue or file separate bugs.

I'm inclined to think that the phone number parsing should be done in dom/telephony/ and dom/sms/ rather than in dom/system/gonk/RadioInterfaceLayer.js, though working with PhoneNumberUtils.jsm is probably a lot easier to do from the JavaScript file than it would be from the C++ files that define the APIs themselves. I don't know about the SMS API, but it doesn't look like the Telephony API was designed with the possibility of bad input in mind. I don't see any docs about the dial() method throwing exceptions for invalid numbers.

Comment 35

[David Flanagan [:djf]]

I cited the wrong bug above.  The new bug about NormalizeNumber() handling letters incorrectly is bug 849119

Comment 36

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to David Flanagan [:djf] from comment #34)
> (In reply to Julien Wajsberg [:julienw] from comment #33)
> > Yep, it's used in [1], which is used in several places in the same file.
> > 
> > [1]
> > https://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/ril/
> > MobileMessageDatabaseService.js#730
> > 
> 
> But it looks like that code is only parsing the number when saving it to or
> reading it from the database.  The number we pass to Send() in
> dom/sms/src/SmsManager.cpp nver gets parsed or filtered at all. Any string
> we pass appears to go right to the modem.  Massive security hole, just like
> for the telephone case.
> 

And reported, yet for some reason not fixed: Bug 836708.

BTW, on this same line, bug 847896 that restricts what can be passed to the dialer and SMS apps through web activities just landed too.

Comment 37

[David Flanagan [:djf]]

(In reply to Antonio Manuel Amaya Calvo from comment #36)

> 
> And reported, yet for some reason not fixed: Bug 836708.

I've nominated that bug as tef+. We really have to fix security holes like that (and this one) before we ship the phone.

> 
> BTW, on this same line, bug 847896 that restricts what can be passed to the
> dialer and SMS apps through web activities just landed too.

I saw that and noticed that it allows letters for SMS but not for the phone. Is that what we want?  Also, I notice that the regexps allow 0-length strings. Would {1,50} be better than {0,50}?

Comment 38

[Julien Wajsberg [:julienw]]

Attachment: patch v1- b2g18 branch

I backed out most of vicamo's work on related bugs, sorry.

This approach is simpler and doesn't have some of the features of the original code:
* the "ext=" feature that (I think) didn't belong here anyway; it will be quite easy to add it again when we'll need it
* I used Normalize after all, and not Parse, as this seems to be more useful here
* we check for a Dialable number after normalization, maybe it's not so useful after all...
* updated the tests

I tried this on Orange France network with various numbers (USSD, SS, international numbers, non international numbers).

Please tell me if this seems good enough for v1 to you.

Comment 39

[Julien Wajsberg [:julienw]]

Attachment: patch v1 - moz-central branch

same here

Comment 40

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

Comment on attachment 722820 [details] [diff] [review]
patch v1- b2g18 branch

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

I'm not the right person to review this because, as David has mentioned several times, I don't even think this is a valid bug. I don't own PhoneNumberUtils. As long as you don't touch |parseWithMcc()| for no reason, I don't really care the actual matching pattern.

Comment 41

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

Comment on attachment 722821 [details] [diff] [review]
patch v1 - moz-central branch

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

Same here. No effective logic changes made to |parseWithMcc()|.

Comment 42

[Julien Wajsberg [:julienw]]

Comment on attachment 722820 [details] [diff] [review]
patch v1- b2g18 branch

asking from Gregor then.

Comment 43

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

(In reply to David Flanagan [:djf] from comment #30)
> Vicamo,
> 
> Thank you for the feedback. It would be more helpful if you would say what
> you think I should do instead of just telling me what not to do.

Did I say "Move to dom/phonenumberutils instead." in my first r- in comment #17?

> In particular you did not comment on the significant performance implications
> of moving my simple regexp into the large (and usually unused)
> PhoneNumberUtils.jsm module.

We have a dom/phonenumberutils folder but you put phone number vaildation code in dom/system/gonk. We have a PhoneNumberUtils.js for phone number utility functions but you don't want to load it at validating phone numbers because it's a 600 line file. Then why should we have dom/phonenumberutils and PhoneNumberUtils.js in the first place?

DON'T push every thing into RadioInterfaceLayer.js. It's not a garbage can.

Comment 44

[David Flanagan [:djf]]

Comment on attachment 722821 [details] [diff] [review]
patch v1 - moz-central branch

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

This patch fixes dial() and dialEmergency() but does not fix sendSMS() in RadioInterfaceLayer.js.  Do you want to do that here, or as part of a separate bug?  I'd say we should go ahead and fix it now by adding the calls to normalize and isSendable to sendSMS().

I had lots of minor comments, but am giving feedback+ because overall, I like the direction of this patch.  We urgently need a patch, and this one looks to me like it will do for now.

Ideally, our telephony and sms apis would raise exceptions synchronously when passed invalid numbers, and the number validation would be done well before the numbers got down to the RIL.  I don't know if there is time to make that sort of change, but that is what my preference would be.  And as part of that, our telephony and sms APIs should have documentation stating what sort of numbers they accept.

And on that note, are we sure that we want to be in the business of letter->number and unicode number->ascii number normalization at the API or RIL level?  Or should that normalization step be performed at the app level?  If we are going to normalize numbers in the RIL, should we do so so aggressively?  Currently we ignore anything that is not an ASCII letter or a Unicode digit. Obviously we want to ignore normal phone number formatting, but maybe we should report errors for things that are obviously errors rather than ignoring them.

I don't like the fact that dom/system/gonk/RadioInterfaceLayer.js depends on the (kind of large) dom/phonenumberutils/ module.  All we really need for the validation we're doing is a few simple regexps, but we're using a 1000+ line library to do it.  The phone number utils module is designed for parsing and formatting human-readable phone numbers and not really for validation. I'd prefer that we did our own validation with simpler code.  Perhaps we could define a separate, smaller, dom/phonenumberutils/validate.js module that we could import without picking up all of the parsing, formatting and metadata functions of the full module.

::: dom/phonenumberutils/PhoneNumber.jsm
@@ +13,5 @@
>  this.PhoneNumber = (function (dataBase) {
>    // Use strict in our context only - users might not want it
>    'use strict';
>  
> +  const MAX_CHARS = 50;

Could you make this constant name more descriptive? 

Maybe MAX_PHONE_NUMBER_LENGTH?

@@ +18,4 @@
>    const UNICODE_DIGITS = /[\uFF10-\uFF19\u0660-\u0669\u06F0-\u06F9]/g;
>    const NON_ALPHA_CHARS = /[^a-zA-Z]/g;
> +  const NON_DIALABLE_CHARS_GLOBAL = /[^,#+\*\d]/g;
> +  const NON_DIALABLE_CHARS = /[^,#+\*\d]/;

I'd switch the order of these, and then write:

const NON_DIALABLE_CHARS_GLOBAL = new Regexp(NON_DIALABLE_CHARS.source, "g");

that way their definitions cannot be different.

@@ +22,1 @@
>    const PLUS_CHARS = "+\uFF0B";

You no longer need this as a separate constant, and can just define LEADING_PLUS_CHARS_PATTERN as a regexp literal

@@ +25,1 @@
>    const VALID_ALPHA = "a-zA-Z";

I don't think you need this separately from VALID_ALPHA_PATTERN anymore

@@ +224,5 @@
>    function NormalizeNumber(number) {
> +    if (number === null) {
> +      return '';
> +    }
> +

By using === above, you handle the null case, but do not handle the undefined case. The code also does not handle booleans, etc.

Maybe you should just do

 if (typeof number !== 'string') return '';

@@ +232,5 @@
>                              });
>      number = number.replace(VALID_ALPHA_PATTERN,
>                              function (ch) {
>                                return (ch.toLowerCase().charCodeAt(0) - 97)/3+2 | 0;
>                              });

The 4 lines above do not work right. But a fix just landed in the upstream repo. See  bug 849119

@@ +234,5 @@
>                              function (ch) {
>                                return (ch.toLowerCase().charCodeAt(0) - 97)/3+2 | 0;
>                              });
>      number = number.replace(LEADING_PLUS_CHARS_PATTERN, "+");
> +    number = number.replace(NON_DIALABLE_CHARS_GLOBAL, "");

I'm not entirely convinced by this line. I know you didn't add it.  I agree that this function should strip whitespace, parentheses and hyphens and any other normal phone number punctuation. 

But if the number includes other characters and we normalize by just discarding them, then I suspect there will be errors that go unreported.  I think it might be better to not normalize here and then let isSendable() report an error.

Of course changing Normalize() will alter the behavior of Parse(), so we probably can't do that.  I'd prefer it, though if our APIs didn't have to depend on the module at all, so maybe we can have a version of normalize and isSendable() somewhere else?  I don't know what the best way to do that would be.

@@ +368,5 @@
>      return null;
>    }
>  
> +  function IsSendablePhoneNumber(number) {
> +    number = '' + number;

If number is not a string, I think that should be treated as an error and we should return false immediately rather than converting it.

@@ +369,5 @@
>    }
>  
> +  function IsSendablePhoneNumber(number) {
> +    number = '' + number;
> +    var isTooLong = (number).length > MAX_CHARS;

nit: no need for the parentheses here.

@@ +370,5 @@
>  
> +  function IsSendablePhoneNumber(number) {
> +    number = '' + number;
> +    var isTooLong = (number).length > MAX_CHARS;
> +    var isEmpty = (number === '');

Since you're testing the length above, I'd just do number.length === 0 here instead of comparing to the empty string.

@@ +371,5 @@
> +  function IsSendablePhoneNumber(number) {
> +    number = '' + number;
> +    var isTooLong = (number).length > MAX_CHARS;
> +    var isEmpty = (number === '');
> +    return !(isTooLong || isEmpty || NON_DIALABLE_CHARS.test(number));

I don't know much about regexp perf. If you used NON_DIALABLE_CHARS_GLOBAL here with the test() method would that actually be slower?  If so, that seems like a bug that should be reported. 

If we can make this work with just a single version of the regexp, that would be better.

::: dom/phonenumberutils/tests/test_phonenumber.xul
@@ +156,5 @@
> +Normalize("123 +123456#", "123+123456#");
> +Normalize("#123#", "#123#");
> +Normalize("*#004#", "*#004#");
> +Normalize("*30#", "*30#");
> +Normalize("*#06#", "*#06#");

If we're going to be supporting the conversions of letters to digits and Unicode digits to ASCII digits, we should test those. 

See the last two lines of https://github.com/andreasgal/PhoneNumber.js/blob/8223916cf465a751a4180ee5caa868b475d8ad97/test.js for letter->number tests

But please also test all of these digits:

UNICODE_DIGITS = /[\uFF10-\uFF19\u0660-\u0669\u06F0-\u06F9]/g;

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2082,5 @@
> +        error: RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER]
> +      });
> +      debug("Number '" + number + "' doesn't seem to be a viable number. Drop.");
> +      return;
> +    }

Is it worth factoring out the error reporting code here and above into a shared helper function?

Are you purposely not normalizing the emergency number?

Comment 45

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to David Flanagan [:djf] from comment #37)
> (In reply to Antonio Manuel Amaya Calvo from comment #36)
> 
> > 
> > And reported, yet for some reason not fixed: Bug 836708.
> 
> I've nominated that bug as tef+. We really have to fix security holes like
> that (and this one) before we ship the phone.
> 
> > 
> > BTW, on this same line, bug 847896 that restricts what can be passed to the
> > dialer and SMS apps through web activities just landed too.
> 
> I saw that and noticed that it allows letters for SMS but not for the phone.
> Is that what we want?  Also, I notice that the regexps allow 0-length
> strings. Would {1,50} be better than {0,50}?

I answered the first part on the other bug already :). The SMS app (and gecko code) allow letters on destination numbers so I allowed it on the webactivity also. The dialer app doesn't so I didn't. 

As for the 1,50 instead of 0,50 the thing is that the number wasn't required initially and I didn't find any security problem with allowing using the activity just to launch the sms or dialer without passing a number. What I didn't wanted was allowing a way to out-of-mem them easily.

Comment 46

[David Flanagan [:djf]]

Comment on attachment 722820 [details] [diff] [review]
patch v1- b2g18 branch

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

It looks like I ended up giving feedback on the m-c version when I though I was looking at the b2g-18 version.  

My feedback on this version of the patch is the same a on the m-c version, except for the one comment below...

::: dom/phonenumberutils/PhoneNumber.jsm
@@ +357,5 @@
>      return null;
>    }
>  
> +  function IsSendablePhoneNumber(number) {
> +    return (number !== '' && !NON_DIALABLE_CHARS.test(number));

Why is this so different in the b2g-18 patch than in the m-c patch?  Since it is a new function, I don't understand why it is different at all.

Was this a mistake in assembling your patch?

Comment 47

[Julien Wajsberg [:julienw]]

(In reply to David Flanagan [:djf] from comment #44)
> Comment on attachment 722821 [details] [diff] [review]
> patch v1 - moz-central branch
> 
> Review of attachment 722821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch fixes dial() and dialEmergency() but does not fix sendSMS() in
> RadioInterfaceLayer.js.  Do you want to do that here, or as part of a
> separate bug?  I'd say we should go ahead and fix it now by adding the calls
> to normalize and isSendable to sendSMS().

I just pushed a patch in Bug 836708 ;)

> 
> I had lots of minor comments, but am giving feedback+ because overall, I
> like the direction of this patch.  We urgently need a patch, and this one
> looks to me like it will do for now.

I'm not sure that this is that urgent. Bug 835750 fixed something in the Gaia part, and bug 847896 did this for the activities.

> 
> Ideally, our telephony and sms apis would raise exceptions synchronously
> when passed invalid numbers, and the number validation would be done well
> before the numbers got down to the RIL.  I don't know if there is time to
> make that sort of change, but that is what my preference would be.  And as
> part of that, our telephony and sms APIs should have documentation stating
> what sort of numbers they accept.

I agree. Please file a bug :-)

> And on that note, are we sure that we want to be in the business of
> letter->number and unicode number->ascii number normalization at the API or
> RIL level?  Or should that normalization step be performed at the app level?

Not the app level because I want that any client of this API could throw this. But this is something to be decided by our API specifiers. Certainly this will ultimately change when going through standardization, let's wait for this !

> If we are going to normalize numbers in the RIL, should we do so so
> aggressively?  Currently we ignore anything that is not an ASCII letter or a
> Unicode digit. Obviously we want to ignore normal phone number formatting,
> but maybe we should report errors for things that are obviously errors
> rather than ignoring them.

I agree also. In SMS we're actually refreshing the title with the actual number that was used, but in the Dialer we're not doing that and this may be strange to the user.

I think this is good enough for now (in a normal usage, the user can only dial numbers anyway, so...).

> 
> I don't like the fact that dom/system/gonk/RadioInterfaceLayer.js depends on
> the (kind of large) dom/phonenumberutils/ module.  All we really need for
> the validation we're doing is a few simple regexps, but we're using a 1000+
> line library to do it.  The phone number utils module is designed for
> parsing and formatting human-readable phone numbers and not really for
> validation. I'd prefer that we did our own validation with simpler code. 
> Perhaps we could define a separate, smaller,
> dom/phonenumberutils/validate.js module that we could import without picking
> up all of the parsing, formatting and metadata functions of the full module.

Agreed.

Actually I don't clearly understand why we have both PhoneNumberUtils and PhoneNumber libraries. Probably we could be more clever here but I'd like to keep this for another time (and someone else ;) ).

Comment 48

[Julien Wajsberg [:julienw]]

Comment on attachment 722820 [details] [diff] [review]
patch v1- b2g18 branch

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

::: dom/phonenumberutils/PhoneNumber.jsm
@@ +357,5 @@
>      return null;
>    }
>  
> +  function IsSendablePhoneNumber(number) {
> +    return (number !== '' && !NON_DIALABLE_CHARS.test(number));

Oops, the right version is the one in the m-c patch. Seems like I forgot to refresh something.

Comment 49

[Julien Wajsberg [:julienw]]

Comment on attachment 722821 [details] [diff] [review]
patch v1 - moz-central branch

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

::: dom/phonenumberutils/PhoneNumber.jsm
@@ +13,5 @@
>  this.PhoneNumber = (function (dataBase) {
>    // Use strict in our context only - users might not want it
>    'use strict';
>  
> +  const MAX_CHARS = 50;

right, will do

@@ +18,4 @@
>    const UNICODE_DIGITS = /[\uFF10-\uFF19\u0660-\u0669\u06F0-\u06F9]/g;
>    const NON_ALPHA_CHARS = /[^a-zA-Z]/g;
> +  const NON_DIALABLE_CHARS_GLOBAL = /[^,#+\*\d]/g;
> +  const NON_DIALABLE_CHARS = /[^,#+\*\d]/;

Good idea, I didn't know |.source| and I didn't want to have a 3rd constant.

@@ +224,5 @@
>    function NormalizeNumber(number) {
> +    if (number === null) {
> +      return '';
> +    }
> +

Yes, that's probably better, thanks

@@ +232,5 @@
>                              });
>      number = number.replace(VALID_ALPHA_PATTERN,
>                              function (ch) {
>                                return (ch.toLowerCase().charCodeAt(0) - 97)/3+2 | 0;
>                              });

I knew it :) that's why I didn't touch this at all.

@@ +234,5 @@
>                              function (ch) {
>                                return (ch.toLowerCase().charCodeAt(0) - 97)/3+2 | 0;
>                              });
>      number = number.replace(LEADING_PLUS_CHARS_PATTERN, "+");
> +    number = number.replace(NON_DIALABLE_CHARS_GLOBAL, "");

I tried to think of it too and then decided this could wait and it was good enough, because the user can't dial something else than numbers, and the sms app shows which number was actually used. What do you think ?

@@ +368,5 @@
>      return null;
>    }
>  
> +  function IsSendablePhoneNumber(number) {
> +    number = '' + number;

you're probably right here

@@ +369,5 @@
>    }
>  
> +  function IsSendablePhoneNumber(number) {
> +    number = '' + number;
> +    var isTooLong = (number).length > MAX_CHARS;

ah ah, I had ('' + number) inside my parentheses :)

@@ +371,5 @@
> +  function IsSendablePhoneNumber(number) {
> +    number = '' + number;
> +    var isTooLong = (number).length > MAX_CHARS;
> +    var isEmpty = (number === '');
> +    return !(isTooLong || isEmpty || NON_DIALABLE_CHARS.test(number));

I originally did that, but with a global, it looks like the parser starts again at the lastIndex where it stopped. That's why I switched to a non-global regexp instead.

Comment 50

[Julien Wajsberg [:julienw]]

Attachment: patch v2 - b2g18 branch

fixed djf's remark

Comment 51

[Julien Wajsberg [:julienw]]

Attachment: patch v2 - moz central branch

same

Comment 52

[Gregor Wagner [:gwagner]]

Comment on attachment 722920 [details] [diff] [review]
patch v2 - b2g18 branch

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

isViable function will be useful within the library and we might add it again in the future but for the current use-case we need isSendablePhoneNumber.
BTW I don't like this name. Lets rename it to isDiable?

r=me with the comments addressed.
Can you also make a PR for upstream?

::: dom/phonenumberutils/tests/test_phonenumber.xul
@@ +29,5 @@
> +   } else {
> +    ok(true, dial + " is " + (result ? "" : "not ") + "sendable as expected.");
> +   }
> + }
> + 

Whitespace

@@ +40,2 @@
>    }
> +  return result;

Why do you need to return here?

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1955,5 @@
> +        callIndex: -1,
> +        error: RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER]
> +      });
> +      debug("Number '" + number + "' doesn't seem to be a viable number. Drop.");
> +      return;

I liked Davids approach better with moving the error handling to its own function.

Comment 53

[Julien Wajsberg [:julienw]]

Comment on attachment 722920 [details] [diff] [review]
patch v2 - b2g18 branch

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

thanks to all, I will fix these last comments on Monday !

::: dom/phonenumberutils/tests/test_phonenumber.xul
@@ +40,2 @@
>    }
> +  return result;

probably left over from something, will remove

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1955,5 @@
> +        callIndex: -1,
> +        error: RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER]
> +      });
> +      debug("Number '" + number + "' doesn't seem to be a viable number. Drop.");
> +      return;

good idea

Comment 54

[Julien Wajsberg [:julienw]]

(In reply to Gregor Wagner [:gwagner] from comment #52)
> Comment on attachment 722920 [details] [diff] [review]
> patch v2 - b2g18 branch
> 
> Review of attachment 722920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> isViable function will be useful within the library and we might add it
> again in the future but for the current use-case we need
> isSendablePhoneNumber.
> BTW I don't like this name. Lets rename it to isDiable?

|isDiable| is not good either, because it will be used by the SMS API too. |isSendable| actually implied (for me) "sendable to the modem".

what about |isPlainPhoneNumber| ?

Comment 55

[Julien Wajsberg [:julienw]]

Attachment: patch v3 - mozcentral branch

Asking again for review again because I made enough changes to not be comfortable carrying the r+.

Comment 56

[Julien Wajsberg [:julienw]]

Attachment: patch v3 - b2g18 branch

same patch for b2g18 branch

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d42cafe0fbfd

Comment 58

[Gregor Wagner [:gwagner]]

Comment on attachment 723415 [details] [diff] [review]
patch v3 - b2g18 branch

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

You don't have to ask for review for the b2g18 version.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d42cafe0fbfd

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/35c65560bd76
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/850e37d7750d

Comment 61

[David Flanagan [:djf]]

I've filed bug 851201 as a followup to get this fixed at the API level instead of the RIL level.

That's the right thing to do, and I learned in https://bugzilla.mozilla.org/show_bug.cgi?id=836708#c30 that this patched RIL isn't even going to be on production phones in the first place!

Comment 62

[Beatriz Rodríguez [:brg]]

Removing ni flag, sorry I forgot this bug and I think now everything is landed or agreed. If you need someting form my side, please add me again.