[go: nahoru, domu]
Skip to content

Commit

Permalink
core(driver): security errors are no longer a fatal or pageload error (…
Browse files Browse the repository at this point in the history 
…#8865)
  • Loading branch information
@paulirish @brendankenny
paulirish authored and brendankenny committed Jun 3, 2019
1 parent f8552d5 commit 93387cd
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 161 deletions.
4 changes: 2 additions & 2 deletions lighthouse-cli/test/smokehouse/error-expectations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ module.exports = [
},
},
{
errorCode: 'INSECURE_DOCUMENT_REQUEST',
errorCode: undefined,
lhr: {
requestedUrl: 'https://expired.badssl.com',
finalUrl: 'https://expired.badssl.com',
finalUrl: 'https://expired.badssl.com/',
audits: {},
},
},
Expand Down
58 changes: 1 addition & 57 deletions lighthouse-core/gather/driver.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -903,53 +903,6 @@ class Driver {
};
}

/**
* Return a promise that resolves when an insecure security state is encountered
* and a method to cancel internal listeners.
* @return {{promise: Promise<string>, cancel: function(): void}}
* @private
*/
_monitorForInsecureState() {
/** @type {(() => void)} */
let cancel = () => {
throw new Error('_monitorForInsecureState.cancel() called before it was defined');
};

const promise = new Promise((resolve, reject) => {
/**
* @param {LH.Crdp.Security.SecurityStateChangedEvent} event
*/
const securityStateChangedListener = ({
securityState,
explanations,
schemeIsCryptographic,
}) => {
if (securityState === 'insecure' && schemeIsCryptographic) {
cancel();
const insecureDescriptions = explanations
.filter(exp => exp.securityState === 'insecure')
.map(exp => exp.description);
resolve(insecureDescriptions.join(' '));
}
};
let canceled = false;
cancel = () => {
if (canceled) return;
canceled = true;
this.off('Security.securityStateChanged', securityStateChangedListener);
// TODO(@patrickhulce): cancel() should really be a promise itself to handle things like this
this.sendCommand('Security.disable').catch(() => {});
};
this.on('Security.securityStateChanged', securityStateChangedListener);
this.sendCommand('Security.enable').catch(() => {});
});

return {
promise,
cancel,
};
}

/**
* Returns whether the page appears to be hung.
* @return {Promise<boolean>}
Expand Down Expand Up @@ -1001,13 +954,6 @@ class Driver {
// CPU listener. Resolves when the CPU has been idle for cpuQuietThresholdMs after network idle.
let waitForCPUIdle = this._waitForNothing();

const monitorForInsecureState = this._monitorForInsecureState();
const securityCheckPromise = monitorForInsecureState.promise.then(securityMessages => {
return function() {
throw new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages});
};
});

// Wait for both load promises. Resolves on cleanup function the clears load
// timeout timer.
const loadPromise = Promise.all([
Expand Down Expand Up @@ -1044,9 +990,8 @@ class Driver {
};
});

// Wait for security issue, load or timeout and run the cleanup function the winner returns.
// Wait for load or timeout and run the cleanup function the winner returns.
const cleanupFn = await Promise.race([
securityCheckPromise,
loadPromise,
maxTimeoutPromise,
]);
Expand All @@ -1056,7 +1001,6 @@ class Driver {
waitForLoadEvent.cancel();
waitForNetworkIdle.cancel();
waitForCPUIdle.cancel();
monitorForInsecureState.cancel();

await cleanupFn();
}
Expand Down
102 changes: 0 additions & 102 deletions lighthouse-core/test/gather/driver-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -675,108 +675,6 @@ describe('.gotoURL', () => {
// Make sure we still cleaned up our listeners
expect(driver._waitForLoadEvent.getMockCancelFn()).toHaveBeenCalled();
});

it('does not reject when page is secure', async () => {
const secureSecurityState = {
explanations: [],
securityState: 'secure',
};

driver.on = driver.once = createMockOnceFn()
.mockEvent('Security.securityStateChanged', secureSecurityState);

const startUrl = 'https://www.example.com';
const loadOptions = {
waitForLoad: true,
passContext: {
settings: {
maxWaitForLoad: 1,
},
},
};

const loadPromise = driver.gotoURL(startUrl, loadOptions);
await flushAllTimersAndMicrotasks();
await loadPromise;
});

it('does not reject when page is insecure but http', async () => {
const secureSecurityState = {
explanations: [],
securityState: 'insecure',
schemeIsCryptographic: false,
};

driver.on = driver.once = createMockOnceFn()
.mockEvent('Security.securityStateChanged', secureSecurityState);

const startUrl = 'https://www.example.com';
const loadOptions = {
waitForLoad: true,
passContext: {
settings: {
maxWaitForLoad: 1,
},
},
};

const loadPromise = driver.gotoURL(startUrl, loadOptions);
await flushAllTimersAndMicrotasks();
await loadPromise;
});

it('rejects when page is insecure', async () => {
const insecureSecurityState = {
explanations: [
{
description: 'reason 1.',
securityState: 'insecure',
},
{
description: 'blah.',
securityState: 'info',
},
{
description: 'reason 2.',
securityState: 'insecure',
},
],
securityState: 'insecure',
schemeIsCryptographic: true,
};

driver.on = driver.once = createMockOnceFn();

const startUrl = 'https://www.example.com';
const loadOptions = {
waitForLoad: true,
passContext: {
passConfig: {
networkQuietThresholdMs: 1,
},
},
};

// 2 assertions in the catch block and the 1 implicit in `findListener`
expect.assertions(3);

try {
const loadPromise = driver.gotoURL(startUrl, loadOptions);
await flushAllTimersAndMicrotasks();

// Use `findListener` instead of `mockEvent` so we can control exactly when the promise resolves
const listener = driver.on.findListener('Security.securityStateChanged');
listener(insecureSecurityState);
await flushAllTimersAndMicrotasks();
await loadPromise;
} catch (err) {
expect(err).toHaveProperty('code', 'INSECURE_DOCUMENT_REQUEST');
expect(err.friendlyMessage).toBeDisplayString(
'The URL you have provided does not have a valid security certificate. ' +
'reason 1. reason 2.'
);
}
});
});
});

Expand Down

0 comments on commit 93387cd

Please sign in to comment.