Closed Bug 941763 Opened 8 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g18 affected, b2g-v1.2 affected)

RESOLVED FIXED
Tracking Status
b2g18 --- affected
b2g-v1.2 --- affected

People

(Reporter: bzumwalt, Assigned: ythej, Mentored)

Details

(Whiteboard: burirun4)

Attachments

(4 files, 2 obsolete files)

Attached image 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
Attached image Screenshot
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
Whiteboard: burirun4 → [mentor=:julienw] burirun4
This issue also occurs when message contains invalid URLs like "anything.anything" regardless of whether domain is valid or not (e.g. google.commmmmmmmm)
Summary: [B2G][SMS] Sent and recieved SMS containing an invalid URL like ".Website.com" are highlighted and function as hyperlinks → [B2G][SMS] Sent and recieved SMS containing an invalid URL like ".Website.com" and "Website.commmmm" are highlighted and function as hyperlinks
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?
Flags: needinfo?(bzumwalt)
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
Flags: needinfo?(bzumwalt)
Ok thanks.

Should be easy to fix for a contributor!
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.
I'll take this bug if no one has taken it yet!
Assignee: nobody → patrick
Attached patch Patch for invalid url detection (obsolete) — Splinter Review
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!
Attachment #8340446 - Flags: review?(felash)
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!
Attachment #8340446 - Flags: review?(felash)
Attached file Patch for invalid url detection (obsolete) —
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
Attachment #8340446 - Attachment is obsolete: true
Attachment #8340797 - Flags: review?(waldron.rick)
Attached file 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!
Attachment #8340797 - Attachment is obsolete: true
Attachment #8340797 - Flags: review?(waldron.rick)
Attachment #8349115 - Flags: review?(waldron.rick)
Attachment #8349115 - Flags: review?(felash)
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!
Attachment #8349115 - Flags: review?(waldron.rick)
Attachment #8349115 - Flags: review?(felash)
Mentor: felash
Whiteboard: [mentor=:julienw] burirun4 → burirun4
Robert Patrick, are you still working on this?

Thanks!
Flags: needinfo?(patrick)
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?
Flags: needinfo?(patrick)
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 :)
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!
Hey Robert, still working on it?
Flags: needinfo?(patrick)
No update from assignee.
settings to nobody.

Vishnu, check this.
Assignee: patrick → nobody
Flags: needinfo?(yvtheja)
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.
Flags: needinfo?(felash)
Summary: [B2G][SMS] Sent and recieved SMS containing an invalid URL like ".Website.com" and "Website.commmmm" are highlighted and function as hyperlinks → [B2G][SMS] Sent and recieved SMS containing an invalid URL like ".Website.com" are highlighted and function as hyperlinks
yeah, that's what I said in comment 4 :) Thanks for changing the title !
Flags: needinfo?(felash)
Flags: needinfo?(yvtheja) → needinfo?(felash)
Hey Vishnu,

please put clearly what you expect from me :)
Thanks !
Flags: needinfo?(felash)
(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 ! :)
Flags: needinfo?(felash)
Ah so what you really want is a feedback :) next time please use a "feedback?" flag on the attachment :)
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 !
Flags: needinfo?(felash)
Flags: needinfo?(patrick)
Assignee: nobody → yvtheja
Correction in Expected result:

.webapp.com should link webapp.com

Thank you !
Attachment #8675437 - Flags: feedback?(felash)
Comment on attachment 8675437 [details] [review]
[gaia] ythej:bug-941763 > mozilla-b2g:master

let's try the regexp first ;)
Attachment #8675437 - Flags: feedback?(felash)
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 :)
Attachment #8675437 - Flags: review?(felash)
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 ?
Attachment #8675437 - Flags: review?(felash) → feedback+
Attachment #8675437 - Flags: review?(felash)
Comment on attachment 8675437 [details] [review]
[gaia] ythej:bug-941763 > mozilla-b2g:master

Please ping me when the tests are green ;)
Attachment #8675437 - Flags: review?(felash)
But otherwise the code seems to work fine.
Attachment #8675437 - Flags: review?(felash)
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.
Attachment #8675437 - Flags: review?(felash) → review?(schung)
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!
Attachment #8675437 - Flags: review?(schung) → review+
Hi Steve,

Changed the title and squashed the commits.

Thanks for the review! :)
Flags: needinfo?(schung)
(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:) )
Flags: needinfo?(schung)
Replied on github :)
Flags: needinfo?(schung)
(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
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(schung)
Resolution: --- → FIXED
Flags: needinfo?(yvtheja)
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 :)
Flags: needinfo?(yvtheja) → needinfo?(schung)
(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!
Flags: needinfo?(schung)
You need to log in before you can comment on or make changes to this bug.