941763 - [B2G][SMS] Sent and recieved SMS containing an invalid URL like ".Website.com" are highlighted and function as hyperlinks

Status: RESOLVED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot

Description:
Sent and recieved messages containing a specific type of invalid URL ("."+"alphanumeric characters"+".com") are highlighted and function as hyperlinks (leading to a "Server not found" page).

Repro Steps:
1) Updated Buri to Build ID: 20131120004000
2) Have User A open Messages app
3) Let User A send an SMS message to User B containing invalid URL ".com.com"
4) Have User B open recieved message from User A

Actual:
Invalid URLs in SMS messages containing a "." before the website address are treated as valid URLs.

Expected:
Invalid URLs in SMS messages are not highlighted and do not function as hyperlinks.

Environmental Variables
Device: Buri v 1.2 COM RIL
Build ID: 20131120004000
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/2d454e0de2ed
Gaia: 5ec2963fff60492c840707df8d8090f9908a5251
Platform Version: 26.0
RIL Version: 01.02.00.019.102 
Firmware Version: V1.2_US_20131115

Notes:
Repro frequency: 3/3, 100%
Link to failed test case: https://moztrap.mozilla.org/manage/case/7186/
See attached: screenshots

Comment 1

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot

Comment 2

[Brogan Zumwalt [Inactive]]

Able to reproduce on 1.1

Result: Invalid URLs in SMS messages containing a "." before the website address are treated as valid URLs.

Environmental Variables
Device: Leo v 1.1 COM RIL)
Build ID: 20131119041201
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/7c3cfc0936ca
Gaia: b585b32441fafa67f2b4582db23be5f3a2afab21
Platform Version: 18.1
RIL Version: 01.01.00.019.281 
Firmware Version: D300F10a

Comment 3

[Brogan Zumwalt [Inactive]]

This issue also occurs when message contains invalid URLs like "anything.anything" regardless of whether domain is valid or not (e.g. google.commmmmmmmm)

Comment 4

[Julien Wajsberg [:julienw]]

The "google.commmmmmmm" case was fixed in bug 923739, that is not in 1.2.

I'm not sure for the ".Website.com" case though. Can you please try your cases on 1.3?

Comment 5

[Brogan Zumwalt [Inactive]]

Issue reproduces on 1.3

Result: Invalid URLs in SMS messages containing a "." before the website address are treated as valid URLs. (Note: Tried sending and recieving ".cnn.com" and ".google.com"; Both highlighted with underline starting under first ".", both invalid URLs function as hyperlinks.)

Environmental Variables
Device: Buri v 1.3 Mozilla RIL
Build ID: 20131121040202
Gecko: http://hg.mozilla.org/mozilla-central/rev/cf378dddfac8
Gaia: 71063dd91bc8cbb15ba335236ed67a1c5058bd58
Platform Version: 28.0a1
RIL Version: 01.02.00.019.102 
Firmware Version: V1.2_US_20131115

Comment 6

[Julien Wajsberg [:julienw]]

Ok thanks.

Should be easy to fix for a contributor!

Comment 7

[Julien Wajsberg [:julienw]]

The code is in apps/sms/js/link_helper.js, and it's probably a lot easier to fix this by adding first a failing testcase in apps/sms/test/unit/link_helper_test.js.

Comment 8

[Robert Patrick Damery]

I'll take this bug if no one has taken it yet!

Comment 9

[Robert Patrick Damery]

Attachment: Patch for invalid url detection

I used the same copy of gaia I was using for Bug 941213 so my changes for that patch appear in this one, if this is a problem let me know and I'll apply my changes to a fresh copy of gaia!

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8340446 [details] [diff] [review]
Patch for invalid url detection

Hey Robert,

please provide a separate pull request containing only the relevant commit, then ask me again a review.

You can easily do that by using the comment "git rebase -i master", and removing the line that picks the other commit.

Thanks!

Comment 11

[Robert Patrick Damery]

Attachment: Patch for invalid url detection

I saw that travis seemed to complain over comparing a variable to an empty string, which is why I made the second commit where I used the length of the variable instead. Let me know if there is anything else I should do + I would really appreciate it if you might point me in the direction of some information about make hint

Comment 12

[Robert Patrick Damery]

Attachment: Updated patch

Sorry for the delay in this guys. I had exams so I was up the walls studying for them for the last couple of weeks!

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8349115 [details] [review]
Updated patch

No problem for the exams, there is nothing urgent here :)

I left comments on github, tell me what you think!

Comment 14

[Julien Wajsberg [:julienw]]

Robert Patrick, are you still working on this?

Thanks!

Comment 15

[Robert Patrick Damery]

I could have sworn I had seen someone else take over this. I can finish this if you want just need to reinstall everything and make sure it's still compatible with the current version of gaia, I can also finish off the other bug if you want me to?

Comment 16

[Julien Wajsberg [:julienw]]

It's fine if you want to stop working on one of them or even both :) I just want to get this bug fixed, if it's not by you, it would be by someone else :)

Comment 17

[Robert Patrick Damery]

if I remember correctly I had fixed both bugs already, the only problem was indentation. I'll rebase gaia and check if the fixes still work, if they work I'll correct the indentation and upload them :) Can't really do it right now though, have a dead line in college for tomorrow so I'm busy finishing up my project!

Comment 18

[kumar rishav (:rishav_)]

Hey Robert, still working on it?

Comment 19

[kumar rishav (:rishav_)]

No update from assignee.
settings to nobody.

Vishnu, check this.

Comment 20

[Vishnu Teja [:ythej]]

Hi Julien, 

I can see that the problem comes with only "." + "[a-z]*" + "." + "com" . I see that "." + "[a-z]*" + "." + "commmm" are working fine ! Please check this.

Thank you.

Comment 21

[Julien Wajsberg [:julienw]]

yeah, that's what I said in comment 4 :) Thanks for changing the title !

Comment 22

[GitHub Autolander Bot]

Attachment: [gaia] ythej:bug-941763 > mozilla-b2g:master

Comment 23

[Julien Wajsberg [:julienw]]

Hey Vishnu,

please put clearly what you expect from me :)
Thanks !

Comment 24

[Vishnu Teja [:ythej]]

(In reply to Julien Wajsberg [:julienw] from comment #23)
> Hey Vishnu,
> 
> please put clearly what you expect from me :)
> Thanks !

Hey Julien, sorry, I thought it's enough as Autolander has created a comment ! It was nice without Autolander !! :P

I fixed the bug and created a PR but I don't if the fix is proper. I fixed it by adding a statement in matchFilter. Please let me know if I need to change it !

Thanks a lot ! :)

Comment 25

[Julien Wajsberg [:julienw]]

Ah so what you really want is a feedback :) next time please use a "feedback?" flag on the attachment :)

Comment 26

[Julien Wajsberg [:julienw]]

answered on github.

This was working for the specific case here, but not for more general cases. So I added a suggestion. Tell me what you think !

Comment 27

[Vishnu Teja [:ythej]]

Correction in Expected result:

.webapp.com should link webapp.com

Thank you !

Comment 28

[Julien Wajsberg [:julienw]]

Comment on attachment 8675437 [details] [review]
[gaia] ythej:bug-941763 > mozilla-b2g:master

let's try the regexp first ;)

Comment 29

[Vishnu Teja [:ythej]]

Comment on attachment 8675437 [details] [review]
[gaia] ythej:bug-941763 > mozilla-b2g:master

Hi Julien,

Changed the RegEx and all the tests are working fine !! I will merge the commit's after the review.

Thank you :)

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 8675437 [details] [review]
[gaia] ythej:bug-941763 > mozilla-b2g:master

As we discussed before, I think we want to link anything after the initial . ? So maybe we should include the '.' character in safeStart ?

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 8675437 [details] [review]
[gaia] ythej:bug-941763 > mozilla-b2g:master

Please ping me when the tests are green ;)

Comment 32

[Julien Wajsberg [:julienw]]

But otherwise the code seems to work fine.

Comment 33

[Julien Wajsberg [:julienw]]

Comment on attachment 8675437 [details] [review]
[gaia] ythej:bug-941763 > mozilla-b2g:master

Hey Steve, do you think you can do the final review here ? In the previous review the code was fine in my tests but unit tests were missing.

Comment 34

[Steve Chung [:steveck]]

Comment on attachment 8675437 [details] [review]
[gaia] ythej:bug-941763 > mozilla-b2g:master

Looks fine. Although I have some concern about the possible phishing like https://.website.com case, it's still better than current regex. Please polish the unit test tittle and squash into 1 commit, thanks!

Comment 35

[Vishnu Teja [:ythej]]

Hi Steve,

Changed the title and squashed the commits.

Thanks for the review! :)

Comment 36

[Steve Chung [:steveck]]

(In reply to Vishnu Teja [:ythej] from comment #35)
> Hi Steve,
> 
> Changed the title and squashed the commits.
> 
> Thanks for the review! :)

I left a comment on github and let me know which method you prefer most(and thanks for the suggestion from Kumar:) )

Comment 37

[Vishnu Teja [:ythej]]

Replied on github :)

Comment 38

[Steve Chung [:steveck]]

(In reply to Vishnu Teja [:ythej] from comment #37)
> Replied on github :)

So please file another bug for the polishing :)
Master: https://github.com/mozilla-b2g/gaia/commit/72e5606932f6e6fa0c5fa000c26eb35f9c2c1c79

Comment 39

[Vishnu Teja [:ythej]]

Hi Steve,

I filed a new one! :)

Bug 1235134

I believe it's a good first bug and it would be better if we add mentor to it?

Thank you :)

Comment 40

[Steve Chung [:steveck]]

(In reply to Vishnu Teja [:ythej] from comment #39)
> Hi Steve,
> 
> I filed a new one! :)
> 
> Bug 1235134
> 
> I believe it's a good first bug and it would be better if we add mentor to
> it?

Yap and you can add any peer as the mentor in Bug 1235134 since it's not complicated. Thanks for filing the bug!