fix(sec): санитизация Telegram-ошибок, SSRF-guard Webhook, чистка логов test-канала, go mod tidy, histogram-бакеты

2026-07-04 13:40:29 +07:00
parent 5a2903ca1e
commit 29f448d4b5
8 changed files with 197 additions and 6 deletions
@@ -225,7 +225,11 @@ func (a *API) handleTestChannel(w http.ResponseWriter, r *http.Request) {
 		secret = string(dec)
 	}
 	if err := a.Dispatch.SendTest(r.Context(), ch.Type, ch.Config, secret); err != nil {
-		log.Printf("api: test channel %s failed: %v", cid, err)
+		// Defense-in-depth: notify implementations sanitize errors before
+		// returning them (no secret/URL material), but this log deliberately
+		// omits the raw error (%v) anyway so a lower-layer regression can
+		// never leak a bot token or webhook URL into logs.
+		log.Printf("api: test channel %s failed", cid)
 		writeErr(w, http.StatusBadGateway, "channel test failed")
 		return
 	}
@@ -40,6 +40,12 @@ func New() *Metrics {
 		CheckDuration: f.NewHistogram(prometheus.HistogramOpts{
 			Name: "dns_ar_check_duration_seconds",
 			Help: "Длительность выполнения проверки домена в секундах.",
+			// Проверка домена — сетевой вызов DNS-провайдера, а не
+			// внутрипроцессная операция (для которой рассчитан
+			// prometheus.DefBuckets, начинающийся с 5мс). Бакеты подобраны
+			// под реалистичный диапазон задержек такого вызова, включая
+			// таймауты/ретраи медленных провайдеров.
+			Buckets: []float64{0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30},
 		}),
 		DriftDomains: f.NewGauge(prometheus.GaugeOpts{
 			Name: "dns_ar_drift_domains",
@@ -27,6 +27,26 @@ func TestMetricsRecord(t *testing.T) {
 	}
 }

+func TestCheckDurationUsesNetworkCallBuckets(t *testing.T) {
+	m := New()
+	m.ObserveCheck("in_sync", 100*time.Millisecond)
+	rec := httptest.NewRecorder()
+	m.Handler().ServeHTTP(rec, httptest.NewRequest("GET", "/metrics", nil))
+	body := rec.Body.String()
+
+	// DefBuckets (le="0.005", ...) is tuned for sub-10ms in-process calls;
+	// dns_ar_check_duration_seconds is a network call to a DNS provider, so
+	// it must use the wider explicit buckets instead.
+	for _, want := range []string{`le="0.05"`, `le="1"`, `le="30"`} {
+		if !strings.Contains(body, `dns_ar_check_duration_seconds_bucket{`+want) {
+			t.Fatalf("expected bucket %s in exposed metrics:\n%s", want, body)
+		}
+	}
+	if strings.Contains(body, `dns_ar_check_duration_seconds_bucket{le="0.005"`) {
+		t.Fatalf("found default histogram bucket 0.005, expected custom buckets:\n%s", body)
+	}
+}
+
 func TestHandlerExposesMetrics(t *testing.T) {
 	m := New()
 	m.ObserveCheck("in_sync", time.Millisecond)
@@ -55,6 +55,30 @@ func TestTelegramSendServerError(t *testing.T) {
 	}
 }

+func TestTelegramSendTransportErrorDoesNotLeakSecret(t *testing.T) {
+	// Bind and immediately close a server: its address is now unreachable
+	// (connection refused), which makes http.Client.Do return a *url.Error
+	// whose Error() embeds the full request URL — including /bot<secret>/.
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
+	deadURL := srv.URL
+	srv.Close()
+
+	tg := &Telegram{BaseURL: deadURL, HTTP: srv.Client()}
+	ev := Event{Project: "proj", Domain: "example.com", Status: "drift", Summary: "x", At: time.Now()}
+
+	const secret = "super-secret-bot-token"
+	err := tg.Send(context.Background(), json.RawMessage(`{"chat_id":"1"}`), secret, ev)
+	if err == nil {
+		t.Fatal("expected error for unreachable host")
+	}
+	if strings.Contains(err.Error(), secret) {
+		t.Fatalf("error leaks secret: %v", err)
+	}
+	if strings.Contains(err.Error(), deadURL) {
+		t.Fatalf("error leaks request URL: %v", err)
+	}
+}
+
 func TestWebhookSendSuccess(t *testing.T) {
 	var gotEvent Event
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -66,7 +90,10 @@ func TestWebhookSendSuccess(t *testing.T) {
 	}))
 	defer srv.Close()

-	wh := &Webhook{HTTP: srv.Client()}
+	// allowPrivate: true — httptest.Server listens on 127.0.0.1, which the
+	// SSRF guard would otherwise reject; production dispatchers never set
+	// this (see TestIsAllowedURL / TestNewDispatcherDoesNotAllowPrivate).
+	wh := &Webhook{HTTP: srv.Client(), allowPrivate: true}
 	ev := Event{Project: "proj", Domain: "example.com", Status: "in_sync", Summary: "resolved", At: time.Now()}

 	cfg, _ := json.Marshal(map[string]string{"url": srv.URL})
@@ -84,7 +111,7 @@ func TestWebhookSendNonSuccessStatus(t *testing.T) {
 	}))
 	defer srv.Close()

-	wh := &Webhook{HTTP: srv.Client()}
+	wh := &Webhook{HTTP: srv.Client(), allowPrivate: true}
 	ev := Event{Project: "proj", Domain: "example.com", Status: "drift", Summary: "x", At: time.Now()}
 	cfg, _ := json.Marshal(map[string]string{"url": srv.URL})

@@ -93,6 +120,51 @@ func TestWebhookSendNonSuccessStatus(t *testing.T) {
 	}
 }

+func TestWebhookSendRejectsPrivateDestinationByDefault(t *testing.T) {
+	wh := &Webhook{HTTP: http.DefaultClient} // allowPrivate not set: SSRF guard active
+	ev := Event{Project: "proj", Domain: "example.com", Status: "drift", Summary: "x", At: time.Now()}
+	cfg, _ := json.Marshal(map[string]string{"url": "http://127.0.0.1:1/hook"})
+
+	err := wh.Send(context.Background(), cfg, "", ev)
+	if err == nil {
+		t.Fatal("expected error for loopback destination")
+	}
+	if !strings.Contains(err.Error(), "destination not allowed") {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestIsAllowedURL(t *testing.T) {
+	cases := []struct {
+		name    string
+		rawurl  string
+		allowed bool
+	}{
+		{"localhost hostname", "http://localhost/hook", false},
+		{"loopback ip", "http://127.0.0.1/hook", false},
+		{"loopback ipv6", "http://[::1]/hook", false},
+		{"link-local metadata", "http://169.254.169.254/latest/meta-data", false},
+		{"private class a", "http://10.0.0.1/hook", false},
+		{"private class c", "http://192.168.1.1/hook", false},
+		{"private class b", "http://172.16.0.1/hook", false},
+		{"unspecified", "http://0.0.0.0/hook", false},
+		{"multicast", "http://224.0.0.1/hook", false},
+		{"non-http scheme", "ftp://example.com/hook", false},
+		{"public ip", "http://93.184.216.34/hook", true},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := isAllowedURL(tc.rawurl)
+			if tc.allowed && err != nil {
+				t.Fatalf("expected %q to be allowed, got error: %v", tc.rawurl, err)
+			}
+			if !tc.allowed && err == nil {
+				t.Fatalf("expected %q to be rejected, got nil error", tc.rawurl)
+			}
+		})
+	}
+}
+
 // --- Dispatcher ---

 type mockChannelStore struct {
@@ -156,6 +228,9 @@ func TestDispatcherSendsToAllChannelsAndAggregatesErrors(t *testing.T) {
 		tg := &Telegram{BaseURL: tgSrv.URL, HTTP: tgSrv.Client()}
 		return tg.Send(ctx, cfg, secret, ev)
 	})
+	// httptest servers listen on loopback, which the SSRF guard rejects by
+	// default; swap in an allowPrivate webhook so this test can still hit it.
+	d.byType["webhook"] = &Webhook{HTTP: whSrv.Client(), allowPrivate: true}

 	ev := Event{Project: "proj", Domain: "example.com", Status: "drift", Summary: "changed", At: time.Now()}
 	err := d.Send(context.Background(), projectID, ev)
@@ -199,6 +274,9 @@ func TestDispatcherDecryptFailureIsAggregatedNotFatal(t *testing.T) {
 		{ID: uuid.New(), ProjectID: projectID, Type: "webhook", Config: json.RawMessage(`{"url":"` + whSrv.URL + `"}`), Enabled: true},
 	}
 	d := NewDispatcher(&mockChannelStore{channels: channels}, &mockDecryptor{fail: true})
+	// httptest servers listen on loopback, which the SSRF guard rejects by
+	// default; swap in an allowPrivate webhook so this test can still hit it.
+	d.byType["webhook"] = &Webhook{HTTP: whSrv.Client(), allowPrivate: true}

 	err := d.Send(context.Background(), projectID, Event{Project: "p", Domain: "d", Status: "drift"})
 	if err == nil {
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 )
@@ -35,7 +36,10 @@ func (t *Telegram) Send(ctx context.Context, cfg json.RawMessage, secret string,
 	req.Header.Set("Content-Type", "application/json")
 	resp, err := t.HTTP.Do(req)
 	if err != nil {
-		return err
+		// Do NOT wrap/return err as-is: *url.Error.Error() embeds the full
+		// request URL, which contains the bot token (/bot<secret>/...). A
+		// caller logging this error would leak the secret.
+		return errors.New("telegram: request failed")
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode >= 300 {
@@ -4,15 +4,77 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"net"
 	"net/http"
+	"net/url"
 )

 // Webhook delivers notifications as a JSON POST of the Event to a
 // project-configured URL. Config is {"url": "..."}. secret is currently
 // unused (reserved for future request signing) and is never logged.
+//
+// The destination URL is project-controlled (any project owner can set it),
+// so it is treated as untrusted input: isAllowedURL blocks requests to
+// loopback/private/link-local/unspecified addresses to prevent SSRF against
+// internal services and cloud metadata endpoints (e.g. 169.254.169.254).
+// Redirects are not followed, since a redirect response could otherwise be
+// used to bypass the destination check.
 type Webhook struct {
 	HTTP *http.Client
+
+	// allowPrivate disables the SSRF guard. It exists only so tests can
+	// exercise Send happy-paths against httptest servers, which listen on
+	// loopback. Production Dispatchers (NewDispatcher) must never set this.
+	allowPrivate bool
+}
+
+// isAllowedURL rejects any URL that is not a plain http/https request to a
+// public, resolvable address. It resolves hostnames and checks every
+// returned address — a hostname that resolves to even one
+// private/loopback/link-local/unspecified address is rejected, since DNS
+// answers are attacker-influenceable (rebinding) and partial trust is not
+// safe.
+func isAllowedURL(rawurl string) error {
+	u, err := url.Parse(rawurl)
+	if err != nil {
+		return fmt.Errorf("webhook: invalid url: %w", err)
+	}
+	if u.Scheme != "http" && u.Scheme != "https" {
+		return errors.New("webhook: destination not allowed")
+	}
+	host := u.Hostname()
+	if host == "" {
+		return errors.New("webhook: destination not allowed")
+	}
+
+	var ips []net.IP
+	if ip := net.ParseIP(host); ip != nil {
+		ips = []net.IP{ip}
+	} else {
+		resolved, err := net.LookupIP(host)
+		if err != nil {
+			return errors.New("webhook: destination not allowed")
+		}
+		ips = resolved
+	}
+
+	for _, ip := range ips {
+		if isDisallowedIP(ip) {
+			return errors.New("webhook: destination not allowed")
+		}
+	}
+	return nil
+}
+
+func isDisallowedIP(ip net.IP) bool {
+	return ip.IsLoopback() ||
+		ip.IsPrivate() ||
+		ip.IsLinkLocalUnicast() ||
+		ip.IsLinkLocalMulticast() ||
+		ip.IsUnspecified() ||
+		ip.IsMulticast()
 }

 func (w *Webhook) Send(ctx context.Context, cfg json.RawMessage, secret string, ev Event) error {
@@ -22,6 +84,11 @@ func (w *Webhook) Send(ctx context.Context, cfg json.RawMessage, secret string,
 	if err := json.Unmarshal(cfg, &c); err != nil {
 		return err
 	}
+	if !w.allowPrivate {
+		if err := isAllowedURL(c.URL); err != nil {
+			return err
+		}
+	}
 	body, err := json.Marshal(ev)
 	if err != nil {
 		return err
@@ -31,7 +98,17 @@ func (w *Webhook) Send(ctx context.Context, cfg json.RawMessage, secret string,
 		return err
 	}
 	req.Header.Set("Content-Type", "application/json")
-	resp, err := w.HTTP.Do(req)
+
+	client := w.HTTP
+	if client.CheckRedirect == nil {
+		clientCopy := *client
+		clientCopy.CheckRedirect = func(req *http.Request, via []*http.Request) error {
+			return http.ErrUseLastResponse
+		}
+		client = &clientCopy
+	}
+
+	resp, err := client.Do(req)
 	if err != nil {
 		return err
 	}