From fb0285257e0c111a3a2a860d081a108788f9cbd5 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 28 Mar 2023 11:57:18 -0700 Subject: [PATCH 1/2] core(link-elements): gracefully handle header parser error --- core/gather/gatherers/link-elements.js | 47 ++++++++++++++----- .../gather/gatherers/link-elements-test.js | 16 +++++++ shared/localization/locales/en-US.json | 3 ++ shared/localization/locales/en-XL.json | 3 ++ 4 files changed, 58 insertions(+), 11 deletions(-) diff --git a/core/gather/gatherers/link-elements.js b/core/gather/gatherers/link-elements.js index d178c9328ca8..b1b18e14fab7 100644 --- a/core/gather/gatherers/link-elements.js +++ b/core/gather/gatherers/link-elements.js @@ -10,6 +10,8 @@ import FRGatherer from '../base-gatherer.js'; import {pageFunctions} from '../../lib/page-functions.js'; import DevtoolsLog from './devtools-log.js'; import {MainResource} from '../../computed/main-resource.js'; +import {Util} from '../../../shared/util.js'; +import * as i18n from '../../lib/i18n/i18n.js'; /* globals HTMLLinkElement getNodeDetails */ @@ -19,6 +21,17 @@ import {MainResource} from '../../computed/main-resource.js'; * headers of the main resource. */ +const UIStrings = { + /** + * @description Warning message explaining that there was an error parsing a link header in an HTTP response. `error` will be an english string with more details on the error. `header` will be the value of the header that caused the error. `link` is a type of HTTP header and should not be translated. + * @example {Expected attribute delimiter at offset 94} error + * @example {; rel=preload; as=style; nopush} error + */ + headerParseWarning: 'Error parsing `link` header ({error}):\n`{header}`', +}; + +const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings); + /** * * @param {string} url @@ -126,18 +139,29 @@ class LinkElements extends FRGatherer { for (const header of mainDocument.responseHeaders) { if (header.name.toLowerCase() !== 'link') continue; - for (const link of LinkHeader.parse(header.value).refs) { - linkElements.push({ - rel: link.rel || '', - href: normalizeUrlOrNull(link.uri, context.baseArtifacts.URL.finalDisplayedUrl), - hrefRaw: link.uri || '', - hreflang: link.hreflang || '', - as: link.as || '', - crossOrigin: getCrossoriginFromHeader(link.crossorigin), - source: 'headers', - fetchPriority: link.fetchpriority, - node: null, + try { + const parsedHeader = LinkHeader.parse(header.value); + + for (const link of parsedHeader.refs) { + linkElements.push({ + rel: link.rel || '', + href: normalizeUrlOrNull(link.uri, context.baseArtifacts.URL.finalDisplayedUrl), + hrefRaw: link.uri || '', + hreflang: link.hreflang || '', + as: link.as || '', + crossOrigin: getCrossoriginFromHeader(link.crossorigin), + source: 'headers', + fetchPriority: link.fetchpriority, + node: null, + }); + } + } catch (err) { + const truncatedHeader = Util.truncate(header.value, 100); + const warning = str_(UIStrings.headerParseWarning, { + error: err.message, + header: truncatedHeader, }); + context.baseArtifacts.LighthouseRunWarnings.push(warning); } } @@ -181,3 +205,4 @@ class LinkElements extends FRGatherer { } export default LinkElements; +export {UIStrings}; diff --git a/core/test/gather/gatherers/link-elements-test.js b/core/test/gather/gatherers/link-elements-test.js index 4a8112fda3b2..065e18f433c4 100644 --- a/core/test/gather/gatherers/link-elements-test.js +++ b/core/test/gather/gatherers/link-elements-test.js @@ -52,6 +52,7 @@ describe('Link Elements gatherer', () => { URL: { finalDisplayedUrl: url, }, + LighthouseRunWarnings: [], }; const passContext = {driver, url, baseArtifacts, computedCache: new Map()}; return [passContext, {}]; @@ -107,4 +108,19 @@ describe('Link Elements gatherer', () => { link({source: 'headers', rel: 'prefetch', href: 'https://example.com/', as: 'image'}), ]); }); + + it('adds toplevel warning on parser error', async () => { + const linkElementsInDOM = []; + const headers = [ + {name: 'Link', value: 'a'}, + ]; + + const passData = getPassData({linkElementsInDOM, headers}); + const result = await new LinkElements().afterPass(...passData); + expect(result).toEqual([]); + expect(passData[0].baseArtifacts.LighthouseRunWarnings).toHaveLength(1); + expect(passData[0].baseArtifacts.LighthouseRunWarnings[0]).toBeDisplayString( + 'Error parsing `link` header (Unexpected character "a" at offset 22):\n`a`' + ); + }); }); diff --git a/shared/localization/locales/en-US.json b/shared/localization/locales/en-US.json index 3a34f77c89dc..1c80cac6a805 100644 --- a/shared/localization/locales/en-US.json +++ b/shared/localization/locales/en-US.json @@ -1715,6 +1715,9 @@ "core/gather/driver/storage.js | warningOriginDataTimeout": { "message": "Clearing the origin data timed out. Try auditing this page again and file a bug if the issue persists." }, + "core/gather/gatherers/link-elements.js | headerParseWarning": { + "message": "Error parsing `link` header ({error}):\n`{header}`" + }, "core/lib/bf-cache-strings.js | appBanner": { "message": "Pages that requested an AppBanner are not currently eligible for back/forward cache." }, diff --git a/shared/localization/locales/en-XL.json b/shared/localization/locales/en-XL.json index 760929431a62..f6648321e694 100644 --- a/shared/localization/locales/en-XL.json +++ b/shared/localization/locales/en-XL.json @@ -1715,6 +1715,9 @@ "core/gather/driver/storage.js | warningOriginDataTimeout": { "message": "Ĉĺêár̂ín̂ǵ t̂h́ê ór̂íĝín̂ d́ât́â t́îḿêd́ ôút̂. T́r̂ý âúd̂ít̂ín̂ǵ t̂h́îś p̂áĝé âǵâín̂ án̂d́ f̂íl̂é â b́ûǵ îf́ t̂h́ê íŝśûé p̂ér̂śîśt̂ś." }, + "core/gather/gatherers/link-elements.js | headerParseWarning": { + "message": "Êŕr̂ór̂ ṕâŕŝín̂ǵ `link` ĥéâd́êŕ ({error}):\n`{header}`" + }, "core/lib/bf-cache-strings.js | appBanner": { "message": "P̂áĝéŝ t́ĥát̂ ŕêq́ûéŝt́êd́ âń Âṕp̂B́âńn̂ér̂ ár̂é n̂ót̂ ćûŕr̂én̂t́l̂ý êĺîǵîb́l̂é f̂ór̂ b́âćk̂/f́ôŕŵár̂d́ ĉáĉh́ê." }, From 6cb97e0b5e471819106c3460c76dcb4546fbb502 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 29 Mar 2023 10:25:51 -0700 Subject: [PATCH 2/2] comments --- core/gather/gatherers/link-elements.js | 35 ++++++++++--------- .../gather/gatherers/link-elements-test.js | 2 +- shared/localization/locales/en-US.json | 2 +- shared/localization/locales/en-XL.json | 2 +- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/core/gather/gatherers/link-elements.js b/core/gather/gatherers/link-elements.js index b1b18e14fab7..61a050ce80df 100644 --- a/core/gather/gatherers/link-elements.js +++ b/core/gather/gatherers/link-elements.js @@ -27,7 +27,7 @@ const UIStrings = { * @example {Expected attribute delimiter at offset 94} error * @example {; rel=preload; as=style; nopush} error */ - headerParseWarning: 'Error parsing `link` header ({error}):\n`{header}`', + headerParseWarning: 'Error parsing `link` header ({error}): `{header}`', }; const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings); @@ -139,22 +139,11 @@ class LinkElements extends FRGatherer { for (const header of mainDocument.responseHeaders) { if (header.name.toLowerCase() !== 'link') continue; + /** @type {LinkHeader.Reference[]} */ + let parsedRefs = []; + try { - const parsedHeader = LinkHeader.parse(header.value); - - for (const link of parsedHeader.refs) { - linkElements.push({ - rel: link.rel || '', - href: normalizeUrlOrNull(link.uri, context.baseArtifacts.URL.finalDisplayedUrl), - hrefRaw: link.uri || '', - hreflang: link.hreflang || '', - as: link.as || '', - crossOrigin: getCrossoriginFromHeader(link.crossorigin), - source: 'headers', - fetchPriority: link.fetchpriority, - node: null, - }); - } + parsedRefs = LinkHeader.parse(header.value).refs; } catch (err) { const truncatedHeader = Util.truncate(header.value, 100); const warning = str_(UIStrings.headerParseWarning, { @@ -163,6 +152,20 @@ class LinkElements extends FRGatherer { }); context.baseArtifacts.LighthouseRunWarnings.push(warning); } + + for (const link of parsedRefs) { + linkElements.push({ + rel: link.rel || '', + href: normalizeUrlOrNull(link.uri, context.baseArtifacts.URL.finalDisplayedUrl), + hrefRaw: link.uri || '', + hreflang: link.hreflang || '', + as: link.as || '', + crossOrigin: getCrossoriginFromHeader(link.crossorigin), + source: 'headers', + fetchPriority: link.fetchpriority, + node: null, + }); + } } return linkElements; diff --git a/core/test/gather/gatherers/link-elements-test.js b/core/test/gather/gatherers/link-elements-test.js index 065e18f433c4..77b9c126f24a 100644 --- a/core/test/gather/gatherers/link-elements-test.js +++ b/core/test/gather/gatherers/link-elements-test.js @@ -120,7 +120,7 @@ describe('Link Elements gatherer', () => { expect(result).toEqual([]); expect(passData[0].baseArtifacts.LighthouseRunWarnings).toHaveLength(1); expect(passData[0].baseArtifacts.LighthouseRunWarnings[0]).toBeDisplayString( - 'Error parsing `link` header (Unexpected character "a" at offset 22):\n`a`' + 'Error parsing `link` header (Unexpected character "a" at offset 22): `a`' ); }); }); diff --git a/shared/localization/locales/en-US.json b/shared/localization/locales/en-US.json index 1c80cac6a805..cef2d4d95b33 100644 --- a/shared/localization/locales/en-US.json +++ b/shared/localization/locales/en-US.json @@ -1716,7 +1716,7 @@ "message": "Clearing the origin data timed out. Try auditing this page again and file a bug if the issue persists." }, "core/gather/gatherers/link-elements.js | headerParseWarning": { - "message": "Error parsing `link` header ({error}):\n`{header}`" + "message": "Error parsing `link` header ({error}): `{header}`" }, "core/lib/bf-cache-strings.js | appBanner": { "message": "Pages that requested an AppBanner are not currently eligible for back/forward cache." diff --git a/shared/localization/locales/en-XL.json b/shared/localization/locales/en-XL.json index f6648321e694..0f8d170e79b9 100644 --- a/shared/localization/locales/en-XL.json +++ b/shared/localization/locales/en-XL.json @@ -1716,7 +1716,7 @@ "message": "Ĉĺêár̂ín̂ǵ t̂h́ê ór̂íĝín̂ d́ât́â t́îḿêd́ ôút̂. T́r̂ý âúd̂ít̂ín̂ǵ t̂h́îś p̂áĝé âǵâín̂ án̂d́ f̂íl̂é â b́ûǵ îf́ t̂h́ê íŝśûé p̂ér̂śîśt̂ś." }, "core/gather/gatherers/link-elements.js | headerParseWarning": { - "message": "Êŕr̂ór̂ ṕâŕŝín̂ǵ `link` ĥéâd́êŕ ({error}):\n`{header}`" + "message": "Êŕr̂ór̂ ṕâŕŝín̂ǵ `link` ĥéâd́êŕ ({error}): `{header}`" }, "core/lib/bf-cache-strings.js | appBanner": { "message": "P̂áĝéŝ t́ĥát̂ ŕêq́ûéŝt́êd́ âń Âṕp̂B́âńn̂ér̂ ár̂é n̂ót̂ ćûŕr̂én̂t́l̂ý êĺîǵîb́l̂é f̂ór̂ b́âćk̂/f́ôŕŵár̂d́ ĉáĉh́ê."