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

Commit

Permalink
Only check third party url when not in main frame
Browse files Browse the repository at this point in the history
fix #7916

Auditors: @bbondy, @bsclifton

Test Plan:
- Make sure automated tests are passing
  - `npm run test -- --grep=adBlockUtil`
- Make sure you don't see ads on YouTube.
- Make sure `http://downloadme.org/` will be reported as malicious site
  • Loading branch information
darkdh authored and bridiver committed Apr 4, 2017
1 parent cc1898c commit ece3db6
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 16 deletions.
3 changes: 1 addition & 2 deletions app/adBlock.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ const startAdBlocking = (adblock, resourceName, shouldCheckMainFrame) => {
}
const firstPartyUrl = urlParse(mainFrameUrl)
const url = urlParse(details.url)
const cancel = (details.resourceType !== 'mainFrame' || shouldCheckMainFrame) &&
shouldDoAdBlockCheck(details.resourceType, firstPartyUrl, url) &&
const cancel = shouldDoAdBlockCheck(details.resourceType, firstPartyUrl, url, shouldCheckMainFrame) &&
adblock.matches(details.url, mapFilterType[details.resourceType], firstPartyUrl.host)

return {
Expand Down
9 changes: 6 additions & 3 deletions app/browser/ads/adBlockUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@ const mapFilterType = {
* @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
* @param shouldCheckMainFrame {boolean} Whether check main frame
*/
const shouldDoAdBlockCheck = (resourceType, firstPartyUrl, url) =>
const shouldDoAdBlockCheck = (resourceType, firstPartyUrl, url, shouldCheckMainFrame) =>
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) &&
shouldCheckMainFrame ||
(resourceType !== 'mainFrame' &&
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
Expand Down
25 changes: 14 additions & 11 deletions test/unit/app/browser/ads/adBlockUtilTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,35 @@ const {shouldDoAdBlockCheck} = require('../../../../../app/browser/ads/adBlockUt
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))
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource, false))
})
it('https protocol allows ad block checks', function () {
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource))
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource, false))
})
it('ftp protocol does not allow ad block checks', function () {
assert.ok(!shouldDoAdBlockCheck('script', new URL('ftp://www.brave.com'), thirdPartyResource))
assert.ok(!shouldDoAdBlockCheck('script', new URL('ftp://www.brave.com'), thirdPartyResource, false))
})
it('should check third party urls', function () {
assert.ok(shouldDoAdBlockCheck('script', site, thirdPartyResource))
assert.ok(shouldDoAdBlockCheck('script', site, thirdPartyResource, false))
})
it('should NOT check first party urls', function () {
assert.ok(!shouldDoAdBlockCheck('script', site, firstPartyResource))
assert.ok(!shouldDoAdBlockCheck('script', site, firstPartyResource, false))
})
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')))
assert.ok(!shouldDoAdBlockCheck('beefaroni', site, new URL('https://disqus.com/test'), false))
})
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')))
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.youtube.com'), new URL('https://www.youtube.com/script.js'), false))
})
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')))
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://disqus.com/test'), false))
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://hello.disqus.com/test'), false))
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://a.disquscdn.com/test'), false))
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://b.a.disquscdn.com/test'), false))
})
it('should NOT check third party urls for main frame', function () {
assert.ok(shouldDoAdBlockCheck('mainFrame', site, site, true))
})
})
})

0 comments on commit ece3db6

Please sign in to comment.