From 017574be143532436ff5ac5506a3461d2ddc1dcc Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Fri, 11 Jan 2019 15:47:38 -0800 Subject: [PATCH] core(driver): exit early when testing page with insecure certificate (#6608) previously tests of pages with insecure certificates ran until the page load timeout --- clients/extension/scripts/popup.js | 2 + lighthouse-cli/run.js | 9 ++ .../test/smokehouse/error-expectations.js | 6 + lighthouse-cli/test/smokehouse/smokehouse.js | 9 +- lighthouse-core/gather/driver.js | 117 ++++++++++++------ lighthouse-core/gather/gather-runner.js | 20 --- lighthouse-core/lib/i18n/en-US.json | 4 +- lighthouse-core/lib/lh-error.js | 4 +- lighthouse-core/test/gather/driver-test.js | 94 +++++++++++++- lighthouse-core/test/gather/fake-driver.js | 8 -- .../test/gather/gather-runner-test.js | 40 ------ 11 files changed, 194 insertions(+), 119 deletions(-) diff --git a/clients/extension/scripts/popup.js b/clients/extension/scripts/popup.js index d49741747b07..9676a8f4a807 100644 --- a/clients/extension/scripts/popup.js +++ b/clients/extension/scripts/popup.js @@ -26,6 +26,8 @@ const NON_BUG_ERROR_MESSAGES = { 'are trying to review.', 'Cannot access contents of the page': 'Lighthouse can only audit URLs that start' + ' with http:// or https://.', + 'INSECURE_DOCUMENT_REQUEST': 'The URL you have provided does not have valid' + + ' security credentials.', 'INVALID_URL': 'Lighthouse can only audit URLs that start' + ' with http:// or https://.', }; diff --git a/lighthouse-cli/run.js b/lighthouse-cli/run.js index a602edc7b4f3..28081d5e3c1a 100644 --- a/lighthouse-cli/run.js +++ b/lighthouse-cli/run.js @@ -23,6 +23,7 @@ const opn = require('opn'); const _RUNTIME_ERROR_CODE = 1; const _PROTOCOL_TIMEOUT_EXIT_CODE = 67; const _PAGE_HUNG_EXIT_CODE = 68; +const _INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69; /** * exported for testing @@ -77,6 +78,12 @@ function showPageHungError(err) { process.exit(_PAGE_HUNG_EXIT_CODE); } +/** @param {LH.LighthouseError} err */ +function showInsecureDocumentRequestError(err) { + console.error('Insecure document request:', err.friendlyMessage); + process.exit(_INSECURE_DOCUMENT_REQUEST_EXIT_CODE); +} + /** * @param {LH.LighthouseError} err */ @@ -98,6 +105,8 @@ function handleError(err) { showProtocolTimeoutError(); } else if (err.code === 'PAGE_HUNG') { showPageHungError(err); + } else if (err.code === 'INSECURE_DOCUMENT_REQUEST') { + showInsecureDocumentRequestError(err); } else { showRuntimeError(err); } diff --git a/lighthouse-cli/test/smokehouse/error-expectations.js b/lighthouse-cli/test/smokehouse/error-expectations.js index 40d05780607b..8d1a59290f96 100644 --- a/lighthouse-cli/test/smokehouse/error-expectations.js +++ b/lighthouse-cli/test/smokehouse/error-expectations.js @@ -15,4 +15,10 @@ module.exports = [ errorCode: 'PAGE_HUNG', audits: {}, }, + { + requestedUrl: 'https://expired.badssl.com', + finalUrl: 'https://expired.badssl.com', + errorCode: 'INSECURE_DOCUMENT_REQUEST', + audits: {}, + }, ]; diff --git a/lighthouse-cli/test/smokehouse/smokehouse.js b/lighthouse-cli/test/smokehouse/smokehouse.js index 603d71aa04b6..4a304496e891 100755 --- a/lighthouse-cli/test/smokehouse/smokehouse.js +++ b/lighthouse-cli/test/smokehouse/smokehouse.js @@ -32,6 +32,7 @@ const log = require('lighthouse-logger'); const PROTOCOL_TIMEOUT_EXIT_CODE = 67; const PAGE_HUNG_EXIT_CODE = 68; +const INSECURE_DOCUMENT_REQUEST_EXIT_CODE = 69; const RETRIES = 3; const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/; @@ -103,7 +104,9 @@ function runLighthouse(url, configPath, isDebug) { if (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE) { console.error(`Lighthouse debugger connection timed out ${RETRIES} times. Giving up.`); process.exit(1); - } else if (runResults.status !== 0 && runResults.status !== PAGE_HUNG_EXIT_CODE) { + } else if (runResults.status !== 0 + && runResults.status !== PAGE_HUNG_EXIT_CODE + && runResults.status !== INSECURE_DOCUMENT_REQUEST_EXIT_CODE) { console.error(`Lighthouse run failed with exit code ${runResults.status}. stderr to follow:`); console.error(runResults.stderr); process.exit(runResults.status); @@ -118,6 +121,10 @@ function runLighthouse(url, configPath, isDebug) { return {requestedUrl: url, finalUrl: url, errorCode: 'PAGE_HUNG', audits: {}}; } + if (runResults.status === INSECURE_DOCUMENT_REQUEST_EXIT_CODE) { + return {requestedUrl: url, finalUrl: url, errorCode: 'INSECURE_DOCUMENT_REQUEST', audits: {}}; + } + const lhr = fs.readFileSync(outputPath, 'utf8'); if (isDebug) { console.log('LHR output available at: ', outputPath); diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 27674132f039..a831effb9e82 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -81,13 +81,6 @@ class Driver { this._eventEmitter.emit(event.method, event.params); }); - /** - * Used for monitoring network status events during gotoURL. - * @type {?LH.Crdp.Security.SecurityStateChangedEvent} - * @private - */ - this._lastSecurityState = null; - /** * @type {number} * @private @@ -546,14 +539,17 @@ class Driver { /** @param {LH.Crdp.Page.LifecycleEventEvent} e */ const lifecycleListener = e => { if (e.name === 'firstContentfulPaint') { + cancel(); resolve(); - this.off('Page.lifecycleEvent', lifecycleListener); } }; this.on('Page.lifecycleEvent', lifecycleListener); + let canceled = false; cancel = () => { + if (canceled) return; + canceled = true; this.off('Page.lifecycleEvent', lifecycleListener); }; }); @@ -634,7 +630,10 @@ class Driver { networkStatusMonitor.on('network-2-busy', logStatus); this.once('Page.domContentEventFired', domContentLoadedListener); + let canceled = false; cancel = () => { + if (canceled) return; + canceled = true; idleTimeout && clearTimeout(idleTimeout); this.off('Page.domContentEventFired', domContentLoadedListener); networkStatusMonitor.removeListener('network-2-busy', onBusy); @@ -666,7 +665,7 @@ class Driver { /** @type {NodeJS.Timer|undefined} */ let lastTimeout; - let cancelled = false; + let canceled = false; const checkForQuietExpression = `(${pageFunctions.checkTimeSinceLastLongTaskString})()`; /** @@ -675,9 +674,9 @@ class Driver { * @return {Promise} */ async function checkForQuiet(driver, resolve) { - if (cancelled) return; + if (canceled) return; const timeSinceLongTask = await driver.evaluateAsync(checkForQuietExpression); - if (cancelled) return; + if (canceled) return; if (typeof timeSinceLongTask === 'number') { if (timeSinceLongTask >= waitForCPUQuiet) { @@ -698,9 +697,10 @@ class Driver { const promise = new Promise((resolve, reject) => { checkForQuiet(this, resolve).catch(reject); cancel = () => { - cancelled = true; + if (canceled) return; + canceled = true; if (lastTimeout) clearTimeout(lastTimeout); - reject(new Error('Wait for CPU idle cancelled')); + reject(new Error('Wait for CPU idle canceled')); }; }); @@ -731,7 +731,10 @@ class Driver { }; this.once('Page.loadEventFired', loadListener); + let canceled = false; cancel = () => { + if (canceled) return; + canceled = true; this.off('Page.loadEventFired', loadListener); loadTimeout && clearTimeout(loadTimeout); }; @@ -743,6 +746,49 @@ class Driver { }; } + /** + * Return a promise that resolves when an insecure security state is encountered + * and a method to cancel internal listeners. + * @return {{promise: Promise, cancel: function(): void}} + * @private + */ + _monitorForInsecureState() { + /** @type {(() => void)} */ + let cancel = () => { + throw new Error('_monitorForInsecureState.cancel() called before it was defined'); + }; + + const promise = new Promise((resolve, reject) => { + /** + * @param {LH.Crdp.Security.SecurityStateChangedEvent} event + */ + const securityStateChangedListener = ({securityState, explanations}) => { + if (securityState === 'insecure') { + cancel(); + const insecureDescriptions = explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + resolve(insecureDescriptions.join(' ')); + } + }; + let canceled = false; + cancel = () => { + if (canceled) return; + canceled = true; + this.off('Security.securityStateChanged', securityStateChangedListener); + // TODO(@patrickhulce): cancel() should really be a promise itself to handle things like this + this.sendCommand('Security.disable').catch(() => {}); + }; + this.on('Security.securityStateChanged', securityStateChangedListener); + this.sendCommand('Security.enable').catch(() => {}); + }); + + return { + promise, + cancel, + }; + } + /** * Returns whether the page appears to be hung. * @return {Promise} @@ -765,6 +811,7 @@ class Driver { /** * Returns a promise that resolves when: * - All of the following conditions have been met: + * - page has no security issues * - pauseAfterLoadMs milliseconds have passed since the load event. * - networkQuietThresholdMs milliseconds have passed since the last network request that exceeded * 2 inflight requests (network-2-quiet has been reached). @@ -793,6 +840,13 @@ class Driver { // CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle. let waitForCPUIdle = this._waitForNothing(); + const monitorForInsecureState = this._monitorForInsecureState(); + const securityCheckPromise = monitorForInsecureState.promise.then(securityMessages => { + return function() { + throw new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages}); + }; + }); + // Wait for both load promises. Resolves on cleanup function the clears load // timeout timer. const loadPromise = Promise.all([ @@ -805,7 +859,6 @@ class Driver { }).then(() => { return function() { log.verbose('Driver', 'loadEventFired and network considered idle'); - maxTimeoutHandle && clearTimeout(maxTimeoutHandle); }; }); @@ -816,11 +869,6 @@ class Driver { }).then(_ => { return async () => { log.warn('Driver', 'Timed out waiting for page load. Checking if page is hung...'); - waitForFCP.cancel(); - waitForLoadEvent.cancel(); - waitForNetworkIdle.cancel(); - waitForCPUIdle.cancel(); - if (await this.isPageHung()) { log.warn('Driver', 'Page appears to be hung, killing JavaScript...'); await this.sendCommand('Emulation.setScriptExecutionDisabled', {value: true}); @@ -830,11 +878,20 @@ class Driver { }; }); - // Wait for load or timeout and run the cleanup function the winner returns. + // Wait for security issue, load or timeout and run the cleanup function the winner returns. const cleanupFn = await Promise.race([ + securityCheckPromise, loadPromise, maxTimeoutPromise, ]); + + maxTimeoutHandle && clearTimeout(maxTimeoutHandle); + waitForFCP.cancel(); + waitForLoadEvent.cancel(); + waitForNetworkIdle.cancel(); + waitForCPUIdle.cancel(); + monitorForInsecureState.cancel(); + await cleanupFn(); } @@ -1007,26 +1064,6 @@ class Driver { return result.body; } - async listenForSecurityStateChanges() { - this.on('Security.securityStateChanged', state => { - this._lastSecurityState = state; - }); - await this.sendCommand('Security.enable'); - } - - /** - * @return {LH.Crdp.Security.SecurityStateChangedEvent} - */ - getSecurityState() { - if (!this._lastSecurityState) { - // happens if 'listenForSecurityStateChanges' is not called, - // or if some assumptions about the Security domain are wrong - throw new Error('Expected a security state.'); - } - - return this._lastSecurityState; - } - /** * @param {string} name The name of API whose permission you wish to query * @return {Promise} The state of permissions, resolved in a promise. diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 6c02edb44cac..53a61a91ce1f 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -110,7 +110,6 @@ class GatherRunner { await driver.cacheNatives(); await driver.registerPerformanceObserver(); await driver.dismissJavaScriptDialogs(); - await driver.listenForSecurityStateChanges(); if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); log.timeEnd(status); } @@ -173,23 +172,6 @@ class GatherRunner { } } - /** - * Throws an error if the security state is insecure. - * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState - * @throws {LHError} - */ - static assertNoSecurityIssues({securityState, explanations}) { - if (securityState === 'insecure') { - const insecureDescriptions = explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description); - throw new LHError( - LHError.errors.INSECURE_DOCUMENT_REQUEST, - {securityMessages: insecureDescriptions.join(' ')} - ); - } - } - /** * Calls beforePass() on gatherers before tracing * has started and before navigation to the target page. @@ -312,8 +294,6 @@ class GatherRunner { const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); log.timeEnd(status); - this.assertNoSecurityIssues(driver.getSecurityState()); - let pageLoadError = GatherRunner.getPageLoadError(passContext.url, networkRecords); // If the driver was offline, a page load error is expected, so do not save it. if (!driver.online) pageLoadError = undefined; diff --git a/lighthouse-core/lib/i18n/en-US.json b/lighthouse-core/lib/i18n/en-US.json index b5bf428090d2..3183629ed91a 100644 --- a/lighthouse-core/lib/i18n/en-US.json +++ b/lighthouse-core/lib/i18n/en-US.json @@ -992,8 +992,8 @@ "description": "Error message explaining that Lighthouse couldn't complete because the page has stopped responding to its instructions." }, "lighthouse-core/lib/lh-error.js | pageLoadFailedInsecure": { - "message": "The URL you have provided does not have valid security credentials. ({securityMessages})", - "description": "Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed." + "message": "The URL you have provided does not have valid security credentials. {securityMessages}", + "description": "Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. securityMessages will be replaced with one or more strings from the browser explaining what was insecure about the page load." }, "lighthouse-core/lib/lh-error.js | pageLoadFailedWithDetails": { "message": "Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Details: {errorDetails})", diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js index df1b3f4069b3..d1ef25f2c99e 100644 --- a/lighthouse-core/lib/lh-error.js +++ b/lighthouse-core/lib/lh-error.js @@ -21,8 +21,8 @@ const UIStrings = { pageLoadFailedWithStatusCode: 'Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: {statusCode})', /** Error message explaining that Lighthouse could not load the requested URL and the steps that might be taken to fix the unreliability. */ pageLoadFailedWithDetails: 'Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Details: {errorDetails})', - /** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. */ - pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. ({securityMessages})', + /** Error message explaining that the credentials included in the Lighthouse run were invalid, so the URL cannot be accessed. securityMessages will be replaced with one or more strings from the browser explaining what was insecure about the page load. */ + pageLoadFailedInsecure: 'The URL you have provided does not have valid security credentials. {securityMessages}', /** Error message explaining that Chrome has encountered an error during the Lighthouse run, and that Chrome should be restarted. */ internalChromeError: 'An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.', /** Error message explaining that fetching the resources of the webpage has taken longer than the maximum time. */ diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 3229755d6533..0030b03e29a9 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -15,16 +15,15 @@ const assert = require('assert'); const EventEmitter = require('events').EventEmitter; const {protocolGetVersionResponse} = require('./fake-driver'); -const connection = new Connection(); -const driverStub = new Driver(connection); - const redirectDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog.json'); const MAX_WAIT_FOR_PROTOCOL = 20; function createOnceStub(events) { return (eventName, cb) => { if (events[eventName]) { - return cb(events[eventName]); + // wait a tick b/c real events never fire immediately + setTimeout(_ => cb(events[eventName]), 0); + return; } throw Error(`Stub not implemented: ${eventName}`); @@ -53,7 +52,7 @@ function createOnceMethodResponse(method, response) { sendCommandMockResponses.set(method, response); } -connection.sendCommand = function(command, params) { +function sendCommandStub(command, params) { sendCommandParams.push({command, params}); if (sendCommandMockResponses.has(command)) { @@ -95,23 +94,35 @@ connection.sendCommand = function(command, params) { case 'Network.getResponseBody': return new Promise(res => setTimeout(res, MAX_WAIT_FOR_PROTOCOL + 20)); case 'Page.enable': + case 'Page.navigate': + case 'Page.setLifecycleEventsEnabled': case 'Network.enable': case 'Tracing.start': case 'ServiceWorker.enable': case 'ServiceWorker.disable': + case 'Security.enable': + case 'Security.disable': case 'Network.setExtraHTTPHeaders': case 'Network.emulateNetworkConditions': case 'Emulation.setCPUThrottlingRate': + case 'Emulation.setScriptExecutionDisabled': return Promise.resolve({}); case 'Tracing.end': return Promise.reject(new Error('tracing not started')); default: throw Error(`Stub not implemented: ${command}`); } -}; +} /* eslint-env jest */ +let driverStub; +beforeEach(() => { + const connection = new Connection(); + connection.sendCommand = sendCommandStub; + driverStub = new Driver(connection); +}); + describe('Browser Driver', () => { beforeEach(() => { sendCommandParams = []; @@ -535,4 +546,75 @@ describe('Multiple tab check', () => { }); }); }); + + describe('Security check', () => { + it('does not reject when page is secure', async () => { + const secureSecurityState = { + explanations: [], + securityState: 'secure', + }; + driverStub.on = driverStub.once = createOnceStub({ + 'Security.securityStateChanged': secureSecurityState, + 'Page.loadEventFired': {}, + 'Page.domContentEventFired': {}, + }); + + const startUrl = 'https://www.example.com'; + const loadOptions = { + waitForLoad: true, + passContext: { + passConfig: { + networkQuietThresholdMs: 1, + }, + settings: { + maxWaitForLoad: 1, + }, + }, + }; + await driverStub.gotoURL(startUrl, loadOptions); + }); + + it('rejects when page is insecure', async () => { + const insecureSecurityState = { + explanations: [ + { + description: 'reason 1.', + securityState: 'insecure', + }, + { + description: 'blah.', + securityState: 'info', + }, + { + description: 'reason 2.', + securityState: 'insecure', + }, + ], + securityState: 'insecure', + }; + driverStub.on = createOnceStub({ + 'Security.securityStateChanged': insecureSecurityState, + }); + + const startUrl = 'https://www.example.com'; + const loadOptions = { + waitForLoad: true, + passContext: { + passConfig: { + networkQuietThresholdMs: 1, + }, + }, + }; + + try { + await driverStub.gotoURL(startUrl, loadOptions); + assert.fail('security check should have rejected'); + } catch (err) { + assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); + assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST'); + /* eslint-disable-next-line max-len */ + expect(err.friendlyMessage).toBeDisplayString('The URL you have provided does not have valid security credentials. reason 1. reason 2.'); + } + }); + }); }); diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js index 75ce1e2f3eca..f90ed3b0f032 100644 --- a/lighthouse-core/test/gather/fake-driver.js +++ b/lighthouse-core/test/gather/fake-driver.js @@ -81,14 +81,6 @@ const fakeDriver = { setExtraHTTPHeaders() { return Promise.resolve(); }, - listenForSecurityStateChanges() { - return Promise.resolve(); - }, - getSecurityState() { - return Promise.resolve({ - securityState: 'secure', - }); - }, }; module.exports = fakeDriver; diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 68c2076e109b..ae3fcd43bd6b 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -322,7 +322,6 @@ describe('GatherRunner', function() { clearDataForOrigin: createCheck('calledClearStorage'), blockUrlPatterns: asyncFunc, setExtraHTTPHeaders: asyncFunc, - listenForSecurityStateChanges: asyncFunc, }; return GatherRunner.setupDriver(driver, {settings: {}}).then(_ => { @@ -382,7 +381,6 @@ describe('GatherRunner', function() { clearDataForOrigin: createCheck('calledClearStorage'), blockUrlPatterns: asyncFunc, setExtraHTTPHeaders: asyncFunc, - listenForSecurityStateChanges: asyncFunc, }; return GatherRunner.setupDriver(driver, { @@ -696,44 +694,6 @@ describe('GatherRunner', function() { }); }); - describe('#assertNoSecurityIssues', () => { - it('succeeds when page is secure', () => { - const secureSecurityState = { - securityState: 'secure', - }; - GatherRunner.assertNoSecurityIssues(secureSecurityState); - }); - - it('fails when page is insecure', () => { - const insecureSecurityState = { - explanations: [ - { - description: 'reason 1', - securityState: 'insecure', - }, - { - description: 'blah.', - securityState: 'info', - }, - { - description: 'reason 2', - securityState: 'insecure', - }, - ], - securityState: 'insecure', - }; - try { - GatherRunner.assertNoSecurityIssues(insecureSecurityState); - assert.fail('expected INSECURE_DOCUMENT_REQUEST LHError'); - } catch (err) { - assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); - assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST'); - expect(err.friendlyMessage) - .toBeDisplayString(/The URL.*security credentials.*reason 1 reason 2/); - } - }); - }); - describe('artifact collection', () => { // Make sure our gatherers never execute in parallel it('runs gatherer lifecycle methods strictly in sequence', async () => {