From 104e399d2f38fb9c2e4742bd75af1500dd483f08 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 16 Oct 2018 17:41:07 -0700 Subject: [PATCH 01/13] core: bail if encounter insecure ssl cert, to avoid hanging forever (#6287) --- lighthouse-core/gather/driver.js | 18 +++++ lighthouse-core/gather/gather-runner.js | 42 +++++++++-- lighthouse-core/lib/lh-error.js | 12 +++ lighthouse-core/lib/strings.js | 2 + lighthouse-core/test/gather/fake-driver.js | 5 ++ .../test/gather/gather-runner-test.js | 73 ++++++++++++++----- 6 files changed, 124 insertions(+), 28 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index bb80ba984dc5..4ad347976681 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -849,6 +849,24 @@ class Driver { }); } + /** + * @param {number} [timeout] + * @return {Promise} + */ + getSecurityState(timeout = 1000) { + return new Promise((resolve, reject) => { + const err = new LHError(LHError.errors.SECURITY_STATE_TIMEOUT); + const asyncTimeout = setTimeout((_ => reject(err)), timeout); + + this.sendCommand('Security.enable'); + this.once('Security.securityStateChanged', (e) => { + clearTimeout(asyncTimeout); + resolve(e); + this.sendCommand('Security.disable').catch(reject); + }); + }); + } + /** * @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 2d045fcf966e..498a8efd0aba 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -146,11 +146,12 @@ class GatherRunner { /** * Returns an error if the original network request failed or wasn't found. + * @param {Driver} driver * @param {string} url The URL of the original requested page. * @param {Array} networkRecords - * @return {LHError|undefined} + * @return {Promise} */ - static getPageLoadError(url, networkRecords) { + static async getPageLoadError(driver, url, networkRecords) { const mainRecord = networkRecords.find(record => { // record.url is actual request url, so needs to be compared without any URL fragment. return URL.equalWithExcludedFragments(record.url, url); @@ -165,6 +166,20 @@ class GatherRunner { } else if (mainRecord.hasErrorStatusCode()) { errorDef = {...LHError.errors.ERRORED_DOCUMENT_REQUEST}; errorDef.message += ` Status code: ${mainRecord.statusCode}.`; + } else if (!mainRecord.finished) { + // could be security error + const securityState = await driver.getSecurityState(); + if (securityState.securityState === 'insecure') { + errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; + const insecureDescriptions = securityState.explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description) + .join(' '); + errorDef.message += ` Insecure: ${insecureDescriptions}`; + } else { + // Not sure what the error is. Could be just a redirect. For now, + // treat as no error. + } } if (errorDef) { @@ -251,7 +266,7 @@ class GatherRunner { * object containing trace and network data. * @param {LH.Gatherer.PassContext} passContext * @param {Partial} gathererResults - * @return {Promise} + * @return {Promise<[LH.Gatherer.LoadData, LH.LighthouseError | undefined]>} */ static async afterPass(passContext, gathererResults) { const driver = passContext.driver; @@ -271,7 +286,8 @@ class GatherRunner { const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); log.verbose('statusEnd', status); - let pageLoadError = GatherRunner.getPageLoadError(passContext.url, networkRecords); + let pageLoadError = await GatherRunner.getPageLoadError(driver, + passContext.url, networkRecords); // If the driver was offline, a page load error is expected, so do not save it. if (!driver.online) pageLoadError = undefined; @@ -288,8 +304,12 @@ class GatherRunner { trace, }; - // Disable throttling so the afterPass analysis isn't throttled - await driver.setThrottling(passContext.settings, {useThrottling: false}); + if (!pageLoadError) { + // Disable throttling so the afterPass analysis isn't throttled + // This will hang if there was a security error. But, there is no + // need to throttle if there is such an error. See #6287 + await driver.setThrottling(passContext.settings, {useThrottling: false}); + } for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; @@ -313,7 +333,7 @@ class GatherRunner { } // Resolve on tracing data using passName from config. - return passData; + return [passData, pageLoadError]; } /** @@ -410,7 +430,8 @@ class GatherRunner { await driver.setThrottling(options.settings, passConfig); await GatherRunner.beforePass(passContext, gathererResults); await GatherRunner.pass(passContext, gathererResults); - const passData = await GatherRunner.afterPass(passContext, gathererResults); + const [passData, pageLoadError] = + await GatherRunner.afterPass(passContext, gathererResults); // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. baseArtifacts.devtoolsLogs[passConfig.passName] = passData.devtoolsLog; @@ -435,6 +456,11 @@ class GatherRunner { baseArtifacts.URL.finalUrl = passContext.url; firstPass = false; } + + if (pageLoadError && pageLoadError.code === LHError.errors.INSECURE_DOCUMENT_REQUEST.code) { + // Some protocol commands will hang, so let's just bail. See #6287 + break; + } } const resetStorage = !options.settings.disableStorageReset; if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js index 557fcc746d53..4428c7759f60 100644 --- a/lighthouse-core/lib/lh-error.js +++ b/lighthouse-core/lib/lh-error.js @@ -142,6 +142,12 @@ const ERRORS = { message: strings.pageLoadFailed, lhrRuntimeError: true, }, + /* Used when security error prevents page load. */ + INSECURE_DOCUMENT_REQUEST: { + code: 'INSECURE_DOCUMENT_REQUEST', + message: strings.pageLoadFailedInsecure, + lhrRuntimeError: true, + }, // Protocol internal failures TRACING_ALREADY_STARTED: { @@ -169,6 +175,12 @@ const ERRORS = { message: strings.requestContentTimeout, }, + // Protocol timeout failures + SECURITY_STATE_TIMEOUT: { + code: 'SECURITY_STATE_TIMEOUT', + message: strings.securityStateTimeout, + }, + // URL parsing failures INVALID_URL: { code: 'INVALID_URL', diff --git a/lighthouse-core/lib/strings.js b/lighthouse-core/lib/strings.js index 24bf4d91cd71..5b31da3d0be9 100644 --- a/lighthouse-core/lib/strings.js +++ b/lighthouse-core/lib/strings.js @@ -11,7 +11,9 @@ module.exports = { badTraceRecording: `Something went wrong with recording the trace over your page load. Please run Lighthouse again.`, pageLoadTookTooLong: `Your page took too long to load. Please follow the opportunities in the report to reduce your page load time, and then try re-running Lighthouse.`, pageLoadFailed: `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.`, + pageLoadFailedInsecure: `The URL you have provided does not have a valid security certificate.`, internalChromeError: `An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.`, requestContentTimeout: 'Fetching resource content has exceeded the allotted time', + securityStateTimeout: 'Fetching security state has exceeded the allotted time', urlInvalid: `The URL you have provided appears to be invalid.`, }; diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js index f90ed3b0f032..0a4d259377e1 100644 --- a/lighthouse-core/test/gather/fake-driver.js +++ b/lighthouse-core/test/gather/fake-driver.js @@ -81,6 +81,11 @@ const fakeDriver = { setExtraHTTPHeaders() { 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 8ab6da53e655..33027a00eb94 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -493,10 +493,11 @@ describe('GatherRunner', function() { ], }; - return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}).then(passData => { - assert.equal(calledTrace, true); - assert.equal(passData.trace, fakeTraceData); - }); + return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}) + .then(([passData]) => { + assert.equal(calledTrace, true); + assert.equal(passData.trace, fakeTraceData); + }); }); it('tells the driver to begin devtoolsLog collection', () => { @@ -543,10 +544,11 @@ describe('GatherRunner', function() { ], }; - return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}).then(vals => { - assert.equal(calledDevtoolsLogCollect, true); - assert.strictEqual(vals.devtoolsLog[0], fakeDevtoolsMessage); - }); + return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}) + .then(([passData]) => { + assert.equal(calledDevtoolsLogCollect, true); + assert.strictEqual(passData.devtoolsLog[0], fakeDevtoolsMessage); + }); }); it('does as many passes as are required', () => { @@ -625,58 +627,89 @@ describe('GatherRunner', function() { }); describe('#getPageLoadError', () => { - it('passes when the page is loaded', () => { + it('passes when the page is loaded', async () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; - assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord])); + assert.ok(!await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord])); }); - it('passes when the page is loaded, ignoring any fragment', () => { + it('passes when the page is loaded, ignoring any fragment', async () => { const url = 'http://example.com/#/page/list'; const mainRecord = new NetworkRequest(); mainRecord.url = 'http://example.com'; - assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord])); + assert.ok(!await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord])); }); - it('fails when page fails to load', () => { + it('fails when page fails to load', async () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.failed = true; mainRecord.localizedFailDescription = 'foobar'; - const error = GatherRunner.getPageLoadError(url, [mainRecord]); + const error = await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord]); assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); - it('fails when page times out', () => { + it('fails when page times out', async () => { const url = 'http://the-page.com'; const records = []; - const error = GatherRunner.getPageLoadError(url, records); + const error = await GatherRunner.getPageLoadError(fakeDriver, url, records); assert.equal(error.message, 'NO_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); - it('fails when page returns with a 404', () => { + it('fails when page returns with a 404', async () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 404; - const error = GatherRunner.getPageLoadError(url, [mainRecord]); + const error = await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); - it('fails when page returns with a 500', () => { + it('fails when page returns with a 500', async () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 500; - const error = GatherRunner.getPageLoadError(url, [mainRecord]); + const error = await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); + + it('fails when page is insecure', async () => { + const mockDriver = Object.assign({}, fakeDriver, { + async getSecurityState() { + return { + explanations: [ + { + description: 'reason 1.', + securityState: 'insecure', + }, + { + description: 'blah.', + securityState: 'info', + }, + { + description: 'reason 2.', + securityState: 'insecure', + }, + ], + securityState: 'insecure', + }; + }, + }); + const url = 'http://the-page.com'; + const mainRecord = new NetworkRequest(); + mainRecord.url = url; + const error = await GatherRunner.getPageLoadError(mockDriver, url, [mainRecord]); + assert.equal(error.message, 'INSECURE_DOCUMENT_REQUEST'); + /* eslint-disable-next-line max-len */ + assert.equal(error.friendlyMessage, 'The URL you have provided does not have a valid security certificate. Insecure: reason 1. reason 2.'); + }); }); describe('artifact collection', () => { From 2eafa4434f2d6198065a359464dfc237d000f54e Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 16 Oct 2018 19:18:31 -0700 Subject: [PATCH 02/13] simplify getPageLoadError, edit strings --- lighthouse-core/gather/gather-runner.js | 31 ++++---- lighthouse-core/lib/strings.js | 2 +- .../test/gather/gather-runner-test.js | 70 +++++++++---------- 3 files changed, 48 insertions(+), 55 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 498a8efd0aba..a0e28b95a88c 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -146,12 +146,12 @@ class GatherRunner { /** * Returns an error if the original network request failed or wasn't found. - * @param {Driver} driver * @param {string} url The URL of the original requested page. * @param {Array} networkRecords - * @return {Promise} + * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState + * @return {LHError|undefined} */ - static async getPageLoadError(driver, url, networkRecords) { + static getPageLoadError(url, networkRecords, securityState) { const mainRecord = networkRecords.find(record => { // record.url is actual request url, so needs to be compared without any URL fragment. return URL.equalWithExcludedFragments(record.url, url); @@ -166,20 +166,12 @@ class GatherRunner { } else if (mainRecord.hasErrorStatusCode()) { errorDef = {...LHError.errors.ERRORED_DOCUMENT_REQUEST}; errorDef.message += ` Status code: ${mainRecord.statusCode}.`; - } else if (!mainRecord.finished) { - // could be security error - const securityState = await driver.getSecurityState(); - if (securityState.securityState === 'insecure') { - errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; - const insecureDescriptions = securityState.explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description) - .join(' '); - errorDef.message += ` Insecure: ${insecureDescriptions}`; - } else { - // Not sure what the error is. Could be just a redirect. For now, - // treat as no error. - } + } else if (securityState.securityState === 'insecure') { + errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; + const insecureDescriptions = securityState.explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + errorDef.message += ` ${insecureDescriptions.join(' ')}`; } if (errorDef) { @@ -286,8 +278,9 @@ class GatherRunner { const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); log.verbose('statusEnd', status); - let pageLoadError = await GatherRunner.getPageLoadError(driver, - passContext.url, networkRecords); + const securityState = await driver.getSecurityState(); + let pageLoadError = GatherRunner.getPageLoadError(passContext.url, + networkRecords, securityState); // 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/strings.js b/lighthouse-core/lib/strings.js index 5b31da3d0be9..de2697495226 100644 --- a/lighthouse-core/lib/strings.js +++ b/lighthouse-core/lib/strings.js @@ -11,7 +11,7 @@ module.exports = { badTraceRecording: `Something went wrong with recording the trace over your page load. Please run Lighthouse again.`, pageLoadTookTooLong: `Your page took too long to load. Please follow the opportunities in the report to reduce your page load time, and then try re-running Lighthouse.`, pageLoadFailed: `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.`, - pageLoadFailedInsecure: `The URL you have provided does not have a valid security certificate.`, + pageLoadFailedInsecure: `The URL you have provided does not have valid security credentials.`, internalChromeError: `An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.`, requestContentTimeout: 'Fetching resource content has exceeded the allotted time', securityStateTimeout: 'Fetching security state has exceeded the allotted time', diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 33027a00eb94..037b90fde6cd 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -34,6 +34,10 @@ class TestGathererNoArtifact extends Gatherer { const fakeDriver = require('./fake-driver'); +const secureSecurityState = { + securityState: 'secure', +}; + function getMockedEmulationDriver(emulationFn, netThrottleFn, cpuThrottleFn, blockUrlFn, extraHeadersFn) { const Driver = require('../../gather/driver'); @@ -627,88 +631,84 @@ describe('GatherRunner', function() { }); describe('#getPageLoadError', () => { - it('passes when the page is loaded', async () => { + it('passes when the page is loaded', () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; - assert.ok(!await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord])); + assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState)); }); - it('passes when the page is loaded, ignoring any fragment', async () => { + it('passes when the page is loaded, ignoring any fragment', () => { const url = 'http://example.com/#/page/list'; const mainRecord = new NetworkRequest(); mainRecord.url = 'http://example.com'; - assert.ok(!await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord])); + assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState)); }); - it('fails when page fails to load', async () => { + it('fails when page fails to load', () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.failed = true; mainRecord.localizedFailDescription = 'foobar'; - const error = await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord]); + const error = GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState); assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); - it('fails when page times out', async () => { + it('fails when page times out', () => { const url = 'http://the-page.com'; const records = []; - const error = await GatherRunner.getPageLoadError(fakeDriver, url, records); + const error = GatherRunner.getPageLoadError(url, records, secureSecurityState); assert.equal(error.message, 'NO_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); - it('fails when page returns with a 404', async () => { + it('fails when page returns with a 404', () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 404; - const error = await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord]); + const error = GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); - it('fails when page returns with a 500', async () => { + it('fails when page returns with a 500', () => { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 500; - const error = await GatherRunner.getPageLoadError(fakeDriver, url, [mainRecord]); + const error = GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); - it('fails when page is insecure', async () => { - const mockDriver = Object.assign({}, fakeDriver, { - async getSecurityState() { - return { - explanations: [ - { - description: 'reason 1.', - securityState: 'insecure', - }, - { - description: 'blah.', - securityState: 'info', - }, - { - description: 'reason 2.', - securityState: 'insecure', - }, - ], + 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', + }; const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; - const error = await GatherRunner.getPageLoadError(mockDriver, url, [mainRecord]); + const error = GatherRunner.getPageLoadError(url, [mainRecord], insecureSecurityState); assert.equal(error.message, 'INSECURE_DOCUMENT_REQUEST'); /* eslint-disable-next-line max-len */ - assert.equal(error.friendlyMessage, 'The URL you have provided does not have a valid security certificate. Insecure: reason 1. reason 2.'); + assert.equal(error.friendlyMessage, 'The URL you have provided does not have valid security credentials. reason 1. reason 2.'); }); }); From 8790ebdd2b506d46c6837adbe59cd754ac11457c Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 16 Oct 2018 19:26:59 -0700 Subject: [PATCH 03/13] stuff pageLoadError on passData, simplifies things --- lighthouse-core/gather/driver.js | 6 +++--- lighthouse-core/gather/gather-runner.js | 9 +++++---- .../test/gather/gather-runner-test.js | 18 ++++++++---------- typings/gatherer.d.ts | 1 + 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 4ad347976681..631882dee573 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -859,10 +859,10 @@ class Driver { const asyncTimeout = setTimeout((_ => reject(err)), timeout); this.sendCommand('Security.enable'); - this.once('Security.securityStateChanged', (e) => { + this.once('Security.securityStateChanged', state => { clearTimeout(asyncTimeout); - resolve(e); - this.sendCommand('Security.disable').catch(reject); + resolve(state); + this.sendCommand('Security.disable'); }); }); } diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index a0e28b95a88c..d7942383a46e 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -258,7 +258,7 @@ class GatherRunner { * object containing trace and network data. * @param {LH.Gatherer.PassContext} passContext * @param {Partial} gathererResults - * @return {Promise<[LH.Gatherer.LoadData, LH.LighthouseError | undefined]>} + * @return {Promise} */ static async afterPass(passContext, gathererResults) { const driver = passContext.driver; @@ -295,6 +295,7 @@ class GatherRunner { networkRecords, devtoolsLog, trace, + pageLoadError, }; if (!pageLoadError) { @@ -326,7 +327,7 @@ class GatherRunner { } // Resolve on tracing data using passName from config. - return [passData, pageLoadError]; + return passData; } /** @@ -423,8 +424,7 @@ class GatherRunner { await driver.setThrottling(options.settings, passConfig); await GatherRunner.beforePass(passContext, gathererResults); await GatherRunner.pass(passContext, gathererResults); - const [passData, pageLoadError] = - await GatherRunner.afterPass(passContext, gathererResults); + const passData = await GatherRunner.afterPass(passContext, gathererResults); // Save devtoolsLog, but networkRecords are discarded and not added onto artifacts. baseArtifacts.devtoolsLogs[passConfig.passName] = passData.devtoolsLog; @@ -450,6 +450,7 @@ class GatherRunner { firstPass = false; } + const pageLoadError = passData.pageLoadError; if (pageLoadError && pageLoadError.code === LHError.errors.INSECURE_DOCUMENT_REQUEST.code) { // Some protocol commands will hang, so let's just bail. See #6287 break; diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 037b90fde6cd..6767947db7b9 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -497,11 +497,10 @@ describe('GatherRunner', function() { ], }; - return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}) - .then(([passData]) => { - assert.equal(calledTrace, true); - assert.equal(passData.trace, fakeTraceData); - }); + return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}).then(passData => { + assert.equal(calledTrace, true); + assert.equal(passData.trace, fakeTraceData); + }); }); it('tells the driver to begin devtoolsLog collection', () => { @@ -548,11 +547,10 @@ describe('GatherRunner', function() { ], }; - return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}) - .then(([passData]) => { - assert.equal(calledDevtoolsLogCollect, true); - assert.strictEqual(passData.devtoolsLog[0], fakeDevtoolsMessage); - }); + return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}).then(passData => { + assert.equal(calledDevtoolsLogCollect, true); + assert.strictEqual(passData.devtoolsLog[0], fakeDevtoolsMessage); + }); }); it('does as many passes as are required', () => { diff --git a/typings/gatherer.d.ts b/typings/gatherer.d.ts index db4755b36939..1279458e0dd8 100644 --- a/typings/gatherer.d.ts +++ b/typings/gatherer.d.ts @@ -26,6 +26,7 @@ declare global { networkRecords: Array; devtoolsLog: DevtoolsLog; trace?: Trace; + pageLoadError?: LighthouseError; } namespace Simulation { From 089147ade1b113413524080bcff26d487d21df58 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 16 Oct 2018 19:31:25 -0700 Subject: [PATCH 04/13] no more securityState.securityState --- lighthouse-core/gather/gather-runner.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index d7942383a46e..0d0a26c5c6ad 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -151,7 +151,7 @@ class GatherRunner { * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState * @return {LHError|undefined} */ - static getPageLoadError(url, networkRecords, securityState) { + static getPageLoadError(url, networkRecords, {securityState, explanations}) { const mainRecord = networkRecords.find(record => { // record.url is actual request url, so needs to be compared without any URL fragment. return URL.equalWithExcludedFragments(record.url, url); @@ -166,9 +166,9 @@ class GatherRunner { } else if (mainRecord.hasErrorStatusCode()) { errorDef = {...LHError.errors.ERRORED_DOCUMENT_REQUEST}; errorDef.message += ` Status code: ${mainRecord.statusCode}.`; - } else if (securityState.securityState === 'insecure') { + } else if (securityState === 'insecure') { errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; - const insecureDescriptions = securityState.explanations + const insecureDescriptions = explanations .filter(exp => exp.securityState === 'insecure') .map(exp => exp.description); errorDef.message += ` ${insecureDescriptions.join(' ')}`; From b2d7c422adc1d2e7046c77932f49c30c9ebfaf5a Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Tue, 16 Oct 2018 19:38:18 -0700 Subject: [PATCH 05/13] register listener then enable Domain --- lighthouse-core/gather/driver.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 631882dee573..6793c82671de 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -858,12 +858,12 @@ class Driver { const err = new LHError(LHError.errors.SECURITY_STATE_TIMEOUT); const asyncTimeout = setTimeout((_ => reject(err)), timeout); - this.sendCommand('Security.enable'); this.once('Security.securityStateChanged', state => { clearTimeout(asyncTimeout); resolve(state); this.sendCommand('Security.disable'); }); + this.sendCommand('Security.enable'); }); } From e7c463d3d83ef873246686908d4215d304126115 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 17 Oct 2018 15:20:17 -0700 Subject: [PATCH 06/13] separate security error checking from page load test. dont return lhr on security error. --- lighthouse-core/gather/gather-runner.js | 42 +++++++++++-------- .../test/gather/gather-runner-test.js | 30 ++++++------- typings/gatherer.d.ts | 1 - 3 files changed, 40 insertions(+), 33 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 0d0a26c5c6ad..6c0db04fcac3 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -148,10 +148,9 @@ class GatherRunner { * Returns an error if the original network request failed or wasn't found. * @param {string} url The URL of the original requested page. * @param {Array} networkRecords - * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState * @return {LHError|undefined} */ - static getPageLoadError(url, networkRecords, {securityState, explanations}) { + static getPageLoadError(url, networkRecords) { const mainRecord = networkRecords.find(record => { // record.url is actual request url, so needs to be compared without any URL fragment. return URL.equalWithExcludedFragments(record.url, url); @@ -166,12 +165,6 @@ class GatherRunner { } else if (mainRecord.hasErrorStatusCode()) { errorDef = {...LHError.errors.ERRORED_DOCUMENT_REQUEST}; errorDef.message += ` Status code: ${mainRecord.statusCode}.`; - } else if (securityState === 'insecure') { - errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; - const insecureDescriptions = explanations - .filter(exp => exp.securityState === 'insecure') - .map(exp => exp.description); - errorDef.message += ` ${insecureDescriptions.join(' ')}`; } if (errorDef) { @@ -179,6 +172,22 @@ class GatherRunner { } } + /** + * Returns an error if the original network request failed or wasn't found. + * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState + * @return {LHError|undefined} + */ + static checkForSecurityIssue({securityState, explanations}) { + if (securityState === 'insecure') { + const errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; + const insecureDescriptions = explanations + .filter(exp => exp.securityState === 'insecure') + .map(exp => exp.description); + errorDef.message += ` ${insecureDescriptions.join(' ')}`; + return new LHError(errorDef, {fatal: true}); + } + } + /** * Navigates to about:blank and calls beforePass() on gatherers before tracing * has started and before navigation to the target page. @@ -278,9 +287,7 @@ class GatherRunner { const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); log.verbose('statusEnd', status); - const securityState = await driver.getSecurityState(); - let pageLoadError = GatherRunner.getPageLoadError(passContext.url, - networkRecords, securityState); + 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; @@ -289,13 +296,18 @@ class GatherRunner { passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); } + const securityState = await driver.getSecurityState(); + const securityError = this.checkForSecurityIssue(securityState); + if (securityError) { + throw securityError; + } + // Expose devtoolsLog, networkRecords, and trace (if present) to gatherers /** @type {LH.Gatherer.LoadData} */ const passData = { networkRecords, devtoolsLog, trace, - pageLoadError, }; if (!pageLoadError) { @@ -449,12 +461,6 @@ class GatherRunner { baseArtifacts.URL.finalUrl = passContext.url; firstPass = false; } - - const pageLoadError = passData.pageLoadError; - if (pageLoadError && pageLoadError.code === LHError.errors.INSECURE_DOCUMENT_REQUEST.code) { - // Some protocol commands will hang, so let's just bail. See #6287 - break; - } } const resetStorage = !options.settings.disableStorageReset; if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 6767947db7b9..fabc3f9746bb 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -34,10 +34,6 @@ class TestGathererNoArtifact extends Gatherer { const fakeDriver = require('./fake-driver'); -const secureSecurityState = { - securityState: 'secure', -}; - function getMockedEmulationDriver(emulationFn, netThrottleFn, cpuThrottleFn, blockUrlFn, extraHeadersFn) { const Driver = require('../../gather/driver'); @@ -633,14 +629,14 @@ describe('GatherRunner', function() { const url = 'http://the-page.com'; const mainRecord = new NetworkRequest(); mainRecord.url = url; - assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState)); + assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord])); }); it('passes when the page is loaded, ignoring any fragment', () => { const url = 'http://example.com/#/page/list'; const mainRecord = new NetworkRequest(); mainRecord.url = 'http://example.com'; - assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState)); + assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord])); }); it('fails when page fails to load', () => { @@ -649,7 +645,7 @@ describe('GatherRunner', function() { mainRecord.url = url; mainRecord.failed = true; mainRecord.localizedFailDescription = 'foobar'; - const error = GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState); + const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); @@ -657,7 +653,7 @@ describe('GatherRunner', function() { it('fails when page times out', () => { const url = 'http://the-page.com'; const records = []; - const error = GatherRunner.getPageLoadError(url, records, secureSecurityState); + const error = GatherRunner.getPageLoadError(url, records); assert.equal(error.message, 'NO_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); @@ -667,7 +663,7 @@ describe('GatherRunner', function() { const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 404; - const error = GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState); + const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); @@ -677,10 +673,19 @@ describe('GatherRunner', function() { const mainRecord = new NetworkRequest(); mainRecord.url = url; mainRecord.statusCode = 500; - const error = GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState); + const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); + }); + + describe('#checkForSecurityIssue', () => { + it('succeeds when page is secure', () => { + const secureSecurityState = { + securityState: 'secure', + }; + assert.ok(!GatherRunner.checkForSecurityIssue(secureSecurityState)); + }); it('fails when page is insecure', () => { const insecureSecurityState = { @@ -700,10 +705,7 @@ describe('GatherRunner', function() { ], securityState: 'insecure', }; - const url = 'http://the-page.com'; - const mainRecord = new NetworkRequest(); - mainRecord.url = url; - const error = GatherRunner.getPageLoadError(url, [mainRecord], insecureSecurityState); + const error = GatherRunner.checkForSecurityIssue(insecureSecurityState); assert.equal(error.message, 'INSECURE_DOCUMENT_REQUEST'); /* eslint-disable-next-line max-len */ assert.equal(error.friendlyMessage, 'The URL you have provided does not have valid security credentials. reason 1. reason 2.'); diff --git a/typings/gatherer.d.ts b/typings/gatherer.d.ts index 1279458e0dd8..db4755b36939 100644 --- a/typings/gatherer.d.ts +++ b/typings/gatherer.d.ts @@ -26,7 +26,6 @@ declare global { networkRecords: Array; devtoolsLog: DevtoolsLog; trace?: Trace; - pageLoadError?: LighthouseError; } namespace Simulation { From 25a98538dbac079431f07422f0b0ce8a83f41bb4 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 17 Oct 2018 15:42:08 -0700 Subject: [PATCH 07/13] listen for security change events sooner, and retain the latest received --- lighthouse-core/gather/driver.js | 25 ++++++++----------- lighthouse-core/gather/gather-runner.js | 4 +-- lighthouse-core/test/gather/fake-driver.js | 3 +++ .../test/gather/gather-runner-test.js | 2 ++ 4 files changed, 18 insertions(+), 16 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 6793c82671de..c6e095139956 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -849,22 +849,19 @@ class Driver { }); } + async listenForSecurityStateChanges() { + this.on('Security.securityStateChanged', state => { + this.lastSecurityState = state; + }); + await this.sendCommand('Security.enable'); + } + /** - * @param {number} [timeout] - * @return {Promise} + * @return {LH.Crdp.Security.SecurityStateChangedEvent} */ - getSecurityState(timeout = 1000) { - return new Promise((resolve, reject) => { - const err = new LHError(LHError.errors.SECURITY_STATE_TIMEOUT); - const asyncTimeout = setTimeout((_ => reject(err)), timeout); - - this.once('Security.securityStateChanged', state => { - clearTimeout(asyncTimeout); - resolve(state); - this.sendCommand('Security.disable'); - }); - this.sendCommand('Security.enable'); - }); + getSecurityState() { + // @ts-ignore + return this.lastSecurityState; } /** diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 6c0db04fcac3..f5355602097c 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -112,6 +112,7 @@ class GatherRunner { await driver.cacheNatives(); await driver.registerPerformanceObserver(); await driver.dismissJavaScriptDialogs(); + await driver.listenForSecurityStateChanges(); if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl); } @@ -296,8 +297,7 @@ class GatherRunner { passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); } - const securityState = await driver.getSecurityState(); - const securityError = this.checkForSecurityIssue(securityState); + const securityError = this.checkForSecurityIssue(driver.getSecurityState()); if (securityError) { throw securityError; } diff --git a/lighthouse-core/test/gather/fake-driver.js b/lighthouse-core/test/gather/fake-driver.js index 0a4d259377e1..75ce1e2f3eca 100644 --- a/lighthouse-core/test/gather/fake-driver.js +++ b/lighthouse-core/test/gather/fake-driver.js @@ -81,6 +81,9 @@ const fakeDriver = { setExtraHTTPHeaders() { return Promise.resolve(); }, + listenForSecurityStateChanges() { + return Promise.resolve(); + }, getSecurityState() { return Promise.resolve({ securityState: 'secure', diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index fabc3f9746bb..3e5016b60030 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -321,6 +321,7 @@ describe('GatherRunner', function() { clearDataForOrigin: createCheck('calledClearStorage'), blockUrlPatterns: asyncFunc, setExtraHTTPHeaders: asyncFunc, + listenForSecurityStateChanges: asyncFunc, }; return GatherRunner.setupDriver(driver, {settings: {}}).then(_ => { @@ -379,6 +380,7 @@ describe('GatherRunner', function() { clearDataForOrigin: createCheck('calledClearStorage'), blockUrlPatterns: asyncFunc, setExtraHTTPHeaders: asyncFunc, + listenForSecurityStateChanges: asyncFunc, }; return GatherRunner.setupDriver(driver, { From 9f14efff20d1f1305f6fcb59119491fb6a3211f6 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 17 Oct 2018 15:52:28 -0700 Subject: [PATCH 08/13] no longer need page load error check on security issue --- lighthouse-core/gather/gather-runner.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index f5355602097c..588f45543f42 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -310,12 +310,7 @@ class GatherRunner { trace, }; - if (!pageLoadError) { - // Disable throttling so the afterPass analysis isn't throttled - // This will hang if there was a security error. But, there is no - // need to throttle if there is such an error. See #6287 - await driver.setThrottling(passContext.settings, {useThrottling: false}); - } + await driver.setThrottling(passContext.settings, {useThrottling: false}); for (const gathererDefn of gatherers) { const gatherer = gathererDefn.instance; From 3a3015c1b08af4dcdc542f7c32cf5b6f2cc8a384 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 17 Oct 2018 18:39:02 -0700 Subject: [PATCH 09/13] fix comments, remove unused error code, delete unneeded fatal property --- lighthouse-core/gather/gather-runner.js | 5 +++-- lighthouse-core/lib/lh-error.js | 6 ------ lighthouse-core/lib/strings.js | 1 - 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 588f45543f42..0ac87e7f4068 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -174,7 +174,7 @@ class GatherRunner { } /** - * Returns an error if the original network request failed or wasn't found. + * Returns an error if the security state is insecure. * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState * @return {LHError|undefined} */ @@ -185,7 +185,7 @@ class GatherRunner { .filter(exp => exp.securityState === 'insecure') .map(exp => exp.description); errorDef.message += ` ${insecureDescriptions.join(' ')}`; - return new LHError(errorDef, {fatal: true}); + return new LHError(errorDef); } } @@ -310,6 +310,7 @@ class GatherRunner { trace, }; + // Disable throttling so the afterPass analysis isn't throttled await driver.setThrottling(passContext.settings, {useThrottling: false}); for (const gathererDefn of gatherers) { diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js index 4428c7759f60..ae1d8dbf3646 100644 --- a/lighthouse-core/lib/lh-error.js +++ b/lighthouse-core/lib/lh-error.js @@ -175,12 +175,6 @@ const ERRORS = { message: strings.requestContentTimeout, }, - // Protocol timeout failures - SECURITY_STATE_TIMEOUT: { - code: 'SECURITY_STATE_TIMEOUT', - message: strings.securityStateTimeout, - }, - // URL parsing failures INVALID_URL: { code: 'INVALID_URL', diff --git a/lighthouse-core/lib/strings.js b/lighthouse-core/lib/strings.js index de2697495226..0b5c81edc013 100644 --- a/lighthouse-core/lib/strings.js +++ b/lighthouse-core/lib/strings.js @@ -14,6 +14,5 @@ module.exports = { pageLoadFailedInsecure: `The URL you have provided does not have valid security credentials.`, internalChromeError: `An internal Chrome error occurred. Please restart Chrome and try re-running Lighthouse.`, requestContentTimeout: 'Fetching resource content has exceeded the allotted time', - securityStateTimeout: 'Fetching security state has exceeded the allotted time', urlInvalid: `The URL you have provided appears to be invalid.`, }; From bb2ce1349aa295e12646411172d366e8c95e8699 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 18 Oct 2018 09:38:32 -0700 Subject: [PATCH 10/13] throw errors more directly --- lighthouse-core/gather/driver.js | 7 ++++++- lighthouse-core/gather/gather-runner.js | 11 ++++------- lighthouse-core/test/gather/gather-runner-test.js | 14 +++++++++----- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index c6e095139956..134f801344d9 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -860,7 +860,12 @@ class Driver { * @return {LH.Crdp.Security.SecurityStateChangedEvent} */ getSecurityState() { - // @ts-ignore + 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; } diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 0ac87e7f4068..6ce85171f9a3 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -176,16 +176,16 @@ class GatherRunner { /** * Returns an error if the security state is insecure. * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState - * @return {LHError|undefined} + * @throws {LHError} */ - static checkForSecurityIssue({securityState, explanations}) { + static assertNoSecurityIssues({securityState, explanations}) { if (securityState === 'insecure') { const errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST}; const insecureDescriptions = explanations .filter(exp => exp.securityState === 'insecure') .map(exp => exp.description); errorDef.message += ` ${insecureDescriptions.join(' ')}`; - return new LHError(errorDef); + throw new LHError(errorDef); } } @@ -297,10 +297,7 @@ class GatherRunner { passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage); } - const securityError = this.checkForSecurityIssue(driver.getSecurityState()); - if (securityError) { - throw securityError; - } + this.assertNoSecurityIssues(driver.getSecurityState()); // Expose devtoolsLog, networkRecords, and trace (if present) to gatherers /** @type {LH.Gatherer.LoadData} */ diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 3e5016b60030..ccd02bddfaa4 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -686,7 +686,7 @@ describe('GatherRunner', function() { const secureSecurityState = { securityState: 'secure', }; - assert.ok(!GatherRunner.checkForSecurityIssue(secureSecurityState)); + GatherRunner.assertNoSecurityIssues(secureSecurityState); }); it('fails when page is insecure', () => { @@ -707,10 +707,14 @@ describe('GatherRunner', function() { ], securityState: 'insecure', }; - const error = GatherRunner.checkForSecurityIssue(insecureSecurityState); - assert.equal(error.message, 'INSECURE_DOCUMENT_REQUEST'); - /* eslint-disable-next-line max-len */ - assert.equal(error.friendlyMessage, 'The URL you have provided does not have valid security credentials. reason 1. reason 2.'); + try { + GatherRunner.assertNoSecurityIssues(insecureSecurityState); + assert.fail('expected INSECURE_DOCUMENT_REQUEST LHError'); + } catch (err) { + assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); + /* eslint-disable-next-line max-len */ + assert.equal(err.friendlyMessage, 'The URL you have provided does not have valid security credentials. reason 1. reason 2.'); + } }); }); From ef5ac10fbec04514aa0997144e950117f643808c Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 18 Oct 2018 12:16:52 -0700 Subject: [PATCH 11/13] explicitly type _lastSecurityState --- lighthouse-core/gather/driver.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 134f801344d9..0ce17eb127e3 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -78,6 +78,13 @@ class Driver { // properties. See https://github.com/Microsoft/TypeScript/pull/22348. this._eventEmitter.emit(event.method, event.params); }); + + /** + * Used for monitoring network status events during gotoURL. + * @type {?LH.Crdp.Security.SecurityStateChangedEvent} + * @private + */ + this._lastSecurityState = null; } static get traceCategories() { @@ -851,7 +858,7 @@ class Driver { async listenForSecurityStateChanges() { this.on('Security.securityStateChanged', state => { - this.lastSecurityState = state; + this._lastSecurityState = state; }); await this.sendCommand('Security.enable'); } @@ -860,13 +867,13 @@ class Driver { * @return {LH.Crdp.Security.SecurityStateChangedEvent} */ getSecurityState() { - if (!this.lastSecurityState) { + 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; + return this._lastSecurityState; } /** From f3bb0bd2ebdcd5178b4cfb83fdc1950994a275d3 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 18 Oct 2018 12:29:03 -0700 Subject: [PATCH 12/13] fix comment, tests --- lighthouse-core/gather/gather-runner.js | 2 +- lighthouse-core/test/gather/gather-runner-test.js | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js index 6ce85171f9a3..003d334ad462 100644 --- a/lighthouse-core/gather/gather-runner.js +++ b/lighthouse-core/gather/gather-runner.js @@ -174,7 +174,7 @@ class GatherRunner { } /** - * Returns an error if the security state is insecure. + * Throws an error if the security state is insecure. * @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState * @throws {LHError} */ diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index ccd02bddfaa4..0353d8971fbb 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -649,6 +649,7 @@ describe('GatherRunner', function() { mainRecord.localizedFailDescription = 'foobar'; const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST'); + assert.equal(error.code, 'FAILED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); @@ -657,6 +658,7 @@ describe('GatherRunner', function() { const records = []; const error = GatherRunner.getPageLoadError(url, records); assert.equal(error.message, 'NO_DOCUMENT_REQUEST'); + assert.equal(error.code, 'NO_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); @@ -667,6 +669,7 @@ describe('GatherRunner', function() { mainRecord.statusCode = 404; const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); + assert.equal(error.code, 'ERRORED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); @@ -677,6 +680,7 @@ describe('GatherRunner', function() { mainRecord.statusCode = 500; const error = GatherRunner.getPageLoadError(url, [mainRecord]); assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST'); + assert.equal(error.code, 'ERRORED_DOCUMENT_REQUEST'); assert.ok(/^Lighthouse was unable to reliably load/.test(error.friendlyMessage)); }); }); @@ -712,6 +716,7 @@ describe('GatherRunner', function() { assert.fail('expected INSECURE_DOCUMENT_REQUEST LHError'); } catch (err) { assert.equal(err.message, 'INSECURE_DOCUMENT_REQUEST'); + assert.equal(err.code, 'INSECURE_DOCUMENT_REQUEST'); /* eslint-disable-next-line max-len */ assert.equal(err.friendlyMessage, 'The URL you have provided does not have valid security credentials. reason 1. reason 2.'); } From 157e84a5263e5ae00723e9c72705107beb38f829 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Thu, 18 Oct 2018 13:00:05 -0700 Subject: [PATCH 13/13] #assertNoSecurityIssues --- lighthouse-core/test/gather/gather-runner-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js index 0353d8971fbb..2e2d2443d4e7 100644 --- a/lighthouse-core/test/gather/gather-runner-test.js +++ b/lighthouse-core/test/gather/gather-runner-test.js @@ -685,7 +685,7 @@ describe('GatherRunner', function() { }); }); - describe('#checkForSecurityIssue', () => { + describe('#assertNoSecurityIssues', () => { it('succeeds when page is secure', () => { const secureSecurityState = { securityState: 'secure',