InvalidHeader: Invalid Retry-After header: 1m0s with github.com

[ssbarnea]

Apparently retry logic inside urllib3 chokes when receiving 429 replies from github itself.

self = Retry(total=2, connect=None, read=None, redirect=None, status=None)
retry_after = '1m0s'

    def parse_retry_after(self, retry_after):
        # Whitespace: https://tools.ietf.org/html/rfc7230#section-3.2.4
        if re.match(r"^\s*[0-9]+\s*$", retry_after):
            seconds = int(retry_after)
        else:
            retry_date_tuple = email.utils.parsedate(retry_after)
            if retry_date_tuple is None:
>               raise InvalidHeader("Invalid Retry-After header: %s" % retry_after)
E               InvalidHeader: Invalid Retry-After header: 1m0s

Even more interesting, the parsedate is now no longer part of the stdlib,

urllib3/src/urllib3/util/retry.py

Line 254 in 728d924

retry_date_tuple = email.utils.parsedate(retry_after)

[pquentin]

Thanks for opening an issue and a pull request! I have a few questions about the issue.

Why would email.utils.parsedate not be part of the standard library? I see it there: https://docs.python.org/3/library/email.utils.html#email.utils.parsedate

1m0s is indeed not a compliant Retry-After header: https://tools.ietf.org/html/rfc7231#section-7.1.3. Do you have a simple way to reproduce the GitHub issue? Did you try submitting a bug to them? They should just send "60" in that case.

Regarding your pull request, urllib3 does not depend on external libraries. We can vendor small libraries, but there needs to be a real use case for this.

[ssbarnea]

I was sure I got an error about missing parsedate on newer version of python but apparently is was not removed https://github.com/python/cpython/blob/master/Lib/email/utils.py -- maybe it was caused by some shadowing from another library or relative path imports. I am glad to use it as adding extra dependency for this is a very bad idea.

I did not raise a bug with github yet, even if my hopes about them doing anything about it are very low. Here is the place where I found the issue https://review.rdoproject.org/zuul/build/5c23a6536eff46778c91beb75598c319 and the code generating it can be seen at https://review.rdoproject.org/r/#/c/24100/9/test/test_verify.py

I was still unable to reproduce this error locally, I guess i will have to do much more DDOS requests to trigger the 429 response from github.

So first we need to find a way to reproduce the error with github. If it does reproduce it would make sense to add an extra regex for this case. Clearly external dependency makes no sense, that piece of code will be removed form the next patch.

[sethmlarson]

Btw I've found companies usually fairly sympathetic to fix HTTP issues. Has worked for me in the past. :) Also are we sure it is GitHub instead of Fedora's website that is triggering the issue?

[ssbarnea]

@sethmlarson good point, my guess was about github because I knew they had the habbit to respond with 429, but clearly I need to look again.

[ssbarnea]

I can confirm tha this error comes from github.com, which replies with non-conformant HTTP header value for Retry-After: 1m0s, instead of the correct one which should be numerical Retry-After: 60

[sethmlarson]

I wasn't able to get a Retry-After header after using up all of the free tier API requests so I can't verify. If you have additional information to help reproduce the issue I'd put it here and also submit a ticket to GitHub Support to get it fixed.

[ssbarnea]

I already submitted a ticket, using two channels. Hopefully they will reply. Is tricky to reproduce because if you ever authenticates the thresholds are really bit. Is much easier to reproduce with a container and ~1000 requests. I will wait a bit to let them reply on the issue.

Our of curiosity, if they confirm the issue, would it make sense to patch urllib3 to make it work with these invalid values?

[sethmlarson]

It's actually sometimes in the interest of the broader community to be strict about supporting behavior that isn't compliant. GitHub is the only site we've heard doing this, so instead of making more work for other HTTP clients, browsers, and tools ("hey your website works with urllib3 but not with curl!") we should make the offending party fix their setup ("hey your website doesn't work with any HTTP clients") to reduce non-standard functionality that all HTTP clients have to support. :)

So we probably won't be supporting this in the client itself unless it becomes a standard, then we definitely will! I'm hopeful your issue can be resolved regardless as we're talking about GitHub as opposed to a smaller company that might be less receptive to issues like this.

[pquentin]

For more details, see https://datatracker.ietf.org/doc/draft-iab-protocol-maintenance/?include_text=1

[sethmlarson]

We're going to close this issue as "not a defect in urllib3". GitHub needs to fix their headers.