Answer 502 for an upstream host with no address or a failed fetch (closes #367)

The image routes answered 500 and logged at error level when the upstream
host had no address, when the fetch failed before the upstream answered,
or when the fetcher refused a redirect. Such a failure is now
ErrInvalidHost or ErrUpstreamRequestFailed, which respondImageError
answers 502 "upstream error" and logs at warn level. A TLS handshake, an
address lookup or a connection that times out is now ErrUpstreamTimeout
and answers 504; its log line keeps the cause. A cancelled fetch or
address lookup is neither ErrUpstreamRequestFailed nor ErrInvalidHost.
README Routes lists these answers.

Model: opus-5-5

internal/handlers/image.go

@@ -254,7 +254,9 @@ func (s *Handlers) respondImageError(
 	switch {
 	case errors.Is(err, httpfetcher.ErrSSRFBlocked):
 		status, message = http.StatusForbidden, "forbidden"
-	case errors.Is(err, httpfetcher.ErrUpstreamError):
+	case errors.Is(err, httpfetcher.ErrUpstreamError),
+		errors.Is(err, httpfetcher.ErrInvalidHost),
+		errors.Is(err, httpfetcher.ErrUpstreamRequestFailed):
 		status, message = http.StatusBadGateway, "upstream error"
 	case errors.Is(err, httpfetcher.ErrUpstreamTimeout):
 		status, message = http.StatusGatewayTimeout, "upstream timeout"

internal/handlers/image_errors_internal_test.go

@@ -12,6 +12,7 @@ import (
 	"net/http/httptest"
 	"slices"
 	"strings"
+	"syscall"
 	"testing"
 	"testing/fstest"
 	"time"
@@ -461,6 +462,13 @@ func TestImageRoutes_SameErrorAnswers(t *testing.T) {
 		{"upstream error status",
 			fmt.Errorf("%w: status 404", httpfetcher.ErrUpstreamError),
 			http.StatusBadGateway, slog.LevelWarn},
+		{"upstream host with no address",
+			fmt.Errorf("%w: cdn.example.com", httpfetcher.ErrInvalidHost),
+			http.StatusBadGateway, slog.LevelWarn},
+		{"connection refused",
+			fmt.Errorf("%w: %w", httpfetcher.ErrUpstreamRequestFailed,
+				syscall.ECONNREFUSED),
+			http.StatusBadGateway, slog.LevelWarn},
 		{"no response headers within the fetch timeout",
 			httpfetcher.ErrUpstreamTimeout, http.StatusGatewayTimeout,
 			slog.LevelWarn},

internal/httpfetcher/httpfetcher.go

@@ -69,7 +69,11 @@ var (
 	ErrInvalidContentType = errors.New("invalid or unsupported content type")
 	ErrUpstreamError      = errors.New("upstream server error")
 	ErrUpstreamTimeout    = errors.New("upstream request timeout")
-	ErrTooManyConnections = errors.New("too many concurrent upstream connections")
+	// ErrUpstreamRequestFailed is a fetch that failed before the upstream
+	// answered, other than by timeout or cancellation: a failed connection
+	// or TLS handshake, or a redirect the fetcher refuses.
+	ErrUpstreamRequestFailed = errors.New("upstream request failed")
+	ErrTooManyConnections    = errors.New("too many concurrent upstream connections")
 )
 
 // Internal fetcher errors.
@@ -283,11 +287,7 @@ func (f *HTTPFetcher) Fetch(
 	fetchDuration := time.Since(startTime)
 
 	if err != nil {
-		if errors.Is(err, context.DeadlineExceeded) {
-			return nil, ErrUpstreamTimeout
-		}
-
-		return nil, fmt.Errorf("upstream request failed: %w", err)
+		return nil, upstreamRequestError(err)
 	}
 
 	// The deferred release gives the connection back, as no body follows
@@ -316,6 +316,33 @@ func (f *HTTPFetcher) Fetch(
 	return result, nil
 }
 
+// upstreamRequestError returns the error Fetch reports when its upstream
+// request fails with err:
+//   - err wrapped in ErrUpstreamTimeout for a timeout, so the log names what
+//     timed out.
+//   - err, still matching context.Canceled, when the caller cancelled the
+//     fetch.
+//   - err wrapped in ErrUpstreamRequestFailed otherwise.
+func upstreamRequestError(err error) error {
+	var netErr net.Error
+
+	switch {
+	case errors.Is(err, context.DeadlineExceeded),
+		errors.Is(err, ErrUpstreamTimeout),
+		errors.As(err, &netErr) && netErr.Timeout():
+		// A timeout other than the fetch timeout, such as the TLS
+		// handshake timeout, does not match context.DeadlineExceeded. An
+		// address lookup or a connection that timed out is
+		// ErrUpstreamTimeout already, so its log line names that error twice.
+		return fmt.Errorf("%w: %w", ErrUpstreamTimeout, err)
+	case errors.Is(err, context.Canceled):
+		// The caller cancelled the fetch; the upstream did not fail
+		return fmt.Errorf("upstream request cancelled: %w", err)
+	default:
+		return fmt.Errorf("%w: %w", ErrUpstreamRequestFailed, err)
+	}
+}
+
 // requestHeaders returns the headers of a fetch: User-Agent, Accept, the
 // ID of the request the fetch serves, so the fetch can be found in the
 // upstream host's logs, and a non-empty ifNoneMatch or ifModifiedSince.
@@ -505,7 +532,7 @@ func validateURL(ctx context.Context, fetchURL *neturl.URL, allowHTTP bool) erro
 	// Resolve the host to check IP addresses
 	addrs, err := net.DefaultResolver.LookupIPAddr(ctx, host)
 	if err != nil {
-		return fmt.Errorf("%w: %s", ErrInvalidHost, host)
+		return addressLookupError(host, err)
 	}
 
 	private := slices.ContainsFunc(addrs, func(addr net.IPAddr) bool {
@@ -518,6 +545,23 @@ func validateURL(ctx context.Context, fetchURL *neturl.URL, allowHTTP bool) erro
 	return nil
 }
 
+// addressLookupError returns the error for a failed address lookup of host:
+//   - ErrUpstreamTimeout when the lookup timed out.
+//   - err, still matching context.Canceled, when the lookup was cancelled.
+//   - ErrInvalidHost otherwise.
+func addressLookupError(host string, err error) error {
+	var netErr net.Error
+	if errors.As(err, &netErr) && netErr.Timeout() {
+		return fmt.Errorf("%w: address lookup of %s", ErrUpstreamTimeout, host)
+	}
+
+	if errors.Is(err, context.Canceled) {
+		return fmt.Errorf("address lookup cancelled: %w", err)
+	}
+
+	return fmt.Errorf("%w: %s", ErrInvalidHost, host)
+}
+
 // isLocalhost checks if the host is localhost.
 func isLocalhost(host string) bool {
 	host = strings.ToLower(host)
@@ -646,7 +690,7 @@ func dialSSRFSafe(
 	// Resolve the address
 	ips, err := net.DefaultResolver.LookupIP(ctx, "ip", host)
 	if err != nil {
-		return nil, fmt.Errorf("%w: %s", ErrInvalidHost, host)
+		return nil, addressLookupError(host, err)
 	}
 
 	// Check all resolved IPs
@@ -657,7 +701,10 @@ func dialSSRFSafe(
 	}
 
 	// Connect using the first valid IP
-	var dialer net.Dialer
+	var (
+		dialer      net.Dialer
+		lastDialErr error
+	)
 
 	for _, ip := range ips {
 		addr := net.JoinHostPort(ip.String(), port)
@@ -666,9 +713,18 @@ func dialSSRFSafe(
 		if err == nil {
 			return conn, nil
 		}
+
+		lastDialErr = err
 	}
 
-	return nil, fmt.Errorf("%w to %s", errConnectFailed, host)
+	// Only the last address tried decides whether the connection timed out
+	var netErr net.Error
+	if errors.As(lastDialErr, &netErr) && netErr.Timeout() {
+		return nil, fmt.Errorf("%w: connect to %s: %w",
+			ErrUpstreamTimeout, host, lastDialErr)
+	}
+
+	return nil, fmt.Errorf("%w to %s: %w", errConnectFailed, host, lastDialErr)
 }
 
 // limitedReader wraps a reader and refuses a body longer than remaining

internal/httpfetcher/request_failed_internal_test.go

@@ -0,0 +1,213 @@
+package httpfetcher
+
+import (
+	"context"
+	"errors"
+	"net"
+	"os"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// TestFetchFailedBeforeAnswer checks the error of a fetch whose connection
+// fails:
+//   - refused: ErrUpstreamRequestFailed.
+//   - timed out: ErrUpstreamTimeout, still matching the dial's error, even for
+//     a timeout error that does not match context.DeadlineExceeded, such as
+//     the TLS handshake timeout.
+//   - cancelled by its caller: context.Canceled, not ErrUpstreamRequestFailed.
+func TestFetchFailedBeforeAnswer(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		dial       func(cancel context.CancelFunc) error
+		wantErrs   []error
+		wantNotErr error
+	}{
+		{
+			name: "connection refused",
+			dial: func(context.CancelFunc) error {
+				return syscall.ECONNREFUSED
+			},
+			wantErrs:   []error{ErrUpstreamRequestFailed},
+			wantNotErr: context.Canceled,
+		},
+		{
+			name: "timed out while connecting",
+			dial: func(context.CancelFunc) error {
+				return os.ErrDeadlineExceeded
+			},
+			wantErrs:   []error{ErrUpstreamTimeout, os.ErrDeadlineExceeded},
+			wantNotErr: ErrUpstreamRequestFailed,
+		},
+		{
+			name: "cancelled while connecting",
+			dial: func(cancel context.CancelFunc) error {
+				cancel()
+
+				return context.Canceled
+			},
+			wantErrs:   []error{context.Canceled},
+			wantNotErr: ErrUpstreamRequestFailed,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithCancel(testContext(t))
+			defer cancel()
+
+			f := New(&Config{
+				Timeout:               30 * time.Second,
+				MaxResponseSize:       50 << 20,
+				AllowHTTP:             true,
+				MaxConnectionsPerHost: 20,
+				MaxConnections:        64,
+				DialContext: func(context.Context, string, string) (net.Conn, error) {
+					return nil, tt.dial(cancel)
+				},
+			})
+
+			_, err := f.Fetch(ctx, upstreamSource("/image"), "", "")
+			t.Logf("Fetch() error: %v", err)
+
+			for _, wantErr := range tt.wantErrs {
+				if !errors.Is(err, wantErr) {
+					t.Errorf("Fetch() error = %v, want %v", err, wantErr)
+				}
+			}
+
+			if errors.Is(err, tt.wantNotErr) {
+				t.Errorf("Fetch() error = %v, must not be %v", err, tt.wantNotErr)
+			}
+		})
+	}
+}
+
+// TestFetchAddressLookupTimedOut checks that a fetch whose address lookup
+// times out fails with ErrUpstreamTimeout, both for the lookup of the source
+// host and for the lookup while connecting. The error must match neither
+// ErrInvalidHost nor ErrUpstreamRequestFailed, which the image routes answer
+// 502. A context whose deadline has passed makes the lookup time out.
+func TestFetchAddressLookupTimedOut(t *testing.T) {
+	t.Parallel()
+
+	checkTimedOut := func(t *testing.T, err error) {
+		t.Helper()
+		t.Logf("Fetch() error: %v", err)
+
+		if !errors.Is(err, ErrUpstreamTimeout) {
+			t.Errorf("Fetch() error = %v, want ErrUpstreamTimeout", err)
+		}
+
+		if errors.Is(err, ErrInvalidHost) || errors.Is(err, ErrUpstreamRequestFailed) {
+			t.Errorf("Fetch() error = %v, must match neither ErrInvalidHost "+
+				"nor ErrUpstreamRequestFailed", err)
+		}
+	}
+
+	t.Run("lookup of the source host", func(t *testing.T) {
+		t.Parallel()
+
+		ctx, cancel := context.WithTimeout(testContext(t), 0)
+		defer cancel()
+
+		f := New(&Config{
+			Timeout:               30 * time.Second,
+			MaxResponseSize:       50 << 20,
+			MaxConnectionsPerHost: 20,
+			MaxConnections:        64,
+		})
+
+		_, err := f.Fetch(ctx, imageSourceOn("cdn.example.com"), "", "")
+		checkTimedOut(t, err)
+	})
+
+	t.Run("lookup while connecting", func(t *testing.T) {
+		t.Parallel()
+
+		// The source host is an address, which validateURL does not look up;
+		// the dialer looks up a name
+		f := New(&Config{
+			Timeout:               30 * time.Second,
+			MaxResponseSize:       50 << 20,
+			MaxConnectionsPerHost: 20,
+			MaxConnections:        64,
+			DialContext: func(ctx context.Context, network, _ string) (net.Conn, error) {
+				ctx, cancel := context.WithTimeout(ctx, 0)
+				defer cancel()
+
+				return dialSSRFSafe(ctx, network, "img.example.com:443", nil)
+			},
+		})
+
+		_, err := f.Fetch(testContext(t), upstreamSource("/image"), "", "")
+		checkTimedOut(t, err)
+	})
+}
+
+// TestFetchConnectionTimedOut checks that a fetch whose connection in
+// dialSSRFSafe times out fails with ErrUpstreamTimeout, which the image
+// routes answer 504. The error must match neither ErrUpstreamRequestFailed
+// nor ErrInvalidHost, which they answer 502. A context whose deadline has
+// passed makes the connection time out without touching the network.
+func TestFetchConnectionTimedOut(t *testing.T) {
+	t.Parallel()
+
+	f := New(&Config{
+		Timeout:               30 * time.Second,
+		MaxResponseSize:       50 << 20,
+		MaxConnectionsPerHost: 20,
+		MaxConnections:        64,
+		DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
+			ctx, cancel := context.WithTimeout(ctx, 0)
+			defer cancel()
+
+			return dialSSRFSafe(ctx, network, addr, nil)
+		},
+	})
+
+	_, err := f.Fetch(testContext(t), upstreamSource("/image"), "", "")
+	t.Logf("Fetch() error: %v", err)
+
+	if !errors.Is(err, ErrUpstreamTimeout) {
+		t.Errorf("Fetch() error = %v, want ErrUpstreamTimeout", err)
+	}
+
+	if errors.Is(err, ErrUpstreamRequestFailed) || errors.Is(err, ErrInvalidHost) {
+		t.Errorf("Fetch() error = %v, must match neither "+
+			"ErrUpstreamRequestFailed nor ErrInvalidHost", err)
+	}
+}
+
+// TestFetchAddressLookupCancelled checks that a fetch whose address lookup
+// its caller cancelled fails with an error matching context.Canceled, not
+// ErrInvalidHost, which the image routes answer 502.
+func TestFetchAddressLookupCancelled(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithCancel(testContext(t))
+	cancel()
+
+	f := New(&Config{
+		Timeout:               30 * time.Second,
+		MaxResponseSize:       50 << 20,
+		MaxConnectionsPerHost: 20,
+		MaxConnections:        64,
+	})
+
+	_, err := f.Fetch(ctx, imageSourceOn("cdn.example.com"), "", "")
+	t.Logf("Fetch() error: %v", err)
+
+	if !errors.Is(err, context.Canceled) {
+		t.Errorf("Fetch() error = %v, want context.Canceled", err)
+	}
+
+	if errors.Is(err, ErrInvalidHost) {
+		t.Errorf("Fetch() error = %v, must not be ErrInvalidHost", err)
+	}
+}