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: use a strong HSTS policy #16257

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sebastian9er
Copy link

Summary

Adding a new audit to Ligththouse, which detects deviations from an optimal HSTS header deployment.

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

Link to documentation is pending internal discussions (@adamraine FYI, either of us can update the Link here once we have the blog post done).

@sebastian9er sebastian9er requested a review from a team as a code owner November 19, 2024 09:32
@sebastian9er sebastian9er requested review from adamraine and removed request for a team November 19, 2024 09:32
@adamraine adamraine changed the title Adding the HSTS audit to Lighthouse new_audit: HSTS policy check Nov 19, 2024
core/audits/has-hsts.js Show resolved Hide resolved
core/audits/has-hsts.js Outdated Show resolved Hide resolved
core/audits/has-hsts.js Outdated Show resolved Hide resolved
core/config/default-config.js Outdated Show resolved Hide resolved
core/test/audits/has-hsts-test.js Outdated Show resolved Hide resolved
core/audits/has-hsts.js Outdated Show resolved Hide resolved
@adamraine adamraine changed the title new_audit: HSTS policy check new_audit: has HSTS Nov 19, 2024
core/audits/has-hsts.js Outdated Show resolved Hide resolved
core/audits/has-hsts.js Outdated Show resolved Hide resolved
core/test/audits/has-hsts-test.js Outdated Show resolved Hide resolved
core/test/audits/has-hsts-test.js Outdated Show resolved Hide resolved
core/audits/has-hsts.js Outdated Show resolved Hide resolved
…il docs are published, some text phrasings and the smoke test assertions.
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.

Overall LGTM! Thanks for the contributions!

/** Summary text for the results of a Lighthouse audit that evaluates the HSTS header. This is displayed if no HSTS header is deployed. "HSTS" stands for "HTTP Strict Transport Security". */
noHsts: 'No HSTS header found',
/** Summary text for the results of a Lighthouse audit that evaluates the HSTS header. This is displayed if the preload directive is missing. "HSTS" stands for "HTTP Strict Transport Security". */
noPreload: 'No preload directive 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
noPreload: 'No preload directive found',
noPreload: 'No `preload` directive found',

/** Summary text for the results of a Lighthouse audit that evaluates the HSTS header. This is displayed if the preload directive is missing. "HSTS" stands for "HTTP Strict Transport Security". */
noPreload: 'No preload directive found',
/** Summary text for the results of a Lighthouse audit that evaluates the HSTS header. This is displayed if the includeSubDomains directive is missing. "HSTS" stands for "HTTP Strict Transport Security". */
noSubdomain: 'No includeSubDomains directive 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
noSubdomain: 'No includeSubDomains directive found',
noSubdomain: 'No `includeSubDomains` directive found',

/** Summary text for the results of a Lighthouse audit that evaluates the HSTS header. This is displayed if the includeSubDomains directive is missing. "HSTS" stands for "HTTP Strict Transport Security". */
noSubdomain: 'No includeSubDomains directive found',
/** Summary text for the results of a Lighthouse audit that evaluates the HSTS header. This is displayed if the max-age directive is missing. "HSTS" stands for "HTTP Strict Transport Security". */
noMaxAge: 'No max-age directive',
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
noMaxAge: 'No max-age directive',
noMaxAge: 'No `max-age` directive',

/** Summary text for the results of a Lighthouse audit that evaluates the HSTS header. This is displayed if the max-age directive is missing. "HSTS" stands for "HTTP Strict Transport Security". */
noMaxAge: 'No max-age directive',
/** Summary text for the results of a Lighthouse audit that evaluates the HSTS header. This is displayed if the provided duration for the max-age directive is too low. "HSTS" stands for "HTTP Strict Transport Security". */
lowMaxAge: 'max-age is too low',
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
lowMaxAge: 'max-age is too low',
lowMaxAge: '`max-age` is too low',

title: 'Use a strong HSTS policy',
/** Description of a Lighthouse audit that evaluates the security of a page's HSTS header. 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. "HSTS" stands for "HTTP Strict Transport Security". */
description: 'Deployment of the HSTS header significantly ' +
'reduces the risk of downgrading of and eavesdropping attacks on HTTP connections. ' +
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
'reduces the risk of downgrading of and eavesdropping attacks on HTTP connections. ' +
'reduces the risk of downgrading HTTP connections and eavesdropping attacks. ' +

scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE,
title: str_(UIStrings.title),
description: str_(UIStrings.description),
requiredArtifacts: ['devtoolsLogs', 'URL'],
Copy link
Member

Choose a reason for hiding this comment

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

Since we are only looking at the main document, this audit is only relevant to page navigations.

Suggested change
requiredArtifacts: ['devtoolsLogs', 'URL'],
requiredArtifacts: ['devtoolsLogs', 'URL'],
supportedModes: ['navigation'],

@adamraine adamraine changed the title new_audit: has HSTS new_audit: use a strong HSTS policy Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants