gh-159044: Restore string conversion for HTTP tunnel header values

[bielpieroli]

HTTPConnection.connect() calls _tunnel(), which builds the CONNECT request from the headers passed to set_tunnel(). Since the fix for gh-146211 (#146212), header names and values are encoded directly with .encode("latin-1"), so non-string values such as None or an int raise AttributeError. This breaks tools like Apache Thrift, which passes {"Proxy-Authorization": None} to set_tunnel() when using an unauthenticated proxy (see the reproducer in the issue).

The issue includes a version table showing the regression appears in 3.12.14, 3.13.14 and 3.14.5 (and is present in 3.15.0rc3), so this should be backported..

As suggested by @ZeroIntensity in the issue, this PR converts header names and values with str() before encoding, restoring the previous behavior. The CR/LF validation is unchanged. Also, the set_tunnel() docs now mention the conversion.

Tests

• ./python -m test test_httplib -m test_tunnel_nonstr_headers -v: passed
• ./python -m test test_httplib: passed
• ./python -m test -j0: passed

• Issue: http.client.HTTPSConnection._tunnel raises AttributeError: 'NoneType' object has no attribute 'encode' when header value is None #159044

Co-authored-by: Lorena Borges lborgescomp@gmail.com

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[read-the-docs-community]

Documentation build overview

📚 cpython-previews | 🛠️ Build #35083768 | 📁 Comparing 7257629 against main (aab474a)

  🔍 Preview build  

2 files changed

± library/http.client.html
± whatsnew/changelog.html

[edvilme]

Suggested change

	self.conn.set_tunnel('destination.com', headers={
	self.conn.set_tunnel('example.com', headers={

If you are using an example domain maybe use that one instead (destination.com is a real non-example domain)?

[bielpieroli]

Thanks for the suggestion, @edvilme! I used destination.com to stay consistent with the other tests in TunnelTests. The file also uses example.com, but in this class destination.com is the most common. So I'd lean towards keeping it, but I'm happy to switch to example.com if you think it reads better.

[edvilme]

You are converting header values and keys to strings. Maybe the tests can also include this to avoid future regressions?

[bielpieroli]

Good idea, I think it makes sense.

However, I tried adding a non-string key to the test, but it fails earlier: set_tunnel() calls header.lower() on every key to check for a Host header, so non-string keys raise AttributeError there, before _tunnel() is reached.

That check predates this issue, so supporting non-string keys would probably be new behavior.

I think it'd make sense to update the docs to only mention values. Do you think that's the right approach?

[bielpieroli]

ERROR: test_tunnel_nonstr_headers (test.test_httplib.TunnelTests.test_tunnel_nonstr_headers)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../cpython/Lib/test/test_httplib.py", line 2801, in test_tunnel_nonstr_headers
    self.conn.set_tunnel('destination.com', headers={
    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        'None-Value': None,
        ^^^^^^^^^^^^^^^^^^^
    ...<4 lines>...
        None: 'none-key',
        ^^^^^^^^^^^^^^^^^
    })
    ^^
  File ".../cpython/Lib/http/client.py", line 974, in set_tunnel
    if not any(header.lower() == "host" for header in self._tunnel_headers):
           ^^^
  File ".../cpython/Lib/http/client.py", line 974, in <genexpr>
    if not any(header.lower() == "host" for header in self._tunnel_headers):
               ^^^^^^^^^^^^
AttributeError: 'int' object has no attribute 'lower'

----------------------------------------------------------------------

[ZeroIntensity]

Yes, let's just limit the PR to values only if keys didn't work anyway.

[bielpieroli]

Thanks for the review, @ZeroIntensity! I reverted str() on keys (since non-string keys already fail earlier in set_tunnel() at the header.lower() check) and updated the docs to mention only values.

-            header_bytes = str(header).encode("latin-1")
+            header_bytes = header.encode("latin-1")
             value_bytes = str(value).encode("latin-1")

And thanks a lot, @edvilme, for the discussion here and in the issue. Your review comments helped us find this gap in the set_tunnel() key check, which we wouldn't have noticed otherwise :D

[edvilme]

Thank YOU, glad you found it :)

[edvilme]

Should we add a try/catch here for str()?

[bielpieroli]

I don't think we need one: if __str__ raises, the exception propagates as it did with the old f-string, and the caller gets the original error. Catching it would mean either hiding the error or raising a different exception type, which would change behavior, wouldn't it?

[ZeroIntensity]

LGTM, thanks!

For future reference, you don't need to force-push. We squash merge at the end.

[ZeroIntensity]

3.11-3.13 RMs: @pablogsal @Yhg1s -- Any opinions on backporting this to the security versions? This fixes a regression introduced in a security fix (#146212), but it doesn't seem too widespread (yet?). I'm not sure what the process is here.