From 29f448d4b5743fee6644aefabf3e9a06f17472f0 Mon Sep 17 00:00:00 2001 From: Vassiliy Yegorov Date: Sat, 4 Jul 2026 13:40:29 +0700 Subject: [PATCH] =?UTF-8?q?fix(sec):=20=D1=81=D0=B0=D0=BD=D0=B8=D1=82?= =?UTF-8?q?=D0=B8=D0=B7=D0=B0=D1=86=D0=B8=D1=8F=20Telegram-=D0=BE=D1=88?= =?UTF-8?q?=D0=B8=D0=B1=D0=BE=D0=BA,=20SSRF-guard=20Webhook,=20=D1=87?= =?UTF-8?q?=D0=B8=D1=81=D1=82=D0=BA=D0=B0=20=D0=BB=D0=BE=D0=B3=D0=BE=D0=B2?= =?UTF-8?q?=20test-=D0=BA=D0=B0=D0=BD=D0=B0=D0=BB=D0=B0,=20go=20mod=20tidy?= =?UTF-8?q?,=20histogram-=D0=B1=D0=B0=D0=BA=D0=B5=D1=82=D1=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- go.mod | 2 +- go.sum | 2 + internal/api/schedule_handlers.go | 6 ++- internal/metrics/metrics.go | 6 +++ internal/metrics/metrics_test.go | 20 ++++++++ internal/notify/notify_test.go | 82 ++++++++++++++++++++++++++++++- internal/notify/telegram.go | 6 ++- internal/notify/webhook.go | 79 ++++++++++++++++++++++++++++- 8 files changed, 197 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 984679c..01466a4 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/google/uuid v1.6.0 github.com/jackc/pgx/v5 v5.10.0 github.com/pressly/goose/v3 v3.27.2 + github.com/prometheus/client_golang v1.23.2 github.com/testcontainers/testcontainers-go v0.43.0 github.com/testcontainers/testcontainers-go/modules/postgres v0.43.0 golang.org/x/crypto v0.53.0 @@ -55,7 +56,6 @@ require ( github.com/opencontainers/image-spec v1.1.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/power-devops/perfstat v0.0.0-20240221224432-82ca36839d55 // indirect - github.com/prometheus/client_golang v1.23.2 // indirect github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.66.1 // indirect github.com/prometheus/procfs v0.20.1 // indirect diff --git a/go.sum b/go.sum index 19cda10..221e696 100644 --- a/go.sum +++ b/go.sum @@ -162,6 +162,8 @@ go.opentelemetry.io/otel/sdk/metric v1.43.0 h1:S88dyqXjJkuBNLeMcVPRFXpRw2fuwdvfC go.opentelemetry.io/otel/sdk/metric v1.43.0/go.mod h1:C/RJtwSEJ5hzTiUz5pXF1kILHStzb9zFlIEe85bhj6A= go.opentelemetry.io/otel/trace v1.43.0 h1:BkNrHpup+4k4w+ZZ86CZoHHEkohws8AY+WTX09nk+3A= go.opentelemetry.io/otel/trace v1.43.0/go.mod h1:/QJhyVBUUswCphDVxq+8mld+AvhXZLhe+8WVFxiFff0= +go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= +go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0= go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y= go.yaml.in/yaml/v2 v2.4.2 h1:DzmwEr2rDGHl7lsFgAHxmNz/1NlQ7xLIrlN2h5d1eGI= diff --git a/internal/api/schedule_handlers.go b/internal/api/schedule_handlers.go index 7bc6e52..099b957 100644 --- a/internal/api/schedule_handlers.go +++ b/internal/api/schedule_handlers.go @@ -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 } diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 5a80224..cd730e1 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -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", diff --git a/internal/metrics/metrics_test.go b/internal/metrics/metrics_test.go index bb19568..9968a7b 100644 --- a/internal/metrics/metrics_test.go +++ b/internal/metrics/metrics_test.go @@ -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) diff --git a/internal/notify/notify_test.go b/internal/notify/notify_test.go index 835d4b4..329bac4 100644 --- a/internal/notify/notify_test.go +++ b/internal/notify/notify_test.go @@ -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/. + 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 { diff --git a/internal/notify/telegram.go b/internal/notify/telegram.go index 57d85b0..4a5a768 100644 --- a/internal/notify/telegram.go +++ b/internal/notify/telegram.go @@ -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/...). A + // caller logging this error would leak the secret. + return errors.New("telegram: request failed") } defer resp.Body.Close() if resp.StatusCode >= 300 { diff --git a/internal/notify/webhook.go b/internal/notify/webhook.go index 06e62c2..1f86e23 100644 --- a/internal/notify/webhook.go +++ b/internal/notify/webhook.go @@ -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 }