diff --git a/CHANGELOG.md b/CHANGELOG.md index e2edbbe..19e3e0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,49 @@ ## Changelog +### What was just completed (2026-02-17 session 39) +- **v0.12.3 — Security & Correctness Bug Fixes (TASK.md — 33 bugs fixed):** + + **CRITICAL fixes (data races, security vulnerabilities):** + - **C1: Data race in RefreshCache** — Moved `m.lastDBDump.Results` mutation inside `m.mu.Lock()`. Was previously mutating shared state without the lock, causing potential torn writes visible to `GetFullStatus()` goroutines. (`internal/backup/backup.go`) + - **C2: SnapshotHistory reversed after unlock** — Moved snapshot reversal loop before `m.cachedStatus = status` (inside the lock). Previously reversed after `Unlock()`, so `m.cachedStatus.SnapshotHistory` was reversed without protection. (`internal/backup/backup.go`) + - **C3: SetStackProvider write without lock** — `m.stackProvider = provider` now wrapped in `m.mu.Lock()`. Read by `resolveAppBackupPaths()` concurrently. (`internal/backup/backup.go`) + - **C4: GetFullStatus shallow-copies mutable pointers** — `LastDBDump` and `LastBackup` are now deep-copied (struct + Results slice) so callers cannot mutate shared manager state. (`internal/backup/backup.go`) + - **C5: IsSystemDisk 8-bit major mask** — Replaced `>> 8 & 0xff` with `unix.Major()`/`unix.Minor()` (12-bit extraction). Also compares disk-portion of minor (groups of 16) to correctly distinguish physical disks of the same type. Adds `golang.org/x/sys/unix` import. (`internal/storage/safety_linux.go`) + - **C6: No /dev/ prefix validation on DevicePath** — `FormatAndMount` now validates `DevicePath` starts with `/dev/` and does not contain `..` before any disk operations. (`internal/storage/format_linux.go`) + - **C7: Path traversal in extractName** — `extractName()` now rejects empty string, `.`, `..`, and names containing `/` or `\`. (`internal/api/router.go`) + - **C8: Path traversal in TargetPath** — Migration API validates `TargetPath` against registered storage paths from settings before starting migration job. (`internal/web/storage_handlers.go`) + - **C9: Path traversal in DestinationPath** — Cross-drive backup config API validates `DestinationPath` against registered storage paths when `enabled=true`. (`internal/api/router.go`) + - **C10: Path traversal in ParseComposeHDDMounts** — `filepath.Clean()` applied before prefix check; uses separator-aware check `cleanHDD + string(filepath.Separator)` to prevent `${HDD_PATH}/../../etc/passwd` escaping. (`internal/stacks/delete.go`) + + **HIGH fixes (logic errors, resource leaks):** + - **H1: ValidateDump reads entire file into memory** — Replaced `os.ReadFile` with `bufio.Scanner` reading line-by-line. 256KB per-line buffer prevents OOM on large (500MB+) SQL dumps during 5-min cache refresh. (`internal/backup/dbdump.go`) + - **H2/H3: Double du invocation per mount + no timeout** — Replaced `appDirSizeHuman()`+`appDirSizeBytes()` with single `appDirSize()` function using `exec.CommandContext` with 30s timeout. Halves subprocess calls per mount point. (`internal/backup/appdata.go`) + - **H4: Snapshot validation only checks first 100** — Replaced `ListSnapshots(100)` existence check with regex validation (`^[0-9a-f]{8,64}$`). Allows restoring any snapshot; `restic restore` returns a clear error for non-existent IDs. (`internal/backup/restore.go`) + - **H5: No pruning for cross-drive restic repos** — Added `pruneResticRepo()` called after each successful cross-drive restic backup (`forget --keep-daily 7 --keep-weekly 4 --prune`). Non-fatal — logs warning on failure. (`internal/backup/crossdrive.go`) + - **H6: Temp password file management** — Reorganized temp file lifecycle: close before deferred remove, remove-on-write-error cleanup. (`internal/backup/crossdrive.go`) + - **H7: dirSizeBytes swallows walk errors** — `filepath.Walk` callback now returns errors instead of `nil`, propagating permission/IO issues. (`internal/backup/crossdrive.go`) + - **H8: Non-atomic fstab write** — `AppendFstabEntry` now reads existing fstab, writes to `.tmp`, then atomically renames. Crash-safe. (`internal/storage/safety_linux.go`) + - **H9: IsDeviceMounted naive prefix matching** — After prefix check, next character must be digit (`0-9`) or `p` (partition marker). Prevents `/dev/sdb` matching `/dev/sdba`. (`internal/storage/safety_linux.go`) + - **H10: eMMC device mapping bug** — `partitionToParentDisk` now handles `mmcblk0p1 → mmcblk0` and `nvme0n1p1 → nvme0n1` patterns. Uses `LastIndex("p")` with digit-suffix check before falling back to `TrimRight("0-9")`. (`internal/storage/scan_linux.go`) + - **H11: Data race on bytesCopied in rsync error path** — Error return path in `runRsync` now reads `bytesCopied` under mutex lock. (`internal/storage/migrate.go`) + - **H13: Path prefix match without separator** — Migration source path check now uses `srcPath == req.CurrentHDDPath || strings.HasPrefix(srcPath, req.CurrentHDDPath+"/")`. Prevents `/mnt/hdd` matching `/mnt/hdd_backup/data`. (`internal/storage/migrate.go`) + - **H14: DeleteStack continues after failed compose down** — `docker compose down` failure now returns an error immediately, preventing deletion of files while containers are still running. (`internal/stacks/delete.go`) + - **H16: exec.Command("docker") without timeout** — `syncFileBrowserMounts()` now uses `exec.CommandContext` with 60s timeout. (`internal/web/handlers.go`) + - **H17: SetNotificationPrefs stores caller's pointer** — Deep-copies `NotificationPrefs` struct and `EnabledEvents` slice before storing. (`internal/settings/settings.go`) + - **H18: wipefs error silently discarded** — wipefs failure logged as warning via progress channel; continues (wipefs may not be installed). (`internal/storage/format_linux.go`) + - **H19: Orphaned fstab entry on mount failure** — New `RemoveFstabEntry()` function atomically removes UUID entry. Called as rollback on `mount` failure and `findmnt` verify failure. (`internal/storage/safety_linux.go`, `format_linux.go`) + + **MEDIUM fixes (edge cases, code quality):** + - **M1: formatBytes duplicate in dbdump.go** — Removed `formatBytes()` from `dbdump.go`; all callers (backup.go, restic.go, dbdump.go) now use `humanizeBytes()` from appdata.go. (`internal/backup/dbdump.go`, `backup.go`, `restic.go`) + - **M2: Dead code .tmp suffix check** — Reordered filter in `ListDumpFiles`: `.tmp` check now comes before `.sql` check to correctly skip `.sql.tmp` temp files (was unreachable before). (`internal/backup/dbdump.go`) + - **M3: sizeBytes() returns 0 for string types** — Added `case string:` to `sizeBytes()` using `strconv.ParseUint`. (`internal/storage/scan_linux.go`) + - **M6: Dead elapsed variable** — Removed `_ = elapsed`; elapsed time now shown inline in the "done" progress message. (`internal/storage/migrate.go`) + - **M7: time.LoadLocation error silently discarded** — Two locations in handlers.go now handle `LoadLocation` error, falling back to `time.UTC`. (`internal/web/handlers.go`) + - **M10: filterSnapshotsByPaths imprecise prefix** — Added `pathCovers()` helper using separator-aware prefix check. Prevents `/mnt/hdd_1` matching `/mnt/hdd_10/data`. (`internal/api/router.go`) + - **M11: XSS in editStorageLabel innerHTML** — `cancelEditLabel()` in settings.html now uses DOM manipulation (`document.createElement`, `.textContent`) instead of `innerHTML` for the label text. (`internal/web/templates/settings.html`) + + **Files modified (15):** `internal/backup/backup.go`, `internal/backup/appdata.go`, `internal/backup/dbdump.go`, `internal/backup/restore.go`, `internal/backup/crossdrive.go`, `internal/backup/restic.go`, `internal/storage/safety_linux.go`, `internal/storage/format_linux.go`, `internal/storage/scan_linux.go`, `internal/storage/migrate.go`, `internal/stacks/delete.go`, `internal/api/router.go`, `internal/web/handlers.go`, `internal/web/storage_handlers.go`, `internal/settings/settings.go`, `internal/web/templates/settings.html` + ### What was just completed (2026-02-17 session 38) - **v0.12.2 — Restore Section Simplification (Bug 4 from v0.12.1 TASK.md):** - **Feature: Snapshot filtering by app** — `GET /api/backup/snapshots?stack={name}` now filters snapshots to those whose `Paths` overlap with the app's HDD mount paths. Uses prefix matching (snapshot path is prefix of required, or vice versa). New `filterSnapshotsByPaths()` helper in `internal/api/router.go`. Manager gains `GetStackHDDMounts()` method to expose stackProvider's mount resolution. diff --git a/controller/internal/api/router.go b/controller/internal/api/router.go index d680e16..ac4c098 100644 --- a/controller/internal/api/router.go +++ b/controller/internal/api/router.go @@ -473,13 +473,14 @@ func (r *Router) backupSnapshots(w http.ResponseWriter, req *http.Request) { // filterSnapshotsByPaths returns only snapshots whose Paths overlap with requiredPaths. // A snapshot matches if any of its paths is a prefix of (or prefixed by) any required path. +// M10: Uses separator-aware prefix check to prevent /mnt/hdd_1 matching /mnt/hdd_10/data. func filterSnapshotsByPaths(snapshots []backup.SnapshotInfo, requiredPaths []string) []backup.SnapshotInfo { var filtered []backup.SnapshotInfo outer: for _, snap := range snapshots { for _, required := range requiredPaths { for _, sp := range snap.Paths { - if strings.HasPrefix(required, sp) || strings.HasPrefix(sp, required) { + if pathCovers(required, sp) || pathCovers(sp, required) { filtered = append(filtered, snap) continue outer } @@ -489,6 +490,14 @@ outer: return filtered } +// pathCovers returns true if base is equal to or a directory-prefix of target. +func pathCovers(base, target string) bool { + if base == target { + return true + } + return strings.HasPrefix(target, strings.TrimRight(base, "/")+"/") +} + // --- Metrics handlers --- func (r *Router) metricsSystem(w http.ResponseWriter, req *http.Request) { @@ -612,6 +621,21 @@ func (r *Router) saveCrossBackupConfig(w http.ResponseWriter, req *http.Request, writeJSON(w, http.StatusBadRequest, apiResponse{OK: false, Error: "schedule must be 'daily', 'weekly', or 'manual'"}) return } + // C9: Validate DestinationPath against registered storage paths to prevent path traversal. + if body.Enabled && body.DestinationPath != "" { + registeredPaths := r.sett.GetStoragePaths() + validDest := false + for _, sp := range registeredPaths { + if body.DestinationPath == sp.Path { + validDest = true + break + } + } + if !validDest { + writeJSON(w, http.StatusBadRequest, apiResponse{OK: false, Error: "destination_path must be a registered storage path"}) + return + } + } // Preserve existing runtime status existing := r.sett.GetCrossDriveConfig(name) @@ -768,7 +792,12 @@ func trimSegment(path, prefix string) string { func extractName(path, suffix string) string { s := strings.TrimPrefix(path, "/stacks/") - return strings.TrimSuffix(s, suffix) + name := strings.TrimSuffix(s, suffix) + // C7: Reject path traversal characters — name is used in file paths and Docker commands. + if name == "" || name == "." || name == ".." || strings.ContainsAny(name, "/\\") { + return "" + } + return name } func writeJSON(w http.ResponseWriter, status int, v interface{}) { diff --git a/controller/internal/backup/appdata.go b/controller/internal/backup/appdata.go index 6b9e6fd..610ec1f 100644 --- a/controller/internal/backup/appdata.go +++ b/controller/internal/backup/appdata.go @@ -1,10 +1,12 @@ package backup import ( + "context" "fmt" "os" "os/exec" "strings" + "time" "gopkg.in/yaml.v3" ) @@ -76,8 +78,7 @@ func DiscoverAppData(provider StackDataProvider, backupPrefs map[string]bool, di path := AppDataPath{HostPath: mount} if fi, err := os.Stat(mount); err == nil && fi.IsDir() { path.Exists = true - path.SizeBytes = appDirSizeBytes(mount) - path.SizeHuman = appDirSizeHuman(mount) + path.SizeBytes, path.SizeHuman = appDirSize(mount) } info.HDDPaths = append(info.HDDPaths, path) info.HDDTotalSize += path.SizeBytes @@ -131,34 +132,23 @@ func parseComposeNamedVolumes(composePath string) []AppDockerVolume { return volumes } -// appDirSizeHuman returns a human-readable size string for a directory using du. -func appDirSizeHuman(path string) string { - cmd := exec.Command("du", "-sh", path) +// appDirSize returns the total byte count and a human-readable string for a directory. +// H2/H3: Single du invocation with 30s timeout replaces two separate calls. +func appDirSize(path string) (int64, string) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "du", "-sb", path) output, err := cmd.Output() if err != nil { - return "?" + return 0, "?" } fields := strings.Fields(string(output)) - if len(fields) > 0 { - return fields[0] + if len(fields) == 0 { + return 0, "?" } - return "?" -} - -// appDirSizeBytes returns the total size in bytes for a directory. -func appDirSizeBytes(path string) int64 { - cmd := exec.Command("du", "-sb", path) - output, err := cmd.Output() - if err != nil { - return 0 - } - fields := strings.Fields(string(output)) - if len(fields) > 0 { - var size int64 - fmt.Sscanf(fields[0], "%d", &size) - return size - } - return 0 + var size int64 + fmt.Sscanf(fields[0], "%d", &size) + return size, humanizeBytes(size) } // humanizeBytes converts bytes to a human-readable string. diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index 096e09d..bc3ce61 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -179,7 +179,7 @@ func (m *Manager) RunDBDumps(ctx context.Context) error { m.logger.Printf("[ERROR] DB dump failed for %s: %v", r.DB.ContainerName, r.Error) } else { totalSize += r.Size - summary = append(summary, fmt.Sprintf("OK %s (%s)", r.DB.ContainerName, formatBytes(r.Size))) + summary = append(summary, fmt.Sprintf("OK %s (%s)", r.DB.ContainerName, humanizeBytes(r.Size))) // Persist validation result to settings.json if m.settings != nil && r.FilePath != "" { @@ -212,12 +212,12 @@ func (m *Manager) RunDBDumps(ctx context.Context) error { // Ping healthcheck uuid := m.cfg.Monitoring.PingUUIDs.DBDump body := fmt.Sprintf("DB dump: %d databases, %s total\n%s", - len(results), formatBytes(totalSize), strings.Join(summary, "\n")) + len(results), humanizeBytes(totalSize), strings.Join(summary, "\n")) if allOK { m.pinger.Ping(uuid, body) m.logger.Printf("[INFO] DB dump completed: %d databases, %s total (%s)", - len(results), formatBytes(totalSize), duration.Round(time.Millisecond)) + len(results), humanizeBytes(totalSize), duration.Round(time.Millisecond)) } else { m.pinger.Fail(uuid, body) return fmt.Errorf("some database dumps failed") @@ -410,8 +410,11 @@ func (m *Manager) ListSnapshots(limit int) ([]SnapshotInfo, error) { } // SetStackProvider sets the stack data provider for app data discovery. +// C3: Write is protected by mutex since stackProvider is read by concurrent goroutines. func (m *Manager) SetStackProvider(provider StackDataProvider) { + m.mu.Lock() m.stackProvider = provider + m.mu.Unlock() } // GetStackHDDMounts returns HDD mount paths for the named stack via the stack provider. @@ -551,8 +554,19 @@ func (m *Manager) RefreshCache(nextDBDump, nextBackup time.Time) { } } - // Cross-check: if LastDBDump results have empty validation but files exist, - // re-validate from disk. This handles controller restarts and race conditions. + // Fill in dynamic fields under lock. + // C1: lastDBDump mutation also happens here to prevent data races with GetFullStatus. + // C2: snapshot history reversal happens before cachedStatus assignment (inside lock). + m.mu.Lock() + status.Running = m.running + status.LastDBDump = m.lastDBDump + status.LastBackup = m.lastBackup + status.LastCheckTime = m.lastCheckTime + status.LastCheckOK = m.lastCheckOK + status.SnapshotHistory = make([]SnapshotRecord, len(m.snapshotHistory)) + copy(status.SnapshotHistory, m.snapshotHistory) + + // C1: Cross-check lastDBDump results inside lock to prevent torn writes. if m.lastDBDump != nil && filesErr == nil { fileValidation := make(map[string]DumpValidation) // keyed by filename for _, f := range files { @@ -570,24 +584,15 @@ func (m *Manager) RefreshCache(nextDBDump, nextBackup time.Time) { } } - // Fill in dynamic fields under lock - m.mu.Lock() - status.Running = m.running - status.LastDBDump = m.lastDBDump - status.LastBackup = m.lastBackup - status.LastCheckTime = m.lastCheckTime - status.LastCheckOK = m.lastCheckOK - status.SnapshotHistory = make([]SnapshotRecord, len(m.snapshotHistory)) - copy(status.SnapshotHistory, m.snapshotHistory) - m.cachedStatus = status - m.cacheTime = time.Now() - m.mu.Unlock() - - // Reverse so newest first + // C2: Reverse snapshot history before assigning to cachedStatus (inside lock). for i, j := 0, len(status.SnapshotHistory)-1; i < j; i, j = i+1, j-1 { status.SnapshotHistory[i], status.SnapshotHistory[j] = status.SnapshotHistory[j], status.SnapshotHistory[i] } + m.cachedStatus = status + m.cacheTime = time.Now() + m.mu.Unlock() + m.logger.Printf("[INFO] Backup status cache refreshed") } @@ -616,8 +621,19 @@ func (m *Manager) GetFullStatus(nextDBDump, nextBackup time.Time) *FullBackupSta status.Running = m.running status.NextDBDump = nextDBDump status.NextBackup = nextBackup - status.LastDBDump = m.lastDBDump - status.LastBackup = m.lastBackup + // C4: Deep-copy lastDBDump and lastBackup so callers cannot mutate shared state. + if m.lastDBDump != nil { + copyDump := *m.lastDBDump + if len(m.lastDBDump.Results) > 0 { + copyDump.Results = make([]DumpResult, len(m.lastDBDump.Results)) + copy(copyDump.Results, m.lastDBDump.Results) + } + status.LastDBDump = ©Dump + } + if m.lastBackup != nil { + copyBackup := *m.lastBackup + status.LastBackup = ©Backup + } // Update snapshot history status.SnapshotHistory = make([]SnapshotRecord, len(m.snapshotHistory)) copy(status.SnapshotHistory, m.snapshotHistory) diff --git a/controller/internal/backup/crossdrive.go b/controller/internal/backup/crossdrive.go index a286623..62058eb 100644 --- a/controller/internal/backup/crossdrive.go +++ b/controller/internal/backup/crossdrive.go @@ -227,27 +227,29 @@ func (r *CrossDriveRunner) runResticBackup(ctx context.Context, stackName, destB return fmt.Errorf("getting restic password: %w", err) } - // Write password to a temp file (restic requires --password-file or env var) + // H6: Write password to temp file with safe cleanup order (close before deferred remove). pwFile, err := os.CreateTemp("", "felhom-crossdrive-pw-*") if err != nil { return fmt.Errorf("creating password file: %w", err) } - defer os.Remove(pwFile.Name()) + pwPath := pwFile.Name() if _, err := pwFile.WriteString(password); err != nil { pwFile.Close() + os.Remove(pwPath) return fmt.Errorf("writing password file: %w", err) } pwFile.Close() + defer os.Remove(pwPath) // Ensure repo is initialized - if err := r.ensureResticRepo(ctx, repoPath, pwFile.Name()); err != nil { + if err := r.ensureResticRepo(ctx, repoPath, pwPath); err != nil { return err } // Run restic backup args := []string{ "backup", "--repo", repoPath, - "--password-file", pwFile.Name(), + "--password-file", pwPath, "--tag", stackName, "--tag", "cross-drive", } @@ -258,6 +260,26 @@ func (r *CrossDriveRunner) runResticBackup(ctx context.Context, stackName, destB if out, err := cmd.CombinedOutput(); err != nil { return fmt.Errorf("restic backup failed: %v (%s)", err, strings.TrimSpace(string(out))) } + + // H5: Prune old snapshots to prevent unbounded accumulation. + return r.pruneResticRepo(ctx, repoPath, pwPath) +} + +// pruneResticRepo forgets old snapshots in a cross-drive restic repo, keeping recent ones. +func (r *CrossDriveRunner) pruneResticRepo(ctx context.Context, repoPath, pwPath string) error { + args := []string{ + "forget", "--repo", repoPath, + "--password-file", pwPath, + "--keep-daily", "7", + "--keep-weekly", "4", + "--prune", + } + cmd := exec.CommandContext(ctx, "restic", args...) + r.logger.Printf("[DEBUG] restic forget (prune): %s", repoPath) + if out, err := cmd.CombinedOutput(); err != nil { + // Non-fatal: log warning but don't fail the backup + r.logger.Printf("[WARN] restic forget failed for %s: %v (%s)", repoPath, err, strings.TrimSpace(string(out))) + } return nil } @@ -294,13 +316,16 @@ func (r *CrossDriveRunner) updateStatus(stackName, status, errMsg string, durati } // dirSizeBytes returns the total byte size of all files under path. +// H7: Walk errors are now propagated instead of silently swallowed. func dirSizeBytes(path string) (int64, error) { var total int64 err := filepath.Walk(path, func(_ string, info os.FileInfo, err error) error { - if err != nil || info.IsDir() { - return nil + if err != nil { + return err // propagate permission/IO errors + } + if !info.IsDir() { + total += info.Size() } - total += info.Size() return nil }) return total, err diff --git a/controller/internal/backup/dbdump.go b/controller/internal/backup/dbdump.go index d8c19bc..7f23da9 100644 --- a/controller/internal/backup/dbdump.go +++ b/controller/internal/backup/dbdump.go @@ -1,6 +1,7 @@ package backup import ( + "bufio" "context" "fmt" "log" @@ -224,7 +225,7 @@ func DumpOne(ctx context.Context, db DiscoveredDB, dumpDir string, logger *log.L result.Validation = ValidateDump(finalPath, db.DBType) logger.Printf("[INFO] DB dump: %s → %s (%s, %s, %d tables)", db.ContainerName, filename, - formatBytes(stat.Size()), result.Duration.Round(time.Millisecond), result.Validation.TableCount) + humanizeBytes(stat.Size()), result.Duration.Round(time.Millisecond), result.Validation.TableCount) return result } @@ -248,18 +249,45 @@ func ValidateDump(filePath string, dbType DBType) DumpValidation { return v } - data, err := os.ReadFile(filePath) + // H1: Use bufio.Scanner to read line-by-line instead of loading entire file into memory. + // Large dumps (500MB+) would cause massive allocations on every 5-min cache refresh. + f, err := os.Open(filePath) if err != nil { v.Error = fmt.Sprintf("read failed: %v", err) log.Printf("[WARN] ValidateDump FAIL: %s — %s", filePath, v.Error) return v } + defer f.Close() - content := string(data) + scanner := bufio.NewScanner(f) + // Increase token buffer for very long lines (some SQL lines can be large) + scanner.Buffer(make([]byte, 256*1024), 256*1024) - // Count CREATE TABLE statements + lineNum := 0 + headerFound := false tableCount := 0 - for _, line := range strings.Split(content, "\n") { + for scanner.Scan() { + line := scanner.Text() + lineNum++ + + // Header check — scan first 10 lines for expected dump header + // MariaDB 11.4+ prepends a sandbox comment before the header line + if lineNum <= 10 && !headerFound { + switch dbType { + case DBTypeMariaDB: + if strings.HasPrefix(line, "-- MariaDB dump") || + strings.HasPrefix(line, "-- MySQL dump") || + strings.HasPrefix(line, "-- mysqldump") { + headerFound = true + } + case DBTypePostgres: + if strings.HasPrefix(line, "-- PostgreSQL database dump") { + headerFound = true + } + } + } + + // Count CREATE TABLE statements upper := strings.ToUpper(strings.TrimSpace(line)) if strings.HasPrefix(upper, "CREATE TABLE") { tableCount++ @@ -267,30 +295,6 @@ func ValidateDump(filePath string, dbType DBType) DumpValidation { } v.TableCount = tableCount - // Header check — scan first 10 lines for expected dump header - // MariaDB 11.4+ prepends a sandbox comment before the header line - headerFound := false - lines := strings.SplitN(content, "\n", 11) // at most 11 parts = 10 lines - for i, line := range lines { - if i >= 10 { - break - } - switch dbType { - case DBTypeMariaDB: - if strings.HasPrefix(line, "-- MariaDB dump") || - strings.HasPrefix(line, "-- MySQL dump") || - strings.HasPrefix(line, "-- mysqldump") { - headerFound = true - } - case DBTypePostgres: - if strings.HasPrefix(line, "-- PostgreSQL database dump") { - headerFound = true - } - } - if headerFound { - break - } - } if !headerFound { switch dbType { case DBTypeMariaDB: @@ -304,7 +308,7 @@ func ValidateDump(filePath string, dbType DBType) DumpValidation { if tableCount == 0 { v.Error = "no CREATE TABLE statements found" - log.Printf("[WARN] ValidateDump FAIL: %s — %s (header was found, scanned %d lines)", filePath, v.Error, len(strings.Split(content, "\n"))) + log.Printf("[WARN] ValidateDump FAIL: %s — %s (header was found, scanned %d lines)", filePath, v.Error, lineNum) return v } @@ -325,10 +329,11 @@ func ListDumpFiles(dumpDir string) ([]DumpFileInfo, error) { var files []DumpFileInfo for _, e := range entries { - if e.IsDir() || !strings.HasSuffix(e.Name(), ".sql") { + // M2: Check .tmp before .sql to correctly skip ".sql.tmp" temp files (was dead code before). + if e.IsDir() || strings.HasSuffix(e.Name(), ".tmp") { continue } - if strings.HasSuffix(e.Name(), ".tmp") { + if !strings.HasSuffix(e.Name(), ".sql") { continue } @@ -464,20 +469,4 @@ func cleanupTmpFiles(dumpDir string, logger *log.Logger) { } } -func formatBytes(b int64) string { - const ( - kb = 1024 - mb = 1024 * kb - gb = 1024 * mb - ) - switch { - case b >= gb: - return fmt.Sprintf("%.1f GB", float64(b)/float64(gb)) - case b >= mb: - return fmt.Sprintf("%.1f MB", float64(b)/float64(mb)) - case b >= kb: - return fmt.Sprintf("%.1f KB", float64(b)/float64(kb)) - default: - return fmt.Sprintf("%d B", b) - } -} +// M1: formatBytes removed — use humanizeBytes() from appdata.go (same package, no duplication). diff --git a/controller/internal/backup/restic.go b/controller/internal/backup/restic.go index 54f76a3..d6a931d 100644 --- a/controller/internal/backup/restic.go +++ b/controller/internal/backup/restic.go @@ -173,7 +173,7 @@ func (r *ResticManager) Snapshot(paths []string, tags []string) (*SnapshotResult result.SnapshotID = msg.SnapshotID result.FilesNew = msg.FilesNew result.FilesChanged = msg.FilesChanged - result.DataAdded = formatBytes(msg.DataAdded) + result.DataAdded = humanizeBytes(msg.DataAdded) } } @@ -282,7 +282,7 @@ func (r *ResticManager) Stats() (*RepoStats, error) { TotalSize uint64 `json:"total_size"` } if json.Unmarshal(out, &raw) == nil { - stats.TotalSize = formatBytes(int64(raw.TotalSize)) + stats.TotalSize = humanizeBytes(int64(raw.TotalSize)) } } diff --git a/controller/internal/backup/restore.go b/controller/internal/backup/restore.go index 0c00cd2..58c2403 100644 --- a/controller/internal/backup/restore.go +++ b/controller/internal/backup/restore.go @@ -1,6 +1,12 @@ package backup -import "fmt" +import ( + "fmt" + "regexp" +) + +// snapshotIDRe validates restic snapshot IDs: 8-64 lowercase hex characters. +var snapshotIDRe = regexp.MustCompile(`^[0-9a-f]{8,64}$`) // RestoreApp restores an app's HDD data from a restic snapshot. func (m *Manager) RestoreApp(stackName, snapshotID string) error { @@ -18,20 +24,10 @@ func (m *Manager) RestoreApp(stackName, snapshotID string) error { return fmt.Errorf("no HDD data paths found for %s", stackName) } - // Validate snapshot exists - snapshots, err := m.restic.ListSnapshots(100) - if err != nil { - return fmt.Errorf("listing snapshots: %w", err) - } - found := false - for _, s := range snapshots { - if s.ID == snapshotID { - found = true - break - } - } - if !found { - return fmt.Errorf("snapshot %s not found", snapshotID) + // H4: Validate snapshot ID format by regex instead of listing all snapshots (list caps at 100). + // restic restore will return a clear error if the snapshot ID doesn't exist. + if !snapshotIDRe.MatchString(snapshotID) { + return fmt.Errorf("invalid snapshot ID: must be 8-64 lowercase hex characters") } // Use the running flag to prevent concurrent backup/restore diff --git a/controller/internal/settings/settings.go b/controller/internal/settings/settings.go index 7559838..d896af3 100644 --- a/controller/internal/settings/settings.go +++ b/controller/internal/settings/settings.go @@ -213,10 +213,18 @@ func (s *Settings) GetNotificationPrefs() *NotificationPrefs { } // SetNotificationPrefs updates notification preferences and saves to disk. +// H17: Deep-copies prefs so caller mutations after the call don't affect stored state. func (s *Settings) SetNotificationPrefs(prefs *NotificationPrefs) error { s.mu.Lock() defer s.mu.Unlock() - s.Notifications = prefs + copy := *prefs + if len(prefs.EnabledEvents) > 0 { + copy.EnabledEvents = make([]string, len(prefs.EnabledEvents)) + for i, e := range prefs.EnabledEvents { + copy.EnabledEvents[i] = e + } + } + s.Notifications = © return s.save() } diff --git a/controller/internal/stacks/delete.go b/controller/internal/stacks/delete.go index 3ace0fe..d629fd4 100644 --- a/controller/internal/stacks/delete.go +++ b/controller/internal/stacks/delete.go @@ -84,11 +84,12 @@ func (m *Manager) DeleteStack(name string, removeHDDData bool) (*DeleteResponse, hddMounts := ParseComposeHDDMounts(stack.ComposePath, hddPath) // Step 2: Run docker compose down --rmi local --volumes + // H14: Return error if docker compose down fails — continuing would leave orphaned containers. env := m.stackEnv(stackDir) output, err := m.composeExecCustomEnv(stackDir, env, "down", "--rmi", "local", "--volumes") if err != nil { - m.logger.Printf("[WARN] docker compose down for %s had errors: %v (output: %s)", name, err, truncateStr(output, 200)) - // Continue anyway — the stack dir will be removed + m.logger.Printf("[ERROR] docker compose down for %s failed: %v (output: %s)", name, err, truncateStr(output, 200)) + return resp, fmt.Errorf("docker compose down failed for %s: %w", name, err) } // Step 3: Identify removed volumes from compose output @@ -244,12 +245,14 @@ func ParseComposeHDDMounts(composePath, hddPath string) []string { // Resolve ${HDD_PATH} variable reference hostPath = strings.ReplaceAll(hostPath, "${HDD_PATH}", hddPath) - // Check if this is an HDD mount - if !strings.HasPrefix(hostPath, hddPath) { + // C10: Clean path BEFORE prefix check to prevent traversal like ${HDD_PATH}/../../etc/passwd. + cleanPath := filepath.Clean(hostPath) + cleanHDD := filepath.Clean(hddPath) + + // Check if this is an HDD mount (must be cleanHDD itself or a direct subpath) + if cleanPath != cleanHDD && !strings.HasPrefix(cleanPath, cleanHDD+string(filepath.Separator)) { continue } - - cleanPath := filepath.Clean(hostPath) if !seen[cleanPath] { seen[cleanPath] = true mounts = append(mounts, cleanPath) diff --git a/controller/internal/storage/format_linux.go b/controller/internal/storage/format_linux.go index 886c0a5..c490760 100644 --- a/controller/internal/storage/format_linux.go +++ b/controller/internal/storage/format_linux.go @@ -36,6 +36,13 @@ func FormatAndMount(req FormatRequest, progress chan<- FormatProgress) (string, if err := ValidateMountName(req.MountName); err != nil { return "", fail("validating", "Érvénytelen csatlakoztatási név", err) } + // C6: Validate DevicePath to prevent path traversal from user-supplied input. + if !strings.HasPrefix(req.DevicePath, "/dev/") { + return "", fail("validating", "Érvénytelen eszközútvonal: /dev/-vel kell kezdődnie", fmt.Errorf("invalid device path: must start with /dev/")) + } + if strings.Contains(req.DevicePath, "..") { + return "", fail("validating", "Érvénytelen eszközútvonal: nem tartalmazhat ..-t", fmt.Errorf("invalid device path: must not contain ..")) + } if _, err := os.Stat(HostDevicePath(req.DevicePath)); err != nil { return "", fail("validating", "Az eszköz nem létezik: "+req.DevicePath, err) } @@ -70,8 +77,12 @@ func FormatAndMount(req FormatRequest, progress chan<- FormatProgress) (string, partDev := req.DevicePath if req.CreatePartition { // Wipe existing partition table and filesystem signatures first + // H18: Log wipefs errors instead of silently discarding them. send("partitioning", fmt.Sprintf("wipefs -a %s ...", HostDevicePath(req.DevicePath)), 12) - _ = exec.Command("wipefs", "-a", HostDevicePath(req.DevicePath)).Run() + if err := exec.Command("wipefs", "-a", HostDevicePath(req.DevicePath)).Run(); err != nil { + // Non-fatal: some systems don't have wipefs; continue anyway + send("partitioning", fmt.Sprintf("[WARN] wipefs sikertelen %s: %v (folytatás)", req.DevicePath, err), 13) + } time.Sleep(500 * time.Millisecond) // Create GPT with single partition spanning whole disk. @@ -146,12 +157,16 @@ func FormatAndMount(req FormatRequest, progress chan<- FormatProgress) (string, send("mounting", fmt.Sprintf("mount -t ext4 %s %s ...", HostDevicePath(partDev), mountPath), 70) if out, err := exec.Command("mount", "-t", "ext4", "-o", "defaults,noatime", HostDevicePath(partDev), mountPath).CombinedOutput(); err != nil { + // H19: Roll back fstab entry to prevent orphaned entry that hangs system on reboot. + _ = RemoveFstabEntry(FstabPath, uuid) return "", fail("mounting", "Csatlakoztatás sikertelen: "+string(out), err) } // Verify mount actually worked (don't just trust exit code) verifyOut, verifyErr := exec.Command("findmnt", "-n", "-o", "SOURCE", "--target", mountPath).Output() if verifyErr != nil || strings.TrimSpace(string(verifyOut)) == "" { + // H19: Also roll back fstab if mount verify fails. + _ = RemoveFstabEntry(FstabPath, uuid) return "", fail("mounting", "A csatlakoztatás nem ellenőrizhető: mount sikerült, de a meghajtó nem látható", fmt.Errorf("mount point %s not found after mount", mountPath)) } diff --git a/controller/internal/storage/migrate.go b/controller/internal/storage/migrate.go index 57e5841..dd2c4fb 100644 --- a/controller/internal/storage/migrate.go +++ b/controller/internal/storage/migrate.go @@ -124,8 +124,9 @@ func MigrateAppData( // --- Step 3: rsync --- var bytesCopied int64 for i, srcPath := range req.HDDMounts { - // Determine destination path: replace CurrentHDDPath prefix with TargetPath - if !strings.HasPrefix(srcPath, req.CurrentHDDPath) { + // Determine destination path: replace CurrentHDDPath prefix with TargetPath. + // H13: Require trailing separator to prevent /mnt/hdd matching /mnt/hdd_backup/data. + if srcPath != req.CurrentHDDPath && !strings.HasPrefix(srcPath, req.CurrentHDDPath+"/") { continue } relPath := strings.TrimPrefix(srcPath, req.CurrentHDDPath) @@ -173,11 +174,10 @@ func MigrateAppData( return fail("starting", "Alkalmazás indítása sikertelen az új tárolóról", err) } - elapsed := int(time.Since(start).Seconds()) send("done", - fmt.Sprintf("Áthelyezés kész! Az alkalmazás az új tárolóról fut. (Régi adat: %s)", req.CurrentHDDPath), + fmt.Sprintf("Áthelyezés kész! Az alkalmazás az új tárolóról fut. (Régi adat: %s, idő: %ds)", + req.CurrentHDDPath, int(time.Since(start).Seconds())), 100, bytesCopied, totalBytes) - _ = elapsed return nil } @@ -240,7 +240,11 @@ func runRsync(srcPath, dstPath string, totalBytes, prevCopied int64, basePct int io.Copy(&stderrBuf, stderr) if err := cmd.Wait(); err != nil { - return bytesCopied, fmt.Errorf("rsync failed: %w — %s", err, stderrBuf.String()) + // H11: Read bytesCopied under lock to avoid data race with the progress goroutine. + mu.Lock() + copied := bytesCopied + mu.Unlock() + return copied, fmt.Errorf("rsync failed: %w — %s", err, stderrBuf.String()) } mu.Lock() diff --git a/controller/internal/storage/safety_linux.go b/controller/internal/storage/safety_linux.go index 04022d3..8616d25 100644 --- a/controller/internal/storage/safety_linux.go +++ b/controller/internal/storage/safety_linux.go @@ -9,6 +9,8 @@ import ( "strings" "syscall" "time" + + "golang.org/x/sys/unix" ) // IsSystemDisk checks if the given device path overlaps with the root filesystem device. @@ -26,14 +28,22 @@ func IsSystemDisk(devicePath string) (bool, error) { return false, fmt.Errorf("cannot stat %s: %w", devicePath, err) } - // Compare major device numbers - rootMajor := rootStat.Dev >> 8 & 0xff - devMajor := devStat.Rdev >> 8 & 0xff - if rootMajor == devMajor { - return true, nil - } + // C5: Use unix.Major/Minor for correct 12-bit extraction (old 0xff mask truncated high bits). + // Also compare the disk portion of the minor to distinguish separate physical disks of the + // same type (e.g., sda and sdb both have major 8, but different disk-minor groups of 16). + rootMajor := unix.Major(rootStat.Dev) + rootMinor := unix.Minor(rootStat.Dev) + devMajor := unix.Major(devStat.Rdev) + devMinor := unix.Minor(devStat.Rdev) - return false, nil + if rootMajor != devMajor { + return false, nil + } + // Same major — compare disk groups (each disk gets 16 minor numbers on SCSI/SATA, + // e.g., sda=0-15, sdb=16-31; NVMe uses similar grouping). + rootDiskGroup := rootMinor / 16 + devDiskGroup := devMinor / 16 + return rootDiskGroup == devDiskGroup, nil } // IsDeviceMounted checks if a device or any of its partitions is currently mounted. @@ -51,9 +61,17 @@ func IsDeviceMounted(devicePath string) (bool, error) { } dev := fields[0] devBase := filepath.Base(dev) - if devBase == base || strings.HasPrefix(devBase, base) { + // H9: Require exact match or that the suffix after base is a digit or 'p' (partition marker). + // Prevents /dev/sdb matching /dev/sdba (hypothetical device) or /dev/sdb_backup (bind). + if devBase == base { return true, nil } + if strings.HasPrefix(devBase, base) { + next := devBase[len(base)] + if next >= '0' && next <= '9' || next == 'p' { + return true, nil + } + } } return false, nil } @@ -87,16 +105,53 @@ func BackupFstab(fstabPath string) error { return os.WriteFile(backupPath, data, 0644) } -// AppendFstabEntry appends a UUID-based fstab entry. +// AppendFstabEntry appends a UUID-based fstab entry atomically (write tmp + rename). +// H8: Direct write to /etc/fstab risks corruption on crash — use atomic write pattern. func AppendFstabEntry(fstabPath, uuid, mountPoint, fsType, options string) error { - entry := fmt.Sprintf("\nUUID=%s\t%s\t%s\t%s\t0 2\n", uuid, mountPoint, fsType, options) - f, err := os.OpenFile(fstabPath, os.O_APPEND|os.O_WRONLY, 0644) - if err != nil { - return fmt.Errorf("cannot open fstab for writing: %w", err) + // Read existing content + existing, err := os.ReadFile(fstabPath) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("cannot read fstab: %w", err) } - defer f.Close() - if _, err := f.WriteString(entry); err != nil { - return fmt.Errorf("cannot write fstab entry: %w", err) + + entry := fmt.Sprintf("\nUUID=%s\t%s\t%s\t%s\t0 2\n", uuid, mountPoint, fsType, options) + newContent := append(existing, []byte(entry)...) + + // Write to .tmp then rename — atomic on same filesystem + tmpPath := fstabPath + ".tmp" + if err := os.WriteFile(tmpPath, newContent, 0644); err != nil { + return fmt.Errorf("cannot write fstab tmp file: %w", err) + } + if err := os.Rename(tmpPath, fstabPath); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("cannot rename fstab tmp file: %w", err) + } + return nil +} + +// RemoveFstabEntry removes any line containing the given UUID from fstab, atomically. +// H19: Called as rollback if mount fails after fstab was written. +func RemoveFstabEntry(fstabPath, uuid string) error { + data, err := os.ReadFile(fstabPath) + if err != nil { + return fmt.Errorf("cannot read fstab: %w", err) + } + + var kept []string + for _, line := range strings.Split(string(data), "\n") { + if !strings.Contains(line, "UUID="+uuid) { + kept = append(kept, line) + } + } + newContent := strings.Join(kept, "\n") + + tmpPath := fstabPath + ".tmp" + if err := os.WriteFile(tmpPath, []byte(newContent), 0644); err != nil { + return fmt.Errorf("cannot write fstab tmp file: %w", err) + } + if err := os.Rename(tmpPath, fstabPath); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("cannot rename fstab tmp file: %w", err) } return nil } diff --git a/controller/internal/storage/scan_linux.go b/controller/internal/storage/scan_linux.go index 9428a0e..f9abca3 100644 --- a/controller/internal/storage/scan_linux.go +++ b/controller/internal/storage/scan_linux.go @@ -34,6 +34,10 @@ func (d *lsblkDevice) sizeBytes() int64 { switch v := d.Size.(type) { case float64: return int64(v) + case string: + // M3: lsblk can return size as a string on some kernel versions. + n, _ := strconv.ParseUint(v, 10, 64) + return int64(n) } return 0 } @@ -157,18 +161,29 @@ func getSystemDiskNames() map[string]bool { } // partitionToParentDisk extracts the parent disk name from a partition device path. -// "/dev/sda2" → "sda", "/dev/nvme0n1p2" → "nvme0n1" +// "/dev/sda2" → "sda", "/dev/nvme0n1p2" → "nvme0n1", "/dev/mmcblk0p1" → "mmcblk0" func partitionToParentDisk(devPath string) string { name := filepath.Base(devPath) - // NVMe: nvme0n1p2 → nvme0n1 - if strings.Contains(name, "nvme") { - if idx := strings.LastIndex(name, "p"); idx > 0 { - if _, err := strconv.Atoi(name[idx+1:]); err == nil { - return name[:idx] + // H10: Handle mmcblk0p1 and nvme0n1p1 patterns where 'p' separates disk# from partition#. + // The prefix before 'p' must end with a digit (e.g., mmcblk0, nvme0n1) to be a disk number. + if idx := strings.LastIndex(name, "p"); idx > 0 { + prefix := name[:idx] + suffix := name[idx+1:] + if len(suffix) > 0 && suffix[0] >= '0' && suffix[0] <= '9' && + len(prefix) > 0 && prefix[len(prefix)-1] >= '0' && prefix[len(prefix)-1] <= '9' { + // Verify suffix is all digits (partition number, not part of device name) + allDigits := true + for _, c := range suffix { + if c < '0' || c > '9' { + allDigits = false + break + } + } + if allDigits { + return prefix // e.g., mmcblk0, nvme0n1 } } - return name } // Standard: sda2 → sda, sdb1 → sdb diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index fe5a049..f81cdf1 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -1,6 +1,7 @@ package web import ( + "context" "fmt" "net/http" "net/url" @@ -446,7 +447,11 @@ func (s *Server) backupsHandler(w http.ResponseWriter, r *http.Request) { } if cfg.LastRun != "" { if t, err := time.Parse(time.RFC3339, cfg.LastRun); err == nil { - loc, _ := time.LoadLocation("Europe/Budapest") + // M7: Handle LoadLocation error — fall back to UTC if tzdata missing. + loc, err := time.LoadLocation("Europe/Budapest") + if err != nil { + loc = time.UTC + } item.LastRunShort = t.In(loc).Format("01-02 15:04") } } @@ -538,7 +543,11 @@ func (s *Server) buildAppBackupRows( crossConfigs map[string]*settings.CrossDriveBackup, destLabels map[string]string, ) []AppBackupRow { - loc, _ := time.LoadLocation("Europe/Budapest") + // M7: Handle LoadLocation error — fall back to UTC if tzdata missing. + loc, err := time.LoadLocation("Europe/Budapest") + if err != nil { + loc = time.UTC + } // Build a quick lookup: which stacks have a DB dump? dbStacks := make(map[string]bool) @@ -1243,8 +1252,10 @@ func (s *Server) syncFileBrowserMounts() { return } - // Recreate container - cmd := exec.Command("docker", "compose", "up", "-d", "--remove-orphans") + // Recreate container — H16: use 60s timeout to prevent hanging indefinitely. + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "docker", "compose", "up", "-d", "--remove-orphans") cmd.Dir = filepath.Dir(composePath) if out, err := cmd.CombinedOutput(); err != nil { s.logger.Printf("[ERROR] Failed to recreate FileBrowser: %s — %v", string(out), err) diff --git a/controller/internal/web/storage_handlers.go b/controller/internal/web/storage_handlers.go index 3166c28..33e147a 100644 --- a/controller/internal/web/storage_handlers.go +++ b/controller/internal/web/storage_handlers.go @@ -409,6 +409,20 @@ func (s *Server) storageMigrateAPIHandler(w http.ResponseWriter, r *http.Request return } + // C8: Validate TargetPath against registered storage paths to prevent path traversal. + registeredPaths := s.settings.GetStoragePaths() + validTarget := false + for _, sp := range registeredPaths { + if req.TargetPath == sp.Path { + validTarget = true + break + } + } + if !validTarget { + jsonError(w, "Érvénytelen célútvonal: nem regisztrált adattároló", http.StatusBadRequest) + return + } + mounts := stacks.ParseComposeHDDMounts(stack.ComposePath, currentHDDPath) if len(mounts) == 0 { jsonError(w, "Az alkalmazáshoz nem találhatók HDD csatlakozások", http.StatusBadRequest) diff --git a/controller/internal/web/templates/settings.html b/controller/internal/web/templates/settings.html index 9ddd81f..9174f2c 100644 --- a/controller/internal/web/templates/settings.html +++ b/controller/internal/web/templates/settings.html @@ -291,8 +291,20 @@ function editStorageLabel(path, currentLabel) { function cancelEditLabel(path, label) { var wrap = document.getElementById('label-wrap-' + path); if (!wrap) return; - wrap.innerHTML = '' + label + '' + - ' '; + // M11: Use DOM manipulation with textContent to prevent XSS if label contains HTML. + wrap.innerHTML = ''; + var span = document.createElement('span'); + span.className = 'storage-path-label'; + span.id = 'label-display-' + path; + span.textContent = label; + var btn = document.createElement('button'); + btn.className = 'btn btn-xs btn-ghost'; + btn.setAttribute('title', 'Átnevezés'); + btn.textContent = '✏️'; + btn.addEventListener('click', function() { editStorageLabel(path, label); }); + wrap.appendChild(span); + wrap.appendChild(document.createTextNode(' ')); + wrap.appendChild(btn); } {{template "layout_end" .}}