From 95ec2aeb5a47551e6c5b6bf11399fd731d5eeb5f Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 15 Sep 2016 11:25:40 -0700 Subject: [PATCH 1/3] Don't register a visit if page returns a non-successful HTTP status code Fixes https://github.com/brave/browser-laptop/issues/3759 Auditors: @mrose17, @bridiver Test Plan: 1. Visit https://clifton.io 2. Stay on page for a while 3. Confirm time spent is incremented 4. Visit https://clifton.io/404 5. Stay on page for a while 6. Confirm time spent is not incremented Udpated w/ review feedback from @bridiver --- app/common/lib/ledgerUtil.js | 55 ++++++++++++++++++++++++ app/ledger.js | 11 +++-- js/actions/windowActions.js | 8 ++++ js/components/frame.js | 4 +- js/constants/windowConstants.js | 3 +- js/stores/eventStore.js | 41 +++++++++++------- test/unit/lib/ledgerUtilTest.js | 74 +++++++++++++++++++++++++++++++++ 7 files changed, 175 insertions(+), 21 deletions(-) create mode 100644 app/common/lib/ledgerUtil.js create mode 100644 test/unit/lib/ledgerUtilTest.js diff --git a/app/common/lib/ledgerUtil.js b/app/common/lib/ledgerUtil.js new file mode 100644 index 00000000000..caa0a44ee22 --- /dev/null +++ b/app/common/lib/ledgerUtil.js @@ -0,0 +1,55 @@ +/* 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/. */ + +'use strict' + +/** + * Returns true if HTTP response code is one we want to collect usage for + * @param {number} responseCode - HTTP response code to be evaluated + * @return {boolean} true if the page should be included, false if not + */ +module.exports.shouldTrackResponseCode = (responseCode) => { + switch (responseCode) { + case 200: // ok + case 203: // non-authoritative + case 206: // partial content + case 304: // not modified (cached) + return true + } + return false +} + +/** + * Is page an actual page being viewed by the user? (not an error page, etc) + * If the page is invalid, we don't want to collect usage info. + * @param {Object} view - an entry from page_view (from EventStore) + * @param {Object} responseList - full page_response array (from EventStore) + * @return {boolean} true if page should have usage collected, false if not + */ +module.exports.shouldTrackView = (view, responseList) => { + if (!view || !view.url || !view.tabId) { + return false + } + if (!responseList || !Array.isArray(responseList) || !responseList.length) { + return false + } + + const tabId = view.tabId + const url = view.url + + for (let i = responseList.length; i > -1; i--) { + const response = responseList[i] + + if (!response) continue + + const responseUrl = response && response.details + ? response.details.originalURL || response.details.newURL + : null + + if (url === responseUrl && response.tabId === tabId) { + return module.exports.shouldTrackResponseCode(response.details.httpResponseCode) + } + } + return false +} diff --git a/app/ledger.js b/app/ledger.js index 44babd7a43b..27e24f3f907 100644 --- a/app/ledger.js +++ b/app/ledger.js @@ -51,6 +51,7 @@ const locale = require('./locale') const appStore = require('../js/stores/appStore') const eventStore = require('../js/stores/eventStore') const rulesolver = require('./extensions/brave/content/scripts/pageInformation.js') +const ledgerUtil = require('./common/lib/ledgerUtil') // TBD: remove these post beta [MTR] const logPath = 'ledger-log.json' @@ -254,8 +255,10 @@ underscore.keys(fileTypes).forEach((fileType) => { signatureMax = Math.ceil(signatureMax * 1.5) eventStore.addChangeListener(() => { - var view = eventStore.getState().toJS().page_view - var info = eventStore.getState().toJS().page_info + const eventState = eventStore.getState().toJS() + var view = eventState.page_view + var info = eventState.page_info + var pageLoad = eventState.page_load if ((!synopsis) || (!util.isArray(info))) return @@ -339,7 +342,9 @@ eventStore.addChangeListener(() => { }) view = underscore.last(view) || {} - visit(view.url || 'NOOP', view.timestamp || underscore.now(), view.tabId) + if (ledgerUtil.shouldTrackView(view, pageLoad)) { + visit(view.url || 'NOOP', view.timestamp || underscore.now(), view.tabId) + } }) /* diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index 8ba106137c6..820159d4675 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -1005,6 +1005,14 @@ const windowActions = { frameProps, source }) + }, + + gotResponseDetails: function (tabId, details) { + dispatch({ + actionType: WindowConstants.WINDOW_GOT_RESPONSE_DETAILS, + tabId, + details + }) } } diff --git a/js/components/frame.js b/js/components/frame.js index c024bd2b67a..195828c2bb3 100644 --- a/js/components/frame.js +++ b/js/components/frame.js @@ -965,7 +965,9 @@ class Frame extends ImmutableComponent { })) } }) - + this.webview.addEventListener('did-get-response-details', (details) => { + windowActions.gotResponseDetails(this.frame.get('tabId'), details) + }) // Handle zoom using Ctrl/Cmd and the mouse wheel. this.webview.addEventListener('mousewheel', this.onMouseWheel.bind(this)) } diff --git a/js/constants/windowConstants.js b/js/constants/windowConstants.js index 57457442e78..3d9ea49b148 100644 --- a/js/constants/windowConstants.js +++ b/js/constants/windowConstants.js @@ -68,7 +68,8 @@ const windowConstants = { WINDOW_SET_CLEAR_BROWSING_DATA_DETAIL: _, WINDOW_SET_AUTOFILL_ADDRESS_DETAIL: _, WINDOW_SET_AUTOFILL_CREDIT_CARD_DETAIL: _, - WINDOW_SET_BLOCKED_RUN_INSECURE_CONTENT: _ + WINDOW_SET_BLOCKED_RUN_INSECURE_CONTENT: _, + WINDOW_GOT_RESPONSE_DETAILS: _ } module.exports = mapValuesByKeys(windowConstants) diff --git a/js/stores/eventStore.js b/js/stores/eventStore.js index d3590626d23..b431a703c40 100644 --- a/js/stores/eventStore.js +++ b/js/stores/eventStore.js @@ -10,6 +10,7 @@ const Immutable = require('immutable') const WindowConstants = require('../constants/windowConstants') const debounce = require('../lib/debounce.js') const { isSourceAboutUrl } = require('../lib/appUrlUtil') +const { shouldTrackResponseCode } = require('../../app/common/lib/ledgerUtil') const electron = require('electron') const BrowserWindow = electron.BrowserWindow @@ -89,22 +90,6 @@ const windowClosed = (windowId) => { // Register callback to handle all updates const doAction = (action) => { switch (action.actionType) { - case WindowConstants.WINDOW_WEBVIEW_LOAD_END: - // create a page view event if this is a page load on the active tabId - if (!lastActiveTabId || action.frameProps.get('tabId') === lastActiveTabId) { - addPageView(action.frameProps.get('location'), action.frameProps.get('tabId')) - } - - if (action.isError || isSourceAboutUrl(action.frameProps.get('location'))) { - break - } - - let pageLoadEvent = Immutable.fromJS({ - timestamp: new Date().getTime(), - url: action.frameProps.get('location') - }) - eventState = eventState.set('page_load', eventState.get('page_load').push(pageLoadEvent)) - break case WindowConstants.WINDOW_SET_FOCUSED_FRAME: lastActiveTabId = action.frameProps.get('tabId') addPageView(action.frameProps.get('location'), lastActiveTabId) @@ -128,6 +113,30 @@ const doAction = (action) => { // retains all past pages, not really sure that's needed... [MTR] eventState = eventState.set('page_info', eventState.get('page_info').push(action.pageInfo)) break + case WindowConstants.WINDOW_GOT_RESPONSE_DETAILS: + // Only capture response for the page (not subresources, like images, JavaScript, etc) + if (action.details && action.details.get('resourceType') === 'mainFrame') { + const pageUrl = action.details.get('originalURL') || action.details.get('newURL') + + // create a page view event if this is a page load on the active tabId + if (!lastActiveTabId || action.tabId === lastActiveTabId) { + addPageView(pageUrl, action.tabId) + } + + const responseCode = action.details.get('httpResponseCode') + if (isSourceAboutUrl(pageUrl) || !shouldTrackResponseCode(responseCode)) { + break + } + + const pageLoadEvent = Immutable.fromJS({ + timestamp: new Date().getTime(), + url: pageUrl, + tabId: action.tabId, + details: action.details + }) + eventState = eventState.set('page_load', eventState.get('page_load').push(pageLoadEvent)) + } + break default: } diff --git a/test/unit/lib/ledgerUtilTest.js b/test/unit/lib/ledgerUtilTest.js new file mode 100644 index 00000000000..105f81e09d5 --- /dev/null +++ b/test/unit/lib/ledgerUtilTest.js @@ -0,0 +1,74 @@ +/* global describe, it */ +const ledgerUtil = require('../../../app/common/lib/ledgerUtil') +const assert = require('assert') + +require('../braveUnit') + +describe('ledgerUtil test', function () { + describe('shouldTrackResponseCode', function () { + describe('expected success codes', function () { + it('returns true for various success responses (200, 203, 206)', function () { + assert.equal(ledgerUtil.shouldTrackResponseCode(200), true) + assert.equal(ledgerUtil.shouldTrackResponseCode(203), true) + assert.equal(ledgerUtil.shouldTrackResponseCode(206), true) + }) + it('returns true for a cached response (304)', function () { + assert.equal(ledgerUtil.shouldTrackResponseCode(304), true) + }) + }) + + describe('expected failure codes', function () { + it('returns false for non-content success codes (used for REST apis, etc)', function () { + assert.equal(ledgerUtil.shouldTrackResponseCode(201), false) // created + assert.equal(ledgerUtil.shouldTrackResponseCode(202), false) // accepted + }) + it('returns false for various server side error responses (500-504)', function () { + assert.equal(ledgerUtil.shouldTrackResponseCode(500), false) // internal server error + assert.equal(ledgerUtil.shouldTrackResponseCode(501), false) // not implemented + assert.equal(ledgerUtil.shouldTrackResponseCode(502), false) // bad gateway + assert.equal(ledgerUtil.shouldTrackResponseCode(503), false) // service unavailable + assert.equal(ledgerUtil.shouldTrackResponseCode(504), false) // gateway timeout + }) + }) + }) + + describe('shouldTrackView', function () { + const validView = { tabId: 1, url: 'https://brave.com/' } + const validResponseList = [{ tabId: validView.tabId, details: { originalURL: validView.url, httpResponseCode: 200 } }] + const noMatchResponseList = [{ tabId: 3, details: {originalURL: 'https://not-brave.com'} }] + const matchButErrored = [{ tabId: validView.tabId, details: { originalURL: validView.url, httpResponseCode: 404 } }] + + describe('input validation', function () { + it('returns false if view is falsey', function () { + assert.equal(ledgerUtil.shouldTrackView(null, validResponseList), false) + }) + it('returns false if view.url is falsey', function () { + assert.equal(ledgerUtil.shouldTrackView({tabId: 1}, validResponseList), false) + }) + it('returns false if view.tabId is falsey', function () { + assert.equal(ledgerUtil.shouldTrackView({url: 'https://brave.com/'}, validResponseList), false) + }) + it('returns false if responseList is falsey', function () { + assert.equal(ledgerUtil.shouldTrackView(validView, null), false) + }) + it('returns false if responseList is not an array', function () { + assert.equal(ledgerUtil.shouldTrackView(validView, {}), false) + }) + it('returns false if responseList is a 0 length array', function () { + assert.equal(ledgerUtil.shouldTrackView(validView, []), false) + }) + }) + + describe('when finding a matching response based on tabId and url', function () { + it('returns false if no match found', function () { + assert.equal(ledgerUtil.shouldTrackView(validView, noMatchResponseList), false) + }) + it('returns false if match is found BUT response code is a failure (ex: 404)', function () { + assert.equal(ledgerUtil.shouldTrackView(validView, matchButErrored), false) + }) + it('returns true when match is found AND response code is a success (ex: 200)', function () { + assert.equal(ledgerUtil.shouldTrackView(validView, validResponseList), true) + }) + }) + }) +}) From d64e7ab48776883161432d032897264703a2c6aa Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 15 Sep 2016 20:27:01 -0700 Subject: [PATCH 2/3] Implemented review feedback Auditors: @bridiver Test Plan: --- .../common/lib/httpUtil.js | 16 +++++++++ app/common/lib/ledgerUtil.js | 20 ++--------- js/components/frame.js | 2 +- js/stores/eventStore.js | 6 ++-- js/stores/windowStore.js | 2 +- test/unit/lib/httpUtilTest.js | 34 +++++++++++++++++++ test/unit/lib/ledgerUtilTest.js | 33 ++---------------- 7 files changed, 61 insertions(+), 52 deletions(-) rename js/lib/errorUtil.js => app/common/lib/httpUtil.js (97%) create mode 100644 test/unit/lib/httpUtilTest.js diff --git a/js/lib/errorUtil.js b/app/common/lib/httpUtil.js similarity index 97% rename from js/lib/errorUtil.js rename to app/common/lib/httpUtil.js index 13bb8ac8605..dfa10c984c4 100644 --- a/js/lib/errorUtil.js +++ b/app/common/lib/httpUtil.js @@ -392,3 +392,19 @@ module.exports.errorMap = { // Failed to sort addresses according to RFC3484. 806: 'dnsSortError' } + +/** + * Returns true if HTTP response code is one we want to collect usage for + * @param {number} responseCode - HTTP response code to be evaluated + * @return {boolean} true if the code represents one w/ content, false if not + */ +module.exports.responseHasContent = (responseCode) => { + switch (responseCode) { + case 200: // ok + case 203: // non-authoritative + case 206: // partial content + case 304: // not modified (cached) + return true + } + return false +} diff --git a/app/common/lib/ledgerUtil.js b/app/common/lib/ledgerUtil.js index caa0a44ee22..5d53a5a3b58 100644 --- a/app/common/lib/ledgerUtil.js +++ b/app/common/lib/ledgerUtil.js @@ -4,21 +4,7 @@ 'use strict' -/** - * Returns true if HTTP response code is one we want to collect usage for - * @param {number} responseCode - HTTP response code to be evaluated - * @return {boolean} true if the page should be included, false if not - */ -module.exports.shouldTrackResponseCode = (responseCode) => { - switch (responseCode) { - case 200: // ok - case 203: // non-authoritative - case 206: // partial content - case 304: // not modified (cached) - return true - } - return false -} +const { responseHasContent } = require('./httpUtil') /** * Is page an actual page being viewed by the user? (not an error page, etc) @@ -44,11 +30,11 @@ module.exports.shouldTrackView = (view, responseList) => { if (!response) continue const responseUrl = response && response.details - ? response.details.originalURL || response.details.newURL + ? response.details.newURL : null if (url === responseUrl && response.tabId === tabId) { - return module.exports.shouldTrackResponseCode(response.details.httpResponseCode) + return responseHasContent(response.details.httpResponseCode) } } return false diff --git a/js/components/frame.js b/js/components/frame.js index 195828c2bb3..dca861b2b1d 100644 --- a/js/components/frame.js +++ b/js/components/frame.js @@ -24,7 +24,7 @@ const getSetting = require('../settings').getSetting const config = require('../constants/config') const settings = require('../constants/settings') const { aboutUrls, isSourceAboutUrl, isTargetAboutUrl, getTargetAboutUrl, getBaseUrl, isNavigatableAboutPage } = require('../lib/appUrlUtil') -const { isFrameError } = require('../lib/errorUtil') +const { isFrameError } = require('../../app/common/lib/httpUtil') const locale = require('../l10n') const appConfig = require('../constants/appConfig') const { getSiteSettingsForHostPattern } = require('../state/siteSettings') diff --git a/js/stores/eventStore.js b/js/stores/eventStore.js index b431a703c40..c0ec94b1f7d 100644 --- a/js/stores/eventStore.js +++ b/js/stores/eventStore.js @@ -10,7 +10,7 @@ const Immutable = require('immutable') const WindowConstants = require('../constants/windowConstants') const debounce = require('../lib/debounce.js') const { isSourceAboutUrl } = require('../lib/appUrlUtil') -const { shouldTrackResponseCode } = require('../../app/common/lib/ledgerUtil') +const { responseHasContent } = require('../../app/common/lib/httpUtil') const electron = require('electron') const BrowserWindow = electron.BrowserWindow @@ -116,7 +116,7 @@ const doAction = (action) => { case WindowConstants.WINDOW_GOT_RESPONSE_DETAILS: // Only capture response for the page (not subresources, like images, JavaScript, etc) if (action.details && action.details.get('resourceType') === 'mainFrame') { - const pageUrl = action.details.get('originalURL') || action.details.get('newURL') + const pageUrl = action.details.get('newURL') // create a page view event if this is a page load on the active tabId if (!lastActiveTabId || action.tabId === lastActiveTabId) { @@ -124,7 +124,7 @@ const doAction = (action) => { } const responseCode = action.details.get('httpResponseCode') - if (isSourceAboutUrl(pageUrl) || !shouldTrackResponseCode(responseCode)) { + if (isSourceAboutUrl(pageUrl) || !responseHasContent(responseCode)) { break } diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index e363f197e54..dc2a002ee2f 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -19,7 +19,7 @@ const urlParse = require('url').parse const currentWindow = require('../../app/renderer/currentWindow') const {tabFromFrame} = require('../state/frameStateUtil') -const { l10nErrorText } = require('../lib/errorUtil') +const { l10nErrorText } = require('../../app/common/lib/httpUtil') const { aboutUrls, getSourceAboutUrl, isIntermediateAboutPage, navigatableTypes } = require('../lib/appUrlUtil') const Serializer = require('../dispatcher/serializer') diff --git a/test/unit/lib/httpUtilTest.js b/test/unit/lib/httpUtilTest.js new file mode 100644 index 00000000000..7f1e772765e --- /dev/null +++ b/test/unit/lib/httpUtilTest.js @@ -0,0 +1,34 @@ +/* global describe, it */ +const httpUtil = require('../../../app/common/lib/httpUtil') +const assert = require('assert') + +require('../braveUnit') + +describe('httpUtil test', function () { + describe('responseHasContent', function () { + describe('expected success codes', function () { + it('returns true for various success responses (200, 203, 206)', function () { + assert.equal(httpUtil.responseHasContent(200), true) + assert.equal(httpUtil.responseHasContent(203), true) + assert.equal(httpUtil.responseHasContent(206), true) + }) + it('returns true for a cached response (304)', function () { + assert.equal(httpUtil.responseHasContent(304), true) + }) + }) + + describe('expected failure codes', function () { + it('returns false for non-content success codes (used for REST apis, etc)', function () { + assert.equal(httpUtil.responseHasContent(201), false) // created + assert.equal(httpUtil.responseHasContent(202), false) // accepted + }) + it('returns false for various server side error responses (500-504)', function () { + assert.equal(httpUtil.responseHasContent(500), false) // internal server error + assert.equal(httpUtil.responseHasContent(501), false) // not implemented + assert.equal(httpUtil.responseHasContent(502), false) // bad gateway + assert.equal(httpUtil.responseHasContent(503), false) // service unavailable + assert.equal(httpUtil.responseHasContent(504), false) // gateway timeout + }) + }) + }) +}) diff --git a/test/unit/lib/ledgerUtilTest.js b/test/unit/lib/ledgerUtilTest.js index 105f81e09d5..96fdc376c89 100644 --- a/test/unit/lib/ledgerUtilTest.js +++ b/test/unit/lib/ledgerUtilTest.js @@ -5,38 +5,11 @@ const assert = require('assert') require('../braveUnit') describe('ledgerUtil test', function () { - describe('shouldTrackResponseCode', function () { - describe('expected success codes', function () { - it('returns true for various success responses (200, 203, 206)', function () { - assert.equal(ledgerUtil.shouldTrackResponseCode(200), true) - assert.equal(ledgerUtil.shouldTrackResponseCode(203), true) - assert.equal(ledgerUtil.shouldTrackResponseCode(206), true) - }) - it('returns true for a cached response (304)', function () { - assert.equal(ledgerUtil.shouldTrackResponseCode(304), true) - }) - }) - - describe('expected failure codes', function () { - it('returns false for non-content success codes (used for REST apis, etc)', function () { - assert.equal(ledgerUtil.shouldTrackResponseCode(201), false) // created - assert.equal(ledgerUtil.shouldTrackResponseCode(202), false) // accepted - }) - it('returns false for various server side error responses (500-504)', function () { - assert.equal(ledgerUtil.shouldTrackResponseCode(500), false) // internal server error - assert.equal(ledgerUtil.shouldTrackResponseCode(501), false) // not implemented - assert.equal(ledgerUtil.shouldTrackResponseCode(502), false) // bad gateway - assert.equal(ledgerUtil.shouldTrackResponseCode(503), false) // service unavailable - assert.equal(ledgerUtil.shouldTrackResponseCode(504), false) // gateway timeout - }) - }) - }) - describe('shouldTrackView', function () { const validView = { tabId: 1, url: 'https://brave.com/' } - const validResponseList = [{ tabId: validView.tabId, details: { originalURL: validView.url, httpResponseCode: 200 } }] - const noMatchResponseList = [{ tabId: 3, details: {originalURL: 'https://not-brave.com'} }] - const matchButErrored = [{ tabId: validView.tabId, details: { originalURL: validView.url, httpResponseCode: 404 } }] + const validResponseList = [{ tabId: validView.tabId, details: { newURL: validView.url, httpResponseCode: 200 } }] + const noMatchResponseList = [{ tabId: 3, details: { newURL: 'https://not-brave.com' } }] + const matchButErrored = [{ tabId: validView.tabId, details: { newURL: validView.url, httpResponseCode: 404 } }] describe('input validation', function () { it('returns false if view is falsey', function () { From 6fbf58dc6ea1217bab19698d81cfe20dad3d6ce6 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Thu, 15 Sep 2016 20:38:51 -0700 Subject: [PATCH 3/3] sneak in some unit tests before merge :) --- test/unit/lib/httpUtilTest.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/unit/lib/httpUtilTest.js b/test/unit/lib/httpUtilTest.js index 7f1e772765e..7b1fe40299d 100644 --- a/test/unit/lib/httpUtilTest.js +++ b/test/unit/lib/httpUtilTest.js @@ -22,6 +22,12 @@ describe('httpUtil test', function () { assert.equal(httpUtil.responseHasContent(201), false) // created assert.equal(httpUtil.responseHasContent(202), false) // accepted }) + it('returns false for various client error responses (400+)', function () { + assert.equal(httpUtil.responseHasContent(400), false) // bad request + assert.equal(httpUtil.responseHasContent(401), false) // unauthorized + assert.equal(httpUtil.responseHasContent(403), false) // forbidden + assert.equal(httpUtil.responseHasContent(404), false) // not found + }) it('returns false for various server side error responses (500-504)', function () { assert.equal(httpUtil.responseHasContent(500), false) // internal server error assert.equal(httpUtil.responseHasContent(501), false) // not implemented