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
There was a problem hiding this comment.
So my question wasn't about auto-noncing anything but rather about checking the renderState.nonce's value. This way we could cache the chunk on it directly and reuse it here if the per-style nonce would match it. But I'm not sure if it is a requirement to pass nonce to renderToPipeableStream (and that's how renderState learns about the nonce in the first place).
There was a problem hiding this comment.
you need to pass nonce to renderToPipeableStream if you are going to use CSP to restrict script execution by nonce. This is because React emits scripts to implement streaming rendering. But we don't typically that this option as an invitation to apply nonces to all possible nonceable things that a user might render like style tags and their own scripts. It's still on you to provide the nonce for your own scripts
| 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
| }; | ||
| 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
There was a problem hiding this comment.
The nonce should be known early even if using preinitStyle.
nonce isn't a part of the PreinitStyleOptions:
react/packages/react-dom/src/shared/ReactDOMTypes.js
Lines 67 to 77 in 2980f27
According to the research I've done before opening this, nonce has no effect on external stylesheets. They are controlled by style-src directive. That's the reason I concluded I have to make it work conditionally like this.
There was a problem hiding this comment.
in this context the nonce for style tag would be the nonce you are configuring for style-src CPS directive. It can be but doesn't have to be the same nonce value used for script-src. but from the perspective of preinitStyle this is irrelevant since this just maps the argument to the <style nonce=...> attribute. So you should just add it as an optional option property for PreinitStyleOptions. It's already part of the public typescript type because we merge all the option types for the public API
There was a problem hiding this comment.
basically in a correctly configured program you will always pass a nonce to preinitStyle or never. So we can infer that the option passed in on the first invocation actually applies to all invocations
There was a problem hiding this comment.
Should I then assume that it has to be passed to preinitStyle for inline <style/>s to work properly (as that will require it to be known upfront)?
There was a problem hiding this comment.
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.
Can we do that though? Styles texts are merged together. This is how it currently works:
it('can render styles with nonce', async () => {
CSPnonce = 'R4nd0m';
await act(() => {
const {pipe} = renderToPipeableStream(
<>
<style
href="foo"
precedence="default"
nonce={CSPnonce}>{`.foo { color: hotpink; }`}</style>
<style
href="bar"
precedence="default"
nonce={CSPnonce}>{`.bar { background-color: blue; }`}</style>
</>,
);
pipe(writable);
});
expect(document.querySelector('style').nonce).toBe(CSPnonce);
expect(getVisibleChildren(document)).toEqual(
<html>
<head />
<body>
<div id="container">
<style
data-precedence="default"
data-href="foo bar"
nonce={
CSPnonce
}>{`.foo { color: hotpink; }.bar { background-color: blue; }`}</style>
</div>
</body>
</html>,
);
});But now if the second style element would have a different nonce we really shouldn't merge it with the first style (the one with the "correct" nonce).
| 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
| ); | ||
| }); | ||
|
|
||
| // @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
|
@gnoff friendly 🏓 |
|
I think we'll want to make this option separate for style and script so that you don't have to include the nonce for every style tag if you're not enforcing it on styles. E.g. |
|
Would this also require passing along react/packages/react-dom/src/shared/ReactDOMFloat.js Lines 233 to 246 in 4a36d3e (EDIT: oh, but this doesn't matter in practice since |
I thought this was explicitly a stated non-goal, see: #32461 (comment) |
Yeah the nonce argument is about providing a nonce to React to use for things it needs to emit that the user doesn't render directly. At the moment I don't think there are any styles that React emits so I don't think we actually need to add this option. But we could go with this if that were to change. |
|
@Andarist I think I know what we should do to resolves the consistency / security issues you brought up. we define Conversely if you did not configure the render to convey that there is a nonce then if you pass a nonce to the style it will log an error and include the style but it will not have the nonce value. So to get nonces on style tags you must pass it to the render method as an option. I'm not sure we need to be as prescriptive with external stylesheets since we can just emit each one with the provided nonce and then the browser will or will not load and apply the stylesheet. |
Andarist
force-pushed
the
fix/style-precendence-with-nonce
branch
2 times, most recently
from
adeb8b7 to
1c11d72
Compare
Andarist
force-pushed
the
fix/style-precendence-with-nonce
branch
from
1c11d72 to
6936609
Compare
|
@gnoff I implemented the requested changes. Could you take a look? |
| @@ -5176,6 +5230,7 @@ function flushStyleTagsLateForBoundary( | |||
| } | |||
| let i = 0; | |||
| if (hrefs.length) { | |||
| writeChunk(this, styleQueue.start); | |||
There was a problem hiding this comment.
any reason we don't just use renderState.startInlineStyle? doesn't seem necessary to have this on every style queue when it must by necessity be the same for each queue
There was a problem hiding this comment.
renderState isn't available here. So I'd have to introduce extra inline functions at those 2 call sites to pass the extra argument:
// ...
hoistableState.styles.forEach(flushStyleTagsLateForBoundary, destination);
// ...
renderState.styles.forEach(flushStylesInPreamble, destination);I could use a module-level variable instead though. I'll push this change in a second.
There was a problem hiding this comment.
done in 68d592a but I imagine you might want to have it done slightly differently - if that's the case I'd appreciate concrete tips as to the preferred solution that would match codebase's style and preferences
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 DiffTrain build for [14094f8](14094f8)
fixes facebook#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 DiffTrain build for [14094f8](facebook@14094f8)
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