From cdf59c4eee58803f79258ce397c3c5cee023c87b Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 07:57:37 -0400 Subject: [PATCH] Stop LLM adapters following redirects and cap Retry-After MQ-01: the Anthropic adapter used a default http.Client, so a 3xx from the configured endpoint was followed and the x-api-key header resent to the redirect target; net/http strips only its own sensitive headers. Both adapters now use a client whose CheckRedirect returns ErrUseLastResponse, so a 3xx comes back as an llm.ProviderError and nothing is requested again. The OpenAI-compatible adapter gets the same change: it forwards configured extra headers (which may carry a credential and which net/http does not treat as sensitive) across hosts, and net/http keeps Authorization on a same-host https-to-http redirect. MQ-02: a delay-seconds Retry-After too large for time.Duration overflowed into a negative wait. retryAfter now caps the value at the most whole seconds a time.Duration holds (also for values beyond int64) and the response still maps to ErrRateLimited. Tests: TestRedirectIsNotFollowed (302 to a second server, which must get no request) and TestRateLimitedHugeRetryAfterIsCapped in both packages, plus huge-number cases in TestRetryAfter. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/anthropic/anthropic.go | 26 ++++++++- internal/ai/llm/anthropic/anthropic_test.go | 60 +++++++++++++++++++++ internal/ai/llm/openai/openai.go | 26 ++++++++- internal/ai/llm/openai/openai_test.go | 60 +++++++++++++++++++++ 4 files changed, 168 insertions(+), 4 deletions(-) diff --git a/internal/ai/llm/anthropic/anthropic.go b/internal/ai/llm/anthropic/anthropic.go index 2891d2f..3625db8 100644 --- a/internal/ai/llm/anthropic/anthropic.go +++ b/internal/ai/llm/anthropic/anthropic.go @@ -14,6 +14,7 @@ import ( "errors" "fmt" "io" + "math" "net" "net/http" "net/url" @@ -94,7 +95,7 @@ func newFromConfig(cfg Config) (*Provider, error) { apiKey: cfg.APIKey, model: cfg.Model, maxTokens: cfg.MaxTokens, - client: &http.Client{}, + client: noRedirectClient(), }, nil } @@ -394,10 +395,15 @@ func retryAfter(value string, now time.Time) time.Duration { if value == "" { return 0 } - if secs, err := strconv.Atoi(value); err == nil { + if secs, err := strconv.ParseInt(value, 10, 64); err == nil || errors.Is(err, strconv.ErrRange) { if secs < 0 { return 0 } + // time.Duration counts nanoseconds in an int64; a larger wait is + // capped to the most whole seconds it holds instead of overflowing. + if secs > maxRetryAfterSeconds { + secs = maxRetryAfterSeconds + } return time.Duration(secs) * time.Second } if at, err := http.ParseTime(value); err == nil && at.After(now) { @@ -406,6 +412,22 @@ func retryAfter(value string, now time.Time) time.Duration { return 0 } +// maxRetryAfterSeconds is the largest delay-seconds value retryAfter +// returns unchanged: the most whole seconds a time.Duration holds. +const maxRetryAfterSeconds = math.MaxInt64 / int64(time.Second) + +// noRedirectClient returns an HTTP client that never follows a redirect. +// A followed redirect would resend the credential headers, which net/http +// strips only for its own list of sensitive headers and only across hosts; +// instead the 3xx response is returned and becomes a provider error. +func noRedirectClient() *http.Client { + return &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }, + } +} + func isTimeout(err error) bool { var ne net.Error return errors.Is(err, context.DeadlineExceeded) || (errors.As(err, &ne) && ne.Timeout()) diff --git a/internal/ai/llm/anthropic/anthropic_test.go b/internal/ai/llm/anthropic/anthropic_test.go index 8137121..ccae5b1 100644 --- a/internal/ai/llm/anthropic/anthropic_test.go +++ b/internal/ai/llm/anthropic/anthropic_test.go @@ -647,6 +647,9 @@ func TestRetryAfter(t *testing.T) { "soon": 0, "Sat, 03 Oct 2026 12:02:00 GMT": 2 * time.Minute, "Sat, 03 Oct 2026 11:59:00 GMT": 0, + "9223372036854775807": time.Duration(maxRetryAfterSeconds) * time.Second, + "99999999999999999999": time.Duration(maxRetryAfterSeconds) * time.Second, + "-99999999999999999999": 0, } for in, want := range cases { if got := retryAfter(in, now); got != want { @@ -910,3 +913,60 @@ func TestRedactedErrorsFormatWithoutKey(t *testing.T) { } } } + +func TestRateLimitedHugeRetryAfterIsCapped(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "9223372036854775807") + w.WriteHeader(http.StatusTooManyRequests) + })) + t.Cleanup(srv.Close) + p, err := New(Config{BaseURL: srv.URL, APIKey: testKey, Model: "test-model", MaxTokens: 1024}) + if err != nil { + t.Fatal(err) + } + err = p.Verify(context.Background()) + var rl *llm.RateLimitedError + if !errors.Is(err, llm.ErrRateLimited) || !errors.As(err, &rl) || rl.RetryAfter != time.Duration(maxRetryAfterSeconds)*time.Second { + t.Fatalf("err = %v, want a RateLimitedError with RetryAfter capped to %v", err, time.Duration(maxRetryAfterSeconds)*time.Second) + } +} + +func TestRedirectIsNotFollowed(t *testing.T) { + var mu sync.Mutex + var secondCalls int + var secondHeader http.Header + second := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + mu.Lock() + secondCalls++ + secondHeader = req.Header.Clone() + mu.Unlock() + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(second.Close) + first := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + http.Redirect(w, req, second.URL+req.URL.Path, http.StatusFound) + })) + t.Cleanup(first.Close) + p, err := New(Config{BaseURL: first.URL, APIKey: testKey, Model: "test-model", MaxTokens: 1024}) + if err != nil { + t.Fatal(err) + } + + err = p.Verify(context.Background()) + var pe *llm.ProviderError + if !errors.Is(err, llm.ErrProvider) || !errors.As(err, &pe) || pe.Status() != http.StatusFound { + t.Fatalf("err = %v, want a ProviderError for HTTP 302", err) + } + mu.Lock() + defer mu.Unlock() + if secondCalls != 0 { + for name, values := range secondHeader { + for _, v := range values { + if strings.Contains(v, testKey) { + t.Errorf("redirect target saw the key in %s", name) + } + } + } + t.Fatalf("redirect target got %d requests, want 0", secondCalls) + } +} diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 33eb4c8..a467e9b 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -15,6 +15,7 @@ import ( "errors" "fmt" "io" + "math" "net" "net/http" "net/textproto" @@ -132,7 +133,7 @@ func newFromConfig(cfg Config) (*Provider, error) { model: cfg.Model, headers: headers, mode: cfg.StructuredOutput, - client: &http.Client{}, + client: noRedirectClient(), }, nil } @@ -462,10 +463,15 @@ func retryAfter(value string, now time.Time) time.Duration { if value == "" { return 0 } - if secs, err := strconv.Atoi(value); err == nil { + if secs, err := strconv.ParseInt(value, 10, 64); err == nil || errors.Is(err, strconv.ErrRange) { if secs < 0 { return 0 } + // time.Duration counts nanoseconds in an int64; a larger wait is + // capped to the most whole seconds it holds instead of overflowing. + if secs > maxRetryAfterSeconds { + secs = maxRetryAfterSeconds + } return time.Duration(secs) * time.Second } if at, err := http.ParseTime(value); err == nil && at.After(now) { @@ -474,6 +480,22 @@ func retryAfter(value string, now time.Time) time.Duration { return 0 } +// maxRetryAfterSeconds is the largest delay-seconds value retryAfter +// returns unchanged: the most whole seconds a time.Duration holds. +const maxRetryAfterSeconds = math.MaxInt64 / int64(time.Second) + +// noRedirectClient returns an HTTP client that never follows a redirect. +// A followed redirect would resend the credential headers, which net/http +// strips only for its own list of sensitive headers and only across hosts; +// instead the 3xx response is returned and becomes a provider error. +func noRedirectClient() *http.Client { + return &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }, + } +} + func isTimeout(err error) bool { var ne net.Error return errors.Is(err, context.DeadlineExceeded) || (errors.As(err, &ne) && ne.Timeout()) diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index bbdd442..23d4c16 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -779,6 +779,9 @@ func TestRetryAfter(t *testing.T) { "soon": 0, "Sat, 03 Oct 2026 12:02:00 GMT": 2 * time.Minute, "Sat, 03 Oct 2026 11:59:00 GMT": 0, + "9223372036854775807": time.Duration(maxRetryAfterSeconds) * time.Second, + "99999999999999999999": time.Duration(maxRetryAfterSeconds) * time.Second, + "-99999999999999999999": 0, } for in, want := range cases { if got := retryAfter(in, now); got != want { @@ -1142,3 +1145,60 @@ func TestRedactedErrorsFormatWithoutKey(t *testing.T) { } } } + +func TestRateLimitedHugeRetryAfterIsCapped(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "9223372036854775807") + w.WriteHeader(http.StatusTooManyRequests) + })) + t.Cleanup(srv.Close) + p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: testKey, Model: "test-model", Headers: map[string]string{"X-Title": testKey}, StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + err = p.Verify(context.Background()) + var rl *llm.RateLimitedError + if !errors.Is(err, llm.ErrRateLimited) || !errors.As(err, &rl) || rl.RetryAfter != time.Duration(maxRetryAfterSeconds)*time.Second { + t.Fatalf("err = %v, want a RateLimitedError with RetryAfter capped to %v", err, time.Duration(maxRetryAfterSeconds)*time.Second) + } +} + +func TestRedirectIsNotFollowed(t *testing.T) { + var mu sync.Mutex + var secondCalls int + var secondHeader http.Header + second := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + mu.Lock() + secondCalls++ + secondHeader = req.Header.Clone() + mu.Unlock() + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(second.Close) + first := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + http.Redirect(w, req, second.URL+req.URL.Path, http.StatusFound) + })) + t.Cleanup(first.Close) + p, err := New(Config{BaseURL: first.URL + "/v1", APIKey: testKey, Model: "test-model", Headers: map[string]string{"X-Title": testKey}, StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + + err = p.Verify(context.Background()) + var pe *llm.ProviderError + if !errors.Is(err, llm.ErrProvider) || !errors.As(err, &pe) || pe.Status() != http.StatusFound { + t.Fatalf("err = %v, want a ProviderError for HTTP 302", err) + } + mu.Lock() + defer mu.Unlock() + if secondCalls != 0 { + for name, values := range secondHeader { + for _, v := range values { + if strings.Contains(v, testKey) { + t.Errorf("redirect target saw the key in %s", name) + } + } + } + t.Fatalf("redirect target got %d requests, want 0", secondCalls) + } +}