Merge pull request #2653 from modernweb-dev/fix/storybook-builder-pro…

…cess-env

fix(storybook-builder): define only necessary process.env globally

.changeset/sweet-sloths-crash.md

@@ -0,0 +1,5 @@
+---
+'@web/storybook-builder': patch
+---
+
+define only necessary process.env globally to improve security and decrease prebundling

packages/storybook-builder/src/generate-iframe-html.ts

@@ -11,6 +11,7 @@ export async function generateIframeHtml(options: Options): Promise<string> {
     'utf-8',
   );
   const { configType, features, presets, serverChannelUrl } = options;
+  const env = await presets.apply<Record<string, string>>('env');
   const frameworkOptions = await presets.apply<Record<string, any> | null>('frameworkOptions');
   const headHtmlSnippet = await presets.apply<PreviewHtml>('previewHead');
   const bodyHtmlSnippet = await presets.apply<PreviewHtml>('previewBody');
@@ -26,6 +27,14 @@ export async function generateIframeHtml(options: Options): Promise<string> {
   }));
   return (
     iframeHtmlTemplate
+      .replace(
+        `'[PROCESS_ENV HERE]'`,
+        JSON.stringify({
+          // IMPORTANT!!!
+          // please do not include the entire "env" to prevent bundling and deploying of sensitive data
+          NODE_ENV: env.NODE_ENV,
+        }),
+      )
       .replace('[CONFIG_TYPE HERE]', configType || '')
       .replace('[LOGLEVEL HERE]', logLevel || '')
       .replace(`'[FRAMEWORK_OPTIONS HERE]'`, JSON.stringify(frameworkOptions))

packages/storybook-builder/src/index.ts

@@ -60,8 +60,6 @@ export const start: WdsBuilder['start'] = async ({ startTime, options, router, s
   router.use('/sb-preview', express.static(previewDirOrigin, { immutable: true, maxAge: '5m' }));
   router.use(`/${PREBUNDLED_MODULES_DIR}`, express.static(resolve(`./${PREBUNDLED_MODULES_DIR}`)));
 
-  const env = await options.presets.apply<Record<string, string>>('env');
-
   const wdsStorybookConfig: DevServerConfig = {
     nodeResolve: true,
     plugins: [
@@ -74,7 +72,7 @@ export const start: WdsBuilder['start'] = async ({ startTime, options, router, s
           }
         },
       },
-      wdsPluginPrebundleModules(env),
+      wdsPluginPrebundleModules(),
       wdsPluginStorybookBuilder(options),
       wdsPluginExternalGlobals(globalsNameReferenceMap || globals),
     ],
@@ -130,8 +128,6 @@ export const start: WdsBuilder['start'] = async ({ startTime, options, router, s
 };
 
 export const build: WdsBuilder['build'] = async ({ startTime, options }) => {
-  const env = await options.presets.apply<Record<string, string>>('env');
-
   const rollupDefaultOutputOptions: OutputOptions = {
     dir: options.outputDir,
   };
@@ -146,7 +142,7 @@ export const build: WdsBuilder['build'] = async ({ startTime, options }) => {
         extractAssets: false,
       }),
       rollupPluginNodeResolve(),
-      rollupPluginPrebundleModules(env),
+      rollupPluginPrebundleModules(),
       rollupPluginStorybookBuilder(options),
       rollupPluginExternalGlobals(globalsNameReferenceMap || globals),
     ],

packages/storybook-builder/src/rollup-plugin-prebundle-modules.ts

@@ -1,12 +1,11 @@
-import { stringifyProcessEnvs } from '@storybook/core-common';
 import { build } from 'esbuild';
 import { join } from 'path';
 import type { Plugin } from 'rollup';
 import { getNodeModuleDir } from './get-node-module-dir.js';
 
 export const PREBUNDLED_MODULES_DIR = 'node_modules/.prebundled_modules';
 
-export function rollupPluginPrebundleModules(env: Record<string, string>): Plugin {
+export function rollupPluginPrebundleModules(): Plugin {
   const modulePaths: Record<string, string> = {};
 
   return {
@@ -37,9 +36,6 @@ export function rollupPluginPrebundleModules(env: Record<string, string>): Plugi
           lodash: getNodeModuleDir('lodash-es'), // more optimal, but also solves esbuild incorrectly compiling lodash/_nodeUtil
           path: require.resolve('path-browserify'),
         },
-        define: {
-          ...stringifyProcessEnvs(env),
-        },
         plugins: [esbuildCommonjsPlugin()],
       });
     },
@@ -64,7 +60,7 @@ function getModules() {
 
 // this is different to https://github.com/storybookjs/storybook/blob/v7.0.0/code/lib/builder-vite/src/optimizeDeps.ts#L7
 // builder-vite bundles different dependencies for performance reasons
-// we aim only at browserifying NodeJS dependencies (CommonJS/process.env/...)
+// we aim only at browserifying dependencies which are CommonJS, or sometimes ESM with issues
 export const CANDIDATES = [
   // @testing-library has ESM, but imports/exports are not working correctly between packages
   // specifically "@testing-library/user-event" has "dist/esm/utils/misc/getWindow.js" (see https://cdn.jsdelivr.net/npm/@testing-library/[email protected]/dist/esm/utils/misc/getWindow.js)
@@ -82,7 +78,4 @@ export const CANDIDATES = [
 
   // CommonJS module used in Storybook MJS files
   'lodash/mapValues.js',
-
-  // ESM, but uses `process.env.NODE_ENV`
-  'tiny-invariant',
 ];

packages/storybook-builder/static/iframe-template.html

@@ -49,7 +49,9 @@
       // We do this so that "module && module.hot" etc. in Storybook source code
       // doesn't fail (it will simply be disabled)
       window.module = undefined;
+
       window.global = window;
+      window.process = { env: '[PROCESS_ENV HERE]' };
     </script>
     <!-- [HEAD HTML SNIPPET HERE] -->
   </head>