Add CSP Nonce option to Select component#3260
Conversation
|
These changes appear to have solved part of the issue - most CSS is now applied, and many CSP warnings went away, but some CSS, specifically CSS related to the aria labels, is still being blocked because the nonce doesn't propagate all the way down. I'm investigating the cause. |
src/Select.js
Outdated
| const selectValue = cleanValue(value); | ||
| const menuOptions = this.buildMenuOptions(props, selectValue); | ||
|
|
||
| this.emotion = createEmotion(props.nonce ? { nonce: props.nonce } : {}); |
There was a problem hiding this comment.
I would recommend adding also key: 'react-select' and caching created instances per nonce to share them between instances
|
My colleague @dicearr investigated and fixed the last remaining CSP issues. He described his findings here: wearereasonablepeople#1 (comment). He also implemented momoization of the emotion instances, as requested by |
src/Select.js
Outdated
|
|
||
| this.emotion = createEmotion(props.nonce ? { nonce: props.nonce } : {}); | ||
| this.createEmotion = memoize(createEmotion); | ||
| this.emotion = this.createEmotion(props.nonce ? { nonce: props.nonce } : {}); |
There was a problem hiding this comment.
@emotion/memoize has a simple POJO cache under the hood, it uses first argument as cache key, so u cant quite memoize based on objects - https://github.com/emotion-js/emotion/blob/fc156cf7214fda37fac3ce654569a4e17965d7e4/packages/memoize/src/index.js
Also - you have to create memoized version in the top level scope to keep cached emotion's "global", instead of caching them per <Select/> instance
correct way of reusing this package would look smth like this
const getEmotion = memoize(nonce => createEmotion(nonce ? { nonce } : {}))
// and in constructor
this.emotion = getEmotion(props.nonce)
package.json
Outdated
| "@emotion/memoize": "^0.7.1", | ||
| "classnames": "^2.2.5", | ||
| "create-emotion": "^10.0.4", | ||
| "emotion": "^9.1.2", |
There was a problem hiding this comment.
We might not have a direct dependency on emotion anymore.
|
I re-checked all the checks in the PR description. |
|
I squashed the fixup commits. I also had a look at the documentation. It seems the |
|
I rebased on master and all tests now pass. |
|
I checked the "Add tests" box in the PR description to indicate that I think I'm done, although specific tests for the nonce property were not added. However, the snapshots were updated to include the |
gwyneplaine
left a comment
gwyneplaine
left a comment
There was a problem hiding this comment.
this is great, thanks @Avaq lgtm
|
Hey, |
|
The work done here caused problems down the line, so it was reverted. See #3346 |
|
See #4283 (comment) for a way to include a Nonce in v4. |
6 participants
Motivation
To close #2917
Changes
<Select + nonce={MY_SECRET_NONCE} >emotiondependency was replaced bycreate-emotion.emotionis created in the Select constructor, passing in thenonceif the user provided it.emotioninstance is passed to all components viacommonProps.emotioninstance instead of the global one.Checklist
ensure all the examples work correctly after your change
ensure you do not introduce any new [linting] errors or warnings
ensure that you do not introduce any new type errors
I was unable to run
yarn flow:make sure there's an issue open for any work you take on
make sure you do not add regressions
Two tests were failing before, and after the changes I made (see fix: update broken tests #3253):
include tests with your changes
I'm not sure how to test this change
check that the coverage hasn't dropped
Coverage increased marginally:
Before
After
update the documentation to reflect potential new features
I would like to ask for pointers as to which sections of the documentation this might have affected.
I will test this new feature in my own project, and I'll let you know how that goes.