Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

refactor isThirdPartyHost and block referer based on base domain #13820

Merged
merged 1 commit into from
Apr 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions app/browser/isThirdPartyHost.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,28 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const getBaseDomain = require('../../js/lib/baseDomain').getBaseDomain
const ip = require('ip')

/**
* baseContextHost {string} - The base host to check against
* testHost {string} - The host to check
* Checks if two hosts are third party. Subdomains count as first-party to the
* parent domain. Uses hostname (no port).
* @param {host1} string - First hostname to compare
* @param {host2} string - Second hostname to compare
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: This is supposed to be the complement of an equivalence relation now, yes? I.e., its complement is supposed to be transitive, reflexive, and symmetric? Can this be noted in the comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure the previous implementation's complement was also transitive, reflexive, and symmetric (at least it was supposed to be), but this refactoring makes those properties more obvious

*/
const isThirdPartyHost = (baseContextHost, testHost) => {
// TODO: Always return true if these are IP addresses that aren't the same
if (!testHost || !baseContextHost) {
const isThirdPartyHost = (host1, host2) => {
if (!host1 || !host2) {
return true
}
const documentDomain = getBaseDomain(baseContextHost)
if (testHost.length > documentDomain.length) {
return (testHost.substr(testHost.length - documentDomain.length - 1) !== '.' + documentDomain)
} else {
return (testHost !== documentDomain)
if (host1 === host2) {
return false
}

if (ip.isV4Format(host1) || ip.isV4Format(host2)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&&?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jumde the problem with && is that getBaseDomain thinks google.com.0.1 and 127.0.0.1 have the same base domain. so if either is an IPv4 address, we should do literal string comparison.

// '127.0.0.1' and '::7f00:1' are actually equal, but ignore such cases for now
return host1 !== host2
}

return getBaseDomain(host1) !== getBaseDomain(host2)
}

module.exports = isThirdPartyHost
18 changes: 9 additions & 9 deletions app/filtering.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,12 +245,13 @@ function registerForBeforeRedirect (session, partition) {
module.exports.applyCookieSetting = (requestHeaders, url, firstPartyUrl, isPrivate) => {
const cookieSetting = module.exports.isResourceEnabled(appConfig.resourceNames.COOKIEBLOCK, firstPartyUrl, isPrivate)
if (cookieSetting) {
const parsedTargetUrl = urlParse(url || '')
const parsedFirstPartyUrl = urlParse(firstPartyUrl)
const targetHostname = urlParse(url || '').hostname
const firstPartyHostname = urlParse(firstPartyUrl).hostname
const targetOrigin = getOrigin(url)
const referer = requestHeaders['Referer']

if (cookieSetting === 'blockAllCookies' ||
isThirdPartyHost(parsedFirstPartyUrl.hostname, parsedTargetUrl.hostname)) {
isThirdPartyHost(firstPartyHostname, targetHostname)) {
let hasCookieException = false
const firstPartyOrigin = getOrigin(firstPartyUrl)
if (cookieExceptions.hasOwnProperty(firstPartyOrigin)) {
Expand All @@ -268,20 +269,19 @@ module.exports.applyCookieSetting = (requestHeaders, url, firstPartyUrl, isPriva
}
}
}
// Clear cookie and referer on third-party requests

// Clear cookie on third-party requests
if (requestHeaders['Cookie'] &&
firstPartyOrigin !== pdfjsOrigin && !hasCookieException) {
requestHeaders['Cookie'] = undefined
}
}

const referer = requestHeaders['Referer']
if (referer &&
cookieSetting !== 'allowAllCookies' &&
!refererExceptions.includes(parsedTargetUrl.hostname) &&
targetOrigin !== getOrigin(referer)) {
// Unless the setting is 'allow all cookies', spoof the referer if it
// is a cross-origin referer
!refererExceptions.includes(targetHostname) &&
isThirdPartyHost(targetHostname, urlParse(referer).hostname)) {
// Spoof third party referer
requestHeaders['Referer'] = targetOrigin
}
}
Expand Down
10 changes: 9 additions & 1 deletion js/lib/baseDomain.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ const checkASCII = function (str) {
* Returns base domain for specified host based on Public Suffix List.
* Derived from Privacy Badger Chrome <https://github.com/EFForg/privacybadger>,
* Copyright (C) 2015 Electronic Frontier Foundation and other contributors
* @param {string} hostname The name of the host to get the base domain for
* TODO: Consider refactoring this into isThirdPartyHost since it's only used
* for that.
* @param {string} hostname The name of the host to get the base domain for.
* The caller must validate that this is a valid, non-IP hostname string!!
*/

module.exports.getBaseDomain = function (hostname) {
Expand All @@ -44,6 +47,11 @@ module.exports.getBaseDomain = function (hostname) {
return baseDomain
}

// If the hostname is a TLD, return '' for the base domain
if (hostname in publicSuffixes) {
return ''
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously for an effective TLD like co.uk, this would return co.uk as the base domain. that seems incorrect so i just set it to the empty string.

note that some eTLDs like github.io are resolvable

}

// search through PSL
var prevDomains = []
var curDomain = hostname
Expand Down
29 changes: 29 additions & 0 deletions test/unit/app/browser/isThirdPartyhostTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,40 @@ describe('isThirdPartyHost test', function () {
})
it('A subdomain URL should not be third party', function () {
assert.ok(!isThirdPartyHost(braveHost, 'ragu.brave.com'))
assert.ok(!isThirdPartyHost('ragu.brave.com', braveHost))
})
it('A 2nd level subdomain URL should not be third party', function () {
assert.ok(!isThirdPartyHost(braveHost, 'foo.bar.brave.com'))
assert.ok(!isThirdPartyHost('foo.bar.brave.com', braveHost))
})
it('Unrelated URLs should be third party', function () {
assert.ok(isThirdPartyHost(braveHost, 'ragu.com'))
assert.ok(isThirdPartyHost('ragu.com', braveHost))
})
it('Checks subdomains properly', function () {
assert.ok(isThirdPartyHost(braveHost, 'brave.ragu.com'))
assert.ok(isThirdPartyHost('brave.ragu.com', braveHost))
})
it('Handles multi-part TLDs', function () {
assert.ok(isThirdPartyHost('diracdeltas.github.io', 'brave.github.io'))
assert.ok(isThirdPartyHost('github.io', 'brave.github.io'))
assert.ok(!isThirdPartyHost('github.io', 'github.io'))
assert.ok(isThirdPartyHost('brave.github.io', 'github.io'))
assert.ok(isThirdPartyHost('brave.co.uk', 'example.co.uk'))
})
it('Handles IPv4', function () {
assert.ok(isThirdPartyHost('172.217.6.46', '173.217.6.46'))
assert.ok(!isThirdPartyHost('172.217.6.46', '172.217.6.46'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test equivalent decimal representations of IP addresses?

No harm if we say that 127.1 and 127.0.0.1, or 1.2.3.4 and 001.002.003.004, are distinct, but we should be explicitly intentional about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's clear from https://github.com/brave/browser-laptop/pull/13820/files#diff-3e19062054041e338c28b2593b82e8d9R23 that this is not intended to handle IP representations that are equivalent but don't have the same string representation for now

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or are you suggesting adding a test for the equivalent case returning third party so that it's explicit based on tests what the behavior should be? i can do that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ probably best to do in a follow-up PR since it's non-critical and adding it here would dismiss the existing reviews

})
it('Handles IPv6', function () {
assert.ok(!isThirdPartyHost('[2001:db8:85a3::8a2e:370:7334]', '[2001:db8:85a3::8a2e:370:7334]'))
assert.ok(!isThirdPartyHost('2001:db8:85a3::8a2e:370:7334', '2001:db8:85a3::8a2e:370:7334'))
assert.ok(isThirdPartyHost('[2001:db8:85a3::8a2e:370:7334]', '[2002:db8:85a3::8a2e:370:7334]'))
assert.ok(isThirdPartyHost('2001:db8:85a3::8a2e:370:7334', '2002:db8:85a3::8a2e:370:7334'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test v4-mapped IPv6 addresses vs equivalent IPv4 addresses?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

})
it('Handles null', function () {
assert.ok(isThirdPartyHost('', ''))
assert.ok(isThirdPartyHost(null, null))
assert.ok(isThirdPartyHost('', null))
})
})
45 changes: 37 additions & 8 deletions test/unit/app/filteringTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,44 @@ describe('filtering unit tests', function () {
isResourceEnabledStub.restore()
})

it('sets the referer to the origin', function () {
const url = 'https://cdnp3.stackassets.com/574db2390a12942fcef927356dadc6f9955edea9/store/fe3eb8fc014a20f2d25810b3c4f4b5b0db8695adfd7e8953721a55c51b90/sale_7217_primary_image.jpg'
const firstPartyUrl = 'https://slashdot.org/'
const requestHeaders = {
Referer: 'https://brave.com'
}
const result = filtering.applyCookieSetting(requestHeaders, url, firstPartyUrl, false)
describe('stubs referer for third-party referer', function () {
const firstPartyUrl = 'https://brave.com'
it('when the hosts are completely different', function () {
const url = 'https://cdnp3.stackassets.com/574db2390a12942fcef927356dadc6f9955edea9/store/fe3eb8fc014a20f2d25810b3c4f4b5b0db8695adfd7e8953721a55c51b90/sale_7217_primary_image.jpg'
const requestHeaders = {
Referer: 'https://brave.com'
}
const result = filtering.applyCookieSetting(requestHeaders, url, firstPartyUrl, false)
assert.equal(result.Referer, 'https://cdnp3.stackassets.com')
})
it('when the hosts have different base domains according to the PSL', function () {
const url = 'https://diracdeltas.github.io/foo?abc#test'
const requestHeaders = {
Referer: 'https://github.io'
}
const result = filtering.applyCookieSetting(requestHeaders, url, firstPartyUrl, false)
assert.equal(result.Referer, 'https://diracdeltas.github.io')
})
})

assert.equal(result.Referer, 'https://cdnp3.stackassets.com')
describe('does not change referer for first-party referer', function () {
const firstPartyUrl = 'https://brave.com'
it('keeps referer when hosts are the same', function () {
const url = 'https://test.github.io/test'
const requestHeaders = {
Referer: 'https://test.github.io/foo'
}
const result = filtering.applyCookieSetting(requestHeaders, url, firstPartyUrl, false)
assert.equal(result.Referer, 'https://test.github.io/foo')
})
it('keeps referer when hosts share a baseDomain', function () {
const url = 'https://docs.google.com'
const requestHeaders = {
Referer: 'https://2.drive.google.com/mydocument#abc'
}
const result = filtering.applyCookieSetting(requestHeaders, url, firstPartyUrl, false)
assert.equal(result.Referer, 'https://2.drive.google.com/mydocument#abc')
})
})

describe('when there is a referer exception', function () {
Expand Down
39 changes: 39 additions & 0 deletions test/unit/js/lib/baseDomainTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/* 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 {getBaseDomain} = require('../../../../js/lib/baseDomain')
const assert = require('assert')

require('../../braveUnit')

describe('getBaseDomain', function () {
it('regular domain and subdomains', function () {
const domain = 'brave.com'
assert.equal(domain, getBaseDomain('brave.com'))
assert.equal(domain, getBaseDomain('test.brave.com'))
assert.equal(domain, getBaseDomain('foo.test.brave.com'))
})
it('international domains', function () {
assert.equal('brave.comа', getBaseDomain('www.brave.xn--com-8cd'))
assert.equal('brave.\u9ce5\u53d6.jp', getBaseDomain('\u9ce5\u53d6.jp.brave.\u9ce5\u53d6.jp'))
assert.equal('ebаy.com', getBaseDomain('xn--eby-7cd.com'))
})
it('multi-part domains', function () {
assert.equal('diracdeltas.github.io', getBaseDomain('diracdeltas.github.io'))
assert.equal('diracdeltas.github.io', getBaseDomain('foo.diracdeltas.github.io'))
assert.equal('bar.ginoza.okinawa.jp', getBaseDomain('foo.bar.ginoza.okinawa.jp'))
assert.equal('brave.co.uk', 'brave.co.uk')
})
it('tlds', function () {
assert.equal('', getBaseDomain('github.io'))
assert.equal('', getBaseDomain('co.uk'))
})
it('non-hostname inputs', function () {
assert.equal('', getBaseDomain(''))
assert.equal('hello world', getBaseDomain('hello world'))
assert.equal('[2001:0db8:85a3:0000:0000:8a2e:0370:7334]', getBaseDomain('[2001:0db8:85a3:0000:0000:8a2e:0370:7334]'))
assert.equal('http://2001::7334', getBaseDomain('http://2001::7334'))
})
})