Hardened isNotURL / isURL code in urlutil.js #6086

bsclifton · 2016-12-08T09:39:09Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Fixes #5911
(which was unintentionally introduced with #4546)

Also fixes these cases (no issues created yet):

view-source/mail-to/etc were not checking from beginning of string
ensures input is string (preventing failing call to trim())

And adds missing tests for:

checking localhost toLowerCase()
checking basic auth formatted URI (with and without protocol)
is input a string
each type of special page
partial bad matches on special pages

Also updates tests for isURL to ensure value is always negated version of isNotURL

Auditors

@bbondy, @darkdh, @diracdeltas

Since this code has a very large potential impact (everything from the URL bar calls this for example), please review and help make sure I covered everything safely :)

Unit test

npm run unittest -- --grep="urlutil"

Webdriver test

# window 1
npm run watch-test
#window 2
npm run uitest -- --grep="when following URLs"

@bbondy

Fixes #5911 (which was unintentionally introduced with #4546) Also fixes these cases (no issues created yet): - view-source/mail-to/etc were not checking from beginning of string - ensures input is string (preventing failing call to trim()) And adds missing tests for: - checking localhost toLowerCase() - checking basic auth formatted URI (with and without protocol) - is input a string - each type of special page - partial bad matches on special pages Also updates tests for isURL to ensure value is always negated version of isNotURL Auditors: @bbondy, @darkdh, @diracdeltas Since this code has a very large potential impact (everything from the URL bar calls this for example), please review and help make sure I covered everything safely :) Unit test: `npm run unittest -- --grep="urlutil" Webdriver test: `npm run uitest -- --grep="when following URLs"`

darkdh · 2016-12-08T13:16:14Z

js/lib/urlutil.js

+    // for cases:
+    // - starts with "?" or "."
+    // - contains "? "
+    // - ends with "." (and was not preceded by a /)


type 123/123. will be search string on other browsers

fixed in cd71e0e and test is also covered. Mainly changing logic of end with period, which is
brave.com/123. will be url and brave/com/123. will not

bsclifton · 2016-12-08T14:58:25Z

test/unit/lib/urlutilTest.js

+        assert.equal(UrlUtil.isNotURL('brave.com/test/cc?_ri_=3vv-8-e.'), false)
+      })
+      it('ends with period (input contains only a forward slash)', function () {
+        assert.equal(UrlUtil.isNotURL('brave/com/test/cc?_ri_=3vv-8-e.'), true)


Testing for an extra . might not be a good idea, because folks may use Brave to browse their intranet. It's not a best practice, but many folks will setup a host file entry and access hosts by computer name (instead of domain)

ex: http://computer001/phpMyAdmin/

actually- I think this behavior might be OK! I am almost positive you are correct about the other behavior (because it's happened to me so many times 😛 ) When prefixed with protocol, it validates just fine: cd88861

diracdeltas · 2016-12-08T19:55:15Z

i'm not sure why we are using regexes and window.URL in urlutil at all. seems like you should just be able to do urlParse and conclude the URL is invalid if protocol and host are null.

bbondy · 2016-12-08T20:04:41Z

@diracdeltas

i'm not sure why we are using regexes and window.URL in urlutil at all. seems like you should just be able to do urlParse and conclude the URL is invalid if protocol and host are null.

I didn't write this original code but really it's more about heuristics about if a user meant to type a url or not. For example require('url').parse('a b') will give output of no protocol but will give pathname, path and href. The user means a search in that case.

For example require('url').parse('www.test.com') will give the same output types. And in that case it's meant to be a URL. So this function is more about what the user intended for it to be, and not whether it can be considered a URL or not.

bbondy · 2016-12-08T20:05:50Z

The second example has host not filled in by the way

bbondy

This is better tested than what we had before and works better so even if we refactored it later to use urlParse in some other way we can still at that point keep the tests and change the implementation. So merging.

bbondy · 2016-12-08T20:06:29Z

js/lib/urlutil.js

    // for cases, pure string
    const case3Reg = /[\?\.\/\s:]/
    // for cases, data:uri, view-source:uri and about
-    const case4Reg = /^data:|view-source:|mailto:|about:|chrome-extension:|magnet:.*/
+    const case4Reg = /^(data|view-source|mailto|about|chrome-extension|magnet):.*/


diracdeltas · 2016-12-08T20:12:28Z

@bbondy i figured, but i was hoping we could just do urlParse after normalizing the input; i will open a separate issue

bsclifton added bug feature/URLbar labels Dec 8, 2016

bsclifton added this to the 0.13.0 milestone Dec 8, 2016

bsclifton assigned diracdeltas, bbondy and darkdh Dec 8, 2016

darkdh reviewed Dec 8, 2016

View reviewed changes

darkdh added 2 commits December 8, 2016 21:48

fix end with period

cd71e0e

move ends with period test == true to correct position

534dc14

bsclifton commented Dec 8, 2016

View reviewed changes

Added example use-case for local network

cd88861

bbondy approved these changes Dec 8, 2016

View reviewed changes

bbondy merged commit 163f1e3 into brave:master Dec 8, 2016

bsclifton deleted the email-link-fixup branch December 8, 2016 20:36

luixxiul added the QA/no-qa-needed label Dec 21, 2016

bsclifton mentioned this pull request Dec 31, 2016

Embedded links in emails treated as Search terms, not Links #6438

Closed

cndouglas mentioned this pull request Jan 29, 2017

Multiple home page URLs stopped working - "Your file was not found" #6913

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hardened isNotURL / isURL code in urlutil.js #6086

Hardened isNotURL / isURL code in urlutil.js #6086

bsclifton commented Dec 8, 2016

darkdh Dec 8, 2016

darkdh Dec 8, 2016

bsclifton Dec 8, 2016 •

edited

Loading

bsclifton Dec 8, 2016

diracdeltas commented Dec 8, 2016

bbondy commented Dec 8, 2016

bbondy commented Dec 8, 2016

bbondy left a comment

bbondy Dec 8, 2016

bbondy Dec 8, 2016

diracdeltas commented Dec 8, 2016

Hardened isNotURL / isURL code in urlutil.js #6086

Hardened isNotURL / isURL code in urlutil.js #6086

Conversation

bsclifton commented Dec 8, 2016

Auditors

Unit test

Webdriver test

darkdh Dec 8, 2016

Choose a reason for hiding this comment

darkdh Dec 8, 2016

Choose a reason for hiding this comment

bsclifton Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

bsclifton Dec 8, 2016

Choose a reason for hiding this comment

diracdeltas commented Dec 8, 2016

bbondy commented Dec 8, 2016

bbondy commented Dec 8, 2016

bbondy left a comment

Choose a reason for hiding this comment

bbondy Dec 8, 2016

Choose a reason for hiding this comment

bbondy Dec 8, 2016

Choose a reason for hiding this comment

diracdeltas commented Dec 8, 2016

bsclifton Dec 8, 2016 •

edited

Loading