From fc24493f2e090b23ed7ba44fee7ac7b3e8ca6f23 Mon Sep 17 00:00:00 2001 From: Anthony Tseng Date: Mon, 27 Mar 2017 14:29:13 -0700 Subject: [PATCH] Only check third party url when not in main frame 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 --- app/adBlock.js | 3 +-- app/browser/ads/adBlockUtil.js | 9 ++++--- test/unit/app/browser/ads/adBlockUtilTest.js | 25 +++++++++++--------- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/app/adBlock.js b/app/adBlock.js index 4b14cd38bc6..021885012db 100644 --- a/app/adBlock.js +++ b/app/adBlock.js @@ -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 { diff --git a/app/browser/ads/adBlockUtil.js b/app/browser/ads/adBlockUtil.js index 492affe5e16..0b28a69e308 100644 --- a/app/browser/ads/adBlockUtil.js +++ b/app/browser/ads/adBlockUtil.js @@ -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 diff --git a/test/unit/app/browser/ads/adBlockUtilTest.js b/test/unit/app/browser/ads/adBlockUtilTest.js index 7ea8590eb23..8ae9d49f012 100644 --- a/test/unit/app/browser/ads/adBlockUtilTest.js +++ b/test/unit/app/browser/ads/adBlockUtilTest.js @@ -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)) }) }) })