-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Changes from 9 commits
104e399
2eafa44
8790ebd
089147a
b2d7c42
e7c463d
25a9853
9f14eff
3a3015c
bb2ce13
ef5ac10
f3bb0bd
157e84a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -849,6 +849,21 @@ class Driver { | |
}); | ||
} | ||
|
||
async listenForSecurityStateChanges() { | ||
this.on('Security.securityStateChanged', state => { | ||
this.lastSecurityState = state; | ||
}); | ||
await this.sendCommand('Security.enable'); | ||
} | ||
|
||
/** | ||
* @return {LH.Crdp.Security.SecurityStateChangedEvent} | ||
*/ | ||
getSecurityState() { | ||
// @ts-ignore | ||
return this.lastSecurityState; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should set this in the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,6 +112,7 @@ class GatherRunner { | |
await driver.cacheNatives(); | ||
await driver.registerPerformanceObserver(); | ||
await driver.dismissJavaScriptDialogs(); | ||
await driver.listenForSecurityStateChanges(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
|
@@ -172,6 +173,22 @@ class GatherRunner { | |
} | ||
} | ||
|
||
/** | ||
* Returns an error if the security state is insecure. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throws |
||
* @param {LH.Crdp.Security.SecurityStateChangedEvent} securityState | ||
* @return {LHError|undefined} | ||
*/ | ||
static checkForSecurityIssue({securityState, explanations}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bikeshed: if it does throw in here, maybe rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It catches anytime the latest security state is marked There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to the idea of throwing in here with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea to check HTTP. Tested with |
||
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); | ||
} | ||
} | ||
|
||
/** | ||
* Navigates to about:blank and calls beforePass() on gatherers before tracing | ||
* has started and before navigation to the target page. | ||
|
@@ -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 = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, { | ||
|
@@ -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 => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok to revert these lines now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
assert.equal(calledDevtoolsLogCollect, true); | ||
assert.strictEqual(vals.devtoolsLog[0], fakeDevtoolsMessage); | ||
assert.strictEqual(passData.devtoolsLog[0], fakeDevtoolsMessage); | ||
}); | ||
}); | ||
|
||
|
@@ -679,6 +681,39 @@ describe('GatherRunner', function() { | |
}); | ||
}); | ||
|
||
describe('#checkForSecurityIssue', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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 () => { | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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](https://user-images.githubusercontent.com/4071474/47178097-0ba18c80-d2cf-11e8-9271-6340b4ac34af.png)
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?