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

tests(smoke): verify CSP violations caused by lighthouse #12391

Merged
merged 9 commits into from
Apr 28, 2021
Merged

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Apr 21, 2021

Part of #12378

@adamraine adamraine requested a review from a team as a code owner April 21, 2021 19:02
@adamraine adamraine requested review from connorjclark and removed request for a team April 21, 2021 19:02
@google-cla google-cla bot added the cla: yes label Apr 21, 2021
},
SourceMaps: [{
// TODO: Fix frame-src violation when using iframe fetcher.
// Doesn't trigger a CSP violation because iframe is injected after InspectorIssues gatherer finishes.
Copy link
Member Author

Choose a reason for hiding this comment

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

Could we move InspectorIssues earlier in the config so CSP violations caused by Lighthouse aren't in the report.

Conversely, we could move InspectorIssues to the end so it catches issues caused by SourceMaps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would that even work since InspectorIssues stops listening in afterPass, and some (all?) of these issues come from snapshot?

... so it catches issues caused by SourceMaps.

nbd if this is just the iframe fetcher, which will be removed soon.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would that even work since InspectorIssues stops listening in afterPass, and some (all?) of these issues come from snapshot?

In current LH flow, snapshot isn't a special phase it's just immediately called by the afterPass. You have an astute observation though that in FR this isn't actually a problem because snapshot is always called after afterTimespan ;)

lighthouse-core/gather/gatherers/seo/tap-targets.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

sweeeet, nice work!

lighthouse-cli/test/fixtures/csp.html Show resolved Hide resolved
},
SourceMaps: [{
// TODO: Fix frame-src violation when using iframe fetcher.
// Doesn't trigger a CSP violation because iframe is injected after InspectorIssues gatherer finishes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would that even work since InspectorIssues stops listening in afterPass, and some (all?) of these issues come from snapshot?

In current LH flow, snapshot isn't a special phase it's just immediately called by the afterPass. You have an astute observation though that in FR this isn't actually a problem because snapshot is always called after afterTimespan ;)

@GoogleChrome GoogleChrome deleted a comment Apr 22, 2021
@GoogleChrome GoogleChrome deleted a comment Apr 22, 2021
@GoogleChrome GoogleChrome deleted a comment Apr 22, 2021
@GoogleChrome GoogleChrome deleted a comment Apr 22, 2021
@GoogleChrome GoogleChrome deleted a comment Apr 22, 2021
@GoogleChrome GoogleChrome deleted a comment Apr 22, 2021
const blockAllExceptInlineScriptCsp = headersParam([[
'Content-Security-Policy',
'default-src \'none\'; script-src \'unsafe-inline\'',
`default-src 'none'; script-src 'sha256-qZLV55/xxILbIrha9pgX0OdkZMhOlaIgfpEo/6Dly2U='`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please provide instructions on how to generate this hash. Or, generate it dynamically based on csp.html.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added instructions. We could also use a hardcoded "nonce" for this test case.

// https://github.com/GoogleChrome/lighthouse/pull/12044#issuecomment-788274938
//
// Fixed with new fetcher using M92.
_maxChromiumMilestone: 91,
Copy link
Member Author

Choose a reason for hiding this comment

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

With the new fetcher, we have our first fixed case 🎉 !

Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

LGTM

I think we'd benefit from doing the same with the dbw tester page. Making the CSP that works for that would be more work, but would allow Lighthouse gatherers that depend on certain junk on the page to do their thing and expose other CSP issues.

@adamraine adamraine mentioned this pull request Apr 28, 2021
@adamraine adamraine merged commit 79acf25 into master Apr 28, 2021
@adamraine adamraine deleted the csp-smoke branch April 28, 2021 20:43
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.

4 participants