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

nonce is missing when rendering a <NuxtIsland> component during SSR #494

Closed
P4sca1 opened this issue Jul 22, 2024 · 13 comments · Fixed by #502
Closed

nonce is missing when rendering a <NuxtIsland> component during SSR #494

P4sca1 opened this issue Jul 22, 2024 · 13 comments · Fixed by #502
Labels
bug Something isn't working

Comments

@P4sca1
Copy link
Contributor

P4sca1 commented Jul 22, 2024

Version

nuxt-security: 2.0.0-rc.9
nuxt: 3.12.3

Reproduction Link

https://stackblitz.com/edit/github-5zfscl?file=pages%2Findex.vue

Steps to reproduce

Check the network log for the initial document request. You will see that the nonce- is missing from the script-src.

Bildschirmfoto 2024-07-22 um 15 37 11

What is Expected?

The nonce- should be there. You can remove the <ServerComponent /> and reload the page. The nonce will be there again.

What is actually happening?

I added some loggings and it seems like the rendering of the page and the server component share the nonce. The nonce is then removed when rendering the page, because it already exists.

@P4sca1 P4sca1 added the bug Something isn't working label Jul 22, 2024
@Baroshem
Copy link
Collaborator

Hey @P4sca1

Thanks for reporting it.

As you have already did some reasearch, maybe you would be interested in implementing a fix for that? :)

Also CC @huang-julien

@P4sca1
Copy link
Contributor Author

P4sca1 commented Jul 26, 2024

The last time I wasn't able to fix the issue, due to missing information about how NuxtIsland rendering works internally. I think rendering a page during SSR with NuxtIsland components shares the ssrContext. I will investigate further using a minimal reproduction and work on a fix.

@P4sca1
Copy link
Contributor Author

P4sca1 commented Jul 28, 2024

@Baroshem I was able to add a test case, which reproduces the behavior. I can confirm that the root cause is, that Nuxt calls the request and the render:html hook multiple times when rendering NuxtIsland components with different events, that share the same context.

I updated the nonce generation so that nonces are only generated if they do not already exist in the context. There may be other places in the code, where additional care needs to be taken if a context is shared between multiple requests / renders.

When rendering the page with a server-only component, the CSP header already includes a script-src entry with a resolved nonce. However, already resolved nonces are filtered away in this line:
https://github.com/Baroshem/nuxt-security/blob/2d5128244e3c53ecc100cdca41e45f593ec086c4/src/runtime/nitro/plugins/50-updateCsp.ts#L34
Just removing the line resolves the issue. I am wondering why it was added in the first place. Could you give some insights?

@P4sca1
Copy link
Contributor Author

P4sca1 commented Jul 28, 2024

I just noticed that the resolveSecurityRules functionality (using different rules depending on the request path) will also break when using server-only components.

The NuxtIsland component will be rendered first. Rules are created for a path similar to /__nuxt_island/ServerComponent_cv0OmafuIw.json?props=%7B%7D locFIxi6Z4wKnnO8L6MuHw==. Afterwards, the page will be rendered, but because the rules already exist in the context, they will never be generated for the actual page path.

@P4sca1
Copy link
Contributor Author

P4sca1 commented Jul 28, 2024

Before implementing a fix, I created a discussion in the nuxt repo to verify if context sharing is expected behavior:
nuxt/nuxt#28340

@Baroshem
Copy link
Collaborator

I think we would need opinion from @vejja about it :)

@vejja
Copy link
Collaborator

vejja commented Jul 29, 2024

Will look at it

vejja added a commit that referenced this issue Jul 30, 2024
<!--- Provide a general summary of your changes in the title above -->

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (a non-breaking change which fixes an issue)
- [x] New feature (a non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Description
<!--- Describe your changes in detail -->
<!--- Why is this change required? What problem does it solve? -->
<!--- If it resolves an open issue, please link to the issue here. For example "Resolves: #137" -->

Closes #494

This PR introduces support for Nuxt Server Components (a.k.a Islands).

## Checklist:
<!--- Put an `x` in all the boxes that apply. -->
<!--- If your change requires a documentation PR, please link it appropriately -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [ ] I have added tests to cover my changes (if not applicable, please state why)
@vejja vejja mentioned this issue Jul 30, 2024
6 tasks
@vejja vejja linked a pull request Jul 30, 2024 that will close this issue
6 tasks
@vejja
Copy link
Collaborator

vejja commented Jul 30, 2024

I'm proposing a very simple fix in PR #500
Bottom line is right now we do not support Nuxt Islands, and this PR is a tentative to introduce support for Islands.

It is only a tentative proposal at this stage, because I do not use Islands myself, so I'm not very knowledgeable of all potential edge cases.

@P4sca1 would you like to test the fix, and also add some tests to verify if it works ?

@vejja
Copy link
Collaborator

vejja commented Jul 30, 2024

Just removing the line resolves the issue. I am wondering why it was added in the first place. Could you give some insights?

That is a very valid remark. At first sight it looks like it's a legacy from the old code base when we didn't have the security context. I will need to double-check but this might have become superfluous and we should probably remove.

Update: So it is code legacy, but still I'd like to keep it because it ensures that nobody can set a constant nonce via options

@vejja
Copy link
Collaborator

vejja commented Jul 30, 2024

Before implementing a fix, I created a discussion in the nuxt repo to verify if context sharing is expected behavior: nuxt/nuxt#28340

It rings a bell. I remember I had a hard time working around what the context was supposed to be, and I figured out it's not the same thing if the request is coming from the client (browser) or if it's a server-generated request. Would love to know the answer of the Nuxt core team also

@P4sca1
Copy link
Contributor Author

P4sca1 commented Jul 31, 2024

Hey @vejja,

thank you for the update. I already worked on a PR for this issue and published it in #502.

Changes are similar to your PR. Additional things that are included:

  • Log warning when removing a static nonce from the header
  • Add a test case for server-only components
  • Don't generate nonce multiple times when rendering island components

My PR does not include the dependency updates, the changes to 60-recombineHtml, and the playground changes. Maybe you could remove the changes for 50-updateCsp from your PR and we can merge both?

@vejja
Copy link
Collaborator

vejja commented Aug 2, 2024

Hey @P4sca1
Sorry, I hadn't seen your PR.
Will merge both

@Baroshem Baroshem mentioned this issue Aug 6, 2024
6 tasks
@Baroshem
Copy link
Collaborator

Released in 2.0.0 :)

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
3 participants