Improve link checker handling of unsafe links

Author: dacharyc

links/check.go

@@ -2,11 +2,9 @@ package links
 
 import (
 	"context"
-	"fmt"
 	"net/http"
 	"strings"
 	"sync"
-	"time"
 
 	"github.com/agent-ecosystem/skill-validator/types"
 )
@@ -47,15 +45,8 @@ func CheckLinks(ctx context.Context, dir, body string) []types.Result {
 	}
 
 	// Shared client for connection reuse across concurrent checks.
-	client := &http.Client{
-		Timeout: 10 * time.Second,
-		CheckRedirect: func(req *http.Request, via []*http.Request) error {
-			if len(via) >= 10 {
-				return fmt.Errorf("too many redirects")
-			}
-			return nil
-		},
-	}
+	// The client uses a safe transport that blocks requests to private IPs.
+	client := newHTTPClient()
 
 	// Check HTTP links concurrently
 	httpResults := make([]linkResult, len(httpLinks))

links/check_test.go

@@ -85,6 +85,12 @@ func TestCheckLinks_SkipsRelative(t *testing.T) {
 }
 
 func TestCheckLinks_HTTP(t *testing.T) {
+	// httptest servers listen on 127.0.0.1, which the safe transport blocks.
+	// Swap in an unrestricted client for these integration tests.
+	orig := newHTTPClient
+	newHTTPClient = func() *http.Client { return testHTTPClient() }
+	t.Cleanup(func() { newHTTPClient = orig })
+
 	mux := http.NewServeMux()
 	mux.HandleFunc("/ok", func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusOK)

links/safenet.go

@@ -0,0 +1,82 @@
+package links
+
+import (
+	"context"
+	"fmt"
+	"net"
+	"net/http"
+	"time"
+)
+
+// privateRanges are the IPv4 CIDR blocks that should not be reachable
+// via link validation. This covers loopback, RFC 1918 private networks,
+// link-local, and the cloud metadata endpoint range.
+var privateRanges []*net.IPNet
+
+func init() {
+	for _, cidr := range []string{
+		"127.0.0.0/8",    // loopback
+		"10.0.0.0/8",     // RFC 1918
+		"172.16.0.0/12",  // RFC 1918
+		"192.168.0.0/16", // RFC 1918
+		"169.254.0.0/16", // link-local
+		"::1/128",        // IPv6 loopback
+		"fc00::/7",       // IPv6 unique local
+		"fe80::/10",      // IPv6 link-local
+	} {
+		_, block, _ := net.ParseCIDR(cidr)
+		privateRanges = append(privateRanges, block)
+	}
+}
+
+// isPrivateIP reports whether ip falls within a private or reserved range.
+func isPrivateIP(ip net.IP) bool {
+	for _, block := range privateRanges {
+		if block.Contains(ip) {
+			return true
+		}
+	}
+	return false
+}
+
+// safeTransport returns an *http.Transport whose DialContext resolves the
+// target hostname and refuses to connect if the resolved IP is private.
+// This prevents SSRF when following URLs extracted from untrusted skill content.
+func safeTransport() *http.Transport {
+	dialer := &net.Dialer{Timeout: 5 * time.Second}
+	return &http.Transport{
+		DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
+			host, port, err := net.SplitHostPort(addr)
+			if err != nil {
+				return nil, err
+			}
+			ips, err := net.DefaultResolver.LookupIPAddr(ctx, host)
+			if err != nil {
+				return nil, err
+			}
+			for _, ip := range ips {
+				if isPrivateIP(ip.IP) {
+					return nil, fmt.Errorf("link validation blocked request to private address %s (%s)", ip.IP, host)
+				}
+			}
+			// Connect to the first resolved address.
+			return dialer.DialContext(ctx, network, net.JoinHostPort(ips[0].IP.String(), port))
+		},
+	}
+}
+
+// newHTTPClient creates the HTTP client used for link checking. It is a
+// package-level variable so tests can replace it with a client that permits
+// loopback connections to httptest servers.
+var newHTTPClient = func() *http.Client {
+	return &http.Client{
+		Transport: safeTransport(),
+		Timeout:   10 * time.Second,
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			if len(via) >= 10 {
+				return fmt.Errorf("too many redirects")
+			}
+			return nil
+		},
+	}
+}

links/safenet_test.go

@@ -0,0 +1,73 @@
+package links
+
+import (
+	"net"
+	"testing"
+)
+
+func TestIsPrivateIP(t *testing.T) {
+	tests := []struct {
+		ip      string
+		private bool
+	}{
+		// IPv4 private/reserved ranges
+		{"127.0.0.1", true},
+		{"127.0.0.2", true},
+		{"10.0.0.1", true},
+		{"10.255.255.255", true},
+		{"172.16.0.1", true},
+		{"172.31.255.255", true},
+		{"192.168.0.1", true},
+		{"192.168.255.255", true},
+		{"169.254.169.254", true}, // cloud metadata
+		{"169.254.0.1", true},
+
+		// IPv6 private/reserved ranges
+		{"::1", true},                       // IPv6 loopback
+		{"fc00::1", true},                   // IPv6 unique local
+		{"fe80::1", true},                   // IPv6 link-local
+		{"8.8.8.8", false},                  // Google DNS
+		{"93.184.216.34", false},            // example.com
+		{"172.32.0.1", false},               // just outside 172.16/12
+		{"192.169.0.1", false},              // just outside 192.168/16
+		{"2607:f8b0:4004:800::200e", false}, // Google public IPv6
+	}
+	for _, tt := range tests {
+		t.Run(tt.ip, func(t *testing.T) {
+			ip := net.ParseIP(tt.ip)
+			if ip == nil {
+				t.Fatalf("failed to parse IP %s", tt.ip)
+			}
+			got := isPrivateIP(ip)
+			if got != tt.private {
+				t.Errorf("isPrivateIP(%s) = %v, want %v", tt.ip, got, tt.private)
+			}
+		})
+	}
+}
+
+func TestSafeTransportBlocksPrivateIPs(t *testing.T) {
+	// CheckLinks with the default safe client should block links to private IPs.
+	// We don't need a server running; the dialer should refuse before connecting.
+	dir := t.TempDir()
+	body := "[metadata](http://169.254.169.254/latest/meta-data/)"
+	results := CheckLinks(t.Context(), dir, body)
+	if len(results) == 0 {
+		t.Fatal("expected a result for blocked private IP link")
+	}
+	r := results[0]
+	if r.Message == "" {
+		t.Fatal("expected non-empty message")
+	}
+	requireContains(t, r.Message, "request failed")
+}
+
+func TestSafeTransportBlocksLocalhost(t *testing.T) {
+	dir := t.TempDir()
+	body := "[local](http://127.0.0.1:8080/admin)"
+	results := CheckLinks(t.Context(), dir, body)
+	if len(results) == 0 {
+		t.Fatal("expected a result for blocked localhost link")
+	}
+	requireContains(t, results[0].Message, "request failed")
+}