Don't register a visit if page returns a non-successful HTTP status code

Fixes brave#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

@@ -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
+}

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)
+  }
 })
 
 /*

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
+    })
   }
 }
 

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))
   }

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)

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:
   }
 

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)
+      })
+    })
+  })
+})