fix: cap Retry-After sleep to RetryWaitMax in DefaultBackoff#283
Open
nghiack7 wants to merge 1 commit into
Open
fix: cap Retry-After sleep to RetryWaitMax in DefaultBackoff#283nghiack7 wants to merge 1 commit into
nghiack7 wants to merge 1 commit into
Conversation
|
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 Nghĩa Nguyễn Ngọc seems not to be a GitHub user. Have you signed the CLA already but the status is still pending? Recheck it. |
DefaultBackoff respected the Retry-After header value without enforcing the RetryWaitMax limit. A server could therefore force an arbitrarily long wait (e.g. "Retry-After: 86400") even when the caller had set a short RetryWaitMax. Add a bounds check in DefaultBackoff so that the parsed Retry-After duration is capped at max, consistent with how the exponential backoff path already enforces the limit. Add TestClient_DefaultBackoff_RetryAfterExceedsMax to cover the case where the header value exceeds RetryWaitMax (429 and 503 variants). Fixes hashicorp#247
bf46940 to
a5ab50a
Compare
Author
|
@hashicorp-cla-app recheck |
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.
Problem
When a server responds with a
Retry-Afterheader (e.g.Retry-After: 3600),DefaultBackoffunconditionally sleeps for that duration — completely ignoring the client's configuredRetryWaitMax. This means a misbehaving or malicious server can force a client to sleep for an arbitrarily long time, bypassing the user's intent.Fixes #247.
Root cause
Fix
Cap the
Retry-Aftersleep atmax(which isRetryWaitMax):This is consistent with how the exponential backoff path already respects
RetryWaitMax.Tests
Added
TestClient_DefaultBackoff_RetryAfterExceedsMaxcovering:Retry-After: 3600→ capped toRetryWaitMax(10s)Retry-After: 3600→ capped toRetryWaitMax(10s)All existing tests pass.