From bc025bbe28d754bdc4fb359a78c5431c28b8dd17 Mon Sep 17 00:00:00 2001
From: LordMathis <matus@namesny.com>
Date: Tue, 21 Oct 2025 22:57:23 +0200
Subject: [PATCH] Fix instance name validation

---
 pkg/manager/manager_test.go | 79 +++++++++++++++++++++++++++++++++++++
 pkg/manager/persistence.go  | 17 +++++---
 2 files changed, 90 insertions(+), 6 deletions(-)

diff --git a/pkg/manager/manager_test.go b/pkg/manager/manager_test.go
index ed9cdcb..8a1b069 100644
--- a/pkg/manager/manager_test.go
+++ b/pkg/manager/manager_test.go
@@ -134,6 +134,85 @@ func TestConcurrentAccess(t *testing.T) {
 	}
 }
 
+// TestCreateInstance_RejectsPathTraversal tests that instance names with path traversal attempts are rejected
+func TestCreateInstance_RejectsPathTraversal(t *testing.T) {
+	tempDir := t.TempDir()
+	cfg := createPersistenceConfig(tempDir)
+	backendConfig := createBackendConfig()
+	mgr := manager.New(backendConfig, cfg, map[string]config.NodeConfig{}, "main")
+
+	options := &instance.Options{
+		BackendOptions: backends.Options{
+			BackendType: backends.BackendTypeLlamaCpp,
+			LlamaServerOptions: &backends.LlamaServerOptions{
+				Model: "/path/to/model.gguf",
+				Port:  8080,
+			},
+		},
+	}
+
+	// Test cases for malicious instance names
+	maliciousNames := []string{
+		"../../etc/passwd",      // Classic path traversal
+		"../../../etc/shadow",   // Multiple parent directory references
+		"/etc/passwd",           // Absolute path
+		"foo/../bar",            // Parent reference in middle
+		".../.../",              // Variation with multiple dots
+		".hidden",               // Hidden file
+		"foo/bar",               // Forward slash
+		"foo\\bar",              // Backslash (Windows-style)
+		"test..instance",        // Double dots not at path boundary (should fail)
+		"normal-name/../escape", // Normal-looking name with traversal
+	}
+
+	for _, name := range maliciousNames {
+		t.Run(name, func(t *testing.T) {
+			_, err := mgr.CreateInstance(name, options)
+			if err == nil {
+				t.Errorf("Expected error when creating instance with malicious name %q, but got none", name)
+			}
+		})
+	}
+}
+
+// TestCreateInstance_AcceptsValidNames tests that valid instance names are accepted
+func TestCreateInstance_AcceptsValidNames(t *testing.T) {
+	tempDir := t.TempDir()
+	cfg := createPersistenceConfig(tempDir)
+	backendConfig := createBackendConfig()
+	mgr := manager.New(backendConfig, cfg, map[string]config.NodeConfig{}, "main")
+	defer mgr.Shutdown()
+
+	options := &instance.Options{
+		BackendOptions: backends.Options{
+			BackendType: backends.BackendTypeLlamaCpp,
+			LlamaServerOptions: &backends.LlamaServerOptions{
+				Model: "/path/to/model.gguf",
+			},
+		},
+	}
+
+	// Valid instance names
+	validNames := []string{
+		"test-instance",
+		"my_instance",
+		"instance123",
+		"test-name-with-dashes",
+		"name_with_underscores",
+	}
+
+	for _, name := range validNames {
+		t.Run(name, func(t *testing.T) {
+			_, err := mgr.CreateInstance(name, options)
+			if err != nil {
+				t.Errorf("Expected instance with valid name %q to be created, but got error: %v", name, err)
+			}
+			// Clean up
+			mgr.DeleteInstance(name)
+		})
+	}
+}
+
 // Helper functions for test configuration
 func createBackendConfig() config.BackendConfig {
 	// Use 'sleep' as a test command instead of 'llama-server'
diff --git a/pkg/manager/persistence.go b/pkg/manager/persistence.go
index 38c1c82..01fe11f 100644
--- a/pkg/manager/persistence.go
+++ b/pkg/manager/persistence.go
@@ -207,12 +207,17 @@ func (p *instancePersister) validateInstanceName(name string) (string, error) {
 		return "", fmt.Errorf("instance name cannot be empty")
 	}
 
-	cleaned := filepath.Clean(name)
-
-	// After cleaning, name should not contain any path separators
-	if cleaned != name || strings.Contains(cleaned, string(filepath.Separator)) {
-		return "", fmt.Errorf("invalid instance name: %s", name)
+	// Check for path separators and parent directory references
+	// This prevents path traversal attacks
+	if strings.Contains(name, "/") || strings.Contains(name, "\\") || strings.Contains(name, "..") {
+		return "", fmt.Errorf("invalid instance name: %s (cannot contain path separators or '..')", name)
 	}
 
-	return cleaned, nil
+	// Additional check: ensure the name doesn't start with a dot (hidden files)
+	// or contain any other suspicious characters
+	if strings.HasPrefix(name, ".") {
+		return "", fmt.Errorf("invalid instance name: %s (cannot start with '.')", name)
+	}
+
+	return name, nil
 }