Extract constants to improve maintainability (#777)

Why
Hard-coded values scattered across files made updates difficult.

Summary
Centralized BUILD_ENV_VARS, DANGEROUS_ENV_VARS, DEFAULT_EXPORT_DIR,
and DEFAULT_CONFIG_FILE constants in types.ts.

Key improvements
- Single source of truth for environment variable whitelists/blacklists
- Replaced 10+ magic strings with named constants
- Added security documentation for DANGEROUS_ENV_VARS

Impact
- Existing: No functional changes, purely refactoring
- New: Easier to modify default paths and security policies

Risks
None.

Author: justin808

package/configExporter/cli.ts

@@ -6,7 +6,16 @@ import { resolve, dirname, sep, delimiter, basename } from "path"
 import { inspect } from "util"
 import { load as loadYaml } from "js-yaml"
 import yargs from "yargs"
-import { ExportOptions, ConfigMetadata, FileOutput } from "./types"
+import {
+  ExportOptions,
+  ConfigMetadata,
+  FileOutput,
+  BUILD_ENV_VARS,
+  isBuildEnvVar,
+  isDangerousEnvVar,
+  DEFAULT_EXPORT_DIR,
+  DEFAULT_CONFIG_FILE
+} from "./types"
 import { YamlSerializer } from "./yamlSerializer"
 import { FileWriter } from "./fileWriter"
 import { ConfigFileLoader, generateSampleConfigFile } from "./configFile"
@@ -27,19 +36,6 @@ try {
   )
 }
 
-/**
- * Environment variable names that can be set by build configurations
- */
-const BUILD_ENV_VARS = [
-  "NODE_ENV",
-  "RAILS_ENV",
-  "NODE_OPTIONS",
-  "BABEL_ENV",
-  "WEBPACK_SERVE",
-  "CLIENT_BUNDLE_ONLY",
-  "SERVER_BUNDLE_ONLY"
-] as const
-
 /**
  * Saves current values of build environment variables for later restoration
  * @returns Object mapping variable names to their current values (or undefined)
@@ -132,8 +128,7 @@ export async function run(args: string[]): Promise<number> {
     if (resolvedOptions.build) {
       const loader = new ConfigFileLoader(resolvedOptions.configFile)
       if (!loader.exists()) {
-        const configPath =
-          resolvedOptions.configFile || "config/shakapacker-builds.yml"
+        const configPath = resolvedOptions.configFile || DEFAULT_CONFIG_FILE
         throw new Error(
           `--build requires a config file but ${configPath} not found. Run --init to create it.`
         )
@@ -182,8 +177,7 @@ QUICK START (for troubleshooting):
     .option("init", {
       type: "boolean",
       default: false,
-      description:
-        "Generate config/shakapacker-builds.yml (use with --ssr for SSR builds)"
+      description: `Generate ${DEFAULT_CONFIG_FILE} (use with --ssr for SSR builds)`
     })
     .option("ssr", {
       type: "boolean",
@@ -206,8 +200,7 @@ QUICK START (for troubleshooting):
     })
     .option("config-file", {
       type: "string",
-      description:
-        "Path to config file (default: config/shakapacker-builds.yml)"
+      description: `Path to config file (default: ${DEFAULT_CONFIG_FILE})`
     })
     // Validation Options
     .option("validate", {
@@ -459,17 +452,14 @@ function applyDefaults(options: ExportOptions): ExportOptions {
     !updatedOptions.output &&
     !updatedOptions.saveDir
   ) {
-    updatedOptions.saveDir = resolve(
-      process.cwd(),
-      "shakapacker-config-exports"
-    )
+    updatedOptions.saveDir = resolve(process.cwd(), DEFAULT_EXPORT_DIR)
   }
 
   return updatedOptions
 }
 
 function runInitCommand(options: ExportOptions): number {
-  const configPath = options.configFile || "config/shakapacker-builds.yml"
+  const configPath = options.configFile || DEFAULT_CONFIG_FILE
   const fullPath = resolve(process.cwd(), configPath)
 
   // Check if SSR variant is requested via --ssr flag
@@ -576,7 +566,7 @@ async function runValidateCommand(options: ExportOptions): Promise<number> {
     // Validate that config file exists
     const loader = new ConfigFileLoader(options.configFile)
     if (!loader.exists()) {
-      const configPath = options.configFile || "config/shakapacker-builds.yml"
+      const configPath = options.configFile || DEFAULT_CONFIG_FILE
       throw new Error(
         `Config file ${configPath} not found. Run --init to create it.`
       )
@@ -609,7 +599,7 @@ async function runValidateCommand(options: ExportOptions): Promise<number> {
       // Handle empty builds edge case
       if (buildsToValidate.length === 0) {
         throw new Error(
-          `No builds found in config file. Add at least one build to config/shakapacker-builds.yml or run --init to see examples.`
+          `No builds found in config file. Add at least one build to ${DEFAULT_CONFIG_FILE} or run --init to see examples.`
         )
       }
     }
@@ -722,8 +712,7 @@ async function runAllBuildsCommand(options: ExportOptions): Promise<number> {
 
     const loader = new ConfigFileLoader(resolvedOptions.configFile)
     if (!loader.exists()) {
-      const configPath =
-        resolvedOptions.configFile || "config/shakapacker-builds.yml"
+      const configPath = resolvedOptions.configFile || DEFAULT_CONFIG_FILE
       throw new Error(
         `Config file ${configPath} not found. Run --init to create it.`
       )
@@ -810,7 +799,7 @@ async function runDoctorMode(
     const createdFiles: string[] = []
 
     // Check if config file exists - always use it for doctor mode
-    const configFilePath = options.configFile || "config/shakapacker-builds.yml"
+    const configFilePath = options.configFile || DEFAULT_CONFIG_FILE
     const loader = new ConfigFileLoader(configFilePath)
 
     if (loader.exists()) {
@@ -1102,29 +1091,20 @@ async function loadConfigsForEnv(
 
     // Set environment variables from config
     // Security: Only allow specific environment variables to prevent malicious configs
-    const DANGEROUS_ENV_VARS = [
-      "PATH",
-      "HOME",
-      "LD_PRELOAD",
-      "LD_LIBRARY_PATH",
-      "DYLD_LIBRARY_PATH",
-      "DYLD_INSERT_LIBRARIES"
-    ]
-
     if (options.verbose) {
       console.log(
         `[Config Exporter] Setting environment variables from build config...`
       )
     }
 
     for (const [key, value] of Object.entries(resolvedBuild.environment)) {
-      if (DANGEROUS_ENV_VARS.includes(key)) {
+      if (isDangerousEnvVar(key)) {
         console.warn(
           `[Config Exporter] Warning: Skipping dangerous environment variable: ${key}`
         )
         continue
       }
-      if (!(BUILD_ENV_VARS as readonly string[]).includes(key)) {
+      if (!isBuildEnvVar(key)) {
         console.warn(
           `[Config Exporter] Warning: Skipping non-whitelisted environment variable: ${key}. ` +
             `Allowed variables are: ${BUILD_ENV_VARS.join(", ")}`

package/configExporter/configFile.ts

@@ -5,7 +5,8 @@ import {
   BundlerConfigFile,
   BuildConfig,
   ResolvedBuildConfig,
-  ExportOptions
+  ExportOptions,
+  DEFAULT_CONFIG_FILE
 } from "./types"
 
 /**
@@ -18,12 +19,12 @@ export class ConfigFileLoader {
   private configFilePath: string
 
   /**
-   * @param configFilePath - Path to config file (defaults to config/shakapacker-builds.yml in cwd)
+   * @param configFilePath - Path to config file (defaults to DEFAULT_CONFIG_FILE in cwd)
    * @throws Error if path is outside project directory
    */
   constructor(configFilePath?: string) {
     this.configFilePath =
-      configFilePath || resolve(process.cwd(), "config/shakapacker-builds.yml")
+      configFilePath || resolve(process.cwd(), DEFAULT_CONFIG_FILE)
     this.validateConfigPath()
   }
 

package/configExporter/types.ts

@@ -1,3 +1,67 @@
+/**
+ * Environment variable names that can be set by build configurations.
+ * These are the only environment variables that build configs are allowed to set.
+ * This whitelist prevents malicious configs from modifying critical system variables.
+ */
+export const BUILD_ENV_VARS = [
+  "NODE_ENV",
+  "RAILS_ENV",
+  "NODE_OPTIONS",
+  "BABEL_ENV",
+  "WEBPACK_SERVE",
+  "CLIENT_BUNDLE_ONLY",
+  "SERVER_BUNDLE_ONLY"
+] as const
+
+/**
+ * Environment variables that must never be set by build configurations.
+ * Setting these could compromise system security or cause unexpected behavior.
+ */
+export const DANGEROUS_ENV_VARS = [
+  "PATH",
+  "HOME",
+  "LD_PRELOAD",
+  "LD_LIBRARY_PATH",
+  "DYLD_LIBRARY_PATH",
+  "DYLD_INSERT_LIBRARIES"
+] as const
+
+/**
+ * Type predicate to check if a string is in the BUILD_ENV_VARS whitelist
+ *
+ * Note: The type assertion is necessary because TypeScript's type system cannot
+ * infer that .includes() on a readonly const array will properly narrow the type.
+ * The assertion is safe because we're only widening the type for the includes() check.
+ */
+export function isBuildEnvVar(
+  key: string
+): key is (typeof BUILD_ENV_VARS)[number] {
+  return (BUILD_ENV_VARS as readonly string[]).includes(key)
+}
+
+/**
+ * Type predicate to check if a string is in the DANGEROUS_ENV_VARS blacklist
+ *
+ * Note: The type assertion is necessary because TypeScript's type system cannot
+ * infer that .includes() on a readonly const array will properly narrow the type.
+ * The assertion is safe because we're only widening the type for the includes() check.
+ */
+export function isDangerousEnvVar(
+  key: string
+): key is (typeof DANGEROUS_ENV_VARS)[number] {
+  return (DANGEROUS_ENV_VARS as readonly string[]).includes(key)
+}
+
+/**
+ * Default directory for config exports when using --doctor or file output modes.
+ */
+export const DEFAULT_EXPORT_DIR = "shakapacker-config-exports"
+
+/**
+ * Default config file path for bundler build configurations.
+ */
+export const DEFAULT_CONFIG_FILE = "config/shakapacker-builds.yml"
+
 export interface ExportOptions {
   doctor?: boolean
   saveDir?: string