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

Commit

Permalink
use main frame url for webrequest first party url
Browse files Browse the repository at this point in the history
adds a test which fails on master (shows 0 blocked elements instead of 23)

fix #4137

auditors: @bridiver
  • Loading branch information
diracdeltas committed Sep 26, 2016
1 parent 80f00fd commit 0384ab3
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 17 deletions.
2 changes: 1 addition & 1 deletion app/adBlock.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const whitelistHosts = ['disqus.com', 'a.disquscdn.com']

const startAdBlocking = (adblock, resourceName, shouldCheckMainFrame) => {
Filtering.registerBeforeRequestFilteringCB((details) => {
const firstPartyUrl = URL.parse(details.firstPartyUrl)
const firstPartyUrl = URL.parse(Filtering.getMainFrameUrl(details))
let firstPartyUrlHost = firstPartyUrl.hostname || ''
const urlHost = URL.parse(details.url).hostname
const cancel = firstPartyUrl.protocol &&
Expand Down
42 changes: 31 additions & 11 deletions app/filtering.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const messages = require('../js/constants/messages')
const electron = require('electron')
const session = electron.session
const BrowserWindow = electron.BrowserWindow
const webContents = electron.webContents
const AppStore = require('../js/stores/appStore')
const appActions = require('../js/actions/appActions')
const appConfig = require('../js/constants/appConfig')
Expand Down Expand Up @@ -81,12 +82,13 @@ module.exports.registerHeadersReceivedFilteringCB = (filteringFn) => {
*/
function registerForBeforeRequest (session) {
session.webRequest.onBeforeRequest((details, cb) => {
// Using an electron binary which isn't from Brave
if (!details.firstPartyUrl || shouldIgnoreUrl(details.url)) {
if (shouldIgnoreUrl(details.url)) {
cb({})
return
}

const firstPartyUrl = module.exports.getMainFrameUrl(details)

This comment has been minimized.

Copy link
@bridiver

bridiver Oct 12, 2016

Collaborator

I'm confused because I thought I fixed all of these to check for null, but I guess I must have missed some of them because there is no check for null in master for this one and a few others

This comment has been minimized.

Copy link
@bridiver

bridiver Oct 12, 2016

Collaborator

I have a fix for it that I'll merge tomorrow


if (appUrlUtil.isTargetAboutUrl(details.url)) {
if (process.env.NODE_ENV === 'development' && !details.url.match(/devServerPort/)) {
// add webpack dev server port
Expand All @@ -106,7 +108,7 @@ function registerForBeforeRequest (session) {
const isHttpsEverywhere = results.resourceName === appConfig.resourceNames.HTTPS_EVERYWHERE
const isTracker = results.resourceName === appConfig.resourceNames.TRACKING_PROTECTION

if (!module.exports.isResourceEnabled(results.resourceName, details.firstPartyUrl)) {
if (!module.exports.isResourceEnabled(results.resourceName, firstPartyUrl)) {
continue
}
if (results.cancel) {
Expand Down Expand Up @@ -171,7 +173,7 @@ function registerForBeforeRedirect (session) {
// Note that onBeforeRedirect listener doesn't take a callback
session.webRequest.onBeforeRedirect(function (details) {
// Using an electron binary which isn't from Brave
if (!details.firstPartyUrl || shouldIgnoreUrl(details.url)) {
if (shouldIgnoreUrl(details.url)) {
return
}
for (let i = 0; i < beforeRedirectFilteringFns.length; i++) {
Expand All @@ -197,7 +199,7 @@ function registerForBeforeSendHeaders (session) {

session.webRequest.onBeforeSendHeaders(function (details, cb) {
// Using an electron binary which isn't from Brave
if (!details.firstPartyUrl || shouldIgnoreUrl(details.url)) {
if (shouldIgnoreUrl(details.url)) {
cb({})
return
}
Expand All @@ -217,9 +219,11 @@ function registerForBeforeSendHeaders (session) {
requestHeaders['User-Agent'] = spoofedUserAgent
}

const firstPartyUrl = module.exports.getMainFrameUrl(details)

for (let i = 0; i < beforeSendHeadersFilteringFns.length; i++) {
let results = beforeSendHeadersFilteringFns[i](details)
if (!module.exports.isResourceEnabled(results.resourceName, details.firstPartyUrl)) {
if (!module.exports.isResourceEnabled(results.resourceName, firstPartyUrl)) {
continue
}
if (results.cancel) {
Expand All @@ -231,12 +235,12 @@ function registerForBeforeSendHeaders (session) {
}
}

if (module.exports.isResourceEnabled(appConfig.resourceNames.COOKIEBLOCK, details.firstPartyUrl)) {
if (module.exports.isThirdPartyHost(urlParse(details.firstPartyUrl || '').hostname,
if (module.exports.isResourceEnabled(appConfig.resourceNames.COOKIEBLOCK, firstPartyUrl)) {
if (module.exports.isThirdPartyHost(urlParse(firstPartyUrl || '').hostname,
parsedUrl.hostname)) {
// Clear cookie and referer on third-party requests
if (requestHeaders['Cookie'] &&
getOrigin(details.firstPartyUrl) !== pdfjsOrigin) {
getOrigin(firstPartyUrl) !== pdfjsOrigin) {
requestHeaders['Cookie'] = undefined
}
if (requestHeaders['Referer'] &&
Expand All @@ -262,13 +266,14 @@ function registerForHeadersReceived (session) {
// Note that onBeforeRedirect listener doesn't take a callback
session.webRequest.onHeadersReceived(function (details, cb) {
// Using an electron binary which isn't from Brave
if (!details.firstPartyUrl || shouldIgnoreUrl(details.url)) {
if (shouldIgnoreUrl(details.url)) {
cb({})
return
}
const firstPartyUrl = module.exports.getMainFrameUrl(details)
for (let i = 0; i < headersReceivedFilteringFns.length; i++) {
let results = headersReceivedFilteringFns[i](details)
if (!module.exports.isResourceEnabled(results.resourceName, details.firstPartyUrl)) {
if (!module.exports.isResourceEnabled(results.resourceName, firstPartyUrl)) {
continue
}
if (results.responseHeaders) {
Expand Down Expand Up @@ -675,3 +680,18 @@ module.exports.clearAutofillData = () => {
ses.autofill.clearAutofillData()
}
}

module.exports.getMainFrameUrl = (details) => {
if (details.resourceType === 'mainFrame') {
return details.url
}
const tabId = details.tabId
const wc = webContents.getAllWebContents()
if (wc && tabId) {
const content = wc.find((item) => item.getId() === tabId)
if (content) {
return content.getURL()
}
}
return null
}
6 changes: 4 additions & 2 deletions app/httpsEverywhere.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ function startHttpsEverywhere () {

function onBeforeHTTPRequest (details) {
let result = { resourceName: module.exports.resourceName }
if (!Filtering.isResourceEnabled(module.exports.resourceName, details.firstPartyUrl)) {
if (!Filtering.isResourceEnabled(module.exports.resourceName,
Filtering.getMainFrameUrl(details))) {
return result
}
// Ignore URLs that are not HTTP
Expand All @@ -136,7 +137,8 @@ function onBeforeHTTPRequest (details) {
}

function onBeforeRedirect (details) {
if (!Filtering.isResourceEnabled(module.exports.resourceName, details.firstPartyUrl)) {
if (!Filtering.isResourceEnabled(module.exports.resourceName,
Filtering.getMainFrameUrl(details))) {
return
}

Expand Down
5 changes: 3 additions & 2 deletions app/siteHacks.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,11 @@ module.exports.init = () => {

let redirectURL
let cancel
const firstPartyUrl = Filtering.getMainFrameUrl(details)
if (hack && hack.onBeforeRequest &&
(hack.enableForAll ||
hack.enableForAdblock && Filtering.isResourceEnabled(appConfig.resourceNames.ADBLOCK, details.firstPartyUrl) ||
hack.enableForTrackingProtection && Filtering.isResourceEnabled(appConfig.resourceNames.TRACKING_PROTECTION, details.firstPartyUrl))) {
hack.enableForAdblock && Filtering.isResourceEnabled(appConfig.resourceNames.ADBLOCK, firstPartyUrl) ||
hack.enableForTrackingProtection && Filtering.isResourceEnabled(appConfig.resourceNames.TRACKING_PROTECTION, firstPartyUrl))) {
const result = hack.onBeforeRequest.call(this, details)
if (result && result.redirectURL) {
redirectURL = result.redirectURL
Expand Down
2 changes: 1 addition & 1 deletion app/trackingProtection.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const whitelistHosts = ['connect.facebook.net', 'connect.facebook.com', 'staticx

const startTrackingProtection = (wnd) => {
Filtering.registerBeforeRequestFilteringCB((details) => {
const firstPartyUrl = URL.parse(details.firstPartyUrl)
const firstPartyUrl = URL.parse(Filtering.getMainFrameUrl(details))
let firstPartyUrlHost = firstPartyUrl.hostname || ''
if (firstPartyUrlHost.startsWith('www.')) {
firstPartyUrlHost = firstPartyUrlHost.substring(4)
Expand Down
12 changes: 12 additions & 0 deletions test/components/braveryPanelTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@ describe('Bravery Panel', function () {
.then((blocked) => blocked === '1')
})
})
it('detects blocked elements in iframe', function * () {
const url = Brave.server.url('slashdot.html')
yield this.app.client
.tabByIndex(0)
.loadUrl(url)
yield openBraveMenu(this.app.client)
yield this.app.client
.waitUntil(function () {
return this.getText(adsBlockedStat)
.then((blocked) => Number(blocked) > 10)
})
})
it('detects https upgrades', function * () {
yield waitForDataFile(this.app.client, 'httpsEverywhere')
const url = Brave.server.url('httpsEverywhere.html')
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/slashdot.html
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<iframe src='https://slashdot.org' />

0 comments on commit 0384ab3

Please sign in to comment.