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) + }) +}