Reject a Retry-After that overflows the Duration conversion - #297
Open
dngr2 wants to merge 1 commit into
Open
Conversation
parseRetryAfterHeader converts a seconds value with
`time.Second * time.Duration(sleep)`. A Duration counts nanoseconds, so
any value above math.MaxInt64/time.Second (9223372036, about 292 years)
wraps and comes back negative:
Retry-After: 9223372037 -> -2562047h47m16s
Retry-After: 9223372036854775807 -> -1s
The header is reported as parsed, so DefaultBackoff and
RateLimitLinearJitterBackoff return that negative duration as the wait.
time.NewTimer fires immediately on a negative duration, so the client
retries with no delay at all and keeps doing so until RetryMax is spent.
A server asking to be left alone for a long time is answered with a burst
of requests instead, which is the opposite of backing off.
The existing `sleep < 0` guard covers a negative value in the header but
not a positive one that becomes negative in the conversion. Values past
the representable range are now treated as unparseable, so the caller
falls back to its usual exponential backoff, which is what happens for
any other unusable header.
Adds table cases for the first overflowing value, MaxInt64, and the
largest value that still converts cleanly. The first two fail without
the guard.
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
parseRetryAfterHeaderconverts a seconds value withtime.Second * time.Duration(sleep). ADurationcounts nanoseconds, so any value abovemath.MaxInt64/time.Second(9223372036, about 292 years) wraps and comes back negative:The header is reported as successfully parsed, so
DefaultBackoffandRateLimitLinearJitterBackoffhand that negative duration back as the wait.client.go:784does:A negative duration makes the timer fire immediately. The client retries with no delay and keeps doing so until
RetryMaxis spent — so a server asking to be left alone for a long time is answered with a burst of requests instead. That is the opposite of what the header, and this library, are for.The existing guard covers a negative value in the header:
but not a positive value that becomes negative in the conversion.
Related Issue
None open that I could find; noticed while reading the backoff paths.
How Has This Been Tested?
Three cases added to the existing
TestParseRetryAfterHeadertable:seconds-overflow9223372037seconds-overflow-max-int649223372036854775807seconds-largest-representable92233720369223372036s, parseableThe third pins the boundary so the guard cannot be tightened by accident and start rejecting valid values.
Values past the representable range are treated as unparseable, so the caller falls back to its usual exponential backoff — the same thing that already happens for an empty, malformed or date-in-the-past header. Clamping to
MaxInt64instead would mean a ~292 year wait, which seemed worse than ignoring an absurd header.go test ./...passes (22.8s). Removing the guard fails the two overflow cases and leaves the boundary case passing.Go 1.23.4, Linux.