vercel / next.js

The React Framework
https://nextjs.org
MIT License
126.78k stars 26.95k forks source link

Content Security Policy (CSP) with next.js #256

Closed dbo closed 6 years ago

dbo commented 7 years ago

Edit: This issue is very old and has now been replaced with https://github.com/vercel/next.js/issues/23993.


I don't see a way using next.js with a sensible CSP employed, foremost because it's eval'ing the component code (unsafe-eval is highly discouraged w.r.t. XSS). In addition, the injected scripts page component scripts should get a computed hash, and an SRI hash should be added for the CDN/next.js script.

Excuse my ignorance about, but wouldn't it be better to use plain old script tag loading for the component module, props etc.?

nkzawa commented 7 years ago

I'm not familiar with CSP, but it seems next.js doesn't support CSP at all for now. eval is used just because it's a more flexible way to execute codes compared to others.

arunoda commented 7 years ago

CSP is kind a standard for web security and usually we disable eval with CSP. (and similar functionalities) As a fix for this, we might need to load the page bundle as a JavaScript. Usually, loading scripts from the same domain is allowed with CSP. So, that won't be an issue.


That's being said, stored XSS is not possible with React unless you use dangerouslySetInnerHTML.

So, we are not in a huge security risk.

arunoda commented 7 years ago

@dbo about this:

In addition, the injected scripts page component scripts should get a computed hash, and an SRI hash should be added for the CDN/next.js script.

I hope this is about another issue. Shall we open a new issue for that?

dbo commented 7 years ago

@arunoda IMHO this is a blocker to seriously deploy next.js apps; CSP is a standard, eval discouraged. Avoiding dangerouslySetInnerHTML in your code is common practice, but what about 3rd party NPMs, e.g. older template engines. You'd have to audit all of your dependencies.

Regarding your latter question: I think this task should be about getting next.js running with strict CSP, i.e. preferably the hashes for the inline script as well as a SRI hash for the CDN-script could be generated (at build time?), so no need to allow any inline-script, CDN script-src.

rauchg commented 7 years ago

@dbo just so I understand the attack surface better: why would you use a template engine in this context?

dbo commented 7 years ago

As I've written, my primary code can (and should) avoid dangerouslySetInnerHTML and also doesn't need a templating engine, since it's react. But any 3rd party NPM (and transitive dependencies) that I use might do so, especially any 3rd party react component might use dangerouslySetInnerHTML.

Also look at this from a different angle: Any serious customer would require you to do a security audit/proof of the site you've built. Given a decent browser baseline, it's much easier and less effort to do that in context of a CSP.

rauchg commented 7 years ago

Definitely. I'll keep this open. Consider this approach: https://github.com/zeit/next.js/issues/253#issuecomment-261111954

The problem is that it introduces a new roundtrip, however.

dbo commented 7 years ago

@rauchg reads sensible @nkzawa To be honest, I consider this a major blocker for any production site. So this doesn't sound like an enhancement to me.

rauchg commented 7 years ago

What you're saying is that you want a protection from yourself (unaudited code that you might include in your own codebase). That by definition is a security enhancement, not a hole.

I think this is important to address, and we're thinking about how to best do it without a tradeoff in performance.

dbo commented 7 years ago

@rauchg Of course this is not about a security hole, but a very important security enhancement, it's highly recommended and standardised practice for good reason. Regarding your point of "protection from yourself", please keep in mind that a lot of developers simply don't have

to thoroughly audit all of their transitive dependencies. I simply don't expect it to happen.

rauchg commented 7 years ago

Absolutely. Was just pointing out that @nkzawa picked the right label, and that we consider it very important still.

On Tue, Nov 22, 2016 at 9:53 AM ǝlzlǝoq lǝᴉuɐp ツ notifications@github.com wrote:

@rauchg https://github.com/rauchg Of course this is not about a security hole, but a very important security enhancement, it's highly recommended and standardised practice for good reason. Regarding your point of "protection from yourself", please keep in mind that a lot of developers simply don't have

  • the time
  • the experience to thoroughly audit all of their transitive dependencies. I simply don't expect it to happen.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zeit/next.js/issues/256#issuecomment-262297176, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAy8Q8YhTOBrwAaIk-XR9x9iSbGkZHQks5rAx4hgaJpZM4Kx9aI .

aesopwolf commented 7 years ago

CSP headers are definitely a high priority in my opinion. However, it's possible to use a meta tag as an alternative for the time being.

https://developers.google.com/web/fundamentals/security/csp/#the_meta_tag

dbo commented 7 years ago

@aesopwolf Using a meta tag as an alternative to HTTP headers is not the issue here, it will prevent the page from working, too, unless you allow unsafe-eval.

aesopwolf commented 7 years ago

@dbo my apologies, I skimmed over the thread too fast.

@rauchg Is there anything we can do to help fast-lane the SRI hash for the CDN script? My company is in the final phases of a new registration app built on next.js and we expect to see 150k signups in January so having the CDN would definitely be nice to have.

rauchg commented 7 years ago

@aesopwolf as of 2.0+ there won't be a "global build" anymore, so it's not possible for us to provide a unique CDN. Each site generates its own minimal build of the framework

aesopwolf commented 7 years ago

@rauchg thanks for the update, that sounds like the best thing to do anyways. It moves work off zeit's end and gives developers full control of their own caching strategy 👍

rauchg commented 7 years ago

Yeah. The goal was to provide an optimized baseline so that Next could be cached across several internet properties. But in the end, there are many slices and versions of Next across those properties, so it's a difficult endeavor.

dav-is commented 7 years ago

Update on this?

pozylon commented 7 years ago

Workaround 😂:

<Head>
      <meta httpEquiv="Content-Security-Policy" content="default-src * 'self' data: 'unsafe-inline' 'unsafe-eval' *" />    
</Head>
dav-is commented 7 years ago

That completely defeats the purpose of CSP.

jeffhandley commented 7 years ago

Edit: I dug up our internal documentation on this topic and I have added some clarification, references, and corrections to my original comment

The approach I've taken on projects where we want server-side rendering of React components and then client-side re-rendering (progressive rendering), where data needs to be serialized and then deserialized as part of the payload is to:

Emit the data payload into a <noscript> tag as a JSON-stringified payload. Then, the library code that relies on the data finds the element by id and parses its content into the necessary variables. In the case of next.js, it would likely be something like this:

<noscript id="__NEXT_DATA__">{props:{...}, ...}</noscript>

We don't just blindly use document.getElementById though. Instead we use querySelectorAll and ensure that there's only a single element on the page with that id. That way, even if there is a dangerouslySetInnerHtml vulnerability elsewhere on the page, this specific feature of loading data from the server would not be vulnerable. We use the textContent property of the element once we've found it.

Using this approach, we serialize our store and other data from the server into these <noscript> tags, load them from our library code, and we get to where we have zero inline <script> tags. Every <script> has a source coming from the same domain and we can meet the CSP requirements.

You can read the OWASP coverage of this topic here: https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet#HTML_entity_encoding

Our approach is aligned with what they document there, but we use that extra check that there's a single element matching the ID we're looking for, and we put the data into a <noscript> element instead of a <div>.

jeffhandley commented 7 years ago

Here's what I spiked out to see if this approach would work. A modified NextScript component's render function:

  render () {
    const { staticMarkup, __NEXT_DATA__, chunks } = this.context._documentProps
    const { pathname, nextExport, buildId, assetPrefix } = __NEXT_DATA__
    const pagePathname = getPagePathname(pathname, nextExport)

    __NEXT_DATA__.chunks = chunks

    return <div>
      {staticMarkup ? null : (
        <div>
          <noscript id='__NEXT_DATA__' dangerouslySetInnerHTML={{ __html: htmlescape(JSON.stringify(__NEXT_DATA__)) }} />
          <script type='text/javascript' src={`${assetPrefix}/static/next-script.js?${buildId}`} />
        </div>
      )}
      <script async id={`__NEXT_PAGE__${pathname}`} type='text/javascript' src={`${assetPrefix}/_next/${buildId}/page${pagePathname}`} />
      <script async id={`__NEXT_PAGE__/_error`} type='text/javascript' src={`${assetPrefix}/_next/${buildId}/page/_error/index.js`} />
      {staticMarkup ? null : this.getDynamicChunks()}
      {staticMarkup ? null : this.getScripts()}
    </div>
  }

And then I had a next-script.js file with the following (that would ideally move into the next main module before __NEXT_DATA is expected to be there):

var nextData = document.querySelectorAll('noscript#__NEXT_DATA__');

if (nextData.length === 1) {
    window.__NEXT_DATA__ = JSON.parse(nextData[0].textContent);
    window.module = {};
    window.__NEXT_LOADED_PAGES__ = [];
    window.__NEXT_LOADED_CHUNKS__ = [];
}

function __NEXT_REGISTER_PAGE(route, fn) {
    __NEXT_LOADED_PAGES__.push({ route: route, fn: fn })
}

function __NEXT_REGISTER_CHUNK(chunkName, fn) {
    __NEXT_LOADED_CHUNKS__.push({ chunkName: chunkName, fn: fn })
}

With that in place, I added the following express middleware to my custom server:

function contentSecurityPolicy(req, res, next) {
    res.setHeader('Content-Security-Policy', 'script-src \'self\'');
    next();
}

Running the app, I get an error:

ndefined is not iterable!
TypeError: undefined is not iterable!
    at e.exports.n.getIterator (http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:7:30409)
    at http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:8:3491
    at r (http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:7:22793)
    at Generator._invoke (http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:7:23838)
    at Generator.e.(anonymous function) [as next] (http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:7:22972)
    at r (http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:2:3765)
    at http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:2:3913
    at new Promise (<anonymous>)
    at new t (http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:2:1955)
    at http://localhost:3000/_next/6c43080790430acdd9608370496ef26e/app.js:2:3707
(anonymous) @ app.js:8
Promise rejected (async)
132 @ app.js:8
n @ app.js:1
131 @ app.js:8
n @ app.js:1
window.webpackJsonp @ app.js:1
(anonymous) @ app.js:8

This is preventing modules from getting loaded, but I've not been able to track down what is causing this.

Potherca commented 7 years ago

Just to be clear... Is this the code that causes all the trouble?

https://github.com/zeit/next.js/blob/9320d9f006164f2e997982ce76e80122049ccb3c/server/build/babel/plugins/handle-import.js#L14

'Cause it does not look like that needs to be evaled at all.[1]


1 I get the distinct impression that I might be missing something here... :confused:

jeffhandley commented 7 years ago

It does look like the eval is an issue for CSP too, but the initial reports for this issue relate to using inline script code.

https://www.owasp.org/index.php/Content_Security_Policy_Cheat_Sheet#Refactoring_inline_code

Instead of having <script> tags with executable code inside, the executable code needs to be moved into <script src> tags--no dynamic executable JavaScript can be used on the page. For universal rendering, that means that initial props/state data needs to be sent to the browser differently--like I illustrated with the <noscript> approach.

After fixing that, the eval issue might also need to be fixed though.

You can use Report-Only headers or CSP tester tools to test CSP compatibility.

jwb commented 6 years ago

I heard from @rauchg that this issue should be resolved in the current release. Is that accurate?

ldthorne commented 6 years ago

@jwb that'd be dandy! Which release is that?

dav-is commented 6 years ago

I got CSP working by generating a script nonce in an express middleware and assigning <NextScript nonce={scriptNonce} /> in _document.js

Obviously we don't want to use a nonce, but it's valid and secure CSP.

arunoda commented 6 years ago

As of today, we can use CSP without any issue. Just expose headers and we are good to go. with the custom server, we can use a NPM module like helmet.

dav-is commented 6 years ago

@arunoda Could you reference the changes?

arunoda commented 6 years ago

@dav-is Check canary branch. We don't use eval anymore.

And just to be clear, there are a ton of stuff we can do with CSP even with enabling unsafe-eval.

One idea is request whitelisting. So, even though someone eval an script, attacker can't send anything back to outside of defined domains.

And it's better to prevent XSS by writing the app carefully rather than rely on CSP. With React it's pretty hard to XSS unless you use dangerouslysetinnerhtml

dbo commented 6 years ago

@arunoda Any plans to support CSP without unsafe-inline, e.g. adding a hash?

dav-is commented 6 years ago

And it's better to prevent XSS by writing the app carefully rather than rely on CSP.

This is a ridiculous reason for closing this issue. You could clearly argue the inverse: it’s better to implement safe CSP rules than rely on React’s anti XSS tactics. CSP is a tool, not a safegaurd, obviously. Just because there’s other ways of protecting against XSS, doesn’t mean CSP is useless.

Really, Nextjs doesn’t need to use inline scripts. The only reason inline is needed is to transfer server state (right?) and we can transfer server state with JSON. I’ll see if I can manage a PR. Here’s how I did it on Afterjs: https://github.com/jaredpalmer/after.js/commit/68b874acf49b02fe53746c5e769dab4bb1fe3d2f

Edit: It looks like there's some functions defined in the inline script instead of just purely data... https://github.com/zeit/next.js/blob/9a10461150f2cfdee83c0d427803f73c70541aba/server/document.js#L189 This should probably be moved to some sort of helper function.

ghost commented 6 years ago

@dbo have you managed something out to get rid of the unsafe-inline in the script-src directive?

dbo commented 6 years ago

@kytwb No, I haven't.

dbo commented 6 years ago

@kytwb I factored out the inline script of NextScript to be able to generate the hash in my custom _document. This enables me to finally set a strict script-src policy. See #4935 .

ghost commented 6 years ago

@dbo thanks for following up, now following the PR!

dav-is commented 6 years ago

I created a WIP PR: https://github.com/zeit/next.js/pull/4943

We can get this moving now and get a permanent solution for everyone, not a workaround.

dav-is commented 6 years ago

If my PR gets merged soon, CSP will be as simple as an extra line in the config file. :)

davidcunha commented 6 years ago

I tested with next.js 7.0 and without adding 'unsafe-eval' to my CSP config I get:

Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive:

What's the plan to not require any workaround or adding 'unsafe-eval' to the config which is definitely against CSP?

dav-is commented 6 years ago

@davidcunha Hot Module Replacement (HMR) requires eval. This is only run in dev mode and unsafe-eval will be automatically added in dev mode.

In production, this isn’t an issue. Also, my PR didn’t make it into 7.0, if you thought it did.

davidcunha commented 6 years ago

Yes indeed, it's not an issue in production. I'm getting only this error while running in dev mode.

Jero786 commented 5 years ago

There is some update from this PR? there would be awesome to be in the next release version.

matoous commented 5 years ago

Hello! Any news?

timneutkens commented 5 years ago

@matoous any news on what?

https://nextjs.org/blog/next-8#removed-inline-javascript

javidjamae commented 4 years ago

Looks like you can include Content Security Policy headers in NextJS 9.5

https://nextjs.org/blog/next-9-5#headers https://nextjs.org/docs/api-reference/next.config.js/headers

OZZlE commented 4 years ago

This worked for me in production mode nextjs, next.config.js:

const withImages = require('next-images')
module.exports = withImages({
  async headers() {
    return [
      {
        source: '/:path*',
        headers: [
          {
            // // https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP
            key: 'Content-Security-Policy',
            value:
              "default-src [your-domains...] 'self' data: 'unsafe-inline'"
          }
        ]
      }
    ]
  }
})

If you don't use next-images then you can just remove that, export the object only.

We are still on next@9.4.4

ivan-kleshnin commented 4 years ago

@OZZlE this adds Content-Security-Policy to responses of static assets, non only HTML. Which hurts performance (slightly).

I believe that:

// pages/_document.js
...
<Head>
  <meta httpEquiv="Content-Security-Policy" content={csp} />
</Head>

is a better approach. You don't have to mess with regexp trying to detect statics. Also it's more obvious in _document than in next.config.js (which is often long and overcomplicated enough) IMO.

OZZlE commented 4 years ago

@ivan-kleshnin probably you are correct but I was just handed a pre-existing project and they have multiple <Head> components for some reason.. I guess the CSP could be fetched from the same place still though :) Their next.config.js was almost empty though in this case :)

bozdoganCihangir commented 3 years ago

All works fine after removing the "helmet" npm package.

trezy commented 3 years ago

For anybody else finding this issue while trying to figure out CSP stuff, I ended up building my own library to handle it. I'd encourage you all to check it out, too.

https://npmjs.com/package/next-safe