Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: ignore invalid Retry-After dates #15833

Closed
wants to merge 4 commits into from
Closed

Conversation

jay
Copy link
Member

@jay jay commented Dec 26, 2024

  • If the Retry-After date is in the past then set the retry after time to 0 instead of a negative number.

Prior to this change, libcurl could set the retry after time to a negative number of seconds. That is not documented and may be unexpected by the user.

Closes #xxxx

- If the Retry-After date is in the past then set the retry after time
  to 0 instead of a negative number.

Prior to this change, libcurl could set the retry after time to a
negative number of seconds. That is not documented and may be
unexpected by the user.

Closes #xxxx
@jay jay added the HTTP label Dec 26, 2024
@jay
Copy link
Member Author

jay commented Dec 26, 2024

This change is causing test 1596 to fail. The test is for Retry-After timestamp parsing using a date in the past, so it expects CURLINFO_RETRY_AFTER to return a negative number.

curl/tests/data/test1596

Lines 15 to 18 in fb1883d

HTTP/1.1 429 Too Many Requests
Date: Thu, 11 Jul 2019 02:26:59 GMT
Server: test-server/swsclose
Retry-After: Thu, 11 Jul 2024 02:26:59 GMT

#ifdef LIB1596
/* we get a relative number of seconds, so add the number of seconds
we're at to make it a somewhat stable number. Then remove accuracy. */
retry += time(NULL);
retry /= 10000;
#endif
printf("Retry-After %" CURL_FORMAT_CURL_OFF_T "\n", retry);

If there's no objection I will change the timestamp to something in the future.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@testclutch
Copy link

Analysis of PR #15833 at 75b506f6:

Test 1596 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 94 different CI jobs (the link just goes to one of them).

Generated by Testclutch

check against a date in the future instead of a date in the past
@github-actions github-actions bot added the tests label Dec 26, 2024
@jay
Copy link
Member Author

jay commented Dec 27, 2024

Being able to set the retry-after date this far into the future (year 2036) also makes me wonder. Is there an upper limit on what an acceptable time should be? I didn't see one in the RFC but I would think after some number of hours it's long enough

@bagder
Copy link
Member

bagder commented Dec 30, 2024

I would think after some number of hours it's long enough

Agreed. Maybe cap it at 5 or 10 hours or so?

@jay
Copy link
Member Author

jay commented Dec 30, 2024

I really don't know. I changed the max to 24 hours but did not document it so that we'll have some flexibility to change it in the future.

@jay jay force-pushed the fix_retry_after branch 2 times, most recently from c272cd7 to aa92760 Compare December 30, 2024 08:24
@jay
Copy link
Member Author

jay commented Dec 30, 2024

I changed my mind and made it 6 hours. 6 hours seems like a long time. Would a server return a longer time?

@bagder
Copy link
Member

bagder commented Dec 30, 2024

Highly unlikely. And even if there is a single one out there doing this, I don't think trying a little too early every 6 hours can be considered abuse.

@dfandrich
Copy link
Contributor

dfandrich commented Dec 30, 2024 via email

@jay jay closed this in 6c70ec1 Dec 31, 2024
@jay jay deleted the fix_retry_after branch December 31, 2024 08:22
@jay
Copy link
Member Author

jay commented Dec 31, 2024

Ok. I agree that there may be legit use cases and also that it's acceptable to try again in a shorter period of time so I landed it unchanged with a 6 hour limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants