From bbcfe7619216a60c331164372157ad183a9ed653 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 8 Dec 2016 02:19:45 -0700 Subject: [PATCH 1/4] Hardened isNotURL / isURL code in urlutil.js Fixes https://github.com/brave/browser-laptop/issues/5911 (which was unintentionally introduced with https://github.com/brave/browser-laptop/issues/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"` --- js/lib/urlutil.js | 19 ++-- test/components/navigationBarTest.js | 84 ++++++++------ test/unit/lib/urlutilTest.js | 160 +++++++++++++++++---------- 3 files changed, 162 insertions(+), 101 deletions(-) diff --git a/js/lib/urlutil.js b/js/lib/urlutil.js index 8b5d2d7ac59..161c1ecd84e 100644 --- a/js/lib/urlutil.js +++ b/js/lib/urlutil.js @@ -99,29 +99,32 @@ const UrlUtil = { if (input === undefined || input === null) { return true } - + if (typeof input !== 'string') { + return true + } // for cases where we have scheme and we dont want spaces in domain names const caseDomain = /^[\w]{2,5}:\/\/[^\s\/]+\// // for cases, quoted strings const case1Reg = /^".*"$/ - // for cases, ?abc, "a? b", ".abc" and "abc." which should searching query - const case2Reg = /^(\?)|(\?.+\s)|^(\.)|(\.)$/ + // for cases: + // - starts with "?" or "." + // - contains "? " + // - ends with "." (and was not preceded by a /) + const case2Reg = /(^\?)|(\?.+\s)|(^\.)|(^[^\/]*\.$)/ // 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):.*/ let str = input.trim() - let scheme = this.getScheme(str) + const scheme = this.getScheme(str) if (str.toLowerCase() === 'localhost') { return false } - if (case1Reg.test(str)) { return true } - if (case2Reg.test(str) || !case3Reg.test(str) || (scheme === undefined && /\s/g.test(str))) { return true @@ -129,11 +132,9 @@ const UrlUtil = { if (case4Reg.test(str)) { return !this.canParseURL(str) } - if (scheme && (scheme !== 'file://')) { return !caseDomain.test(str + '/') } - str = this.prependScheme(str) return !this.canParseURL(str) }, diff --git a/test/components/navigationBarTest.js b/test/components/navigationBarTest.js index 265f249ac27..a05964778d2 100644 --- a/test/components/navigationBarTest.js +++ b/test/components/navigationBarTest.js @@ -885,44 +885,66 @@ describe('navigationBar tests', function () { }) describe('with url input value', function () { - Brave.beforeAll(this) - - before(function * () { - this.page1 = Brave.server.url('page1.html') + describe('with regards to the webview', function () { + Brave.beforeAll(this) + + before(function * () { + this.page1 = Brave.server.url('page1.html') + + yield setup(this.app.client) + // wait for the urlInput to be fully initialized + yield this.app.client.waitForExist(urlInput) + yield this.app.client.keys(this.page1) + // hit enter + yield this.app.client.keys(Brave.keys.ENTER) + }) - yield setup(this.app.client) - // wait for the urlInput to be fully initialized - yield this.app.client.waitForExist(urlInput) - yield this.app.client.keys(this.page1) - // hit enter - yield this.app.client.keys(Brave.keys.ENTER) - }) + it('webview has focus', function * () { + yield this.app.client.waitForElementFocus(activeWebview) + }) - it('webview has focus', function * () { - yield this.app.client.waitForElementFocus(activeWebview) - }) + it('webview loads url', function * () { + var page1 = this.page1 + yield this.app.client.waitUntil(function () { + return this.getAttribute(activeWebview, 'src').then((src) => src === page1) + }) + }) - it('webview loads url', function * () { - var page1 = this.page1 - yield this.app.client.waitUntil(function () { - return this.getAttribute(activeWebview, 'src').then((src) => src === page1) + it('urlbar shows webview url when focused', function * () { + var page1 = this.page1 + yield blur(this.app.client) + yield this.app.client.waitUntil(function () { + return this.isExisting(urlInput).then((exists) => exists === false) + }) + yield this.app.client + .ipcSend('shortcut-focus-url') + yield this.app.client.waitUntil(function () { + return this.getValue(urlInput).then((val) => val === page1) + }) + yield this.app.client.keys('abc') + yield this.app.client.waitUntil(function () { + return this.getValue(urlInput).then((val) => val === 'abc') + }) }) }) - it('urlbar shows webview url when focused', function * () { - var page1 = this.page1 - yield blur(this.app.client) - yield this.app.client.waitUntil(function () { - return this.isExisting(urlInput).then((exists) => exists === false) - }) - yield this.app.client - .ipcSend('shortcut-focus-url') - yield this.app.client.waitUntil(function () { - return this.getValue(urlInput).then((val) => val === page1) + describe('when following URLs', function () { + Brave.beforeEach(this) + + beforeEach(function * () { + yield setup(this.app.client) + yield this.app.client.waitForExist(urlInput) }) - yield this.app.client.keys('abc') - yield this.app.client.waitUntil(function () { - return this.getValue(urlInput).then((val) => val === 'abc') + + it('goes to the page (instead of search for the URL)', function * () { + const url = 'https://brave.com/page/cc?_ri_=3vv-8-e.' + yield this.app.client.keys(url) + yield this.app.client.keys(Brave.keys.ENTER) + yield this.app.client.waitUntil(function () { + return this.getValue(urlInput).then((val) => { + return val === url + }) + }) }) }) }) diff --git a/test/unit/lib/urlutilTest.js b/test/unit/lib/urlutilTest.js index 92f2d58a4a9..6fa18f90930 100644 --- a/test/unit/lib/urlutilTest.js +++ b/test/unit/lib/urlutilTest.js @@ -42,58 +42,106 @@ describe('urlutil', function () { }) describe('isNotURL', function () { - it('returns true when input is null', function () { - assert.equal(UrlUtil.isNotURL(null), true) - }) - it('returns true when input is undefined', function () { - assert.equal(UrlUtil.isNotURL(), true) - }) - it('returns false when input is "localhost"', function () { - assert.equal(UrlUtil.isNotURL('localhost'), false) - }) - it('returns true when input is a quoted string', function () { - assert.equal(UrlUtil.isNotURL('"brave.com"'), true) - }) - it('returns true when input is a pure string (no TLD)', function () { - assert.equal(UrlUtil.isNotURL('brave'), true) - }) - it('returns false when input is a string with whitespace but has schema', function () { - assert.equal(UrlUtil.isNotURL('https://wwww.brave.com/test space.jpg'), false) - }) - it('returns true when input is a string with schema but invalid domain name', function () { - assert.equal(UrlUtil.isNotURL('https://www.bra ve.com/test space.jpg'), true) - }) - it('returns true when input contains more than one word', function () { - assert.equal(UrlUtil.isNotURL('brave is cool'), true) - }) - it('returns false when input has custom protocol', function () { - assert.equal(UrlUtil.isNotURL('brave://test'), false) - }) - it('returns true when input has space in schema', function () { - assert.equal(UrlUtil.isNotURL('https ://brave.com'), true) - }) - it('returns false when input is chrome-extension', function () { - assert.equal(UrlUtil.isNotURL('chrome-extension://fmfcbgogabcbclcofgocippekhfcmgfj/cast_sender.js'), false) - }) - it('returns false when input is mailto', function () { - assert.equal(UrlUtil.isNotURL('mailto:brian@brave.com'), false) - }) - describe('search query', function () { - it('returns true when input starts with ?', function () { - assert.equal(UrlUtil.isNotURL('?brave'), true) + describe('returns false when input:', function () { + it('is a valid URL', function () { + assert.equal(UrlUtil.isNotURL('brave.com'), false) + }) + it('is an absolute file path without scheme', function () { + assert.equal(UrlUtil.isNotURL('/file/path/to/file'), false) + }) + it('is an absolute file path with scheme', function () { + assert.equal(UrlUtil.isNotURL('file:///file/path/to/file'), false) + }) + describe('for special pages', function () { + it('is a data URI', function () { + assert.equal(UrlUtil.isNotURL('data:text/html,hi'), false) + }) + it('is a view source URL', function () { + assert.equal(UrlUtil.isNotURL('view-source://url-here'), false) + }) + it('is a mailto link', function () { + assert.equal(UrlUtil.isNotURL('mailto:brian@brave.com'), false) + }) + it('is an about page', function () { + assert.equal(UrlUtil.isNotURL('about:preferences'), false) + }) + it('is a chrome-extension page', function () { + assert.equal(UrlUtil.isNotURL('chrome-extension://fmfcbgogabcbclcofgocippekhfcmgfj/cast_sender.js'), false) + }) + it('is a magnet URL', function () { + assert.equal(UrlUtil.isNotURL('magnet:?xt=urn:sha1:YNCKHTQCWBTRNJIV4WNAE52SJUQCZO5C'), false) + }) + }) + it('contains a hostname and port number', function () { + assert.equal(UrlUtil.isNotURL('someBraveServer:8089'), false) + }) + it('starts or ends with whitespace', function () { + assert.equal(UrlUtil.isNotURL(' http://brave.com '), false) + assert.equal(UrlUtil.isNotURL('\n\nhttp://brave.com\n\n'), false) + assert.equal(UrlUtil.isNotURL('\t\thttp://brave.com\t\t'), false) }) - it('returns true when input has a question mark followed by a space', function () { - assert.equal(UrlUtil.isNotURL('? brave'), true) + it('is a URL which contains basic auth user/pass', function () { + assert.equal(UrlUtil.isNotURL('http://username:password@example.com'), false) }) - it('returns true when input starts with .', function () { - assert.equal(UrlUtil.isNotURL('.brave'), true) + it('is localhost (case-insensitive)', function () { + assert.equal(UrlUtil.isNotURL('LoCaLhOsT'), false) }) - it('returns true when input end with .', function () { - assert.equal(UrlUtil.isNotURL('brave.'), true) + it('ends with period (input contains a forward slash)', function () { + assert.equal(UrlUtil.isNotURL('https://brave.com/test/cc?_ri_=3vv-8-e.'), false) + }) + it('is a string with whitespace but has schema', function () { + assert.equal(UrlUtil.isNotURL('https://wwww.brave.com/test space.jpg'), false) + }) + it('has custom protocol', function () { + assert.equal(UrlUtil.isNotURL('brave://test'), false) }) }) - it('returns false when input is a valid URL', function () { - assert.equal(UrlUtil.isNotURL('brave.com'), false) + + describe('returns true when input:', function () { + it('is null or undefined', function () { + assert.equal(UrlUtil.isNotURL(), true) + assert.equal(UrlUtil.isNotURL(null), true) + }) + it('is not a string', function () { + assert.equal(UrlUtil.isNotURL(false), true) + assert.equal(UrlUtil.isNotURL(333.449), true) + }) + it('is a quoted string', function () { + assert.equal(UrlUtil.isNotURL('"search term here"'), true) + }) + it('is a pure string (no TLD)', function () { + assert.equal(UrlUtil.isNotURL('brave'), true) + }) + describe('search query', function () { + it('starts with ?', function () { + assert.equal(UrlUtil.isNotURL('?brave'), true) + }) + it('has a question mark followed by a space', function () { + assert.equal(UrlUtil.isNotURL('? brave'), true) + }) + it('starts with .', function () { + assert.equal(UrlUtil.isNotURL('.brave'), true) + }) + it('ends with . (input does NOT contain a forward slash)', function () { + assert.equal(UrlUtil.isNotURL('brave.'), true) + }) + }) + it('is a string with schema but invalid domain name', function () { + assert.equal(UrlUtil.isNotURL('https://www.bra ve.com/test space.jpg'), true) + }) + it('contains more than one word', function () { + assert.equal(UrlUtil.isNotURL('brave is cool'), true) + }) + it('is not an about page / view source / data URI / mailto / etc', function () { + assert.equal(UrlUtil.isNotURL('not-a-chrome-extension:'), true) + assert.equal(UrlUtil.isNotURL('mailtoo:'), true) + }) + it('is a URL (without protocol) which contains basic auth user/pass', function () { + assert.equal(UrlUtil.isNotURL('username:password@example.com'), true) + }) + it('has space in schema', function () { + assert.equal(UrlUtil.isNotURL('https ://brave.com'), true) + }) }) }) @@ -110,20 +158,10 @@ describe('urlutil', function () { }) describe('isURL', function () { - it('absolute file path without scheme', function () { - assert.equal(UrlUtil.isURL('/file/path/to/file'), true) - }) - it('absolute file path with scheme', function () { - assert.equal(UrlUtil.isURL('file:///file/path/to/file'), true) - }) - it('detects data URI', function () { - assert.equal(UrlUtil.isURL('data:text/html,hi'), true) - }) - it('someBraveServer:8089', function () { - assert.equal(UrlUtil.isURL('someBraveServer:8089'), true) - }) - it('localhost', function () { - assert.equal(UrlUtil.isURL('localhost:8089'), true) + it('returns !isNotURL', function () { + assert.equal(UrlUtil.isURL('brave.com'), !UrlUtil.isNotURL('brave.com')) + assert.equal(UrlUtil.isURL('brave is cool'), !UrlUtil.isNotURL('brave is cool')) + assert.equal(UrlUtil.isURL('mailto:brian@brave.com'), !UrlUtil.isNotURL('mailto:brian@brave.com')) }) }) From cd71e0ec6681ae543bc70771bedc44852beea323 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Thu, 8 Dec 2016 21:48:07 +0800 Subject: [PATCH 2/4] fix end with period --- js/lib/urlutil.js | 4 ++-- test/unit/lib/urlutilTest.js | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/js/lib/urlutil.js b/js/lib/urlutil.js index 161c1ecd84e..dfda8da489c 100644 --- a/js/lib/urlutil.js +++ b/js/lib/urlutil.js @@ -109,8 +109,8 @@ const UrlUtil = { // for cases: // - starts with "?" or "." // - contains "? " - // - ends with "." (and was not preceded by a /) - const case2Reg = /(^\?)|(\?.+\s)|(^\.)|(^[^\/]*\.$)/ + // - ends with "." (and was not preceded by a domain or /) + const case2Reg = /(^\?)|(\?.+\s)|(^\.)|(^[^.+\..+]*[^\/]*\.$)/ // for cases, pure string const case3Reg = /[\?\.\/\s:]/ // for cases, data:uri, view-source:uri and about diff --git a/test/unit/lib/urlutilTest.js b/test/unit/lib/urlutilTest.js index 6fa18f90930..da122fed5df 100644 --- a/test/unit/lib/urlutilTest.js +++ b/test/unit/lib/urlutilTest.js @@ -86,8 +86,11 @@ describe('urlutil', function () { it('is localhost (case-insensitive)', function () { assert.equal(UrlUtil.isNotURL('LoCaLhOsT'), false) }) - it('ends with period (input contains a forward slash)', function () { - assert.equal(UrlUtil.isNotURL('https://brave.com/test/cc?_ri_=3vv-8-e.'), false) + it('ends with period (input contains a forward slash and domain)', function () { + 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) }) it('is a string with whitespace but has schema', function () { assert.equal(UrlUtil.isNotURL('https://wwww.brave.com/test space.jpg'), false) From 534dc14bd1724947d8d2483d994863a9942a6c4e Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Thu, 8 Dec 2016 22:32:58 +0800 Subject: [PATCH 3/4] move ends with period test == true to correct position --- test/unit/lib/urlutilTest.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/lib/urlutilTest.js b/test/unit/lib/urlutilTest.js index da122fed5df..5a0a0c20873 100644 --- a/test/unit/lib/urlutilTest.js +++ b/test/unit/lib/urlutilTest.js @@ -89,9 +89,6 @@ describe('urlutil', function () { it('ends with period (input contains a forward slash and domain)', function () { 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) - }) it('is a string with whitespace but has schema', function () { assert.equal(UrlUtil.isNotURL('https://wwww.brave.com/test space.jpg'), false) }) @@ -128,6 +125,9 @@ describe('urlutil', function () { it('ends with . (input does NOT contain a forward slash)', function () { assert.equal(UrlUtil.isNotURL('brave.'), true) }) + it('ends with period (input contains only a forward slash)', function () { + assert.equal(UrlUtil.isNotURL('brave/com/test/cc?_ri_=3vv-8-e.'), true) + }) }) it('is a string with schema but invalid domain name', function () { assert.equal(UrlUtil.isNotURL('https://www.bra ve.com/test space.jpg'), true) From cd88861cfa29f5e369001386b459ee0fe949cc57 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 8 Dec 2016 08:04:23 -0700 Subject: [PATCH 4/4] Added example use-case for local network --- test/unit/lib/urlutilTest.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/unit/lib/urlutilTest.js b/test/unit/lib/urlutilTest.js index 5a0a0c20873..e28035f7c60 100644 --- a/test/unit/lib/urlutilTest.js +++ b/test/unit/lib/urlutilTest.js @@ -86,6 +86,9 @@ describe('urlutil', function () { it('is localhost (case-insensitive)', function () { assert.equal(UrlUtil.isNotURL('LoCaLhOsT'), false) }) + it('is a hostname (not a domain)', function () { + assert.equal(UrlUtil.isNotURL('http://computer001/phpMyAdmin'), false) + }) it('ends with period (input contains a forward slash and domain)', function () { assert.equal(UrlUtil.isNotURL('brave.com/test/cc?_ri_=3vv-8-e.'), false) })