Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: bail if encounter insecure ssl cert, to avoid hanging forever #6300

Merged
merged 13 commits into from
Oct 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -849,6 +856,26 @@ class Driver {
});
}

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<string>} The state of permissions, resolved in a promise.
Expand Down
19 changes: 19 additions & 0 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class GatherRunner {
await driver.cacheNatives();
await driver.registerPerformanceObserver();
await driver.dismissJavaScriptDialogs();
await driver.listenForSecurityStateChanges();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing it this way, I guess we'll find out if there are issues with listening through the whole page load :)

But it does nicely pave the way for bailing earlier if we want to do that.

if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl);
}

Expand Down Expand Up @@ -172,6 +173,22 @@ 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 errorDef = {...LHError.errors.INSECURE_DOCUMENT_REQUEST};
const insecureDescriptions = explanations
.filter(exp => exp.securityState === 'insecure')
.map(exp => exp.description);
errorDef.message += ` ${insecureDescriptions.join(' ')}`;
throw new LHError(errorDef);
}
}

/**
* Navigates to about:blank and calls beforePass() on gatherers before tracing
* has started and before navigation to the target page.
Expand Down Expand Up @@ -280,6 +297,8 @@ class GatherRunner {
passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage);
}

this.assertNoSecurityIssues(driver.getSecurityState());

// Expose devtoolsLog, networkRecords, and trace (if present) to gatherers
/** @type {LH.Gatherer.LoadData} */
const passData = {
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/lib/strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +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 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',
urlInvalid: `The URL you have provided appears to be invalid.`,
Expand Down
8 changes: 8 additions & 0 deletions lighthouse-core/test/gather/fake-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ const fakeDriver = {
setExtraHTTPHeaders() {
return Promise.resolve();
},
listenForSecurityStateChanges() {
return Promise.resolve();
},
getSecurityState() {
return Promise.resolve({
securityState: 'secure',
});
},
};

module.exports = fakeDriver;
Expand Down
48 changes: 46 additions & 2 deletions lighthouse-core/test/gather/gather-runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ describe('GatherRunner', function() {
clearDataForOrigin: createCheck('calledClearStorage'),
blockUrlPatterns: asyncFunc,
setExtraHTTPHeaders: asyncFunc,
listenForSecurityStateChanges: asyncFunc,
};

return GatherRunner.setupDriver(driver, {settings: {}}).then(_ => {
Expand Down Expand Up @@ -379,6 +380,7 @@ describe('GatherRunner', function() {
clearDataForOrigin: createCheck('calledClearStorage'),
blockUrlPatterns: asyncFunc,
setExtraHTTPHeaders: asyncFunc,
listenForSecurityStateChanges: asyncFunc,
};

return GatherRunner.setupDriver(driver, {
Expand Down Expand Up @@ -543,9 +545,9 @@ describe('GatherRunner', function() {
],
};

return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}).then(vals => {
return GatherRunner.afterPass({url, driver, passConfig}, {TestGatherer: []}).then(passData => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok to revert these lines now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think passData is more clear than vals.

assert.equal(calledDevtoolsLogCollect, true);
assert.strictEqual(vals.devtoolsLog[0], fakeDevtoolsMessage);
assert.strictEqual(passData.devtoolsLog[0], fakeDevtoolsMessage);
});
});

Expand Down Expand Up @@ -647,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));
});

Expand All @@ -655,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));
});

Expand All @@ -665,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));
});

Expand All @@ -675,10 +680,49 @@ 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));
});
});

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');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe check err.code as well/instead, since that's what we use for the top level runtimeError

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Some other tests were just testing .message, so I added checks there too. lmk if I should just remove the .message assertions.

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.');
}
});
});

describe('artifact collection', () => {
// Make sure our gatherers never execute in parallel
it('runs gatherer lifecycle methods strictly in sequence', async () => {
Expand Down