Merge pull request #14286 from Slava/fix-14250

Strip cookies from requests fetch favicons

app/filtering.js

@@ -18,6 +18,7 @@ const urlParse = require('./common/urlParse')
 const getSetting = require('../js/settings').getSetting
 const {getExtensionsPath} = require('../js/lib/appUrlUtil')
 const appUrlUtil = require('../js/lib/appUrlUtil')
+const faviconUtil = require('../js/lib/faviconUtil')
 const siteSettings = require('../js/state/siteSettings')
 const settings = require('../js/constants/settings')
 const userPrefs = require('../js/state/userPrefs')
@@ -64,6 +65,11 @@ const registeredSessions = {}
  */
 const permissionCallbacks = {}
 
+/**
+ * A set to keep track of URLs fetching favicons that need special treatment later
+ */
+const faviconURLs = new Set()
+
 const getBraverySettingsForUrl = (url, appState, isPrivate) => {
   const cachedBraverySettings = getBraverySettingsCache(url, isPrivate)
   if (cachedBraverySettings) {
@@ -137,13 +143,21 @@ function registerForBeforeRequest (session, partition) {
       }
     }
 
+    const url = details.url
+    // filter out special urls for fetching favicons
+    if (faviconUtil.isWrappedFaviconUrl(url)) {
+      const redirectURL = faviconUtil.unwrapFaviconUrl(url)
+      faviconURLs.add(redirectURL)
+      muonCb({ redirectURL })
+      return
+    }
+
     if (shouldIgnoreUrl(details)) {
       muonCb({})
       return
     }
 
     const firstPartyUrl = module.exports.getMainFrameUrl(details)
-    const url = details.url
     // this can happen if the tab is closed and the webContents is no longer available
     if (!firstPartyUrl) {
       muonCb({})
@@ -329,9 +343,15 @@ function registerForBeforeSendHeaders (session, partition) {
   const isPrivate = module.exports.isPrivate(partition)
 
   session.webRequest.onBeforeSendHeaders(function (details, muonCb) {
+    // strip cookies from all requests fetching favicons
+    if (faviconURLs.has(details.url)) {
+      faviconURLs.delete(details.url)
+      delete details.requestHeaders['Cookie']
+    }
+
     // Using an electron binary which isn't from Brave
     if (shouldIgnoreUrl(details)) {
-      muonCb({})
+      muonCb({ requestHeaders: details.requestHeaders })
       return
     }
 

app/renderer/rendererTabEvents.js

@@ -21,6 +21,7 @@ const siteSettingsState = require('../common/state/siteSettingsState')
 const contextMenuState = require('../common/state/contextMenuState')
 
 const domUtil = require('./lib/domUtil')
+const faviconUtil = require('../../js/lib/faviconUtil')
 const imageUtil = require('../../js/lib/imageUtil')
 const historyUtil = require('../common/lib/historyUtil')
 const UrlUtil = require('../../js/lib/urlutil')
@@ -116,13 +117,15 @@ const api = module.exports = {
           break
         }
         if (e.favicons &&
-            e.favicons.length > 0 &&
-            // Favicon changes lead to recalculation of top site data so only fire
-            // this when needed.  Some sites update favicons very frequently.
-            e.favicons[0] !== frame.get('icon')) {
-          imageUtil.getWorkingImageUrl(e.favicons[0], (error) => {
-            windowActions.setFavicon(frame, error ? null : e.favicons[0])
-          })
+            e.favicons.length > 0) {
+          const url = faviconUtil.wrapFaviconUrl(e.favicons[0])
+          // Favicon changes lead to recalculation of top site data so only fire
+          // this when needed.  Some sites update favicons very frequently.
+          if (url !== frame.get('icon')) {
+            imageUtil.getWorkingImageUrl(url, (error) => {
+              windowActions.setFavicon(frame, error ? null : url)
+            })
+          }
         }
         break
       }

js/lib/faviconUtil.js

@@ -0,0 +1,11 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const suffix = '#brave-favicon-fragment'
+
+module.exports = {
+  wrapFaviconUrl: (url) => url && `${url}${suffix}`,
+  unwrapFaviconUrl: (url) => url && url.substring(0, url.length - suffix.length),
+  isWrappedFaviconUrl: (url) => !!(url && url.endsWith(suffix))
+}

test/bravery-components/braveryPanelTest.js

@@ -1053,4 +1053,28 @@ describe('Bravery Panel', function () {
         })
     })
   })
+
+  // see #14250
+  describe('Favicon requests cannot set cookies', function () {
+    Brave.beforeEach(this)
+    beforeEach(function * () {
+      yield setup(this.app.client)
+    })
+    it('does not allow favicon request to set cookie', function * () {
+      const url = Brave.server.url('cookie-favicon.html').replace('localhost', '127.0.0.1')
+      const testResultUrl = Brave.server.url('cookie-favicon-test-result.html')
+      yield this.app.client
+        .tabByIndex(0)
+        .loadUrl(url)
+        .openBraveMenu(braveMenu, braveryPanel)
+        .click(cookieControl)
+        .waitForVisible(allowAllCookiesOption)
+        .click(allowAllCookiesOption)
+        .tabByIndex(0)
+        .loadUrl(url)
+        .waitForExist('head>link')
+        .loadUrl(testResultUrl)
+        .waitForTextValue('body', 'pass')
+    })
+  })
 })

test/fixtures/cookie-favicon-test.html

@@ -0,0 +1,17 @@
+<html>
+<head>
+</head>
+<body>
+cookies:
+<div id='cookie-favicon'>
+</div>
+
+<script>
+function setText (id, result) {
+    document.getElementById(id).innerText = JSON.stringify(result)
+}
+
+setText('cookie-favicon', document.cookie)
+</script>
+</body>
+</html>

test/fixtures/cookie-favicon.html

@@ -0,0 +1,34 @@
+<html>
+<head>
+</head>
+<body>
+
+<iframe id='test-frame' height=200 width=500></iframe>
+
+<script>
+var host = window.location.origin
+
+function loadIco() {
+  var faviconLocation = host.replace('127.0.0.1', 'localhost') + '/cookie-favicon.ico'
+  var link = document.createElement('link')
+  link.rel = 'shortcut icon'
+  link.href = faviconLocation
+  document.head.appendChild(link);
+}
+
+
+function loadFrame() {
+  var testPageLocation = host.replace('127.0.0.1', 'localhost') + '/cookie-favicon-test.html'
+  var frame = document.getElementById('test-frame')
+  frame.onload = loadIco
+  frame.src = testPageLocation
+}
+
+loadFrame()
+
+function setText (id, result) {
+    document.getElementById(id).innerText = JSON.stringify(result)
+}
+</script>
+</body>
+</html>

test/lib/serverChild.js

@@ -91,6 +91,9 @@ Server.prototype = {
   start: function (port) {
     // using node-static for now we can do fancy stuff in the future.
     var file = new nodeStatic.Server(root)
+
+    // temporary state to control 3rd-party fingerprinting with favicons
+    var faviconCookieDetected = false
     this.http = http.createServer(function (req, res) {
       req.addListener('end', function () {
         // Handle corked urls.
@@ -124,6 +127,31 @@ Server.prototype = {
           return
         }
 
+        // the following routes are setup to test 3rd-party cookie sharing over favicons
+        if (req.url === '/cookie-favicon.ico') {
+          if (req.headers.cookie && req.headers.cookie !== '') {
+            faviconCookieDetected = true
+          }
+          file.servePath('/img/test.ico', 200, {}, req, res, () => res.end())
+          return
+        }
+
+        if (req.url === '/cookie-favicon-test.html') {
+          file.servePath(req.url, 200, {
+            'Set-Cookie': 'new-cookie'
+          }, req, res, () => res.end())
+          return
+        }
+
+        if (req.url === '/cookie-favicon-test-result.html') {
+          res.writeHead(200, {
+            'Content-Type': 'text/html'
+          })
+          const text = faviconCookieDetected ? 'fail' : 'pass'
+          res.end(`<body>${text}</body>`)
+          return
+        }
+
         // If we have been asked to send our own headers for this URL, do that
         if (server.urlsWithHeaders[fullUrl]) {
           file.serveWithHeaders(req, res, server.urlsWithHeaders[fullUrl])

test/unit/js/lib/faviconUtilTest.js

@@ -0,0 +1,41 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+/* global describe, it */
+
+const faviconUtil = require('../../../../js/lib/faviconUtil')
+const assert = require('assert')
+
+require('../../braveUnit')
+
+describe('faviconUtil', function () {
+  it('wraps, identifies and unwraps URLs', function () {
+    const url = 'http://webserver.com/favicon.ico'
+    const wrappedUrl = faviconUtil.wrapFaviconUrl(url)
+    const unwrappedUrl = faviconUtil.unwrapFaviconUrl(wrappedUrl)
+
+    assert.equal(faviconUtil.isWrappedFaviconUrl(url), false)
+    assert.equal(faviconUtil.isWrappedFaviconUrl(wrappedUrl), true)
+    assert.equal(faviconUtil.isWrappedFaviconUrl(unwrappedUrl), false)
+    assert.equal(url, unwrappedUrl)
+  })
+  it('wraps, identifies and unwraps URLs with query params', function () {
+    const url = 'http://webserver.com/favicon.ico?qp=1&qp2=2'
+    const wrappedUrl = faviconUtil.wrapFaviconUrl(url)
+    const unwrappedUrl = faviconUtil.unwrapFaviconUrl(wrappedUrl)
+
+    assert.equal(faviconUtil.isWrappedFaviconUrl(url), false)
+    assert.equal(faviconUtil.isWrappedFaviconUrl(wrappedUrl), true)
+    assert.equal(faviconUtil.isWrappedFaviconUrl(unwrappedUrl), false)
+    assert.equal(url, unwrappedUrl)
+  })
+  it('works with undefined', function () {
+    const url = undefined
+    const wrappedUrl = faviconUtil.wrapFaviconUrl(url)
+    const unwrappedUrl = faviconUtil.unwrapFaviconUrl(wrappedUrl)
+
+    assert.equal(faviconUtil.isWrappedFaviconUrl(url), false)
+    assert.equal(wrappedUrl, undefined)
+    assert.equal(unwrappedUrl, undefined)
+  })
+})