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

Commit

Permalink
Allow first party URLs from www.youtube.com
Browse files Browse the repository at this point in the history
Auditors:
  - @bsclifton: For code review, also please pull into 0.14.0 if that's
    ok.
  - @ukemulks: For verification that ads are being blocked on YouTube

Test cases:
- Make sure automated tests are passing
  - `npm run test -- --grep=isThirdPartyHost`
  - `npm run test -- --grep=adBlockUtil`
- Make sure things that used to be blocked are still being blocked.
- Make sure you don't see ads on YouTube.

Refactored code to be more easily unit tested and added some as well

Fix #7432
  • Loading branch information
bbondy committed Mar 21, 2017
1 parent 2aa57f5 commit 85464b9
Show file tree
Hide file tree
Showing 8 changed files with 153 additions and 39 deletions.
35 changes: 12 additions & 23 deletions app/adBlock.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
'use strict'

const urlParse = require('./common/urlParse')
const {AdBlockClient, FilterOptions} = require('ad-block')
const {AdBlockClient} = require('ad-block')
const DataFile = require('./dataFile')
const Filtering = require('./filtering')
const appConfig = require('../js/constants/appConfig')
Expand All @@ -19,23 +19,18 @@ const getSetting = require('../js/settings').getSetting
const {ADBLOCK_CUSTOM_RULES} = require('../js/constants/settings')
const customFilterRulesUUID = 'CE61F035-9F0A-4999-9A5A-D4E46AF676F7'
const appActions = require('../js/actions/appActions')
const {mapFilterType, shouldDoAdBlockCheck} = require('./browser/ads/adBlockUtil')

module.exports.adBlockResourceName = 'adblock'
module.exports.safeBrowsingResourceName = 'safeBrowsing'

let mapFilterType = {
mainFrame: FilterOptions.document,
subFrame: FilterOptions.subdocument,
stylesheet: FilterOptions.stylesheet,
script: FilterOptions.script,
image: FilterOptions.image,
object: FilterOptions.object,
xhr: FilterOptions.xmlHttpRequest,
other: FilterOptions.other
}

const whitelistHosts = ['disqus.com', 'a.disquscdn.com']

/**
* Starts ad blocking
* @param adblock {AdBlockClient} - The ad block client to start
* @param resourceName {string} - The resource name e.g. adblock, safeBrowsing
* @param shouldCheckMainFrame {boolean} - True if main frame URLs should be checked.
* for example safe browsing checks main frame URLs but ad block checks do not.
*/
const startAdBlocking = (adblock, resourceName, shouldCheckMainFrame) => {
Filtering.registerBeforeRequestFilteringCB((details) => {
const mainFrameUrl = Filtering.getMainFrameUrl(details)
Expand All @@ -46,15 +41,9 @@ const startAdBlocking = (adblock, resourceName, shouldCheckMainFrame) => {
}
}
const firstPartyUrl = urlParse(mainFrameUrl)
let firstPartyUrlHost = firstPartyUrl.hostname || ''
const urlHost = urlParse(details.url).hostname
const cancel = firstPartyUrl.protocol &&
(shouldCheckMainFrame || (details.resourceType !== 'mainFrame' &&
Filtering.isThirdPartyHost(firstPartyUrlHost, urlHost))) &&
firstPartyUrl.protocol.startsWith('http') &&
mapFilterType[details.resourceType] !== undefined &&
!whitelistHosts.includes(urlHost) &&
!urlHost.endsWith('.disqus.com') &&
const url = urlParse(details.url)
const cancel = (details.resourceType !== 'mainFrame' || shouldCheckMainFrame) &&
shouldDoAdBlockCheck(details.resourceType, firstPartyUrl, url) &&
adblock.matches(details.url, mapFilterType[details.resourceType], firstPartyUrl.host)

return {
Expand Down
45 changes: 45 additions & 0 deletions app/browser/ads/adBlockUtil.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/* 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 {siteHacks} = require('../../../js/data/siteHacks')
const {FilterOptions} = require('ad-block')
const isThirdPartyHost = require('../isThirdPartyHost')

const whitelistHosts = ['disqus.com', 'a.disquscdn.com']

/**
* Maps filtering request resourceTypes to ones that our adBlock library understands
*/
const mapFilterType = {
mainFrame: FilterOptions.document,
subFrame: FilterOptions.subdocument,
stylesheet: FilterOptions.stylesheet,
script: FilterOptions.script,
image: FilterOptions.image,
object: FilterOptions.object,
xhr: FilterOptions.xmlHttpRequest,
other: FilterOptions.other
}

/**
* @param resourceType {string} The resource type from the web request API
* @param firstPartyUrl {Url} The parsed URL of the main frame URL loading the url
* @param url {Url} The parsed URL of the resource for consideration
*/
const shouldDoAdBlockCheck = (resourceType, firstPartyUrl, url) =>
firstPartyUrl.protocol &&
// By default first party hosts are allowed, but enable the check if a flag is specified in siteHacks
(isThirdPartyHost(firstPartyUrl.hostname || '', url.hostname) ||
siteHacks[firstPartyUrl.hostname] && siteHacks[firstPartyUrl.hostname].allowFirstPartyAdblockChecks) &&
// Only check http and https for now
firstPartyUrl.protocol.startsWith('http') &&
// Only do adblock if the host isn't in the whitelist
!whitelistHosts.find((whitelistHost) => whitelistHost === url.hostname || url.hostname.endsWith('.' + whitelistHost)) &&
// Make sure there's a valid resource type before trying to use adblock
mapFilterType[resourceType] !== undefined

module.exports = {
mapFilterType,
shouldDoAdBlockCheck
}
24 changes: 24 additions & 0 deletions app/browser/isThirdPartyHost.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/* 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 getBaseDomain = require('../../js/lib/baseDomain').getBaseDomain

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

module.exports = isThirdPartyHost
17 changes: 2 additions & 15 deletions app/filtering.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ const appConfig = require('../js/constants/appConfig')
const hostContentSettings = require('./browser/contentSettings/hostContentSettings')
const downloadStates = require('../js/constants/downloadStates')
const urlParse = require('./common/urlParse')
const getBaseDomain = require('../js/lib/baseDomain').getBaseDomain
const getSetting = require('../js/settings').getSetting
const appUrlUtil = require('../js/lib/appUrlUtil')
const promisify = require('../js/lib/promisify')
Expand All @@ -33,6 +32,7 @@ const getOrigin = require('../js/state/siteUtil').getOrigin
const {adBlockResourceName} = require('./adBlock')
const {updateElectronDownloadItem} = require('./browser/electronDownloadItem')
const {fullscreenOption} = require('./common/constants/settingsEnums')
const isThirdPartyHost = require('./browser/isThirdPartyHost')

let appStore = null

Expand Down Expand Up @@ -260,7 +260,7 @@ function registerForBeforeSendHeaders (session, partition) {
const parsedFirstPartyUrl = urlParse(firstPartyUrl)

if (cookieSetting === 'blockAllCookies' ||
module.exports.isThirdPartyHost(parsedFirstPartyUrl.hostname, parsedTargetUrl.hostname)) {
isThirdPartyHost(parsedFirstPartyUrl.hostname, parsedTargetUrl.hostname)) {
// Clear cookie and referer on third-party requests
if (requestHeaders['Cookie'] &&
getOrigin(firstPartyUrl) !== pdfjsOrigin) {
Expand Down Expand Up @@ -456,19 +456,6 @@ function registerPermissionHandler (session, partition) {
})
}

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

function updateDownloadState (downloadId, item, state) {
updateElectronDownloadItem(downloadId, item, state)

Expand Down
3 changes: 2 additions & 1 deletion app/trackingProtection.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const urlParse = require('./common/urlParse')
const TrackingProtection = require('tracking-protection').CTPParser
const DataFile = require('./dataFile')
const Filtering = require('./filtering')
const isThirdPartyHost = require('./browser/isThirdPartyHost')
const LRUCache = require('lru-cache')

module.exports.resourceName = 'trackingProtection'
Expand Down Expand Up @@ -48,7 +49,7 @@ const startTrackingProtection = (wnd) => {
trackingProtection.matchesTracker(firstPartyUrlHost, urlHost) &&
urlHost !== firstPartyUrl.hostname &&
!cachedFirstParty.get(firstPartyUrlHost).find((baseHost) =>
!Filtering.isThirdPartyHost(baseHost, urlHost))
!isThirdPartyHost(baseHost, urlHost))

DataFile.debug(module.exports.resourceName, details, cancel)
return {
Expand Down
3 changes: 3 additions & 0 deletions js/data/siteHacks.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,5 +135,8 @@ module.exports.siteHacks = {
'player.siriusxm.com': {
enableFlashCTP: true,
redirectURL: 'https://player.siriusxm.com'
},
'www.youtube.com': {
allowFirstPartyAdblockChecks: true
}
}
43 changes: 43 additions & 0 deletions test/unit/app/browser/ads/adBlockUtilTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/* global describe, it */

require('../../../braveUnit')
const assert = require('assert')
const {URL} = require('url')

const site = new URL('https://www.brave.com')
const thirdPartyResource = new URL('https://www.coffee.com/logo.png')
const firstPartyResource = new URL('https://www.brave.com/logo.png')
const {shouldDoAdBlockCheck} = require('../../../../../app/browser/ads/adBlockUtil')

describe('adBlockUtil test', function () {
describe('shouldDoAdBlockCheck', function () {
it('http protocol allows ad block checks', function () {
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource))
})
it('https protocol allows ad block checks', function () {
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource))
})
it('ftp protocol does not allow ad block checks', function () {
assert.ok(!shouldDoAdBlockCheck('script', new URL('ftp://www.brave.com'), thirdPartyResource))
})
it('should check third party urls', function () {
assert.ok(shouldDoAdBlockCheck('script', site, thirdPartyResource))
})
it('should NOT check first party urls', function () {
assert.ok(!shouldDoAdBlockCheck('script', site, firstPartyResource))
})
it('Avoid checks with unknown resource types', function () {
// This test is valid just as long as we don't start handling beefaroni resource types in the ad block lib!!!
assert.ok(!shouldDoAdBlockCheck('beefaroni', site, new URL('https://disqus.com/test')))
})
it('should check first party hosts on youtube', function () {
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.youtube.com'), new URL('https://www.youtube.com/script.js')))
})
it('diqus is allowed as third party, for now', function () {
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://disqus.com/test')))
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://hello.disqus.com/test')))
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://a.disquscdn.com/test')))
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://b.a.disquscdn.com/test')))
})
})
})
22 changes: 22 additions & 0 deletions test/unit/app/browser/isThirdPartyhostTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/* global describe, it */

require('../../braveUnit')
const assert = require('assert')
const isThirdPartyHost = require('../../../../app/browser/isThirdPartyHost')

const braveHost = 'brave.com'

describe('isThirdPartyHost test', function () {
it('A URL should not be third party to itself', function () {
assert.ok(!isThirdPartyHost(braveHost, braveHost))
})
it('A subdomain URL should not be third party', function () {
assert.ok(!isThirdPartyHost(braveHost, 'ragu.brave.com'))
})
it('Unrelated URLs should be third party', function () {
assert.ok(isThirdPartyHost(braveHost, 'ragu.com'))
})
it('Checks subdomains properly', function () {
assert.ok(isThirdPartyHost(braveHost, 'brave.ragu.com'))
})
})

3 comments on commit 85464b9

@bbondy
Copy link
Member Author

@bbondy bbondy commented on 85464b9 Mar 21, 2017

Choose a reason for hiding this comment

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

Commit title should probably read:
Allow first party URL checks from www.youtube.com.

Sorry for the confusion.

@bsclifton
Copy link
Member

Choose a reason for hiding this comment

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

++ looks great 😄 Thanks for the unit tests!

I moved issue to 0.14.0 and copied the steps there 😄

@lukemulks
Copy link
Collaborator

@lukemulks lukemulks commented on 85464b9 Mar 21, 2017

Choose a reason for hiding this comment

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

@bbondy
G2G!

  • I've gone through 40+ YT videos w/o being authenticated, and am not seeing any ads.
  • I've gone through 40+ YT videos while authenticated - and am not seeing any ads.
  • I'm seeing the annotations being blocked, which is good.
  • I'm seeing bad 1p requests blocked. w00t!

This is good to push from my end, any anomalies can be captured and worked thru as they come in, but this is fantastic for Youtube.com. Thanks again!

Please sign in to comment.