From 6defdaaa9dbe3ecd221f03c0ec685e6f1afa138e 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, such as on a refused connection, or when the fetcher refused a redirect. The fetcher now marks such a failure with ErrUpstreamRequestFailed; a cancelled fetch is not marked. respondImageError answers it, and ErrInvalidHost, 502 "upstream error" and logs them at warn level. The TLS handshake timeout and an address lookup that times out also answered 500; both are now ErrUpstreamTimeout and answer 504. README Routes lists these answers. Model: opus-5-5 --- README.md | 9 ++ internal/handlers/image.go | 4 +- .../handlers/image_errors_internal_test.go | 8 + internal/httpfetcher/httpfetcher.go | 52 +++++- .../request_failed_internal_test.go | 148 ++++++++++++++++++ 5 files changed, 212 insertions(+), 9 deletions(-) create mode 100644 internal/httpfetcher/request_failed_internal_test.go diff --git a/README.md b/README.md index 786deb9..d0e925e 100644 --- a/README.md +++ b/README.md @@ -337,8 +337,17 @@ 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 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..73ea306 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,31 @@ func (f *HTTPFetcher) Fetch( return result, nil } +// upstreamRequestError returns the error Fetch reports when its upstream +// request fails with err: +// - ErrUpstreamTimeout for a timeout. +// - 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 that timed out is ErrUpstreamTimeout already. + return ErrUpstreamTimeout + 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 +530,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 +543,17 @@ 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, 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) + } + + 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 +682,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 diff --git a/internal/httpfetcher/request_failed_internal_test.go b/internal/httpfetcher/request_failed_internal_test.go new file mode 100644 index 0000000..f512993 --- /dev/null +++ b/internal/httpfetcher/request_failed_internal_test.go @@ -0,0 +1,148 @@ +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, 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 + wantErr error + wantNotErr error + }{ + { + name: "connection refused", + dial: func(context.CancelFunc) error { + return syscall.ECONNREFUSED + }, + wantErr: ErrUpstreamRequestFailed, + wantNotErr: context.Canceled, + }, + { + name: "timed out while connecting", + dial: func(context.CancelFunc) error { + return os.ErrDeadlineExceeded + }, + wantErr: ErrUpstreamTimeout, + wantNotErr: ErrUpstreamRequestFailed, + }, + { + name: "cancelled while connecting", + dial: func(cancel context.CancelFunc) error { + cancel() + + return context.Canceled + }, + wantErr: 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) + + if !errors.Is(err, tt.wantErr) { + t.Errorf("Fetch() error = %v, want %v", err, tt.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) + }) +}