electron / forge

:electron: A complete tool for building and publishing Electron applications
https://electronforge.io
MIT License
6.41k stars 507 forks source link

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

Closed jinye123 closed 8 months ago

jinye123 commented 1 year ago

Pre-flight checklist

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

yes, seeing this too (6.2.1)

caoxiemeihao commented 1 year ago

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

jbtobar commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@GitMurf the current webpack template does work correctly btw.

jbtobar commented 1 year ago

This also sounds related: https://github.com/electron/forge/pull/2175

bermanboris commented 1 year ago

Is there any temporary fix to this issue?

jbtobar commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

this direction is giving me results btw:

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

GitMurf commented 1 year ago

This also appears related: https://github.com/electron/forge/issues/1250

GitMurf commented 1 year ago

ok yep this ignore function is in webpack plugin but not in vite plugin: https://github.com/electron/forge/blob/c7a68918cf6f99f85e07c4c63f0965e72a76b4a3/packages/plugin/webpack/src/WebpackPlugin.ts#L202

cc @caoxiemeihao

ht-sauce commented 1 year ago

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 commented 1 year ago

@caoxiemeihao PTAL

caoxiemeihao commented 1 year ago

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

GitMurf commented 1 year ago

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 commented 9 months ago

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 commented 9 months ago

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

BlackHole1 commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

@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 commented 8 months ago

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 commented 8 months ago

Fixed in #3336