fix(config): sanitize dotenv errors avoiding data leaking (#3529)

* fix(config): sanitize dotenv errors avoiding data leaking

* fix(config): add comment for newline error handling fallback

* Apply suggestions from code review

Co-authored-by: Han Qiao <[email protected]>

* Update config_test.go

---------

Co-authored-by: Han Qiao <[email protected]>

Author: avallete

pkg/config/config.go

@@ -929,7 +929,33 @@ func loadDefaultEnv(env string) error {
 
 func loadEnvIfExists(path string) error {
 	if err := godotenv.Load(path); err != nil && !errors.Is(err, os.ErrNotExist) {
-		return errors.Errorf("failed to load %s: %w", ".env", err)
+		// If DEBUG=1, return the error as is for full debugability
+		if viper.GetBool("DEBUG") {
+			return errors.Errorf("failed to load %s: %w", path, err)
+		}
+		msg := err.Error()
+		switch {
+		case strings.HasPrefix(msg, "unexpected character"):
+			// Try to extract the character, fallback to generic
+			start := strings.Index(msg, "unexpected character \"")
+			if start != -1 {
+				start += len("unexpected character \"")
+				end := strings.Index(msg[start:], "\"")
+				if end != -1 {
+					char := msg[start : start+end]
+					return errors.Errorf("failed to parse environment file: %s (unexpected character '%s' in variable name)", path, char)
+				}
+			}
+			return errors.Errorf("failed to parse environment file: %s (unexpected character in variable name)", path)
+		case strings.HasPrefix(msg, "unterminated quoted value"):
+			return errors.Errorf("failed to parse environment file: %s (unterminated quoted value)", path)
+		// If the error message contains newlines, there is a high chance that the actual content of the
+		// dotenv file is being leaked. In such cases, we return a generic error to avoid unwanted leaks in the logs
+		case strings.Contains(msg, "\n"):
+			return errors.Errorf("failed to parse environment file: %s (syntax error)", path)
+		default:
+			return errors.Errorf("failed to load %s: %w", path, err)
+		}
 	}
 	return nil
 }

pkg/config/config_test.go

@@ -3,12 +3,15 @@ package config
 import (
 	"bytes"
 	_ "embed"
+	"fmt"
+	"os"
 	"path"
 	"strings"
 	"testing"
 	fs "testing/fstest"
 
 	"github.com/BurntSushi/toml"
+	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -525,3 +528,90 @@ func TestLoadFunctionErrorMessageParsing(t *testing.T) {
 		assert.ErrorContains(t, err, `'functions[verify_jwt]' expected a map, got 'bool'`)
 	})
 }
+
+func TestLoadEnvIfExists(t *testing.T) {
+	t.Run("returns nil when file does not exist", func(t *testing.T) {
+		err := loadEnvIfExists("nonexistent.env")
+		assert.NoError(t, err)
+	})
+
+	t.Run("returns raw error when file exists but is malformed and DEBUG=1", func(t *testing.T) {
+		// Set DEBUG=1
+		t.Setenv("DEBUG", "1")
+		viper.AutomaticEnv()
+
+		// Create a temporary file with malformed content
+		tmpFile, err := os.CreateTemp("", "test-*.env")
+		require.NoError(t, err)
+		defer os.Remove(tmpFile.Name())
+
+		// Write malformed content
+		_, err = tmpFile.WriteString("[invalid]\nvalue=secret_value\n")
+		require.NoError(t, err)
+		require.NoError(t, tmpFile.Close())
+
+		// Test loading the malformed file
+		err = loadEnvIfExists(tmpFile.Name())
+		// Should contain the raw error, including the secret value
+		assert.ErrorContains(t, err, "unexpected character")
+		assert.ErrorContains(t, err, "secret_value")
+	})
+
+	t.Run("returns error when file exists but is malformed invalid character", func(t *testing.T) {
+		// Create a temporary file with malformed content
+		tmpFile, err := os.CreateTemp("", "test-*.env")
+		require.NoError(t, err)
+		defer os.Remove(tmpFile.Name())
+
+		// Write malformed content
+		_, err = tmpFile.WriteString("[invalid]\nvalue=secret_value\n")
+		require.NoError(t, err)
+		require.NoError(t, tmpFile.Close())
+
+		// Test loading the malformed file
+		err = loadEnvIfExists(tmpFile.Name())
+		assert.ErrorContains(t, err, fmt.Sprintf("failed to parse environment file: %s (unexpected character '[' in variable name)", tmpFile.Name()))
+		assert.NotContains(t, err.Error(), "secret_value")
+	})
+
+	t.Run("returns error when file exists but is malformed unterminated quotes", func(t *testing.T) {
+		// Create a temporary file with malformed content
+		tmpFile, err := os.CreateTemp("", "test-*.env")
+		require.NoError(t, err)
+		defer os.Remove(tmpFile.Name())
+
+		// Write malformed content
+		_, err = tmpFile.WriteString("value=\"secret_value\n")
+		require.NoError(t, err)
+		require.NoError(t, tmpFile.Close())
+
+		// Test loading the malformed file
+		err = loadEnvIfExists(tmpFile.Name())
+		assert.ErrorContains(t, err, fmt.Sprintf("failed to parse environment file: %s (unterminated quoted value)", tmpFile.Name()))
+		assert.NotContains(t, err.Error(), "secret_value")
+	})
+
+	t.Run("loads valid env file successfully", func(t *testing.T) {
+		// Create a temporary file with valid content
+		tmpFile, err := os.CreateTemp("", "test-*.env")
+		require.NoError(t, err)
+		defer os.Remove(tmpFile.Name())
+
+		// Write valid content
+		_, err = tmpFile.WriteString("TEST_KEY=test_value\nANOTHER_KEY=another_value")
+		require.NoError(t, err)
+		require.NoError(t, tmpFile.Close())
+
+		// Test loading the valid file
+		err = loadEnvIfExists(tmpFile.Name())
+		assert.NoError(t, err)
+
+		// Verify environment variables were loaded
+		assert.Equal(t, "test_value", os.Getenv("TEST_KEY"))
+		assert.Equal(t, "another_value", os.Getenv("ANOTHER_KEY"))
+
+		// Clean up environment variables
+		os.Unsetenv("TEST_KEY")
+		os.Unsetenv("ANOTHER_KEY")
+	})
+}