870711 - [B2G] [Leo][SMS] It is possible to invoke the Browser App from an invalid URL text included in a SMS

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[bov]

Tested on a Leo device. 
Platform Version: 18.0 Build ID: 20130425170448 Gaia: f9d8af0

STEPS:
1. Send from another device to the Device under Test an SMS including an invalid URL expression 
(Valid expression in accordance with the wireframe https://bugzilla.mozilla.org/attachment.cgi?id=725423, is:
" /(^|\s|,|;)[-\w:%\+.~#?&//=]{2,256}\.[a-z]{2,6}(?:\/[-\w:%\+.~#?&//=]*)?/mgi")  
(e.g. invalid expressions testing both in sent and recieved SMS are:
"http://www.b/g/v.com/",
"htt://www.google.com/", 
"http://www.wikipedia.o/",)
2. Open in the Device under Test the SMS APP
3. Search and tap on the SMS

EXPECTED RESULT:
1 y 2. The invalid URL expresion is shown in the same way that the rest of the content of the SMS thread (text).
3. Cannot open the Browser App

ACTUAL RESULT:
The invalid URL expresion is shown highlighted in the SMS thread view, and tapping in the two last expressions ("htt://www.google.com/","http://www.wikipedia.o/"), the Browser is opened showing incorrect sites.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Nice to have polish but not a blocker for shipping.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

While dogfooding I've had messages with ellipses (...) turned into URLs.  Here are some actual examples:

  "no...not"
  "sleepy...and"
  "crutches...be"

Comment 4

[Rick Waldron [:rwaldron]]

As I did in the dup...

Requesting leo because the user experience is incredibly poor

Comment 5

[bov]

This issue is tested on Leo 1.1 (Test Run Firefox OS 1.1.0 Full – Leo)
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8e3f39363c54
Gaia: ea18de80fd04110756becaed214656642388401d
Platform Version: 18.0

STEPS:
1. Send from another device to the DUT, or from the DUT send to another device an SMS including an invalid URL expression 
(Valid expression in accordance with the WF is:
" /(^|\s|,|;)[-\w:%\+.~#?&//=]{2,256}\.[a-z]{2,6}(?:\/[-\w:%\+.~#?&//=]*)?/mgi")  
(e.g. invalid expressions testing both in sent and recieved SMS are:
   a)"http://www.b/g/v.com/",
   b)"htt://www.google.com/", 
   c)"http://www.wikipedia.o/"
2. Open in the Device under Test the SMS APP
3. Search and tap on the SMS

EXPECTED RESULT:
1. y 2. The invalid URL expresion is shown in the same way that the rest of the content of the SMS thread (text).
3. Cannot open the Browser App clicking on the URL's

ACTUAL RESULT:
   a)"http://www.b/g/v.com/": The invalid URL expresion is shown underlined and tapping on it the browser is opened but cannot find the site.
   b)"htt://www.google.com/": It works as expected.
   c)"http://www.wikipedia.o/": The invalid URL expresion is shown underlined and tapping on it the browser is opened showing an incorrect (redirected) site.

Link to failed test case: 
https://moztrap.mozilla.org/manage/case/7201/
https://moztrap.mozilla.org/manage/case/7203/
https://moztrap.mozilla.org/manage/case/7205/

Comment 6

[:gerard-majax]

For the http://www.b/g/v.com/ case, it is solved by changing /(^|\s|,|;)[-\w:%\+.~#?&//=]{2,256}\.[a-z]{2,6}(?:\/[-\w:%\+.~#?&//=]*)?/mgi to /(^|\s|,|;)[-\w:%\+.~#?&=]{2,256}\.[a-z]{2,6}(?:\/[-\w:%\+.~#?&//=]*)?/mgi

Another buggy case arises with URLs with credentials.

Comment 7

[:gerard-majax]

(In reply to bov from comment #5)
> This issue is tested on Leo 1.1 (Test Run Firefox OS 1.1.0 Full – Leo)
> Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8e3f39363c54
> Gaia: ea18de80fd04110756becaed214656642388401d
> Platform Version: 18.0
> 
> STEPS:
> 1. Send from another device to the DUT, or from the DUT send to another
> device an SMS including an invalid URL expression 
> (Valid expression in accordance with the WF is:
> "
> /(^|\s|,|;)[-\w:%\+.~#?&//=]{2,256}\.[a-z]{2,6}(?:\/[-\w:%\+.~#?&//=]*)?/
> mgi")  
> (e.g. invalid expressions testing both in sent and recieved SMS are:
>    a)"http://www.b/g/v.com/",
>    b)"htt://www.google.com/", 
>    c)"http://www.wikipedia.o/"
> 2. Open in the Device under Test the SMS APP
> 3. Search and tap on the SMS
> 
> EXPECTED RESULT:
> 1. y 2. The invalid URL expresion is shown in the same way that the rest of
> the content of the SMS thread (text).
> 3. Cannot open the Browser App clicking on the URL's
> 
> ACTUAL RESULT:
>    a)"http://www.b/g/v.com/": The invalid URL expresion is shown underlined
> and tapping on it the browser is opened but cannot find the site.
>    b)"htt://www.google.com/": It works as expected.
>    c)"http://www.wikipedia.o/": The invalid URL expresion is shown
> underlined and tapping on it the browser is opened showing an incorrect
> (redirected) site.
> 
> Link to failed test case: 
> https://moztrap.mozilla.org/manage/case/7201/
> https://moztrap.mozilla.org/manage/case/7203/
> https://moztrap.mozilla.org/manage/case/7205/

I have a new regex proposal, it's working better on the specified cases:
(^|\s|,|;|<br>)(https?://)?(\w+@|\w+:.*@)?([\w\.]{2,256})?\.([a-zA-Z]{2,6})(:[0-9]{1,5})?(\/[-\w:%\+.~#?;&//=]*)?

It even handles passing login/password in the URL. It does not recognize IPv4 nor IPv6.

Comment 8

[:gerard-majax]

Rha turns out it fails on the wikipedia.o example.

Comment 9

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #8)
> Rha turns out it fails on the wikipedia.o example.

FYI in http://www.wikipedia.o/ example, "www." is recognized as the server name and "wikipe" is recognized as the TLD.

Comment 10

[:gerard-majax]

I'll try and see to apply :julienw's work that was suggested in bug 846373 https://bug846373.bugzilla.mozilla.org/attachment.cgi?id=731827

Comment 11

[bov]

Also link to failed test case:
https://moztrap.mozilla.org/manage/case/7187/

Comment 12

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/10396

Please find attached a link to the Github pull requests. It fixes the reported cases with a simple set of workaround, without rewriting anything.

I don't think we can efficiently reach perfection for this.

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 762665 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10396

redirecting to Corey who's free this morning ;)

Comment 14

[Corey Frang [:gnarf]]

Comments on pull request - please fix lint & units

Comment 15

[:gerard-majax]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #14)
> Comments on pull request - please fix lint & units

Thanks for your feedback. I've addressed the nits and also fixed a bit more the regexes.

Comment 16

[Corey Frang [:gnarf]]

Comment on attachment 762665 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10396

Still a lot of unit tests failing...

Comment 17

[:gerard-majax]

Comment on attachment 762665 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10396

Fixed the unit tests, this was due to another bug being fixed in the meantime, as commented on the pull request.

Comment 18

[Corey Frang [:gnarf]]

Comment on attachment 762665 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10396

sent a pull to your pull: https://github.com/lissyx/gaia/pull/2 - please r? me again after update

Comment 19

[Corey Frang [:gnarf]]

Paul - I was hoping you could tell me if we need to support URL's like http://1.2.3.4/ as linkables in SMS, Could you test this on a 1.0.1 device?

Comment 20

[Paul Yang [:pyang] (away)]

I believe this feature is started from v1.1
https://bugzilla.mozilla.org/show_bug.cgi?id=838005 
[SMS][User Story] Browser invocation from message

Comment 21

[Natalya Kot [:nkot]]

 - v1.0.1 - SMS app does not linkify any URLs 
 
 - http://1.2.3.4/ - is not linkable on v1.1
 
 - v1.1 - latest 6/25 Leo build, all issues are still reproducible as per comment 5 

"ACTUAL RESULT:
   a)"http://www.b/g/v.com/": The invalid URL expresion is shown underlined and tapping on it the browser is opened but cannot find the site.
   b)"htt://www.google.com/": It works as expected.
   c)"http://www.wikipedia.o/": The invalid URL expresion is shown underlined and tapping on it the browser is opened showing an incorrect (redirected) site."


Unagi
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
Gaia   93241eb6c5d6c110710fad8da3ccd4423312b0c9
Build  20130625070207
Leo
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/29933d1937db
Gaia   1436e2778b90bd74635b0b94d1cf8ccb0d71b60c
Build  20130625070217

Comment 22

[Rick Waldron [:rwaldron]]

(In reply to nkot from comment #21)
>  - v1.0.1 - SMS app does not linkify any URLs 
>  
>  - http://1.2.3.4/ - is not linkable on v1.1
>  
>  - v1.1 - latest 6/25 Leo build, all issues are still reproducible as per
> comment 5 
> 
> "ACTUAL RESULT:
>    a)"http://www.b/g/v.com/": The invalid URL expresion is shown underlined
> and tapping on it the browser is opened but cannot find the site.
>    b)"htt://www.google.com/": It works as expected.
>    c)"http://www.wikipedia.o/": The invalid URL expresion is shown
> underlined and tapping on it the browser is opened showing an incorrect
> (redirected) site."
> 
> 

Note that even Bugzilla thinks those are valid URL strings that should be formatted as clickable links. The same thing you describe happens when you click on them from here.

Comment 23

[Rick Waldron [:rwaldron]]

Also, SMS should use the same mechanism that Browser does for fixing up user input in the address bar:

- Determine if a string is just a word or a URL
- Creating links from strings that are a URL

https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/js/browser.js#L692-L707

https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/js/browser_extensions.js

This is actually ported from Firefox's own mechanism for the same operation: 

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp#773

Comment 24

[Corey Frang [:gnarf]]

I've been playing a bit more with this, adding more edges: https://github.com/gnarf/gaia/compare/url-regexp

It can now handle (mozilla.org) and (Check out mzl.la/ac) and http://en.wikipedia.org/wiki/Arete_(disambiguation) correctly

Comment 25

[Corey Frang [:gnarf]]

Which I must add - bugzilla can't ;)

Comment 26

[Corey Frang [:gnarf]]

I'd like to take the work started here and base my work on bug 887737 on it - requesting leo+

Comment 27

[Julien Wajsberg [:julienw]]

blocking a blocker then.

Corey, please request feedback on Alexandre :)

Comment 28

[Corey Frang [:gnarf]]

Comment on attachment 762665 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10396

I'm going to pick up this work and run with it - There are a lot of things that need to be fixed in LinkHelper

Comment 29

[Corey Frang [:gnarf]]

Attachment: pull request on github

Please let me merge this myself

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 770089 [details] [review]
pull request on github

r=me

thanks for this hard work, let's see how this behaves on big workloads ;)

Comment 31

[Corey Frang [:gnarf]]

Comment on attachment 770089 [details] [review]
pull request on github

gerrad peeked at this already and said it looked good to him, never updated the flag though

Comment 32

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/a310b24b731d5a3c1779e65f112dc5ee88758ddc
v1-train: https://github.com/mozilla-b2g/gaia/commit/af4e9d625324f824a10a08cfb46ef0e82e271a54

Comment 33

[bov]

This issue tested on Leo 1.1 (Test Run Firefox OS 1.1.0 Full – Leo Run 4)
BUILD:
Gecko a378807ff04076c20f08b0102286b9eb2d08d60a
Gaia 1436e2778b90bd74635b0b94d1cf8ccb0d71b60c
Platform Version: 18.1

STEPS:
1. Send from another device to the DUT, or from the DUT send to another device an SMS including an invalid URL expression 
   a)"http://www.b/g/v.com/",
   b)"htt://www.google.com/", 
   c)"http://www.wikipedia.o/"
2. Open in the Device under Test the SMS APP
3. Search and tap on the SMS

EXPECTED RESULT:
1. y 2. The invalid URL expresions are shown in the same way that the rest of the content of the SMS thread (text).
3. Cannot open the Browser App clicking on the URL's

ACTUAL RESULT:
   a)"http://www.b/g/v.com/": The invalid URL expresion is shown underlined and tapping on it the browser is opened but cannot find the site.
   b)"htt://www.google.com/": It works as expected.
   c)"http://www.wikipedia.o/": The invalid URL expresion is shown underlined and tapping on it the browser is opened but cannot find the site.

THIS RUN IS EXECUTED WITH A PREVIOUS BUILD THAT THE ONE THE BUG IS FIXED AS IT IS MENTIONED HERE, SO IT IS EXPECTED TO WORK OK TILL NOW.

Link to failed test case: 
 https://moztrap.mozilla.org/manage/case/7201/
 https://moztrap.mozilla.org/manage/case/7203/
 https://moztrap.mozilla.org/manage/case/7205/

Comment 34

[bov]

Tested on Unagi device.
Build: user.manifest.V1-train.Rel0.4.Sprint12.B-290.Gecko-e78450a.Gaia-7c40bda (2013-07-09)
Commercial RIL: 152

 STEPS:
1. Send from another device to the DUT, or from the DUT send to another device an SMS including an invalid URL expression 
   a)"http://www.b/g/v.com/",
   b)"htt://www.google.com/", 
   c)"http://www.wikipedia.o/"
2. Open in the Device under Test the SMS APP
3. Search and tap on the SMS

EXPECTED RESULT:
1. y 2. The invalid URL expresions are shown in the same way that the rest of the content of the SMS thread (text).
3. Cannot open the Browser App clicking on the URL's

ACTUAL
It works as expected

Comment 35

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: af4e9d625324f824a10a08cfb46ef0e82e271a54