Skip to content

Allow nonce to be used on hoistable styles #32461

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.

Sign up for GitHub

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

Merged
gnoff merged 20 commits into from
May 29, 2025
+398 −30
Merged

Allow nonce to be used on hoistable styles #32461

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
cf5d7dd
Allow `nonce` to be used on hoistable styles
Andarist Feb 23, 2025
9c044b7
Merge remote-tracking branch 'origin/main' into fix/style-precendence…
Andarist Apr 5, 2025
947e2f1
remove `@gate`
Andarist Apr 6, 2025
57b4c9e
tweak logic
Andarist Apr 25, 2025
cbf18c5
tweak logic
Andarist Apr 25, 2025
574374a
Merge remote-tracking branch 'origin/main' into fix/style-precendence…
Andarist Apr 25, 2025
7906af5
fmt
Andarist Apr 25, 2025
3c152a9
fix type
Andarist Apr 25, 2025
9967076
fix types
Andarist Apr 26, 2025
a3259a0
Merge remote-tracking branch 'origin/main' into fix/style-precendence…
Andarist May 25, 2025
6936609
Refactor
Andarist May 26, 2025
a89a50d
fixed types
Andarist May 27, 2025
63e0282
propagate nonce type through resume options etc
Andarist May 27, 2025
44d19b1
fmt
Andarist May 27, 2025
0c0eaf8
render styles without nonce when render call didn't receive nonce
Andarist May 27, 2025
3a6a817
add tests
Andarist May 27, 2025
7ede4ed
loosen up expect
Andarist May 27, 2025
aaebd2f
tweak console.error messaging
Andarist May 29, 2025
68d592a
get rid of `styleQueue.render`
Andarist May 29, 2025
acc9f05
fmt
Andarist May 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 36 additions & 8 deletions packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2712,6 +2712,7 @@ function pushStyle(
}
const precedence = props.precedence;
const href = props.href;
const nonce = props.nonce;

if (
insertionMode === SVG_MODE ||
Expand Down Expand Up @@ -2759,9 +2760,23 @@ function pushStyle(
rules: ([]: Array<Chunk | PrecomputedChunk>),
hrefs: [stringToChunk(escapeTextForBrowser(href))],
sheets: (new Map(): Map<string, StylesheetResource>),
nonce: nonce && stringToChunk(escapeTextForBrowser(nonce)),
};
renderState.styles.set(precedence, styleQueue);
} else {
if (!('nonce' in styleQueue)) {
Copy link
Collaborator

@gnoff gnoff Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Andarist Andarist Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

export type PreinitStyleOptions = {
crossOrigin?: ?CrossOriginEnum,
integrity?: ?string,
fetchPriority?: ?string,
};
export type PreinitScriptOptions = {
crossOrigin?: ?CrossOriginEnum,
integrity?: ?string,
fetchPriority?: ?string,
nonce?: ?string,
};

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.

Copy link
Contributor Author

@Andarist Andarist Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnoff friendly 🏓

Copy link
Collaborator

@gnoff gnoff Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@gnoff gnoff Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Andarist Andarist Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Collaborator

@gnoff gnoff Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

Copy link
Contributor Author

@Andarist Andarist Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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` could have been created by `preinit` where `nonce` is not required
styleQueue.nonce = nonce && stringToChunk(escapeTextForBrowser(nonce));
}
Copy link
Contributor Author

@Andarist Andarist Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Collaborator

@gnoff gnoff Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Andarist Andarist Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

@gnoff gnoff Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (__DEV__) {
if (nonce !== styleQueue.nonce) {
Copy link
Collaborator

@gnoff gnoff Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)),
styleQueue.nonce,
);
}
}
// We have seen this precedence before and need to track this href
styleQueue.hrefs.push(stringToChunk(escapeTextForBrowser(href)));
}
Expand Down Expand Up @@ -4684,8 +4699,9 @@ function escapeJSObjectForInstructionScripts(input: Object): string {
const lateStyleTagResourceOpen1 = stringToPrecomputedChunk(
'<style media="not all" data-precedence="',
);
const lateStyleTagResourceOpen2 = stringToPrecomputedChunk('" data-href="');
const lateStyleTagResourceOpen3 = stringToPrecomputedChunk('">');
const lateStyleTagResourceOpen2 = stringToPrecomputedChunk('" nonce="');
const lateStyleTagResourceOpen3 = stringToPrecomputedChunk('" data-href="');
const lateStyleTagResourceOpen4 = stringToPrecomputedChunk('">');
const lateStyleTagTemplateClose = stringToPrecomputedChunk('</style>');

// Tracks whether the boundary currently flushing is flushign style tags or has any
Expand All @@ -4701,6 +4717,7 @@ function flushStyleTagsLateForBoundary(
) {
const rules = styleQueue.rules;
const hrefs = styleQueue.hrefs;
const nonce = styleQueue.nonce;
if (__DEV__) {
if (rules.length > 0 && hrefs.length === 0) {
console.error(
Expand All @@ -4712,13 +4729,17 @@ function flushStyleTagsLateForBoundary(
if (hrefs.length) {
writeChunk(this, lateStyleTagResourceOpen1);
writeChunk(this, styleQueue.precedence);
writeChunk(this, lateStyleTagResourceOpen2);
if (nonce) {
writeChunk(this, lateStyleTagResourceOpen2);
writeChunk(this, nonce);
}
writeChunk(this, lateStyleTagResourceOpen3);
for (; i < hrefs.length - 1; i++) {
writeChunk(this, hrefs[i]);
writeChunk(this, spaceSeparator);
}
writeChunk(this, hrefs[i]);
writeChunk(this, lateStyleTagResourceOpen3);
writeChunk(this, lateStyleTagResourceOpen4);
for (i = 0; i < rules.length; i++) {
writeChunk(this, rules[i]);
}
Expand Down Expand Up @@ -4805,9 +4826,10 @@ function flushStyleInPreamble(
const styleTagResourceOpen1 = stringToPrecomputedChunk(
'<style data-precedence="',
);
const styleTagResourceOpen2 = stringToPrecomputedChunk('" data-href="');
const styleTagResourceOpen2 = stringToPrecomputedChunk('" nonce="');
const styleTagResourceOpen3 = stringToPrecomputedChunk('" data-href="');
const spaceSeparator = stringToPrecomputedChunk(' ');
const styleTagResourceOpen3 = stringToPrecomputedChunk('">');
const styleTagResourceOpen4 = stringToPrecomputedChunk('">');

const styleTagResourceClose = stringToPrecomputedChunk('</style>');

Expand All @@ -4822,22 +4844,27 @@ function flushStylesInPreamble(

const rules = styleQueue.rules;
const hrefs = styleQueue.hrefs;
const nonce = styleQueue.nonce;
// If we don't emit any stylesheets at this precedence we still need to maintain the precedence
// order so even if there are no rules for style tags at this precedence we emit an empty style
// tag with the data-precedence attribute
if (!hasStylesheets || hrefs.length) {
writeChunk(this, styleTagResourceOpen1);
writeChunk(this, styleQueue.precedence);
if (nonce) {
writeChunk(this, styleTagResourceOpen2);
writeChunk(this, nonce);
}
let i = 0;
if (hrefs.length) {
writeChunk(this, styleTagResourceOpen2);
writeChunk(this, styleTagResourceOpen3);
for (; i < hrefs.length - 1; i++) {
writeChunk(this, hrefs[i]);
writeChunk(this, spaceSeparator);
}
writeChunk(this, hrefs[i]);
}
writeChunk(this, styleTagResourceOpen3);
writeChunk(this, styleTagResourceOpen4);
for (i = 0; i < rules.length; i++) {
writeChunk(this, rules[i]);
}
Expand Down Expand Up @@ -5534,6 +5561,7 @@ export type StyleQueue = {
rules: Array<Chunk | PrecomputedChunk>,
hrefs: Array<Chunk | PrecomputedChunk>,
sheets: Map<string, StylesheetResource>,
nonce?: ?Chunk,
};

export function createHoistableState(): HoistableState {
Expand Down
45 changes: 45 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10331,4 +10331,49 @@ describe('ReactDOMFizzServer', () => {
</html>,
);
});

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,
);
});

// @gate __DEV__
Copy link
Collaborator

@gnoff gnoff Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

it('warns when it encounters a mismatched nonce on a style', 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}${CSPnonce}`}>{`.bar { background-color: blue; }`}</style>
</>,
);
pipe(writable);
});
assertConsoleErrorDev([
'React encountered a hoistable style tag with "R4nd0mR4nd0m" nonce. It doesn\'t match the previously encountered nonce "R4nd0m". They have to be the same',
]);
});
});
79 changes: 79 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8435,6 +8435,85 @@ background-color: green;
: '\n in body (at **)' + '\n in html (at **)'),
]);
});

it('can emit styles with nonce', async () => {
const nonce = 'R4nD0m';
const fooCss = '.foo { color: hotpink; }';
const barCss = '.bar { background-color: blue; }';
const bazCss = '.baz { border: 1px solid black; }';
await act(() => {
renderToPipeableStream(
<html>
<body>
<Suspense>
<BlockedOn value="first">
<div>first</div>
<style href="foo" precedence="default" nonce={nonce}>
{fooCss}
</style>
<style href="bar" precedence="default" nonce={nonce}>
{barCss}
</style>
<BlockedOn value="second">
<div>second</div>
<style href="baz" precedence="default" nonce={nonce}>
{bazCss}
</style>
</BlockedOn>
</BlockedOn>
</Suspense>
</body>
</html>,
).pipe(writable);
});

expect(getMeaningfulChildren(document)).toEqual(
<html>
<head />
<body />
</html>,
);

await act(() => {
resolveText('first');
});

expect(getMeaningfulChildren(document)).toEqual(
<html>
<head />
<body>
<style
data-href="foo bar"
data-precedence="default"
media="not all"
nonce={nonce}>
{`${fooCss}${barCss}`}
</style>
</body>
</html>,
);

await act(() => {
resolveText('second');
});

expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<style data-href="foo bar" data-precedence="default" nonce={nonce}>
{`${fooCss}${barCss}`}
</style>
<style data-href="baz" data-precedence="default" nonce={nonce}>
{bazCss}
</style>
</head>
<body>
<div>first</div>
<div>second</div>
</body>
</html>,
);
});
});

describe('Script Resources', () => {
Expand Down
Loading