Skip to content

Commit

Permalink
core(gather-runner): only fail for mainRecord interstitials (#9576)
Browse files Browse the repository at this point in the history
  • Loading branch information
@patrickhulce @paulirish
patrickhulce authored and paulirish committed Nov 6, 2019
1 parent 556f5c0 commit ddab768
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 47 deletions.
7 changes: 7 additions & 0 deletions lighthouse-cli/test/fixtures/badssl-iframe.html
View file
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -440,12 +440,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 @@ -464,7 +467,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 @@ -803,6 +806,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 @@ -885,14 +889,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 @@ -901,17 +898,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 @@ -922,7 +917,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 @@ -934,7 +929,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 @@ -947,19 +942,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 @@ -968,7 +967,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 @@ -980,7 +979,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 @@ -993,7 +992,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 @@ -1009,12 +1008,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 @@ -1033,6 +1049,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

0 comments on commit ddab768

Please sign in to comment.