chore: security improvements

SECURITY.md

@@ -0,0 +1,38 @@
+# Security
+
+This document describes security-relevant behavior and recommendations when using cli-kit.
+
+## Path validation (validator)
+
+- **Path traversal**: `ValidatePath` rejects paths containing `..` both in the raw input and after normalization (`filepath.Clean`), to reduce bypass via encoding or platform quirks.
+- **AllowedDirs**: When `AllowedDirs` is set, the resolved path must be exactly an allowed directory or under it; prefix tricks (e.g. `/tmpfoo` when `/tmp` is allowed) are rejected. The error message does not include the list of allowed directories to avoid information disclosure if the error is surfaced to untrusted parties.
+- **Symlinks**: Symlinks are not resolved. Paths under an allowed directory may point outside it via symlinks. For strict containment, resolve symlinks at the call site or use OS-specific checks.
+
+## URL validation (validator)
+
+- **SSRF**: `ValidateURL` blocks private IPs and localhost by default. Use `URLOptions.AllowLocalhost` / `AllowPrivateIP` only when intentional.
+- **Resolution**: When `ResolveHostTimeout` is set, hostnames are resolved and all resolved IPs are checked against the same rules.
+- **Redirects**: Validation applies only to the URL as given. It does not protect against HTTP redirects (e.g. to private IPs) when the application later performs the request. Configure the HTTP client to restrict redirects or re-validate the resolved URL if needed.
+
+## Configuration and environment (env, configutil)
+
+- **Empty vs unset**: `env.Get` and `env.GetTrimmed` return the default when the variable is **not set or set to empty**. To tell “not set” from “set to empty”, use `env.Lookup` or `env.Has`. This matters for “must be set” or “empty means disable” semantics.
+- **Sensitive values**: Avoid logging or error messages that include resolved config (e.g. URLs, paths, tokens). Prefer redaction in logs.
+- **Environment variable keys**: Keys containing NUL (`\x00`) can have undefined or unsafe behavior on some systems. The `env` package does not validate keys; avoid passing user-controlled or untrusted strings as keys. Use testutil's `EnvManager` in tests, which rejects empty and NUL keys.
+
+## Passwords (flagutil)
+
+- **ReadPasswordFromFile**: The path is validated with traversal checks. The password is returned as a string and will remain in process memory; minimize copies and lifetime where possible.
+- **TOCTOU**: There is a short window between path validation and reading the file. If an attacker can replace the path with a symlink in between (e.g. to another file), the read may target the new target. For highest assurance, use a dedicated directory with restricted permissions and no symlinks, or open files under a locked working directory.
+
+## Validators (validator)
+
+- **UsernameOptions.CustomPattern**: If the regex is built from user or external input, it can be vulnerable to ReDoS (catastrophic backtracking). Use only fixed, well-tested patterns or restrict pattern complexity when the source is untrusted.
+
+## Test utilities (testutil)
+
+- **EnvManager**: `Set`, `Unset`, and `SetMultiple` reject empty or NUL-containing environment variable keys to avoid unsafe or platform-dependent behavior.
+
+## Reporting
+
+If you believe you’ve found a security issue, please report it privately (e.g. via the maintainers or a private security channel) rather than in a public issue.

configutil/priority_test.go

@@ -1136,6 +1136,55 @@ func TestResolveStringSliceMulti(t *testing.T) {
 			t.Errorf("ResolveStringSliceMulti() length = %d, want %d", len(got), len(expected))
 		}
 	})
+
+	t.Run("ENV used when flag set but currentFlagValue empty", func(t *testing.T) {
+		fs := flag.NewFlagSet("test", flag.ContinueOnError)
+		fs.String("test-flag", "", "test flag")
+		setEnv(t, "TEST_ENV", "from_env_a,from_env_b")
+		defer unsetEnv(t, "TEST_ENV")
+
+		if err := fs.Parse([]string{"--test-flag", "x"}); err != nil {
+			t.Fatalf("fs.Parse() failed: %v", err)
+		}
+
+		got := ResolveStringSliceMulti(fs, "test-flag", "TEST_ENV", []string{}, []string{"default"}, ",")
+		expected := []string{"from_env_a", "from_env_b"}
+		if len(got) != len(expected) || (len(got) > 0 && (got[0] != expected[0] || got[1] != expected[1])) {
+			t.Errorf("ResolveStringSliceMulti() = %v, want %v", got, expected)
+		}
+	})
+
+	t.Run("Default when ENV set but parses to empty", func(t *testing.T) {
+		fs := flag.NewFlagSet("test", flag.ContinueOnError)
+		fs.String("test-flag", "", "test flag")
+		setEnv(t, "TEST_ENV_MULTI_EMPTY", "  ,  ,  ")
+		defer unsetEnv(t, "TEST_ENV_MULTI_EMPTY")
+
+		if err := fs.Parse([]string{}); err != nil {
+			t.Fatalf("fs.Parse() failed: %v", err)
+		}
+
+		got := ResolveStringSliceMulti(fs, "test-flag", "TEST_ENV_MULTI_EMPTY", []string{}, []string{"default"}, ",")
+		if len(got) != 1 || got[0] != "default" {
+			t.Errorf("ResolveStringSliceMulti() = %v, want [default]", got)
+		}
+	})
+
+	t.Run("Empty separator defaults to comma", func(t *testing.T) {
+		fs := flag.NewFlagSet("test", flag.ContinueOnError)
+		fs.String("test-flag", "", "test flag")
+		setEnv(t, "TEST_ENV_SEP", "a,b,c")
+		defer unsetEnv(t, "TEST_ENV_SEP")
+
+		if err := fs.Parse([]string{}); err != nil {
+			t.Fatalf("fs.Parse() failed: %v", err)
+		}
+
+		got := ResolveStringSliceMulti(fs, "test-flag", "TEST_ENV_SEP", []string{}, []string{"default"}, "")
+		if len(got) != 3 || got[0] != "a" || got[1] != "b" || got[2] != "c" {
+			t.Errorf("ResolveStringSliceMulti() with empty sep = %v, want [a b c]", got)
+		}
+	})
 }
 
 func TestResolveEnum(t *testing.T) {

env/env.go

@@ -21,7 +21,9 @@ func Has(key string) bool {
 	return ok
 }
 
-// Get retrieves an environment variable value, returning defaultValue if not set
+// Get retrieves an environment variable value, returning defaultValue if the variable
+// is not set or is set to the empty string. To distinguish "not set" from "set to empty",
+// use Lookup or Has.
 func Get(key, defaultValue string) string {
 	if value := os.Getenv(key); value != "" {
 		return value

flagutil/flagutil.go

@@ -3,10 +3,11 @@ package flagutil
 import (
 	"flag"
 	"os"
-	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
+
+	"github.com/soulteary/cli-kit/validator"
 )
 
 // HasFlag checks if a command-line flag is set in the given FlagSet
@@ -154,24 +155,21 @@ func GetDuration(fs *flag.FlagSet, name string, defaultValue time.Duration) time
 	return defaultValue
 }
 
-// ReadPasswordFromFile reads password from file (security improvement)
-// File path should be absolute path or relative to working directory
-// File content will have leading and trailing whitespace trimmed
+// ReadPasswordFromFile reads password from file (security improvement).
+// Path is validated with path traversal check; relative paths are resolved to absolute.
+// File content is trimmed of leading and trailing whitespace.
 func ReadPasswordFromFile(filePath string) (string, error) {
-	// Security check: ensure file path is relative or absolute path, prevent path traversal attacks
-	absPath, err := filepath.Abs(filePath)
+	// Security: reject path traversal and resolve to absolute path
+	safePath, err := validator.ValidatePath(filePath, &validator.PathOptions{CheckTraversal: true})
 	if err != nil {
 		return "", err
 	}
 
-	// Read file content
-	// #nosec G304 -- file path has been validated via filepath.Abs, is safe
-	data, err := os.ReadFile(absPath)
+	data, err := os.ReadFile(safePath)
 	if err != nil {
 		return "", err
 	}
 
-	// Trim leading and trailing whitespace
 	password := strings.TrimSpace(string(data))
 	return password, nil
 }

flagutil/flagutil_test.go

@@ -339,6 +339,13 @@ func TestReadPasswordFromFile_Nonexistent(t *testing.T) {
 	}
 }
 
+func TestReadPasswordFromFile_PathTraversal(t *testing.T) {
+	_, err := ReadPasswordFromFile("../../../etc/passwd")
+	if err == nil {
+		t.Error("ReadPasswordFromFile() should return error for path traversal")
+	}
+}
+
 func TestReadPasswordFromFile_AbsError(t *testing.T) {
 	// Test filepath.Abs error branch by using an invalid path
 	// On some systems, very long paths or special characters might cause Abs to fail

testutil/env.go

@@ -3,8 +3,12 @@ package testutil
 import (
 	"fmt"
 	"os"
+	"strings"
 )
 
+// ErrInvalidEnvKey is returned when an environment variable key is empty or invalid
+var ErrInvalidEnvKey = fmt.Errorf("environment variable key cannot be empty or contain NUL")
+
 // EnvManager manages environment variables for testing
 // It saves original values and can restore them after tests
 type EnvManager struct {
@@ -18,8 +22,22 @@ func NewEnvManager() *EnvManager {
 	}
 }
 
+// validateEnvKey returns an error if the key is empty or contains NUL (unsafe on many systems)
+func validateEnvKey(key string) error {
+	if key == "" {
+		return fmt.Errorf("%w", ErrInvalidEnvKey)
+	}
+	if strings.ContainsRune(key, 0) {
+		return fmt.Errorf("%w", ErrInvalidEnvKey)
+	}
+	return nil
+}
+
 // Set sets an environment variable and saves the original value
 func (m *EnvManager) Set(key, value string) error {
+	if err := validateEnvKey(key); err != nil {
+		return err
+	}
 	// Save original value if not already saved
 	if _, exists := m.original[key]; !exists {
 		m.original[key] = os.Getenv(key)
@@ -29,6 +47,9 @@ func (m *EnvManager) Set(key, value string) error {
 
 // Unset unsets an environment variable and saves the original value
 func (m *EnvManager) Unset(key string) error {
+	if err := validateEnvKey(key); err != nil {
+		return err
+	}
 	// Save original value if not already saved
 	if _, exists := m.original[key]; !exists {
 		m.original[key] = os.Getenv(key)
@@ -61,6 +82,9 @@ func (m *EnvManager) Cleanup() {
 // SetMultiple sets multiple environment variables at once
 func (m *EnvManager) SetMultiple(vars map[string]string) error {
 	for key, value := range vars {
+		if err := validateEnvKey(key); err != nil {
+			return err
+		}
 		if err := m.Set(key, value); err != nil {
 			return err
 		}

testutil/env_test.go

@@ -1,10 +1,15 @@
 package testutil
 
 import (
+	"errors"
 	"os"
 	"testing"
 )
 
+func isErrInvalidEnvKey(err error) bool {
+	return errors.Is(err, ErrInvalidEnvKey)
+}
+
 func TestEnvManager(t *testing.T) {
 	t.Run("Set and Restore", func(t *testing.T) {
 		originalValue := os.Getenv("TEST_ENV_MANAGER")
@@ -170,6 +175,54 @@ func TestEnvManager(t *testing.T) {
 		}
 	})
 
+	t.Run("Set with empty key returns error", func(t *testing.T) {
+		manager := NewEnvManager()
+		defer manager.Cleanup()
+		if err := manager.Set("", "value"); err == nil {
+			t.Error("Set() with empty key want error, got nil")
+		} else if err != ErrInvalidEnvKey && !isErrInvalidEnvKey(err) {
+			t.Errorf("Set() with empty key want ErrInvalidEnvKey, got %v", err)
+		}
+	})
+
+	t.Run("Unset with empty key returns error", func(t *testing.T) {
+		manager := NewEnvManager()
+		defer manager.Cleanup()
+		if err := manager.Unset(""); err == nil {
+			t.Error("Unset() with empty key want error, got nil")
+		} else if err != ErrInvalidEnvKey && !isErrInvalidEnvKey(err) {
+			t.Errorf("Unset() with empty key want ErrInvalidEnvKey, got %v", err)
+		}
+	})
+
+	t.Run("SetMultiple with empty key returns error", func(t *testing.T) {
+		manager := NewEnvManager()
+		defer manager.Cleanup()
+		if err := manager.SetMultiple(map[string]string{"": "v"}); err == nil {
+			t.Error("SetMultiple() with empty key want error, got nil")
+		} else if err != ErrInvalidEnvKey && !isErrInvalidEnvKey(err) {
+			t.Errorf("SetMultiple() with empty key want ErrInvalidEnvKey, got %v", err)
+		}
+	})
+
+	t.Run("Set with NUL key returns error", func(t *testing.T) {
+		manager := NewEnvManager()
+		defer manager.Cleanup()
+		if err := manager.Set("\x00", "value"); err == nil {
+			t.Error("Set() with NUL key want error, got nil")
+		} else if !isErrInvalidEnvKey(err) {
+			t.Errorf("Set() with NUL key want ErrInvalidEnvKey, got %v", err)
+		}
+	})
+
+	t.Run("Clear with no variables", func(t *testing.T) {
+		manager := NewEnvManager()
+		defer manager.Cleanup()
+		if err := manager.Clear(); err != nil {
+			t.Errorf("Clear() with no variables = %v, want nil", err)
+		}
+	})
+
 	t.Run("Restore with multiple variables", func(t *testing.T) {
 		originalValue1 := os.Getenv("TEST_RESTORE_VAR1")
 		originalValue2 := os.Getenv("TEST_RESTORE_VAR2")

validator/path.go

@@ -1,6 +1,8 @@
 package validator
 
 import (
+	"crypto/rand"
+	"encoding/hex"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -29,7 +31,7 @@ func defaultPathOptions() *PathOptions {
 // ValidatePath validates a file path to prevent path traversal attacks
 //
 // This function validates file paths, including:
-// - Path traversal detection (..)
+// - Path traversal detection (..), including after normalization to resist bypasses
 // - Absolute vs relative path handling
 // - Optional directory restrictions
 //
@@ -63,27 +65,55 @@ func ValidatePath(path string, opts *PathOptions) (string, error) {
 		return "", fmt.Errorf("unable to parse path: %w", err)
 	}
 
-	// Check directory restrictions
+	// Security: after normalization, ensure no ".." segment remains (guards against
+	// encoding/unicode bypasses or platform quirks that might bypass the string check)
+	if opts.CheckTraversal {
+		cleaned := filepath.Clean(absPath)
+		if containsTraversalSegment(cleaned) {
+			return "", fmt.Errorf("path cannot contain path traversal characters (..)")
+		}
+		absPath = cleaned
+	}
+
+	// Check directory restrictions: path must be exactly allowedDir or under it (no prefix bypass)
 	if len(opts.AllowedDirs) > 0 {
 		allowed := false
+		sep := string(filepath.Separator)
 		for _, allowedDir := range opts.AllowedDirs {
 			allowedAbsDir, err := filepath.Abs(allowedDir)
 			if err != nil {
 				continue
 			}
-			if strings.HasPrefix(absPath, allowedAbsDir) {
+			allowedAbsDir = filepath.Clean(allowedAbsDir)
+			if absPath == allowedAbsDir {
+				allowed = true
+				break
+			}
+			prefix := allowedAbsDir + sep
+			if strings.HasPrefix(absPath, prefix) {
 				allowed = true
 				break
 			}
 		}
 		if !allowed {
-			return "", fmt.Errorf("path must be under allowed directories: %v", opts.AllowedDirs)
+			// Do not include AllowedDirs in error to avoid leaking allowed paths to callers (e.g. API responses)
+			return "", fmt.Errorf("path is not under allowed directories")
 		}
 	}
 
 	return absPath, nil
 }
 
+// containsTraversalSegment returns true if path contains ".." as a path segment.
+func containsTraversalSegment(path string) bool {
+	for _, part := range strings.Split(path, string(filepath.Separator)) {
+		if part == ".." {
+			return true
+		}
+	}
+	return false
+}
+
 // ErrFileNotFound is returned when a file does not exist
 var ErrFileNotFound = fmt.Errorf("file not found")
 
@@ -205,7 +235,11 @@ func ValidateDirWritable(path string) error {
 	return nil
 }
 
-// randomSuffix generates a simple random suffix for test files
+// randomSuffix generates a random suffix for write-test filenames to avoid predictability and races.
 func randomSuffix() string {
-	return fmt.Sprintf("%d", os.Getpid())
+	b := make([]byte, 4)
+	if _, err := rand.Read(b); err != nil {
+		return fmt.Sprintf("%d", os.Getpid())
+	}
+	return hex.EncodeToString(b)
 }