From 504c4c081fc018bb644756518695e51b6079958d Mon Sep 17 00:00:00 2001 From: Vassiliy Yegorov Date: Sat, 4 Jul 2026 14:58:09 +0700 Subject: [PATCH] fix(phase3): skip templateless domains in scheduler; block CGNAT range in webhook SSRF guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Domains imported without a template (TemplateID == nil) are a valid, unconfigured state, not a failure — RunOnce now skips them before calling checkDomain instead of letting LoadDomain's "no template" error turn into StatusError and a spammy unknown->error notification. isBlockedIP now also rejects 100.64.0.0/10 (RFC 6598 carrier-grade NAT), which net.IP.IsPrivate() does not cover, closing an SSRF gap in the webhook destination guard (both the pre-request check and the per-dial check use isBlockedIP). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01BwxdSt4reTm7Dj1oxRvpP3 --- internal/notify/notify_test.go | 26 +++++++++++ internal/notify/webhook.go | 25 +++++++++-- internal/scheduler/scheduler.go | 10 +++++ internal/scheduler/scheduler_test.go | 65 +++++++++++++++++++++++++--- 4 files changed, 117 insertions(+), 9 deletions(-) diff --git a/internal/notify/notify_test.go b/internal/notify/notify_test.go index 9c1126d..62a7c0d 100644 --- a/internal/notify/notify_test.go +++ b/internal/notify/notify_test.go @@ -3,6 +3,7 @@ package notify import ( "context" "encoding/json" + "net" "net/http" "net/http/httptest" "strings" @@ -195,6 +196,31 @@ func TestDialControlBlocksActualConnectingAddress(t *testing.T) { } } +func TestIsBlockedIPCGNATRange(t *testing.T) { + cases := []struct { + name string + ip string + blocked bool + }{ + {"cgnat start", "100.64.0.1", true}, + {"cgnat end", "100.127.255.255", true}, + {"just below cgnat", "100.63.255.255", false}, + {"just above cgnat", "100.128.0.0", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ip := net.ParseIP(tc.ip) + if ip == nil { + t.Fatalf("failed to parse %q", tc.ip) + } + got := isBlockedIP(ip) + if got != tc.blocked { + t.Fatalf("isBlockedIP(%q) = %v, want %v", tc.ip, got, tc.blocked) + } + }) + } +} + func TestDialControlAllowsEverythingWhenAllowPrivate(t *testing.T) { control := dialControl(true) if err := control("tcp", "127.0.0.1:80", nil); err != nil { diff --git a/internal/notify/webhook.go b/internal/notify/webhook.go index d15a5ea..60567d5 100644 --- a/internal/notify/webhook.go +++ b/internal/notify/webhook.go @@ -83,17 +83,34 @@ func isAllowedURL(rawurl string) error { return nil } +// cgnatBlock is the shared address space reserved for carrier-grade NAT +// (RFC 6598, 100.64.0.0/10). net.IP.IsPrivate() only covers RFC1918/RFC4193 +// and does not treat this range as private, so it must be checked +// explicitly or CGNAT-addressed internal services would be reachable via +// webhook SSRF. +var cgnatBlock = func() *net.IPNet { + _, block, err := net.ParseCIDR("100.64.0.0/10") + if err != nil { + panic(err) + } + return block +}() + // isBlockedIP reports whether ip must never be connected to: loopback, -// private (RFC1918 etc.), link-local, unspecified, or multicast. Used both -// by isAllowedURL's pre-request check and by dialControl's per-connection -// check. +// private (RFC1918 etc.), link-local, unspecified, multicast, or +// carrier-grade NAT (RFC 6598). Used both by isAllowedURL's pre-request +// check and by dialControl's per-connection check. func isBlockedIP(ip net.IP) bool { + if v4 := ip.To4(); v4 != nil { + ip = v4 + } return ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() || - ip.IsMulticast() + ip.IsMulticast() || + cgnatBlock.Contains(ip) } // dialControl returns a net.Dialer.Control function enforcing the SSRF guard diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index c470d09..240ad9d 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -102,6 +102,16 @@ func (s *Scheduler) RunOnce(ctx context.Context, now time.Time) error { } for _, d := range domains { + // A domain with no template attached is not yet configured for + // checking (a valid, expected state right after import) — not a + // failure. Checking it would make LoadDomain return "domain has + // no template", turning into a StatusError that spams a + // notification and shows a red badge for a domain the user + // simply hasn't set up yet. Skip it silently: no check, no + // status change, no notification. + if d.TemplateID == nil { + continue + } s.checkDomain(ctx, sch.ProjectID, d, now) } diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index dda06ee..280e1d6 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -75,13 +75,19 @@ func (m *mockStore) CountDriftDomains(ctx context.Context) (int, error) { return m.driftCount, nil } -// mockChecker returns a preset Changeset or error per domainID. +// mockChecker returns a preset Changeset or error per domainID, and records +// which domain IDs it was called with. type mockChecker struct { + mu sync.Mutex results map[uuid.UUID]diff.Changeset errs map[uuid.UUID]error + calls []uuid.UUID } func (c *mockChecker) Check(ctx context.Context, projectID, domainID uuid.UUID) (diff.Changeset, error) { + c.mu.Lock() + c.calls = append(c.calls, domainID) + c.mu.Unlock() if err, ok := c.errs[domainID]; ok { return diff.Changeset{}, err } @@ -113,8 +119,9 @@ func driftChangeset() diff.Changeset { func TestRunOnce_NotifiesOnDriftNotOnFirstInSync(t *testing.T) { projectID := uuid.New() - domainA := store.Domain{ID: uuid.New(), ProjectID: projectID} - domainB := store.Domain{ID: uuid.New(), ProjectID: projectID} + templateID := uuid.New() + domainA := store.Domain{ID: uuid.New(), ProjectID: projectID, TemplateID: &templateID} + domainB := store.Domain{ID: uuid.New(), ProjectID: projectID, TemplateID: &templateID} st := newMockStore() st.schedules = []store.Schedule{{ID: uuid.New(), ProjectID: projectID, IntervalSeconds: 3600, Enabled: true}} @@ -175,7 +182,8 @@ func TestRunOnce_NotifiesOnDriftNotOnFirstInSync(t *testing.T) { func TestRunOnce_Idempotent_NoRepeatNotifyOnUnchangedDrift(t *testing.T) { projectID := uuid.New() - domainA := store.Domain{ID: uuid.New(), ProjectID: projectID} + templateID := uuid.New() + domainA := store.Domain{ID: uuid.New(), ProjectID: projectID, TemplateID: &templateID} st := newMockStore() st.schedules = []store.Schedule{{ID: uuid.New(), ProjectID: projectID, IntervalSeconds: 3600, Enabled: true}} @@ -205,7 +213,8 @@ func TestRunOnce_Idempotent_NoRepeatNotifyOnUnchangedDrift(t *testing.T) { func TestRunOnce_CheckError_StatusErrorAndNotify(t *testing.T) { projectID := uuid.New() - domainA := store.Domain{ID: uuid.New(), ProjectID: projectID} + templateID := uuid.New() + domainA := store.Domain{ID: uuid.New(), ProjectID: projectID, TemplateID: &templateID} st := newMockStore() st.schedules = []store.Schedule{{ID: uuid.New(), ProjectID: projectID, IntervalSeconds: 3600, Enabled: true}} @@ -236,6 +245,52 @@ func TestRunOnce_CheckError_StatusErrorAndNotify(t *testing.T) { } } +func TestRunOnce_SkipsDomainWithoutTemplate(t *testing.T) { + projectID := uuid.New() + templateID := uuid.New() + domainNoTemplate := store.Domain{ID: uuid.New(), ProjectID: projectID, TemplateID: nil} + domainWithTemplate := store.Domain{ID: uuid.New(), ProjectID: projectID, TemplateID: &templateID} + + st := newMockStore() + st.schedules = []store.Schedule{{ID: uuid.New(), ProjectID: projectID, IntervalSeconds: 3600, Enabled: true}} + st.domains[projectID] = []store.Domain{domainNoTemplate, domainWithTemplate} + + checker := &mockChecker{ + results: map[uuid.UUID]diff.Changeset{domainWithTemplate.ID: {}}, + } + notifier := &mockNotifier{} + m := metrics.New() + sched := New(st, checker, notifier, m) + + if err := sched.RunOnce(context.Background(), time.Now()); err != nil { + t.Fatalf("RunOnce: %v", err) + } + + for _, id := range checker.calls { + if id == domainNoTemplate.ID { + t.Fatalf("Checker.Check was called for templateless domain %s, want skipped", id) + } + } + if len(checker.calls) != 1 || checker.calls[0] != domainWithTemplate.ID { + t.Fatalf("Checker.Check calls = %v, want exactly [%s]", checker.calls, domainWithTemplate.ID) + } + + if _, ok := st.status[domainNoTemplate.ID]; ok { + t.Fatalf("templateless domain status = %q, want no status set (never checked)", st.status[domainNoTemplate.ID]) + } + if st.status[domainWithTemplate.ID] != StatusInSync { + t.Fatalf("domain with template status = %q, want in_sync", st.status[domainWithTemplate.ID]) + } + + if got := notifier.count(); got != 0 { + t.Fatalf("notifications sent = %d, want 0 (templateless skip is silent, and template domain unknown->in_sync is not news)", got) + } + + if got := testutil.ToFloat64(m.ChecksTotal.WithLabelValues(StatusInSync)); got != 1 { + t.Fatalf("ChecksTotal{in_sync} = %v, want 1 (only the templated domain was checked)", got) + } +} + func TestShouldNotify(t *testing.T) { cases := []struct { name string