Skip to content

Commit fc24493

Browse files
committed
Only check third party url when not in main frame
fix brave#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
1 parent 0744193 commit fc24493

File tree

3 files changed

+21
-16
lines changed

3 files changed

+21
-16
lines changed

app/adBlock.js

+1-2
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,7 @@ const startAdBlocking = (adblock, resourceName, shouldCheckMainFrame) => {
4242
}
4343
const firstPartyUrl = urlParse(mainFrameUrl)
4444
const url = urlParse(details.url)
45-
const cancel = (details.resourceType !== 'mainFrame' || shouldCheckMainFrame) &&
46-
shouldDoAdBlockCheck(details.resourceType, firstPartyUrl, url) &&
45+
const cancel = shouldDoAdBlockCheck(details.resourceType, firstPartyUrl, url, shouldCheckMainFrame) &&
4746
adblock.matches(details.url, mapFilterType[details.resourceType], firstPartyUrl.host)
4847

4948
return {

app/browser/ads/adBlockUtil.js

+6-3
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,15 @@ const mapFilterType = {
2626
* @param resourceType {string} The resource type from the web request API
2727
* @param firstPartyUrl {Url} The parsed URL of the main frame URL loading the url
2828
* @param url {Url} The parsed URL of the resource for consideration
29+
* @param shouldCheckMainFrame {boolean} Whether check main frame
2930
*/
30-
const shouldDoAdBlockCheck = (resourceType, firstPartyUrl, url) =>
31+
const shouldDoAdBlockCheck = (resourceType, firstPartyUrl, url, shouldCheckMainFrame) =>
3132
firstPartyUrl.protocol &&
3233
// By default first party hosts are allowed, but enable the check if a flag is specified in siteHacks
33-
(isThirdPartyHost(firstPartyUrl.hostname || '', url.hostname) ||
34-
siteHacks[firstPartyUrl.hostname] && siteHacks[firstPartyUrl.hostname].allowFirstPartyAdblockChecks) &&
34+
shouldCheckMainFrame ||
35+
(resourceType !== 'mainFrame' &&
36+
isThirdPartyHost(firstPartyUrl.hostname || '', url.hostname) ||
37+
siteHacks[firstPartyUrl.hostname] && siteHacks[firstPartyUrl.hostname].allowFirstPartyAdblockChecks) &&
3538
// Only check http and https for now
3639
firstPartyUrl.protocol.startsWith('http') &&
3740
// Only do adblock if the host isn't in the whitelist

test/unit/app/browser/ads/adBlockUtilTest.js

+14-11
Original file line numberDiff line numberDiff line change
@@ -12,32 +12,35 @@ const {shouldDoAdBlockCheck} = require('../../../../../app/browser/ads/adBlockUt
1212
describe('adBlockUtil test', function () {
1313
describe('shouldDoAdBlockCheck', function () {
1414
it('http protocol allows ad block checks', function () {
15-
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource))
15+
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource, false))
1616
})
1717
it('https protocol allows ad block checks', function () {
18-
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource))
18+
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.brave.com'), thirdPartyResource, false))
1919
})
2020
it('ftp protocol does not allow ad block checks', function () {
21-
assert.ok(!shouldDoAdBlockCheck('script', new URL('ftp://www.brave.com'), thirdPartyResource))
21+
assert.ok(!shouldDoAdBlockCheck('script', new URL('ftp://www.brave.com'), thirdPartyResource, false))
2222
})
2323
it('should check third party urls', function () {
24-
assert.ok(shouldDoAdBlockCheck('script', site, thirdPartyResource))
24+
assert.ok(shouldDoAdBlockCheck('script', site, thirdPartyResource, false))
2525
})
2626
it('should NOT check first party urls', function () {
27-
assert.ok(!shouldDoAdBlockCheck('script', site, firstPartyResource))
27+
assert.ok(!shouldDoAdBlockCheck('script', site, firstPartyResource, false))
2828
})
2929
it('Avoid checks with unknown resource types', function () {
3030
// This test is valid just as long as we don't start handling beefaroni resource types in the ad block lib!!!
31-
assert.ok(!shouldDoAdBlockCheck('beefaroni', site, new URL('https://disqus.com/test')))
31+
assert.ok(!shouldDoAdBlockCheck('beefaroni', site, new URL('https://disqus.com/test'), false))
3232
})
3333
it('should check first party hosts on youtube', function () {
34-
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.youtube.com'), new URL('https://www.youtube.com/script.js')))
34+
assert.ok(shouldDoAdBlockCheck('script', new URL('https://www.youtube.com'), new URL('https://www.youtube.com/script.js'), false))
3535
})
3636
it('diqus is allowed as third party, for now', function () {
37-
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://disqus.com/test')))
38-
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://hello.disqus.com/test')))
39-
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://a.disquscdn.com/test')))
40-
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://b.a.disquscdn.com/test')))
37+
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://disqus.com/test'), false))
38+
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://hello.disqus.com/test'), false))
39+
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://a.disquscdn.com/test'), false))
40+
assert.ok(!shouldDoAdBlockCheck('script', site, new URL('https://b.a.disquscdn.com/test'), false))
41+
})
42+
it('should NOT check third party urls for main frame', function () {
43+
assert.ok(shouldDoAdBlockCheck('mainFrame', site, site, true))
4144
})
4245
})
4346
})

0 commit comments

Comments
 (0)