fix(phase3): skip templateless domains in scheduler; block CGNAT range in webhook SSRF guard
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BwxdSt4reTm7Dj1oxRvpP3
This commit is contained in:
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user