-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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(ssr): normalize manifest filenames #3315
Conversation
Why hashing? |
It prevents details from the inner organization of the code from being disclosed in the distribution files. It also doesn't add any drawback and potentially reduce the size of the ssr manifest file. |
Paths relative to `root` are not disclosing any additional information.
I don't see the size of the ssr manifest to be an issue.
…On Mon, May 24, 2021 at 1:23 PM ferdinando-ferreira < ***@***.***> wrote:
why hashing?
It prevents details from the inner organization of the code from being
disclosed in the distribution files. It also doesn't add any drawback and
potentially reduce the size of the ssr manifest file.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3315 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHVQRSFQNFMH24656JS2KDTPIZLXANCNFSM44OJRZCQ>
.
|
That would be true in the assumption that the source code used to generates the build always accompanies the distribution files. That is not always the case. And when that's not the case, having information about the inner organization of the source code to be disclosed in the distribution files may be detrimental. Not disclosing it carries no drawback and solves a potential problem. |
Who would be able to read the manifest that you don't want to? Distribution
files can be read only by your deployment provider, which you need to trust
anyways.
Not disclosing it carries no drawback and solves a potential problem.
A lot of the ecosystem built around Vite and many Vite users are using the
manifest. Hashing the manifest keys would make their life harder in many
ways.
…On Mon, May 24, 2021 at 1:33 PM ferdinando-ferreira < ***@***.***> wrote:
Paths relative to root are not disclosing any additional information.
That would be true in the assumption that the source code used to
generates the build always accompanies the distribution files. That is not
always the case.
And when that's not the case, having information about the inner
organization of the source code to be disclosed in the distribution files
may be detrimental. Not disclosing it carries no drawback and solves a
potential problem.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3315 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHVQRQGV6HFXWHEBVIKPJ3TPI2QDANCNFSM44OJRZCQ>
.
|
Imagine the final result of the distribution (the /dist/client and /dist/server) is a deliverable to the third party using it but not the source code that generated it.
Can you provide an example? From the examples provided at the playground (like https://github.com/vitejs/vite/blob/main/packages/playground/ssr-vue/) it gives the impression that the only requirement to use the ssr-manifest.json is that the key of the manifest file and the key of the vite/packages/playground/ssr-vue/src/entry-server.js Lines 25 to 40 in 3475351
This proposed PR does not change this assumption. Can you provide an example that illustrated the use of the id for the object |
I still don't see much of a security concern.
For example #2282.
…On Mon, May 24, 2021 at 3:03 PM ferdinando-ferreira < ***@***.***> wrote:
Who would be able to read the manifest that you don't want to?
Imagine the final result of the distribution (the /dist/client and
/dist/server) is a deliverable to the third party using it but not the
source code that generated it.
Hashing the manifest keys would make their life harder in many ways.
Can you provide an example?
From the examples provided at the playground (like
https://github.com/vitejs/vite/blob/main/packages/playground/ssr-vue/) it
gives the impression that the only requirement to use the ssr-manifest.json
is that the key of the manifest file and the key of the ctx.modules
populated by the renderToString method matches.
https://github.com/vitejs/vite/blob/3475351a371df2e17689a99968fdee827b0b2d16/packages/playground/ssr-vue/src/entry-server.js#L25-L40
This proposed PR does not change this assumption. Can you provide an
example that illustrated the use of the id for the object
ssr-manifest.json in a different way than simply matching the ids for
ctx.modules?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3315 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHVQRVB2VNG7LA2AUL2KBTTPJFC3ANCNFSM44OJRZCQ>
.
|
The reasoning for this concern is the same reasoning that motivates the generation of sourcemaps to default to It is generally undesirable to have details of the internal structure of the source code present in the files generated for public consumption. That it is true even if it is not a concern for some project and developers because their source code accompanies the distributed files.
I see, the use case for keeping the id plain instead of hashed is to allow for it to be used to provide a way to generate the preload resources for ssr on
Here are two branches prepared to illustrate that, both based on the ssr-vue example in the playground First: altering the current (pre hash) cd c:\tests
git clone https://github.com/ferdinando-ferreira/vite.git --single-branch -b before-merge before-merge
cd before-merge
yarn
yarn build
cd packages/playground/ssr-vue
yarn dev Opening localhost:3000 will then show the dev environment, with the styles appended to the The changes from the vanilla Second: with this well streamlined version it would be easy to adapt it to work with this PR merged in. The steps to test are cd c:\tests
git clone https://github.com/ferdinando-ferreira/vite.git --single-branch -b after-merge after-merge
cd after-merge
yarn
yarn build
cd packages/playground/ssr-vue
yarn dev This branch is the same of the other but it has the PR merged in and the necessary change to deal with the hashed id, which are the following lines (in addition to what was presented in the const { createHash } = require('crypto')
const normalizedId = path.relative(vite.config.root, module.id)
const hashedId = createHash('sha256')
.update(normalizedId)
.digest('hex')
manifestIdToModule[hashedId] = module Even if it works as specified, all of this is highly inadvisable tho. It makes use of undocumented internal artifacts of the build process, which can change at any moment in the future with the easy to predict consequences. With that said, this whole example demonstrates that, as long as the only use cases are in development, hashing the id of the module brings no impediment whatsoever for any implementation. |
I'm still not convinced about this whole thing. See my reply at #3303. AFAICT, this PR only introduces a breaking change that would make my life harder (and of anyone using the manifest) without real benefit. |
Replied at #3303. It is not a breaking change for the intended use case of the manifest, both Any other use for the keys of these data structure (outside of matching these two variables) is undocumented and likely unintended. With that said, a way for the use case put forward at #2282 was provided. Even with this PR merged it continues to be possible to implement it. Did you have the chance to look at it? It solves the same problem without being hindered by this PR |
That's precisely the point: if this PR would get merged it would break This PR is not a bug fix.
This means that the only benefit of this PR is a (from my perspective miniscule and negligible) reduction in attack surface. Is that worth it for breaking the ecosytem and making our life harder? (Your code snippet actually shows that things become more complex, and there are other aspects of why your PR makes things harder.)
Looks like it, I appreciate that you don't take our disagreement personally :-). |
Quite the contrary, it motivates me to find a solution that is mutually agreeable as it is very clear that your goals and mine align perfectly: a well designed and implemented ssr system based on
That is always a risk when using internal unpublished data structures in a way that is not sanctioned by the developers of the tool. |
Love it. I'm curious, what are you building?
I knew you'd say that ;-). Well, the manifest data structur is not internal and is part of the public API "contract". I do (rightfully) expect the manifest to not change. |
@ferdinando-ferreira we discussed this with Evan. We think that it is important to stop leaking the full path in the manifest. Evan proposed to us the path relative to the project root instead of the full path, avoiding hashes. What do you think? @brillout would you confirm that this change will work for you too? |
This is actually already the case. If you look at the paths of the manifest, you'll see they are already all relative to the project root. You can test and actually move the project directory somewhere else in your filesystem and everything keeps working (which is important for users who build locally upon deployement). If you want to check for yourself: yarn create vite-plugin-ssr
cd vite-ssr-project/
yarn install
yarn build
cat dist/client/manifest.json # see that all paths are relative to the project root
cd ../
mv vite-ssr-project/ /tmp/
cd /tmp/vite-ssr-project/
yarn server:prod # see that things still work even though we moved the project root
Yes and AFAICT Vite already doesn't leak any path outside the project root. To sum up: this PR only hashes the relative path and that's why I'm against this PR. @ferdinando-ferreira Thanks for the PR though and also your vite-plugin-ssr PR 😊 Looking forward to review further of your PRs :-). |
@patak-js : after some time to reflect I agree that normalizing the file path and hashing the file path are two distinct changes, one essential and one elective. I'll change the PR to only include the normalization part |
@brillout : the problem arises when there is a team with multiple programmers in different systems coding different parts of the application. Here is the basic use case for the example, please follow these instructions so you can understand the point being made: git clone https://github.com/vitejs/vite.git vite
cd vite
yarn
yarn build
cd packages/playground/ssr-vue
yarn build Here is the resulting ssr manifest ( {
"c:/tests/vite/packages/playground/ssr-vue/src/App.vue?vue&type=style&index=0&lang.css": [
"/assets/Inter-Italic.bab4e808.woff2",
"/assets/Inter-Italic.7b187d57.woff"
],
"c:/tests/vite/packages/playground/ssr-vue/src/App.vue": [
"/assets/Inter-Italic.bab4e808.woff2",
"/assets/Inter-Italic.7b187d57.woff"
],
"vite/preload-helper": [
"/assets/Inter-Italic.bab4e808.woff2",
"/assets/Inter-Italic.7b187d57.woff"
],
"c:/tests/vite/packages/playground/ssr-vue/src/router.js": [
"/assets/Inter-Italic.bab4e808.woff2",
"/assets/Inter-Italic.7b187d57.woff"
],
"c:/tests/vite/packages/playground/ssr-vue/src/main.js": [
"/assets/Inter-Italic.bab4e808.woff2",
"/assets/Inter-Italic.7b187d57.woff"
],
"c:/tests/vite/packages/playground/ssr-vue/src/entry-client.js": [
"/assets/Inter-Italic.bab4e808.woff2",
"/assets/Inter-Italic.7b187d57.woff"
],
"c:/tests/vite/packages/playground/ssr-vue/index.html": [
"/assets/Inter-Italic.bab4e808.woff2",
"/assets/Inter-Italic.7b187d57.woff"
],
"C:/tests/vite/node_modules/@vue/shared/dist/shared.esm-bundler.js": [
"/assets/vendor.42400580.js"
],
"C:/tests/vite/node_modules/@vue/reactivity/dist/reactivity.esm-bundler.js": [
"/assets/vendor.42400580.js"
],
"C:/tests/vite/node_modules/@vue/runtime-core/dist/runtime-core.esm-bundler.js": [
"/assets/vendor.42400580.js"
],
"C:/tests/vite/node_modules/@vue/runtime-dom/dist/runtime-dom.esm-bundler.js": [
"/assets/vendor.42400580.js"
],
"C:/tests/vite/node_modules/vue/dist/vue.runtime.esm-bundler.js": [
"/assets/vendor.42400580.js"
],
"C:/tests/vite/node_modules/vue-router/dist/vue-router.esm-bundler.js": [
"/assets/vendor.42400580.js"
],
"c:/tests/vite/packages/playground/ssr-vue/src/pages/About.vue?vue&type=style&index=0&scoped=true&lang.css": [
"/assets/About.6f080a3a.js",
"/assets/About.41cb2065.css"
],
"c:/tests/vite/packages/playground/ssr-vue/src/pages/About.vue": [
"/assets/About.6f080a3a.js",
"/assets/About.41cb2065.css"
],
"c:/tests/vite/packages/playground/ssr-vue/src/assets/logo.png": [
"/assets/Home.c51e372c.js",
"/assets/Home.b0141ad0.css",
"/assets/logo.03d6d6da.png"
],
"@foo": [
"/assets/Home.c51e372c.js",
"/assets/Home.b0141ad0.css",
"/assets/logo.03d6d6da.png"
],
"c:/tests/vite/packages/playground/ssr-vue/src/pages/Home.vue?vue&type=style&index=0&scoped=true&lang.css": [
"/assets/Home.c51e372c.js",
"/assets/Home.b0141ad0.css",
"/assets/logo.03d6d6da.png"
],
"c:/tests/vite/packages/playground/ssr-vue/src/pages/Home.vue": [
"/assets/Home.c51e372c.js",
"/assets/Home.b0141ad0.css",
"/assets/logo.03d6d6da.png"
],
"c:/tests/vite/packages/playground/ssr-vue/src/components/ImportType.vue?vue&type=script&setup=true&lang.ts": [
"/assets/ImportType.63fd8073.js"
],
"c:/tests/vite/packages/playground/ssr-vue/src/components/ImportType.vue": [
"/assets/ImportType.63fd8073.js"
],
"c:/tests/vite/packages/playground/ssr-vue/src/components/foo.css": [
"/assets/Foo.d979b2f9.js",
"/assets/Foo.a8752494.css"
],
"c:/tests/vite/packages/playground/ssr-vue/src/components/Foo.jsx": [
"/assets/Foo.d979b2f9.js",
"/assets/Foo.a8752494.css"
]
} This is matched by the references on $ grep "c:/" entry-server.js
(ssrContext.modules || (ssrContext.modules = new Set())).add("c:/tests/vite/packages/playground/ssr-vue/src/App.vue");
(ssrContext.modules || (ssrContext.modules = new Set())).add("c:/tests/vite/packages/playground/ssr-vue/src/pages/About.vue");
(ssrContext.modules || (ssrContext.modules = new Set())).add("c:/tests/vite/packages/playground/ssr-vue/src/pages/Home.vue");
(ssrContext.modules || (ssrContext.modules = new Set())).add("c:/tests/vite/packages/playground/ssr-vue/src/components/ImportType.vue"); Now assume a second programmer from the team, in a different workstation using a different path. This second programmer builds only the server in his machine (without any change in the code) and deploy the resulting entry-server.js. git clone https://github.com/vitejs/vite.git vite2
cd vite2
yarn
yarn build
cd packages/playground/ssr-vue
yarn build:server Now the references to the ssr modules in his $ grep "c:/" entry-server.js
(ssrContext.modules || (ssrContext.modules = new Set())).add("c:/tests/vite2/packages/playground/ssr-vue/src/App.vue");
(ssrContext.modules || (ssrContext.modules = new Set())).add("c:/tests/vite2/packages/playground/ssr-vue/src/pages/About.vue");
(ssrContext.modules || (ssrContext.modules = new Set())).add("c:/tests/vite2/packages/playground/ssr-vue/src/pages/Home.vue");
(ssrContext.modules || (ssrContext.modules = new Set())).add("c:/tests/vite2/packages/playground/ssr-vue/src/components/ImportType.vue"); |
Okay I'm seeing the absolute paths in the SSR manifest. Sorry my bad I didn't check the SSR manifests. (I only use normal manifests for vite-plugin-ssr.) I'm 👍 for this PR once the hasing thing is removed :-). @patak-js It's only a breaking change for SSR manifests, normal manifests won't be affected. |
e96ad05
to
32c596c
Compare
@patak-js : Closed and resubmitted with the requested changes on #3706 |
Description
Fixed #3303
As described on #3303, both ssr-manifest.json and the ssrContext embedded in the resulting server entry contains information about the filenames present in the source code of the compilation.
Additional context
It is possible to reproduce using the ssr-vue own example in this codebase.
Two files will contain complete paths from the source code:
This PR changes the generation of both the manifest and the ssrContext by
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).