832946 - [Settings] Call support of Help menu don't link with call app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Leo]

1. Title : Call support of Help menu don't link with call app
2. Precondition : Settings - Help - Call support 
3. Tester's Action : touch number(*8486)
4. Detailed Symptom (ENG.) : No action. But when touch 1058, go to dialer screen. It's differnt action.
6. Expected : 
7. Reproducibility: Y
1)Frequency Rate : 100%
8. Gaia revision : a03f7b532e9998e646d55f93a0fc03a04d7ca7d9

Comment 1

[Sam Shipley]

Call app is still not spawned and number auto population is unsuccessful when user selects the touch number (*8486) from the "Call Support" Help menu.

Unagi Device

Environmental  Variables:
Build ID: 20130221070203
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/effe37a77f18
Gaia: bb633c6f2deb829b2f3d5b9e7bac7fa24467d02a

Comment 2

[Sam Shipley]

Call app is still not spawned and number auto population is unsuccessful when user selects the touch number (*8486) from the "Call Support" Help menu during testrun 5.1

Unagi Device

Environmental  Variables:
Build ID: 20130225070200
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/3a5a27992a75
Gaia: 5691a16fff8e1403c75ed9d6f3a443b7e58198c6

Comment 3

[Dietrich Ayala (:dietrich)]

The "call support" button in the phone should actually call support.

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

The number *8486 is ignored by gecko. This bug was regressed by bug 797034. There should be a better way examining invalid phone numbers. 

John, is there any plan of improving it?

Comment 5

[John Schoenick [:johns]]

Bug 794034 was a rushed fix for this, then bug 797034 was opened to follow up and improve it, which is still open 

I'm not very involved with B2G work and probably wouldn't be the guy to fix the latter, unfortunately :(

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Patch v1

Boris, this change improves the method of ignoring MMI/USSD codes. Could you help review the change? Thanks!

Comment 7

[John Schoenick [:johns]]

Comment on attachment 724314 [details] [diff] [review]
Patch v1

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

::: b2g/components/TelURIParser.jsm
@@ +109,5 @@
>        }
>      }
>  
> +    // Ignore MWI and USSD codes. See 794034.
> +    if (number.match(/^[#\*].*#$/)) {

This would not block *#*#7780#*#* which is a factory reset on some android devices.

See https://bugzilla.mozilla.org/show_bug.cgi?id=794034#c20

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 724314 [details] [diff] [review]
Patch v1

I can't review this, unfortunately, because I have no idea what the goal of this code is on b2g and hence which things it needs to block...

Comment 9

[Wayne Chang [:wchang]]

I think we can consider this bug invalid -
1 > There is bug 797034 to track a better method of parsing/handling bad TEL formats
and
2 > as indicated in Bug 846197 the information displayed currently are for vivo and the device should not ship with this. This page should be customizable by carriers and it should be recommended that they use only appropriate number formats.

setting leo? to be revisited by triage

Comment 10

[Dietrich Ayala (:dietrich)]

Triage group agrees that we cannot ship in the current state.

We either need to:

1. not present unsupported tel: numbers as clickable

or 

2. support USSD in tel: links

Do we need a new bug to track #2?

Daniel: Does Vivo require USSD help number? Is that number the right one?

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Backend does support USSD. Blocking USSD here was preventing users from clicking on a link with a factory reset code by mistake (although they still need to press the dial button). The question is there seems no exact pattern of such numbers because it is determined by the carrier. Thus after the patch of bug 794034, all numbers with '*' or '#' get blocked. It seems like there are two options:

1. Suggest carriers not using numbers with '*' or '#'.
2. Only block numbers of USSD format plus numbers of the pattern *#*#7780#*#*.

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

not sure if :dpv can provide more input here?

Comment 13

[David Palomino [:dpalomino]]

Hi, 

Not sure if it could help this. 

Sometimes carriers use MSISDNs like (*number, f.i. "*86", "*123" are used for voice mailbox). Not sure about more complex structures. 

I think at least we should allow strings such as "*" + "number". 

Thks!
David

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

Let's block numbers starting with '#' or '*' and ending with '#' or '*'. Does it make sense?

Comment 15

[John Schoenick [:johns]]

(In reply to Arthur Chen (OOO 3/28 ~ 3/29) [:arthurcc] from comment #14)
> Let's block numbers starting with '#' or '*' and ending with '#' or '*'.
> Does it make sense?

##7764726 – Hidden service menu for Motorola Droid

http://www.redmondpie.com/hidden-android-secret-codes-for-samsung-htc-motorola-sony-lg-and-other-devices/

It seems like a whitelist approach of allowed patterns would be safer, e.g. c13

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Patch v2

Ignore numbers containing '*' or '#' except for ones in the format of "*" and "#" following by numbers. John, could you help review it?

Comment 17

[John Schoenick [:johns]]

Comment on attachment 731789 [details] [diff] [review]
Patch v2

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

::: b2g/components/TelURIParser.jsm
@@ +109,5 @@
>        }
>      }
>  
> +    // Ignore MWI and USSD codes. See 794034.
> +    if (number.match(/[#\*]/) && !number.match(/^[#\*]\d+$/)) {

Does the number potentially contain other characters (spaces, commas) at this point?

I don't see any immediately abusive codes this allows (though there's nothing technically stopping the carrier from having *1234 be a device reset)

I'm also not a peer on any of the android stuff, and am mostly working off of google here, so somebody on the android team should probably give the final r+ here.

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 731789 [details] [diff] [review]
Patch v2

Philipp, I'm not sure if you are able to review this change, or maybe you can redirect it to the right one? Thanks!

Comment 19

[Dietrich Ayala (:dietrich)]

Arthur, it's unlikely philikon will be able to review this soon. Who else can review this patch?

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 731789 [details] [diff] [review]
Patch v2

Blake, could you help review this change? Thanks!

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 731789 [details] [diff] [review]
Patch v2

I'm sorry, I don't know nearly enough about special numbers to review this. Gregor might have a better idea.

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 731789 [details] [diff] [review]
Patch v2

Gregor, could you help review the change or add qualified reviewers? Thanks.

Comment 23

[Gregor Wagner [:gwagner]]

(In reply to Arthur Chen [:arthurcc] from comment #22)
> Comment on attachment 731789 [details] [diff] [review]
> Patch v2
> 
> Gregor, could you help review the change or add qualified reviewers? Thanks.

Can you add tests here?
https://mxr.mozilla.org/mozilla-central/source/b2g/components/test/unit/test_bug793310.js

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Patch v3

New patch with tests added.

Comment 25

[Gregor Wagner [:gwagner]]

Comment on attachment 736171 [details] [diff] [review]
Patch v3

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

Thanks!

Comment 26

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks Gregor!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ee76e6261e6

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ee76e6261e6

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e5757cbd5e29

Comment 30

[William Hsu [:whsu]]

Creating a new test case.
- https://moztrap.mozilla.org/manage/case/8469/