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 8 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
15 changes: 15 additions & 0 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,21 @@ class Driver {
});
}

async listenForSecurityStateChanges() {
this.on('Security.securityStateChanged', state => {
this.lastSecurityState = state;
Copy link
Member

Choose a reason for hiding this comment

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

the type checker now allows this (in JS files as of 3.0 or 3.1), but should still declare it in the constructor so there's a clear place to check for what's on Driver, we don't dynamically mutate the object's shape, etc

Copy link
Collaborator Author

@connorjclark connorjclark Oct 18, 2018

Choose a reason for hiding this comment

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

yeah this really surprised me, how it was able to infer the type. it was also able to infer it could be undefined, since the ctor didn't set it.

Makes sense. This is not good:
image

I guess TSC just infers a permissive union type for this.property, based on all setters? What new feature of tsc 3.0/3.1 is this?

});
await this.sendCommand('Security.enable');
}

/**
* @return {LH.Crdp.Security.SecurityStateChangedEvent}
*/
getSecurityState() {
// @ts-ignore
return this.lastSecurityState;
Copy link
Member

Choose a reason for hiding this comment

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

should set this in the Driver constructor (I guess to null since SecurityStateChangedEvent is a relatively large object so it's probably not worth constructing an unknown starting value). Won't need to ts-ignore then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's ts-ignored to coerce a non-nullable value, since I believe this should always be set. I could remove it and allow this getter to be nullable, but would also need to handle that in the calling code (a case that I think will never happen). wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we believe it's always set but don't know for sure, WDYT about throwing an explicit error if it's not?

This way it'll be clear to us when something unexpected happens and ts is happy by default :)

}

/**
* @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
23 changes: 22 additions & 1 deletion 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 {
}
}

/**
* Returns an error if the original network request failed or wasn't found.
Copy link
Member

Choose a reason for hiding this comment

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

need to update string

* @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState
* @return {LHError|undefined}
*/
static checkForSecurityIssue({securityState, explanations}) {
Copy link
Member

Choose a reason for hiding this comment

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

bikeshed: if it does throw in here, maybe rename assertCertificateError or something? Or does this catch a larger class of issues

Copy link
Collaborator Author

@connorjclark connorjclark Oct 18, 2018

Choose a reason for hiding this comment

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

It catches anytime the latest security state is marked insecure. a certificate error is one example. I don't know if it's just one of, most of them, or all possible scenarios.

Copy link
Collaborator

@patrickhulce patrickhulce Oct 18, 2018

Choose a reason for hiding this comment

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

Just triple checking because the protocol name is getting me concerned a bit, but securityState === 'insecure' only when the page is HTTPS but actually isn't secure, right? Not just every time the page is served over HTTP without encryption? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 to the idea of throwing in here with a assert* rename too, btw. Seems much easier and removes the cognitive load from the caller

Copy link
Collaborator Author

@connorjclark connorjclark Oct 18, 2018

Choose a reason for hiding this comment

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

Good idea to check HTTP. Tested with node lighthouse-cli http://www.httpvshttps.com/ --verbose --view - it still generated a full LHR. I presume "insecure" is only used for HTTPS connections that don't comply with security standards (and I guess they always go hand-in-hand with interstitial security warnings).

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

Choose a reason for hiding this comment

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

rather than returning the error and throwing on the return value, I'd say just throw in here

Copy link
Member

Choose a reason for hiding this comment

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

{fatal: true} shouldn't be necessary anymore

Copy link
Collaborator Author

@connorjclark connorjclark Oct 18, 2018

Choose a reason for hiding this comment

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

OK, I can do that. I was thinking it's nicer to make tests that check return values rather than throwing conditions, but just my preference, I could have it either way.

I'll remove fatal. yay, that means the resolve or throw thing really can still be removed. It wasn't being used any way, doh.

}
}

/**
* 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,11 @@ class GatherRunner {
passContext.LighthouseRunWarnings.push(pageLoadError.friendlyMessage);
}

const securityError = this.checkForSecurityIssue(driver.getSecurityState());
if (securityError) {
throw securityError;
}

// Expose devtoolsLog, networkRecords, and trace (if present) to gatherers
/** @type {LH.Gatherer.LoadData} */
const passData = {
Expand All @@ -288,7 +310,6 @@ class GatherRunner {
trace,
};

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

for (const gathererDefn of gatherers) {
Expand Down
12 changes: 12 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 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
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.`,
};
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
39 changes: 37 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 @@ -679,6 +681,39 @@ describe('GatherRunner', function() {
});
});

describe('#checkForSecurityIssue', () => {
Copy link
Member

Choose a reason for hiding this comment

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

#assertNoSecurityIssues

it('succeeds when page is secure', () => {
const secureSecurityState = {
securityState: 'secure',
};
assert.ok(!GatherRunner.checkForSecurityIssue(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',
};
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.');
});
});

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