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.

Sign up for GitHub

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

Jump to bottom

fix(ssr): normalize manifest filenames #3315

Closed
wants to merge 0 commits into from
Closed

fix(ssr): normalize manifest filenames #3315

wants to merge 0 commits into from

Conversation

ferdinando-ferreira
Copy link
Contributor

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.

git clone https://github.com/vitejs/vite.git vite
cd vite
yarn
yarn build
cd packages/playground/ssr-vue
yarn build

Two files will contain complete paths from the source code:

  • packages/playground/ssr-vue/dist/client/ssr-manifest.json
  • packages/playground/ssr-vue/dist/server/entry-server.js

This PR changes the generation of both the manifest and the ssrContext by

  • using, instead of the filenames, their sha-256 hashes to index the data structure, to ensure they are not present in the resulting code
  • using, instead of the complete filenames, their filenames relative to options.root, to ensure compilations of the same codebase in different tree structures will result in the same hashes, ensuring determinism

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@ferdinando-ferreira ferdinando-ferreira mentioned this pull request May 9, 2021
Closed
6 tasks
@Shinigami92 Shinigami92 added feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority) plugin: vue labels May 10, 2021
@frandiox frandiox mentioned this pull request May 13, 2021
Closed
@brillout
Copy link
Contributor

brillout commented May 24, 2021 •
edited

Why hashing?

@ferdinando-ferreira
Copy link
Contributor Author

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.

@brillout
Copy link
Contributor

brillout commented May 24, 2021 via email

@ferdinando-ferreira
Copy link
Contributor Author

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.

@brillout
Copy link
Contributor

brillout commented May 24, 2021 via email

@ferdinando-ferreira
Copy link
Contributor Author

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.

vite/packages/playground/ssr-vue/src/entry-server.js

Lines 25 to 40 in 3475351

function renderPreloadLinks(modules, manifest) {
let links = ''
const seen = new Set()
modules.forEach((id) => {
const files = manifest[id]
if (files) {
files.forEach((file) => {
if (!seen.has(file)) {
seen.add(file)
links += renderPreloadLink(file)
}
})
}
})
return links
}

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?

@brillout
Copy link
Contributor

brillout commented May 24, 2021 via email

@ferdinando-ferreira
Copy link
Contributor Author

ferdinando-ferreira commented May 25, 2021 •
edited

I still don't see much of a security concern.

The reasoning for this concern is the same reasoning that motivates the generation of sourcemaps to default to false on production builds in most builders including vite, rollup and webpack.

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.

For example #2282.

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 vite on middleware mode. Now

  1. It is highly inadvisable to use undocumented internal artifacts of a library to implement a functionality for this exact reason. It either may be changed in the future and break functionalities that depend on them; or be frozen and not allowed to change even if needed because of these "accidental" dependencies.
  2. The initial ticket (SSR HTML transformation in middleware mode doesn't add style tags for statically imported css  #2013) was correctly closed with the following comment: "That's expected, imported CSS are dynamically injected at runtime during dev.".
    It should not really a concern for the initial render to avoid FOUC considering it is a HMR environment which will flash content with each source change, ranging from small content changes to full page refreshes.
  3. It is possible to implement the same functionality in a way that this PR does not "make their life harder" for the projects depending on the unhashed id as long as it is in development time.

Here are two branches prepared to illustrate that, both based on the ssr-vue example in the playground

First: altering the current (pre hash) ssr-vue example to append the <style> to the html generated for the development version in middleware mode. The steps to test are

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 <head> tag of the html.

The changes from the vanilla ssr-vue are simple and can be seen at ferdinando-ferreira@e70bd50

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 before-merge branch)

https://github.com/ferdinando-ferreira/vite/blob/e1c8d3e7706a54c64c70283a5733e6636c507285/packages/playground/ssr-vue/server.js#L123-L128

          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.

@ferdinando-ferreira ferdinando-ferreira mentioned this pull request May 25, 2021
Closed
@brillout
Copy link
Contributor

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.

@ferdinando-ferreira
Copy link
Contributor Author

ferdinando-ferreira commented May 26, 2021 •
edited

Replied at #3303.

It is not a breaking change for the intended use case of the manifest, both ssr-manifest.json and ctx.modules were changed in a way that their keys continue to match and that's the entire reason for ssr-manifest to exists.

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

@brillout
Copy link
Contributor

That's precisely the point: if this PR would get merged it would break vite-plugin-ssr and other projects as well.

This PR is not a bug fix.

This is actually not the case. By design, Vite does things properly in that regard. Maybe you found a bug, but I couldn't reproduce it; moving the root folder around in my FS doesn't break things.

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.)

I believe you and I will not be able to find common ground in this matter

Looks like it, I appreciate that you don't take our disagreement personally :-).

@ferdinando-ferreira
Copy link
Contributor Author

ferdinando-ferreira commented May 26, 2021 •
edited

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 vite.

That's precisely the point: if this PR would get merged it would break vite-plugin-ssr and other projects as well.

That is always a risk when using internal unpublished data structures in a way that is not sanctioned by the developers of the tool.

@brillout
Copy link
Contributor

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 vite.

Love it. I'm curious, what are you building?

That is always a risk when using internal unpublished data structures in a way that is not sanctioned by the developers of the tool.

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.

@patak-dev
Copy link
Member

patak-dev commented Jun 6, 2021 •
edited by Shinigami92

@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?

@brillout
Copy link
Contributor

brillout commented Jun 7, 2021

We think that it is important to stop leaking the full path in the manifest. [...] path relative to the project root instead of the full path

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

We think that it is important to stop leaking the full path in the manifest

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 :-).

@ferdinando-ferreira
Copy link
Contributor Author

@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

@ferdinando-ferreira
Copy link
Contributor Author

@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 (/packages/playground/ssr-vue/dist/client/ssr-manifest.json) in my system, with c:/tests/vite being the vite cloned folder:

{
  "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 /packages/playground/ssr-vue/dist/server/entry-server.js

$ 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 entry-server.js contain the absolute of his machine, which will mismatch the ssr-manifest.json generated by the first programmer without any change whatsoever in the source code for either member of the team.

$ 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");

@brillout
Copy link
Contributor

brillout commented Jun 7, 2021

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.

@ferdinando-ferreira ferdinando-ferreira mentioned this pull request Jun 7, 2021
Merged
9 tasks
@ferdinando-ferreira
Copy link
Contributor Author

@patak-js : Closed and resubmitted with the requested changes on #3706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patak-dev patak-dev Awaiting requested review from patak-dev

@antfu antfu Awaiting requested review from antfu

Assignees
No one assigned
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ssr-manifest.json and ssrContext disclose folder structure present in the source code
4 participants
@ferdinando-ferreira @brillout @patak-dev @Shinigami92