diff --git a/docs/cli-reference.md b/docs/cli-reference.md index bc61a744..b691bd22 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -46,6 +46,11 @@ fetch -X DELETE example.com/resource/123 Set custom headers. Can be used multiple times. +Request framing is validated before connecting. Repeated `Content-Length` +values must agree and are normalized to one value; `Content-Length` cannot be +combined with `Transfer-Encoding`, and `chunked` is the only supported request +transfer encoding. + ```sh fetch -H "Authorization: Bearer token" example.com fetch -H "X-Custom: value" -H "Accept: application/json" example.com @@ -469,7 +474,7 @@ allowed. Maximum number of retries for transient failures. Default: `0` (no retries). The value must be between `0` and `100`. -Retries occur on connection errors and retryable status codes (429, 502, 503, 504) for GET, HEAD, OPTIONS, and TRACE requests. Non-retryable errors (4xx, TLS certificate errors) are not retried. PUT and DELETE are not retried by default: although HTTP describes them as idempotent, individual APIs may implement side effects that are unsafe to repeat. POST, PATCH, PUT, DELETE, and custom methods require the explicit `--retry-unsafe` opt-in. Uses exponential backoff with jitter between attempts. +Retries occur on transient connection errors and retryable status codes (408, 425, 429, 500, 502, 503, 504) for GET, HEAD, OPTIONS, and TRACE requests. DNS name-not-found failures, other non-retryable 4xx responses, and TLS certificate errors are not retried. PUT and DELETE are not retried by default: although HTTP describes them as idempotent, individual APIs may implement side effects that are unsafe to repeat. POST, PATCH, PUT, DELETE, and custom methods require the explicit `--retry-unsafe` opt-in. Uses exponential backoff with jitter between attempts. All attempts, redirects, response reads, bounded drains, and retry delays share one `--timeout` wall-clock budget. A request body must be replayable before a retry starts. Only the final attempt's response body is written to stdout. Retry notifications are printed to stderr (suppressed with `--silent`). diff --git a/docs/configuration.md b/docs/configuration.md index 20b9eea7..cfa3b19a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -381,7 +381,7 @@ redirects remain allowed. **Type**: Integer **Default**: `0` (no retries) -Maximum number of retries for transient failures. Retries occur on connection errors and retryable status codes (429, 502, 503, 504) only for GET, HEAD, OPTIONS, and TRACE by default. PUT, DELETE, POST, PATCH, and custom methods require `retry-unsafe = true`. +Maximum number of retries for transient failures. Retries occur on transient connection errors and retryable status codes (408, 425, 429, 500, 502, 503, 504) only for GET, HEAD, OPTIONS, and TRACE by default. DNS name-not-found failures are not retried. PUT, DELETE, POST, PATCH, and custom methods require `retry-unsafe = true`. The value must be between `0` and `100`. ```ini diff --git a/internal/client/client.go b/internal/client/client.go index 41e4ca00..df254560 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -1088,6 +1088,7 @@ func (c *Client) NewRequest(ctx context.Context, cfg RequestConfig) (*http.Reque if source != nil { body.Attach(req, source) } + inferredContentLength := req.ContentLength if urlBasic != nil { req.SetBasicAuth(urlBasic.Key, urlBasic.Val) MarkCredentialHeaders(req, "Authorization") @@ -1121,6 +1122,9 @@ func (c *Client) NewRequest(ctx context.Context, cfg RequestConfig) (*http.Reque // explicit value, then append so repeated headers remain distinct. seenHeaders := make(map[string]struct{}, len(cfg.Headers)) acceptEncodingSet := false + var contentLengths []int64 + var transferEncodings []string + transferEncodingSet := false for _, kv := range cfg.Headers { if strings.EqualFold(kv.Key, "Host") { req.Host = kv.Val @@ -1147,20 +1151,62 @@ func (c *Client) NewRequest(ctx context.Context, cfg RequestConfig) (*http.Reque if err != nil || length < 0 { return nil, fmt.Errorf("invalid Content-Length header %q", kv.Val) } - req.ContentLength = length + contentLengths = append(contentLengths, length) case "transfer-encoding": - var encodings []string + transferEncodingSet = true for encoding := range strings.SplitSeq(kv.Val, ",") { - encoding = strings.TrimSpace(encoding) + encoding = strings.ToLower(strings.TrimSpace(encoding)) if encoding != "" { - encodings = append(encodings, encoding) + transferEncodings = append(transferEncodings, encoding) } } - req.TransferEncoding = encodings - if len(encodings) > 0 { - req.ContentLength = -1 + } + } + + // Content-Length and Transfer-Encoding are framing metadata represented by + // dedicated Request fields. Validate them here so malformed or ambiguous + // framing fails before any connection is opened. Equal duplicate lengths + // are safe but are normalized to one value; differing values are rejected. + if len(contentLengths) > 0 { + length := contentLengths[0] + for _, other := range contentLengths[1:] { + if other != length { + return nil, fmt.Errorf("conflicting Content-Length headers: %d and %d", length, other) } } + if length == 0 && req.Body != nil && req.Body != http.NoBody { + return nil, errors.New("Content-Length 0 cannot be used with a request body") + } + if length == 0 && (req.Method == http.MethodGet || req.Method == http.MethodHead) { + return nil, fmt.Errorf("Content-Length 0 is not transmitted for %s requests without a body", req.Method) + } + if length > 0 && (req.Body == nil || req.Body == http.NoBody) { + return nil, errors.New("positive Content-Length cannot be used without a request body") + } + if inferredContentLength >= 0 && req.Body != nil && req.Body != http.NoBody && length != inferredContentLength { + return nil, fmt.Errorf("Content-Length %d does not match request body length %d", length, inferredContentLength) + } + req.ContentLength = length + req.Header.Set("Content-Length", strconv.FormatInt(length, 10)) + } + if transferEncodingSet && len(transferEncodings) == 0 { + return nil, errors.New("Transfer-Encoding must specify chunked") + } + if len(transferEncodings) > 0 { + // Go's HTTP transports support chunked request framing only. Reject other + // codings early instead of surfacing an opaque error from RoundTrip. + if len(transferEncodings) != 1 || transferEncodings[0] != "chunked" { + return nil, fmt.Errorf("unsupported Transfer-Encoding %q; only chunked is supported", strings.Join(transferEncodings, ", ")) + } + if len(contentLengths) > 0 { + return nil, errors.New("Content-Length and Transfer-Encoding cannot be used together") + } + if req.Body == nil || req.Body == http.NoBody { + return nil, errors.New("Transfer-Encoding chunked requires a request body") + } + req.TransferEncoding = transferEncodings + req.ContentLength = -1 + req.Header.Set("Transfer-Encoding", "chunked") } // Set the compression policy after explicit headers have been applied. An diff --git a/internal/client/client_test.go b/internal/client/client_test.go index faa94911..ff93610a 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -234,6 +234,222 @@ func TestNewRequestDoesNotAccumulateURLDefaults(t *testing.T) { } } +func TestNewRequestValidatesFramingHeaders(t *testing.T) { + tests := []struct { + name string + headers []core.KeyVal[string] + wantErr string + }{ + { + name: "conflicting content lengths", + headers: []core.KeyVal[string]{ + {Key: "Content-Length", Val: "3"}, + {Key: "content-length", Val: "4"}, + }, + wantErr: "conflicting Content-Length headers", + }, + { + name: "content length and transfer encoding", + headers: []core.KeyVal[string]{ + {Key: "Content-Length", Val: "3"}, + {Key: "Transfer-Encoding", Val: "chunked"}, + }, + wantErr: "cannot be used together", + }, + { + name: "unsupported transfer encoding", + headers: []core.KeyVal[string]{{Key: "Transfer-Encoding", Val: "gzip, chunked"}}, + wantErr: "only chunked is supported", + }, + { + name: "zero content length with body", + headers: []core.KeyVal[string]{{Key: "Content-Length", Val: "0"}}, + wantErr: "cannot be used with a request body", + }, + { + name: "content length smaller than body", + headers: []core.KeyVal[string]{{Key: "Content-Length", Val: "2"}}, + wantErr: "does not match request body length 3", + }, + { + name: "content length larger than body", + headers: []core.KeyVal[string]{{Key: "Content-Length", Val: "4"}}, + wantErr: "does not match request body length 3", + }, + { + name: "empty transfer encoding", + headers: []core.KeyVal[string]{{Key: "Transfer-Encoding", Val: " , "}}, + wantErr: "must specify chunked", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewClient(ClientConfig{}).NewRequest(context.Background(), RequestConfig{ + Data: strings.NewReader("abc"), + Headers: tt.headers, + URL: mustURL(t, "https://example.com"), + }) + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error = %v, want substring %q", err, tt.wantErr) + } + }) + } +} + +func TestNewRequestRejectsPositiveContentLengthWithoutBody(t *testing.T) { + _, err := NewClient(ClientConfig{}).NewRequest(context.Background(), RequestConfig{ + Headers: []core.KeyVal[string]{{Key: "Content-Length", Val: "1"}}, + URL: mustURL(t, "https://example.com"), + }) + if err == nil || !strings.Contains(err.Error(), "without a request body") { + t.Fatalf("error = %v, want bodyless Content-Length error", err) + } +} + +func TestNewRequestAllowsZeroContentLengthForBodylessPost(t *testing.T) { + req, err := NewClient(ClientConfig{}).NewRequest(context.Background(), RequestConfig{ + Headers: []core.KeyVal[string]{{Key: "Content-Length", Val: "0"}}, + Method: http.MethodPost, + URL: mustURL(t, "https://example.com"), + }) + if err != nil { + t.Fatal(err) + } + if req.Body != nil && req.Body != http.NoBody { + t.Fatalf("body = %T, want no body", req.Body) + } + if req.ContentLength != 0 || req.Header.Get("Content-Length") != "0" { + t.Fatalf("content length = %d, header = %q", req.ContentLength, req.Header.Get("Content-Length")) + } +} + +func TestNewRequestRejectsFramingThatTransportWouldStrip(t *testing.T) { + tests := []struct { + name string + method string + header core.KeyVal[string] + want string + }{ + {name: "zero length GET", method: http.MethodGet, header: core.KeyVal[string]{Key: "Content-Length", Val: "0"}, want: "is not transmitted"}, + {name: "zero length HEAD", method: http.MethodHead, header: core.KeyVal[string]{Key: "Content-Length", Val: "0"}, want: "is not transmitted"}, + {name: "bodyless chunked", method: http.MethodPost, header: core.KeyVal[string]{Key: "Transfer-Encoding", Val: "chunked"}, want: "requires a request body"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := NewClient(ClientConfig{}).NewRequest(context.Background(), RequestConfig{ + Headers: []core.KeyVal[string]{tt.header}, + Method: tt.method, + URL: mustURL(t, "https://example.com"), + }) + if err == nil || !strings.Contains(err.Error(), tt.want) { + t.Fatalf("error = %v, want substring %q", err, tt.want) + } + }) + } +} + +func TestExplicitRequestFramingReachesWire(t *testing.T) { + type receivedRequest struct { + contentLength int64 + transferEncoding []string + body string + } + received := make(chan receivedRequest, 2) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + data, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("reading request body: %v", err) + } + received <- receivedRequest{ + contentLength: r.ContentLength, + transferEncoding: slices.Clone(r.TransferEncoding), + body: string(data), + } + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + + c := NewClient(ClientConfig{}) + defer c.Close() + tests := []struct { + name string + cfg RequestConfig + wantCL int64 + wantTE []string + wantBody string + }{ + { + name: "zero-length POST", + cfg: RequestConfig{ + Headers: []core.KeyVal[string]{{Key: "Content-Length", Val: "0"}}, + Method: http.MethodPost, + }, + wantCL: 0, + }, + { + name: "chunked body", + cfg: RequestConfig{ + Data: strings.NewReader("abc"), + Headers: []core.KeyVal[string]{{Key: "Transfer-Encoding", Val: "chunked"}}, + Method: http.MethodPost, + }, + wantCL: -1, + wantTE: []string{"chunked"}, + wantBody: "abc", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.cfg.URL = mustURL(t, server.URL) + req, err := c.NewRequest(context.Background(), tt.cfg) + if err != nil { + t.Fatal(err) + } + resp, err := c.Do(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + got := <-received + if got.contentLength != tt.wantCL || !slices.Equal(got.transferEncoding, tt.wantTE) || got.body != tt.wantBody { + t.Fatalf("wire framing = length %d, encodings %q, body %q", got.contentLength, got.transferEncoding, got.body) + } + }) + } +} + +func TestNewRequestNormalizesSafeFramingHeaders(t *testing.T) { + req, err := NewClient(ClientConfig{}).NewRequest(context.Background(), RequestConfig{ + Data: strings.NewReader("abc"), + Headers: []core.KeyVal[string]{ + {Key: "Content-Length", Val: "3"}, + {Key: "content-length", Val: "3"}, + }, + URL: mustURL(t, "https://example.com"), + }) + if err != nil { + t.Fatal(err) + } + defer req.Body.Close() + if req.ContentLength != 3 || !slices.Equal(req.Header.Values("Content-Length"), []string{"3"}) { + t.Fatalf("content length = %d, headers = %q", req.ContentLength, req.Header.Values("Content-Length")) + } + + chunked, err := NewClient(ClientConfig{}).NewRequest(context.Background(), RequestConfig{ + Data: strings.NewReader("abc"), + Headers: []core.KeyVal[string]{{Key: "Transfer-Encoding", Val: "CHUNKED"}}, + URL: mustURL(t, "https://example.com"), + }) + if err != nil { + t.Fatal(err) + } + defer chunked.Body.Close() + if chunked.ContentLength != -1 || !slices.Equal(chunked.TransferEncoding, []string{"chunked"}) { + t.Fatalf("chunked framing = length %d, encodings %q", chunked.ContentLength, chunked.TransferEncoding) + } +} + func TestNewRequestRejectsNilURL(t *testing.T) { _, err := NewClient(ClientConfig{}).NewRequest(context.Background(), RequestConfig{}) if err == nil || err.Error() != "request URL is required" { diff --git a/internal/fetch/fetch.go b/internal/fetch/fetch.go index 51588661..6334a929 100644 --- a/internal/fetch/fetch.go +++ b/internal/fetch/fetch.go @@ -712,7 +712,11 @@ func formatResponse(ctx context.Context, r *Request, resp *http.Response, cc *cl } if output != "" && r.Output != "-" { - size := client.WireContentLength(resp) + // Progress tracks the bytes written to disk. Once a response has been + // decoded, its compressed wire length is not the output length and can + // make the progress bar exceed 100%. Decoders set ContentLength to -1, + // which correctly selects the unknown-size spinner in that case. + size := responseOutputSize(resp) p := r.PrinterHandle.Stderr() return nil, writeOutputToFile(output, resp.Body, size, p, r.Verbosity, r.Clobber) } @@ -818,6 +822,13 @@ func formatResponse(ctx context.Context, r *Request, resp *http.Response, cc *cl return newUntrustedResponseReader(bytes.NewReader(buf)), nil } +func responseOutputSize(resp *http.Response) int64 { + if resp == nil { + return -1 + } + return resp.ContentLength +} + func rejectHAROutputPath(r *Request, output string) error { if r == nil || r.harRecorder == nil || output == "" || output == "-" { return nil diff --git a/internal/fetch/format_response_test.go b/internal/fetch/format_response_test.go index 72e61631..1589fa19 100644 --- a/internal/fetch/format_response_test.go +++ b/internal/fetch/format_response_test.go @@ -26,6 +26,20 @@ func TestFormatWithBoundedOutput(t *testing.T) { } } +func TestResponseOutputSizeUsesDecodedLength(t *testing.T) { + resp := &http.Response{ContentLength: -1} + if got := responseOutputSize(resp); got != -1 { + t.Fatalf("responseOutputSize = %d, want unknown decoded length", got) + } + resp.ContentLength = 123 + if got := responseOutputSize(resp); got != 123 { + t.Fatalf("responseOutputSize = %d, want 123", got) + } + if got := responseOutputSize(nil); got != -1 { + t.Fatalf("responseOutputSize(nil) = %d, want -1", got) + } +} + func TestFormatResponseFormatsExactMaxBodyBytes(t *testing.T) { prefix := []byte(`{"a":1}`) body := append(append([]byte(nil), prefix...), bytes.Repeat([]byte(" "), maxBodyBytes-len(prefix))...) diff --git a/internal/fetch/retry.go b/internal/fetch/retry.go index e002eaf7..e8e29e09 100644 --- a/internal/fetch/retry.go +++ b/internal/fetch/retry.go @@ -603,10 +603,13 @@ func shouldRetry(method string, retryUnsafe bool, resp *http.Response, err error return false, 0 } switch resp.StatusCode { - case http.StatusTooManyRequests, // 429 - http.StatusBadGateway, // 502 - http.StatusServiceUnavailable, // 503 - http.StatusGatewayTimeout: // 504 + case http.StatusRequestTimeout, // 408 + http.StatusTooEarly, // 425 + http.StatusTooManyRequests, // 429 + http.StatusInternalServerError, // 500 + http.StatusBadGateway, // 502 + http.StatusServiceUnavailable, // 503 + http.StatusGatewayTimeout: // 504 return true, parseRetryAfter(resp.Header) default: return false, 0 @@ -651,6 +654,15 @@ func isRetryableError(err error) bool { return isRetryableError(urlErr.Err) } + // A positively identified name-not-found failure cannot improve on a + // subsequent attempt. Leave unclassified DNS errors retryable: platform + // resolvers do not consistently set the timeout/temporary flags. + if dnsErr, ok := errors.AsType[*net.DNSError](err); ok { + if dnsErr.IsNotFound { + return false + } + } + // Retry on per-attempt timeout (ErrRequestTimedOut is the custom // cause set via context.WithTimeoutCause for --timeout). if _, ok := errors.AsType[core.ErrRequestTimedOut](err); ok { diff --git a/internal/fetch/retry_test.go b/internal/fetch/retry_test.go index 2f89dd3a..9cd907c8 100644 --- a/internal/fetch/retry_test.go +++ b/internal/fetch/retry_test.go @@ -208,6 +208,19 @@ func TestSchemelessPlaintextHint(t *testing.T) { } func TestShouldRetry(t *testing.T) { + for _, status := range []int{ + http.StatusRequestTimeout, + http.StatusTooEarly, + http.StatusInternalServerError, + } { + t.Run(fmt.Sprintf("%d is retryable", status), func(t *testing.T) { + ok, _ := shouldRetry(http.MethodGet, false, &http.Response{StatusCode: status}, nil) + if !ok { + t.Fatalf("expected %d to be retryable", status) + } + }) + } + t.Run("429 is retryable", func(t *testing.T) { resp := &http.Response{StatusCode: 429, Header: http.Header{}} ok, _ := shouldRetry(http.MethodGet, false, resp, nil) @@ -273,7 +286,7 @@ func TestShouldRetry(t *testing.T) { }) t.Run("connection error is retryable", func(t *testing.T) { - err := &net.OpError{Op: "dial", Err: &net.DNSError{Err: "no such host"}} + err := &net.OpError{Op: "dial", Err: &net.DNSError{Err: "temporary resolver failure", IsTemporary: true}} ok, _ := shouldRetry(http.MethodGet, false, nil, err) if !ok { t.Error("expected connection error to be retryable") @@ -288,7 +301,7 @@ func TestShouldRetry(t *testing.T) { }) t.Run("url error wrapping net error is retryable", func(t *testing.T) { - err := &url.Error{Op: "Get", URL: "http://example.com", Err: &net.OpError{Op: "dial", Err: &net.DNSError{Err: "no such host"}}} + err := &url.Error{Op: "Get", URL: "http://example.com", Err: &net.OpError{Op: "dial", Err: &net.DNSError{Err: "resolver timeout", IsTimeout: true}}} ok, _ := shouldRetry(http.MethodGet, false, nil, err) if !ok { t.Error("expected url.Error wrapping net error to be retryable") @@ -332,6 +345,27 @@ func TestShouldRetry(t *testing.T) { } func TestIsRetryableError(t *testing.T) { + t.Run("name not found is not retryable", func(t *testing.T) { + err := &url.Error{Op: "Get", URL: "https://missing.invalid", Err: &net.DNSError{Err: "no such host", Name: "missing.invalid", IsNotFound: true}} + if isRetryableError(err) { + t.Error("expected permanent DNS name-not-found error to not be retryable") + } + }) + + t.Run("temporary DNS failure is retryable", func(t *testing.T) { + err := &net.DNSError{Err: "server misbehaving", IsTemporary: true} + if !isRetryableError(err) { + t.Error("expected temporary DNS error to be retryable") + } + }) + + t.Run("unclassified DNS failure is retryable", func(t *testing.T) { + err := &net.DNSError{Err: "resolver failure"} + if !isRetryableError(err) { + t.Error("expected unclassified DNS error to remain retryable") + } + }) + t.Run("TLS cert error wrapped in url.Error is not retryable", func(t *testing.T) { err := &url.Error{ Op: "Get",