search

psf / requests

A simple, yet elegant, HTTP library.
https://requests.readthedocs.io/en/latest/
Apache License 2.0
52.19k stars 9.33k forks source link

Deprecated `HTTPAdapter.get_connection()` method is never called, causing breakage without deprecation warnings #6733

Closed wgreenberg closed 5 months ago

wgreenberg commented 5 months ago

After upgrading several of certbot's packages, including requests to 2.32.3, we suddenly started experiencing confusing DNS resolution errors with no clear cause. Only after bisecting the changed packages and isolating requests as the source of the trouble did we see that 2.32.2 deprecated the HTTPAdapter.get_connection() method we used.

Unfortunately, the change which deprecated get_connection() also causes it never to be called by Session, so the DeprecationWarning added in https://github.com/psf/requests/pull/6710 will never be printed. If Session perhaps checked if get_connection() was defined and called it, the warning would've printed and would've saved us quite a lot of time tracking down the issue.

I also generally wouldn't expect a minor version upgrade in a dependency to cause a breaking change, so this result was extra confusing.

Expected Result

HTTPAdapter.get_connection() to still be called, resulting in no breaking changes and giving users the deprecation warning. Or at the very least, check if get_connection is defined and print the deprecation warning then.

Actual Result

HTTPAdapter.get_connection() is silently left uncalled, causing a breakage.

nateprewitt commented 5 months ago

Hi @wgreenberg, the commit you linked appears to be part way through the issue. #6710 was to provide a public API for users who were overriding get_connection. The initial breakage came from #6655 to address CVE-2024-35195.

While we don't generally make breaking changes in minor patch versions, users were unknowingly exposing data over unverified connections. We chose to patch the API but it appears docker and certbot are two cases that were negatively impacted.

There were a few misses in messaging around this change because impact wasn't assessed completely prior to releasing 2.32.0. The workaround provided in #6710 will be the backwards compatible middle ground to keep adapters using a custom get_connection working with the least amount of work going forward.

nateprewitt commented 5 months ago

I'm going to resolve this since we haven't heard back anything else. Let us know if you have any other questions.