WIP fix: better env loading [IDE-1314]

pkg/envvars/environment.go

@@ -20,62 +20,120 @@ const (
 	ShellEnvVarName = "SHELL"
 )
 
-// LoadConfiguredEnvironment updates the environment with local configuration. Precedence as follows:
-//  1. std folder-based config files
-//  2. given command-line parameter config file
-//  3. std config file in home directory
-//  4. global shell configuration
+// LoadConfiguredEnvironment updates the environment with local configuration.
+// First the user's SHELL is read, then the configuration files.
+// Any new PATH is prepended to the existing PATH.
+// See LoadShellEnvironment and LoadConfigFiles.
 func LoadConfiguredEnvironment(customConfigFiles []string, workingDirectory string) {
+	LoadShellEnvironment()
+
+	LoadConfigFiles(customConfigFiles, workingDirectory)
+}
+
+// LoadShellEnvironment loads the user's shell environment and prepends
+// any PATH entries to the current PATH. This ensures shell PATH takes precedence.
+func LoadShellEnvironment() {
 	bashOutput := getEnvFromShell("bash")
 
+	// Prepend the Bash PATH now, as it is low priority
+	// We use the Bash shell env as well, since the more info scraping the better to help OSS scans
+	env := gotenv.Parse(strings.NewReader(bashOutput))
+	if val, ok := env[PathEnvVarName]; ok {
+		UpdatePath(val, true)
+	}
+
 	// this is applied at the end always, as it does not overwrite existing variables
+	// We use the Bash shell env as well, since the more info scraping the better to help OSS scans
 	defer func() { _ = gotenv.Apply(strings.NewReader(bashOutput)) }() //nolint:errcheck // we can't do anything with the error
 
-	env := gotenv.Parse(strings.NewReader(bashOutput))
+	env = gotenv.Parse(strings.NewReader(bashOutput))
 	specificShell, ok := env[ShellEnvVarName]
 	if ok {
 		fromSpecificShell := getEnvFromShell(specificShell)
 		_ = gotenv.Apply(strings.NewReader(fromSpecificShell)) //nolint:errcheck // we can't do anything with the error
-	}
 
-	// process config files
-	for _, file := range customConfigFiles {
-		if !filepath.IsAbs(file) {
-			file = filepath.Join(workingDirectory, file)
+		env = gotenv.Parse(strings.NewReader(fromSpecificShell))
+		if val, ok := env[PathEnvVarName]; ok {
+			UpdatePath(val, true)
 		}
-		loadFile(file)
 	}
 }
 
-func loadFile(fileName string) {
-	// preserve path
-	previousPath := os.Getenv(PathEnvVarName)
+// LoadConfigFiles loads environment variables from configuration files.
+// Handles PATH and SDK environment variables specially.
+// The resultant PATH is constructed as follows:
+// 1. Config file PATH entries (highest precedence)
+// 2. SDK bin directories (if SDK variables like JAVA_HOME, GOROOT are set by the config file)
+// 3. Previous PATH entries (lowest precedence)
+func LoadConfigFiles(customConfigFiles []string, workingDirectory string) {
+	// Capture current SDK environment variables and track their latest values
+	sdkVarNames := []string{"JAVA_HOME", "GOROOT"}
+	sdkValues := make(map[string]string)
+	for _, sdkVar := range sdkVarNames {
+		sdkValues[sdkVar] = os.Getenv(sdkVar)
+		// Unset the env var so we can capture if the config file sets it.
+		_ = os.Unsetenv(sdkVar)
+	}
+
+	// Process config files
+	for _, envFilePath := range customConfigFiles {
+		if !filepath.IsAbs(envFilePath) {
+			envFilePath = filepath.Join(workingDirectory, envFilePath)
+		}
 
-	// overwrite existing variables with file config
-	err := gotenv.OverLoad(fileName)
-	if err != nil {
-		return
+		// Preserve original PATH and unset it to ensure correct precedence order.
+		// Without unsetting, if the config file has no PATH, SDK bins will be appended to original PATH (wrong precedence).
+		previousPath := os.Getenv(PathEnvVarName)
+		_ = os.Unsetenv(PathEnvVarName)
+
+		// overwrite existing variables with file config
+		err := gotenv.OverLoad(envFilePath)
+		if err != nil {
+			// Restore PATH if config file loading failed.
+			_ = os.Setenv(PathEnvVarName, previousPath)
+			continue
+		}
+
+		// Check if SDK variables were set by this config file and append their bin directories
+		for _, sdkVar := range sdkVarNames {
+			currentValue := os.Getenv(sdkVar)
+			if currentValue != "" {
+				binPath := filepath.Join(currentValue, "bin")
+				UpdatePath(binPath, false)
+				// Update our tracking and unset for the next file
+				sdkValues[sdkVar] = currentValue
+				_ = os.Unsetenv(sdkVar)
+			}
+		}
+
+		// Add previous PATH to the end of the new
+		UpdatePath(previousPath, false)
 	}
 
-	// add previous path to the end of the new
-	UpdatePath(previousPath, false)
+	// Set final SDK values (latest from config files, or original if not overridden)
+	for _, sdkVar := range sdkVarNames {
+		if sdkValues[sdkVar] != "" {
+			_ = os.Setenv(sdkVar, sdkValues[sdkVar])
+		}
+	}
 }
 
 // guard against command injection
 var shellWhiteList = map[string]bool{
-	"bash":          true,
-	"/bin/zsh":      true,
-	"/bin/sh":       true,
-	"/bin/fish":     true,
-	"/bin/csh":      true,
-	"/bin/ksh":      true,
-	"/bin/bash":     true,
-	"/usr/bin/zsh":  true,
-	"/usr/bin/sh":   true,
-	"/usr/bin/fish": true,
-	"/usr/bin/csh":  true,
-	"/usr/bin/ksh":  true,
-	"/usr/bin/bash": true,
+	"bash":                   true,
+	"/bin/zsh":               true,
+	"/bin/sh":                true,
+	"/bin/fish":              true,
+	"/bin/csh":               true,
+	"/bin/ksh":               true,
+	"/bin/bash":              true,
+	"/usr/bin/zsh":           true,
+	"/usr/bin/sh":            true,
+	"/usr/bin/fish":          true,
+	"/usr/bin/csh":           true,
+	"/usr/bin/ksh":           true,
+	"/usr/bin/bash":          true,
+	"/opt/homebrew/bin/bash": true,
 }
 
 func getEnvFromShell(shell string) string {

pkg/envvars/environment_test.go

@@ -8,6 +8,7 @@ import (
 	"strconv"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -117,16 +118,6 @@ func TestUpdatePathWithDefaults(t *testing.T) {
 	})
 }
 
-func TestLoadFile(t *testing.T) {
-	t.Run("should load given config file", func(t *testing.T) {
-		uniqueEnvVar, fileName := setupTestFile(t, "env-file", t.TempDir())
-
-		loadFile(fileName)
-
-		require.Equal(t, uniqueEnvVar, os.Getenv(uniqueEnvVar))
-	})
-}
-
 func TestLoadConfiguredEnvironment(t *testing.T) {
 	t.Run("should load default config files", func(t *testing.T) {
 		dir := t.TempDir()
@@ -151,6 +142,199 @@ func TestLoadConfiguredEnvironment(t *testing.T) {
 	})
 }
 
+func TestLoadConfigFiles(t *testing.T) {
+	t.Run("should load config files and prepend PATH", func(t *testing.T) {
+		dir := t.TempDir()
+		originalPathValue := "original_path"
+		t.Setenv("PATH", originalPathValue)
+
+		// Create a config file with PATH entry (no overlapping paths)
+		configFileName := filepath.Join(dir, ".snyk.env")
+		configContent := []byte("TEST_VAR=test_value\nPATH=config" + pathListSep + "file\n")
+		err := os.WriteFile(configFileName, configContent, 0660)
+		require.NoError(t, err)
+
+		files := []string{configFileName}
+		LoadConfigFiles(files, dir)
+
+		// Verify environment variable was set
+		require.Equal(t, "test_value", os.Getenv("TEST_VAR"))
+
+		// Verify PATH was prepended (config path should come first)
+		expectedPath := "config" + pathListSep + "file" + pathListSep + originalPathValue
+		require.Equal(t, expectedPath, os.Getenv("PATH"))
+	})
+
+	t.Run("should handle relative config file paths", func(t *testing.T) {
+		dir := t.TempDir()
+		originalPathValue := "original_path"
+		t.Setenv("PATH", originalPathValue)
+
+		// Create a config file with relative path
+		configFileName := ".test.env"
+		configFilePath := filepath.Join(dir, configFileName)
+		configContent := []byte("PATH=relative" + pathListSep + "path\n")
+		err := os.WriteFile(configFilePath, configContent, 0660)
+		require.NoError(t, err)
+
+		files := []string{configFileName} // relative path
+		LoadConfigFiles(files, dir)
+
+		// Verify PATH was prepended
+		expectedPath := "relative" + pathListSep + "path" + pathListSep + originalPathValue
+		require.Equal(t, expectedPath, os.Getenv("PATH"))
+	})
+
+	t.Run("should add config file PATH and SDK bin directories to PATH (in that precedence order)", func(t *testing.T) {
+		dir := t.TempDir()
+
+		// Set a pre-existing PATH value
+		originalPathValue := "original_path"
+		t.Setenv("PATH", originalPathValue)
+
+		// Test with JAVA_HOME unset and GOROOT originally set
+		t.Setenv("JAVA_HOME", "")
+		t.Setenv("GOROOT", "system_go")
+
+		// Create a config file that sets SDK variables and PATH
+		configFileName := filepath.Join(dir, ".snyk.env")
+		configFilePathValue := "config_bin"
+		configFileJavaHomeValue := "project_java"
+		configFileGoRootValue := "project_go"
+		configContent := []byte("JAVA_HOME=" + configFileJavaHomeValue + "\nGOROOT=" + configFileGoRootValue + "\nPATH=" + configFilePathValue + "\n")
+		err := os.WriteFile(configFileName, configContent, 0660)
+		require.NoError(t, err)
+
+		// Act
+		LoadConfigFiles([]string{configFileName}, dir)
+
+		// Verify SDK variables were set
+		assert.Equal(t, "project_java", os.Getenv("JAVA_HOME"))
+		assert.Equal(t, "project_go", os.Getenv("GOROOT"))
+
+		// Verify PATH order: config PATH, then SDK bins, then original PATH
+		// Build expected paths using platform-appropriate separators
+		javaHomeBin := filepath.Join(configFileJavaHomeValue, "bin")
+		goRootBin := filepath.Join(configFileGoRootValue, "bin")
+		expectedPath := configFilePathValue + pathListSep + javaHomeBin + pathListSep + goRootBin + pathListSep + originalPathValue
+		assert.Equal(t, expectedPath, os.Getenv("PATH"))
+	})
+
+	t.Run("should not add SDK bin directories when SDK variables are pre-existing and not overridden", func(t *testing.T) {
+		dir := t.TempDir()
+
+		// Set a pre-existing PATH value
+		originalPathValue := "original_path"
+		t.Setenv("PATH", originalPathValue)
+
+		// Pre-set SDK variables (as if from IDE or system)
+		preExistingJavaHome := "system_java"
+		preExistingGoRoot := "system_go"
+		t.Setenv("JAVA_HOME", preExistingJavaHome)
+		t.Setenv("GOROOT", preExistingGoRoot)
+
+		// Create a config file that doesn't change these SDK variables
+		configFileName := filepath.Join(dir, ".snyk.env")
+		configContent := []byte("OTHER_VAR=other_value\n")
+		err := os.WriteFile(configFileName, configContent, 0660)
+		require.NoError(t, err)
+
+		// Act
+		LoadConfigFiles([]string{configFileName}, dir)
+
+		// Verify SDK variables are unchanged
+		assert.Equal(t, preExistingJavaHome, os.Getenv("JAVA_HOME"))
+		assert.Equal(t, preExistingGoRoot, os.Getenv("GOROOT"))
+
+		// Verify PATH was not modified by SDK bin directories
+		assert.Equal(t, originalPathValue, os.Getenv("PATH"))
+	})
+
+	t.Run("should add bin directories only for SDK variables changed by config files", func(t *testing.T) {
+		dir := t.TempDir()
+
+		// Set a pre-existing PATH value
+		originalPathValue := "original_path"
+		t.Setenv("PATH", originalPathValue)
+
+		// Pre-set some SDK variables
+		preExistingJavaHome := "system_java"
+		t.Setenv("JAVA_HOME", preExistingJavaHome)
+		t.Setenv("GOROOT", "")
+
+		// Create a config file that only changes GOROOT
+		configFileName := filepath.Join(dir, ".snyk.env")
+		configFileGoRootValue := "project_go"
+		configContent := []byte("GOROOT=" + configFileGoRootValue + "\n")
+		err := os.WriteFile(configFileName, configContent, 0660)
+		require.NoError(t, err)
+
+		// Act
+		LoadConfigFiles([]string{configFileName}, dir)
+
+		// Verify JAVA_HOME is unchanged, GOROOT is changed
+		assert.Equal(t, preExistingJavaHome, os.Getenv("JAVA_HOME"))
+		assert.Equal(t, configFileGoRootValue, os.Getenv("GOROOT"))
+
+		// Verify only GOROOT/bin was added to PATH (prepended before the original PATH)
+		// Build expected path using platform-appropriate separators
+		goRootBin := filepath.Join(configFileGoRootValue, "bin")
+		expectedPath := goRootBin + pathListSep + originalPathValue
+		assert.Equal(t, expectedPath, os.Getenv("PATH"))
+	})
+
+	t.Run("should re-prioritize SDK bin directories when config file sets same value", func(t *testing.T) {
+		dir := t.TempDir()
+
+		// Set up PATH with JAVA_HOME/bin already in the middle
+		javaHomeValue := "project_java"
+		javaHomeBinPath := filepath.Join(javaHomeValue, "bin")
+		systemBin := "system_bin"
+		usrBin := "usr_bin"
+		originalPathValue := systemBin + pathListSep + javaHomeBinPath + pathListSep + usrBin
+		t.Setenv("PATH", originalPathValue)
+
+		// Pre-set JAVA_HOME (as if from IDE or system)
+		t.Setenv("JAVA_HOME", javaHomeValue)
+
+		// Create a config file that sets JAVA_HOME to the same value
+		configFileName := filepath.Join(dir, ".snyk.env")
+		configFilePathValue := "config_path"
+		configContent := []byte("JAVA_HOME=" + javaHomeValue + "\nPATH=" + configFilePathValue + "\n")
+		err := os.WriteFile(configFileName, configContent, 0660)
+		require.NoError(t, err)
+
+		// Act
+		LoadConfigFiles([]string{configFileName}, dir)
+
+		// Verify JAVA_HOME is still the same value
+		assert.Equal(t, javaHomeValue, os.Getenv("JAVA_HOME"))
+
+		// Verify PATH re-prioritization: config PATH, then JAVA_HOME/bin, then the original PATH with JAVA_HOME/bin deduplicated out
+		// Build expected path using platform-appropriate separators
+		javaHomeBin := filepath.Join(javaHomeValue, "bin")
+		expectedPath := configFilePathValue + pathListSep + javaHomeBin + pathListSep + systemBin + pathListSep + usrBin
+		assert.Equal(t, expectedPath, os.Getenv("PATH"))
+	})
+}
+
+func TestLoadShellEnvironment(t *testing.T) {
+	t.Run("should load shell environment and prepend PATH", func(t *testing.T) {
+		originalPathValue := "original_path"
+		t.Setenv("PATH", originalPathValue)
+
+		// Note: This test will only work properly on non-Windows systems
+		// and when a shell is available. On Windows or in environments
+		// without shell access, the function will be a no-op.
+		LoadShellEnvironment()
+
+		// The shell environment loading behavior depends on the actual shell
+		// and environment. We can't predict exact PATH changes, but we can
+		// verify the function doesn't crash and PATH is still set.
+		require.NotEmpty(t, os.Getenv("PATH"))
+	})
+}
+
 func setupTestFile(t *testing.T, fileName string, dir string) (string, string) {
 	t.Helper()
 	uniqueEnvVar := strconv.Itoa(rand.Int())