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

Conversation

sebastian9er
Copy link
Contributor

Summary
Adding a new audit to Ligththouse, which detects missing Clickjacking mitigation through the X-Frame-Options or Content-Security-Policy HTTP header.

Part of a larger change to introduce more similar header deployments.

Similar to the HSTS audit (#16257), the description contains a placeholder doc link until the internal doc is approved. @adamraine FYI

@sebastian9er sebastian9er requested a review from a team as a code owner December 18, 2024 13:43
@sebastian9er sebastian9er requested review from connorjclark and removed request for a team December 18, 2024 13:43
@adamraine adamraine changed the title new_audit: ensure clickjacking mitigation through xfo or csp new_audit: ensure clickjacking mitigation through XFO or CSP Dec 20, 2024
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

Can you revert your changes to cli/test/smokehouse/frontends/smokehouse-bin.js

/** 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).

/** 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).',
/** 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',

Comment on lines +92 to +102
// 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,
}],
};
}
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.

Comment on lines +104 to +112
// 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: []};
}
}
}
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: []};
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants