Ssrf guarded client

docs/CONFIGURATION.md

@@ -56,16 +56,20 @@ For more rmapi-specific configuration, see [their documentation](https://github.
 
 | Variable                 | Required? | Default | Description |
 |--------------------------|-----------|---------|-------------|
-| BLOCK_PRIVATE_IPS        | No        | false   | Set to `true` to block URLs pointing to private/local IP addresses (RFC1918, loopback, link-local) |
+| BLOCK_PRIVATE_IPS        | No        | true    | Set to `false` to allow URLs pointing to private/local IP addresses (RFC1918, loopback). Link-local is always blocked. |
 | BLOCKED_DOMAINS          | No        |         | Comma-separated list of domains to block (e.g., `internal.corp,local.net`) |
 
 ### Security Configuration Notes
 
-- **BLOCK_PRIVATE_IPS**: When enabled, prevents Server-Side Request Forgery (SSRF) attacks by blocking URLs that resolve to:
+- **Link-local addresses are always blocked**, whatever `BLOCK_PRIVATE_IPS` is set to: 169.254.0.0/16 and fe80::/10. This range carries the cloud instance metadata endpoints (169.254.169.254 and 169.254.170.2), and no deployment serves documents from it. Every address is checked at connection time, including each hop of a redirect, so a redirect or a DNS record that changes between check and fetch cannot reach a blocked address.
+- **BLOCK_PRIVATE_IPS**: Left at the default, Aviary refuses to fetch from your own network. Set it to `false` if you point Aviary at a host on your LAN, at a Docker Compose sibling by service name, or at a machine on your tailnet. With the default in place, these are refused:
   - Private IPv4 ranges: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16
   - Loopback addresses: 127.0.0.0/8, ::1
-  - Link-local addresses: 169.254.0.0/16, fe80::/10
+  - Carrier-grade NAT: 100.64.0.0/10
+  - Unique local IPv6: fc00::/7
   - Other special-use addresses
+
+  A refused address is reported to the user as a blocked address, and the server log names the address and this variable.
 - **BLOCKED_DOMAINS**: Blocks specific domains and their subdomains. For example, setting `BLOCKED_DOMAINS=example.com` will block both `example.com` and `*.example.com`
 
 ## Multi-User Mode Configuration

internal/converter/reader.go

@@ -66,9 +66,8 @@ func ExtractFromURL(urlStr string) (*ArticleContent, error) {
 	}
 	req.Header.Set("User-Agent", downloader.PickUA())
 
-	client := &http.Client{
-		Timeout: 30 * time.Second,
-	}
+	client := security.NewHTTPClient()
+	client.Timeout = 30 * time.Second
 	// codeql[go/request-forgery]: URL is validated by security.ValidateURL above
 	resp, err := client.Do(req)
 	if err != nil {
@@ -201,15 +200,18 @@ func extractImageURLs(html string) []string {
 func DownloadImage(imageURL, outputPath string) error {
 	logging.Logf("[READER] DownloadImage: fetching %s", imageURL)
 
+	if err := security.ValidateURL(imageURL); err != nil {
+		return fmt.Errorf("URL validation failed: %w", err)
+	}
+
 	req, err := http.NewRequest("GET", imageURL, nil)
 	if err != nil {
 		return fmt.Errorf("failed to create request: %w", err)
 	}
 	req.Header.Set("User-Agent", downloader.PickUA())
 
-	client := &http.Client{
-		Timeout: 30 * time.Second,
-	}
+	client := security.NewHTTPClient()
+	client.Timeout = 30 * time.Second
 	resp, err := client.Do(req)
 	if err != nil {
 		return fmt.Errorf("failed to download image: %w", err)

internal/downloader/client.go

@@ -1,10 +1,10 @@
 package downloader
 
 import (
-	"net/http"
 	"time"
 
 	"github.com/rmitchellscott/aviary/internal/config"
+	"github.com/rmitchellscott/aviary/internal/security"
 )
 
 // Clients used for HTTP requests. Timeouts are configured via environment
@@ -15,8 +15,8 @@ import (
 var (
 	sniffTimeout    = 30 * time.Second
 	downloadTimeout = 60 * time.Second
-	sniffClient     = &http.Client{}
-	downloadClient  = &http.Client{}
+	sniffClient     = security.NewHTTPClient()
+	downloadClient  = security.NewHTTPClient()
 )
 
 func init() {

internal/downloader/handler.go

@@ -4,6 +4,7 @@ import (
 	"net/http"
 
 	"github.com/gin-gonic/gin"
+	"github.com/rmitchellscott/aviary/internal/security"
 )
 
 // SniffHandler responds with the MIME type of the ?url parameter.
@@ -16,6 +17,10 @@ func SniffHandler(c *gin.Context) {
 
 	mt, err := SniffMime(urlStr)
 	if err != nil {
+		if security.IsBlockedAddress(err) {
+			c.JSON(http.StatusForbidden, gin.H{"error": "backend.status.blocked_address"})
+			return
+		}
 		c.JSON(http.StatusInternalServerError, gin.H{"error": "backend.status.internal_error"})
 		return
 	}

internal/downloader/sniff_test.go

@@ -5,11 +5,16 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
 	"testing"
 	"time"
 )
 
 func TestSniffMimeTimeout(t *testing.T) {
+	// the test server listens on loopback, which is blocked by default
+	os.Setenv("BLOCK_PRIVATE_IPS", "false")
+	defer os.Unsetenv("BLOCK_PRIVATE_IPS")
+
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		time.Sleep(200 * time.Millisecond)
 		w.WriteHeader(http.StatusOK)

internal/security/httpclient.go

@@ -0,0 +1,76 @@
+package security
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"net"
+	"net/http"
+	"time"
+)
+
+const maxRedirects = 10
+
+var ErrTooManyRedirects = errors.New("too many redirects")
+
+// NewHTTPClient returns a client that validates every address it connects to.
+// Callers set Timeout themselves. Validation at dial time covers each redirect
+// hop and the address the connection actually reaches, which a check against
+// the requested URL alone does not.
+func NewHTTPClient() *http.Client {
+	return &http.Client{
+		Transport:     newGuardedTransport(),
+		CheckRedirect: checkRedirect,
+	}
+}
+
+func checkRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= maxRedirects {
+		return ErrTooManyRedirects
+	}
+	return ValidateURL(req.URL.String())
+}
+
+func newGuardedTransport() *http.Transport {
+	dialer := &net.Dialer{
+		Timeout:   30 * time.Second,
+		KeepAlive: 30 * time.Second,
+	}
+
+	transport := http.DefaultTransport.(*http.Transport).Clone()
+	transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+		return dialGuarded(ctx, dialer, network, addr)
+	}
+
+	return transport
+}
+
+// dialGuarded resolves the host itself and connects to a validated address, so
+// the connection cannot land somewhere a second resolution would have returned.
+func dialGuarded(ctx context.Context, dialer *net.Dialer, network, addr string) (net.Conn, error) {
+	host, port, err := net.SplitHostPort(addr)
+	if err != nil {
+		return nil, err
+	}
+
+	addrs, err := net.DefaultResolver.LookupIPAddr(ctx, host)
+	if err != nil {
+		return nil, fmt.Errorf("%w: %v", ErrIPResolutionFailed, err)
+	}
+
+	lastErr := error(fmt.Errorf("%w: no IPs found for hostname", ErrIPResolutionFailed))
+	for _, resolved := range addrs {
+		if err := checkIPAllowed(resolved.IP); err != nil {
+			lastErr = err
+			continue
+		}
+
+		conn, err := dialer.DialContext(ctx, network, net.JoinHostPort(resolved.IP.String(), port))
+		if err == nil {
+			return conn, nil
+		}
+		lastErr = err
+	}
+
+	return nil, lastErr
+}

internal/security/httpclient_test.go

@@ -0,0 +1,107 @@
+package security
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"testing"
+	"time"
+)
+
+func newTestClient() *http.Client {
+	client := NewHTTPClient()
+	client.Timeout = 5 * time.Second
+	return client
+}
+
+func TestRedirectToLinkLocalRefused(t *testing.T) {
+	os.Setenv("BLOCK_PRIVATE_IPS", "false")
+	defer os.Unsetenv("BLOCK_PRIVATE_IPS")
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "http://169.254.169.254/latest/meta-data/", http.StatusFound)
+	}))
+	defer server.Close()
+
+	_, err := newTestClient().Get(server.URL)
+	if err == nil {
+		t.Fatal("expected redirect to link-local address to be refused")
+	}
+	if !errors.Is(err, ErrLinkLocal) {
+		t.Errorf("expected ErrLinkLocal, got %v", err)
+	}
+}
+
+func TestDialToLinkLocalRefused(t *testing.T) {
+	os.Unsetenv("BLOCK_PRIVATE_IPS")
+
+	_, err := newTestClient().Get("http://169.254.169.254/latest/meta-data/")
+	if err == nil {
+		t.Fatal("expected link-local address to be refused at dial time")
+	}
+	if !errors.Is(err, ErrLinkLocal) {
+		t.Errorf("expected ErrLinkLocal, got %v", err)
+	}
+}
+
+func TestRedirectLimit(t *testing.T) {
+	os.Setenv("BLOCK_PRIVATE_IPS", "false")
+	defer os.Unsetenv("BLOCK_PRIVATE_IPS")
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/again", http.StatusFound)
+	}))
+	defer server.Close()
+
+	_, err := newTestClient().Get(server.URL)
+	if !errors.Is(err, ErrTooManyRedirects) {
+		t.Errorf("expected ErrTooManyRedirects, got %v", err)
+	}
+}
+
+func TestLinkLocalBlockedRegardlessOfFlag(t *testing.T) {
+	os.Unsetenv("BLOCK_PRIVATE_IPS")
+	defer os.Unsetenv("BLOCK_PRIVATE_IPS")
+
+	for _, rawURL := range []string{
+		"http://169.254.169.254/latest/meta-data/",
+		"http://169.254.170.2/v2/credentials",
+		"http://[fe80::1]/",
+	} {
+		if err := ValidateURL(rawURL); !errors.Is(err, ErrLinkLocal) {
+			t.Errorf("ValidateURL(%s) = %v, want ErrLinkLocal", rawURL, err)
+		}
+	}
+}
+
+func TestPrivateAddressesBlockedByDefault(t *testing.T) {
+	os.Unsetenv("BLOCK_PRIVATE_IPS")
+
+	for _, rawURL := range []string{
+		"http://192.168.1.50/doc.pdf",
+		"http://10.0.0.5/doc.pdf",
+		"http://127.0.0.1:8080/doc.pdf",
+		"http://100.64.0.1/doc.pdf",
+	} {
+		if err := ValidateURL(rawURL); !errors.Is(err, ErrPrivateIP) {
+			t.Errorf("ValidateURL(%s) = %v, want ErrPrivateIP", rawURL, err)
+		}
+	}
+}
+
+func TestPrivateAddressesAllowedWhenOptedOut(t *testing.T) {
+	os.Setenv("BLOCK_PRIVATE_IPS", "false")
+	defer os.Unsetenv("BLOCK_PRIVATE_IPS")
+
+	for _, rawURL := range []string{
+		"http://192.168.1.50/doc.pdf",
+		"http://10.0.0.5/doc.pdf",
+		"http://127.0.0.1:8080/doc.pdf",
+		"http://100.64.0.1/doc.pdf",
+	} {
+		if err := ValidateURL(rawURL); err != nil {
+			t.Errorf("ValidateURL(%s) = %v, want nil", rawURL, err)
+		}
+	}
+}