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

The packaged file volume is too large and contains nodes_ modules electron-forge package (--template=vite) #3224

Closed
3 tasks done
jinye123 opened this issue Apr 28, 2023 · 29 comments

Comments

@jinye123
Copy link

Pre-flight checklist

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project uses.
  • I have searched the issue tracker for a bug that matches the one I want to file, without success.

Electron Forge version

6.1.1

Electron version

24.1.2

Operating system

macOs 12.2.1

Last known working Electron Forge version

6.1.1

Expected behavior

yarn create electron-app my-new-app --template=vite and run electron-forga package The packaged file volume is too large and contains nodes_ modules in out =>app =>resource

Actual behavior

yarn create electron-app my-new-app --template=vite and run electron-forga package The packaged file volume is too large and contains nodes_ modules in out =>app =>resource

Steps to reproduce

yarn create electron-app my-new-app --template=vite
npm run package

Additional information

No response

@owo
Copy link

owo commented May 18, 2023

In addition to node_modules, the actual source code is being included in the app directory. Why aren't the compiled and minified files in .vite being used?

@MDransfeld
Copy link

This still happens with version 6.2.1, the whole source folder including the node_modules folder is copied to resources/app. Instead only the .vite folder should be copied.

@jbtobar
Copy link

jbtobar commented Jul 3, 2023

yes, seeing this too (6.2.1)

@caoxiemeihao
Copy link
Member

caoxiemeihao commented Jul 4, 2023

Can you provide a specific repository for replication? Maybe you put most of the packages in dependencies.

@jbtobar
Copy link

jbtobar commented Jul 4, 2023

it happens with the template repository, no modifications:

yarn create electron-app my-new-app --template=vite
cd my-new-app
yarn package

I am on mac, the my-new-app/out/my-new-app-darwin-x64/my-new-app.app/Contents/Resources/app looks like this:

.
├── .gitignore
├── .vite
│   ├── build
│   │   ├── main.js
│   │   └── preload.js
│   └── renderer
│       └── main_window
│           ├── assets
│           │   ├── index-620ac58a.css
│           │   └── index-f38d6f86.js
│           └── index.html
├── forge.config.js
├── index.html
├── node_modules
│   ├── .yarn-integrity
│   ├── @electron
│   ├── @electron-forge
│   ├── @esbuild
│   ├── @isaacs
│   ├── @malept
│   ├── @nodelib
│   ├── @npmcli
│   ├── @pkgjs
│   ├── @sindresorhus
│   ├── @szmarczak
│   ├── @tootallnate
│   ├── @types
│   └── electron-squirrel-startup
│       ├── .eslintrc
│       ├── .jsfmtrc
│       ├── .npmignore
│       ├── .travis.yml
│       ├── LICENSE
│       ├── README.md
│       ├── appveyor.yml
│       ├── index.js
│       ├── node_modules
│       │   ├── debug
│       │   │   ├── .coveralls.yml
│       │   │   ├── .eslintrc
│       │   │   ├── .npmignore
│       │   │   ├── .travis.yml
│       │   │   ├── CHANGELOG.md
│       │   │   ├── LICENSE
│       │   │   ├── Makefile
│       │   │   ├── README.md
│       │   │   ├── component.json
│       │   │   ├── karma.conf.js
│       │   │   ├── node.js
│       │   │   ├── package.json
│       │   │   └── src
│       │   │       ├── browser.js
│       │   │       ├── debug.js
│       │   │       ├── index.js
│       │   │       ├── inspector-log.js
│       │   │       └── node.js
│       │   └── ms
│       │       ├── index.js
│       │       ├── license.md
│       │       ├── package.json
│       │       └── readme.md
│       ├── package.json
│       └── test
│           ├── index.test.js
│           └── mocha.opts
├── package.json
├── src
│   ├── index.css
│   ├── main.js
│   ├── preload.js
│   └── renderer.js
├── vite.main.config.mjs
├── vite.preload.config.mjs
└── vite.renderer.config.mjs

25 directories, 49 files

and for example, on a previous project where I used webpack, the my-new-app/out/my-new-app-darwin-x64/my-new-app.app/Contents/Resources/app.asar looks like this (the node_modules folder is empty):

image

@GitMurf
Copy link

GitMurf commented Jul 14, 2023

Any word on this? I noticed the same thing. All of our source code is bundled up with the package. It more or less appears as if the build advantages of vite using rollup etc. is nullified as the packaged app is not tree shook, not bundled to the few main.js/preload.js etc. and not minified / obfuscated (code of our app can easily be read by users).

It almost seems as if the regular electron-forge package/make procedure is being used and ignoring vite/rollup. Anyone have any thoughts?

@jbtobar
Copy link

jbtobar commented Jul 14, 2023

its also quite a security vulnerability because the entire source code is copied into the release, so users who use this template and then distribute the app would be distributing their entire repo without realizing.

not sure who is in charge on this (@BlackHole1 @caoxiemeihao ) but maybe maintainers might want to remove this template until fixed or warn users.

@GitMurf
Copy link

GitMurf commented Jul 14, 2023

@jbtobar I know the vite template was based on the webpack template... do you know if the webpack template (with and/or without TypeScript) is packaging correctly? I am not a webpack guy so have never used it.

@jbtobar
Copy link

jbtobar commented Jul 14, 2023

I am using the webpack typescript template with electron-forge v ^6.0.4 and it has been packaging correctly. I haven't tested the latest version but my guess is it should package correctly as well.

I am not familiar with the forge repo but from quick glancing I think forge/packages/plugin/vite might be where the issue is.

@jbtobar
Copy link

jbtobar commented Jul 15, 2023

@GitMurf the current webpack template does work correctly btw.

@jbtobar
Copy link

jbtobar commented Jul 15, 2023

This also sounds related: #2175

@bermanboris
Copy link

Is there any temporary fix to this issue?

@jbtobar
Copy link

jbtobar commented Jul 15, 2023

I am trying to figure one out. I have been trying to play with these hooks (postStart
packageAfterCopy
packageAfterPrune
packageAfterExtract
prePackage
postPackage
ignore
)

in the forge.config but no dice. Now playing with the source code to figure out the issue. @erickzhao or anyone familiar with electron-forge code base maybe knows something.

@jbtobar
Copy link

jbtobar commented Jul 15, 2023

this direction is giving me results btw:

packagerConfig: {
      ignore: (path) => {
          if (path.startsWith('/.vite')) return false;
          if (path === '/package.json') return false;
          if (!path) return false;
          return true;
      },
  },

@jbtobar
Copy link

jbtobar commented Jul 15, 2023

@bermanboris fyi the above dirty hack doesn't package electron-squirrel-startup so there's that. Still trying to figure out how to make this work. Hopefully @caoxiemeihao can offer some clarity.

@GitMurf
Copy link

GitMurf commented Jul 16, 2023

this direction is giving me results btw:

So is the idea here that this is ignoring everything except .Vite and package.Json?

@GitMurf
Copy link

GitMurf commented Jul 18, 2023

This also appears related: #1250

@GitMurf
Copy link

GitMurf commented Jul 18, 2023

ok yep this ignore function is in webpack plugin but not in vite plugin:

forgeConfig.packagerConfig.ignore = (file: string) => {

cc @caoxiemeihao

@ht-sauce
Copy link

ht-sauce commented Aug 8, 2023

this direction is giving me results btw:

packagerConfig: {
      ignore: (path) => {
          if (path.startsWith('/.vite')) return false;
          if (path === '/package.json') return false;
          if (!path) return false;
          return true;
      },
  },

This function cannot identify the code in the nodejs section of the main process, and will remove the dependent packages in the main process that should not be ignored. This function has a bug
My main process relies on a specific package file, which has resulted in the package being removed, but the path address was not returned correctly. That is to say, it is ignored by default.

@BlackHole1
Copy link
Member

@caoxiemeihao PTAL

@caoxiemeihao
Copy link
Member

@GitMurf @ht-sauce Okay!
Thx for helps, I'll back in the week end!

@GitMurf
Copy link

GitMurf commented Aug 9, 2023

This function cannot identify the code in the nodejs section of the main process, and will remove the dependent packages in the main process that should not be ignored. This function has a bug My main process relies on a specific package file, which has resulted in the package being removed, but the path address was not returned correctly. That is to say, it is ignored by default.

@ht-sauce you are correct. This assume you are not using anything like a native module that has to be "externalized". Same if you use squirrel, need to make sure that node module is included (NOT ignored) as well. Below is what I got working where I use Realm DB which is a native node module and has to be packaged as an external dependency.

cc @caoxiemeihao I recommend doing testing using something like Realm (https://github.com/realm/realm-js) as a way to validate external dependencies work as well :)

Update this array with the dependencies you need to externalize: const NATIVE_MODULES_TO_PACKAGE = ['realm', 'electron-squirrel-startup'];

Note: this uses the flora-colossus library (created by @MarshallOfSound) to get the dependency tree of the native modules (since you need to also include the nested dependencies of the dependencies).

import type { ForgeConfig } from '@electron-forge/shared-types';
import { dirname } from 'node:path';
import { Walker, DepType, type Module } from 'flora-colossus';

// any packages that you must mark as "external" in vite
const NATIVE_MODULES_TO_PACKAGE = ['realm', 'electron-squirrel-startup'];
const INCLUDE_NESTED_DEPS = true as const;
let nativeModuleDependenciesToPackage: Set<string>;

const config: ForgeConfig = {
  // other config stuff...
  hooks: {
    prePackage: async (forgeConfig) => {
      if (forgeConfig.packagerConfig.ignore !== undefined) {
        throw new Error(
          'forgeConfig.packagerConfig.ignore is already defined. Please remove it from your forge config and instead use the prePackage hook to dynamically set it.'
        );
      }

      const getExternalNestedDependencies = async (
        nodeModuleNames: string[],
        includeNestedDeps = true
      ) => {
        const foundModules = new Set(nodeModuleNames);
        if (includeNestedDeps) {
          for (const external of nodeModuleNames) {
            type MyPublicClass<T> = {
              [P in keyof T]: T[P];
            };
            type MyPublicWalker = MyPublicClass<Walker> & {
              modules: Module[];
              walkDependenciesForModule: (
                moduleRoot: string,
                depType: DepType
              ) => Promise<void>;
            };
            const moduleRoot = dirname(
              require.resolve(`${external}/package.json`, { paths: [__dirname] })
            );
            const walker = new Walker(moduleRoot) as unknown as MyPublicWalker;
            walker.modules = [];
            await walker.walkDependenciesForModule(moduleRoot, DepType.PROD);
            walker.modules
              .filter((dep) => (dep.nativeModuleType as number) === DepType.PROD)
              .map((dep) => dep.name)
              .forEach((name) => foundModules.add(name));
          }
        }
        return foundModules;
      };

      nativeModuleDependenciesToPackage = await getExternalNestedDependencies(
        NATIVE_MODULES_TO_PACKAGE,
        INCLUDE_NESTED_DEPS
      );

      forgeConfig.packagerConfig.ignore = (path) => {
        // .vite bundled build files
        if (path.startsWith('/.vite')) {
          return false;
        }
        // main package.json file
        if (path === '/package.json') {
          return false;
        }
        if (!path) {
          return false;
        }
        // need to first NOT ignore the root node_modules folder
        if (path === '/node_modules') {
          return false;
        }
        // if path is in nativeModuleDependenciesToPackage, return false (to package it)
        const foundModules: Set<string> = nativeModuleDependenciesToPackage;
        for (const module of foundModules) {
          if (
            path.startsWith(`/node_modules/${module}`) ||
            path.startsWith(`/node_modules/${module.split('/')[0]}`)
          ) {
            return false;
          }
        }

        // for everything else, ignore it
        return true;
      };
    },
  },
  // other config stuff...
};

Hope this helps!

@akash07k
Copy link

akash07k commented Dec 9, 2023

Hi all, I too am plagued with this issue even in latest 7.2.x release. this is a huge security risk and should be fixed as soon as possible.
Is there anyone who is working on this issue or any other update?
I will really be very thankful to the team if this gets fixed.
@BlackHole1
@caoxiemeihao

@akash07k
Copy link

akash07k commented Dec 9, 2023

Also related #3377 also highlights how big this is a risk for security

@BlackHole1
Copy link
Member

Hi @akash07k. I recently communicated with @caoxiemeihao. He is currently looking for a new job, so he temporarily doesn’t have extra time to address the related PR. However, he also mentioned that once his job is stable, he will refocus on these issues.

@akash07k
Copy link

Oh, I see.
I wish him all the very best for the new job and I hope he gets it soon. :-)

Hi @akash07k. I recently communicated with @caoxiemeihao. He is currently looking for a new job, so he temporarily doesn’t have extra time to address the related PR. However, he also mentioned that once his job is stable, he will refocus on these issues.

@caoxiemeihao
Copy link
Member

@akash07k Hey! 👋

I'm about to fix this issue and have been stuck on CI for a few days.


BTW, I know the specific reason for the CI error, I have temporarily resolved it, and I will try to completely fix the CI error in the next upgrade of Vite5 PR (it is coming soon).

@akash07k
Copy link

akash07k commented Jan 8, 2024

Thanks so much @caoxiemeihao, I'll be waiting for the update. :-)

@akash07k Hey! 👋

I'm about to fix this issue and have been stuck on CI for a few days.

BTW, I know the specific reason for the CI error, I have temporarily resolved it, and I will try to completely fix the CI error in the next upgrade of Vite5 PR (it is coming soon).

@caoxiemeihao
Copy link
Member

Fixed in #3336

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

No branches or pull requests