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.

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

new_audit: ensure clickjacking mitigation through XFO or CSP #16290

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
4 changes: 4 additions & 0 deletions cli/test/smokehouse/core-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import a11y from './test-definitions/a11y.js';
import byteEfficiency from './test-definitions/byte-efficiency.js';
import byteGzip from './test-definitions/byte-gzip.js';
import clickjackingMissingHeaders from './test-definitions/clickjacking-missing-headers.js';
import clickjackingMitigationPresent from './test-definitions/clickjacking-mitigation-headers-present.js';
import crash from './test-definitions/crash.js';
import cspAllowAll from './test-definitions/csp-allow-all.js';
import cspBlockAll from './test-definitions/csp-block-all.js';
Expand Down Expand Up @@ -69,6 +71,8 @@ const smokeTests = [
a11y,
byteEfficiency,
byteGzip,
clickjackingMissingHeaders,
clickjackingMitigationPresent,
crash,
cspAllowAll,
cspBlockAll,
Expand Down
Empty file modified cli/test/smokehouse/frontends/smokehouse-bin.js
100644 → 100755
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

/**
* @type {Smokehouse.ExpectedRunnerResult}
* Expected Lighthouse results for a site with missing Clickjacking mitigations
* (through the X-Frame-Options or Content-Security-Policy headers).
*/
const expectations = {
lhr: {
requestedUrl: 'https://example.com/',
finalDisplayedUrl: 'https://example.com/',
audits: {
'clickjacking-mitigation': {
score: 0,
details: {
items: [
{
description: 'No Clickjacking mitigation found.',
severity: 'High',
},
],
},
},
},
},
};

export default {
id: 'clickjacking-missing-headers',
expectations,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

/**
* @type {Smokehouse.ExpectedRunnerResult}
* Expected Lighthouse results for a site with present Clickjacking mitigations
* (through the X-Frame-Options or Content-Security-Policy headers).
*/
const expectations = {
lhr: {
requestedUrl: 'https://developer.mozilla.org/en-US/',
finalDisplayedUrl: 'https://developer.mozilla.org/en-US/',
audits: {
'clickjacking-mitigation': {
score: null,
},
},
},
};

export default {
id: 'clickjacking-mitigation-headers-present',
expectations,
};
159 changes: 159 additions & 0 deletions core/audits/clickjacking-mitigation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import {Audit} from './audit.js';
import {MainResource} from '../computed/main-resource.js';
import * as i18n from '../lib/i18n/i18n.js';

const UIStrings = {
/** Title of a Lighthouse audit that evaluates whether the set CSP or XFO header is mitigating Clickjacking attacks. "XFO" stands for "X-Frame-Options". "CSP" stands for "Content-Security-Policy". */
title: 'Ensure Clickjacking mitigation through XFO or CSP.',
/** Description of a Lighthouse audit that evaluates whether the set CSP or XFO header is mitigating Clickjacking attacks. This is displayed after a user expands the section to see more. No character length limits. The last sentence starting with 'Learn' becomes link text to additional documentation. "XFO" stands for "X-Frame-Options". "CSP" stands for "Content-Security-Policy". */
description: 'Deployment of either the X-Frame-Options or Content-Security-Policy (with the frame-ancestors directive) header will prevent Clickjacking attacks. While the XFO header is simpler to deploy, the CSP header is more flexible. [Learn more about mitigating Clickjacking with XFO and CSP](https://developer.chrome.com/docs/lighthouse/best-practices/clickjacking-mitigation).',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: 'Deployment of either the X-Frame-Options or Content-Security-Policy (with the frame-ancestors directive) header will prevent Clickjacking attacks. While the XFO header is simpler to deploy, the CSP header is more flexible. [Learn more about mitigating Clickjacking with XFO and CSP](https://developer.chrome.com/docs/lighthouse/best-practices/clickjacking-mitigation).',
description: 'The `X-Frame-Options` (XFO) header or the `frame-ancestors` directive in the `Content-Security-Policy` (CSP) header can be used to mitigate clickjacking attacks. While the XFO header is simpler to deploy, the `frame-ancestors` CSP directive is more flexible. [Learn more about mitigating clickjacking](https://developer.chrome.com/docs/lighthouse/best-practices/clickjacking-mitigation).',

Also what makes the XFO simpler to deploy? Seems like they have the same level of complexity to deploy (just modify header values).

/** Summary text for the results of a Lighthouse audit that evaluates whether the set CSP or XFO header is mitigating Clickjacking attacks. This is displayed if there is neither a CSP nor XFO header deployed. "XFO" stands for "X-Frame-Options". "CSP" stands for "Content-Security-Policy". */
noClickjackingMitigation: 'No Clickjacking mitigation found.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
noClickjackingMitigation: 'No Clickjacking mitigation found.',
noClickjackingMitigation: 'No XFO or CSP frame-ancestors found',

/** Label for a column in a data table; entries will be a directive of the XFO or CSP header. "XFO" stands for "X-Frame-Options". "CSP" stands for "Content-Security-Policy". */
columnDirective: 'Directive',
/** Label for a column in a data table; entries will be the severity of an issue with the XFO or CSP header. "XFO" stands for "X-Frame-Options". "CSP" stands for "Content-Security-Policy". */
columnSeverity: 'Severity',
};

const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings);

class ClickjackingMitigation extends Audit {
/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'clickjacking-mitigation',
scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE,
title: str_(UIStrings.title),
description: str_(UIStrings.description),
requiredArtifacts: ['devtoolsLogs', 'URL'],
supportedModes: ['navigation'],
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @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);

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);

// Sanitize the XFO header value.
xfoHeaders = xfoHeaders.map(v => v.toLowerCase().replace(/\s/g, ''));

return {cspHeaders, xfoHeaders};
}

/**
* @param {string | undefined} directive
* @param {LH.IcuMessage | string} findingDescription
* @param {LH.IcuMessage=} severity
* @return {LH.Audit.Details.TableItem}
*/
static findingToTableItem(directive, findingDescription, severity) {
return {
directive: directive,
description: findingDescription,
severity,
};
}

/**
* @param {string[]} cspHeaders
* @param {string[]} xfoHeaders
* @return {{score: number, results: LH.Audit.Details.TableItem[]}}
*/
static constructResults(cspHeaders, xfoHeaders) {
const rawXfo = [...xfoHeaders];
const allowedDirectives = ['deny', 'sameorigin'];

// If there is none of the two headers, return early.
if (!rawXfo.length && !cspHeaders.length) {
return {
score: 0,
results: [{
severity: str_(i18n.UIStrings.itemSeverityHigh),
description: str_(UIStrings.noClickjackingMitigation),
directive: undefined,
}],
};
}
Comment on lines +92 to +102
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If there is none of the two headers, return early.
if (!rawXfo.length && !cspHeaders.length) {
return {
score: 0,
results: [{
severity: str_(i18n.UIStrings.itemSeverityHigh),
description: str_(UIStrings.noClickjackingMitigation),
directive: undefined,
}],
};
}

This is redundant, if the header lists are empty we will just skip to the bottom anyway.


// Check for frame-ancestors in CSP.
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: []};
}
}
}
Comment on lines +104 to +112
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Check for frame-ancestors in CSP.
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: []};
}
}
}
// Check for frame-ancestors in CSP.
for (const cspHeader of cspHeaders) {
if (cspHeader.includes('frame-ancestors')) {
// Pass the audit if frame-ancestors is present.
return {score: 1, results: []};
}
}


for (const actualDirective of xfoHeaders) {
if (allowedDirectives.includes(actualDirective)) {
// DENY or SAMEORIGIN are present.
return {score: 1, results: []};
}
}

return {
score: 0,
results: [{
severity: str_(i18n.UIStrings.itemSeverityHigh),
description: str_(UIStrings.noClickjackingMitigation),
directive: undefined,
}],
};
}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
const {cspHeaders, xfoHeaders} = await this.getRawCspsAndXfo(artifacts, context);
const {score, results} = this.constructResults(cspHeaders, xfoHeaders);

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
/* eslint-disable max-len */
{key: 'description', valueType: 'text', subItemsHeading: {key: 'description'}, label: str_(i18n.UIStrings.columnDescription)},
{key: 'directive', valueType: 'code', subItemsHeading: {key: 'directive'}, label: str_(UIStrings.columnDirective)},
{key: 'severity', valueType: 'text', subItemsHeading: {key: 'severity'}, label: str_(UIStrings.columnSeverity)},
/* eslint-enable max-len */
];
const details = Audit.makeTableDetails(headings, results);

return {
score,
notApplicable: !results.length,
details,
};
}
}

export default ClickjackingMitigation;
export {UIStrings};
2 changes: 2 additions & 0 deletions core/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ const defaultConfig = {
'csp-xss',
'has-hsts',
'origin-isolation',
'clickjacking-mitigation',
'script-treemap-data',
'accessibility/accesskeys',
'accessibility/aria-allowed-attr',
Expand Down Expand Up @@ -545,6 +546,7 @@ const defaultConfig = {
{id: 'csp-xss', weight: 0, group: 'best-practices-trust-safety'},
{id: 'has-hsts', weight: 0, group: 'best-practices-trust-safety'},
{id: 'origin-isolation', weight: 0, group: 'best-practices-trust-safety'},
{id: 'clickjacking-mitigation', weight: 0, group: 'hidden'},
// User Experience
{id: 'paste-preventing-inputs', weight: 3, group: 'best-practices-ux'},
{id: 'image-aspect-ratio', weight: 1, group: 'best-practices-ux'},
Expand Down
4 changes: 3 additions & 1 deletion core/scripts/i18n/collect-strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -732,6 +732,7 @@ function checkKnownFixedCollisions(strings) {
'Directive',
'Directive',
'Directive',
'Directive',
'Document contains a $MARKDOWN_SNIPPET_0$ that triggers $MARKDOWN_SNIPPET_1$',
'Document contains a $MARKDOWN_SNIPPET_0$ that triggers $MARKDOWN_SNIPPET_1$',
'Document has a valid $MARKDOWN_SNIPPET_0$',
Expand All @@ -753,6 +754,7 @@ function checkKnownFixedCollisions(strings) {
'Severity',
'Severity',
'Severity',
'Severity',
'The page was evicted from the cache to allow another page to be cached.',
'The page was evicted from the cache to allow another page to be cached.',
'Use $MARKDOWN_SNIPPET_0$ to detect unused JavaScript code. $LINK_START_0$Learn more$LINK_END_0$',
Expand All @@ -764,7 +766,7 @@ function checkKnownFixedCollisions(strings) {
'Use the $MARKDOWN_SNIPPET_0$ component instead of $MARKDOWN_SNIPPET_1$ to automatically lazy-load images. $LINK_START_0$Learn more$LINK_END_0$.',
'Use the $MARKDOWN_SNIPPET_0$ component instead of $MARKDOWN_SNIPPET_1$ to automatically lazy-load images. $LINK_START_0$Learn more$LINK_END_0$.',
'Use the $MARKDOWN_SNIPPET_0$ component instead of $MARKDOWN_SNIPPET_1$ to automatically optimize image format. $LINK_START_0$Learn more$LINK_END_0$.',
'Use the $MARKDOWN_SNIPPET_0$ component instead of $MARKDOWN_SNIPPET_1$ to automatically optimize image format. $LINK_START_0$Learn more$LINK_END_0$.',
'Use the $MARKDOWN_SNIPPET_0$ component instead of $MARKDOWN_SNIPPET_1$ to automatically optimize image format. $LINK_START_0$Learn more$LINK_END_0$.'
]);
} catch (err) {
console.log('The number of duplicate strings has changed. Consider duplicating the `description` to match existing strings so they\'re translated together or update this assertion if they must absolutely be translated separately');
Expand Down
Loading