-
Notifications
You must be signed in to change notification settings - Fork 261
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
Support Strict CSP with a nonce #482
Changes from 5 commits
f15dc2d
bb98185
fe01906
278e5d5
6f5f14f
368a6e4
bb588e9
195bcf1
0cdda13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,13 +7,11 @@ const isProd = process.env && process.env.NODE_ENV === 'production' | |
const isString = o => Object.prototype.toString.call(o) === '[object String]' | ||
|
||
export default class StyleSheet { | ||
constructor( | ||
{ | ||
name = 'stylesheet', | ||
optimizeForSpeed = isProd, | ||
isBrowser = typeof window !== 'undefined' | ||
} = {} | ||
) { | ||
constructor({ | ||
name = 'stylesheet', | ||
optimizeForSpeed = isProd, | ||
isBrowser = typeof window !== 'undefined' | ||
} = {}) { | ||
invariant(isString(name), '`name` must be a string') | ||
this._name = name | ||
this._deletedRulePlaceholder = `#${name}-deleted-rule____{}` | ||
|
@@ -29,9 +27,14 @@ export default class StyleSheet { | |
this._tags = [] | ||
this._injected = false | ||
this._rulesCount = 0 | ||
|
||
const node = | ||
typeof document === 'object' && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here you can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like changing this might break my test 😟 because there isn't any browser testing implemented in this repo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably in your test you can do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah no you can pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is an isolated unit test so it is perfectly fine. If you are talking about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was right about the unintended side effects. It's possible that 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want me to break the tests, I will, I just don't have time to fix them myself. |
||
document.querySelector('meta[property="csp-nonce"]') | ||
this._nonce = node ? node.getAttribute('content') : null | ||
} | ||
|
||
setOptimizeForSpeed(bool) { | ||
setOptimizeForSpeed(bool, options = { flush: {} }) { | ||
invariant( | ||
typeof bool === 'boolean', | ||
'`setOptimizeForSpeed` accepts a boolean' | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you need to pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case that method gets other options eventually. |
||
this._optimizeForSpeed = bool | ||
this.inject() | ||
} | ||
|
@@ -50,7 +53,7 @@ export default class StyleSheet { | |
return this._optimizeForSpeed | ||
} | ||
|
||
inject() { | ||
inject(options = { flush: {} }) { | ||
invariant(!this._injected, 'sheet already injected') | ||
this._injected = true | ||
if (this._isBrowser && this._optimizeForSpeed) { | ||
|
@@ -62,7 +65,7 @@ export default class StyleSheet { | |
'StyleSheet: optimizeForSpeed mode not supported falling back to standard mode.' | ||
) // eslint-disable-line no-console | ||
} | ||
this.flush() | ||
this.flush(options.flush) | ||
this._injected = true | ||
} | ||
return | ||
|
@@ -225,6 +228,7 @@ export default class StyleSheet { | |
) | ||
} | ||
const tag = document.createElement('style') | ||
if (this._nonce) tag.setAttribute('nonce', this._nonce) | ||
tag.type = 'text/css' | ||
tag.setAttribute(`data-${name}`, '') | ||
if (cssString) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,29 @@ | ||
import React from 'react' | ||
import { flush } from './style' | ||
|
||
export default function flushToReact() { | ||
export default function flushToReact(options = {}) { | ||
return flush().map(args => { | ||
const id = args[0] | ||
const css = args[1] | ||
return React.createElement('style', { | ||
id: `__${id}`, | ||
// Avoid warnings upon render with a key | ||
key: `__${id}`, | ||
nonce: options.nonce ? options.nonce : undefined, | ||
dangerouslySetInnerHTML: { | ||
__html: css | ||
} | ||
}) | ||
}) | ||
} | ||
|
||
export function flushToHTML() { | ||
export function flushToHTML(options = {}) { | ||
return flush().reduce((html, args) => { | ||
const id = args[0] | ||
const css = args[1] | ||
html += `<style id="__${id}">${css}</style>` | ||
html += `<style id="__${id}"${ | ||
options.nonce ? ` nonce="${options.nonce}"` : '' | ||
}>${css}</style>` | ||
return html | ||
}, '') | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,8 +94,8 @@ export default class StyleSheetRegistry { | |
this.remove(props) | ||
} | ||
|
||
flush() { | ||
this._sheet.flush() | ||
flush(options = {}) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case someone is using this method and needs to set the nonce. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is necessary also Can you revert this change? |
||
this._sheet.flush(options) | ||
this._sheet.inject() | ||
this._fromServer = undefined | ||
this._indices = {} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should replace
flush
withflushToReact