fix: trim values loaded from _FILE env vars in AppConfig (#1741)

backend/internal/appconfig/service.go

@@ -217,31 +217,28 @@ func (s *AppConfigService) loadDbConfigFromEnv() (*AppConfigModel, error) {
 			return nil, fmt.Errorf("app configuration field %s is missing its environment variable name", field.Name)
 		}
 
-		// Set the value if it's set
+		// Sensitive values can also be loaded from a file, using the variable with the "_FILE" suffix (files have precedence over the env var)
+		if field.Tag.Get("sensitive") == "true" {
+			value, ok, err := common.LoadStringEnvVarFromFile(envVarName)
+			if err != nil {
+				return nil, err
+			}
+			if ok {
+				rv.Field(i).SetString(value)
+				continue
+			}
+		}
+
+		// Set the value from the environment variable if it's set
 		value, ok := os.LookupEnv(envVarName)
 		if ok {
 			rv.Field(i).SetString(value)
-			continue
-		}
-
-		// If it's sensitive, we also allow reading from file
-		if field.Tag.Get("sensitive") == "true" {
-			fileName := os.Getenv(envVarName + "_FILE")
-			if fileName != "" {
-				// #nosec G703 - Value is provided by admin
-				b, err := os.ReadFile(fileName)
-				if err != nil {
-					return nil, fmt.Errorf("failed to read secret '%s' from file '%s': %w", envVarName, fileName, err)
-				}
-
-				rv.Field(i).SetString(string(b))
-				continue
-			}
 		}
 	}
 
 	// Validate the resolved configuration before exposing values to the rest of the application
-	if err := validateEnvConfig(dest); err != nil {
+	err := validateEnvConfig(dest)
+	if err != nil {
 		return nil, err
 	}
 
@@ -252,12 +249,13 @@ func (s *AppConfigService) loadDbConfigFromEnv() (*AppConfigModel, error) {
 func validateEnvConfig(config *AppConfigModel) error {
 	// Map the resolved model to the canonical update DTO so both configuration paths share validation rules
 	var input dto.AppConfigUpdateDto
-	if err := dto.MapStruct(config, &input); err != nil {
+	err := dto.MapStruct(config, &input)
+	if err != nil {
 		return fmt.Errorf("failed to prepare environment app configuration for validation: %w", err)
 	}
 
 	// Collect every invalid environment variable
-	err := input.Validate()
+	err = input.Validate()
 	if err != nil {
 		validationErrors, ok := errors.AsType[validator.ValidationErrors](err)
 		if !ok {

backend/internal/appconfig/service_test.go

@@ -2,6 +2,8 @@ package appconfig
 
 import (
 	"encoding/json"
+	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -458,3 +460,69 @@ func TestService_CIMDURLAllowlist(t *testing.T) {
 		assert.Empty(t, svc.GetCIMDURLAllowlist())
 	})
 }
+
+func TestService_LoadDbConfigFromEnv(t *testing.T) {
+	// writeSecretFile writes content to a file in a temporary directory that is removed when the test ends, returning its path
+	writeSecretFile := func(t *testing.T, content string) string {
+		t.Helper()
+
+		fileName := filepath.Join(t.TempDir(), "secret.txt")
+		err := os.WriteFile(fileName, []byte(content), 0600)
+		require.NoError(t, err)
+
+		return fileName
+	}
+
+	t.Run("loads a sensitive value from a file", func(t *testing.T) {
+		t.Setenv("LDAP_BIND_PASSWORD_FILE", writeSecretFile(t, "my-ldap-password"))
+
+		svc := &AppConfigService{}
+		cfg, err := svc.loadDbConfigFromEnv()
+		require.NoError(t, err)
+
+		assert.Equal(t, AppConfigValue("my-ldap-password"), cfg.LdapBindPassword)
+	})
+
+	t.Run("trims whitespace from values loaded from a file", func(t *testing.T) {
+		// Files containing secrets normally end with a trailing newline, which must not be part of the value
+		t.Setenv("LDAP_BIND_PASSWORD_FILE", writeSecretFile(t, "my-ldap-password\n"))
+		t.Setenv("SMTP_PASSWORD_FILE", writeSecretFile(t, "  my-smtp-password  \r\n"))
+
+		svc := &AppConfigService{}
+		cfg, err := svc.loadDbConfigFromEnv()
+		require.NoError(t, err)
+
+		assert.Equal(t, AppConfigValue("my-ldap-password"), cfg.LdapBindPassword)
+		assert.Equal(t, AppConfigValue("my-smtp-password"), cfg.SmtpPassword)
+	})
+
+	t.Run("the file takes precedence over the environment variable", func(t *testing.T) {
+		t.Setenv("LDAP_BIND_PASSWORD", "from-env")
+		t.Setenv("LDAP_BIND_PASSWORD_FILE", writeSecretFile(t, "from-file\n"))
+
+		svc := &AppConfigService{}
+		cfg, err := svc.loadDbConfigFromEnv()
+		require.NoError(t, err)
+
+		assert.Equal(t, AppConfigValue("from-file"), cfg.LdapBindPassword)
+	})
+
+	t.Run("values that are not sensitive are not loaded from a file", func(t *testing.T) {
+		t.Setenv("LDAP_BIND_DN_FILE", writeSecretFile(t, "cn=from-file"))
+
+		svc := &AppConfigService{}
+		cfg, err := svc.loadDbConfigFromEnv()
+		require.NoError(t, err)
+
+		assert.Empty(t, cfg.LdapBindDn)
+	})
+
+	t.Run("returns an error when the file cannot be read", func(t *testing.T) {
+		t.Setenv("LDAP_BIND_PASSWORD_FILE", filepath.Join(t.TempDir(), "does-not-exist.txt"))
+
+		svc := &AppConfigService{}
+		_, err := svc.loadDbConfigFromEnv()
+		require.Error(t, err)
+		assert.ErrorContains(t, err, "LDAP_BIND_PASSWORD_FILE")
+	})
+}

backend/internal/common/env_config.go

@@ -390,36 +390,30 @@ func resolveFileBasedEnvVariable(field reflect.Value, fieldType reflect.StructFi
 		return nil
 	}
 
-	// Only process fields with the "env" tag
+	// Only process fields with the "env" tag, ignoring any option that follows the name of the variable
-	envTag := fieldType.Tag.Get("env")
+	envVarName, _, _ := strings.Cut(fieldType.Tag.Get("env"), ",")
-	if envTag == "" {
+	if envVarName == "" {
 		return nil
 	}
 
-	envVarName := envTag
+	// Load the value from the file referenced by the "_FILE" variable, keeping the value parsed from the environment if that variable is not set
-	if commaIndex := len(envTag); commaIndex > 0 {
-		envVarName = envTag[:commaIndex]
-	}
-
-	// If the file environment variable is not set, skip
-	envVarFileName := envVarName + "_FILE"
-	envVarFileValue := os.Getenv(envVarFileName)
-	if envVarFileValue == "" {
-		return nil
-	}
-
-	// #nosec G703 - Path is passed by the admin
-	fileContent, err := os.ReadFile(envVarFileValue)
-	if err != nil {
-		return fmt.Errorf("failed to read file for env var %s: %w", envVarFileName, err)
-	}
-
 	if isString {
-		field.SetString(strings.TrimSpace(string(fileContent)))
+		value, ok, err := LoadStringEnvVarFromFile(envVarName)
-	} else {
+		if err != nil || !ok {
-		field.SetBytes(fileContent)
+			return err
+		}
+
+		field.SetString(value)
+		return nil
 	}
 
+	value, ok, err := LoadEnvVarFromFile(envVarName)
+	if err != nil || !ok {
+		return err
+	}
+
+	field.SetBytes(value)
+
 	return nil
 }
 

backend/internal/common/env_file.go

@@ -0,0 +1,41 @@
+package common
+
+import (
+	"fmt"
+	"os"
+	"strings"
+)
+
+// FileEnvVarSuffix is the suffix of the environment variable that contains the path to the file with the value of another environment variable
+// For example, the value of "DB_CONNECTION_STRING" can be loaded from the file whose path is in "DB_CONNECTION_STRING_FILE"
+const FileEnvVarSuffix = "_FILE"
+
+// LoadEnvVarFromFile loads the value of the environment variable envVarName from the file referenced by the corresponding "_FILE" environment variable
+// The second return value is false when the "_FILE" variable is not set, in which case the caller should use the value of the environment variable itself
+// The content of the file is returned as-is: use LoadStringEnvVarFromFile for values that are used as strings
+func LoadEnvVarFromFile(envVarName string) ([]byte, bool, error) {
+	fileEnvVarName := envVarName + FileEnvVarSuffix
+	fileName := os.Getenv(fileEnvVarName)
+	if fileName == "" {
+		return nil, false, nil
+	}
+
+	// #nosec G703 - Path is passed by the admin
+	fileContent, err := os.ReadFile(fileName)
+	if err != nil {
+		return nil, false, fmt.Errorf("failed to read file '%s' for env var %s: %w", fileName, fileEnvVarName, err)
+	}
+
+	return fileContent, true, nil
+}
+
+// LoadStringEnvVarFromFile is like LoadEnvVarFromFile, but returns the content of the file as a string, with leading and trailing whitespace removed
+// Trimming is required because tools that write secrets to a file, including shell redirections and most editors, normally append a trailing newline
+func LoadStringEnvVarFromFile(envVarName string) (string, bool, error) {
+	fileContent, ok, err := LoadEnvVarFromFile(envVarName)
+	if err != nil || !ok {
+		return "", false, err
+	}
+
+	return strings.TrimSpace(string(fileContent)), true, nil
+}

backend/internal/common/env_file_test.go

@@ -0,0 +1,93 @@
+package common
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLoadEnvVarFromFile(t *testing.T) {
+	t.Run("returns false when the file env var is not set", func(t *testing.T) {
+		value, ok, err := LoadEnvVarFromFile("TEST_SECRET")
+
+		require.NoError(t, err)
+		assert.False(t, ok)
+		assert.Empty(t, value)
+	})
+
+	t.Run("returns the content of the file as-is", func(t *testing.T) {
+		fileName := writeTempFile(t, "secret.txt", "  my-secret\n")
+		t.Setenv("TEST_SECRET_FILE", fileName)
+
+		value, ok, err := LoadEnvVarFromFile("TEST_SECRET")
+
+		require.NoError(t, err)
+		assert.True(t, ok)
+		assert.Equal(t, []byte("  my-secret\n"), value)
+	})
+
+	t.Run("returns an error when the file cannot be read", func(t *testing.T) {
+		t.Setenv("TEST_SECRET_FILE", filepath.Join(t.TempDir(), "does-not-exist.txt"))
+
+		_, ok, err := LoadEnvVarFromFile("TEST_SECRET")
+
+		require.Error(t, err)
+		require.ErrorContains(t, err, "TEST_SECRET_FILE")
+		assert.False(t, ok)
+	})
+}
+
+func TestLoadStringEnvVarFromFile(t *testing.T) {
+	t.Run("returns false when the file env var is not set", func(t *testing.T) {
+		value, ok, err := LoadStringEnvVarFromFile("TEST_SECRET")
+
+		require.NoError(t, err)
+		assert.False(t, ok)
+		assert.Empty(t, value)
+	})
+
+	t.Run("trims leading and trailing whitespace", func(t *testing.T) {
+		fileName := writeTempFile(t, "secret.txt", "\tmy-secret \r\n")
+		t.Setenv("TEST_SECRET_FILE", fileName)
+
+		value, ok, err := LoadStringEnvVarFromFile("TEST_SECRET")
+
+		require.NoError(t, err)
+		assert.True(t, ok)
+		assert.Equal(t, "my-secret", value)
+	})
+
+	t.Run("returns an empty value when the file is empty", func(t *testing.T) {
+		fileName := writeTempFile(t, "empty.txt", "\n")
+		t.Setenv("TEST_SECRET_FILE", fileName)
+
+		value, ok, err := LoadStringEnvVarFromFile("TEST_SECRET")
+
+		require.NoError(t, err)
+		assert.True(t, ok)
+		assert.Empty(t, value)
+	})
+
+	t.Run("returns an error when the file cannot be read", func(t *testing.T) {
+		t.Setenv("TEST_SECRET_FILE", filepath.Join(t.TempDir(), "does-not-exist.txt"))
+
+		_, ok, err := LoadStringEnvVarFromFile("TEST_SECRET")
+
+		require.Error(t, err)
+		assert.False(t, ok)
+	})
+}
+
+// writeTempFile writes content to a file in a temporary directory that is removed when the test ends, returning its path
+func writeTempFile(t *testing.T, name string, content string) string {
+	t.Helper()
+
+	fileName := filepath.Join(t.TempDir(), name)
+	err := os.WriteFile(fileName, []byte(content), 0600)
+	require.NoError(t, err)
+
+	return fileName
+}