From 1717db06a9e1aaec8e830a2eccd09199b15691ce Mon Sep 17 00:00:00 2001 From: Sebastian Neuner Date: Thu, 19 Dec 2024 11:08:32 +0100 Subject: [PATCH] Remove unnecessary meta element lookups. --- core/audits/clickjacking-mitigation.js | 49 +++---- .../audits/clickjacking-mitigation-test.js | 129 ++---------------- 2 files changed, 25 insertions(+), 153 deletions(-) diff --git a/core/audits/clickjacking-mitigation.js b/core/audits/clickjacking-mitigation.js index 32659540b316..1ac4cbe42c8e 100644 --- a/core/audits/clickjacking-mitigation.js +++ b/core/audits/clickjacking-mitigation.js @@ -33,7 +33,7 @@ class ClickjackingMitigation extends Audit { scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE, title: str_(UIStrings.title), description: str_(UIStrings.description), - requiredArtifacts: ['devtoolsLogs', 'MetaElements', 'URL'], + requiredArtifacts: ['devtoolsLogs', 'URL'], supportedModes: ['navigation'], }; } @@ -41,44 +41,29 @@ class ClickjackingMitigation extends Audit { /** * @param {LH.Artifacts} artifacts * @param {LH.Audit.Context} context - * @return {Promise<{cspHeadersAndMetaTags: string, xfoHeaders: string[]}>} + * @return {Promise<{cspHeaders: string[], xfoHeaders: string[]}>} */ static async getRawCspsAndXfo(artifacts, context) { const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS]; const mainResource = await MainResource.request({devtoolsLog, URL: artifacts.URL}, context); - let cspMetaTags = ['']; - - const cspHeaders = - mainResource.responseHeaders - .filter(h => { - return h.name.toLowerCase() === 'content-security-policy'; - }) - .flatMap(h => h.value.split(',')); + + const cspHeaders = mainResource.responseHeaders + .filter(h => { + return h.name.toLowerCase() === 'content-security-policy'; + }) + .flatMap(h => h.value.split(',')) + .filter(rawCsp => rawCsp.replace(/\s/g, '')); let xfoHeaders = mainResource.responseHeaders .filter(h => { return h.name.toLowerCase() === 'x-frame-options'; }) .flatMap(h => h.value); - if (undefined !== artifacts.MetaElements) { - cspMetaTags = - artifacts.MetaElements - .filter(m => { - return m.httpEquiv && - m.httpEquiv.toLowerCase() === 'content-security-policy'; - }) - .flatMap(m => (m.content || '').split(',')); - } - - const cspHeadersAndMetaTags = - cspHeaders.map(v => v.toLowerCase()) - .concat(cspMetaTags.map(v => v.toLowerCase())) - .join(';').replace(/\s/g, ''); // Sanitize the XFO header value. xfoHeaders = xfoHeaders.map(v => v.toLowerCase().replace(/\s/g, '')); - return {cspHeadersAndMetaTags, xfoHeaders}; + return {cspHeaders, xfoHeaders}; } /** @@ -96,16 +81,16 @@ class ClickjackingMitigation extends Audit { } /** - * @param {string} cspHeadersAndMetaTags + * @param {string[]} cspHeaders * @param {string[]} xfoHeaders * @return {{score: number, results: LH.Audit.Details.TableItem[]}} */ - static constructResults(cspHeadersAndMetaTags, xfoHeaders) { + static constructResults(cspHeaders, xfoHeaders) { const rawXfo = [...xfoHeaders]; const allowedDirectives = ['deny', 'sameorigin']; // If there is none of the two headers, return early. - if (!rawXfo.length && !cspHeadersAndMetaTags.length) { + if (!rawXfo.length && !cspHeaders.length) { return { score: 0, results: [{ @@ -117,8 +102,8 @@ class ClickjackingMitigation extends Audit { } // Check for frame-ancestors in CSP. - if (cspHeadersAndMetaTags.length) { - for (const cspDirective of cspHeadersAndMetaTags.split(';')) { + if (cspHeaders.length) { + for (const cspDirective of cspHeaders) { if (cspDirective.includes('frame-ancestors')) { // Pass the audit if frame-ancestors is present. return {score: 1, results: []}; @@ -149,8 +134,8 @@ class ClickjackingMitigation extends Audit { * @return {Promise} */ static async audit(artifacts, context) { - const {cspHeadersAndMetaTags, xfoHeaders} = await this.getRawCspsAndXfo(artifacts, context); - const {score, results} = this.constructResults(cspHeadersAndMetaTags, xfoHeaders); + const {cspHeaders, xfoHeaders} = await this.getRawCspsAndXfo(artifacts, context); + const {score, results} = this.constructResults(cspHeaders, xfoHeaders); /** @type {LH.Audit.Details.Table['headings']} */ const headings = [ diff --git a/core/test/audits/clickjacking-mitigation-test.js b/core/test/audits/clickjacking-mitigation-test.js index 7a00bae44db6..1a025655959e 100644 --- a/core/test/audits/clickjacking-mitigation-test.js +++ b/core/test/audits/clickjacking-mitigation-test.js @@ -14,7 +14,6 @@ it('marked N/A if no violations found', async () => { mainDocumentUrl: 'https://example.com', finalDisplayedUrl: 'https://example.com', }, - MetaElements: [], devtoolsLogs: { defaultPass: networkRecordsToDevtoolsLog([ { @@ -39,7 +38,6 @@ it('marked N/A if no violations found', async () => { it('No XFO header but CSP header found', async () => { const artifacts = { - MetaElements: [], devtoolsLogs: { defaultPass: networkRecordsToDevtoolsLog([ { @@ -69,7 +67,6 @@ it('No XFO header but CSP header found', async () => { it('No CSP header but XFO header found', async () => { const artifacts = { - MetaElements: [], devtoolsLogs: { defaultPass: networkRecordsToDevtoolsLog([ { @@ -98,39 +95,6 @@ it('No CSP header but XFO header found', async () => { }); it('No CSP and no XFO headers but foo header found', async () => { - const artifacts = { - MetaElements: [], - devtoolsLogs: { - defaultPass: networkRecordsToDevtoolsLog([ - { - url: 'https://example.com', - responseHeaders: [ - {name: 'Foo-Header', value: `same-origin`}, - ], - }, - ]), - }, - URL: { - requestedUrl: 'https://example.com', - mainDocumentUrl: 'https://example.com', - finalDisplayedUrl: 'https://example.com', - }, - }; - - const results = - await ClickjackingMitigation.audit(artifacts, {computedCache: new Map()}); - expect(results.notApplicable).toBeFalsy(); - expect(results.details.items[0].severity).toBeDisplayString('High'); - expect(results.details.items[0].description) - .toBeDisplayString('No Clickjacking mitigation found.'); - expect(results.details.items).toMatchObject([ - { - directive: undefined, - }, - ]); -}); - -it('No CSP and no XFO headers and not Meta but foo header found', async () => { const artifacts = { devtoolsLogs: { defaultPass: networkRecordsToDevtoolsLog([ @@ -164,7 +128,6 @@ it('No CSP and no XFO headers and not Meta but foo header found', async () => { it('Messed up XFO directive and no CSP present.', async () => { const artifacts = { - MetaElements: [], devtoolsLogs: { defaultPass: networkRecordsToDevtoolsLog([ { @@ -197,7 +160,6 @@ it('Messed up XFO directive and no CSP present.', async () => { it('Messed up CSP directive and no XFO present.', async () => { const artifacts = { - MetaElements: [], devtoolsLogs: { defaultPass: networkRecordsToDevtoolsLog([ { @@ -230,7 +192,6 @@ it('Messed up CSP directive and no XFO present.', async () => { it('Messed up CSP and XFO directives.', async () => { const artifacts = { - MetaElements: [], devtoolsLogs: { defaultPass: networkRecordsToDevtoolsLog([ { @@ -262,81 +223,9 @@ it('Messed up CSP and XFO directives.', async () => { ]); }); -it('Frame-ancestors in the MetaElement and no XFO present.', async () => { - const artifacts = { - MetaElements: [ - { - httpEquiv: 'Content-Security-Policy', - content: `frame-ancestors 'none'`, - }, - ], - devtoolsLogs: { - defaultPass: networkRecordsToDevtoolsLog([ - { - url: 'https://example.com', - responseHeaders: [ - ], - }, - ]), - }, - URL: { - requestedUrl: 'https://example.com', - mainDocumentUrl: 'https://example.com', - finalDisplayedUrl: 'https://example.com', - }, - }; - - const results = - await ClickjackingMitigation.audit(artifacts, {computedCache: new Map()}); - expect(results.details.items).toHaveLength(0); - expect(results.notApplicable).toBeTruthy(); -}); - -it('Messed up CSP directive in the header, but frame-ancestors in the MetaElement and no XFO present.', - async () => { - const artifacts = { - MetaElements: [ - { - httpEquiv: 'Content-Security-Policy', - content: `frame-ancestors 'self'`, - }, - ], - devtoolsLogs: { - defaultPass: networkRecordsToDevtoolsLog([ - { - url: 'https://example.com', - responseHeaders: [ - { - name: 'Content-Security-Policy', - value: - `foo-directive 'none'; report-uri https://csp.example.com` - }, - ], - }, - ]), - }, - URL: { - requestedUrl: 'https://example.com', - mainDocumentUrl: 'https://example.com', - finalDisplayedUrl: 'https://example.com', - }, - }; - - const results = - await ClickjackingMitigation.audit(artifacts, {computedCache: new Map()}); - expect(results.details.items).toHaveLength(0); - expect(results.notApplicable).toBeTruthy(); - }); - describe('getRawCspsAndXfo', () => { it('basic case', async () => { const artifacts = { - MetaElements: [ - { - httpEquiv: 'Content-Security-Policy', - content: `default-src 'none'`, - }, - ], URL: { requestedUrl: 'https://example.com', mainDocumentUrl: 'https://example.com', @@ -360,16 +249,15 @@ describe('getRawCspsAndXfo', () => { ]), }, }; - const {cspHeadersAndMetaTags, xfoHeaders} = + const {cspHeaders, xfoHeaders} = await ClickjackingMitigation.getRawCspsAndXfo(artifacts, {computedCache: new Map()}); - expect(cspHeadersAndMetaTags).toEqual(`frame-ancestors'self';default-src'none'`); + expect(cspHeaders).toEqual(["frame-ancestors 'self'"]); expect(xfoHeaders).toEqual([`sameorigin`]); }); it('ignore if empty', async () => { const artifacts = { - MetaElements: [], - URL: { + URL: { requestedUrl: 'https://example.com', mainDocumentUrl: 'https://example.com', finalDisplayedUrl: 'https://example.com', @@ -392,9 +280,9 @@ describe('getRawCspsAndXfo', () => { ]), }, }; - const {cspHeadersAndMetaTags, xfoHeaders} = + const {cspHeaders, xfoHeaders} = await ClickjackingMitigation.getRawCspsAndXfo(artifacts, {computedCache: new Map()}); - expect(cspHeadersAndMetaTags).toEqual(``); + expect(cspHeaders).toEqual([]); expect(xfoHeaders).toEqual([ `deny`, ]); @@ -402,8 +290,7 @@ describe('getRawCspsAndXfo', () => { it('ignore if only whitespace', async () => { const artifacts = { - MetaElements: [], - URL: { + URL: { requestedUrl: 'https://example.com', mainDocumentUrl: 'https://example.com', finalDisplayedUrl: 'https://example.com', @@ -426,9 +313,9 @@ describe('getRawCspsAndXfo', () => { ]), }, }; - const {cspHeadersAndMetaTags, xfoHeaders} = + const {cspHeaders, xfoHeaders} = await ClickjackingMitigation.getRawCspsAndXfo(artifacts, {computedCache: new Map()}); - expect(cspHeadersAndMetaTags).toEqual(``); + expect(cspHeaders).toEqual([]); expect(xfoHeaders).toEqual([ `deny`, ]);