Closed Bug 870711 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
Windows 7
defect

Tracking

(blocking-b2g:leo+, b2g18+ verified, b2g-v1.1hd fixed)

VERIFIED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
b2g18 + verified
b2g-v1.1hd --- fixed

People

(Reporter: bov, Assigned: gnarf)

References

Details

(Whiteboard: leorun1, c=, leorun3, leorun4,[leo-triage] ,[LeoVB+] )

Attachments

(1 file, 1 obsolete file)

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.
Nice to have polish but not a blocker for shipping.
blocking-b2g: leo? → -
tracking-b2g18: --- → +
Whiteboard: leorun1 → leorun1, c=
While dogfooding I've had messages with ellipses (...) turned into URLs.  Here are some actual examples:

  "no...not"
  "sleepy...and"
  "crutches...be"
Duplicate of this bug: 874719
As I did in the dup...

Requesting leo because the user experience is incredibly poor
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/
Whiteboard: leorun1, c= → leorun1, c=, leorun3
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.
(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.
Rha turns out it fails on the wikipedia.o example.
(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.
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
Also link to failed test case:
https://moztrap.mozilla.org/manage/case/7187/
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.
Attachment #762665 - Flags: review?(felash)
Assignee: nobody → lissyx+mozillians
Status: NEW → ASSIGNED
Comment on attachment 762665 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10396

redirecting to Corey who's free this morning ;)
Attachment #762665 - Flags: review?(felash) → review?(gnarf37)
Attachment #762665 - Flags: review?(gnarf37) → review-
Comments on pull request - please fix lint & units
(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.
Attachment #762665 - Flags: review- → review?(gnarf37)
Comment on attachment 762665 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10396

Still a lot of unit tests failing...
Attachment #762665 - Flags: review?(gnarf37) → review-
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.
Attachment #762665 - Flags: review- → review?(gnarf37)
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
Attachment #762665 - Flags: review?(gnarf37)
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?
Flags: needinfo?(pyang)
Keywords: qawanted
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
Flags: needinfo?(pyang)
 - 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
Keywords: qawanted
(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.
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
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
Which I must add - bugzilla can't ;)
I'd like to take the work started here and base my work on bug 887737 on it - requesting leo+
blocking-b2g: - → leo?
blocking a blocker then.

Corey, please request feedback on Alexandre :)
Assignee: lissyx+mozillians → gnarf37
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
Attachment #762665 - Attachment is obsolete: true
blocking-b2g: leo? → leo+
Priority: -- → P1
Target Milestone: --- → 1.1 QE4 (15jul)
Attached file pull request on github
Please let me merge this myself
Attachment #770089 - Flags: review?(felash)
Attachment #770089 - Flags: feedback?(lissyx+mozillians)
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 ;)
Attachment #770089 - Flags: review?(felash) → review+
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
Attachment #770089 - Flags: feedback?(lissyx+mozillians)
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/
Whiteboard: leorun1, c=, leorun3 → leorun1, c=, leorun3, leorun4
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
Status: RESOLVED → VERIFIED
blocking-b2g: leo+ → leo?
Whiteboard: leorun1, c=, leorun3, leorun4 → leorun1, c=, leorun3, leorun4,[leo-triage]
blocking-b2g: leo? → leo+
Whiteboard: leorun1, c=, leorun3, leorun4,[leo-triage] → leorun1, c=, leorun3, leorun4,[leo-triage] ,[LeoVB+]
v1.1.0hd: af4e9d625324f824a10a08cfb46ef0e82e271a54
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.