diff --git a/hack/bats/tests/preserve-env.bats b/hack/bats/tests/preserve-env.bats index e53257356a5..345e5e3c955 100644 --- a/hack/bats/tests/preserve-env.bats +++ b/hack/bats/tests/preserve-env.bats @@ -93,13 +93,13 @@ local_setup() { assert_line BAR=bar } -@test 'wildcard does only work at the end of the pattern' { +@test 'wildcard works at the start of the pattern' { export LIMA_SHELLENV_BLOCK="*FOO" export FOO=foo export BARFOO=barfoo run -0 limactl shell --preserve-env "$NAME" printenv - assert_line FOO=foo - assert_line BARFOO=barfoo + refute_line --regexp '^BARFOO=' + refute_line --regexp '^FOO=' } @test 'block list can use a , separated list with whitespace ignored' { @@ -114,37 +114,72 @@ local_setup() { assert_line BARBAZ=barbaz } -@test 'allow list overrides block list but blocks everything else' { - export LIMA_SHELLENV_ALLOW=SSH_FOO +@test 'allow list can use a , separated list with whitespace ignored' { + export LIMA_SHELLENV_ALLOW="SSH_FOO, , BAR*, LD_UID" export SSH_FOO=ssh_foo export SSH_BAR=ssh_bar + export SSH_BLOCK=ssh_block export BAR=bar + export BARBAZ=barbaz + export LD_UID=randomuid run -0 limactl shell --preserve-env "$NAME" printenv + assert_line SSH_FOO=ssh_foo + assert_line BAR=bar + assert_line BARBAZ=barbaz + assert_line LD_UID=randomuid + refute_line --regexp '^SSH_BAR=' - refute_line --regexp '^BAR=' + refute_line --regexp '^SSH_BLOCK=' } -@test 'allow list can use a , separated list with whitespace ignored' { - export LIMA_SHELLENV_ALLOW="FOO*, , BAR" +@test 'wildcard patterns work in all positions' { + export LIMA_SHELLENV_BLOCK="*FOO*BAR*" export FOO=foo export FOOBAR=foobar + export FOOXYZBAR=fooxyzbar + export FOOBAZ=foobaz + export BAZBAR=bazbar export BAR=bar - export BARBAZ=barbaz + export XFOOYBARZDOTCOM=xfooybarzdotcom + export NORMAL_VAR=normal_var + export UNRELATED=unrelated run -0 limactl shell --preserve-env "$NAME" printenv - assert_line FOO=foo - assert_line FOOBAR=foobar + + refute_line --regexp '^FOOBAR=' + refute_line --regexp '^FOOXYZBAR=' + refute_line --regexp '^XFOOYBARZDOTCOM=' + + assert_line FOOBAZ=foobaz + assert_line NORMAL_VAR=normal_var + assert_line UNRELATED=unrelated + assert_line BAZBAR=bazbar assert_line BAR=bar - refute_line --regexp '^BARBAZ=' + assert_line FOO=foo } -@test 'setting both allow list and block list generates a warning' { - export LIMA_SHELLENV_ALLOW=FOO - export LIMA_SHELLENV_BLOCK=BAR - export FOO=foo - run -0 --separate-stderr limactl shell --preserve-env "$NAME" printenv FOO - assert_output foo - assert_stderr --regexp 'level=warning msg="Both LIMA_SHELLENV_BLOCK and LIMA_SHELLENV_ALLOW are set' +@test 'allowlist overrides default blocklist with wildcards' { + export LIMA_SHELLENV_ALLOW="SSH_*,CUSTOM*" + export LIMA_SHELLENV_BLOCK="+*TOKEN" + export SSH_AUTH_SOCK=ssh_auth_sock + export SSH_CONNECTION=ssh_connection + export CUSTOM_VAR=custom_var + export MY_TOKEN=my_token + export UNRELATED=unrelated + run -0 limactl shell --preserve-env "$NAME" printenv + + assert_line SSH_AUTH_SOCK=ssh_auth_sock + assert_line SSH_CONNECTION=ssh_connection + assert_line CUSTOM_VAR=custom_var + refute_line --regexp '^MY_TOKEN=' + assert_line UNRELATED=unrelated +} + +@test 'invalid characters in patterns cause fatal errors' { + export LIMA_SHELLENV_BLOCK="FOO-BAR" + run ! limactl shell --preserve-env "$NAME" printenv + assert_output --partial "Invalid LIMA_SHELLENV_BLOCK pattern" + assert_output --partial "contains invalid character" } @test 'limactl info includes the default block list' { diff --git a/hack/test-preserve-env.sh b/hack/test-preserve-env.sh deleted file mode 100755 index 20959107d98..00000000000 --- a/hack/test-preserve-env.sh +++ /dev/null @@ -1,94 +0,0 @@ -#!/usr/bin/env bash - -# SPDX-FileCopyrightText: Copyright The Lima Authors -# SPDX-License-Identifier: Apache-2.0 - -set -eu -o pipefail - -scriptdir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -# shellcheck source=common.inc.sh -source "${scriptdir}/common.inc.sh" - -if [ "$#" -ne 1 ]; then - ERROR "Usage: $0 NAME" - exit 1 -fi - -NAME="$1" - -INFO "Testing --preserve-env flag" - -# Environment variable propagation with --preserve-env -INFO "=== Environment variable propagation with --preserve-env ===" -test_var_name="LIMA_TEST_PRESERVE_ENV_VAR" -test_var_value="test-value-${RANDOM}" - -got="$(LIMA_SHELLENV_ALLOW="LIMA_TEST_*" LIMA_TEST_PRESERVE_ENV_VAR="$test_var_value" limactl shell --preserve-env "$NAME" printenv "$test_var_name" 2>/dev/null || echo "NOT_FOUND")" -INFO "$test_var_name: expected=${test_var_value}, got=${got}" -if [ "$got" != "$test_var_value" ]; then - ERROR "Environment variable was not propagated with --preserve-env" - exit 1 -fi - -# Clean up before next test -unset LIMA_TEST_PRESERVE_ENV_VAR -unset LIMA_SHELLENV_ALLOW - -# Environment variable NOT propagated without --preserve-env -INFO "=== Environment variable NOT propagated without --preserve-env ===" -got_without_flag="$(LIMA_TEST_PRESERVE_ENV_VAR="$test_var_value" limactl shell "$NAME" printenv "$test_var_name" 2>/dev/null || echo "NOT_FOUND")" -INFO "$test_var_name without --preserve-env: got=${got_without_flag}" -if [ "$got_without_flag" != "NOT_FOUND" ]; then - ERROR "Environment variable was unexpectedly propagated without --preserve-env" - exit 1 -fi - -# Blocked environment variables should not be propagated even with --preserve-env -INFO "=== Blocked environment variables are not propagated with --preserve-env ===" -blocked_var_name="HOME" -fake_home="/tmp/fake-home-${RANDOM}" -got_blocked="$(HOME="$fake_home" limactl shell --preserve-env "$NAME" printenv "$blocked_var_name" 2>/dev/null || echo "NOT_FOUND")" -INFO "$blocked_var_name: host=${fake_home}, guest=${got_blocked}" -if [ "$got_blocked" = "$fake_home" ]; then - ERROR "Blocked environment variable $blocked_var_name was propagated" - exit 1 -fi - -# LIMA_SHELLENV_BLOCK functionality -INFO "=== LIMA_SHELLENV_BLOCK with custom pattern ===" -custom_test_var="LIMA_TEST_CUSTOM_BLOCK" -custom_test_value="should-be-blocked" -got_custom_blocked="$(LIMA_SHELLENV_BLOCK="+LIMA_TEST_CUSTOM_*" LIMA_TEST_CUSTOM_BLOCK="$custom_test_value" limactl shell --preserve-env "$NAME" printenv "$custom_test_var" 2>/dev/null || echo "NOT_FOUND")" -INFO "$custom_test_var with LIMA_SHELLENV_BLOCK: got=${got_custom_blocked}" -if [ "$got_custom_blocked" != "NOT_FOUND" ]; then - ERROR "Custom blocked environment variable was propagated" - exit 1 -fi - -# Clean up before next test -unset LIMA_TEST_CUSTOM_BLOCK 2>/dev/null || true -unset LIMA_SHELLENV_BLOCK 2>/dev/null || true - -# LIMA_SHELLENV_ALLOW functionality -INFO "=== LIMA_SHELLENV_ALLOW with custom pattern ===" -allow_test_var="LIMA_TEST_ALLOW_VAR" -allow_test_value="should-be-allowed" -got_allowed="$(LIMA_SHELLENV_ALLOW="LIMA_TEST_ALLOW_*" LIMA_TEST_ALLOW_VAR="$allow_test_value" limactl shell --preserve-env "$NAME" printenv "$allow_test_var" 2>/dev/null || echo "NOT_FOUND")" -INFO "$allow_test_var with LIMA_SHELLENV_ALLOW: got=${got_allowed}" -if [ "$got_allowed" != "$allow_test_value" ]; then - ERROR "Allowed environment variable was not propagated" - exit 1 -fi - -# Non-allowed variables are blocked when LIMA_SHELLENV_ALLOW is set -INFO "=== Non-allowed variables are blocked when LIMA_SHELLENV_ALLOW is set ===" -other_test_var="LIMA_TEST_OTHER_VAR" -other_test_value="should-be-blocked" -got_other="$(LIMA_SHELLENV_ALLOW="LIMA_TEST_ALLOW_*" LIMA_TEST_OTHER_VAR="$other_test_value" limactl shell --preserve-env "$NAME" printenv "$other_test_var" 2>/dev/null || echo "NOT_FOUND")" -INFO "$other_test_var with LIMA_SHELLENV_ALLOW (should be blocked): got=${got_other}" -if [ "$got_other" != "NOT_FOUND" ]; then - ERROR "Non-allowed environment variable was propagated when LIMA_SHELLENV_ALLOW was set" - exit 1 -fi - -INFO "All --preserve-env tests passed" diff --git a/hack/test-templates.sh b/hack/test-templates.sh index e1b97b5e945..6174a04ab69 100755 --- a/hack/test-templates.sh +++ b/hack/test-templates.sh @@ -339,10 +339,6 @@ if [[ -n ${CHECKS["mount-home"]} ]]; then "${scriptdir}"/test-mount-home.sh "$NAME" fi -if [[ -n ${CHECKS["preserve-env"]} ]]; then - "${scriptdir}"/test-preserve-env.sh "$NAME" -fi - if [[ -n ${CHECKS["ssh-over-vsock"]} ]]; then if [[ "$(limactl ls "${NAME}" --yq .vmType)" == "vz" ]]; then INFO "Testing SSH over vsock" diff --git a/pkg/envutil/envutil.go b/pkg/envutil/envutil.go index 023b9e0e91a..d5147d5a8c7 100644 --- a/pkg/envutil/envutil.go +++ b/pkg/envutil/envutil.go @@ -4,7 +4,9 @@ package envutil import ( + "fmt" "os" + "regexp" "slices" "strings" @@ -42,27 +44,55 @@ var defaultBlockList = []string{ "_*", // Variables starting with underscore are typically internal } +func validatePattern(pattern string) error { + invalidChar := regexp.MustCompile(`([^a-zA-Z0-9_*])`) + if matches := invalidChar.FindStringSubmatch(pattern); matches != nil { + invalidCharacter := matches[1] + pos := strings.Index(pattern, invalidCharacter) + return fmt.Errorf("pattern %q contains invalid character %q at position %d", + pattern, invalidCharacter, pos) + } + return nil +} + // getBlockList returns the list of environment variable patterns to be blocked. -// The second return value indicates whether the list was explicitly set via LIMA_SHELLENV_BLOCK. -func getBlockList() ([]string, bool) { +func getBlockList() []string { blockEnv := os.Getenv("LIMA_SHELLENV_BLOCK") if blockEnv == "" { - return defaultBlockList, false + return defaultBlockList } - after, found := strings.CutPrefix(blockEnv, "+") - if !found { - return parseEnvList(blockEnv), true + + shouldAppend := strings.HasPrefix(blockEnv, "+") + patterns := parseEnvList(strings.TrimPrefix(blockEnv, "+")) + + for _, pattern := range patterns { + if err := validatePattern(pattern); err != nil { + logrus.Fatalf("Invalid LIMA_SHELLENV_BLOCK pattern: %v", err) + } + } + + if shouldAppend { + return slices.Concat(defaultBlockList, patterns) } - return slices.Concat(defaultBlockList, parseEnvList(after)), true + return patterns } // getAllowList returns the list of environment variable patterns to be allowed. -// The second return value indicates whether the list was explicitly set via LIMA_SHELLENV_ALLOW. -func getAllowList() ([]string, bool) { - if allowEnv := os.Getenv("LIMA_SHELLENV_ALLOW"); allowEnv != "" { - return parseEnvList(allowEnv), true +func getAllowList() []string { + allowEnv := os.Getenv("LIMA_SHELLENV_ALLOW") + if allowEnv == "" { + return nil } - return nil, false + + patterns := parseEnvList(allowEnv) + + for _, pattern := range patterns { + if err := validatePattern(pattern); err != nil { + logrus.Fatalf("Invalid LIMA_SHELLENV_ALLOW pattern: %v", err) + } + } + + return patterns } func parseEnvList(envList string) []string { @@ -82,8 +112,14 @@ func matchesPattern(name, pattern string) bool { return true } - prefix, found := strings.CutSuffix(pattern, "*") - return found && strings.HasPrefix(name, prefix) + regexPattern := strings.ReplaceAll(pattern, "*", ".*") + regexPattern = "^" + regexPattern + "$" + + match, err := regexp.MatchString(regexPattern, name) + if err != nil { + return false + } + return match } func matchesAnyPattern(name string, patterns []string) bool { @@ -96,17 +132,10 @@ func matchesAnyPattern(name string, patterns []string) bool { // It returns a slice of environment variables that are not blocked by the current configuration. // The filtering is controlled by LIMA_SHELLENV_BLOCK and LIMA_SHELLENV_ALLOW environment variables. func FilterEnvironment() []string { - allowList, isAllowListSet := getAllowList() - blockList, isBlockListSet := getBlockList() - - if isBlockListSet && isAllowListSet { - logrus.Warn("Both LIMA_SHELLENV_BLOCK and LIMA_SHELLENV_ALLOW are set. Block list will be ignored.") - blockList = nil - } return filterEnvironmentWithLists( os.Environ(), - allowList, - blockList, + getAllowList(), + getBlockList(), ) } @@ -121,10 +150,7 @@ func filterEnvironmentWithLists(env, allowList, blockList []string) []string { name := parts[0] - if len(allowList) > 0 { - if !matchesAnyPattern(name, allowList) { - continue - } + if len(allowList) > 0 && matchesAnyPattern(name, allowList) { filtered = append(filtered, envVar) continue } diff --git a/pkg/envutil/envutil_test.go b/pkg/envutil/envutil_test.go index 3530288f425..6904580fa7d 100644 --- a/pkg/envutil/envutil_test.go +++ b/pkg/envutil/envutil_test.go @@ -88,11 +88,9 @@ func TestGetBlockAndAllowLists(t *testing.T) { t.Setenv("LIMA_SHELLENV_BLOCK", "") t.Setenv("LIMA_SHELLENV_ALLOW", "") - blockList, isBlockListSet := getBlockList() - allowList, isAllowListSet := getAllowList() + blockList := getBlockList() + allowList := getAllowList() - assert.Assert(t, !isBlockListSet) - assert.Assert(t, !isAllowListSet) assert.Assert(t, isUsingDefaultBlockList()) assert.DeepEqual(t, blockList, defaultBlockList) assert.Equal(t, len(allowList), 0) @@ -101,8 +99,7 @@ func TestGetBlockAndAllowLists(t *testing.T) { t.Run("custom blocklist", func(t *testing.T) { t.Setenv("LIMA_SHELLENV_BLOCK", "PATH,HOME") - blockList, isSet := getBlockList() - assert.Assert(t, isSet) + blockList := getBlockList() assert.Assert(t, !isUsingDefaultBlockList()) expected := []string{"PATH", "HOME"} assert.DeepEqual(t, blockList, expected) @@ -111,8 +108,7 @@ func TestGetBlockAndAllowLists(t *testing.T) { t.Run("additive blocklist", func(t *testing.T) { t.Setenv("LIMA_SHELLENV_BLOCK", "+CUSTOM_VAR") - blockList, isSet := getBlockList() - assert.Assert(t, isSet) + blockList := getBlockList() assert.Assert(t, isUsingDefaultBlockList()) expected := slices.Concat(GetDefaultBlockList(), []string{"CUSTOM_VAR"}) assert.DeepEqual(t, blockList, expected) @@ -121,8 +117,7 @@ func TestGetBlockAndAllowLists(t *testing.T) { t.Run("allowlist", func(t *testing.T) { t.Setenv("LIMA_SHELLENV_ALLOW", "FOO,BAR") - allowList, isSet := getAllowList() - assert.Assert(t, isSet) + allowList := getAllowList() expected := []string{"FOO", "BAR"} assert.DeepEqual(t, allowList, expected) }) @@ -173,9 +168,11 @@ func TestFilterEnvironment(t *testing.T) { t.Run("allowlist", func(t *testing.T) { result := filterEnvironmentWithLists(testEnv, []string{"FOO", "USER"}, nil) - expected := []string{"FOO=bar", "USER=testuser"} - assert.Equal(t, len(result), len(expected)) + + // since FOO and USER are included in testEnv, the filtered result should not differ + // from what is in the testEnv + assert.Equal(t, len(result), len(testEnv)) assert.Assert(t, containsAll(result, expected)) }) @@ -213,3 +210,42 @@ func TestGetDefaultBlockList(t *testing.T) { assert.Assert(t, found, "Expected builtin blocklist to contain %q", item) } } + +func TestValidatePattern(t *testing.T) { + tests := []struct { + name string + pattern string + valid bool + }{ + {"simple alphanumeric uppercase", "FOO", true}, + {"simple alphanumeric lowercase", "foo", true}, + {"mixed case", "FooBar", true}, + {"with underscore", "FOO_BAR", true}, + {"with numbers", "VAR123", true}, + {"with trailing asterisk", "FOO*", true}, + {"with multiple asterisks", "FOO*BAR*", true}, + {"asterisk at beginning", "*FOO", true}, + {"asterisk in middle", "FOO*BAR", true}, + {"only asterisk", "*", true}, + {"with dash", "FOO-BAR", false}, + {"with space", "FOO BAR", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validatePattern(tt.pattern) + if tt.valid { + assert.NilError(t, err, "Expected pattern %q to be valid", tt.pattern) + } else { + assert.Assert(t, err != nil, "Expected pattern %q to be invalid", tt.pattern) + } + }) + } +} + +func TestValidatePatternErrorMessage(t *testing.T) { + err := validatePattern("FOO-BAR") + assert.Assert(t, err != nil, "Expected pattern with dash to be invalid") + expectedMsg := `pattern "FOO-BAR" contains invalid character "-" at position 3` + assert.Equal(t, err.Error(), expectedMsg) +}