Support Strict CSP with a nonce #482

dav-is · 2018-08-28T16:09:15Z

Content Security Policy

Strict CSP is supported. You must pass a nonce as a parameter to either flush(nonce) or flushToHTML(nonce) and set a <meta property="csp-nonce" content="nonce"> tag.

You should generate a nonce per request.

import nanoid from 'nanoid'

const nonce = Buffer.from(nanoid()).toString('base64') //ex: N2M0MDhkN2EtMmRkYi00MTExLWFhM2YtNDhkNTc4NGJhMjA3

Your CSP policy must have this same nonce as well.
Content-Security-Policy: default-src 'self'; style-src 'self' 'nonce-N2M0MDhkN2EtMmRkYi00MTExLWFhM2YtNDhkNTc4NGJhMjA3';

giuseppeg

Awesome! thank you for contributing!

giuseppeg · 2018-08-29T06:37:57Z

src/server.js

      dangerouslySetInnerHTML: {
        __html: css
      }
    })
  })
 }

-export function flushToHTML() {
+export function flushToHTML(nonce = undefined) {


= undefined is redundant, you can remove it.

can we make the arguments an object? This will allow us to add more options in the future.

function flushToHTML(options = {}) { // options.nonce }

giuseppeg · 2018-08-29T06:42:07Z

src/lib/stylesheet.js

@@ -29,6 +27,11 @@ export default class StyleSheet {
    this._tags = []
    this._injected = false
    this._rulesCount = 0
+
+    const node =
+      typeof document === 'object' &&


Here you can use this._isBrowser

I feel like changing this might break my test 😟 because there isn't any browser testing implemented in this repo ☹️

probably in your test you can do global.window = true.

ah no you can pass isBrowser: true to makeRegistry https://github.com/zeit/styled-jsx/blob/master/test/stylesheet-registry.js#L8-L12

@giuseppeg Will this not cause unintended side effects because we are telling the application it is running in a browser and it's not really? If somewhere else in the application it checks isBrowser and if I set it to true, it might try to execute some other type of browser only script.

this is an isolated unit test so it is perfectly fine. If you are talking about src/lib/stylesheet.js instead that's fine too since you are checking that typeof document === 'object' is defined i.e. you are in a browser environment.

I was right about the unintended side effects. It's possible that isBrowser == true and typeof document === 'undefined', which will create an error.

Making the change you requested, allows other tests to break code when it sets isBrowser to true, even when it's not technically in a browser. I feel like this is an issue of it's own.

it should behave the same when isBrowser is true. If there are failing tests this is a good thing and we should look into this issue. In a real browser typeof document === 'object' will be the same as using this._isBrowser because the latter is just typeof window !== 'undefined;

If you want me to break the tests, I will, I just don't have time to fix them myself.

readme.md

giuseppeg · 2018-08-29T06:43:24Z

readme.md

+Strict CSP is supported. You must pass a nonce as a parameter to either `flush(nonce)` or `flushToHTML(nonce)` **and** set a `<meta property="csp-nonce" content="nonce">` tag.
+
+You should generate a nonce per request.
+```jsx


this can be js

giuseppeg · 2018-08-30T06:57:05Z

src/lib/stylesheet.js

@@ -29,6 +27,11 @@ export default class StyleSheet {
    this._tags = []
    this._injected = false
    this._rulesCount = 0
+
+    const node =
+      typeof document === 'object' &&


ah no you can pass isBrowser: true to makeRegistry https://github.com/zeit/styled-jsx/blob/master/test/stylesheet-registry.js#L8-L12

test/stylesheet-registry.js

giuseppeg · 2018-08-30T17:45:47Z

src/lib/stylesheet.js

@@ -41,7 +44,7 @@ export default class StyleSheet {
      this._rulesCount === 0,
      'optimizeForSpeed cannot be when rules have already been inserted'
    )
-    this.flush()
+    this.flush(options.flush)


why do you need to pass options.flush?

In case that method gets other options eventually.

giuseppeg · 2018-08-30T17:46:32Z

src/stylesheet-registry.js

@@ -94,8 +94,8 @@ export default class StyleSheetRegistry {
    this.remove(props)
  }

-  flush() {
-    this._sheet.flush()
+  flush(options = {}) {


what is this for?

In case someone is using this method and needs to set the nonce.

I don't think it is necessary also StyleSheet#flush is not using those options https://github.com/zeit/styled-jsx/blob/368a6e4e3e1211e84b6bc4bb4db44dcd426a0bd9/src/lib/stylesheet.js#L192

Can you revert this change?

giuseppeg · 2018-08-30T18:36:08Z

Thanks for your patience @dav-is! To fix the tests you can create a little helper function to mock document and document.querySelector like you did in your test. Something like

function withMock(mockFn, testFn) {
  const cleanUp = mockFn()
  if (typeof cleanUp !== 'function') {
    throw new Error('mockFn should return a cleanup function')
  }
  testFn()
  cleanUp()
}

function withMockDocument() {
  const originalDocument = global.document
  // We need to stub a document in order to simulate the meta tag
  global.document = {
    querySelector(query) {
      t.is(query, 'meta[property="csp-nonce"]')
      return {
        getAttribute(attr) {
          t.is(attr, 'content')
          return 'test-nonce'
        }
      }
    }
  }

  return () => {
    global.document = originalDocument
  }
}


withMock(
  withMockDocument,
  () => {
    const registry = makeRegistry()
    registry.add({ styleId: '123', css: [cssRule] })
    t.is(registry._sheet._nonce, 'test-nonce')
  }
)

dav-is · 2018-08-31T19:18:22Z

@giuseppeg All done :)

giuseppeg · 2018-08-31T19:50:15Z

@thealjey do you mind taking a quick look at this PR and let us know if you had something like this in mind when you opened #423 ?

timneutkens · 2018-09-04T17:49:35Z

@thealjey sorry for the second ping, could you have a look? 🙏 🙌

thealjey · 2018-09-05T06:26:29Z

oops, sure! will do in a couple hours :)

thealjey · 2018-09-05T06:59:40Z

The API is exactly as I thought it should be (although, this will require the changes to be also made to Next.js to actually pass nonce to flush and, preferably, create the meta tag also).
I do have a question regarding the actual changes.
The readme states that you can pass nonce to the flush function, but I do not see flush actually accept it (flushToHTML and flushToReact both do, but not flush). It only reads the value from the meta tag on the client.
Maybe I just don't see something.

giuseppeg · 2018-09-05T07:15:30Z

readme.md

+const nonce = Buffer.from(nanoid()).toString('base64') //ex: N2M0MDhkN2EtMmRkYi00MTExLWFhM2YtNDhkNTc4NGJhMjA3
+```
+
+You must then pass a nonce to either `flush({ nonce })` or `flushToHTML({ nonce })` **and** set a `<meta property="csp-nonce" content={nonce} />` tag.


we should replace flush with flushToReact

giuseppeg · 2018-09-05T07:16:15Z

@thealjey thank you <3

The readme states that you can pass nonce to the flush function, but I do not see flush actually accept it

yep we should fix that

giuseppeg · 2018-09-05T07:19:41Z

@dav-is thank you a lot for your contribution!!

dav-is · 2018-09-07T18:48:51Z

@timneutkens @giuseppeg Do you think we could get a release?

dav-is · 2018-09-24T21:09:39Z

@giuseppeg Could you please make a new release?

giuseppeg · 2018-09-26T06:06:22Z

@dav-is published 3.1.0 sorry for the delay

xereda · 2024-03-27T20:17:06Z

The updated documentation states that we should apply registry.styles({ nonce }). Where and how can I do this in a Next.js project (14.+)?

dav-is · 2024-03-27T23:04:25Z

@xereda you can follow this guide. I believe adding the CSP header to the request headers allows Next.js to automatically use the nonce. Then the client uses the CSP additionally sent in the response headers.

I would also verify the <meta property="csp-nonce" content={nonce} /> is returned in the html, if not, you may have to do it manually using the x-nonce request header mentioned in the guide.

xereda · 2024-03-28T12:33:12Z

@dav-is I did all of this. I was even able to apply it to scripts, but it's not working for styles. I have no idea where to implement "registry.styles({ nonce })".

dav-is · 2024-03-28T12:38:52Z

@xereda you may need to file an issue on the Next.js repo then, I would imagine since Next.js handles styled-jsx for you, it should add the nonce for you too

Support setting a nonce

f15dc2d

dav-is requested a review from giuseppeg as a code owner August 28, 2018 16:09

dav-is mentioned this pull request Aug 28, 2018

[WIP] Strong CSP Support vercel/next.js#4943

Closed

14 tasks

dav-is added 2 commits August 28, 2018 17:23

Parameter is not required

bb98185

Update README.md

fe01906

giuseppeg reviewed Aug 29, 2018

View reviewed changes

Requested Fixes

278e5d5

giuseppeg reviewed Aug 30, 2018

View reviewed changes

Minor fixes

6f5f14f

giuseppeg reviewed Aug 30, 2018

View reviewed changes

use isBrowser :(

368a6e4

dav-is added 2 commits August 31, 2018 14:12

Fix tests, remove useless parameters

bb588e9

Merge branch 'master' into csp-nonce

195bcf1

giuseppeg reviewed Sep 5, 2018

View reviewed changes

Fix docs

0cdda13

giuseppeg merged commit 03af155 into vercel:master Sep 5, 2018

dav-is deleted the csp-nonce branch September 7, 2018 18:49

giuseppeg mentioned this pull request Sep 28, 2018

[feature] Content Security Policy (CSP) #423

Closed

Support Strict CSP with a nonce #482

Support Strict CSP with a nonce #482

Conversation

dav-is commented Aug 28, 2018 • edited Loading

Content Security Policy

giuseppeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dav-is Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppeg commented Aug 30, 2018

dav-is commented Aug 31, 2018

giuseppeg commented Aug 31, 2018

timneutkens commented Sep 4, 2018

thealjey commented Sep 5, 2018

thealjey commented Sep 5, 2018

Choose a reason for hiding this comment

giuseppeg commented Sep 5, 2018 • edited Loading

giuseppeg commented Sep 5, 2018

dav-is commented Sep 7, 2018

dav-is commented Sep 24, 2018

giuseppeg commented Sep 26, 2018

xereda commented Mar 27, 2024

dav-is commented Mar 27, 2024

xereda commented Mar 28, 2024

dav-is commented Mar 28, 2024

dav-is commented Aug 28, 2018 •

edited

Loading

dav-is Aug 30, 2018 •

edited

Loading

giuseppeg commented Sep 5, 2018 •

edited

Loading