improve flexibility of limactl shell cmd

Signed-off-by: olalekan odukoya <[email protected]>

Author: olamilekan000

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' {
@@ -138,15 +138,6 @@ local_setup() {
     refute_line --regexp '^BARBAZ='
 }
 
-@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 'limactl info includes the default block list' {
     run -0 limactl info
     run -0 limactl yq '.shellEnvBlock[]' <<<"$output"

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 {
+	validPattern := regexp.MustCompile(`^[a-zA-Z0-9_*]*$`)
+	if !validPattern.MatchString(pattern) {
+		return fmt.Errorf("pattern %q contains invalid characters. Only alphanumeric characters, underscores, and asterisk are allowed", pattern)
+	}
+	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, "+")
+	var patterns []string
+	if !found {
+		patterns = parseEnvList(blockEnv)
+	} else {
+		patterns = parseEnvList(after)
+	}
+
+	for _, pattern := range patterns {
+		if err := validatePattern(pattern); err != nil {
+			logrus.Fatalf("Invalid LIMA_SHELLENV_BLOCK pattern: %v", err)
+		}
+	}
+
 	if !found {
-		return parseEnvList(blockEnv), true
+		return patterns
 	}
-	return slices.Concat(defaultBlockList, parseEnvList(after)), true
+	return slices.Concat(defaultBlockList, 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) {
+func getAllowList() []string {
 	if allowEnv := os.Getenv("LIMA_SHELLENV_ALLOW"); allowEnv != "" {
-		return parseEnvList(allowEnv), true
+		patterns := parseEnvList(allowEnv)
+
+		for _, pattern := range patterns {
+			if err := validatePattern(pattern); err != nil {
+				logrus.Fatalf("Invalid LIMA_SHELLENV_ALLOW pattern: %v", err)
+			}
+		}
+		return patterns
 	}
-	return nil, false
+	return nil
 }
 
 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(),
 	)
 }
 

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)
 	})
@@ -213,3 +208,53 @@ 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", "FOO", 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 dot", "FOO.BAR", false},
+		{"with space", "FOO BAR", false},
+		{"with slash", "FOO/BAR", false},
+		{"with at symbol", "FOO@BAR", false},
+		{"with dollar", "$FOO", false},
+		{"with percent", "FOO%", false},
+		{"with hash", "#FOO", false},
+		{"with exclamation", "FOO!", false},
+		{"with colon", "FOO:BAR", false},
+		{"with semicolon", "FOO;BAR", false},
+		{"with parentheses", "FOO(BAR)", false},
+		{"with brackets", "FOO[BAR]", false},
+		{"with braces", "FOO{BAR}", false},
+		{"with plus", "FOO+BAR", false},
+		{"with equals", "FOO=BAR", false},
+		{"with pipe", "FOO|BAR", false},
+		{"with backslash", "FOO/BAR", false},
+		{"with question mark", "FOO?", false},
+		{"with less than", "FOO<BAR", false},
+		{"with greater than", "FOO>BAR", false},
+		{"with comma", "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.Error(t, err, "pattern \""+tt.pattern+"\" contains invalid characters. Only alphanumeric characters, underscores, and asterisk are allowed")
+			}
+		})
+	}
+}