new_audit: has HSTS #16257
new_audit: has HSTS #16257
Conversation
… update:sample-json.
… into hsts-audit
adamraine
changed the title
| */ | ||
| const expectations = { | ||
| lhr: { | ||
| requestedUrl: 'https://hstspreload.org/', |
There was a problem hiding this comment.
Should we be worried about the HSTS policy changing on this site?
Not a huge deal, but it could cause the tests to start failing if something changes on this site.
| static get meta() { | ||
| return { | ||
| id: 'has-hsts', | ||
| scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE, |
There was a problem hiding this comment.
Leaving this as INFORMATIVE SGTM. It should have similar severity to the CSP XSS audit.
| // Sanitize the header value / directives. | ||
| hstsHeaders = hstsHeaders.map(v => v.toLowerCase().replace(/\s/g, '')); | ||
|
|
||
| return {hstsHeaders}; |
There was a problem hiding this comment.
nit: we can just return hstsHeaders. No need to return in a wrapper object.
| // If there is a directive that's not an official HSTS directive. | ||
| if (!allowedDirectives.includes(actualDirective)) { | ||
| // max-age=<number> would always trigger. | ||
| if (actualDirective.includes('max-age')) { |
There was a problem hiding this comment.
nit: can we just add this to the condition above with an || operator
| @@ -541,6 +542,7 @@ const defaultConfig = { | |||
| {id: 'geolocation-on-start', weight: 1, group: 'best-practices-trust-safety'}, | |||
| {id: 'notification-on-start', weight: 1, group: 'best-practices-trust-safety'}, | |||
| {id: 'csp-xss', weight: 0, group: 'best-practices-trust-safety'}, | |||
| {id: 'has-hsts', weight: 0, group: 'best-practices-trust-safety'}, | |||
There was a problem hiding this comment.
Can we do this until documentation is published? This will keep the audit in the JSON result but hide it for anyone looking at the report UI.
| {id: 'has-hsts', weight: 0, group: 'best-practices-trust-safety'}, | |
| {id: 'has-hsts', weight: 0}, |
| severity: { | ||
| i18nId: "core/lib/i18n/i18n.js | itemSeverityMedium", | ||
| values: undefined, | ||
| formattedDefault: 'Medium' |
There was a problem hiding this comment.
Pro tip: we have a custom assertion check for display strings (example)
So this could be expect(results.details.items[0].severity).toBeDisplayString('Medium'). Then you can drop the i18nId/values stuff from expectations.
| f.directive, f.description, | ||
| str_(i18n.UIStrings.itemSeverityLow))), | ||
| ]; | ||
| return {score: violations.length ? 0 : 1, results}; |
There was a problem hiding this comment.
Should also fail if syntax.length > 1 IMO
| finalDisplayedUrl: 'https://developer.mozilla.org/en-US/', | ||
| audits: { | ||
| 'has-hsts': { | ||
| score: 1, |
There was a problem hiding this comment.
What are the HSTS header issues? Could we add some assertions for that?
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).