From de546fa2e92802c11caafddb5544defdd62478b8 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 11 Nov 2026 21:13:39 +0000 Subject: [PATCH] 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 --- README.md | 11 + internal/handlers/image.go | 4 +- .../handlers/image_errors_internal_test.go | 8 + internal/httpfetcher/httpfetcher.go | 76 ++++++- .../request_failed_internal_test.go | 213 ++++++++++++++++++ 5 files changed, 301 insertions(+), 11 deletions(-) create mode 100644 internal/httpfetcher/request_failed_internal_test.go diff --git a/README.md b/README.md index 786deb9..7839810 100644 --- a/README.md +++ b/README.md @@ -337,8 +337,19 @@ to any method the list does not give for it. neither the image nor its source in its cache. - 502 likewise for 5 minutes after a body whose first bytes are not those of its `Content-Type`. + - 502 when the upstream host, or a host it redirects to, has no address. + - 502 when the upstream redirects to a URL that is not `https`, or redirects + 10 times. + - 502 when the fetch fails before the upstream answers, such as on a refused + connection or a failed TLS handshake. - 503 when pixa is busy (see Limits). - 503 in maintenance mode. + - 504 when the address lookup of the upstream host, or of a host it + redirects to, times out. + - 504 when the connection to the upstream host, or to a host it redirects + to, times out. + - 504 when the upstream has not finished the TLS handshake within 10 + seconds. - 504 when the upstream has not sent its response headers within `upstream_fetch_timeout`. - 500 when that time runs out while the image itself is still arriving. diff --git a/internal/handlers/image.go b/internal/handlers/image.go index ff11b6e..108e3e6 100644 --- a/internal/handlers/image.go +++ b/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" diff --git a/internal/handlers/image_errors_internal_test.go b/internal/handlers/image_errors_internal_test.go index c8c2a33..360a6f1 100644 --- a/internal/handlers/image_errors_internal_test.go +++ b/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}, diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 7f8a8f7..4a01128 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/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 diff --git a/internal/httpfetcher/request_failed_internal_test.go b/internal/httpfetcher/request_failed_internal_test.go new file mode 100644 index 0000000..b71dac6 --- /dev/null +++ b/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) + } +} -- 2.54.0