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

feat: Add documentation for experimentalCspAllowList and CSP information #5194

Conversation

pgoforth
Copy link

@pgoforth pgoforth commented Apr 14, 2023

Documentation for stripCspDirectives experimentalCspAllowList feature

@netlify
Copy link

netlify bot commented Apr 14, 2023

👷 Deploy request for cypress-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 418592c

@CLAassistant
Copy link

CLAassistant commented Apr 14, 2023

CLA assistant check
All committers have signed the CLA.

@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch 3 times, most recently from 17e0d96 to 2856164 Compare April 17, 2023 14:28
@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch 3 times, most recently from 1bbe725 to 43db1d8 Compare May 15, 2023 21:46
@AtofStryker
Copy link
Contributor

NOTE: we need to update the target branch to the next release and we also need to deploy target links during the release due to https://github.com/cypress-io/cypress/pull/26483/files#diff-9fde1fb2f7eaba5ea49b585c811461495bfbba88e614e900a96863ecfe6b980aR3059

@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch from 43db1d8 to 0e61db7 Compare May 16, 2023 18:09
@pgoforth pgoforth requested a review from chrisbreiding May 16, 2023 18:09
@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch 4 times, most recently from b763f80 to 7a50b35 Compare May 23, 2023 14:04
Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

One typo commented on in-line.

@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch from 7a50b35 to e6810d3 Compare May 23, 2023 18:28
@pgoforth
Copy link
Author

@MikeMcC399 Thanks for reviewing. Fixed the typo.

@pgoforth pgoforth requested a review from MikeMcC399 May 23, 2023 18:28
@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch from e6810d3 to 382caac Compare May 23, 2023 18:33
Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

Typos are fixed. Thanks!

tests against your application. This may be desired if you are testing against
specific CSP directives.

:::
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a note that this experiment is not compatible with the experimentalSourceRewriting experiment?

Copy link
Author

Choose a reason for hiding this comment

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

@emilyrohrbough This should still be compatible in certain scenarios. Can we say may not be compatible?

Copy link
Member

Choose a reason for hiding this comment

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

@pgoforth @AtofStryker had flagged this as a possible risk / scenario where this configuration opt-in may fail.

  • JavaScript is re-written - PR does not handle this scenario
    • What we do know: Similar to SRI tags, nonces/shas need to be provided on inline-scripts CSP into the document before the actual inline script is fetched. If the inline script is rewritten by the proxy, the nonce will work fine, but the sha will break and the script will not execute. This is why when experimentalModifyObstructiveThirdPartyCode is enabled, integrity tags are stripped out of HTML documents as it is really difficult to inform the browser of the SHA to expect before the resource is rewritten and SHAed on the server (a bit of the chicken or the egg problem)

Mind adding a blurb stating if there are issues and this may be an known issue

Copy link
Member

Choose a reason for hiding this comment

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

@pgoforth I didn't see any code changes in the rewriting that would indicate this wouldn't cause issues with the nonce injection. Did you test any by chance?

Copy link
Author

@pgoforth pgoforth Jun 1, 2023

Choose a reason for hiding this comment

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

@emilyrohrbough Nonce injection is completely independent of SHA hashing. They are handled differently through CSP.

  • A SHA hash is an inline validation against the script that gets downloaded and executed. If the version downloaded does not match the SHA hash, the script is not executed.
  • A nonce value is randomly generated at runtime and does not rely on the file hash at all.

So even if Cypress made inline changes to a script, the nonce would still pass CSP validation. We are generating a nonce for inline script injections, but are currently not generating a nonce for scripts that have been modified by Cypress. experimentalSourceRewriting does not trigger a CSP violation by default unless a SHA is used on the script node in question. Therefore, allowing CSP headers is unlikely to cause a the source rewrite code to fail CSP unless there are already specific directive values in place that validate against those scripts. That's why I said it may be an issue, because it's totally dependent on the original CSP headers that Cypress intercepts.

Edit:
I think I was being unclear on re-reading what I've written. SRI integrity attributes do not function the same way CSP nonce attributes do. They are mutually exclusive. Adding a nonce after already removing the integrity attribute would be moot. The CSP headers that affect rewrites are 'trusted-types' and 'require-trusted-types-for', which we will ALWAYS strip.

Copy link
Contributor

Choose a reason for hiding this comment

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

the main issue with SHA inline scripts is if the script is rewritten and the sha doesnt match the actual hash. But I think it might be OK to say it may work with experimentalSourceRewriting because it is going to depend on the context of what is rewritten, similar to how things work now with the regex rewriter

Copy link
Author

Choose a reason for hiding this comment

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

We can probably specify the scenario (using '<hash-algorithm>-<base64-value>' inside a CSP directive) as that's the only place it could trip against experimentalSourceRewriting or experimentalModifyObstructiveThirdPartyCode/modifyObstructiveCode.

I also genuinely feel like that is a problem that can be solved given our ability to modify individual directive values. We would just need to match the original SHA in the CSP directive with the one we replaced, and swap the value for the replacement. Outside of the scope of this PR, but definitely doable in the near future.

@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch from 4ffcfcc to 55fe82f Compare June 1, 2023 17:24
docs/guides/references/experiments.mdx Outdated Show resolved Hide resolved
tests against your application. This may be desired if you are testing against
specific CSP directives.

:::
Copy link
Member

Choose a reason for hiding this comment

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

@pgoforth @AtofStryker had flagged this as a possible risk / scenario where this configuration opt-in may fail.

  • JavaScript is re-written - PR does not handle this scenario
    • What we do know: Similar to SRI tags, nonces/shas need to be provided on inline-scripts CSP into the document before the actual inline script is fetched. If the inline script is rewritten by the proxy, the nonce will work fine, but the sha will break and the script will not execute. This is why when experimentalModifyObstructiveThirdPartyCode is enabled, integrity tags are stripped out of HTML documents as it is really difficult to inform the browser of the SHA to expect before the resource is rewritten and SHAed on the server (a bit of the chicken or the egg problem)

Mind adding a blurb stating if there are issues and this may be an known issue

@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch from 55fe82f to 2a34179 Compare June 1, 2023 21:06
@AtofStryker AtofStryker self-requested a review June 1, 2023 23:23
@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch from 2a34179 to 852deec Compare June 2, 2023 14:59
@pgoforth
Copy link
Author

pgoforth commented Jun 2, 2023

@emilyrohrbough I've addressed your comments and added a blurb about potential incompatibility with modifyObstructiveCode and experimentalSourceRewriting. Please have another look and make sure I didn't forget anything.

@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch from ad93a7c to ee1867f Compare June 5, 2023 14:28
@AtofStryker AtofStryker assigned mschile and unassigned AtofStryker Jun 6, 2023
pgoforth added 3 commits June 7, 2023 10:56
- Remove more general `stripCspDirectives` documentation
- Mention known issues related to `modifyObstructiveCode` and `experimentalSourceRewriting`
@pgoforth pgoforth force-pushed the cy-issue-1030/pgoforth/document-stripCspDirectives branch from bebfaf3 to 418592c Compare June 7, 2023 14:56
@pgoforth
Copy link
Author

pgoforth commented Jun 7, 2023

@emilyrohrbough Comments addressed.

@mschile mschile changed the base branch from main to release-12.15.0 June 7, 2023 21:46
@mschile mschile merged commit e8df9cb into cypress-io:release-12.15.0 Jun 14, 2023
@mschile mschile changed the title feat: Add documentation for stripCspDirectives and CSP information feat: Add documentation for experimentalCspAllowList and CSP information Jun 14, 2023
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.

7 participants