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

Figure out how to load site with Content-Security-Policy without stripping header #1030

Closed
bahmutov opened this issue Dec 7, 2017 · 10 comments · Fixed by #24760 or #26483
Closed

Figure out how to load site with Content-Security-Policy without stripping header #1030

bahmutov opened this issue Dec 7, 2017 · 10 comments · Fixed by #24760 or #26483
Labels
stage: needs review The PR code is done & tested, needs review type: feature New feature that does not currently exist

Comments

@bahmutov
Copy link
Contributor

bahmutov commented Dec 7, 2017

Feature proposal

Currently we strip "content-security-policy" when loading documents.

  • allow rewriting this header in a plugin to only strip what is preventing Cypress from iframing the site

screen shot 2017-12-06 at 8 43 01 pm

By passing through CSP, and possibly tweaking it, we should be able to add security testing to Cypress.

  • is my CSP setup correctly?
  • are my violations reported?
  • can I load code / style / object from given domain or will it throw an error?

Observation

  • if we run content security policy with report-only mode, everything is working and no problems are reported when running the test itself (and any inline scripts are executed, but reported)

using helmet to generate CSP, we can do nothing and just run in report mode.

// basically do not allow inline scripts
app.use(
  helmet.contentSecurityPolicy({
    directives: {
      scriptSrc: ["'self'"],
      reportUri: '/report-violation'
    },
    reportOnly: true
  })
)

In the following page we catch two errors:

<!DOCTYPE html>
<html lang="en">

<head>
  <title></title>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1">
</head>

<body>
  <p>a few scripts from files index.0.js etc</p>
  <script src="index.0.js"></script>
  <script src="index.1.js"></script>
  <script src="index.2.js"></script>
  <p>an inline script with alert below</p>
  <script>alert('abc')</script>
</body>

</html>
  1. the Cypress injection code <head> <script type='text/javascript'> document.domain = 'localhost'; var Cypress = window.Cypress ...
  2. inline alert call

The server receives the following calls from the browser

Example app listening on port 3000!
CSP Violation:  { 'csp-report': 
   { 'document-uri': 'http://localhost:3000/',
     referrer: 'http://localhost:3000/__/',
     'violated-directive': 'script-src \'self\'',
     'effective-directive': 'script-src',
     'original-policy': 'script-src \'self\'; report-uri /report-violation',
     'blocked-uri': 'inline',
     'line-number': 4,
     'status-code': 200 } }
CSP Violation:  { 'csp-report': 
   { 'document-uri': 'http://localhost:3000/',
     referrer: 'http://localhost:3000/__/',
     'violated-directive': 'script-src \'self\'',
     'effective-directive': 'script-src',
     'original-policy': 'script-src \'self\'; report-uri /report-violation',
     'blocked-uri': 'inline',
     'line-number': 16,
     'status-code': 200 } }

Which leads me to conclude that we should inject our Cypress top level script from external url and not inline (where it might conflict)

@bahmutov bahmutov added the type: feature New feature that does not currently exist label Dec 7, 2017
@bahmutov
Copy link
Contributor Author

bahmutov commented Dec 7, 2017

  • allow customizing and editing content-security-policy header
  • move injection script into separate file instead of being inline
    or
  • serve injection script inline but with a nonce value to allow specific script. This is tricky because the plugin editing content-security-policy header would have to know the nonce values.

@bahmutov
Copy link
Contributor Author

bahmutov commented Dec 7, 2017

BTW, it would be nice to stub report violation calls, but since they are not XHR, we would need to #687 to do this. Then it would be sweet!

@jamime
Copy link

jamime commented Jul 19, 2019

We were hoping to test that there were no CSP violations. The way this is currently implemented prevents us from doing so.

@tlolkema
Copy link

tlolkema commented Feb 6, 2020

Any update on this one?

@mdantonio
Copy link

It would be a very useful improvement

@flotwig
Copy link
Contributor

flotwig commented Sep 25, 2020

We had some additional discussion for the actual implementation of this improvement here: #7936 (review)

Now it just needs to be implemented.

@bahmutov
Copy link
Contributor Author

bahmutov commented Dec 2, 2020

Extra context: https://glebbahmutov.com/blog/testing-csp-almost/

@DJSdev
Copy link
Contributor

DJSdev commented Apr 14, 2021

+1 to this. It would be nice to test for CSP violations

@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review and removed stage: needs investigating Someone from Cypress needs to look at this labels Nov 21, 2022
flotwig pushed a commit that referenced this issue Jan 11, 2023
Co-authored-by: Zach Bloomquist <git@chary.us>
Closes #1030
AtofStryker pushed a commit that referenced this issue Jan 11, 2023
Co-authored-by: Zach Bloomquist <git@chary.us>
Closes #1030
@AtofStryker
Copy link
Contributor

Hi @pgoforth. First I want to say thank you for the work you put in on getting #24760. Unfortunately, we have to revert the changes due to some issues we found that I described in the revert request #25445. We are hoping to revisit this soon and find a way to make CSP as compatible as possible with Cypress. We are still discussing options, but for right now we need to revert the change to prevent us from breaking existing users. I hope you understand and once again thank you for the large effort on the contribution!

@AtofStryker AtofStryker reopened this Jan 12, 2023
@pgoforth
Copy link
Contributor

pgoforth commented Apr 10, 2023

@AtofStryker Thanks for these notes. It appears that the course of action should be:
1.) Figure out why the CSP is overriding Content-Security-Policy=upgrade-insecure-requests (or other CSP headers in this style) instead of augmenting it. This seems like an instance where I can start by writing tests against this specific type of CSP header and work my way out to testing against all valid CSP headers in an exhaustive way
2.) The frame-ancestors issue should be much more straightforward. There will have to be either:

  • A selective stripping of that CSP header...which would allow Cypress to run
  • An augmentation that would remove 'none' and add the src Cypress is being served from...which could prove difficult because it's likely based on environment and may not be available at runtime.

EDIT
@AtofStryker @flotwig
After going through everything i have the following solutions:
1.) The RegExp was indeed incorrect for detecting CSP directives that did not contain values. I have fixed the RegExp and have added a test loop against all valid CSP directives, and arrays of valid values for each (including undefined) which results in ~250 permutation tests. Those are all passing now with the new RegExp.
Screenshot 2023-04-11 at 11 33 57 AM
Screenshot 2023-04-11 at 11 35 36 AM

2.) I have added a excludeCSPDirectives array that contains any individual directives that Cypress wants to strip in order to maintain functionality (frame-ancestors being the only one currently).

The thought occurred to me while doing this that people have probably written a lot of tests assuming that Cypress is stripping CSP headers. Headers like upgrade-insecure-requests is an example where your test environment could be hosting local files over an insecure protocol, but your response headers could be sending the upgrade-insecure-requests directive. This results in a situation where Cypress is now no longer stripping that header, and your tests are failing, but the actual behavior is correct because your local files over http: are not actually available over https.

That brought me to the following idea which could be executed in two ways:
1.) Opt-In for CSP Header stripping (default true to maintain parity with previous Cypress versions)
2.) Allow customer defined/maintained excludeCSPDirectives and/or includeCSPDirectives array (default to includeCSPDirectives=[] which would {again} maintain parity with previous Cypress versions)

EDIT 2
I have opened a PR that uses a new config option stripCspDirectives that defaults to true. This allows the behavior to be opt-in, meaning that by default, Cypress will still strip all CSP headers from the response. This should avoid any accidental consequences from this feature. I am unsure if I implemented the new config option correctly, so any eyes on that part of the PR would be very helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage: needs review The PR code is done & tested, needs review type: feature New feature that does not currently exist
Projects
None yet