Repository navigation
Send set_proxy's credentials on the HTTPS CONNECT, and keep proxy credentials off redirects - #3970
Thanatos9404 wants to merge 2 commits into
Conversation
ProxyHandler adds the credentials of a proxy URL, such as HTTPS_PROXY=http://user:password@proxy:3128, to each request it routes through that proxy as an ordinary Proxy-Authorization header, and HTTPRedirectHandler copies ordinary headers onto the redirected request. When that request is not proxied (its host is in NO_PROXY or the system bypass list), the credentials went to its host. Add _ProxyCredentialsRedirectHandler, which drops the header from the redirected request, and make _ValidatingRedirectHandler a subclass of it: pathsec.urlopen validates the new URL first, then drops the header. ProxyHandler adds it again when the redirected request is proxied. The SSRF attack harness gets cases that run pathsec.urlopen against stub servers on 127.0.0.1, with the credentials in HTTP_PROXY and HTTPS_PROXY, for each redirect code: a redirect to a NO_PROXY host must not receive the header, and a redirect the proxy carries must still authenticate. This comes first because the fix for nltk#3109 makes nltk.set_proxy put its user and password in the proxy URL too. Refs nltk#3109.
nltk.set_proxy(proxy, user=..., password=...) only installed ProxyBasicAuthHandler, which answers a 407 to a plain HTTP request. An HTTPS request goes through a CONNECT tunnel that http.client opens before any handler sees a response, so the proxy got no credentials and nltk.download failed with "Tunnel connection failed: 407 Proxy Authentication Required". Put the credentials in the proxy URL given to ProxyHandler, percent-encoded so that an "@", ":" or "/" in them stays data. urllib sends them as Proxy-Authorization with every request it routes through the proxy, and urllib.request.AbstractHTTPHandler.do_open moves that header onto the CONNECT and keeps it from the origin server. pathsec.urlopen already copies the proxy URL from the installed opener, so the downloader picks them up. The basic and digest handlers stay for plain HTTP. The scheme and host come from urllib.request._parse_proxy, the parser ProxyHandler itself uses, so the credentials go to the host the requests go to, also when the proxy URL holds another URL in its path, query or fragment. Credentials already in the URL are replaced, and a proxy without a scheme still works. ProxyHandler adds the credentials as an ordinary header, which urllib's redirect handler copies onto a redirected request. So the opener that set_proxy installs gets pathsec's _ProxyCredentialsRedirectHandler from the previous commit: a redirect to a host that bypasses the proxy (NO_PROXY) must not receive the credentials, also through a plain urllib.request.urlopen, and ProxyHandler adds them again when the redirected request is proxied. password=None is treated like the default empty password. The (user, password) tuple that web/data.rst passed as the second argument is still accepted; a user or password that is not exactly a str now raises TypeError instead of being sent in its str() form. Also fix the web/data.rst example to pass user= and password=. The SSRF attack harness gets the set_proxy cases: the redirect cases of the previous commit, the CONNECT of an HTTPS request (with the credentials from set_proxy or from HTTPS_PROXY), hostile credential strings, and proxy URLs with another URL after the host. Fixes nltk#3109.
|
Please read #3883 carefully (human too). Solving the previous PR you’ve opened together with this would make more sense. Also this involves security, please turn up the agent model’s effort to xhigh or above. And make sure the benign, malicious paths and all pentesting flow from develop-security branch is properly followed |
| new_req = super().redirect_request(req, fp, code, msg, headers, newurl) | ||
| if new_req is not None: | ||
| new_req.remove_header("Proxy-authorization") | ||
| return new_req |
There was a problem hiding this comment.
This becomes a threat vector by itself. Please ask your agent to review this and go through the pentest playbook thoroughly with xhigh effort. https://github.com/nltk/nltk/blob/develop-security/tools/pentest/AGENT_PENTEST.md
There was a problem hiding this comment.
Thanks, it needed a closer look. I went through AGENT_PENTEST.md on develop-security for this surface, mainly the network class in section 7.2 (a redirect to a blocked target, a DNS rebind between validation and connect, a proxy that resolves the name). The handler hid a bigger problem instead of solving it. The fixes below are on #3965, which now also carries this PR's commits, as I explain there.
Under a proxy, pathsec.urlopen left out the pinning handlers entirely, since they could not open a CONNECT tunnel. The opt in vouches for the proxy, but urllib still makes some connections itself: to a host in NO_PROXY or the system bypass list, to a redirect target that leaves the proxy (the case this handler was written for), and to a proxy a caller's Request names, when ProxyHandler leaves it in place. Those went out by host name after validate_network_url had resolved it once, so a name that validated as public could rebind to loopback. My own test asserted that such a redirect succeeds, which made the path look supported. With HTTPS_PROXY and a redirect it was already reachable on develop.
0af531e keeps the pinning handlers in place under a proxy too. They hand a connection to urllib only when ProxyHandler routed the request to one of the configured proxies, either a plain HTTP request to it or the CONNECT of a tunnel through it. Everything else is pinned like a direct fetch. The proxy hosts are read with urllib.request._parse_proxy, the parser ProxyHandler uses. The SSRF harness gets section 21: the rebind behind a redirect off the proxy (environment or set_proxy, 302 or 307, http or https), the first hop with the proxy from the environment or from set_proxy, and a Request that names its own proxy. Before the fix the egress recorder caught a connection by name in 10 of those 11 cases; the eleventh was already pinned. test_env_proxy_opt_in_skips_pinning_handlers encoded the old design, so it is replaced by a test that the env ProxyHandler keeps its proxies and both pinning handlers trust exactly that proxy.
The handler itself stays, because it still does one job: with credentials in HTTP_PROXY and a second proxy without credentials in HTTPS_PROXY, a redirect from http to https would otherwise carry the first proxy's credentials onto the second proxy's CONNECT. There is now a test for that, with a teeth case that restores urllib's own redirect handling and sees the leak.
The same pass found a credential confusion in set_proxy (066afdc). A : in the user name is percent encoded into the proxy URL, but urllib unquotes it and sends user:password, so the proxy splits in the wrong place: for [email protected]:8080/ it reads the user [email protected]. My hostile credential test compared header strings and missed it. RFC 7617 also allows no control characters. set_proxy now raises ValueError for both, without repeating the credentials, and leaves the installed opener alone.
If you had a different vector in mind for this handler, please point me at it and I will add it to the harness.
Drafted with Claude Code, posted on my behalf.
Fixes #3109.
What and why
nltk.set_proxy(proxy, user=..., password=...)installedProxyBasicAuthHandlerandProxyDigestAuthHandler, which only answer a407to a plain HTTP request. NLTK data comes over HTTPS, which a proxy carries through aCONNECTtunnel, and urllib opens that tunnel before any handler sees a response. So the proxy never got the credentials, andnltk.download()failed withTunnel connection failed: 407 Proxy Authentication Required. The 2026-10-07 issue map in #3883 reproduced this and found that credentials in the proxy URL do get sent.How
Two commits. The redirect fix comes first, so no commit in the history sends
set_proxy's credentials to a host that develop did not send them to.nltk/pathsec.py: do not carry proxy credentials across a redirect. When a proxy URL has credentials,ProxyHandleraddsProxy-Authorizationto each request it routes through the proxy as an ordinary header, andHTTPRedirectHandlercopies ordinary headers onto the redirected request. If that request bypasses the proxy (NO_PROXYor the system bypass list), its target received the credentials. A new_ProxyCredentialsRedirectHandlerremoves the header from the redirected request, and_ValidatingRedirectHandlernow subclasses it:pathsec.urlopenruns the SSRF check of the new URL first, then drops the header.ProxyHandleradds the header back if the redirected request is proxied. On develop this leak already happened withHTTPS_PROXY=http://user:password@...credentials (with the proxied-fetch opt-in set); the next commit makesset_proxyuse the same mechanism. The SSRF attack harness gets the redirect cases for this commit (see Tests).nltk/util.py: send the credentials on the CONNECT.set_proxyputsuserandpasswordinto the proxy URL it givesProxyHandler, each percent-encoded withquote(..., safe="")so that an@,:,/,%or aDOMAIN\userbackslash stays data. urllib then sendsProxy-Authorization: Basic ...with every request it routes through the proxy. For HTTPS,urllib.request.AbstractHTTPHandler.do_openmoves that header onto theCONNECTand does not send it to the origin server.pathsec.urlopenalready copies the proxy URL from the installed opener, sonltk.download()needs no other change.host:portcome fromurllib.request._parse_proxy, the parserProxyHandleritself uses, and the URL is rebuilt asscheme://user:password@host:port. The credentials therefore go to the host the requests go to, also when the proxy URL holds another URL in its path, query or fragment (http://proxy:3128/?next=http://other:8080). Credentials passed toset_proxyreplace any already in the proxy URL. A proxy without a scheme (proxy.example.com:3128), an upper-case scheme and an IPv6 literal also work.set_proxyinstalls also gets pathsec's_ProxyCredentialsRedirectHandlerfrom commit 1. Code that callsurllib.request.urlopenafterset_proxyuses that opener, notpathsec.urlopen.utilimports the handler,quoteand_parse_proxyunder private names, sofrom nltk.util import *adds nothing to thenltknamespace.password=Nonenow means"". The(user, password)tuple thatweb/data.rstshowed is accepted. Any otheruserorpasswordthat is not exactly astrraisesTypeErrorbefore the installed opener is replaced. Astrsubclass is refused too (type(x) is not str), the rule in the proposed AGENTS.md (Add AGENTS.md file #3892).TypeError.web/data.rstnow passesuser=andpassword=.Tests
All offline. Every proxy and origin is a stub server on 127.0.0.1. The target,
93.184.216.34, is a public address literal as in the SSRF tests, and only the stub proxy ever receives requests for it. Each stub proxy wants Basic credentials. It records the method, target andProxy-Authorizationof every request, and it never opens a tunnel (an authenticated CONNECT gets a 502). Everyurlopencall in the new tests passestimeout=5. In the attack harness, any connection to something other than a stub is recorded and refused, and each case asserts that none was attempted.54 new cases: 11 in commit 1 and 43 in commit 2.
Commit 1,
nltk/test/unit/test_pathsec.py:test_redirect_does_not_carry_proxy_credentials: a redirected request keeps its other headers but notProxy-Authorization.Commit 1,
nltk/test/unit/test_attack_ssrf_expanded.py(new section 20, "Proxy credentials reach the proxy and nothing else"). The credentials come fromHTTP_PROXYandHTTPS_PROXY, with the proxied-fetch opt-in set andNO_PROXY=127.0.0.1, and every request goes throughpathsec.urlopen:test_proxy_credentials_stay_off_a_redirect_that_bypasses_the_proxy[environment-301, -302, -303, -307, -308]: the proxy redirects to a second stub that bypasses it, and that stub gets noProxy-Authorization.test_proxy_credentials_follow_a_redirect_that_stays_proxied[environment-301, -302, -303, -307, -308]: the proxy gets the credentials on both requests, with no 407 in between.Commit 2,
nltk/test/unit/test_util.py. The stub proxy wantsCORP\alice/p@ss:w/rd %:test_set_proxy_authenticates_the_https_tunnel[url, no-scheme, upper-case-scheme, replaces-credentials-in-url, url-in-query]:pathsec.urlopen("https://93.184.216.34/index.xml")sends exactly one CONNECT to the configured proxy, and it carries the credentials.url-in-queryishttp://127.0.0.1:P/?next=http://127.0.0.1:9.test_set_proxy_still_authenticates_plain_httptest_set_proxy_keeps_credentials_off_a_redirect_that_bypasses_the_proxy: throughurllib.request.urlopenand the openerset_proxyinstalls, a redirect to a stub that bypasses the proxy carries noProxy-Authorization.test_set_proxy_authenticates_a_redirect_that_stays_proxied[urllib, pathsec]test_set_proxy_puts_the_credentials_in_the_proxy_url[password-none, legacy-tuple, ipv6-literal]test_set_proxy_rejects_credentials_that_are_not_strings[bytes-user, int-password, tuple-and-password, str-subclass]: each raisesTypeErrorand leaves the installed opener as it was.Commit 2,
nltk/test/unit/test_attack_ssrf_expanded.py. All of these go throughpathsec.urlopen:set_proxy:[set_proxy-301 ... set_proxy-308]for both redirect tests.test_proxy_credentials_authenticate_the_https_tunnel[environment, set_proxy]: the CONNECT carries the credentials fromHTTPS_PROXYor fromset_proxy.test_set_proxy_hostile_credentials_reach_the_proxy_as_data[https|http × url-delimiters, authority-in-user, crlf-in-password, query-fragment-brackets, percent-encoded-text, non-ascii]: 12 cases. The proxy receives exactly the credentials given, once, and no header is injected.test_set_proxy_credentials_go_to_the_configured_proxy_only[url-in-query, url-in-fragment, url-in-path]: a second stub, whose address appears later in the proxy URL, receives nothing.Failing on develop. The branch's three test files were run against develop's source with
-k "proxy_credentials or set_proxy or redirect_does_not_carry". That selects the 54 new cases and the existingtest_urlopen_honors_set_proxy_and_redirect_validation. Result: 46 failed and 9 passed on 3.13 and 3.14. On 3.10, 43 failed, 8 passed and the 4 cases for 308 were skipped.Tunnel connection failed: 407 Proxy Authentication Required. These are the 5test_utiltunnel cases, theset_proxytunnel case and the 6 hostile HTTPS cases.go_to_the_configured_proxy_onlycases fail because the CONNECT reached the configured proxy without credentials. On develop, too, the second stub got nothing.environmentbypass cases fail with "the proxy credentials left the proxy". This is the existing leak throughpathsec.urlopen.set_proxybypass cases, the 5set_proxystays-proxied cases, the 2test_utilstays-proxied cases and the 6 hostile HTTP cases fail because each request first goes out without credentials and gets a 407.assert 'Basic YWxpY2U6c2VjcmV0' is None.puts_the_credentials_in_the_proxy_urlcases fail because develop leaves the credentials out of the URL.DID NOT RAISE TypeError.8 new cases pass on develop, as intended. They are the plain-HTTP case, the
test_utilbypass case (develop sentset_proxy's credentials only as a header that urllib does not copy onto a redirect), the 5environmentstays-proxied cases and theenvironmenttunnel case. The last six are urllib's own behaviour.On this branch. All 55 pass on 3.13 and 3.14. On 3.10, 51 pass and the 4 cases for 308 are skipped. The 55 existing proxy, proxied and redirect tests pass on both develop and this branch. They are in
test_util.py,test_pathsec.py,test_attack_ssrf_expanded.py,test_advisory_probes.pyandtest_downloader_url_scheme_security.py.The downloader test files were run offline on 3.13 with the network-only
live_indextest deselected. Both the branch and develop give 634 passed, 83 skipped and 3 failed. The 3 failures are the tests that download real data. The security probe report (python -m nltk.test.unit.security_probes, 65 probes) gives the same verdicts on the branch and develop. black, isort, ruff, pyupgrade and the repository's guard hooks pass at both commits. At commit 1 alone, its own 11 cases pass (9 on 3.10, where the two 308 cases are skipped), and the rest oftest_pathsec.py,test_util.pyandtest_attack_ssrf_expanded.pygive the same results as on develop.Security notes
user, every request routed through the proxy now carriesProxy-Authorization: Basic ...from the start, including the CONNECT. develop sent it only after a Basic 407 to a plain HTTP request. Basic is base64, not encryption. On the client-to-proxy hop of anhttp://proxy, anyone who can read that traffic can read the credentials. They are sent only on requests routed through the proxy.ValueError: ... does not support the following scheme, and CONNECT failed with a 407. The outcome is still a failure, because urllib supports neither scheme on CONNECT.pathsec.urlopenand in the openerset_proxyinstalls.ProxyHandleradds them back only when the redirected request goes through the proxy. The harness covers 301, 302, 303, 307 and 308 throughpathsec.urlopen, with credentials from the environment and fromset_proxy. A target that bypasses the proxy received no header. A proxied target received the header once per request, with no 407. Python 3.10 does not follow 308 at all. The change also stopspathsec.urlopenfrom forwardingHTTPS_PROXY=http://user:password@...credentials to aNO_PROXYredirect target, which develop did. That includes a redirect sent by an HTTPS origin inside the tunnel, which was probed with a local TLS origin.set_proxytakes the proxy's scheme and host withurllib.request._parse_proxy, asProxyHandlerdoes. So withhttp://127.0.0.1:P1/?next=http://127.0.0.1:P2, or another URL in the path or fragment, the CONNECT and the credentials go toP1, andP2receives nothing. A version that split the string at its last://and@sent both toP2(see the audit trail).[email protected]:8080/, a password containing\r\nX-Injected: 1, URL and IPv6 delimiters, already percent-encoded text and non-ASCII. Each reached the stub proxy once, decoded back exactly, and added no header.Downloader.download.pathsec.urlopenstill refuses any proxied fetch unlessNLTK_ALLOW_PROXIED_URLOPEN=1ornltk.pathsec.ALLOW_PROXIED_FETCH = Trueis set, because NLTK cannot SSRF-pin a fetch made through a proxy (GHSA-6ww7). Without the opt-in,nltk.download()throughset_proxyreports that refusal, on develop and here alike. With the opt-in, develop got the 407 from this issue, and this branch authenticates.Compatibility
(user, password)tuple from the oldweb/data.rstexample is accepted. It never worked on develop: over plain HTTP the proxy received the literal user name('USERNAME', 'PASSWORD')and answered 407, and over HTTPS nothing was sent.password=Nonemeans"". On develop,Nonemade the basic handler send nothing.userandpasswordmust be exactlystr, and anything else raisesTypeError, including astrsubclass. On develop, anintpassword worked over plain HTTP, sent in itsstr()form. It never worked over HTTPS. Abytesuser was sent asb'alice'.user, a proxy URL that urllib cannot parse, such ashttp:/proxy:3128with no authority, now raises urllib'sValueErrorfromset_proxyitself, before the installed opener is replaced. On develop,set_proxyaccepted it and the sameValueErrorcame from the first fetch.Proxy-Authorizationheader that a caller sets on its ownRequestis now also dropped on a redirect through the openerset_proxyinstalls.Limits
urllib.request.urlopenwith credentials inHTTP(S)_PROXYand noset_proxycall uses urllib's default opener, which still copies the header onto redirects. That is stdlib behaviour. NLTK's own fetches go throughpathsec.urlopen.set_proxydoes not validate the proxy string beyond urllib's parsing. With an@after the host (http://proxy:3128/x@y), urllib readsyas the host, on develop too, and the credentials go there with the requests.Transparency (per #3883)
Prompt
(local paths redacted; "…" marks omitted boilerplate about local paths and tooling)
Read To contributors and agents scanning Github issues to create new pull requests #3883 and its issue map, Tunnel connection failed: 407 Proxy Authentification Required #3109 and its comment, fix set_proxy for https #1624, the open AGENTS.md PR Add AGENTS.md file #3892, and the proxy code in
nltk/util.pyandnltk/pathsec.py. The pathsec code includesurlopen's handler copying and the proxied-fetch opt-in. PR searches found nothing addressing Tunnel connection failed: 407 Proxy Authentification Required #3109.Wrote the stub-proxy test first. On develop the CONNECT carried no credentials and failed with the reported 407, while plain HTTP authenticated after a 407.
Read the urllib paths.
ProxyHandler.proxy_openaddsProxy-authorizationfrom the URL's credentials, andAbstractHTTPHandler.do_openmoves it into the tunnel headers. Put the quoted credentials into the proxy URL. Probing then showed that the credentials now followed a redirect to a host that bypasses the proxy, which develop'sset_proxydid not do. Added a fix inpathsec._ValidatingRedirectHandler. First version: 2 commits, theset_proxychange first, +125 −4, 3 test cases.Review pass 1, and the changes it led to:
set_proxycommit leaked on its own until the pathsec commit, so the order was swapped.urllib.request.urlopenafterset_proxystill copied the header to a bypassed host, so the openerset_proxyinstalls got its own redirect handler.quoteis now imported as_quote, sofrom nltk.util import *does not export it.password=Nonenow means"". Before, it raised aTypeErrorfromquote().(user, password)tuple from the old docs is accepted, and other non-strings raise a clearTypeError.Review pass 2:
set_proxysplit the proxy string at its last://and@, so withhttp://127.0.0.1:P1/?next=http://127.0.0.1:P2the CONNECT and the credentials went toP2. Second, the SSRF attack harness had no cases for this surface, which To contributors and agents scanning Github issues to create new pull requests #3883's audit asks for. Third, the header-dropping handler was written twice, andfrom nltk.util import *exportednltk.HTTPRedirectHandler, which develop did not.Review pass 3, a security review of that branch. It asked for the following:
urllib.request._parse_proxy, with aurl-in-querytunnel case that fails on the earlier version.pathsec.urlopen: credentials fromset_proxyand fromHTTPS_PROXY, every redirect code, hostile credentials, and proxy URL shapes.pathsec, import it under a private name, and add no public names tonltk.type(x) is not strin the proposed AGENTS.md (Add AGENTS.md file #3892).AbstractHTTPHandler.do_openin the commit message, passtimeout=to every newurlopen, and document theTypeError, the non-empty HTTPS password and the preemptive Basic in the docstring.Both commits were rebuilt from develop to meet these points. Commit 1 now adds
_ProxyCredentialsRedirectHandlerand the harness section, and commit 2 imports that handler. Thestr-subclass, upper-case-scheme and IPv6 cases were added, and so was an HTTPS tunnel case that runs with both credential sources. The result is 2 commits, +609 −9, with 54 new test cases. Theurl-in-querycases in both files, and theurl-in-fragmentandurl-in-pathharness cases, fail on the earlier version and pass now.dir(nltk)is the same as on develop.Re-ran the matrix on Python 3.10, 3.13 and 3.14. The branch's tests were run against develop's source and against the earlier version. Each commit was also tested alone, and the branch was tested merged onto the current upstream develop. The scratch probes ran on the branch, commit 1, develop and the earlier version. Then came the security probe report, the offline downloader test files, the formatters and the guard hooks.