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.

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

fix(nonce): duplication #586

Open
dargmuesli opened this issue Dec 11, 2024 · 5 comments
Open

fix(nonce): duplication #586

dargmuesli opened this issue Dec 11, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@dargmuesli
Copy link
Contributor

dargmuesli commented Dec 11, 2024

Environment

- Operating System: Linux
- Node Version:     v22.12.0
- Nuxt Version:     -
- CLI Version:      3.16.0
- Nitro Version:    -
- Package Manager:  [email protected]
- Builder:          -
- User Config:      -
- Runtime Modules:  -
- Build Modules:    -

Nuxt Security Version

v2.1.5

Default setup used?

Yes, the bug happens even if the security option is not customized

Reproduction

will provide if necessary

Description

Currently a nonce is added to elements regardless of a nonce being applied already. For example, the nuxt/image module allows to set nonces for image preload links. The regexes could be changed to not match elements which already have a nonce added.

@dargmuesli dargmuesli added the bug Something isn't working label Dec 11, 2024
@vejja
Copy link
Collaborator

vejja commented Dec 12, 2024

Hi @dargmuesli

I'm a bit annoyed by this one.

On the one hand you have a valid point. Duplicate nonce attributes are not valid, so if a nonce is already present our code should definitely not result in invalid tags.

On the other hand, I think the @nuxt/image implementation is wrong, if not dangerous. Allowing users to inject a nonce by themselves opens a whole can of worms security-wise:

  • How do you make sure the user is not injecting the same value always?
  • Is that value cryptographically secure and generated in accordance with the RFC?
  • How does the user then copies the nonce into the CSP header?

In order to properly implement the nonce, users should always rely on Nuxt Security to achieve that.
Which then means that the nonce property of @nuxt/image is useless.

I would seriously recommend to never use the @nuxt/image nonce property. Installing Nuxt Security takes care of injecting nonces automatically and seamlessly, and provides the necessary security guarantees.

In conclusion, the best way to address the situation should be:
The regexes could be changed to not match replace elements which already have a nonce added

WDYT?

@dargmuesli
Copy link
Contributor Author

Yes, I agree with this train of thought 👍

Alternatively, I thought about changing nuxt/image so that it doesn't add a duplicate nonce when nuxt-security is detected. But your approach fixes this behavior in a general manner, also for other such sources of confusion.

@vejja
Copy link
Collaborator

vejja commented Dec 12, 2024

Pinging our regex czar @GalacticHypernova here 😊
Would you have an idea on how to do this ?

@GalacticHypernova
Copy link
Contributor

GalacticHypernova commented Dec 26, 2024

Hey! So sorry for the late response, I've gotten caught up a bit with personal life. I think I know how to do that, would you like me to make a PR?

Should it replace instances with a nonce, essentially removing the existent one and overriding with the nonce from Nuxt Security?

The only downside with this is that technically it would indeed make the nonce feature useless in Nuxt Image, which could inadvertently act as a sort of deterrent from using Nuxt Security for people who for some reason would want to use their own nonce. I think the best move here would indeed be to just deprecate the nonce in Nuxt Image, to allow only Nuxt Security to handle, well, security.

@vejja
Copy link
Collaborator

vejja commented Dec 26, 2024

Hey! So sorry for the late response, I've gotten caught up a bit with personal life. I think I know how to do that, would you like me to make a PR?

Hey @GalacticHypernova yes a PR would be great, thanks

Should it replace instances with a nonce, essentially removing the existent one and overriding with the nonce from Nuxt Security?

Yes, that’s exactly it

The only downside with this is that technically it would indeed make the nonce feature useless in Nuxt Image, which could inadvertently act as a sort of deterrent from using Nuxt Security for people who for some reason would want to use their own nonce. I think the best move here would indeed be to just deprecate the nonce in Nuxt Image, to allow only Nuxt Security to handle, well, security.

I think it’s ok, as long as we only replace the nonce attribute in the html tag and we don’t delete the nonce value in the CSP header itself, we are not modifying any existing functionality in Nuxt Image, but we are improving security

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants