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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Oct 19, 2018
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
104e399
core: bail if encounter insecure ssl cert, to avoid hanging forever …
connorjclark Oct 17, 2018
2eafa44
simplify getPageLoadError, edit strings
connorjclark Oct 17, 2018
8790ebd
stuff pageLoadError on passData, simplifies things
connorjclark Oct 17, 2018
089147a
no more securityState.securityState
connorjclark Oct 17, 2018
b2d7c42
register listener then enable Domain
connorjclark Oct 17, 2018
e7c463d
separate security error checking from page load test. dont return lhr…
connorjclark Oct 17, 2018
25a9853
listen for security change events sooner, and retain the latest received
connorjclark Oct 17, 2018
9f14eff
no longer need page load error check on security issue
connorjclark Oct 17, 2018
3a3015c
fix comments, remove unused error code, delete unneeded fatal property
connorjclark Oct 18, 2018
bb2ce13
throw errors more directly
connorjclark Oct 18, 2018
ef5ac10
explicitly type _lastSecurityState
connorjclark Oct 18, 2018
f3bb0bd
fix comment, tests
connorjclark Oct 18, 2018
157e84a
#assertNoSecurityIssues
connorjclark Oct 18, 2018
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
18 changes: 18 additions & 0 deletions lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,24 @@ class Driver {
});
}

/**
* @param {number} [timeout]
* @return {Promise<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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this will eventually be replaced by #6296


this.once('Security.securityStateChanged', state => {
Copy link
Member

@brendankenny brendankenny Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

aside: does this check break if someone else has been listening to the Security domain? It would be nice to get a non-event based version of this in the protocol (AFAIK not much we can do to guard against that case for now).

Copy link
Collaborator Author

@connorjclark connorjclark Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I enabled just before calling this function, and yes, it does break. It times out + rejects with SECURITY_STATE_TIMEOUT (which actually kills Lighthouse. oops)

Besides other users enabling the Security domain via code, could this fail if a developer opens the Security tab (this enables the domain) before running an audit?

clearTimeout(asyncTimeout);
resolve(state);
this.sendCommand('Security.disable');
});
this.sendCommand('Security.enable');
Copy link
Member

Choose a reason for hiding this comment

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

If we want this to be a general utility function, it will need to disable the domain and remove the event listener in the rejection case. Otherwise we'll need to think how to make rejections always lead to a program exit, as the state of things might get weird (we'll have to deal with the same thing in #6296)

Copy link
Member

Choose a reason for hiding this comment

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

If we want this to be a general utility function, it will need to disable the domain and remove the event listener in the rejection case.

Maybe this is overkill. If it's timing out (unexpectedly), something is pretty wrong and LH probably won't recover. Disabling the domain also might not even work. OTOH, not cleaning up feels wrong :)

});
}

/**
* @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
28 changes: 24 additions & 4 deletions lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,10 @@ 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<LH.Artifacts.NetworkRequest>} networkRecords
* @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState
* @return {LHError|undefined}
*/
static getPageLoadError(url, networkRecords) {
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);
Expand All @@ -165,6 +166,12 @@ 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) {
Expand Down Expand Up @@ -271,7 +278,9 @@ class GatherRunner {
const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);
log.verbose('statusEnd', status);

let pageLoadError = GatherRunner.getPageLoadError(passContext.url, networkRecords);
const securityState = await driver.getSecurityState();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we're making this harder than it needs to be by trying to unify with with the other page load errors.

getPageLoadError is trying really hard to not interrupt normal control flow, inserting the promise rejection into the gather results instead of into the executing chain. Meanwhile, this security check is trying to abandon the gathering altogether.

Maybe this should just be a separate function, checkPageSecurityState() or whatever, called here-ish. If the page is insecure, throw in there. It'll bail back to here, which will bail up to the catch in run(). If we really want to still return something in that case, the check against LHError.errors.INSECURE_DOCUMENT_REQUEST.code can happen there (or rethrow if it's something different).

This also gets around extending passData, which makes sense, because really there isn't any other useful pass data when there's this kind of error.

WDYT?

Copy link
Collaborator Author

@connorjclark connorjclark Oct 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Would be great to utilize error throwing to simplify this. But, I can't quite figure out how to still display a LHR in run's catch. I marked the security LHError fatal (so it would throw), moved baseArtifacts up a bit (so it can be accessed in catch), but get this silly error:

Error: CSSUsage failed to provide an artifact.
    at Function.collectArtifacts (/Users/cjamcl/src/lighthouse/lighthouse-core/gather/gather-runner.js:366:15)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:189:7)

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;

Expand All @@ -286,10 +295,15 @@ class GatherRunner {
networkRecords,
devtoolsLog,
trace,
pageLoadError,
};

// Disable throttling so the afterPass analysis isn't throttled
Copy link
Member

Choose a reason for hiding this comment

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

restore?

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;
Expand Down Expand Up @@ -435,6 +449,12 @@ 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);
Expand Down
12 changes: 12 additions & 0 deletions lighthouse-core/lib/lh-error.js
View file
Open in desktop
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 Expand Up @@ -169,6 +175,12 @@ const ERRORS = {
message: strings.requestContentTimeout,
},

// Protocol timeout failures
SECURITY_STATE_TIMEOUT: {
Copy link
Member

Choose a reason for hiding this comment

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

in the usual case for the security state check, it should just reject on an insecure security state, right? If so, we probably want to make this a more general protocol communication timeout error (anticipating #6296)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My plan was to modify that bit in the referenced issue. but now that we have this concrete proto definition it makes sense to make it good sooner rather than later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I actually removed the timeout stuff for security checking. I'll remove this too.

code: 'SECURITY_STATE_TIMEOUT',
message: strings.securityStateTimeout,
},

// URL parsing failures
INVALID_URL: {
code: 'INVALID_URL',
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/lib/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 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.`,
};
5 changes: 5 additions & 0 deletions lighthouse-core/test/gather/fake-driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ const fakeDriver = {
setExtraHTTPHeaders() {
return Promise.resolve();
},
getSecurityState() {
return Promise.resolve({
securityState: 'secure',
});
},
};

module.exports = fakeDriver;
Expand Down
47 changes: 39 additions & 8 deletions lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -543,9 +547,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 @@ -629,14 +633,14 @@ describe('GatherRunner', function() {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord]));
assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState));
});

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]));
assert.ok(!GatherRunner.getPageLoadError(url, [mainRecord], secureSecurityState));
});

it('fails when page fails to load', () => {
Expand All @@ -645,15 +649,15 @@ describe('GatherRunner', function() {
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'foobar';
const error = GatherRunner.getPageLoadError(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', () => {
const url = 'http://the-page.com';
const records = [];
const error = GatherRunner.getPageLoadError(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));
});
Expand All @@ -663,7 +667,7 @@ describe('GatherRunner', function() {
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 404;
const error = GatherRunner.getPageLoadError(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));
});
Expand All @@ -673,10 +677,37 @@ describe('GatherRunner', function() {
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 500;
const error = GatherRunner.getPageLoadError(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', () => {
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 = 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 valid security credentials. reason 1. reason 2.');
});
});

describe('artifact collection', () => {
Expand Down
1 change: 1 addition & 0 deletions typings/gatherer.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ declare global {
networkRecords: Array<Artifacts.NetworkRequest>;
devtoolsLog: DevtoolsLog;
trace?: Trace;
pageLoadError?: LighthouseError;
}

namespace Simulation {
Expand Down