Repository navigation
Fix redirect SSRF parsing, DNS rebinding, and private-target redirects - #3
Merged
Phantom9869 merged 2 commits intoOct 21, 2026
Conversation
Validate each redirect against the URL requests will connect to, dial the addresses that check vetted, resolve names with the system resolver under one deadline, and follow same-host redirects for private literals. Co-authored-by: Kamal Choubey <Phantom9869@users.noreply.github.com>
The pinned session kept trust_env off, so REQUESTS_CA_BUNDLE was ignored and a certificate failure was retried as plain HTTP. Apply the env CA bundle explicitly, and surface TLS verification errors instead of downgrading. Follow same-host redirects for names that resolve only to non-public addresses, still blocking a public-to-private pivot. Check proxies per URL, limit redirect ports to the same port or 80/443, return the 3xx when a followed hop fails, and resolve a port scan's host once. Co-authored-by: Kamal Choubey <Phantom9869@users.noreply.github.com>
Phantom9869
merged commit Oct 21, 2026
884c041
into
cursor/security-review-fixes-9f2b
8 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the four findings from the independent review of
cursor/security-review-fixes-9f2b(954e811), plus the follow-up review of head111307d. This branch is based on that branch. Base iscursor/security-review-fixes-9f2b, notmain, so it can be reviewed and folded into PR #2. Nothing here is merged, and PR #2's branch is untouched.1. Redirect SSRF via URL parser differential (high)
urllib.parse.urlsplitand the parser requests/urllib3 connect with do not agree on a backslash in the authority.Location: http://127.0.0.1:18080\@example.com/is hostexample.comtourlsplitandhttp://127.0.0.1:18080/%5C@example.com/to requests, so the old same-host check passed and the client dialed loopback.Redirects are decided on
requests.Request(...).prepare().urlparsed withurllib3.util.parse_url. That prepared URL is what the next hop sends, and it must survive a second prepare unchanged. The host that is checked is the host that hop connects to. Location values that contain a raw backslash, whitespace, or a control character are refused as well; the parser check still blocks the backslash case when that filter is bypassed.%5Cand@are covered too. Both parsers agree on those, so they are not the bypass:%5C@example.comstays onexample.com, andexample.com@127.0.0.1is a different host and is not followed.2. DNS rebinding / TOCTOU (medium)
The safety check resolved with dnspython, then requests resolved again with
getaddrinfoat connect time. The connection now opens a socket to the exact addresses that decision vetted. The URL host is left in place for the Host header and TLS SNI. There is no second name lookup between the check and the connect, including on redirect hops.Residual risk:
proxies=, orHTTP_PROXY/HTTPS_PROXY/ALL_PROXYand the host is not bypassed), redirects are not followed and that one request uses requests' normal proxy path.NO_PROXYis consulted per URL; a bypassed host keeps the pin and may follow redirects.getaddrinfocannot be cancelled. A lookup that exceeds its deadline is abandoned; the daemon thread stays blocked in libc until the call returns. The scan does not wait, and the thread does not keep the process alive.3. System resolver regression (medium)
resolve_ipsonly used dnspython, so/etc/hosts, mDNS, and NSS were ignored (localhostcould come back as a non-loopback address). That brokehost_reachable, port scans, and TLS for lab names.Resolution is one
getaddrinfo(hosts file, then DNS, A and AAAA together) on a daemon thread with a single deadline. The deadline is not spent once per record type. Address literals still skip the lookup.connectstill dials the resolved address rather than the name.ports.runresolves the host once and passes that list into every port connect, so a scan does not start one resolver thread per port.connect(..., resolved=ips)is keyword-only; other callers still resolve themselves.4. Private-target redirects (medium)
Same-host redirects required every address to be public, so an internal HTTP to HTTPS hop stopped on the 301.
Choice: the user-supplied scan target is treated as private when it is a non-public address literal, or when every address from its first resolution is non-public. That covers
127.0.0.1/10.x/192.168.x/::1and hosts-file lab names such astarget.htbandlocalhost. An opt-in flag was the alternative; it would leave the common lab case (a name in the hosts file, HTTP to HTTPS on the same host) broken, which is the bug. The scan target is already the host the operator asked to scan.A same-host redirect is followed when every vetted address is public, or when that original target is private and every address from a fresh lookup of the next hop is non-public. The next hop is resolved again and is not allowed to reuse the first snapshot, so a name whose first answer is public (or mixed) and whose redirect answer is private is still a pivot and is not followed. Mixed addresses fail closed in both directions. The port must be the original port or the other side of the 80/443 pair; a hop from one high port to another is not followed. If the followed hop raises, the 3xx already in hand is returned instead of the exception.
5. CA bundle and TLS verification downgrade (medium, follow-up)
The pinned session sets
trust_env = Falseso an environment proxy cannot replace the dialed address. requests only readsREQUESTS_CA_BUNDLE/CURL_CA_BUNDLEwhentrust_envis true, so a target that verifies with plain requests and an internal CA failed here withCERTIFICATE_VERIFY_FAILED, andget_for_hosttreated thatSSLErroras a dead HTTPS port and retried over plain HTTP.trust_envstays false. The bundle is copied intoverifywhen the caller did not passverify=Falseor an explicit path (REQUESTS_CA_BUNDLE, thenCURL_CA_BUNDLE). A certificate verification failure, including a hostname mismatch, is re-raised asrequests.exceptions.SSLErrorwithnot retrying over HTTPin the message. The preferred scheme is not cached. Fetch modules that already catchRequestExceptionrecord that text on the existing "Could not fetch site" finding. A protocol mismatch (WRONG_VERSION_NUMBERon an HTTP-only port) still falls back to HTTP.6. NO_PROXY, redirect ports, and port-scan DNS (low, follow-up)
Proxy use is decided per prepared URL with
requests.utils.should_bypass_proxiesandget_environ_proxies. A host listed inNO_PROXY(orproxies["no_proxy"]) keeps pinning and same-host redirects. Any other configured proxy still skips the pin and does not follow redirects. The pinned send drops the proxies mapping soselect_proxycannot send a bypassed host through the proxy anyway (trust_envfalse does not strip an explicit proxy dict).Redirects may change port only for the 80↔443 pair, compared with the original request's port. A different high port is returned as the 3xx and is not fetched. A followed hop that errors (connection failure, TLS failure, and the rest of
RequestException) returns that 3xx withgambit_redirectsstill counting only hops that succeeded. The redirect body is buffered for the caller.Tests
python3 -m pytest -q: 180 passed.New coverage includes a real test CA (OpenSSL) on the pinned HTTPS path:
REQUESTS_CA_BUNDLEverifiespinned.gambit.test, a hostname mismatch is rejected (so this is notverify=False), andget_for_hostdoes not open a second connection for plain HTTP.NO_PROXYis tested with a proxy pointed at a closed port andgetaddrinfoblackholed, while127.0.0.1still follows a same-host redirect. Port changes cover same-port and 80↔443 follows, and a different high port against a local server is not contacted. Also covered: a private-only name (target.htb) andlocalhostfollow same-host redirects; a public-then-private resolution does not; a failed hop returns the 3xx;ports.runresolves once. The CA test skips whenopensslis not onPATH, so Windows CI without it stays green.pip-auditon the unchanged pins reports no known vulnerabilities for the direct requirements. CI still runs the full audit, plus pytest on 3.11 and 3.12.