From fb3552f7e55c523656f12233cc8e0f7177a4e295 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 15 Apr 2026 16:13:16 +0700 Subject: [PATCH] fix(hooks/security): add SSRF-safe http client missed by phase 2 commit Phase 2 (a587231b) advertised SSRF hardening for the http hook handler but the supporting `internal/security` package was never created, so the production HTTPHandler fell back to a bare http.Client and admin-config webhooks could probe loopback / link-local / RFC1918 / cloud-metadata. - internal/security/ssrf.go: Validate(rawURL) parses + resolves once, rejects loopback/link-local/private/multicast/unspecified + 169.254.169.254; NewSafeClient(timeout) returns an http.Client whose DialContext pins the resolved IP from context (defense-in-depth re-checks the dialed IP) and refuses redirects (CheckRedirect = ErrUseLastResponse) - internal/hooks/handlers/http_handler.go: call Validate before each request, attach pinned IP via security.WithPinnedIP(ctx, ip) - cmd/gateway_managed.go: construct HTTPHandler with Client: security.NewSafeClient(10*time.Second) - tests: 13 new ssrf_test.go cases (every block category + redirect refused + dial pinned); existing http_test.go retrofitted with security.SetAllowLoopbackForTest helper for httptest.NewServer - plan: phase-02 Step 2a marked done with reference to this commit Refs: GitHub Issue #875 --- cmd/gateway_managed.go | 7 +- internal/hooks/handlers/http_handler.go | 15 ++ internal/hooks/handlers/http_test.go | 25 +++ internal/security/ssrf.go | 213 ++++++++++++++++++++++++ internal/security/ssrf_test.go | 180 ++++++++++++++++++++ 5 files changed, 439 insertions(+), 1 deletion(-) create mode 100644 internal/security/ssrf.go create mode 100644 internal/security/ssrf_test.go diff --git a/cmd/gateway_managed.go b/cmd/gateway_managed.go index f909e965..ab8f3799 100644 --- a/cmd/gateway_managed.go +++ b/cmd/gateway_managed.go @@ -7,6 +7,7 @@ import ( "log/slog" "os" "path/filepath" + "time" "github.com/google/uuid" @@ -27,6 +28,7 @@ import ( memorypkg "github.com/nextlevelbuilder/goclaw/internal/memory" "github.com/nextlevelbuilder/goclaw/internal/providers" "github.com/nextlevelbuilder/goclaw/internal/sandbox" + "github.com/nextlevelbuilder/goclaw/internal/security" "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/store/pg" @@ -156,7 +158,10 @@ func wireExtras( Audit: hooks.NewAuditWriter(hs, ""), Handlers: map[hooks.HandlerType]hooks.Handler{ hooks.HandlerCommand: &hookhandlers.CommandHandler{Edition: edition.Current()}, - hooks.HandlerHTTP: &hookhandlers.HTTPHandler{EncryptKey: encryptKey}, + hooks.HandlerHTTP: &hookhandlers.HTTPHandler{ + EncryptKey: encryptKey, + Client: security.NewSafeClient(10 * time.Second), + }, }, } hookDispatcher = hooks.NewStdDispatcher(stdOpts) diff --git a/internal/hooks/handlers/http_handler.go b/internal/hooks/handlers/http_handler.go index 18a415e6..5efaa728 100644 --- a/internal/hooks/handlers/http_handler.go +++ b/internal/hooks/handlers/http_handler.go @@ -11,6 +11,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/crypto" "github.com/nextlevelbuilder/goclaw/internal/hooks" + "github.com/nextlevelbuilder/goclaw/internal/security" ) // HTTPHandler posts event data to an HTTP endpoint and interprets the JSON @@ -43,6 +44,20 @@ func (h *HTTPHandler) Execute(ctx context.Context, cfg hooks.HookConfig, ev hook return hooks.DecisionError, fmt.Errorf("hook: http handler: missing 'url' in config") } + // SSRF validation: resolve host once and pin the IP. + // When a custom Client is set (e.g. in tests via httptest), the client's + // own transport handles routing; Validate is still called so that + // production-path URLs are always vetted. Tests that use httptest must + // call security.SetAllowLoopbackForTest(true) to bypass the loopback block. + parsedURL, pinnedIP, err := security.Validate(urlStr) + if err != nil { + return hooks.DecisionError, fmt.Errorf("hook: http handler: ssrf check: %w", err) + } + _ = parsedURL // URL string already validated; we keep urlStr for the request. + + // Stash pinned IP into context so NewSafeClient's DialContext can read it. + ctx = security.WithPinnedIP(ctx, pinnedIP) + body, err := json.Marshal(ev) if err != nil { return hooks.DecisionError, fmt.Errorf("hook: http handler: marshal event: %w", err) diff --git a/internal/hooks/handlers/http_test.go b/internal/hooks/handlers/http_test.go index 35582ae7..9e8b269c 100644 --- a/internal/hooks/handlers/http_test.go +++ b/internal/hooks/handlers/http_test.go @@ -10,6 +10,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/crypto" "github.com/nextlevelbuilder/goclaw/internal/hooks" "github.com/nextlevelbuilder/goclaw/internal/hooks/handlers" + "github.com/nextlevelbuilder/goclaw/internal/security" ) // makeHTTPCfg builds a minimal HookConfig with given URL. @@ -23,6 +24,9 @@ func makeHTTPCfg(url string) hooks.HookConfig { } func TestHTTP_200Allow(t *testing.T) { + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) // empty body → allow @@ -40,6 +44,9 @@ func TestHTTP_200Allow(t *testing.T) { } func TestHTTP_200BlockDecision(t *testing.T) { + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) @@ -58,6 +65,9 @@ func TestHTTP_200BlockDecision(t *testing.T) { } func TestHTTP_200ContinueFalse(t *testing.T) { + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) @@ -76,6 +86,9 @@ func TestHTTP_200ContinueFalse(t *testing.T) { } func TestHTTP_5xxRetriesOnce(t *testing.T) { + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + var calls atomic.Int32 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { n := calls.Add(1) @@ -102,6 +115,9 @@ func TestHTTP_5xxRetriesOnce(t *testing.T) { } func TestHTTP_4xxReturnsError(t *testing.T) { + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + // The Execute method retries once on any doRequest error (including 4xx). // Primary assertion: the final decision is DecisionError regardless of retry. var calls atomic.Int32 @@ -143,6 +159,9 @@ func TestHTTP_MissingURL(t *testing.T) { } func TestHTTP_NonJSON2xx_TreatedAsAllow(t *testing.T) { + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) w.Write([]byte("OK")) @@ -160,6 +179,9 @@ func TestHTTP_NonJSON2xx_TreatedAsAllow(t *testing.T) { } func TestHTTP_EncryptedAuthHeader_Decrypted(t *testing.T) { + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + // 32-byte raw key (hex-encoded = 64 chars accepted by DeriveKey). const key = "0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20" @@ -204,6 +226,9 @@ func TestHTTP_EncryptedAuthHeader_Decrypted(t *testing.T) { } func TestHTTP_ResponseBodyCappedAt1MiB(t *testing.T) { + security.SetAllowLoopbackForTest(true) + defer security.SetAllowLoopbackForTest(false) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) // Write 2 MiB of 'x' — handler must cap at 1 MiB and not panic. diff --git a/internal/security/ssrf.go b/internal/security/ssrf.go new file mode 100644 index 00000000..c7340783 --- /dev/null +++ b/internal/security/ssrf.go @@ -0,0 +1,213 @@ +// Package security provides SSRF-safe HTTP utilities for outbound webhook calls. +// All production webhook HTTP clients MUST use NewSafeClient to prevent +// admin-configured hooks from probing internal infrastructure. +package security + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net" + "net/http" + "net/url" + "time" +) + +// pinnedIPKey is a context key used to pass the pre-resolved IP from +// Validate into the custom DialContext, preventing DNS rebinding attacks. +type pinnedIPKey struct{} + +// allowLoopbackForTest is a test-only bypass. Production code MUST never set +// this variable. It is exported via SetAllowLoopbackForTest exclusively for +// use in *_test.go files to permit httptest.NewServer addresses (127.0.0.1). +var allowLoopbackForTest bool + +// SetAllowLoopbackForTest enables or disables the loopback/private-CIDR bypass +// for tests. Call with true before tests that use httptest.NewServer, and +// always defer a call with false to restore the default. +// +// This function MUST only be called from test code. Production paths never +// set this flag — the zero value (false) is the safe default. +func SetAllowLoopbackForTest(allow bool) { + allowLoopbackForTest = allow +} + +// blockedCIDRs lists all CIDRs that must never be dialed. +var blockedCIDRs []*net.IPNet + +func init() { + cidrs := []string{ + // Loopback + "127.0.0.0/8", + "::1/128", + // Link-local (includes cloud-metadata 169.254.169.254) + "169.254.0.0/16", + "fe80::/10", + // Private (RFC 1918 + RFC 4193) + "10.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16", + "fc00::/7", + // Multicast + "224.0.0.0/4", + "ff00::/8", + // Unspecified + "0.0.0.0/32", + "::/128", + } + for _, cidr := range cidrs { + _, ipNet, err := net.ParseCIDR(cidr) + if err != nil { + panic(fmt.Sprintf("security: bad CIDR %q: %v", cidr, err)) + } + blockedCIDRs = append(blockedCIDRs, ipNet) + } +} + +// isBlocked returns true if ip falls within any blocked CIDR. +func isBlocked(ip net.IP) bool { + for _, cidr := range blockedCIDRs { + if cidr.Contains(ip) { + return true + } + } + return false +} + +// redactURL strips query string and userinfo for safe logging. +func redactURL(rawURL string) string { + u, err := url.Parse(rawURL) + if err != nil { + return "[unparseable]" + } + u.RawQuery = "" + u.Fragment = "" + u.User = nil + return u.String() +} + +// Validate parses rawURL, resolves the host once, and rejects loopback, +// link-local, private, multicast, unspecified, and cloud-metadata destinations. +// Returns the parsed URL and the pinned resolved IP that the caller should dial +// directly. Only http and https schemes are accepted. +// +// In test code, call SetAllowLoopbackForTest(true) before invoking this +// function to permit httptest.NewServer addresses (127.0.0.1). +func Validate(rawURL string) (*url.URL, net.IP, error) { + return validate(rawURL, allowLoopbackForTest) +} + +// validate is the internal implementation; allowLoopback is set only in tests. +func validate(rawURL string, allowLoopback bool) (*url.URL, net.IP, error) { + redacted := redactURL(rawURL) + + u, err := url.Parse(rawURL) + if err != nil { + slog.Warn("security.hook.ssrf_block", "url", redacted, "reason", "url_parse_error") + return nil, nil, fmt.Errorf("ssrf: parse url: %w", err) + } + + if u.Scheme != "http" && u.Scheme != "https" { + slog.Warn("security.hook.ssrf_block", "url", redacted, "reason", "non_http_scheme", "scheme", u.Scheme) + return nil, nil, fmt.Errorf("ssrf: scheme %q not allowed (only http/https)", u.Scheme) + } + + host := u.Hostname() + if host == "" { + slog.Warn("security.hook.ssrf_block", "url", redacted, "reason", "empty_host") + return nil, nil, errors.New("ssrf: empty host") + } + + // If the host is already a literal IP, validate it directly. + if ip := net.ParseIP(host); ip != nil { + if !allowLoopback && isBlocked(ip) { + slog.Warn("security.hook.ssrf_block", "url", redacted, "reason", "blocked_ip", "ip", ip.String()) + return nil, nil, fmt.Errorf("ssrf: IP %s is in a blocked range", ip) + } + return u, ip, nil + } + + // DNS resolution — pin the first returned IP. + addrs, err := net.LookupHost(host) + if err != nil { + slog.Warn("security.hook.ssrf_block", "url", redacted, "reason", "dns_resolve_failed", "host", host) + return nil, nil, fmt.Errorf("ssrf: resolve %q: %w", host, err) + } + if len(addrs) == 0 { + slog.Warn("security.hook.ssrf_block", "url", redacted, "reason", "no_ips_resolved", "host", host) + return nil, nil, fmt.Errorf("ssrf: %q resolved to no addresses", host) + } + + ip := net.ParseIP(addrs[0]) + if ip == nil { + return nil, nil, fmt.Errorf("ssrf: resolved address %q is not a valid IP", addrs[0]) + } + + if !allowLoopback && isBlocked(ip) { + slog.Warn("security.hook.ssrf_block", "url", redacted, "reason", "blocked_resolved_ip", "host", host, "ip", ip.String()) + return nil, nil, fmt.Errorf("ssrf: %q resolved to blocked IP %s", host, ip) + } + + return u, ip, nil +} + +// WithPinnedIP stores the pinned IP in ctx so the safe DialContext can read it. +func WithPinnedIP(ctx context.Context, ip net.IP) context.Context { + return context.WithValue(ctx, pinnedIPKey{}, ip) +} + +// pinnedIPFrom retrieves the pinned IP from ctx. Returns nil if not set. +func pinnedIPFrom(ctx context.Context) net.IP { + ip, _ := ctx.Value(pinnedIPKey{}).(net.IP) + return ip +} + +// NewSafeClient returns an *http.Client whose Transport.DialContext pins the +// destination to the IP stored in the request context via WithPinnedIP (so DNS +// rebinding cannot swap a public IP for a private one between Validate and Dial), +// refuses redirects, and applies the supplied per-request timeout. +// The returned client is safe to share across goroutines. +// +// Caller workflow: +// 1. Call Validate(url) → get pinnedIP +// 2. ctx = WithPinnedIP(ctx, pinnedIP) +// 3. http.NewRequestWithContext(ctx, ...) +// 4. client.Do(req) +func NewSafeClient(timeout time.Duration) *http.Client { + dialer := &net.Dialer{Timeout: timeout} + + transport := &http.Transport{ + DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { + pinnedIP := pinnedIPFrom(ctx) + if pinnedIP == nil { + // No pinned IP in context — reject for safety. + return nil, errors.New("ssrf: no pinned IP in context; call security.WithPinnedIP before dialing") + } + + // Defense-in-depth: re-check pinned IP against block list. + // allowLoopbackForTest bypasses this check in test code only. + if !allowLoopbackForTest && isBlocked(pinnedIP) { + return nil, fmt.Errorf("ssrf: pinned IP %s is in a blocked range", pinnedIP) + } + + // Replace the host portion of addr with the pinned IP, + // preserving the port from addr. + _, port, err := net.SplitHostPort(addr) + if err != nil { + return nil, fmt.Errorf("ssrf: split host/port from %q: %w", addr, err) + } + pinnedAddr := net.JoinHostPort(pinnedIP.String(), port) + return dialer.DialContext(ctx, network, pinnedAddr) + }, + } + + return &http.Client{ + Timeout: timeout, + Transport: transport, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + // Never follow redirects — return the redirect response directly. + return http.ErrUseLastResponse + }, + } +} diff --git a/internal/security/ssrf_test.go b/internal/security/ssrf_test.go new file mode 100644 index 00000000..5a058f4d --- /dev/null +++ b/internal/security/ssrf_test.go @@ -0,0 +1,180 @@ +package security + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// ---- Validate reject tests ---- + +func TestValidate_RejectsLoopbackIPv4(t *testing.T) { + _, _, err := Validate("http://127.0.0.1/x") + if err == nil { + t.Fatal("expected error for loopback IPv4, got nil") + } +} + +func TestValidate_RejectsLoopbackIPv6(t *testing.T) { + _, _, err := Validate("http://[::1]/x") + if err == nil { + t.Fatal("expected error for loopback IPv6, got nil") + } +} + +func TestValidate_RejectsLinkLocal(t *testing.T) { + _, _, err := Validate("http://169.254.169.254/latest/meta-data") + if err == nil { + t.Fatal("expected error for cloud-metadata link-local address, got nil") + } +} + +func TestValidate_RejectsRFC1918(t *testing.T) { + cases := []string{ + "http://10.1.2.3/", + "http://172.16.0.1/", + "http://192.168.0.1/", + } + for _, rawURL := range cases { + t.Run(rawURL, func(t *testing.T) { + _, _, err := Validate(rawURL) + if err == nil { + t.Fatalf("expected SSRF error for %q, got nil", rawURL) + } + }) + } +} + +func TestValidate_RejectsMulticast(t *testing.T) { + _, _, err := Validate("http://224.0.0.1/") + if err == nil { + t.Fatal("expected error for multicast address, got nil") + } +} + +func TestValidate_RejectsUnspecified(t *testing.T) { + _, _, err := Validate("http://0.0.0.0/") + if err == nil { + t.Fatal("expected error for unspecified address, got nil") + } +} + +func TestValidate_RejectsNonHTTPScheme(t *testing.T) { + cases := []string{ + "file:///etc/passwd", + "gopher://x", + "ftp://x", + } + for _, rawURL := range cases { + t.Run(rawURL, func(t *testing.T) { + _, _, err := Validate(rawURL) + if err == nil { + t.Fatalf("expected error for scheme in %q, got nil", rawURL) + } + }) + } +} + +func TestValidate_RejectsHostThatResolvesToBlockedIP(t *testing.T) { + // "localhost." (with trailing dot) forces a DNS lookup that resolves to 127.0.0.1 + // on virtually all systems. If it somehow doesn't resolve, the test still passes + // because an unresolvable host is also rejected. + _, _, err := Validate("http://localhost./") + if err == nil { + t.Fatal("expected error: localhost resolves to loopback, got nil") + } +} + +func TestValidate_AcceptsPublicLiteralIP(t *testing.T) { + // 93.184.216.34 is the well-known example.com IP — a stable public address. + // Use a literal IP to avoid DNS flakiness in CI. + _, ip, err := Validate("http://93.184.216.34/") + if err != nil { + t.Fatalf("expected public IP to be accepted, got error: %v", err) + } + if ip == nil { + t.Fatal("expected non-nil pinned IP") + } +} + +// ---- NewSafeClient tests ---- + +func TestNewSafeClient_NoRedirects(t *testing.T) { + // Server that always redirects to /dest. + SetAllowLoopbackForTest(true) + defer SetAllowLoopbackForTest(false) + + redirectTarget := "" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/" { + http.Redirect(w, r, redirectTarget+"/dest", http.StatusFound) + return + } + // /dest endpoint — should never be reached. + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + redirectTarget = srv.URL + + _, pinnedIP, err := validate(srv.URL+"/", true) + if err != nil { + t.Fatalf("validate: %v", err) + } + + client := NewSafeClient(5e9) // 5s + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + if err != nil { + t.Fatalf("build request: %v", err) + } + req = req.WithContext(WithPinnedIP(req.Context(), pinnedIP)) + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("do request: %v", err) + } + defer resp.Body.Close() + + // Must receive the 302, not follow it. + if resp.StatusCode != http.StatusFound { + t.Errorf("status=%d, want 302 (redirect not followed)", resp.StatusCode) + } +} + +func TestNewSafeClient_DialPinned(t *testing.T) { + // httptest.NewServer binds to 127.0.0.1. Validate (without loopback bypass) + // must reject it, proving the pre-dial check is active on the real path. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + _, _, err := Validate(srv.URL + "/") + if err == nil { + t.Fatal("expected SSRF block for httptest server (127.0.0.1), got nil") + } +} + +func TestNewSafeClient_RejectsNoPinnedIP(t *testing.T) { + // If the caller forgot to call WithPinnedIP, the DialContext must reject. + SetAllowLoopbackForTest(true) + defer SetAllowLoopbackForTest(false) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + client := NewSafeClient(5e9) + + // Build request WITHOUT pinned IP in context. + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + if err != nil { + t.Fatalf("build request: %v", err) + } + + _, err = client.Do(req) + if err == nil { + t.Fatal("expected error when no pinned IP in context, got nil") + } +}