From 986f9f1d550f0eeda3480e153ee0435e1331fbd2 Mon Sep 17 00:00:00 2001 From: Tyler Koenig Date: Tue, 26 May 2026 17:25:47 -0400 Subject: [PATCH] fix(security): phase 4 code quality and low-severity fixes - Fix limitStr to handle multi-byte UTF-8 characters correctly - Sanitize log messages: strip ANSI escape sequences and newlines - URL-encode probe node_id instead of string concatenation - Fix follower resp.Body leak on non-200 responses - Make SSH host key path configurable via UPTOP_SSH_HOST_KEY env var - Add HTTP method checks on GET-only endpoints (405 for wrong methods) - Extract magic numbers into named constants across monitor/store/server - Standardize error output to stderr for all startup errors --- cmd/uptop/main.go | 14 +++++++------- internal/cluster/cluster.go | 7 ++++--- internal/cluster/probe.go | 4 +++- internal/monitor/monitor.go | 38 +++++++++++++++++++++++++++---------- internal/server/server.go | 26 +++++++++++++++++++++---- internal/store/sqlstore.go | 18 +++++++++++++----- internal/tui/tui.go | 12 +++++++----- 7 files changed, 84 insertions(+), 35 deletions(-) diff --git a/cmd/uptop/main.go b/cmd/uptop/main.go index 26aa1af..659d509 100644 --- a/cmd/uptop/main.go +++ b/cmd/uptop/main.go @@ -323,7 +323,7 @@ func runServe(args []string) { fmt.Printf("Using SQLite: %s\n", *flagDSN) } if dbErr != nil { - fmt.Printf("Database connection error: %v\n", dbErr) + fmt.Fprintf(os.Stderr, "database connection error: %v\n", dbErr) os.Exit(1) } defer ss.Close() @@ -341,7 +341,7 @@ func runServe(args []string) { var s store.Store = ss if err := s.Init(); err != nil { - fmt.Printf("Database init error: %v\n", err) + fmt.Fprintf(os.Stderr, "database init error: %v\n", err) os.Exit(1) } if *demo { @@ -351,12 +351,12 @@ func runServe(args []string) { if *importKuma != "" { kb, err := importer.LoadKumaFile(*importKuma) if err != nil { - fmt.Printf("Kuma import error: %v\n", err) + fmt.Fprintf(os.Stderr, "kuma import error: %v\n", err) os.Exit(1) } backup := importer.ConvertKuma(kb) if err := s.ImportData(backup); err != nil { - fmt.Printf("Import failed: %v\n", err) + fmt.Fprintf(os.Stderr, "import failed: %v\n", err) os.Exit(1) } fmt.Printf("Imported %d monitors and %d alerts from Uptime Kuma v%s\n", len(backup.Sites), len(backup.Alerts), kb.Version) @@ -409,7 +409,7 @@ func runServe(args []string) { if isatty.IsTerminal(os.Stdout.Fd()) || isatty.IsCygwinTerminal(os.Stdout.Fd()) { p := tea.NewProgram(tui.InitialModel(true, s, eng), tea.WithAltScreen(), tea.WithMouseCellMotion()) if _, err := p.Run(); err != nil { - fmt.Printf("Error: %v\n", err) + fmt.Fprintf(os.Stderr, "error: %v\n", err) } } else { fmt.Println("uptop running in HEADLESS mode") @@ -437,7 +437,7 @@ func runServe(args []string) { func startSSHServer(port int, db store.Store, eng *monitor.Engine, kc *keyCache) *ssh.Server { s, err := wish.NewServer( wish.WithAddress(fmt.Sprintf(":%d", port)), - wish.WithHostKeyPath(".ssh/id_ed25519"), + wish.WithHostKeyPath(envOrDefault("UPTOP_SSH_HOST_KEY", ".ssh/id_ed25519")), wish.WithPublicKeyAuth(func(ctx ssh.Context, key ssh.PublicKey) bool { return kc.IsAllowed(key) }), @@ -448,7 +448,7 @@ func startSSHServer(port int, db store.Store, eng *monitor.Engine, kc *keyCache) ), ) if err != nil { - fmt.Printf("SSH server error: %v\n", err) + fmt.Fprintf(os.Stderr, "SSH server error: %v\n", err) return nil } go func() { diff --git a/internal/cluster/cluster.go b/internal/cluster/cluster.go index b10580c..4ed2da9 100644 --- a/internal/cluster/cluster.go +++ b/internal/cluster/cluster.go @@ -3,10 +3,11 @@ package cluster import ( "context" "fmt" - "gitea.lerkolabs.com/lerko/uptop/internal/monitor" "net/http" "strings" "time" + + "gitea.lerkolabs.com/lerko/uptop/internal/monitor" ) type Config struct { @@ -57,8 +58,8 @@ func runFollowerLoop(ctx context.Context, cfg Config, eng *monitor.Engine) { resp, err := client.Do(req) isLeaderHealthy := false - if err == nil && resp.StatusCode == 200 { - isLeaderHealthy = true + if err == nil { + isLeaderHealthy = resp.StatusCode == 200 _ = resp.Body.Close() } diff --git a/internal/cluster/probe.go b/internal/cluster/probe.go index 7f36d9e..5037675 100644 --- a/internal/cluster/probe.go +++ b/internal/cluster/probe.go @@ -8,6 +8,7 @@ import ( "fmt" "log" "net/http" + "net/url" "sync" "time" @@ -102,7 +103,8 @@ func probeRegister(ctx context.Context, client *http.Client, cfg ProbeConfig) er } func probeFetchAssignments(ctx context.Context, client *http.Client, cfg ProbeConfig) ([]models.Site, error) { - req, err := http.NewRequestWithContext(ctx, "GET", cfg.LeaderURL+"/api/probe/assignments?node_id="+cfg.NodeID, nil) + assignURL := cfg.LeaderURL + "/api/probe/assignments?" + url.Values{"node_id": {cfg.NodeID}}.Encode() + req, err := http.NewRequestWithContext(ctx, "GET", assignURL, nil) if err != nil { return nil, err } diff --git a/internal/monitor/monitor.go b/internal/monitor/monitor.go index a7fcf1c..b31e0c5 100644 --- a/internal/monitor/monitor.go +++ b/internal/monitor/monitor.go @@ -6,6 +6,8 @@ import ( "fmt" "math/rand/v2" "net/http" + "regexp" + "strings" "sync" "time" @@ -14,6 +16,13 @@ import ( "gitea.lerkolabs.com/lerko/uptop/internal/store" ) +const ( + maxLogEntries = 100 + pollInterval = 5 * time.Second + pushGracePeriod = 5 * time.Second + minCheckInterval = 5 +) + type Engine struct { mu sync.RWMutex liveState map[int]models.Site @@ -78,14 +87,23 @@ func (e *Engine) SetInsecureSkipVerify(skip bool) { e.insecureSkipVerify = skip } +var ansiRe = regexp.MustCompile(`\x1b\[[0-9;]*[a-zA-Z]`) + +func sanitizeLog(s string) string { + s = ansiRe.ReplaceAllString(s, "") + s = strings.ReplaceAll(s, "\n", "\\n") + s = strings.ReplaceAll(s, "\r", "") + return s +} + func (e *Engine) AddLog(msg string) { e.logMu.Lock() defer e.logMu.Unlock() ts := time.Now().Format("15:04:05") - entry := fmt.Sprintf("[%s] %s", ts, msg) + entry := fmt.Sprintf("[%s] %s", ts, sanitizeLog(msg)) e.logStore = append([]string{entry}, e.logStore...) - if len(e.logStore) > 100 { - e.logStore = e.logStore[:100] + if len(e.logStore) > maxLogEntries { + e.logStore = e.logStore[:maxLogEntries] } go func() { _ = e.db.SaveLog(entry) }() } @@ -210,7 +228,7 @@ func (e *Engine) Start(ctx context.Context) { if err != nil { e.AddLog(fmt.Sprintf("Failed to load sites: %v", err)) select { - case <-time.After(5 * time.Second): + case <-time.After(pollInterval): case <-ctx.Done(): return } @@ -244,7 +262,7 @@ func (e *Engine) Start(ctx context.Context) { } select { - case <-time.After(5 * time.Second): + case <-time.After(pollInterval): case <-ctx.Done(): return } @@ -314,7 +332,7 @@ func (e *Engine) monitorRoutine(ctx context.Context, id int) { if !e.IsActive() { select { - case <-time.After(5 * time.Second): + case <-time.After(pollInterval): case <-ctx.Done(): return } @@ -330,7 +348,7 @@ func (e *Engine) monitorRoutine(ctx context.Context, id int) { if site.Paused { select { - case <-time.After(5 * time.Second): + case <-time.After(pollInterval): case <-ctx.Done(): return } @@ -338,8 +356,8 @@ func (e *Engine) monitorRoutine(ctx context.Context, id int) { } interval := site.Interval - if interval < 5 { - interval = 5 + if interval < minCheckInterval { + interval = minCheckInterval } jitter := time.Duration(rand.IntN(interval*100)) * time.Millisecond //nolint:gosec // non-security jitter select { @@ -380,7 +398,7 @@ func (e *Engine) checkByID(id int) { } func (e *Engine) checkPush(site models.Site) { - deadline := site.LastCheck.Add(time.Duration(site.Interval) * time.Second).Add(5 * time.Second) + deadline := site.LastCheck.Add(time.Duration(site.Interval) * time.Second).Add(pushGracePeriod) if time.Now().After(deadline) { e.handleStatusChange(site, "DOWN", 0, 0) } else if site.Status != "UP" { diff --git a/internal/server/server.go b/internal/server/server.go index 1f9d3a4..85b791d 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -18,6 +18,8 @@ import ( "gitea.lerkolabs.com/lerko/uptop/internal/store" ) +const maxRequestBody = 1 << 20 + func checkSecret(got, want string) bool { return subtle.ConstantTimeCompare([]byte(got), []byte(want)) == 1 } @@ -204,6 +206,10 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { // 1. Push Heartbeat mux.HandleFunc("/api/push", RateLimit(pushRL, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet && r.Method != http.MethodPost { + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return + } token := extractBearerToken(r) if token == "" { if qt := r.URL.Query().Get("token"); qt != "" { @@ -225,6 +231,10 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { // 2. Health Check (For Cluster Follower) mux.HandleFunc("/api/health", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return + } if cfg.ClusterKey != "" && !checkSecret(r.Header.Get("X-Upkeep-Secret"), cfg.ClusterKey) { http.Error(w, "Unauthorized", http.StatusUnauthorized) return @@ -263,7 +273,7 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { http.Error(w, "Unauthorized", http.StatusUnauthorized) return } - r.Body = http.MaxBytesReader(w, r.Body, 1<<20) + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBody) var data models.Backup if err := json.NewDecoder(r.Body).Decode(&data); err != nil { http.Error(w, "Invalid JSON", http.StatusBadRequest) @@ -287,7 +297,7 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { http.Error(w, "Unauthorized", http.StatusUnauthorized) return } - r.Body = http.MaxBytesReader(w, r.Body, 1<<20) + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBody) var kb importer.KumaBackup if err := json.NewDecoder(r.Body).Decode(&kb); err != nil { log.Printf("Invalid Kuma JSON: %v", err) @@ -313,7 +323,7 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { http.Error(w, "Unauthorized", http.StatusUnauthorized) return } - r.Body = http.MaxBytesReader(w, r.Body, 1<<20) + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBody) var req struct { ID string `json:"id"` Name string `json:"name"` @@ -340,6 +350,10 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { // 7. Probe Assignment Fetch mux.HandleFunc("/api/probe/assignments", RateLimit(probeRL, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return + } if cfg.ClusterKey == "" || !checkSecret(r.Header.Get("X-Upkeep-Secret"), cfg.ClusterKey) { http.Error(w, "Unauthorized", http.StatusUnauthorized) return @@ -385,7 +399,7 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { http.Error(w, "Unauthorized", http.StatusUnauthorized) return } - r.Body = http.MaxBytesReader(w, r.Body, 1<<20) + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBody) var req struct { NodeID string `json:"node_id"` Results []struct { @@ -416,6 +430,10 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { // 9. Prometheus Metrics mux.HandleFunc("/metrics", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return + } if !cfg.MetricsPublic && cfg.ClusterKey != "" { if !checkSecret(r.Header.Get("X-Upkeep-Secret"), cfg.ClusterKey) { http.Error(w, "Unauthorized", http.StatusUnauthorized) diff --git a/internal/store/sqlstore.go b/internal/store/sqlstore.go index 88281cf..f8a1e20 100644 --- a/internal/store/sqlstore.go +++ b/internal/store/sqlstore.go @@ -12,6 +12,13 @@ import ( "gitea.lerkolabs.com/lerko/uptop/internal/models" ) +const ( + maxCheckHistory = 1000 + checkHistoryPruneAt = 1100 + maxMaintenanceExport = 1000 + maxRequestBody = 1 << 20 +) + type SQLStore struct { db *sql.DB dialect Dialect @@ -351,10 +358,11 @@ func (s *SQLStore) SaveCheckFromNode(siteID int, nodeID string, latencyNs int64, } var count int _ = s.db.QueryRow(s.q("SELECT COUNT(*) FROM check_history WHERE site_id = ?"), siteID).Scan(&count) - if count > 1100 { - _, err = s.db.Exec(s.q(`DELETE FROM check_history WHERE site_id = ? AND id NOT IN ( - SELECT id FROM check_history WHERE site_id = ? ORDER BY checked_at DESC LIMIT 1000 - )`), siteID, siteID) + if count > checkHistoryPruneAt { + pruneQuery := fmt.Sprintf(`DELETE FROM check_history WHERE site_id = ? AND id NOT IN ( + SELECT id FROM check_history WHERE site_id = ? ORDER BY checked_at DESC LIMIT %d + )`, maxCheckHistory) + _, err = s.db.Exec(s.q(pruneQuery), siteID, siteID) return err } return nil @@ -570,7 +578,7 @@ func (s *SQLStore) ExportData() (models.Backup, error) { if err != nil { return models.Backup{}, err } - windows, err := s.GetAllMaintenanceWindows(1000) + windows, err := s.GetAllMaintenanceWindows(maxMaintenanceExport) if err != nil { return models.Backup{}, err } diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 3969f98..7fbbfd2 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -3,14 +3,15 @@ package tui import ( "encoding/json" "fmt" - "gitea.lerkolabs.com/lerko/uptop/internal/models" - "gitea.lerkolabs.com/lerko/uptop/internal/monitor" - "gitea.lerkolabs.com/lerko/uptop/internal/store" "math" "sort" "strings" "time" + "gitea.lerkolabs.com/lerko/uptop/internal/models" + "gitea.lerkolabs.com/lerko/uptop/internal/monitor" + "gitea.lerkolabs.com/lerko/uptop/internal/store" + "github.com/charmbracelet/bubbles/viewport" tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/harmonica" @@ -956,8 +957,9 @@ func siteOrder(s models.Site) int { } func limitStr(text string, max int) string { - if len(text) > max { - return text[:max-3] + "..." + runes := []rune(text) + if len(runes) > max { + return string(runes[:max-3]) + "..." } return text }