Add validation to siteUtil.updateSiteFavicon (includes tests for each…

… case)

Properly fixes #4860

Was discovered by @diracdeltas
#5445 (comment)

Auditors: @diracdeltas

Test Plan:
1. Launch Brave and go to bing.com
2. Open a new tab and type "bi" and let autocomplete show "bing.com"
3. Hit enter... and it no longer crashes :)

js/state/siteUtil.js

@@ -7,6 +7,7 @@ const normalizeUrl = require('normalize-url')
 const siteTags = require('../constants/siteTags')
 const settings = require('../constants/settings')
 const getSetting = require('../settings').getSetting
+const UrlUtil = require('../lib/urlutil')
 const urlParse = require('url').parse
 
 const isBookmark = (tags) => {
@@ -337,9 +338,19 @@ module.exports.getDetailFromFrame = function (frame, tag) {
  * @param favicon favicon URL
  */
 module.exports.updateSiteFavicon = function (sites, location, favicon) {
+  if (UrlUtil.isNotURL(location)) {
+    return sites
+  }
+
   const matchingIndices = []
 
   sites.filter((site, index) => {
+    if (isBookmarkFolder(site.get('tags'))) {
+      return false
+    }
+    if (UrlUtil.isNotURL(site.get('location'))) {
+      return false
+    }
     if (normalizeUrl(site.get('location')) === normalizeUrl(location)) {
       matchingIndices.push(index)
       return true

test/unit/common/state/aboutNewTabStateTest.js

@@ -56,6 +56,14 @@ describe('aboutNewTabState', function () {
     },
     tag: siteTags.BOOKMARK
   }
+  const historyAction = {
+    siteDetail: {
+      title: 'Brave',
+      location: 'https://brave.com',
+      lastAccessedTime: testTime
+    },
+    tag: undefined
+  }
 
   describe('mergeDetails', function () {
     it('updates the `updatedStamp` value on success', function () {
@@ -106,12 +114,6 @@ describe('aboutNewTabState', function () {
       assert.equal(updatedValue, bookmarkAction.siteDetail.location)
     })
 
-    it('removes the tags when adding to the sites list', function () {
-      const state = aboutNewTabState.addSite(defaultAppState, bookmarkAction)
-      const updatedValue = state.getIn(['about', 'newtab', 'sites', 0, 'tags'])
-      assert.deepEqual(updatedValue.toJS(), [])
-    })
-
     it('will add lastAccessedTime to the siteDetail if missing from history entry', function () {
       const action = {siteDetail: {location: 'https://brave.com'}}
       const state = aboutNewTabState.addSite(defaultAppState, action)
@@ -145,10 +147,10 @@ describe('aboutNewTabState', function () {
     })
 
     it('removes the entry from the sites list', function () {
-      const stateWithSite = aboutNewTabState.addSite(defaultAppState, bookmarkAction)
+      const stateWithSite = aboutNewTabState.addSite(defaultAppState, historyAction)
       assert.equal(stateWithSite.size, 1)
 
-      const state = aboutNewTabState.removeSite(stateWithSite, bookmarkAction)
+      const state = aboutNewTabState.removeSite(stateWithSite, historyAction)
       const sites = state.getIn(['about', 'newtab', 'sites'])
       assert.equal(sites.size, 0)
     })

test/unit/state/siteUtilTest.js

@@ -479,25 +479,45 @@ describe('siteUtil', function () {
 
   describe('updateSiteFavicon', function () {
     it('updates the favicon for all matching entries', function () {
-      const siteDetail1 = Immutable.fromJS({
-        tags: [siteTags.BOOKMARK],
-        location: testUrl1,
-        title: 'bookmarked site'
-      })
-      const siteDetail2 = Immutable.fromJS({
-        tags: [],
-        location: testUrl1,
-        title: 'visited site'
-      })
-      const sites = Immutable.fromJS([siteDetail1, siteDetail2])
+      const sites = Immutable.fromJS([bookmarkMinFields, siteMinFields])
       const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, 'https://brave.com/favicon.ico')
-      const updatedSiteDetail1 = siteDetail1.set('favicon', 'https://brave.com/favicon.ico')
-      const updatedSiteDetail2 = siteDetail2.set('favicon', 'https://brave.com/favicon.ico')
+      const updatedSiteDetail1 = bookmarkMinFields.set('favicon', 'https://brave.com/favicon.ico')
+      const updatedSiteDetail2 = siteMinFields.set('favicon', 'https://brave.com/favicon.ico')
       const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2])
 
       assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
     })
 
+    describe('when searching for matches', function () {
+      it('disregards folders', function () {
+        const sites = siteUtil.addSite(emptySites, folderMinFields)
+        const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, 'https://brave.com/favicon.ico')
+        assert.deepEqual(processedSites.toJS(), sites.toJS())
+      })
+      it('ensures entry.location is truthy', function () {
+        const invalidSite = Immutable.fromJS({
+          title: 'sample'
+        })
+        const sites = siteUtil.addSite(emptySites, invalidSite)
+        const processedSites = siteUtil.updateSiteFavicon(sites, testUrl1, 'https://brave.com/favicon.ico')
+        assert.deepEqual(processedSites.toJS(), sites.toJS())
+      })
+      it('ensures input and entry.location are valid URLs', function () {
+        const invalidSite = Immutable.fromJS({
+          title: 'sample',
+          location: '......not a real URL'
+        })
+        const sites = siteUtil.addSite(emptySites, invalidSite)
+        const processedSites = siteUtil.updateSiteFavicon(sites, '......not a real URL', 'https://brave.com/favicon.ico')
+        assert.deepEqual(processedSites.toJS(), sites.toJS())
+      })
+      it('ensures input is truthy', function () {
+        const sites = siteUtil.addSite(emptySites, bookmarkMinFields)
+        const processedSites = siteUtil.updateSiteFavicon(sites, undefined, 'https://brave.com/favicon.ico')
+        assert.deepEqual(processedSites.toJS(), sites.toJS())
+      })
+    })
+
     describe('normalizes the URL when searching for matches', function () {
       it('normalizes trailing slashes', function () {
         const siteDetail1 = Immutable.fromJS({
@@ -526,16 +546,11 @@ describe('siteUtil', function () {
           location: 'https://brave.com:443',
           title: 'bookmarked site'
         })
-        const siteDetail2 = Immutable.fromJS({
-          tags: [],
-          location: 'https://brave.com/',
-          title: 'visited site'
-        })
 
-        const sites = Immutable.fromJS([siteDetail1, siteDetail2])
+        const sites = Immutable.fromJS([siteDetail1, siteMinFields])
         const processedSites = siteUtil.updateSiteFavicon(sites, 'https://brave.com/', 'https://brave.com/favicon.ico')
         const updatedSiteDetail1 = siteDetail1.set('favicon', 'https://brave.com/favicon.ico')
-        const updatedSiteDetail2 = siteDetail2.set('favicon', 'https://brave.com/favicon.ico')
+        const updatedSiteDetail2 = siteMinFields.set('favicon', 'https://brave.com/favicon.ico')
         const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2])
 
         assert.deepEqual(processedSites.toJS(), expectedSites.toJS())
@@ -547,16 +562,11 @@ describe('siteUtil', function () {
           location: 'https://www.brave.com/',
           title: 'bookmarked site'
         })
-        const siteDetail2 = Immutable.fromJS({
-          tags: [],
-          location: 'https://brave.com/',
-          title: 'visited site'
-        })
 
-        const sites = Immutable.fromJS([siteDetail1, siteDetail2])
+        const sites = Immutable.fromJS([siteDetail1, siteMinFields])
         const processedSites = siteUtil.updateSiteFavicon(sites, 'https://brave.com/', 'https://brave.com/favicon.ico')
         const updatedSiteDetail1 = siteDetail1.set('favicon', 'https://brave.com/favicon.ico')
-        const updatedSiteDetail2 = siteDetail2.set('favicon', 'https://brave.com/favicon.ico')
+        const updatedSiteDetail2 = siteMinFields.set('favicon', 'https://brave.com/favicon.ico')
         const expectedSites = Immutable.fromJS([updatedSiteDetail1, updatedSiteDetail2])
 
         assert.deepEqual(processedSites.toJS(), expectedSites.toJS())