From 809620340eb65b230d6434b706c8cf4242e198e5 Mon Sep 17 00:00:00 2001 From: Tyler Koenig Date: Wed, 10 Jun 2026 18:50:19 -0400 Subject: [PATCH] fix(security): close XFF bypass and three secret-leak paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes hardening the secrets and rate-limit posture a prior audit left or that regressed: X-Forwarded-For rate-limit bypass + memory DoS (ratelimit.go): clientIP returned the raw XFF header, so an attacker rotating it minted unlimited distinct limiter keys — never tripping the limit and growing the visitors map without bound. XFF is now honored only when the immediate peer is a configured trusted proxy (UPTOP_TRUSTED_PROXIES, CIDRs or bare IPs), using the right-most non-trusted hop; otherwise the key is the real RemoteAddr. The visitors map is bounded with LRU eviction as defense in depth. Export redaction denylist -> per-provider allowlist (server.go): the old six-key denylist missed the actual credentials — the webhook URL for discord/slack/webhook/ntfy/gotify and api_key for opsgenie — exporting them in the clear. redactByProvider keeps only known-safe keys per provider type and redacts everything else, so unknown/new keys fail safe. ImportData plaintext secrets (sqlstore.go): import inserted raw json.Marshal(settings), bypassing the encryption AddAlert/UpdateAlert use. It now routes through marshalSettings, so a restore with UPTOP_ENCRYPTION_KEY set stores enc:-prefixed ciphertext, not plaintext. Alert error credential leak (alert.go): provider Send returned the raw *url.Error, whose URL carries the secret (Telegram bot token in the path, webhook secrets in the URL); it was persisted to AlertHealth.LastError and shown in the TUI. sanitizeError strips the URL, keeping the operation and underlying cause. Tests cover trusted/untrusted XFF + spoofed-bypass + map bound, the allowlist per provider, encrypted-on-import round-trip, and URL-stripped errors. README documents UPTOP_TRUSTED_PROXIES. Full suite green under -race; golangci-lint clean. --- README.md | 13 ++++ cmd/uptop/main.go | 53 +++++++++++++--- internal/alert/alert.go | 22 ++++++- internal/alert/alert_test.go | 32 ++++++++++ internal/server/ratelimit.go | 76 ++++++++++++++++++++--- internal/server/server.go | 68 +++++++++++++-------- internal/server/server_test.go | 105 ++++++++++++++++++++++++++++++++ internal/store/sqlstore.go | 6 +- internal/store/sqlstore_test.go | 40 ++++++++++++ 9 files changed, 371 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index 58e9a19..6e3d604 100644 --- a/README.md +++ b/README.md @@ -138,9 +138,22 @@ Full reference in [docs/config-as-code.md](docs/config-as-code.md). | `UPTOP_INSECURE_SKIP_VERIFY` | `false` | Skip TLS verification for checks | | `UPTOP_ALLOW_PRIVATE_TARGETS` | `false` | Allow monitoring RFC1918/loopback addresses | | `UPTOP_ADMIN_KEY` | | SSH public key seeded as first admin on startup | +| `UPTOP_TRUSTED_PROXIES` | | Comma-separated CIDRs/IPs whose `X-Forwarded-For` is trusted ([details](#running-behind-a-reverse-proxy)) | See [`.env.example`](.env.example) for all options including TLS, probes, and advanced settings. +### Running behind a reverse proxy + +By default uptop ignores the `X-Forwarded-For` header and rate-limits by the direct connection address — so a client can't spoof the header to bypass limits. If uptop sits behind a reverse proxy (nginx, Caddy, Cloudflare, an ALB), set `UPTOP_TRUSTED_PROXIES` to the proxy's address(es) so the real client IP is used instead: + + # single nginx/Caddy on the same host + UPTOP_TRUSTED_PROXIES=127.0.0.1 + + # a proxy subnet, or Cloudflare ranges + UPTOP_TRUSTED_PROXIES=10.0.0.0/8,172.16.0.0/12 + +Only requests whose immediate peer is in this list have their `X-Forwarded-For` honored (right-most non-trusted hop wins). Bare IPs are treated as single hosts; invalid entries are warned about and skipped. Leave it unset if uptop is exposed directly. + ### Encryption Set `UPTOP_ENCRYPTION_KEY` to encrypt alert credentials (SMTP passwords, webhook URLs, API tokens) at rest with AES-256-GCM. Generate a key: diff --git a/cmd/uptop/main.go b/cmd/uptop/main.go index a1ccd0b..0a00dfe 100644 --- a/cmd/uptop/main.go +++ b/cmd/uptop/main.go @@ -7,6 +7,7 @@ import ( "flag" "fmt" "log" + "net" "net/url" "os" "os/signal" @@ -85,6 +86,39 @@ func redactDSN(dsn string) string { return u.String() } +// parseTrustedProxies turns UPTOP_TRUSTED_PROXIES (comma-separated CIDRs or +// bare IPs) into networks the rate limiter trusts to set X-Forwarded-For. Bare +// IPs are treated as single-host ranges. Invalid entries are warned about and +// skipped, so a typo degrades to "ignore XFF" (safe) rather than aborting boot. +func parseTrustedProxies(raw string) []*net.IPNet { + if strings.TrimSpace(raw) == "" { + return nil + } + var cidrs []*net.IPNet + for _, part := range strings.Split(raw, ",") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + if !strings.Contains(part, "/") { + if ip := net.ParseIP(part); ip != nil { + bits := 32 + if ip.To4() == nil { + bits = 128 + } + part = fmt.Sprintf("%s/%d", part, bits) + } + } + _, ipnet, err := net.ParseCIDR(part) + if err != nil { + fmt.Fprintf(os.Stderr, "WARNING: ignoring invalid UPTOP_TRUSTED_PROXIES entry %q: %v\n", part, err) + continue + } + cidrs = append(cidrs, ipnet) + } + return cidrs +} + func openStore(dbType, dsn string) store.Store { var ss *store.SQLStore var err error @@ -397,15 +431,16 @@ func runServe(args []string) { tlsKey := os.Getenv("UPTOP_TLS_KEY") httpSrv := server.Start(server.ServerConfig{ - Port: httpPort, - EnableStatus: enableStatus, - Title: statusTitle, - ClusterKey: clusterKey, - TLSCert: tlsCert, - TLSKey: tlsKey, - ClusterMode: clusterMode, - MetricsPublic: os.Getenv("UPTOP_METRICS_PUBLIC") == "true", - CORSOrigin: os.Getenv("UPTOP_CORS_ORIGIN"), + Port: httpPort, + EnableStatus: enableStatus, + Title: statusTitle, + ClusterKey: clusterKey, + TLSCert: tlsCert, + TLSKey: tlsKey, + ClusterMode: clusterMode, + MetricsPublic: os.Getenv("UPTOP_METRICS_PUBLIC") == "true", + CORSOrigin: os.Getenv("UPTOP_CORS_ORIGIN"), + TrustedProxies: parseTrustedProxies(os.Getenv("UPTOP_TRUSTED_PROXIES")), }, s, eng) cluster.Start(ctx, cluster.Config{ diff --git a/internal/alert/alert.go b/internal/alert/alert.go index 6b5262e..6e4bf03 100644 --- a/internal/alert/alert.go +++ b/internal/alert/alert.go @@ -4,9 +4,11 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "net/http" "net/smtp" + "net/url" "strconv" "strings" "time" @@ -16,6 +18,22 @@ import ( var alertClient = &http.Client{Timeout: 10 * time.Second} +// sanitizeError strips the request URL from transport errors before they are +// stored or displayed. *url.Error embeds the full URL, which for several +// providers carries the credential itself (Telegram bot token in the path, +// webhook secrets in the URL). The operation and underlying cause — the useful +// diagnostic — are preserved. +func sanitizeError(err error) error { + if err == nil { + return nil + } + var urlErr *url.Error + if errors.As(err, &urlErr) { + return fmt.Errorf("%s request failed: %w", urlErr.Op, urlErr.Err) + } + return err +} + type Provider interface { Send(ctx context.Context, title, message string) error } @@ -43,7 +61,7 @@ func (h *HTTPProvider) Send(ctx context.Context, title, message string) error { } resp, err := alertClient.Do(req) if err != nil { - return err + return sanitizeError(err) } defer resp.Body.Close() if resp.StatusCode >= 400 { @@ -262,7 +280,7 @@ func (n *NtfyProvider) Send(ctx context.Context, title, message string) error { } resp, err := alertClient.Do(req) if err != nil { - return err + return sanitizeError(err) } defer resp.Body.Close() if resp.StatusCode >= 400 { diff --git a/internal/alert/alert_test.go b/internal/alert/alert_test.go index cf16dde..e4bdd9d 100644 --- a/internal/alert/alert_test.go +++ b/internal/alert/alert_test.go @@ -3,8 +3,11 @@ package alert import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" + "net/url" + "strings" "testing" "gitea.lerkolabs.com/lerkolabs/uptop/internal/models" @@ -298,3 +301,32 @@ func TestSanitizeHeader(t *testing.T) { } } } + +// sanitizeError must strip the credential-bearing URL from a *url.Error while +// keeping the operation and underlying cause. +func TestSanitizeError(t *testing.T) { + urlErr := &url.Error{ + Op: "Post", + URL: "https://api.telegram.org/bot123456:SECRET_TOKEN/sendMessage", + Err: errors.New("dial tcp: connection refused"), + } + got := sanitizeError(urlErr).Error() + + for _, leak := range []string{"SECRET_TOKEN", "api.telegram.org", "sendMessage", "bot123456"} { + if strings.Contains(got, leak) { + t.Errorf("sanitized error leaked %q: %s", leak, got) + } + } + if !strings.Contains(got, "connection refused") { + t.Errorf("expected underlying cause preserved, got: %s", got) + } + + // Non-url errors pass through unchanged. + plain := errors.New("plain failure") + if sanitizeError(plain).Error() != "plain failure" { + t.Errorf("non-url error altered: %s", sanitizeError(plain)) + } + if sanitizeError(nil) != nil { + t.Error("nil should stay nil") + } +} diff --git a/internal/server/ratelimit.go b/internal/server/ratelimit.go index b8f82f7..ca3aac7 100644 --- a/internal/server/ratelimit.go +++ b/internal/server/ratelimit.go @@ -3,10 +3,17 @@ package server import ( "net" "net/http" + "strings" "sync" "time" ) +// maxVisitors caps the rate-limiter map so a flood of distinct keys can't grow +// it without bound. With the trusted-proxy gate below, keys come from real peer +// addresses, so this is a defense-in-depth ceiling rather than the primary +// guard. +const maxVisitors = 10000 + type visitor struct { tokens float64 lastSeen time.Time @@ -17,13 +24,15 @@ type RateLimiter struct { visitors map[string]*visitor rate float64 burst float64 + trusted []*net.IPNet } -func NewRateLimiter(requestsPerMinute int) *RateLimiter { +func NewRateLimiter(requestsPerMinute int, trusted []*net.IPNet) *RateLimiter { rl := &RateLimiter{ visitors: make(map[string]*visitor), rate: float64(requestsPerMinute) / 60.0, burst: float64(requestsPerMinute), + trusted: trusted, } go rl.cleanup() return rl @@ -37,6 +46,9 @@ func (rl *RateLimiter) Allow(ip string) bool { now := time.Now() if !exists { + if len(rl.visitors) >= maxVisitors { + rl.evictOldest() + } rl.visitors[ip] = &visitor{tokens: rl.burst - 1, lastSeen: now} return true } @@ -55,6 +67,22 @@ func (rl *RateLimiter) Allow(ip string) bool { return true } +// evictOldest removes the least-recently-seen visitor. Called only when the map +// is at capacity, so the O(n) scan is rare. Caller holds rl.mu. +func (rl *RateLimiter) evictOldest() { + var oldestKey string + var oldest time.Time + for k, v := range rl.visitors { + if oldestKey == "" || v.lastSeen.Before(oldest) { + oldestKey = k + oldest = v.lastSeen + } + } + if oldestKey != "" { + delete(rl.visitors, oldestKey) + } +} + func (rl *RateLimiter) cleanup() { for { time.Sleep(5 * time.Minute) @@ -69,20 +97,54 @@ func (rl *RateLimiter) cleanup() { } } -func clientIP(r *http.Request) string { - if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" { - return fwd - } +// clientIP determines the rate-limit key for a request. X-Forwarded-For is only +// honored when the immediate peer (RemoteAddr) is a configured trusted proxy; +// otherwise the header is attacker-controlled and ignored, so a spoofed XFF +// can't mint unlimited distinct keys (rate-limit bypass + memory DoS). When the +// peer is trusted, the right-most address that is not itself a trusted proxy is +// the real client (RFC 7239 right-most-untrusted-hop). +func clientIP(r *http.Request, trusted []*net.IPNet) string { host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { - return r.RemoteAddr + host = r.RemoteAddr + } + + if len(trusted) == 0 || !ipInCIDRs(net.ParseIP(host), trusted) { + return host + } + + xff := r.Header.Get("X-Forwarded-For") + if xff == "" { + return host + } + parts := strings.Split(xff, ",") + for i := len(parts) - 1; i >= 0; i-- { + ip := net.ParseIP(strings.TrimSpace(parts[i])) + if ip == nil { + continue + } + if !ipInCIDRs(ip, trusted) { + return ip.String() + } } return host } +func ipInCIDRs(ip net.IP, cidrs []*net.IPNet) bool { + if ip == nil { + return false + } + for _, c := range cidrs { + if c.Contains(ip) { + return true + } + } + return false +} + func RateLimit(limiter *RateLimiter, next http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - if !limiter.Allow(clientIP(r)) { + if !limiter.Allow(clientIP(r, limiter.trusted)) { http.Error(w, "Rate limit exceeded", http.StatusTooManyRequests) return } diff --git a/internal/server/server.go b/internal/server/server.go index 9059a5e..8439207 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -6,6 +6,7 @@ import ( "fmt" "html/template" "log" + "net" "net/http" "sort" "strings" @@ -32,18 +33,36 @@ func extractBearerToken(r *http.Request) string { return "" } -var sensitiveKeys = map[string]bool{ - "pass": true, "password": true, "token": true, - "routing_key": true, "user": true, "username": true, +// safeSettingKeys lists, per provider type, the settings that are NOT secret +// and may be exported in the clear. Everything else is redacted. Providers +// absent from this map (discord, slack, webhook, pushover) carry their secret +// in a field a denylist would miss — the webhook URL, the pushover token/user — +// so all of their settings are redacted. +var safeSettingKeys = map[string]map[string]bool{ + "email": {"host": true, "port": true, "to": true, "from": true}, + "ntfy": {"topic": true, "priority": true}, + "telegram": {"chat_id": true}, + "pagerduty": {"severity": true}, + "gotify": {"priority": true}, + "opsgenie": {"priority": true, "eu": true}, } -func redactSettings(settings map[string]string) map[string]string { +// redactByProvider keeps only the known-safe keys for the alert type and +// redacts everything else. An allowlist fails safe: an unknown or newly added +// setting is redacted by default instead of leaking. This closes the denylist +// gap where url (discord/slack/webhook/ntfy/gotify) and api_key (opsgenie) — +// the actual credentials — were exported in the clear. +func redactByProvider(alertType string, settings map[string]string) map[string]string { + safe := safeSettingKeys[alertType] redacted := make(map[string]string, len(settings)) for k, v := range settings { - if sensitiveKeys[k] && v != "" { - redacted[k] = "***REDACTED***" - } else { + switch { + case v == "": + redacted[k] = "" + case safe[k]: redacted[k] = v + default: + redacted[k] = "***REDACTED***" } } return redacted @@ -182,15 +201,16 @@ var statusTpl = template.Must(template.New("status").Parse(` `)) type ServerConfig struct { - Port int - EnableStatus bool - Title string - ClusterKey string - TLSCert string - TLSKey string - ClusterMode string - MetricsPublic bool - CORSOrigin string + Port int + EnableStatus bool + Title string + ClusterKey string + TLSCert string + TLSKey string + ClusterMode string + MetricsPublic bool + CORSOrigin string + TrustedProxies []*net.IPNet } func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { @@ -198,10 +218,10 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { fmt.Println("WARNING: No UPTOP_CLUSTER_SECRET set. Cluster API endpoints are unauthenticated.") } - pushRL := NewRateLimiter(60) - probeRL := NewRateLimiter(30) - backupRL := NewRateLimiter(10) - statusRL := NewRateLimiter(120) + pushRL := NewRateLimiter(60, cfg.TrustedProxies) + probeRL := NewRateLimiter(30, cfg.TrustedProxies) + backupRL := NewRateLimiter(10, cfg.TrustedProxies) + statusRL := NewRateLimiter(120, cfg.TrustedProxies) mux := http.NewServeMux() @@ -258,7 +278,7 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { } if r.URL.Query().Get("redact_secrets") != "false" { for i := range data.Alerts { - data.Alerts[i].Settings = redactSettings(data.Alerts[i].Settings) + data.Alerts[i].Settings = redactByProvider(data.Alerts[i].Type, data.Alerts[i].Settings) } } _ = json.NewEncoder(w).Encode(data) //nolint:errcheck @@ -482,7 +502,7 @@ func Start(cfg ServerConfig, s store.Store, eng *monitor.Engine) *http.Server { fmt.Println("WARNING: Cluster mode active without TLS. Secrets transmitted in cleartext.") } - handler := loggingMiddleware(securityHeadersMiddleware(mux)) + handler := loggingMiddleware(cfg.TrustedProxies, securityHeadersMiddleware(mux)) if cfg.TLSCert != "" { handler = hstsMiddleware(handler) } @@ -522,13 +542,13 @@ func (w *statusWriter) WriteHeader(code int) { w.ResponseWriter.WriteHeader(code) } -func loggingMiddleware(next http.Handler) http.Handler { +func loggingMiddleware(trusted []*net.IPNet, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { start := time.Now() sw := &statusWriter{ResponseWriter: w, code: 200} next.ServeHTTP(sw, r) path := strings.ReplaceAll(strings.ReplaceAll(r.URL.Path, "\n", ""), "\r", "") - log.Printf("%s %s %d %s %s", r.Method, path, sw.code, time.Since(start).Round(time.Millisecond), clientIP(r)) //nolint:gosec // path sanitized above + log.Printf("%s %s %d %s %s", r.Method, path, sw.code, time.Since(start).Round(time.Millisecond), clientIP(r, trusted)) //nolint:gosec // path sanitized above }) } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index a2805ed..377293e 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -565,3 +565,108 @@ func TestProbeAssignments_Unauthorized(t *testing.T) { t.Errorf("expected 401, got %d", resp.StatusCode) } } + +// --- Security: X-Forwarded-For trusted-proxy handling --- + +func mustCIDR(t *testing.T, s string) *net.IPNet { + t.Helper() + _, n, err := net.ParseCIDR(s) + if err != nil { + t.Fatalf("ParseCIDR(%q): %v", s, err) + } + return n +} + +func TestClientIP_TrustedProxyHandling(t *testing.T) { + trusted := []*net.IPNet{mustCIDR(t, "10.0.0.0/8")} + + tests := []struct { + name string + remoteAddr string + xff string + trusted []*net.IPNet + want string + }{ + {"no trusted proxies ignores XFF", "203.0.113.9:5000", "1.2.3.4", nil, "203.0.113.9"}, + {"untrusted peer ignores XFF", "203.0.113.9:5000", "1.2.3.4", trusted, "203.0.113.9"}, + {"trusted peer honors XFF", "10.0.0.5:5000", "1.2.3.4", trusted, "1.2.3.4"}, + {"trusted peer, rightmost-untrusted hop", "10.0.0.5:5000", "1.2.3.4, 10.0.0.9", trusted, "1.2.3.4"}, + {"trusted peer, no XFF falls back to peer", "10.0.0.5:5000", "", trusted, "10.0.0.5"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, _ := http.NewRequest(http.MethodGet, "/", nil) + r.RemoteAddr = tt.remoteAddr + if tt.xff != "" { + r.Header.Set("X-Forwarded-For", tt.xff) + } + if got := clientIP(r, tt.trusted); got != tt.want { + t.Errorf("clientIP = %q, want %q", got, tt.want) + } + }) + } +} + +// A spoofed, rotating X-Forwarded-For from an untrusted peer must NOT bypass +// the limiter: all requests key on the real RemoteAddr, so the bucket trips. +func TestRateLimit_SpoofedXFFCannotBypass(t *testing.T) { + rl := NewRateLimiter(60, nil) // no trusted proxies + allowed := 0 + for i := 0; i < 200; i++ { + r, _ := http.NewRequest(http.MethodGet, "/", nil) + r.RemoteAddr = "203.0.113.9:5000" + r.Header.Set("X-Forwarded-For", fmt.Sprintf("9.9.9.%d", i%256)) + if rl.Allow(clientIP(r, rl.trusted)) { + allowed++ + } + } + if allowed > 60 { + t.Errorf("spoofed XFF bypassed limiter: %d/200 allowed (burst is 60)", allowed) + } +} + +func TestRateLimit_VisitorMapBounded(t *testing.T) { + rl := NewRateLimiter(60, nil) + for i := 0; i < maxVisitors+500; i++ { + rl.Allow(fmt.Sprintf("10.1.%d.%d", i/256, i%256)) + } + rl.mu.Lock() + n := len(rl.visitors) + rl.mu.Unlock() + if n > maxVisitors { + t.Errorf("visitor map exceeded cap: %d > %d", n, maxVisitors) + } +} + +// --- Security: export redaction allowlist --- + +func TestRedactByProvider(t *testing.T) { + tests := []struct { + name string + typ string + in map[string]string + redacted []string // keys expected to be ***REDACTED*** + kept []string // keys expected to survive verbatim + }{ + {"discord url is secret", "discord", map[string]string{"url": "https://discord.com/api/webhooks/1/abc"}, []string{"url"}, nil}, + {"opsgenie api_key redacted, priority kept", "opsgenie", map[string]string{"api_key": "k", "priority": "P1", "eu": "true"}, []string{"api_key"}, []string{"priority", "eu"}}, + {"email creds redacted, routing kept", "email", map[string]string{"host": "smtp.x.com", "port": "587", "to": "a@x.com", "from": "b@x.com", "user": "u", "pass": "p"}, []string{"user", "pass"}, []string{"host", "port", "to", "from"}}, + {"telegram token redacted, chat_id kept", "telegram", map[string]string{"token": "123:ABC", "chat_id": "42"}, []string{"token"}, []string{"chat_id"}}, + {"unknown provider redacts everything", "mystery", map[string]string{"anything": "x"}, []string{"anything"}, nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := redactByProvider(tt.typ, tt.in) + for _, k := range tt.redacted { + if out[k] != "***REDACTED***" { + t.Errorf("key %q: expected redacted, got %q", k, out[k]) + } + } + for _, k := range tt.kept { + if out[k] != tt.in[k] { + t.Errorf("key %q: expected kept %q, got %q", k, tt.in[k], out[k]) + } + } + }) + } +} diff --git a/internal/store/sqlstore.go b/internal/store/sqlstore.go index f5e7af8..1f0198f 100644 --- a/internal/store/sqlstore.go +++ b/internal/store/sqlstore.go @@ -724,11 +724,13 @@ func (s *SQLStore) ImportData(data models.Backup) error { } } for _, a := range data.Alerts { - jsonBytes, err := json.Marshal(a.Settings) + // Encrypt on import exactly as AddAlert/UpdateAlert do, so a restore + // honors UPTOP_ENCRYPTION_KEY instead of writing secrets in plaintext. + settingsStr, err := s.marshalSettings(a.Settings) if err != nil { return err } - if _, err := tx.Exec(s.q("INSERT INTO alerts (id, name, type, settings) VALUES (?, ?, ?, ?)"), a.ID, a.Name, a.Type, string(jsonBytes)); err != nil { + if _, err := tx.Exec(s.q("INSERT INTO alerts (id, name, type, settings) VALUES (?, ?, ?, ?)"), a.ID, a.Name, a.Type, settingsStr); err != nil { return err } } diff --git a/internal/store/sqlstore_test.go b/internal/store/sqlstore_test.go index d041b2d..172bc80 100644 --- a/internal/store/sqlstore_test.go +++ b/internal/store/sqlstore_test.go @@ -2,6 +2,7 @@ package store import ( "fmt" + "strings" "testing" "time" @@ -441,3 +442,42 @@ func TestPruneExpiredMaintenanceWindows(t *testing.T) { } } } + +// ImportData must encrypt alert settings (like AddAlert/UpdateAlert) so a +// restore with UPTOP_ENCRYPTION_KEY set never lands secrets in plaintext. +func TestImportData_EncryptsAlertSettings(t *testing.T) { + s := newTestStore(t) + enc, err := NewEncryptor(strings.Repeat("ab", 32)) // 64 hex chars = 32 bytes + if err != nil { + t.Fatalf("NewEncryptor: %v", err) + } + s.SetEncryptor(enc) + + backup := models.Backup{ + Alerts: []models.AlertConfig{ + {ID: 1, Name: "tg", Type: "telegram", Settings: map[string]string{"token": "123:SECRET", "chat_id": "42"}}, + }, + } + if err := s.ImportData(backup); err != nil { + t.Fatalf("ImportData: %v", err) + } + + var raw string + if err := s.db.QueryRow("SELECT settings FROM alerts WHERE id = 1").Scan(&raw); err != nil { + t.Fatalf("query settings: %v", err) + } + if !strings.HasPrefix(raw, encryptedPrefix) { + t.Errorf("imported settings not encrypted: %q", raw) + } + if strings.Contains(raw, "SECRET") { + t.Errorf("plaintext secret found in stored column: %q", raw) + } + + alerts, err := s.GetAllAlerts() + if err != nil { + t.Fatalf("GetAllAlerts: %v", err) + } + if len(alerts) != 1 || alerts[0].Settings["token"] != "123:SECRET" { + t.Errorf("decrypt round-trip failed: %+v", alerts) + } +}