feat: Do not strip CSP headers from HTTPResponse

[pgoforth]

• Closes Issue #1030

User facing changelog

• Cypress no longer strips Content-Security-Policy and Content-Security-Policy-Report-Only headers from requests.

Additional details

• add generated nonce to inline script injection
• append generated nonce policy value to each CSP header script-src directive
• remove content-security-policy and content-security-policy-report-only header stripping

Steps to test

// TODO

How has the user experience changed?

This change does not affect UI/UX

PR Tasks

• Have tests been added/updated?
• Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• [na] Has a PR for user-facing changes been opened in cypress-documentation?
• [na] Have API changes been updated in the type definitions?

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[CLAassistant]

All committers have signed the CLA.

[pgoforth]

I decided to try my hand at remedying the CSP header issue reported by @bahmutov here and implement the suggestion from @flotwig here.
I generated a nonce and added it into into the CSP headers we are intercepting from the HtTPResponse, and removed the stripping of those headers.

I've been having issues building Cypress from source, so I've been unable to run a build and test these changes locally. Any help in this regard would be appreciated.

[flotwig]

Nice, thanks for taking this on.

I've been having issues building Cypress from source, so I've been unable to run a build and test these changes locally. Any help in this regard would be appreciated.

@pgoforth What's the error you're encountering?

[pgoforth]

@pgoforth What's the error you're encountering?
@flotwig
I use nvm for node management, and switched to Node v16.18.1 and am running Yarn v1.19.1. Run a yarn install, then: yarn start

Error that I see is:

(node:34477) UnhandledPromiseRejectionWarning: SyntaxError: Cannot use import statement outside a module
/Users/pgoforth/Develop/pgoforth-cypress/packages/server/lib/modes/index.ts:1
import { setCtx } from '@packages/data-context'

As an aside, I am using yarn v1.19.1 because I could not get anything to work with yarn v3 due to this error:

Internal Error: Duplicate workspace name cypress: /Users/pgoforth/Develop/pgoforth-cypress/cli conflicts with /Users/pgoforth/Develop/pgoforth-cypress

[flotwig]

@pgoforth I see some commits being pushed up - are you unblocked? I tried to reproduce the "import" issue, but was unable to on Linux. My next step was to try reproducing the issue you were hitting on a Mac.

[pgoforth]

@pgoforth I see some commits being pushed up - are you unblocked? I tried to reproduce the "import" issue, but was unable to on Linux. My next step was to try reproducing the issue you were hitting on a Mac.

@flotwig I have not pushed any updates. I have been rebasing in the hopes that I would get unblocked by other work going into develop, but it has not unblocked me.

[flotwig]

@pgoforth Ok, I just tried to yarn and yarn start with Node 16.18.1 and Yarn 1.22.19 on macOS Ventura on a fresh clone, and it worked. So I'm not able to reproduce the issue...

We do use Yarn Classic and not Yarn v3. Did you try a fresh clone/git clean -xfd after trying Yarn v3? I wonder if it could have left some files that are causing the build to misbehave.

If you still are having trouble after building from a clean clone, please share the full logs, I can take a look.

[pgoforth]

@flotwig Good thought. I tried a fresh clone and was able to get everything running. I am unblocked, and am going to run my changes through local manual testing.

[pgoforth]

@flotwig and perhaps @mjhenkes
I have everything running and CSP headers are being allowed through and respected...however I have run into a snag using the nonce method.
It seems this line of code is causing the injected script not to run because:

Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' 'nonce-fakeNonce' 'nonce-Avwln1epGsuyPN8OYPPQ2g=='"

Apparently eval is so dangerous, you must explicitly allow it in the CSP directive, even if you have assigned a nonce to the injected script. Can either of you think of a way around this? A different way of implementing the same behavior with the test runner without using an eval?

Edit:
Perhaps adding unsafe-eval is a sufficient compromise for a testing library. It would be the only aspect of your CSP you could not test, and it would get us 99% of the way there...so I'm looking for feedback on it. I'm going to test it today and see if it works.

For reference on the issue:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#unsafe_eval_expressions
https://csplite.com/csp/test152/

Edit2:
@flotwig
Adding unsafe-eval was 100% successful. Cypress is now allowing the CSP headers through, running tests successfully. This might be a nice compromise for the ability to test every other CSP header. Thoughts?

[flotwig]

Adding unsafe-eval was 100% successful. Cypress is now allowing the CSP headers through, running tests successfully. This might be a nice compromise for the ability to test every other CSP header. Thoughts?

@pgoforth Been thinking about this and ultimately I think that this is too dangerous. We want apps running in Cypress to run as closely as possible to how they'd run in a web browser. If we added unsafe-eval to CSPs, users could push code changes that use eval, all their tests would pass, and then it would completely break in production. What do you think? Is there another way?

[bahmutov]

You can serve the cypress domain snippet via a script and not inline. Thus you could use the original csp plus insert a nonce for just the single added scriptSent from my iPhoneOn Dec 7, 2022, at 17:07, Zach Bloomquist ***@***.***> wrote: Adding unsafe-eval was 100% successful. Cypress is now allowing the CSP headers through, running tests successfully. This might be a nice compromise for the ability to test every other CSP header. Thoughts? @pgoforth Been thinking about this and ultimately I think that this is too dangerous. We want apps running in Cypress to run as closely as possible to how they'd run in a web browser. If we added unsafe-eval to CSPs, users could push code changes that use eval, all their tests would pass, and then it would completely break in production. What do you think? Is there another way? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[pgoforth]

You can serve the cypress domain snippet via a script and not inline. Thus you could use the original csp plus insert a nonce for just the single added script

@bahmutov Are you talking about this line of code? The only items necessitating unsafe-eval are pieces of the codebase that are allowing Cypress to execute code in the browser window context via window.eval or Function(). Even if you externalize that code and load it through a tag, the eval would still be prevented even when you add a nonce to the loaded script tag.

What do you think? Is there another way?

@flotwig
I'm going to give it some thought, but unless we eliminate the need to execute arbitrary code in the browser window context there won't be a way around it. Where are all the instances in Cypress (both methods and internals) where we allow arbitrary code execution? If we find all instances where we would normally eval and can replace with static methods, it might work. There might also be a way to run code in an iframe that could access the window context, and we could relay through postMessage. We can green-light iframes with nonces the same as scripts. I'll go down those paths, but any help in tracking down the eval outliers would be helpful.

Edit:
Additionally, Cypress is currently allowing unsafe-eval by removing CSP headers. Is the concern that any CSP tests written would give people a false sense of security because they might be circumvented by an eval? Even if we allow unsafe-eval, IMO the product is still better off than it was. Is there an attack vector I'm missing?

Edit2:
A solution I did find was to listen for uncaught exceptions and detect if Cypress inline script unsafe-eval was the culprit. Without it however, any test using an internal Cypress method using wrapped timers will fail:

cy.on('uncaught:exception', err => {
    expect(err.message).to.include('unsafe-eval');
    expect(err.stack).to.include('/injection/main.js');
    return false;
});

I just see many other instances where code is injected, triggering unsafe-eval and I have no idea where it's coming from, like in the attached screenshot:

It seems to be a part of the cypress_cross_origin_runner but I cannot find the code.

[pgoforth]

@flotwig and @bahmutov
I have gone through and removed the unsafe-eval from the CSP headers. We are now only adding the nonce value. It turns out that the errors being thrown by Cypress on the time wraps were errors that SHOULD be thrown regardless, because they are trying to emulate things like setTimeout('alert("This causes unsafe-eval also")', 0) , which also trigger unsafe eval. I feel like the PR is ready now.

[cypress]

---

Test summary

26418 • 0 • 1179 • 0 • 44

---

Run details

Project	cypress
Status	Passed
Commit	daa9fe4
Started	Jan 4, 2023 3:58 PM
Ended	Jan 4, 2023 4:17 PM
Duration	18:47 💡
OS	Linux Debian -
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Flakiness

	create-from-component.cy.ts		2
1	... > runs generated spec
2	... > runs generated spec
	commands/net_stubbing.cy.ts		3
1	network stubbing > intercepting request > can delay and throttle a StaticResponse
2	network stubbing > intercepting request > can delay and throttle a StaticResponse
3	... > stops waiting when an fetch request is canceled
This comment includes only the first 5 flaky tests. See all 44 flaky tests in the Cypress Dashboard.

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[flotwig]

@pgoforth Sorry it took me so long to get to reviewing this again. I reviewed your changes and they looked good, really nice job on this. I pushed up a small style fix and an e2e test. Once we get another review on this it can merge.

[flotwig]

For some reason I always thought that this was a hash of the script's source, this is way simpler than I thought.

[bahmutov]

Once this lands and is released it would be really cool to intercept and tear CSP violation reporting Sent from my iPhoneOn Jan 11, 2023, at 10:59, Zach Bloomquist ***@***.***> wrote: @flotwig approved this pull request. @pgoforth Sorry it took me so long to get to reviewing this again. I reviewed your changes and they looked good, really nice job on this. I pushed up a small style fix and an e2e test. Once we get another review on this it can merge. In packages/proxy/lib/http/response-middleware.ts:

@@ -311,6 +313,36 @@ const SetInjectionLevel: ResponseMiddleware = function () {

// We set the header here only for proxied requests that have scripts injected that set the domain. // Other proxied requests are ignored. this.res.setHeader('Origin-Agent-Cluster', '?0') + + // Only patch the headers that are being supplied by the response + const incomingCSPHeaders = ['content-security-policy', 'content-security-policy-report-only'] + .filter((headerName) => hasCspHeader(this.incomingRes.headers, headerName)) + + if (incomingCSPHeaders.length) { + // In order to allow the injected script to run on sites with a CSP header + // we must add a generated `nonce` into the response headers + const nonce = crypto.randomBytes(16).toString('base64') For some reason I always thought that this was a hash of the script's source, this is way simpler than I thought. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[flotwig]

Once this lands and is released it would be really cool to intercept and tear CSP violation reporting

@bahmutov timely request :) check out 12.2.0, we added support for resourceType to cy.intercept: https://docs.cypress.io/guides/references/changelog#12-2-0

So you can do this for example to catch all CSP reports:

cy.intercept({ resourceType: 'cspviolationreport' }).as('cspViolationReport')

[flotwig]

@pgoforth Thank you for the contribution, again, great work. This will come out with the next release of Cypress at the beginning of next week.