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(gather-runner): only fail for mainRecord interstitials #9576

Merged
merged 4 commits into from
Aug 22, 2019
Merged

core(gather-runner): only fail for mainRecord interstitials #9576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba58208
core(gather-runner): only fail for mainRecord interstitials
patrickhulce Aug 19, 2019
0af35ab
tests
patrickhulce Aug 19, 2019
fd7a910
lint
patrickhulce Aug 19, 2019
3fdcf2e
add explanatory comment
patrickhulce Aug 20, 2019
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
7 changes: 7 additions & 0 deletions lighthouse-cli/test/fixtures/badssl-iframe.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<html>
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<body>
<h1>I have a bad iframe!</h1>
<iframe src="https://expired.badssl.com"></iframe>
</body>
</html>
22 changes: 22 additions & 0 deletions lighthouse-cli/test/smokehouse/error-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,26 @@ module.exports = [
},
},
},
{
lhr: {
// Our interstitial error handling used to be quite aggressive, so we'll test a page
// that has a bad iframe to make sure LH audits successfully.
// https://github.com/GoogleChrome/lighthouse/issues/9562
requestedUrl: 'http://localhost:10200/badssl-iframe.html',
finalUrl: 'http://localhost:10200/badssl-iframe.html',
audits: {
'first-contentful-paint': {
scoreDisplayMode: 'numeric',
},
},
},
artifacts: {
devtoolsLogs: {
defaultPass: NONEMPTY_ARRAY,
},
traces: {
defaultPass: {traceEvents: NONEMPTY_ARRAY},
},
},
},
];
48 changes: 24 additions & 24 deletions lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const log = require('lighthouse-logger');
const manifestParser = require('../lib/manifest-parser.js');
const stacksGatherer = require('../lib/stack-collector.js');
const LHError = require('../lib/lh-error.js');
const URL = require('../lib/url-shim.js');
const NetworkAnalyzer = require('../lib/dependency-graph/simulator/network-analyzer.js');
const NetworkRecorder = require('../lib/network-recorder.js');
const constants = require('../config/constants.js');
const i18n = require('../lib/i18n/i18n.js');
Expand Down Expand Up @@ -132,16 +132,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.Artifacts.NetworkRequest|undefined} mainRecord
* @return {LH.LighthouseError|undefined}
*/
static getNetworkError(url, networkRecords) {
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);
});

static getNetworkError(mainRecord) {
if (!mainRecord) {
return new LHError(LHError.errors.NO_DOCUMENT_REQUEST);
} else if (mainRecord.failed) {
Expand Down Expand Up @@ -170,32 +164,31 @@ class GatherRunner {

/**
* Returns an error if we ended up on the `chrome-error` page and all other requests failed.
* @param {LH.Artifacts.NetworkRequest|undefined} mainRecord
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
* @return {LH.LighthouseError|undefined}
*/
static getInterstitialError(networkRecords) {
static getInterstitialError(mainRecord, networkRecords) {
// If we never requested a document, there's no interstitial error, let other cases handle it.
if (!mainRecord) return undefined;

const interstitialRequest = networkRecords
.find(record => record.documentURL.startsWith('chrome-error://'));
// If the page didn't end up on a chrome interstitial, there's no error here.
if (!interstitialRequest) return undefined;

const pageNetworkRecords = networkRecords
.filter(record => !URL.NON_NETWORK_PROTOCOLS.includes(record.protocol) &&
!record.documentURL.startsWith('chrome-error://'));
// If none of the requests failed, there's no error here.
// We don't expect that this case could ever occur, but better safe than sorry.
// Note also that in cases of redirects, the initial requests could succeed and we still end up
// on the error interstitial page.
if (!pageNetworkRecords.some(record => record.failed)) return undefined;
// If the main document didn't fail, we didn't end up on an interstitial.
// FIXME: This doesn't handle client-side redirects.
// None of our error-handling deals with this case either because passContext.url doesn't handle non-network redirects.
if (!mainRecord.failed) return undefined;

// If a request failed with the `net::ERR_CERT_*` collection of errors, then it's a security issue.
const insecureRequest = pageNetworkRecords.find(record =>
record.failed && record.localizedFailDescription.startsWith('net::ERR_CERT'));
if (insecureRequest) {
if (mainRecord.localizedFailDescription.startsWith('net::ERR_CERT')) {
return new LHError(LHError.errors.INSECURE_DOCUMENT_REQUEST, {securityMessages:
insecureRequest.localizedFailDescription});
mainRecord.localizedFailDescription});
}

// If we made it this far, it's a generic Chrome interstitial error.
return new LHError(LHError.errors.CHROME_INTERSTITIAL_ERROR);
}

Expand All @@ -208,8 +201,15 @@ class GatherRunner {
* @return {LighthouseError|undefined}
*/
static getPageLoadError(passContext, loadData, navigationError) {
const networkError = GatherRunner.getNetworkError(passContext.url, loadData.networkRecords);
const interstitialError = GatherRunner.getInterstitialError(loadData.networkRecords);
const {networkRecords} = loadData;
/** @type {LH.Artifacts.NetworkRequest|undefined} */
let mainRecord;
try {
mainRecord = NetworkAnalyzer.findMainDocument(networkRecords, passContext.url);
} catch (_) {}

const networkError = GatherRunner.getNetworkError(mainRecord);
const interstitialError = GatherRunner.getInterstitialError(mainRecord, networkRecords);

// If the driver was offline, the load will fail without offline support. Ignore this case.
if (!passContext.driver.online) return;
Expand Down
72 changes: 49 additions & 23 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 @@ -426,12 +426,15 @@ describe('GatherRunner', function() {

it('returns a pageLoadError and no artifacts when there is a network error', async () => {
const requestedUrl = 'https://example.com';
// This page load error should be overriden by NO_DOCUMENT_REQUEST (for being
// more specific) since requestedUrl does not match any network request in fixture.
// This page load error should be overriden by ERRORED_DOCUMENT_REQUEST (for being
// more specific) since the main document network request failed with a 500.
const navigationError = new LHError(LHError.errors.NO_FCP);
const driver = Object.assign({}, fakeDriver, {
online: true,
gotoURL: url => url.includes('blank') ? null : Promise.reject(navigationError),
endDevtoolsLog() {
return networkRecordsToDevtoolsLog([{url: requestedUrl, statusCode: 500}]);
},
});

const config = new Config({
Expand All @@ -450,7 +453,7 @@ describe('GatherRunner', function() {
const artifacts = await GatherRunner.run(config.passes, options);
expect(artifacts.LighthouseRunWarnings).toHaveLength(1);
expect(artifacts.PageLoadError).toBeInstanceOf(Error);
expect(artifacts.PageLoadError.code).toEqual('NO_DOCUMENT_REQUEST');
expect(artifacts.PageLoadError.code).toEqual('ERRORED_DOCUMENT_REQUEST');
expect(artifacts.TestGatherer).toBeUndefined();
});

Expand Down Expand Up @@ -789,6 +792,7 @@ describe('GatherRunner', function() {
// resolved URL here does not match any request in the network records, causing a runtime error.
gotoURL: async _ => requestedUrl,
online: true,
endDevtoolsLog: () => [],
});

const config = new Config({
Expand Down Expand Up @@ -871,14 +875,7 @@ describe('GatherRunner', function() {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
assert.ok(!GatherRunner.getNetworkError(url, [mainRecord]));
});

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

it('fails when page fails to load', () => {
Expand All @@ -887,17 +884,15 @@ describe('GatherRunner', function() {
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'foobar';
const error = GatherRunner.getNetworkError(url, [mainRecord]);
const error = GatherRunner.getNetworkError(mainRecord);
assert.equal(error.message, 'FAILED_DOCUMENT_REQUEST');
assert.equal(error.code, 'FAILED_DOCUMENT_REQUEST');
expect(error.friendlyMessage)
.toBeDisplayString(/^Lighthouse was unable to reliably load.*foobar/);
});

it('fails when page times out', () => {
const url = 'http://the-page.com';
const records = [];
const error = GatherRunner.getNetworkError(url, records);
const error = GatherRunner.getNetworkError(undefined);
assert.equal(error.message, 'NO_DOCUMENT_REQUEST');
assert.equal(error.code, 'NO_DOCUMENT_REQUEST');
expect(error.friendlyMessage).toBeDisplayString(/^Lighthouse was unable to reliably load/);
Expand All @@ -908,7 +903,7 @@ describe('GatherRunner', function() {
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 404;
const error = GatherRunner.getNetworkError(url, [mainRecord]);
const error = GatherRunner.getNetworkError(mainRecord);
assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST');
assert.equal(error.code, 'ERRORED_DOCUMENT_REQUEST');
expect(error.friendlyMessage)
Expand All @@ -920,7 +915,7 @@ describe('GatherRunner', function() {
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.statusCode = 500;
const error = GatherRunner.getNetworkError(url, [mainRecord]);
const error = GatherRunner.getNetworkError(mainRecord);
assert.equal(error.message, 'ERRORED_DOCUMENT_REQUEST');
assert.equal(error.code, 'ERRORED_DOCUMENT_REQUEST');
expect(error.friendlyMessage)
Expand All @@ -933,19 +928,23 @@ describe('GatherRunner', function() {
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'net::ERR_NAME_NOT_RESOLVED';
const error = GatherRunner.getNetworkError(url, [mainRecord]);
const error = GatherRunner.getNetworkError(mainRecord);
assert.equal(error.message, 'DNS_FAILURE');
assert.equal(error.code, 'DNS_FAILURE');
expect(error.friendlyMessage).toBeDisplayString(/^DNS servers could not resolve/);
});
});

describe('#getInterstitialError', () => {
it('passes when the page was not requested', () => {
expect(GatherRunner.getInterstitialError(undefined, [])).toBeUndefined();
});

it('passes when the page is loaded', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
expect(GatherRunner.getInterstitialError([mainRecord])).toBeUndefined();
expect(GatherRunner.getInterstitialError(mainRecord, [mainRecord])).toBeUndefined();
});

it('passes when page fails to load normally', () => {
Expand All @@ -954,7 +953,7 @@ describe('GatherRunner', function() {
mainRecord.url = url;
mainRecord.failed = true;
mainRecord.localizedFailDescription = 'foobar';
expect(GatherRunner.getInterstitialError([mainRecord])).toBeUndefined();
expect(GatherRunner.getInterstitialError(mainRecord, [mainRecord])).toBeUndefined();
});

it('passes when page gets a generic interstitial but somehow also loads everything', () => {
Expand All @@ -966,7 +965,7 @@ describe('GatherRunner', function() {
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
expect(GatherRunner.getInterstitialError(records)).toBeUndefined();
expect(GatherRunner.getInterstitialError(mainRecord, records)).toBeUndefined();
});

it('fails when page gets a generic interstitial', () => {
Expand All @@ -979,7 +978,7 @@ describe('GatherRunner', function() {
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
const error = GatherRunner.getInterstitialError(records);
const error = GatherRunner.getInterstitialError(mainRecord, records);
expect(error.message).toEqual('CHROME_INTERSTITIAL_ERROR');
expect(error.code).toEqual('CHROME_INTERSTITIAL_ERROR');
expect(error.friendlyMessage).toBeDisplayString(/^Chrome prevented/);
Expand All @@ -995,12 +994,29 @@ describe('GatherRunner', function() {
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, interstitialRecord];
const error = GatherRunner.getInterstitialError(records);
const error = GatherRunner.getInterstitialError(mainRecord, records);
expect(error.message).toEqual('INSECURE_DOCUMENT_REQUEST');
expect(error.code).toEqual('INSECURE_DOCUMENT_REQUEST');
expect(error.friendlyMessage).toBeDisplayString(/valid security certificate/);
expect(error.friendlyMessage).toBeDisplayString(/net::ERR_CERT_COMMON_NAME_INVALID/);
});

it('passes when page iframe gets a generic interstitial', () => {
const url = 'http://the-page.com';
const mainRecord = new NetworkRequest();
mainRecord.url = url;
mainRecord.failed = false;
const iframeRecord = new NetworkRequest();
iframeRecord.failed = true;
iframeRecord.url = 'https://the-ad.com';
iframeRecord.documentURL = 'https://the-ad.com';
const interstitialRecord = new NetworkRequest();
interstitialRecord.url = 'data:text/html;base64,abcdef';
interstitialRecord.documentURL = 'chrome-error://chromewebdata/';
const records = [mainRecord, iframeRecord, interstitialRecord];
const error = GatherRunner.getInterstitialError(mainRecord, records);
expect(error).toBeUndefined();
});
});

describe('#getPageLoadError', () => {
Expand All @@ -1019,6 +1035,16 @@ describe('GatherRunner', function() {
expect(error).toBeUndefined();
});

it('passes when the page is loaded, ignoring any fragment', () => {
const url = 'http://example.com/#/page/list';
const mainRecord = new NetworkRequest();
const passContext = {url, driver: {online: true}};
const loadData = {networkRecords: [mainRecord]};
mainRecord.url = 'http://example.com';
const error = GatherRunner.getPageLoadError(passContext, loadData, undefined);
expect(error).toBeUndefined();
});

it('passes when the page is offline', () => {
const passContext = {url: 'http://the-page.com', driver: {online: false}};
const mainRecord = new NetworkRequest();
Expand Down