fix(proxy): omit CSP report only header

[sdemjanenko]

Since Cypress omits the blocking CSP header, it should also omit the
report-only CSP header. This fixes a lot of unsafe-inline script-src
warnings that I have seen when running cypress.

A test has been added to checked that this header is removed.

[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.

[jennifer-shehane]

@sdemjanenko There are some linting errors in the spec file.

You mention:

This fixes a lot of unsafe-inline script-src warnings that I have seen when running cypress.

Can you describe this in more detail? Do you have a screenshot or example of the errors that typically show?

[sdemjanenko]

Here is the console warning that shows up (and logged to our CSP reporting service):

As for the Linting warnings:

• Where do I go to see these warnings? The only error I found was the one I talk about below.
• Also, I copied the test block that checks content-security-policy is stripped out of the headers. If there are lint warnings in the test that I added, then there are lint warnings in the test that I copied. If that is the case, I recommend that the lint warnings get fixed independently to ensure the repo is in a passing state.

I also noticed that some tests failed because of a 1 character shift in the CLI output

   ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
-  │ Cypress:    1.2.3                                                                              │
+  │ Cypress:    1.2.3                                                                             │
   │ Browser:    FooBrowser 88                                                                      │
   │ Specs:      1 found (images_spec.coffee)                                                       │
   │ Searched:   cypress/integration/images_spec.coffee                                             │
   └────────────────────────────────────────────────────────────────────────────────────────────────┘

Do we know why this happens? How can we make this more stable?

Dismissing my initial review as addressed

[flotwig]

@sdemjanenko I see why you want to disable Content-Security-Policy-Report-Only - Cypress rewrites JS, so it causes many hash mismatches to throw warnings (due to the require-sri-for)

However, there are many other CSP directives, all of which can also be used with Report-Only: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy

In light of this, I do not think the correct approach is to completely strip the Content-Security-Policy-Report-Only header. Many of the warnings it throws are still useful to developers within Cypress.

You may have seen how we treat the Feature-Policy header - only the document-domain directive adversely affects Cypress, so we just strip out that single directive if it exists, without removing the other Feature-Policy directives:

cypress/packages/proxy/lib/http/response-middleware.ts

Lines 246 to 267 in d6f159b

	// https://github.com/cypress-io/cypress/issues/6480
	const MaybeStripDocumentDomainFeaturePolicy: ResponseMiddleware = function () {
	const { 'feature-policy': featurePolicy } = this.incomingRes.headers
	if (featurePolicy) {
	const directives = parseFeaturePolicy(<string>featurePolicy)
	if (directives['document-domain']) {
	delete directives['document-domain']
	const policy = stringifyFeaturePolicy(directives)
	if (policy) {
	this.res.set('feature-policy', policy)
	} else {
	this.res.removeHeader('feature-policy')
	}
	}
	}
	this.next()
	}

So what do you think about using a similar approach for Content-Security-Policy-Report-Only? Instead of stripping the entire header, you could only strip the specific directives that conflict with Cypress.

[sdemjanenko]

The decision that gets made for content-security-policy-report-only should get mirrored for content-security-policy. Usually the report-only version is a jump-off step for moving to a CSP config that blocks violations instead of just reporting on them.

Personally, I don't find much value in watching CSP reports come in from Cypress, especially because Cypress' internals do a bunch of rewriting.

[flotwig]

The decision that gets made for content-security-policy-report-only should get mirrored for content-security-policy.

That's a good point, if we decide to only strip certain CSP directives, it should apply to both headers equally.

Personally, I don't find much value in watching CSP reports come in from Cypress, especially because Cypress' internals do a bunch of rewriting.

Yeah, it would be ideal if we could eliminate the warnings caused by rewriting, while keeping the rest.

Are the unsafe-inline script-src warnings the only ones you're seeing?

[sdemjanenko]

Yes, unsafe-inline script-src is the only warning that I observe.

[flotwig]

Ok, you are probably seeing unsafe-inline warnings because Cypress injects Javascript into the page header, not because of proxy's header changes:

cypress/packages/proxy/lib/http/util/inject.ts

Lines 1 to 25 in cf1432e

	import { oneLine } from 'common-tags'
	export function partial (domain) {
	return oneLine`
	<script type='text/javascript'>
	document.domain = '${domain}';
	</script>
	`
	}
	export function full (domain) {
	return oneLine`
	<script type='text/javascript'>
	document.domain = '${domain}';
	var Cypress = window.Cypress = parent.Cypress;
	if (!Cypress) {
	throw new Error('Something went terribly wrong and we cannot proceed. We expected to find the global Cypress in the parent window but it is missing!. This should never happen and likely is a bug. Please open an issue!');
	};
	Cypress.action('app:window:before:load', window);
	</script>
	`
	}

In light of this, the best solution IMO is to always add a script-src 'unsafe-inline' directive whenever a Content-Security-Policy or Content-Security-Policy-Report-Only is encountered in the proxy, either by adding to the existing script-src directive, or by adding a new script-src directive.

I think this will get rid of these Cypress-only warnings while letting us retain the behavior of CSP, which helps accomplish the goal of making Cypress tests closer to matching the "real world".

What do you think?

[flotwig]

Slightly better idea: we could always add 'nonce-<base64-value>' as documented here, with a nonce matching the injected HTML, so we don't need to universally apply 'unsafe-inline'. The injected HTML contains variables, so you'd need to be passing the nonce around a bit, so this is a little more work.

[sdemjanenko]

I like the nonce idea best. Adding unsafe-inline feels too general.

[flotwig]

Converting this to a draft since it's not ready for review. @sdemjanenko if you pick this back up again, please feel free to tag me with any questions/when you are ready for review.

[sdemjanenko]

@flotwig I'm actually pretty annoyed by this. Given your responses indicating the desire for new functionality, I assumed you and your team would be taking that on.

I think this commit is independent of that work. This commit just brings the same behavior to the report only header as already exists for the normal header.

I would ask that you reconsider pulling in this change as is and then put that additional functionality on your roadmap for your team to do.

[flotwig]

I assumed you and your team would be taking that on.

I assumed the opposite 😄 Apologies for not communicating more clearly on that point.

I think this commit is independent of that work. This commit just brings the same behavior to the report only header as already exists for the normal header.

Thanks for the reminder, I see what you mean now. With that context, I think this is fine to merge as-is.

[flotwig]

Thanks for the PR! I'll open an issue to track the discussion we had around more selectively supressing the CSP and CSP-Report headers.

[sdemjanenko]

Thanks for the quick response. I should have followed up earlier as well. I am looking forward to seeing this hit a production release. It will fix a lot of CSP report only noise that I see.

[flotwig]

Oh wow, our very own @bahmutov opened an issue for this 3 years ago: #1030

[cypress-bot]

Released in 5.3.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v5.3.0, please open a new issue.