-
Notifications
You must be signed in to change notification settings - Fork 57
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.
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
Chore/2.0.0 #492
base: main
Are you sure you want to change the base?
Chore/2.0.0 #492
Conversation
feat(csp): support style nonce in development
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
feat-#487: local dev with nuxt devtools
feat(doc): introduce Nuxt Scripts as alternative to `useScript`
fix: ensure RegExp[] origin can be passed to appSecurityOptions
@vejja if you agree, I would love to merge this PR and release a brand new 2.0.0 version :) |
@@ -215,15 +215,6 @@ export default defineNuxtConfig({ | |||
- `"'nonce-{{nonce}}'"` placeholder: Include this value in any individual policy that you want to be governed by nonce. | |||
|
|||
|
|||
::alert{type="warning"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section should not be removed
The vite nonce is only inserted in dev mode, if I understand correctly
@dargmuesli is this true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all offending actions are only executed in dev mode as well. Styles should not be added through JavaScript by Nuxt in production as far as I know. They're just referenced as compiled CSS content in production, at least without changes to any configuration.
I have the fix contained in this PR running for a few of my sites for quite some time now and have not observed any issues (even with close monitoring done by Sentry).
Maybe @danielroe could quickly confirm or refute that style loading through JavaScript only happens in dev mode by vite and, by default, not by Nuxt in production?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure we had to use unsafe-inline
in styles in our own doc website, because @nuxt/ui uses pinceau under the hood, and pinceau modifies styles dynamically at runtime via Javascript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but that's somewhat 3rd party, no? The section talks about "Nuxt's mechanism for Client-Side hydration of styles" so maybe the wording should be changed then to reflect the actual reason for the recommendation better. I'd still say the recommendation doesn't really need to be that prominent for the pinceau use case though, but that's just an opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll unsubscribe from this PR to reduce noise, just tag me again if you need me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got 2 general comments on this PR:
1- Let's check whether unbuild
manages to find the local imports. I think I remember we need to add them manually to package.json
2- The regex serializer/deserializer is quite complex. Unless @Baroshem is comfortable with the logic, I would love to see comments and test examples added, so that we understand better the rationale
If the CSP header is malformed or does not exist, the value will be undefined.
Signed-off-by: Pascal Sthamer <10992664+P4sca1@users.noreply.github.com>
docs: update information about Nuxt Image
Signed-off-by: Pascal Sthamer <10992664+P4sca1@users.noreply.github.com>
Signed-off-by: Pascal Sthamer <10992664+P4sca1@users.noreply.github.com>
</script> | ||
``` | ||
When using `<NuxtImg>` or `<NuxtPicture>`, an inline script will be used for error handling during SSR. | ||
This will lead to CSP issues if `unsafe-inline` is not allowed and the image fails to load. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @P4sca1
Isn’t it a bit severe ?
You could probably use ‘unsafe-hashes’ here, and the inline code is always the same so you could pre-hash it.
I do agree this is not ideal though. @harlan-zw was able to replace all inline event handlers with addEventListener in @nuxt/scripts so maybe the team at NuxtImg can use the same approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline code will indeed be always the same, so using unsafe-hashes
could work. Maybe we could add it by default in non strict mode or behind a feature flag in strict mode to support <NuxtImg>
and <NuxtPicture>
.
I calculated the hash that would be needed:
echo -n "this.setAttribute('data-error', 1)" | openssl sha256 -binary | openssl base64
bwK6T5wZVTANitXbrTsel7kl/PyCjCd/Dq5Qoz3imjM=
Using addEventListener
in this case is not trivial, because the event listener would be attached in onMounted()
, which is too late for some kind of errors. So some errors, e.g. when the url is invalid, could be missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @P4sca1
What is the issue when CSP denies execution of the error handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data-error
property does not get set on the image tag and the error event is never emitted.
From a user's perspective the page works fine, it just shows an unloaded image.
@Baroshem just a general comment here on providing a regex serializer/deserializer On balance though, the CORS handler is a native |
Hmm, at this point maybe it would be safer to revert this change and plan it for 2.1.0? What would be your recommendation Sebastien? :) |
Yes, agree with this Jakub. |
This reverts commit 4528880.
Thanks for pointing out the documentary :) |
About the test and document, I want to know if there's a better solution to solve this problem. #497 it maybe or should be replaced with a better solution |
Understood @Shana-AE, thanks for the explanation |
Here is my attempt on fixing the RegExp issue: #509 UPDATE: I also added a new test fixture for CORS with my PR. |
Types of changes
#475
#488
#485
#496
#494
#501
Description
Checklist: