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

ssr-manifest.json and ssrContext disclose folder structure present in the source code #3303

Closed
6 tasks done
ferdinando-ferreira opened this issue May 7, 2021 · 4 comments · Fixed by #3706
Closed
6 tasks done

Comments

@ferdinando-ferreira
Copy link
Contributor

ferdinando-ferreira commented May 7, 2021

Describe the bug

In a ssr build, ssr-manifest.json and ssrContext (in the dist/server/entry-server.js file), will contain the complete filenames (including the folder structure) present in the source code. This causes two problems:

  • It may "leak" personal or confidential information about the source code being built
    • It could be the internal and private structure of the source code
    • it could be the name of the developers building their applications (if it is present, for instance, in the home folder where the application source code resides)
  • More importantly, it makes a build "non deterministic"
    • Two programmers in the same organization, using the exact same codebase, will generate distinct builds differing only on these specific files, based on the layout of their file systems and where they stored the source code

Submitted #3315 as a comprehensive fix

Reproduction

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

System Info

Output of npx envinfo --system --npmPackages vite,@vitejs/plugin-vue --binaries --browsers:

  System:
    OS: Windows 10 10.0.19041
    CPU: (8) x64 Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
    Memory: 3.22 GB / 7.91 GB
  Binaries:
    Node: 15.11.0
    Yarn: 1.22.10
    npm: 7.6.0
  Browsers:
    Chrome: 90.0.4430.93
    Edge: Spartan (44.19041.906.0), Chromium (90.0.818.51)
    Internet Explorer: 11.0.19041.1

Used package manager:
yarn

Logs

N/A


Other comments

Here is a "naive" solution for the root cause
EDIT: Sent a #3315 as a more robust implementation that solves both the "leak" and the determinism issues.

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

@brillout
Copy link
Contributor

More importantly, it makes a build "non deterministic"
Two programmers in the same organization, using the exact same codebase, will generate distinct builds differing only on these specific files, based on the layout of their file systems and where they stored the source code

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.

It may "leak" personal or confidential information about the source code being built

If you don't want to expose the structure of your source code, simply don't make it public. No one forces you to make your manifest files public.

As far as I'm concerned there is no issue here.

@ferdinando-ferreira
Copy link
Contributor Author

If you don't want to expose the structure of your source code, simply don't make it public.

There are plenty of cases where the resulting files of the build are meant to be a deliverable but not the source code. I believe you and I will not be able to find common ground in this matter so I'll let the people responsible for reviewing the PR of this software to decide.

Did you have the opportunity to take a look at the example I provided in the PR? It solves the use case put forward in #2282 (preload links in dev mode ssr) and is not hindered by #3315.

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

The relevant change is

https://github.com/ferdinando-ferreira/vite/blob/e70bd50575bdf8413d77fbd0d10c0e669f1b3dcb/packages/playground/ssr-vue/server.js#L109-L145

      let preloadLinks = ''
      let idToCode = {}
      let manifest
      if (isProd) {
        manifest = prodManifest
      } else {
        function requireFromString(src) {
          var Module = module.constructor
          var m = new Module()
          m._compile(src, '')
          return m.exports
        }
        const manifestIdToModule = {}
        vite.moduleGraph.idToModuleMap.forEach((module) => {
          manifestIdToModule[module.id] = module
          if (/.css$/.test(module.id)) {
            if (module.transformResult?.code) {
              const found = module.transformResult.code
                .split(/\r?\n/)
                .filter((line) => /^const css/.test(line))
              if (found) {
                idToCode[module.id] = requireFromString(
                  found[0] + '\nmodule.exports = css'
                )
              }
            }
          }
        })
        const devManifest = {}
        Array.from(modules).forEach((moduleId) => {
          devManifest[moduleId] = Array.from(
            manifestIdToModule[moduleId].importedModules
          ).map((module) => module.id)
        })
        manifest = devManifest
      }
      preloadLinks = renderPreloadLinks(modules, manifest, isProd, idToCode)

That reads the correct data structure for the use case you are trying to solve (that being vite.moduleGraph.idToModuleMap) and can be trivially fixed even after that PR is merged.

Did you have the opportunity to review it?

@ferdinando-ferreira
Copy link
Contributor Author

As requested on #3315 another PR was submitted with only the normalization of the absolute file path into the relative file path (See #3706)

antfu pushed a commit that referenced this issue Jun 26, 2021
* fix(ssr): normalize manifest filenames

Fixes #3303

* fix(ssr): normalize manifest filenames
@github-actions
Copy link

This issue gets locked because it has been closed for more than 14 days.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this issue Nov 8, 2021
* fix(ssr): normalize manifest filenames

Fixes vitejs#3303

* fix(ssr): normalize manifest filenames
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants