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

feat: Extract initialState to asset to respect CSP #67

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

Conversation

pantajoe
Copy link
Contributor

This PR extracts the initialState (#42) to an asset file at build time and, thus, avoids an inline script.
For that reason, it now respects the default of CSP where no inline scripts are allowed.

Closes #49.

@antfu
Copy link
Member

antfu commented Jul 15, 2021

There are some problem need to be resolved with this solution

  • Each page might have different initial state, which ideally, we need have different initial-state.js file for each entry rendered
    • In this case, it will generate too many additional files which will lead to addition network request, and most of the time, they are just setting empty object.
  • Currently assets/initial-state.js is not hashed - it will lead to incorrect cache state on site updating.

So, I would think this external them as scripts might not be a very good solution. A few ideas:

  • We use nonce to ensure inline script are safe with CSP
  • We smartly detect if the initial state is an empty object, if so, we emit the inline script generation - so most of the case, if you don't actually use this feature, it's safe for you.

@antfu antfu marked this pull request as draft July 15, 2021 18:06
@pantajoe
Copy link
Contributor Author

pantajoe commented Sep 4, 2021

@antfu Sorry it took so long to answer. I had a lot on my plate.

I agree I have not though about these complications. It definitely makes sense to not use the initial state if it is empty. However, wouldn't it be fine to issue a network request for an initial-state.${hash}.js file for each page if an initial state exists? Nuxt.js does it the same way.
I have not worked with nonces on script tags before, but you would need a CSP Response Header with a different nonce on the inline script on each HTTP request, and I am not sure how you would do this with a statically generated site.

@pantajoe pantajoe marked this pull request as ready for review September 8, 2021 10:03
@pantajoe pantajoe force-pushed the feat/respect-csp-for-initial-state branch from 772717e to c4f5b63 Compare September 8, 2021 10:08
@userquin
Copy link
Member

userquin commented Sep 8, 2021

@antfu The initialState is wrong, we should use the initialState inside the promise map instead global state for SSR. This should fix the initialState problem: always use the initialState on first route invocation (on SSR) outside the promise map.

On line 91 we should remove initialState and add it on line 121 (on this PR):

// line 91
const { routes/*, initialState*/ } = await createApp(false) // <= `initialState` should be removed
// line 121
const { app, router, head, initialState } = await createApp(false, route) // <= `initialState` should be added

@userquin
Copy link
Member

userquin commented Sep 8, 2021

I don't know if you're fixing it and I don't want to bother you, I added this comment a few days ago: #72 (comment)

@userquin
Copy link
Member

userquin commented Sep 8, 2021

@pantajoe Can you test with those little changes on local?

@pantajoe
Copy link
Contributor Author

pantajoe commented Sep 8, 2021

@userquin Yeah, I'll test it and let you know if it solves the issue in the comment you linked.

@pantajoe
Copy link
Contributor Author

pantajoe commented Sep 8, 2021

@antfu @userquin Due to the discussion in #72, let's put this on hold and implement this together with the outcome of #72.

It's no longer required to use a router hook
to initialize the initialState
@pantajoe
Copy link
Contributor Author

pantajoe commented Sep 8, 2021

@antfu Sorry for the spam, but on second thought, I believe we can utilize this feature as the initialState now is compatible with CSP. And due to the discussion in #72, it's best if the initialState is only provided globally and not per route.
The isolated page state is another feature that has to be implemented differently and separately.

@userquin What do you think about this?

@userquin
Copy link
Member

userquin commented Sep 8, 2021

@pantajoe I think it is a breaking change, the problem is that the changes on #72 are to fix the expected behavior, it is a bug on vite-ssg.

I suggest you wait the input from @antfu ...

@husayt
Copy link
Contributor

husayt commented Sep 21, 2021

I have raised a separate pr #108 to resolve #72. It solves my issues and will help here as well.

Thanks @userquin for the proposed fix

@pantajoe I think it is a breaking change, the problem is that the changes on #72 are to fix the expected behavior, it is a bug on vite-ssg.

I suggest you wait the input from @antfu ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vite-ssg 生成 inline script 产生的问题
4 participants