Skip to content
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

Jump to bottom

LocalizedStringProvider: nonce parameter for Content Security Policy #6219

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

LocalizedStringProvider: nonce parameter for Content Security Policy #6219

wants to merge 4 commits into from
+281 −6

Conversation

Julienng
Copy link
Contributor

@Julienng Julienng commented Apr 17, 2024 •
edited

Closes #6218

✅ Pull Request Checklist:

  • Included link to corresponding LocalizedStringProvider: missing nonce parameter when CSP is enabled.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component). Would like help on where to document there
  • Looked at the Accessibility Practices for this feature - Aria Practices no concern

📝 Test Instructions:

#6218
Reproduction repo and deployed test on vercel to check

🧢 Your Project:

@Julienng Julienng force-pushed the nonce-parameter-for-localized-string-provider branch from 185b9a1 to b2e1311 Compare April 30, 2024 09:41
snowystinger
snowystinger previously approved these changes Apr 30, 2024
View reviewed changes
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Seems reasonable, we should see about adding it to our example project for verification.
https://github.com/adobe/react-spectrum/tree/main/examples/next-app

@Julienng
Copy link
Contributor Author

Oh i missed that one, I can work on that based on this reproduction bug repository:

https://github.com/Julienng/csp-next-and-react-aria-components-provider

@Julienng Julienng force-pushed the nonce-parameter-for-localized-string-provider branch from d726ab8 to 70b6c49 Compare May 2, 2024 11:05
@Julienng
Copy link
Contributor Author

Julienng commented May 2, 2024

@snowystinger I've added the example project. It matches with the next-app but with CSP

By testing it, I noticed hydration error with CSP because the browser remove nonce parameter from the DOM before React hydration.

To circumvent that problem, I followed the same pattern as next-themes in this PR: pacocoursey/next-themes#262

new code:

<script nonce={typeof window === 'undefined' ? nonce : ''} suppressHydrationWarning dangerouslySetInnerHTML={{__html: getPackageLocalizationScript(locale, strings)}} />;

@Julienng Julienng requested a review from snowystinger May 2, 2024 11:45
@Julienng Julienng force-pushed the nonce-parameter-for-localized-string-provider branch from 70b6c49 to 84fdccb Compare May 6, 2024 06:24
@Julienng Julienng force-pushed the nonce-parameter-for-localized-string-provider branch from 84fdccb to 81099b7 Compare May 13, 2024 12:26
@snowystinger
Copy link
Member

Linking relevant React issue facebook/react#26028

snowystinger
snowystinger reviewed May 14, 2024
View reviewed changes
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Ok, might be a silly question, but how do I run this to verify what it's doing?
I tried

cd examples/next-app-csp
yarn
yarn dev

and it just renders a blank page with an error, which is about the nonce, element in question. So is this what it's supposed to do?
Screenshot 2024-05-14 at 5 43 20 PM

@Julienng
Copy link
Contributor Author

Oh yeah, I had to yarn build before for the example to work. I don't know if this is expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snowystinger snowystinger snowystinger left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LocalizedStringProvider: missing nonce parameter when CSP is enabled
2 participants
@Julienng @snowystinger