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
This commit is contained in:
@@ -5,6 +5,7 @@ import (
|
|||||||
"log/slog"
|
"log/slog"
|
||||||
"net/http"
|
"net/http"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"regexp"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
"github.com/go-chi/chi/v5"
|
"github.com/go-chi/chi/v5"
|
||||||
@@ -12,6 +13,9 @@ import (
|
|||||||
"github.com/alexei/docker-watcher/internal/store"
|
"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.
|
// validateVolumePath checks that the source path does not contain path traversal.
|
||||||
func validateVolumePath(source string) bool {
|
func validateVolumePath(source string) bool {
|
||||||
cleaned := filepath.Clean(source)
|
cleaned := filepath.Clean(source)
|
||||||
@@ -41,6 +45,9 @@ func validateVolumeScope(scope, name string) string {
|
|||||||
if (scope == "project_named" || scope == "named") && strings.TrimSpace(name) == "" {
|
if (scope == "project_named" || scope == "named") && strings.TrimSpace(name) == "" {
|
||||||
return "name is required for " + scope + " scope"
|
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 ""
|
return ""
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -236,6 +243,12 @@ func (s *Server) updateVolume(w http.ResponseWriter, r *http.Request) {
|
|||||||
updated.Name = strings.TrimSpace(req.Name)
|
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 {
|
if err := s.store.UpdateVolume(updated); err != nil {
|
||||||
slog.Error("failed to update volume", "volume_id", volID, "error", err)
|
slog.Error("failed to update volume", "volume_id", volID, "error", err)
|
||||||
respondError(w, http.StatusInternalServerError, "failed to update volume")
|
respondError(w, http.StatusInternalServerError, "failed to update volume")
|
||||||
|
|||||||
+11
-4
@@ -116,10 +116,17 @@ func (s *Store) runMigrations() error {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Data migration: copy mode→scope for volumes that have scope still empty.
|
// Data migration: copy mode→scope for volumes that have scope still empty.
|
||||||
// shared→project, isolated→instance.
|
// shared→project, isolated→instance. Log errors but don't fail startup.
|
||||||
_, _ = s.db.Exec(`UPDATE volumes SET scope = 'project' WHERE scope = '' AND mode = 'shared'`)
|
dataMigrations := []struct{ query, desc string }{
|
||||||
_, _ = s.db.Exec(`UPDATE volumes SET scope = 'instance' WHERE scope = '' AND mode = 'isolated'`)
|
{`UPDATE volumes SET scope = 'project' WHERE scope = '' AND mode = 'shared'`, "migrate shared→project"},
|
||||||
_, _ = s.db.Exec(`UPDATE volumes SET scope = 'project' WHERE scope = '' AND mode = ''`)
|
{`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
|
return nil
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user