Allow nonce to be used on hoistable styles
#32461
Conversation
Andarist
force-pushed
the
fix/style-precendence-with-nonce
branch
from
f4284b9 to
8d3962d
Compare
Andarist
force-pushed
the
fix/style-precendence-with-nonce
branch
from
8d3962d to
ed2efde
Compare
Andarist
force-pushed
the
fix/style-precendence-with-nonce
branch
from
ed2efde to
805f507
Compare
Andarist
force-pushed
the
fix/style-precendence-with-nonce
branch
from
805f507 to
70d83c5
Compare
Andarist
force-pushed
the
fix/style-precendence-with-nonce
branch
from
70d83c5 to
cf5d7dd
Compare
| if (!('nonce' in styleQueue)) { | ||
| // `styleQueue` could have been created by `preinit` where `nonce` is not required | ||
| styleQueue.nonce = nonce && stringToChunk(escapeTextForBrowser(nonce)); | ||
| } |
There was a problem hiding this comment.
perhaps we should just consult the the one from the renderState? I'm not sure if it's a user requirement to pass nonce to renderToPipeableStream though and then maintain the consistency across the tree .
There was a problem hiding this comment.
we've thought about whether we should auto-nonce styles and scripts but have held off b/c you aren't necessarily in control of every tag you emit and so it's a little safer to still require the author pass these nonces around. However it's not really great protection b/c if you have a compromised package you are probably in trouble in many other ways too.
That said the nonce argument is implied to be for scripts and while rare it's technically possible for the nonce for styles to be different than the nonce for scripts so we'd have to expose another new option. Worth considering perhaps but I think making it just work with rendered props is the way to go
| if (!('nonce' in styleQueue)) { | ||
| // `styleQueue` could have been created by `preinit` where `nonce` is not required | ||
| styleQueue.nonce = nonce && stringToChunk(escapeTextForBrowser(nonce)); | ||
| } |
There was a problem hiding this comment.
we've thought about whether we should auto-nonce styles and scripts but have held off b/c you aren't necessarily in control of every tag you emit and so it's a little safer to still require the author pass these nonces around. However it's not really great protection b/c if you have a compromised package you are probably in trouble in many other ways too.
That said the nonce argument is implied to be for scripts and while rare it's technically possible for the nonce for styles to be different than the nonce for scripts so we'd have to expose another new option. Worth considering perhaps but I think making it just work with rendered props is the way to go
| @@ -5534,6 +5561,7 @@ export type StyleQueue = { | |||
| rules: Array<Chunk | PrecomputedChunk>, | |||
| hrefs: Array<Chunk | PrecomputedChunk>, | |||
| sheets: Map<string, StylesheetResource>, | |||
| nonce?: ?Chunk, | |||
There was a problem hiding this comment.
let's make the property required. fewer hidden class lookups when the nonce is added later
| }; | ||
| renderState.styles.set(precedence, styleQueue); | ||
| } else { | ||
| if (!('nonce' in styleQueue)) { |
There was a problem hiding this comment.
The nonce should be known early even if using preinitStyle. we can assume whatever nonce was used at styleQueue creation time is sufficient for all other style rules and don't need this lazy nonce check.
We can separately add a dev only warning which indicates when you are passing incompatible nonces to style tags. You would do this by having a dev only extra property like
const styleQueue = {
...
}
if (__DEV__) {
styleQueue.__nonceString = nonce
}
It wouldn't be part of the type and you'd have to type cast to any when you read it
| styleQueue.nonce = nonce && stringToChunk(escapeTextForBrowser(nonce)); | ||
| } | ||
| if (__DEV__) { | ||
| if (nonce !== styleQueue.nonce) { |
There was a problem hiding this comment.
this won't work b/c Chunks are sometimes objects (typed arrays). But if you do what I suggested above you can use a dev only string version of the nonce to determine if this warning is necessary
| if (nonce !== styleQueue.nonce) { | ||
| console.error( | ||
| 'React encountered a hoistable style tag with "%s" nonce. It doesn\'t match the previously encountered nonce "%s". They have to be the same', | ||
| nonce && stringToChunk(escapeTextForBrowser(nonce)), |
There was a problem hiding this comment.
We don't want to log a chunk here, it could be a typed array.
| ); | ||
| }); | ||
|
|
||
| // @gate __DEV__ |
There was a problem hiding this comment.
the right way to assert warnings is to use assertConsoleErrorDev inside a test that isn't gated specifically on dev environment.
In this case you might want to assert the existed behavior that both rules are included in the final output style tag regardless of the bad nonce and that in dev you get a warning for it. In this case you're not actually asserting that the program output any styles at all so while I suspect it works it'd be good to encode the expected semantic even when the test is about the warning
fixes #32449
This is my first time touching this code. There are multiple systems in place here and I wouldn't be surprised to learn that this has to be handled in some other areas too. I have found some other style-related code areas but I had no time yet to double-check them.
cc @gnoff