From bb2729ad129e75a6dc839a519d9a28094946ebe0 Mon Sep 17 00:00:00 2001
From: "alexei.dolgolyov" <dolgolyov.alexei@gmail.com>
Date: Tue, 31 Mar 2026 23:31:27 +0300
Subject: [PATCH] fix: address volume scopes review findings

- CRITICAL: validate volume Name against path traversal (safe regex)
- HIGH: log data migration errors instead of silently ignoring
- HIGH: reject empty source when switching from ephemeral scope
---
 internal/api/volumes.go | 13 +++++++++++++
 internal/store/store.go | 15 +++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/internal/api/volumes.go b/internal/api/volumes.go
index 5df2644..a3234dd 100644
--- a/internal/api/volumes.go
+++ b/internal/api/volumes.go
@@ -5,6 +5,7 @@ import (
 	"log/slog"
 	"net/http"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"github.com/go-chi/chi/v5"
@@ -12,6 +13,9 @@ import (
 	"github.com/alexei/docker-watcher/internal/store"
 )
 
+// safeNamePattern restricts volume names to alphanumeric, dash, underscore, and dot.
+var safeNamePattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`)
+
 // validateVolumePath checks that the source path does not contain path traversal.
 func validateVolumePath(source string) bool {
 	cleaned := filepath.Clean(source)
@@ -41,6 +45,9 @@ func validateVolumeScope(scope, name string) string {
 	if (scope == "project_named" || scope == "named") && strings.TrimSpace(name) == "" {
 		return "name is required for " + scope + " scope"
 	}
+	if name != "" && !safeNamePattern.MatchString(name) {
+		return "name must start with a letter or digit and contain only letters, digits, dashes, underscores, or dots"
+	}
 	return ""
 }
 
@@ -236,6 +243,12 @@ func (s *Server) updateVolume(w http.ResponseWriter, r *http.Request) {
 		updated.Name = strings.TrimSpace(req.Name)
 	}
 
+	// Non-ephemeral scopes require a source path.
+	if updated.Scope != "ephemeral" && updated.Source == "" {
+		respondError(w, http.StatusBadRequest, "source is required for non-ephemeral scopes")
+		return
+	}
+
 	if err := s.store.UpdateVolume(updated); err != nil {
 		slog.Error("failed to update volume", "volume_id", volID, "error", err)
 		respondError(w, http.StatusInternalServerError, "failed to update volume")
diff --git a/internal/store/store.go b/internal/store/store.go
index ff524da..4b9440a 100644
--- a/internal/store/store.go
+++ b/internal/store/store.go
@@ -116,10 +116,17 @@ func (s *Store) runMigrations() error {
 	}
 
 	// Data migration: copy mode→scope for volumes that have scope still empty.
-	// shared→project, isolated→instance.
-	_, _ = s.db.Exec(`UPDATE volumes SET scope = 'project' WHERE scope = '' AND mode = 'shared'`)
-	_, _ = s.db.Exec(`UPDATE volumes SET scope = 'instance' WHERE scope = '' AND mode = 'isolated'`)
-	_, _ = s.db.Exec(`UPDATE volumes SET scope = 'project' WHERE scope = '' AND mode = ''`)
+	// shared→project, isolated→instance. Log errors but don't fail startup.
+	dataMigrations := []struct{ query, desc string }{
+		{`UPDATE volumes SET scope = 'project' WHERE scope = '' AND mode = 'shared'`, "migrate shared→project"},
+		{`UPDATE volumes SET scope = 'instance' WHERE scope = '' AND mode = 'isolated'`, "migrate isolated→instance"},
+		{`UPDATE volumes SET scope = 'project' WHERE scope = '' AND mode = ''`, "migrate empty→project"},
+	}
+	for _, dm := range dataMigrations {
+		if _, err := s.db.Exec(dm.query); err != nil {
+			fmt.Printf("volume scope migration warning (%s): %v\n", dm.desc, err)
+		}
+	}
 
 	return nil
 }