Repository navigation
feat(ipv6): full IPv6 support across the stack (#201) - #212
Merged
Merged
Conversation
Closes #201. Make IPv6 a first-class citizen alongside IPv4 across detection, validation, anonymity comparison, SOCKS5 proxying, and provider parsing - replacing the prior situation of broken/half-wired IPv6 stubs scattered across the codebase. Why: Worldwide IPv6 user adoption is now ~45-50% (Google IPv6 stats). proxybroker2 previously rejected v6 from `Resolver.get_real_ext_ip()` via an IPv4-only `host_is_ip()`, so users on IPv6-only / IPv6-first networks failed at startup. The old `IPv6Pattern` regex (~700 chars, nosemgrep-suppressed) emitted tuples not strings via `findall`, so v6 anonymity detection silently never worked. A previous attempt (commit c1b60b0, July 2025) tried to add v6 support incrementally, shipped real bugs (e.g. `::ffff:192.0.2.1` rejected because it contains both `.` and `:`), and never made it to master. This PR fixes the root cause - lack of a coherent IP-handling contract - by adopting RFC 5952 canonical form (`str(ipaddress.ip_address(...))`) as the single in-process IP representation and threading it through every comparison site. Layer summary: - L1 (resolver): `host_is_ip` accepts v4+v6 via stdlib ipaddress (preserves legacy IPv4 leading-zero tolerance for provider feed compatibility). `get_real_ext_ip` returns canonical form. Adds `https://api64.ipify.org/` to `_ip_hosts` so v6-only networks work. - L2 (utils.get_all_ip): delete the 700-char `IPv6Pattern` regex + its `# nosemgrep` block. New narrow `_IPV6_CANDIDATE_PATTERN` tokenizer (provably ReDoS-free) feeds `ipaddress.ip_address` for validation; results returned in RFC 5952 canonical form so equivalent encodings collapse to one set element. New public `canonicalize_ip(s) -> str | None` helper. - L3 (checker._get_anonymity_lvl, judge.Judge.check): canonicalise both sides of the IP visibility check so v6 leaks classify correctly regardless of how the judge formatted the address (uppercase, expanded leading zeros, ::-compressed). Substring `real_ext_ip in page` swapped for canonical-form set membership in judge.py, matching the checker's logic. - L4 (negotiators.Socks5Ngtr): SOCKS5 ATYP=0x04 + 16-byte address for IPv6 destinations per RFC 1928; ATYP=0x01 + 4-byte for IPv4 (regression-tested). Reply size adjusted to match (10 bytes for v4, 22 bytes for v6). SOCKS4 stays IPv4-only by spec. - L6 (utils, providers, api): new `IPv6BracketedPortPattern` (`[v6]:port` per RFC 3986 § 3.2.2) + `find_proxy_pairs(text)` helper. Wired into `Provider._find_proxies` (so every provider feed surfaces v6 entries) and `Broker._load` (so file/raw-string proxy lists with bracketed v6 forms parse correctly). - L7 (proxy.Proxy): construction with v6 host literal validates via L1's host_is_ip. New `_format_host_port(host, port)` helper brackets v6 per RFC 3986 in `as_text`, `__repr__`, and `Proxy.log` so output is unambiguous and pastes cleanly into URI-syntax proxy clients. Out of scope (explicitly NOT half-fixes; orthogonal feature concerns): IPv6-only judges in defaults (existing httpbin et al already have AAAA records and aiohttp does Happy Eyeballs); live IPv6 integration tests in CI (runner v6 not guaranteed); GeoLite2 v6 ranges (blocked by #200, bundled DB has limited v6 coverage). Verification: - pytest: 239 pass (+32 new IPv6 tests; the 3 pre-existing test_resolver event_loop fixture errors are unrelated and were present on master before this branch). - Ruff: clean (`ruff check --fix`, `ruff format`). - Docker: built `proxybroker2-ipv6-smoke:latest`, smoke-tested all 5 capability layers (host_is_ip, canonicalize_ip, get_all_ip, find_proxy_pairs, Proxy(host=v6)) inside the container, and ran `find --types HTTP --limit 2` to regression-check the IPv4 production path - returned 2 working real HTTP proxies (US), confirming no v4 regression. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements RFC‑5952 canonical IP handling, replaces the fragile IPv6 regex with tokenization + ipaddress validation, extracts canonical ChangesIPv6 First-Class Support
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver
participant DNS_A as DNS(A)
participant DNS_AAAA as DNS(AAAA)
Client->>Resolver: request resolve(host, qtype=_QTYPE_DEFAULT)
Resolver->>DNS_A: query A
Resolver->>DNS_AAAA: query AAAA
alt AAAA returns non-empty first
DNS_AAAA-->>Resolver: AAAA result
else A returns non-empty first
DNS_A-->>Resolver: A result
end
Resolver->>Resolver: canonicalize results via canonicalize_ip()
Resolver-->>Client: return canonical answer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements comprehensive IPv6 support across the library, updating IP detection, provider parsing, and anonymity checking to use RFC 5952 canonical forms. It also adds SOCKS5 support for IPv6 destinations and ensures IPv6 hosts are correctly bracketed in URI components per RFC 3986. A high-severity issue was identified in the SOCKS5 negotiator where the response buffer size is incorrectly hardcoded based on the request's address type, which could cause connection hangs or data corruption if the server returns a different address type in its reply.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ebf8e3013
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
proxybroker/api.py (1)
382-388: 💤 Low valueConsider sharing the bracketed-IPv6 extraction with
utils.find_proxy_pairs.The inline loop here duplicates the v6 portion of
find_proxy_pairs(text)(extract bracketed token →canonicalize_ip→ drop onNone). The reason_loadcannot just callfind_proxy_pairsdirectly is thatfind_proxy_pairsusesIPPortPatternGlobal(lookahead) for v4, while_loaddeliberately uses the line-anchoredIPPortPatternLine(one pair per line — safer for raw-text and file inputs).A small extraction keeps both call sites in sync as the v6 tokenizer evolves and matches the coding guideline that
find_proxy_pairsis the single helper for[(ip_canonical, port), ...]output. As per coding guidelines: "use helperfind_proxy_pairs(text)to return[(ip_canonical, port), ...]for both v4 and v6".♻️ Suggested extraction
In
proxybroker/utils.py, expose a v6-only helper alongsidefind_proxy_pairs:def find_bracketed_ipv6_pairs(text): """Extract [(ip_canonical, port), ...] from bracketed IPv6:port forms.""" pairs = [] for raw_v6, port in IPv6BracketedPortPattern.findall(text): canonical = canonicalize_ip(raw_v6) if canonical is not None: pairs.append((canonical, port)) return pairsThen in
api.py:from .utils import ( IPPortPatternLine, - IPv6BracketedPortPattern, - canonicalize_ip, + find_bracketed_ipv6_pairs, log, ) @@ if isinstance(data, str): v4_pairs = IPPortPatternLine.findall(data) - v6_pairs = [] - for raw_v6, port in IPv6BracketedPortPattern.findall(data): - canonical = canonicalize_ip(raw_v6) - if canonical is not None: - v6_pairs.append((canonical, port)) - data = v4_pairs + v6_pairs + data = v4_pairs + find_bracketed_ipv6_pairs(data)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxybroker/api.py` around lines 382 - 388, Extract the bracketed-IPv6 parsing loop from _load and move it into a new helper like find_bracketed_ipv6_pairs(text) in utils, which should use IPv6BracketedPortPattern and canonicalize_ip to return [(ip_canonical, port), ...]; then in _load replace the inline loop with a call to that helper and combine its result with IPPortPatternLine.findall(data) (i.e. v4_pairs + find_bracketed_ipv6_pairs(data)); keep IPPortPatternLine, IPv6BracketedPortPattern, canonicalize_ip and find_proxy_pairs behavior unchanged.proxybroker/utils.py (1)
41-47: 💤 Low value
IPv6BracketedPortPatterncannot capture zone IDs — minor consistency gap.The character class
[0-9A-Fa-f:.%]+only allows hex digits, colon, dot, and percent. So a bracketed link-local with a zone ID like[fe80::1%eth0]:8080will not match (t,h, etc. are outside[a-f]). The unbracketed_IPV6_CANDIDATE_PATTERNon line 27 does support zone IDs via(?:%[A-Za-z0-9_.\-]+)?, andcanonicalize_ip's docstring (line 73) explicitly advertises preserving them, so the two paths disagree.In practice, zone-IDs in published proxy feeds are rare, but if you want symmetry with the unbracketed tokenizer the inner class can be widened.
♻️ Optional widening for symmetry
-IPv6BracketedPortPattern = re.compile(r"\[([0-9A-Fa-f:.%]+)\]:(\d{2,5})") +IPv6BracketedPortPattern = re.compile( + r"\[([0-9A-Fa-f:.]+(?:%[A-Za-z0-9_.\-]+)?)\]:(\d{2,5})" +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxybroker/utils.py` around lines 41 - 47, IPv6BracketedPortPattern currently disallows zone ID characters, causing bracketed addresses like [fe80::1%eth0]:8080 to not match; update the regex for IPv6BracketedPortPattern so its inner capture mirrors the unbracketed _IPV6_CANDIDATE_PATTERN by allowing zone ID characters after a percent (i.e., include A-Za-z0-9_.- or the equivalent %... sequence inside the bracketed character class), ensuring canonicalize_ip's promise to preserve zone IDs is honored for bracketed addresses too.proxybroker/providers.py (1)
166-178: ⚡ Quick winConsider using the
find_proxy_pairshelper for consistency.The manual extraction loop duplicates logic available in the
find_proxy_pairshelper. As per coding guidelines, provider parsing should usefind_proxy_pairs(text)which returns[(ip_canonical, port), ...]for both IPv4 and IPv6, already handling canonicalization and validation.♻️ Refactor to use find_proxy_pairs helper
+from .utils import ( + find_proxy_pairs, + get_headers, + log, +) + def _find_proxies(self, page): - # Default IPv4 extraction via the legacy pattern (subclasses may - # override `_pattern` for source-specific HTML quirks). IPv6 - # entries in `[v6]:port` form are extracted in addition - they - # are validated and canonicalised via stdlib `ipaddress` so - # invalid bracketed garbage is silently dropped rather than - # surfacing as proxy candidates. - proxies = list(self._pattern.findall(page)) - for raw_v6, port in IPv6BracketedPortPattern.findall(page): - canonical = canonicalize_ip(raw_v6) - if canonical is not None: - proxies.append((canonical, port)) - return proxies + # Use find_proxy_pairs which handles both IPv4 and IPv6 extraction + # with canonicalization. Subclasses can still override this method + # for source-specific HTML quirks. + return find_proxy_pairs(page)Based on coding guidelines: "Parse IPv6 provider feeds using
IPv6BracketedPortPattern+ stdlib validation, and use helperfind_proxy_pairs(text)to return[(ip_canonical, port), ...]for both v4 and v6"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxybroker/providers.py` around lines 166 - 178, Replace the manual IPv6 extraction loop in _find_proxies with the shared helper: remove the IPv6BracketedPortPattern loop and the extra canonicalize_ip logic and instead return list(find_proxy_pairs(page)), ensuring you no longer rely on manual _pattern.findall(page) + post-processing; use find_proxy_pairs(page) (which returns [(ip_canonical, port), ...] for both IPv4 and IPv6) to keep canonicalization/validation consistent across providers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_negotiators.py`:
- Around line 167-186: The test test_socks4_ipv6_raises_helpful_error uses
asyncio.get_event_loop().run_until_complete which is incompatible with Python
3.14; change the test to an async test by adding `@pytest.mark.asyncio` and making
it async def, then await ngtr.negotiate(ip="2001:db8::1", port=443) directly
(keeping the same exception expectation for Socks4Ngtr.negotiat e), so the test
uses the same async pattern as the other tests.
---
Nitpick comments:
In `@proxybroker/api.py`:
- Around line 382-388: Extract the bracketed-IPv6 parsing loop from _load and
move it into a new helper like find_bracketed_ipv6_pairs(text) in utils, which
should use IPv6BracketedPortPattern and canonicalize_ip to return
[(ip_canonical, port), ...]; then in _load replace the inline loop with a call
to that helper and combine its result with IPPortPatternLine.findall(data) (i.e.
v4_pairs + find_bracketed_ipv6_pairs(data)); keep IPPortPatternLine,
IPv6BracketedPortPattern, canonicalize_ip and find_proxy_pairs behavior
unchanged.
In `@proxybroker/providers.py`:
- Around line 166-178: Replace the manual IPv6 extraction loop in _find_proxies
with the shared helper: remove the IPv6BracketedPortPattern loop and the extra
canonicalize_ip logic and instead return list(find_proxy_pairs(page)), ensuring
you no longer rely on manual _pattern.findall(page) + post-processing; use
find_proxy_pairs(page) (which returns [(ip_canonical, port), ...] for both IPv4
and IPv6) to keep canonicalization/validation consistent across providers.
In `@proxybroker/utils.py`:
- Around line 41-47: IPv6BracketedPortPattern currently disallows zone ID
characters, causing bracketed addresses like [fe80::1%eth0]:8080 to not match;
update the regex for IPv6BracketedPortPattern so its inner capture mirrors the
unbracketed _IPV6_CANDIDATE_PATTERN by allowing zone ID characters after a
percent (i.e., include A-Za-z0-9_.- or the equivalent %... sequence inside the
bracketed character class), ensuring canonicalize_ip's promise to preserve zone
IDs is honored for bracketed addresses too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b4b8bf85-5244-4197-8e01-09e75aaf05ea
📒 Files selected for processing (15)
CHANGELOG.mdCLAUDE.mdproxybroker/api.pyproxybroker/checker.pyproxybroker/judge.pyproxybroker/negotiators.pyproxybroker/providers.pyproxybroker/proxy.pyproxybroker/resolver.pyproxybroker/utils.pytests/test_checker.pytests/test_negotiators.pytests/test_proxy.pytests/test_resolver.pytests/test_utils.py
Round 2 of #201: addresses three real IPv6 gaps that the first commit left unfixed, plus configures security-scanner ignores so the 44 phantom findings (all from stale local venvs and scan-output artifacts) stop polluting the signal. IPv6 gaps fixed: - HTTP CONNECT request format (negotiators._CONNECT_request): produced "CONNECT 2001:db8::1:443 HTTP/1.1" for IPv6 hosts - ambiguous and rejected by standards-compliant proxies. Now brackets the authority and Host header per RFC 9112 § 3.2.3 and RFC 9110 § 7.2: "CONNECT [2001:db8::1]:443 HTTP/1.1" with "Host: [2001:db8::1]". Affects HTTPS, CONNECT:80, CONNECT:25 negotiators (all share _CONNECT_request). - DNS resolution AAAA fallback (Resolver.resolve): qtype defaulted to "A" with no AAAA path, so hostnames with only AAAA records (no A) silently failed to resolve. Now falls back to AAAA when the caller didn't pin qtype - making v6-only judges and v6-only proxy hosts reachable end-to-end. If both A and AAAA fail, ResolveError still propagates (preserves the test_resolve "256.0.0.1 raises" contract). - Tests for both gaps (4 new): bracketed CONNECT for IPv4 vs IPv6 across CONNECT:80/25/HTTPS:443 ports; AAAA fallback when A raises ResolveError. Scan-noise hygiene: The pre-push opsera scan reported 44 findings (1 critical, 43 high) that I initially deferred as "pre-existing in committed code". Closer inspection shows ALL 44 are actually in untracked local artifacts: - 38 findings in stale local virtualenvs (`venv/`, `venv_py313_test/`) that were last touched May 2025 and contain superseded dependency metadata (aiohttp 3.10, urllib3 1.26, cryptography 37, etc.). The active project pins `aiohttp ^3.12`, etc. via pyproject.toml. - 6 gitleaks AWS-key matches in `.claude/settings.local.json` (Claude Code per-machine config) and `tmp/gitleaks-report.json` (the scan output itself - recursive false positive). None are in committed code. The fix is to keep scanners from looking at this noise so future runs surface real issues: - .gitignore: add `tmp/`, `.claude/`, `venv/`, `venv_*/`, `*-report.json`, `*-stderr.txt`. (`.venv` and `.env` were already covered.) - .semgrepignore (new): same scope - semgrep doesn't read .gitignore by default, so it needs its own list. The recursive AWS-key false positive (semgrep scanning gitleaks' own output) is the most acute case. Out of scope for this PR (intentionally separated, not deferred): - Stale local venvs (`venv/`, `venv_py313_test/`) are now gitignored; cleaning them up is a user-local task, not a repo change. - Hadolint DL3008 (Dockerfile apt pin) is one line in committed code; pinning Debian apt versions breaks `apt-get upgrade` and is industry-accepted as won't-fix on slim base images. - Real dependency CVEs (h11, urllib3, etc.) in transitive deps: if any actually apply to the runtime env, Dependabot's existing monthly grouped updates (.github/dependabot.yml from PR #207) will surface them - tracked there, not here. Verification: 243 pytest pass (+4 new gap tests on top of round 1's +32). Pre-commit hooks green. The 3 pre-existing test_resolver event_loop fixture errors remain unrelated. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Gitleaks doesn't read .gitignore, so the .gitignore entries from the prior commit didn't quiet it. Adds .gitleaks.toml with an explicit allowlist for the same paths covered by .semgrepignore: .claude/, tmp/, .venv/, venv/, venv_*/. Result of the cleanup pass (#212): pre-push opsera scan critical+high findings dropped from 22 to 0 across the source tree. The lone remaining hadolint DL3008 in Dockerfile is one line of pre-existing lint, unrelated to this PR; pinning Debian apt versions conflicts with the `apt-get upgrade` line and is industry-accepted as won't-fix on slim base images. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82360bf9f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/test_negotiators.py (1)
198-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
asyncio.get_event_loop().run_until_completeis deprecated and breaks on Python 3.14.This pattern raises
RuntimeErroron Python 3.14 when no loop is running, so the test fails on the supported matrix. The other tests in this file already use@pytest.mark.asyncio+async def— adopt the same pattern here:Fix
- def test_socks4_ipv6_raises_helpful_error(self): + `@pytest.mark.asyncio` + async def test_socks4_ipv6_raises_helpful_error(self): """SOCKS4 RFC 1928 has no IPv6 address type. Asking for v6 over SOCKS4 should fail loudly, not silently truncate or crash with a cryptic struct error. """ - # The current implementation crashes with OSError from inet_aton. - # We document the current observable behavior here so we notice - # if it ever changes silently. A future tighter check could raise - # a domain-specific BadResponseError instead. - import asyncio from unittest.mock import MagicMock from proxybroker.negotiators import Socks4Ngtr mock_proxy = MagicMock() ngtr = Socks4Ngtr(mock_proxy) with pytest.raises((OSError, ValueError)): - asyncio.get_event_loop().run_until_complete( - ngtr.negotiate(ip="2001:db8::1", port=443) - ) + await ngtr.negotiate(ip="2001:db8::1", port=443)As per coding guidelines: "Use async/await patterns consistently with modern asyncio; avoid deprecated asyncio patterns".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_negotiators.py` around lines 198 - 217, Replace the deprecated asyncio.get_event_loop().run_until_complete usage in test_socks4_ipv6_raises_helpful_error by making the test an async function decorated with `@pytest.mark.asyncio` and directly await ngtr.negotiate(ip="2001:db8::1", port=443); keep the same assertion that it raises (OSError, ValueError) and retain the setup creating Socks4Ngtr(mock_proxy) so the test logic and exception expectations remain unchanged.
🧹 Nitpick comments (1)
tests/test_resolver.py (1)
113-137: ⚡ Quick winAvoid private-method coupling in fallback test.
This test currently patches
resolver._resolve, which couples it to internal implementation instead of externally observable behavior. Prefer mocking the DNS query boundary and asserting the final resolved value.Refactor direction
- async def fake_resolve(host, qtype): - a_calls.append(qtype) - if qtype == "A": - raise ResolveError - from types import SimpleNamespace - return [SimpleNamespace(host="2001:db8::abcd")] - - mocker.patch.object(resolver, "_resolve", side_effect=fake_resolve) + async def fake_query(_self, _host, qtype): + a_calls.append(qtype) + if qtype == "A": + raise asyncio.TimeoutError + from types import SimpleNamespace + return [SimpleNamespace(host="2001:db8::abcd")] + + mocker.patch("aiodns.DNSResolver.query", side_effect=fake_query)As per coding guidelines,
tests/**/*.py: Test behavior and user-visible outcomes, not implementation details; avoid testing internal method calls or private attributes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_resolver.py` around lines 113 - 137, The test is coupling to the private method resolver._resolve; change it to mock the external DNS query boundary that resolver.resolve uses (for example the aiodns.DNSResolver.query or the module/function that performs the actual DNS lookup in the resolver implementation) so you only assert externally observable behavior: call resolver.resolve("v6only.example.com") and assert it returns "2001:db8::abcd" while removing assertions about a_calls or explicit checks of resolver._resolve; keep the fake response to return an aiodns-style record (SimpleNamespace(host="2001:db8::abcd")) for the AAAA query so the fallback path is exercised without touching private methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@proxybroker/negotiators.py`:
- Around line 89-96: The reply-size logic in the SOCKS5 negotiation (around the
request/resp handling using variables request, atyp, resp and calls to
self._proxy.send / self._proxy.recv) assumes the server BND.ADDR length matches
the request ATYP; instead read the fixed 4-byte header first (VER, REP, RSV,
BND.ATYP), inspect the response ATYP from that header, then perform a second
recv for the BND.ADDR of the correct length (1 byte for domain len+name, 4 for
IPv4, 16 for IPv6) and finally recv the 2-byte BND.PORT; update the code that
currently computes reply_size and directly awaits recv(reply_size) to this
two-step recv based on the response ATYP so mismatched ATYPs do not
under/over-read.
In `@proxybroker/resolver.py`:
- Around line 159-160: The resolve() path currently returns and stores raw IP
string values (via host_is_ip(host) early-return and later when caching r.host),
risking non-canonical IPv6 forms; update resolve() so any time an IP string is
emitted or written to cache/state it is passed through
proxybroker.utils.canonicalize_ip() — specifically modify the early return in
resolve() where host_is_ip(host) is true to return canonicalize_ip(host) and
replace assignments/usages of r.host in the caching/storage block (the code
around the 185-199 area that writes r.host into cache/state) to store
canonicalize_ip(r.host) instead.
- Around line 149-183: The resolve method's A→AAAA fallback logic makes
Resolver.resolve() too complex; extract that branching into a small helper (e.g.
_resolve_with_fallback or _resolve_a_then_aaaa) that takes (host, qtype) and
encapsulates the try/except/conditional-fallback behavior using self._resolve,
returning the response or None and re-raising ResolveError when appropriate;
then simplify resolve() to call this helper, retaining the existing host_is_ip
check, _cached_hosts lookup, and caching behavior so functionality is unchanged
while cognitive complexity is reduced.
---
Duplicate comments:
In `@tests/test_negotiators.py`:
- Around line 198-217: Replace the deprecated
asyncio.get_event_loop().run_until_complete usage in
test_socks4_ipv6_raises_helpful_error by making the test an async function
decorated with `@pytest.mark.asyncio` and directly await
ngtr.negotiate(ip="2001:db8::1", port=443); keep the same assertion that it
raises (OSError, ValueError) and retain the setup creating
Socks4Ngtr(mock_proxy) so the test logic and exception expectations remain
unchanged.
---
Nitpick comments:
In `@tests/test_resolver.py`:
- Around line 113-137: The test is coupling to the private method
resolver._resolve; change it to mock the external DNS query boundary that
resolver.resolve uses (for example the aiodns.DNSResolver.query or the
module/function that performs the actual DNS lookup in the resolver
implementation) so you only assert externally observable behavior: call
resolver.resolve("v6only.example.com") and assert it returns "2001:db8::abcd"
while removing assertions about a_calls or explicit checks of resolver._resolve;
keep the fake response to return an aiodns-style record
(SimpleNamespace(host="2001:db8::abcd")) for the AAAA query so the fallback path
is exercised without touching private methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b7b1b26-f636-4abe-a09e-81b6bd93b659
📒 Files selected for processing (7)
.gitignore.gitleaks.toml.semgrepignoreproxybroker/negotiators.pyproxybroker/resolver.pytests/test_negotiators.pytests/test_resolver.py
✅ Files skipped from review due to trivial changes (3)
- .semgrepignore
- .gitleaks.toml
- .gitignore
Round 3 of #212. Pushes the IPv6 work from "standards-compliant" to SOTA by closing the gaps I'd called out in self-assessment as "not SOTA". Filed #213 for the remaining architectural items (connection-layer Happy Eyeballs, DNSSEC, IDN) as separate work. Happy Eyeballs DNS (RFC 8305 § 3): - Replaces sequential A→AAAA fallback with parallel race in Resolver.resolve(). Both queries fire concurrently; first non-empty answer wins; slower task is cancelled. If both fail, the most recent error propagates (preserves the legacy "could not resolve" contract). - Sequential fallback added a full DNS round-trip of latency for v6-only hostnames. Parallel race makes v4-only and v6-only hosts resolve at roughly the same speed and shaves the worst-case latency for dual-stack hosts on broken networks. - 4 new tests: v4 wins when faster, v6 wins when faster, v6-only resolves when A raises, both-fail propagates ResolveError. - Existing test_resolve_family + test_create_by_domain updated to account for the parallel A+AAAA query (mock now returns A, raises on AAAA so v4 wins deterministically). Error UX: - Socks4Ngtr.negotiate(ip=v6) raises BadResponseError("SOCKS4 protocol does not support IPv6 destinations") instead of cryptic OSError from inet_aton. Logs point users at SOCKS5 for IPv6. SOCKS4 spec only defines a 4-byte IPv4 address field - this isn't a bug, it's a protocol limitation that should be communicated clearly. API consistency: - find_proxy_pairs(text) now canonicalises both IPv4 AND IPv6 entries. IPv4 canonical form equals identity, so legacy v4-only feeds see no behavior change - the contract is just consistent: every returned (ip, port) has a canonical IP, regardless of family or source feed encoding. Type hints (PEP 604): - canonicalize_ip(s: str | None) -> str | None - find_proxy_pairs(text: str) -> list[tuple[str, str]] - _format_host_port(host: str, port: int | str) -> str Verification: - pytest: 246 pass (+5 new tests since round 2). Pre-existing test_resolver event_loop fixture errors unchanged. - Pre-push opsera scan: 0 critical/high across all 5 scanners. - Docker smoke (in-container): find_proxy_pairs canonical v4+v6 ✓; SOCKS4 v6 → BadResponseError with clear message ✓. - IPv4 regression check: docker run find --types HTTP --limit 2 returned 2 working real proxies (US, KR) - no v4 path regression. Out of scope (filed as #213): connection-layer Happy Eyeballs (RFC 8305 § 5), DNSSEC opt-in via aiodns, IDN/Punycode handling. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
uv.lock was accidentally committed in fe1ef08 by `git add -A` picking up the locally-generated lockfile from my uv-managed dev venv. The project uses Poetry (with poetry.lock checked in) as the canonical dependency lockfile - committing both creates ambiguity for contributors about which is authoritative. Untrack from git and add to .gitignore so future `uv sync`/`uv pip install` operations won't re-add it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5049fdab9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
proxybroker/resolver.py (1)
164-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize
resolve()outputs before returning or caching them.Direct IP inputs and DNS answers still leave
resolve()as raw text, so callers can get non-RFC-5952 variants even though the rest of the stack now compares canonicalized IPs. Run both the early-return path and eachr.hostthroughcanonicalize_ip(...)before storing or returning them.Suggested fix
if self.host_is_ip(host): - return host + return canonicalize_ip(host) or host @@ if resp: hosts = [ { "hostname": host, - "host": r.host, + "host": canonicalize_ip(r.host) or r.host, "port": port, "family": family, "proto": socket.IPPROTO_IP, "flags": socket.AI_NUMERICHOST, }As per coding guidelines,
**/*.py: Canonicalize IPv6 addresses using RFC 5952 format at every text-to-comparison boundary, and useproxybroker.utils.canonicalize_ip(s) -> str | Nonewhen IPs cross from text into comparison contexts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxybroker/resolver.py` around lines 164 - 191, The resolve() implementation must canonicalize IP text before returning or caching: when host_is_ip(host) returns True, pass host through proxybroker.utils.canonicalize_ip(host) and return that canonical value (or None-handled), and when building hosts from resp (from _race_a_aaaa or _resolve) run each r.host through canonicalize_ip before placing it into the hosts entries and before storing into self._cached_hosts; ensure you handle canonicalize_ip returning None (skip or fallback to original) and keep caching logic for family vs single-host consistent with the existing _cached_hosts usage.proxybroker/negotiators.py (1)
93-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse the SOCKS5 reply from the response
ATYP, not the requestATYP.
reply_size = 22 if atyp == 0x04 else 10assumes the proxy binds with the same address family the client requested. That is not guaranteed, so a dual-stack proxy can legally return a shorter or longerBND.ADDRhere. In that case this read will under/over-read the socket and can stall the negotiation.Suggested fix
await self._proxy.send(request) - # Reply size: VER(1)+REP(1)+RSV(1)+ATYP(1)+BND.ADDR(4 or 16)+BND.PORT(2) - reply_size = 22 if atyp == 0x04 else 10 - resp = await self._proxy.recv(reply_size) - - if resp[0] != 0x05 or resp[1] != 0x00: + header = await self._proxy.recv(4) + if header[0] != 0x05 or header[1] != 0x00: self._proxy.log("Failed (invalid data)", err=BadResponseError) raise BadResponseError - else: - self._proxy.log("Request is granted") + rep_atyp = header[3] + if rep_atyp == 0x01: + addr_len = 4 + elif rep_atyp == 0x04: + addr_len = 16 + elif rep_atyp == 0x03: + length = await self._proxy.recv(1) + addr_len = length[0] + else: + self._proxy.log("Failed (invalid data)", err=BadResponseError) + raise BadResponseError + + await self._proxy.recv(addr_len + 2) + self._proxy.log("Request is granted")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proxybroker/negotiators.py` around lines 93 - 96, The code currently uses the request variable atyp to compute reply_size and does a single recv that can under/over-read; change the logic in the SOCKS5 negotiation (around self._proxy.send and self._proxy.recv) to first read the fixed 4-byte reply header (VER, REP, RSV, ATYP) from the proxy, extract the response ATYP (not the request atyp), then compute the remaining payload length (BND.ADDR length = 4 for IPv4, 16 for IPv6, or variable for domain) and do a second recv for the rest of the reply; update variable names (e.g., resp_header vs resp_body) and ensure you use the response ATYP to determine reply_size rather than the original atyp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 60-63: Update the changelog bullet about Socks5Ngtr to correct the
RFC/reference: change the clause saying “SOCKS4 stays IPv4-only by spec (RFC
1928 has no v6 ATYP)” to a correct phrasing such as “SOCKS4 remains IPv4-only by
design/spec” or cite the SOCKS4 specification instead; keep the rest of the line
about Socks5Ngtr emitting ATYP=0x04 + 16-byte IPv6 addresses and mention that
IPv4 remains ATYP=0x01 + 4-byte, and reference the symbol Socks5Ngtr and the
ATYP values (0x04, 0x01) so the revised sentence replaces the incorrect RFC 1928
claim.
- Around line 10-80: Merge the duplicate "### Added" and "### Changed" sections
under "Unreleased" into one "Added" block and one "Changed" block respectively:
move all bullet points referencing symbols like Resolver.resolve(),
canonicalize_ip, find_proxy_pairs, Socks4Ngtr.negotiate, Resolver.host_is_ip,
Resolver.get_real_ext_ip, proxybroker/utils.py (canonicalize_ip, get_all_ip),
IPv6BracketedPortPattern, find_proxy_pairs, Provider._find_proxies,
Broker._load, _get_anonymity_lvl, Judge.check, Proxy.as_text()/__repr()/log,
Socks5Ngtr, IPv6Pattern, and _IPV6_CANDIDATE_PATTERN into the appropriate
consolidated heading so there is exactly one "### Added" and one "### Changed"
subsection; preserve original bullet order and text, remove the extra heading
lines, and ensure headings follow the Keep a Changelog structure.
---
Duplicate comments:
In `@proxybroker/negotiators.py`:
- Around line 93-96: The code currently uses the request variable atyp to
compute reply_size and does a single recv that can under/over-read; change the
logic in the SOCKS5 negotiation (around self._proxy.send and self._proxy.recv)
to first read the fixed 4-byte reply header (VER, REP, RSV, ATYP) from the
proxy, extract the response ATYP (not the request atyp), then compute the
remaining payload length (BND.ADDR length = 4 for IPv4, 16 for IPv6, or variable
for domain) and do a second recv for the rest of the reply; update variable
names (e.g., resp_header vs resp_body) and ensure you use the response ATYP to
determine reply_size rather than the original atyp.
In `@proxybroker/resolver.py`:
- Around line 164-191: The resolve() implementation must canonicalize IP text
before returning or caching: when host_is_ip(host) returns True, pass host
through proxybroker.utils.canonicalize_ip(host) and return that canonical value
(or None-handled), and when building hosts from resp (from _race_a_aaaa or
_resolve) run each r.host through canonicalize_ip before placing it into the
hosts entries and before storing into self._cached_hosts; ensure you handle
canonicalize_ip returning None (skip or fallback to original) and keep caching
logic for family vs single-host consistent with the existing _cached_hosts
usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 70cf3014-bf7b-405f-b942-c581e3cc1dac
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
CHANGELOG.mdproxybroker/negotiators.pyproxybroker/proxy.pyproxybroker/resolver.pyproxybroker/utils.pytests/test_negotiators.pytests/test_proxy.pytests/test_resolver.py
🚧 Files skipped from review as they are similar to previous changes (3)
- proxybroker/proxy.py
- tests/test_proxy.py
- proxybroker/utils.py
This was referenced May 9, 2026
…ix SOCKS5 reply parsing Addresses 3 actionable items from coderabbitai review: 1. Resolver.resolve() canonicalises both the IP-literal early-return path and DNS-returned r.host values via canonicalize_ip(). Without this, callers downstream of resolve() could see non-canonical IP strings (e.g. uppercase 2001:DB8::1 instead of 2001:db8::1), breaking the canonical-form contract that the rest of the stack relies on for set-membership comparison. 2. Socks5Ngtr now reads the SOCKS5 connect-reply in two stages: first the fixed 4-byte header (VER+REP+RSV+ATYP), then the variable-length BND.ADDR + BND.PORT sized from the *response* ATYP (not the request ATYP). RFC 1928 § 6 explicitly allows dual-stack proxies to bind a different address family than the client requested. The old code derived reply_size from the request atyp, so a v4 client request that the proxy bound to v6 (or vice versa) would under/over-read the socket and stall negotiation. Domain-form ATYP=0x03 also handled with its 1-byte length prefix. 3. tests/test_negotiators.py SOCKS5 mocks updated to provide the new 3-stage recv sequence (greeting, header, body). Added explicit regression test test_socks5_dual_stack_proxy_returns_v6_bnd_for_v4_request that exercises the family-mismatch case the old code couldn't handle. Plus: - tests/test_proxy.py: drop unused future_iter import (CI ruff lint failure on F401 across all 5 Python matrix cells). - 10 SonarCloud Security Hotspots (S1313 hardcoded-IP) marked SAFE with rationale: all are RFC 3849/5737 documentation-range addresses used as test data; never routed in production. Verification: - pytest: 247 pass (+1 dual-stack regression test). 3 pre-existing test_resolver event_loop fixture errors unchanged (tracked in #214). - Ruff: clean. - Pre-push opsera scan: 0 critical/high. Also merges latest master (PR #211 GeoIP RuntimeError test integrated without conflict). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
CI revealed test_resolve_cache was failing with: RuntimeError: coroutine raised StopIteration Locally it errors on event_loop fixture per #214 (different pytest-asyncio plugin behavior between local and CI), so I missed it when fixing the same pattern in test_resolve_family / test_create_by_domain. Root cause: my Happy Eyeballs DNS change makes Resolver.resolve() fire A AND AAAA queries in parallel via _race_a_aaaa(). The test mocked aiodns.DNSResolver.query with future_iter([result1], [result2]) - 2 results - but two resolve() calls now consume 4 query calls (2 hosts x 2 families). Iterator exhaustion on the 3rd call surfaces in CI as StopIteration wrapped in RuntimeError. Fix: switch to query_side_effect callable that returns the v4 future for A queries and raises aiodns.error.DNSError for AAAA, matching the pattern already used in test_resolve_family / test_create_by_domain. This deterministically lets v4 win regardless of timing while keeping _resolve.call_count == 2 (one per resolve() call). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13f11b699e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Real CI failure (the 'event_loop fixture not found' in local was masking this): assertion `_resolve.call_count == 2` is wrong now that Happy Eyeballs DNS fires both A and AAAA per resolve() call. Each fresh resolve makes 2 underlying _resolve invocations. Updated assertions: - after 2 fresh resolves: 4 (was 2) - after 2 additional cached resolves: still 4 (cache short-circuits) - after 1 failed resolve: 6 (failed resolve still fires both families) This is the same Happy Eyeballs DNS behavior the other tests already account for; test_resolve_cache was missed in the earlier mock fixes because locally it errored on the unrelated event_loop fixture issue (#214) instead of running. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc8f90dc86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Same Happy Eyeballs DNS pattern as test_resolve_cache, test_resolve_family, test_create_by_domain. The single-result future_iter mock gets exhausted on the second query call (A→AAAA parallel race) and surfaces in CI as RuntimeError: coroutine raised StopIteration. Fix: explicit query_side_effect that returns the v4 future for A queries and raises aiodns.error.DNSError for AAAA, so v4 wins deterministically. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4866d201e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three test functions in test_resolver.py declared `event_loop` as a fixture argument but never used it inside the body. The arg dates back to 2017 (commit d560297) when pytest-asyncio's `event_loop` fixture needed to be requested explicitly. Modern pytest-asyncio (the project is on 0.26.0) handles loop setup automatically via @pytest.mark.asyncio + asyncio_default_fixture_loop_scope, so the arg is not just unused — it's actively wrong: pytest-asyncio 0.23+ removed the auto-provided fixture, and the resulting "fixture 'event_loop' not found" error silently skipped these 3 tests in some environments. Closes #214. Why this is in PR #212: This bug masked the real Happy Eyeballs DNS test breakage during rounds 4-6 of #212. Each push revealed ONE more test that mocked aiodns.DNSResolver.query with future_iter([single_result]) — the parallel A+AAAA race exhausts the iterator on the second call. With the event_loop arg present, those 3 tests errored locally before they could surface the real bug, so I shipped 3 round-trips instead of catching everything in one local run. After this fix, local `uv run pytest tests/` reports `251 passed, 0 errors` — the same view as CI. The next contributor working in this area gets accurate signal locally instead of having to push and wait for the CI matrix to find their breakage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
Addresses 7 still-open review items from inline coderabbit + codex comments on PR #212 that haven't been resolved: CHANGELOG.md: - Collapse the two `### Added` and two `### Changed` blocks under `[Unreleased]` into one each (coderabbit; markdownlint MD024). - Fix the SOCKS RFC reference: SOCKS4 isn't from RFC 1928 (that's the SOCKS5 spec which DEFINES IPv6 ATYP=0x04). Reworded to "the SOCKS4 protocol predates IPv6 and only defines a 4-byte address field" (coderabbit). proxybroker/utils.py: - Strip trailing `.` from IPv6 candidate tokens before validation so pages like "Real IP: 2001:db8::1." still surface the address (codex). The dot is in the tokenizer character class, but ipaddress.ip_address rejects the trailing dot, so the address was silently dropped. - Extend `IPv6BracketedPortPattern` to allow zone-id alphanumeric chars (`a-zA-Z`, `_-`) inside brackets so `[fe80::1%eth0]:8080` parses (codex). canonicalize_ip + host_is_ip already accept zone IDs; the pattern was the bottleneck. proxybroker/api.py: - Mask bracketed IPv6 spans before running `IPPortPatternLine.findall` on raw input so IPv4-mapped IPv6 entries like `[::ffff:1.2.3.4]:8080` don't ALSO produce a phantom `1.2.3.4:8080` IPv4 entry (codex). proxybroker/providers.py: - Move IPv6 bracketed-pair extraction from the base `_find_proxies` helper to the public `Provider.find_proxies` method (codex). Subclasses like `Proxy_list_org.find_proxies` and `Free_proxy_cz` reuse `_find_proxies()` as a raw-regex helper and pass each match to `b64decode(hp)` / similar; the (host, port) tuple shape from v6 extraction broke their parsing. Default `find_proxies` still gets v6; subclasses with custom decoding aren't polluted. proxybroker/negotiators.py: - `_CONNECT_request` strips caller-supplied brackets first so values from `urlparse('https://[2001:db8::1]/').netloc` (already bracketed) don't get double-bracketed into invalid `CONNECT [[2001:db8::1]]:443` (codex). proxybroker/resolver.py: - Introduce a `_QTYPE_DEFAULT` sentinel so `resolve()` can distinguish "caller didn't pin a query type" from "caller explicitly asked for qtype='A'" (codex). Happy Eyeballs only fires on the unpinned path; callers passing `qtype="A"` or `family=socket.AF_INET[/INET6]` get the single-family path back, preserving the legacy contract that AF_INET requests never return v6 addresses. - Extract the query-strategy decision into `_fetch_host_records` to drop `resolve()` cognitive complexity below SonarCloud's threshold (coderabbit). Tests: - 5 new tests covering the 5 source fixes (trailing-dot, zone-id in bracketed, mask brackets in api.py, _CONNECT bracketed-host unbrackets, qtype-explicit-A doesn't race). - test_resolve_cache assertion updated to 3 (was 4): with the sentinel fix, family=AF_INET is single-query, not parallel race, so 1 fresh resolve + 1 family-pinned resolve = 2 + 1 = 3 _resolve calls. Verification: - pytest: 256 pass (+5 since round 7). 0 errors locally — full match with CI now that #214 is folded in. - Ruff: clean. - Pre-push opsera scan: 0 critical/high. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review Round 8 push (commit 09807a6) addresses all 7 still-open inline suggestions from prior rounds: CHANGELOG.md
proxybroker/utils.py
proxybroker/api.py
proxybroker/providers.py
proxybroker/negotiators.py
proxybroker/resolver.py
Tests: +5 covering the new behaviors. 256 pass, ruff clean, opsera scan 0 critical/high. The earlier |
|
Noted on the ✅ Actions performedReview triggered.
|
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09807a6790
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_resolver.py (2)
63-66: ⚡ Quick winThis test currently mocks the method it claims to validate.
test_get_real_ext_ippatchesresolver.get_real_ext_ipand then asserts the patched value, so it can’t catch regressions in real behavior. Consider removing it or replacing it with an observable contract case (like the canonicalization test below).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_resolver.py` around lines 63 - 66, The test test_get_real_ext_ip incorrectly patches the very function under test (resolver.get_real_ext_ip), so it never validates real behavior; remove the patch and either call the real async resolver.get_real_ext_ip (optionally with a small aiohttp-responses or requests-mocking fixture) to assert the actual IP/canonicalization behavior, or replace the test with a contract-style check that exercises canonicalization logic (e.g., feed a known external-response into the resolver's HTTP client and assert the normalized IP result). Locate test_get_real_ext_ip and resolver.get_real_ext_ip to implement the change and ensure the test validates real logic rather than a mocked return.
125-330: 🏗️ Heavy liftReduce coupling to private resolver internals in race/cache tests.
A lot of these tests assert
_resolvecall counts and mutate_cached_hostsdirectly. That overfits to internals and makes refactors brittle. Prefer asserting externally visible outcomes (resolved value/error behavior) with collaborator fakes instead.As per coding guidelines,
**/test_*.py: Test behavior, not implementation; focus on user-visible outcomes and don't test internal method calls or private attributes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_resolver.py` around lines 125 - 330, The tests currently couple to private internals by clearing resolver._cached_hosts and asserting resolver._resolve.call_count; change them to exercise only the public API: stop mutating _cached_hosts directly and instead create a fresh resolver fixture or use whatever public cache-clear method exists (or re-instantiate resolver) before each case, remove assertions that check resolver._resolve.call_count and any spy on _resolve, and assert the externally visible results from resolver.resolve (returned address lists, exceptions, and cached-return behavior via repeated resolver.resolve calls) while keeping the existing aiodns.DNSResolver.query fakes to control DNS outcomes.tests/test_negotiators.py (1)
100-137: 🏗️ Heavy liftAvoid asserting private helper
_CONNECT_requestdirectly.These tests are tightly coupled to a private function, which makes refactors noisy. Prefer driving the same assertions through public negotiator contracts (e.g., via
NGTRS[...]+ sent request bytes).As per coding guidelines,
**/test_*.py: Test behavior, not implementation; focus on user-visible outcomes and don't test internal method calls or private attributes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_negotiators.py` around lines 100 - 137, The tests call the private helper _CONNECT_request directly; instead, drive the same assertions through the public negotiator contract by using the NGTRS mapping to obtain the appropriate negotiator class, instantiate it (or use its public API) and generate the outgoing request bytes via the negotiator's public send/build method, then assert the Host/authority and bracket behavior on those bytes; remove direct references to _CONNECT_request and replace with calls to NGTRS[...] (the public negotiator) to make the tests validate observable behavior rather than the private helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 27-31: The changelog mentions that IPv6 parsing was wired into
Provider._find_proxies but the shipped API uses Provider.find_proxies; update
the CHANGELOG entry to replace the stale symbol Provider._find_proxies with
Provider.find_proxies and confirm other referenced symbols still match
(IPv6BracketedPortPattern, find_proxy_pairs, Broker._load) so the description
accurately reflects that IPv6 bracketed-port extraction is integrated via
Provider.find_proxies and Broker._load.
---
Nitpick comments:
In `@tests/test_negotiators.py`:
- Around line 100-137: The tests call the private helper _CONNECT_request
directly; instead, drive the same assertions through the public negotiator
contract by using the NGTRS mapping to obtain the appropriate negotiator class,
instantiate it (or use its public API) and generate the outgoing request bytes
via the negotiator's public send/build method, then assert the Host/authority
and bracket behavior on those bytes; remove direct references to
_CONNECT_request and replace with calls to NGTRS[...] (the public negotiator) to
make the tests validate observable behavior rather than the private helper.
In `@tests/test_resolver.py`:
- Around line 63-66: The test test_get_real_ext_ip incorrectly patches the very
function under test (resolver.get_real_ext_ip), so it never validates real
behavior; remove the patch and either call the real async
resolver.get_real_ext_ip (optionally with a small aiohttp-responses or
requests-mocking fixture) to assert the actual IP/canonicalization behavior, or
replace the test with a contract-style check that exercises canonicalization
logic (e.g., feed a known external-response into the resolver's HTTP client and
assert the normalized IP result). Locate test_get_real_ext_ip and
resolver.get_real_ext_ip to implement the change and ensure the test validates
real logic rather than a mocked return.
- Around line 125-330: The tests currently couple to private internals by
clearing resolver._cached_hosts and asserting resolver._resolve.call_count;
change them to exercise only the public API: stop mutating _cached_hosts
directly and instead create a fresh resolver fixture or use whatever public
cache-clear method exists (or re-instantiate resolver) before each case, remove
assertions that check resolver._resolve.call_count and any spy on _resolve, and
assert the externally visible results from resolver.resolve (returned address
lists, exceptions, and cached-return behavior via repeated resolver.resolve
calls) while keeping the existing aiodns.DNSResolver.query fakes to control DNS
outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7324a701-26db-4138-9c3d-c2d9696c5e0c
📒 Files selected for processing (10)
CHANGELOG.mdproxybroker/api.pyproxybroker/negotiators.pyproxybroker/providers.pyproxybroker/resolver.pyproxybroker/utils.pytests/test_negotiators.pytests/test_proxy.pytests/test_resolver.pytests/test_utils.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (6)
- proxybroker/providers.py
- proxybroker/api.py
- proxybroker/resolver.py
- proxybroker/negotiators.py
- tests/test_proxy.py
- proxybroker/utils.py
Addresses 2 NEW review items from coderabbit on round 8:
resolver.py: cache family validation (resolver.py:189)
- Bug: prior default Happy-Eyeballs lookup caches "host -> v6". Later
caller pinning family=AF_INET or qtype="A" hits the cache and gets
the v6 back, violating the A-only contract.
- Fix: new `_cache_compatible(cached, family, qtype)` validates that
the cached IP's address family matches the caller's intent. Cache
entries from default lookups still serve future default lookups
(any-family path); pinned-family lookups only short-circuit on a
cached entry of the matching family.
- Test: pre-populate v6 in cache, request family=AF_INET, assert
fresh v4 lookup (not cached v6). Then request default, assert v6
cache hit still works.
providers.py: mask v6 brackets before IPv4 regex (providers.py:169)
- Bug: same phantom-IPv4 issue I fixed in api.py (raw `data` loader)
also applies to the default Provider.find_proxies path. A page
containing `[::ffff:192.0.2.1]:8080` produced both the v6 entry
AND a phantom `192.0.2.1:8080` IPv4 entry that the feed never
advertised.
- Fix: mask bracketed v6 spans with spaces before running
`_find_proxies(masked)`, then extract v6 entries from the
unmasked page.
- Tests: explicit assertion that v4-mapped v6 produces ONE entry,
and a mixed v4/v6 page produces both forms cleanly.
The codex resolver.py:43 ("Discover the IPv6 external address
deterministically") concern is intentionally deferred — making
ip_host selection deterministic vs randomized is a design question
(round-robin? prefer dual-stack? probe both v4 and v6 separately?).
That's its own ticket, not this PR.
Verification:
- pytest: 259 pass (+3 since round 8). 0 errors.
- Ruff: clean.
- Pre-push gitleaks: 0 findings.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
Coderabbit caught a doc drift: round 9 actually wires v6 extraction into the public `Provider.find_proxies`, not the lower-level `_find_proxies` raw-regex helper. The CHANGELOG entry still said the old version. Updated to match the shipped API surface, including the rationale for keeping v6 out of `_find_proxies` (subclasses that pipe its output through `b64decode`/custom decoders). No code change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No IssuesAll SAST findings were determined to be false positives, test code, or non-actionable. No real security issues require attention. fossabot analyzed this PR using SAST security analysis (changed files only). |
bluet
added a commit
that referenced
this pull request
May 9, 2026
Bump version 2.0.0b2 -> 2.0.0b3 and move the IPv6 epic content from [Unreleased] into the [2.0.0b3] block in CHANGELOG.md. Headline feature: full IPv6 support across the stack (closes #201). End-to-end IPv6 from external-IP discovery, Resolver.host_is_ip validation, get_all_ip page extraction, _get_anonymity_lvl + Judge canonical-form set membership, [v6]:port provider parsing, SOCKS5 ATYP=0x04 + correct response-ATYP-driven reply parsing, RFC 9112/9110-compliant CONNECT bracketing, RFC 3986 v6 host bracketing in Proxy output, and Happy Eyeballs DNS (RFC 8305 § 3) in Resolver.resolve. Also closes #214 (test fixture cleanup folded into the same PR). PR #212: 14 commits, 1357 additions, 93 deletions, 36 new tests (259 total). Reviewed and approved by coderabbitai; SonarCloud Quality Gate passed; opsera scan 0 critical/high. Follow-up tickets filed for the next-quarter work that's intentionally out of scope here: - #213 connection-layer Happy Eyeballs + DNSSEC + IDN - #215 codebase-wide type hints + mypy CI - #216 SBOM + sigstore (supply-chain hardening) - #217 cert pinning for default judge servers - #218 opt-in ECH (Encrypted Client Hello) - #219 opt-in TLS 1.3-only mode - #220 deterministic IPv6 external-IP discovery Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
4 tasks
bluet
added a commit
that referenced
this pull request
May 9, 2026
…de path CI revealed this pre-existing test (from #212) failed under the new get_real_ext_ips() flow: it patched aiohttp.ClientSession to return v6 text, but my refactor moved the actual probe behind _probe_family, which uses its own family-pinned ClientSession and a defensive wrong-family-response check. On CI where v6 isn't routable, only the v4 family was probed, the v6 mock response was rejected as wrong-family, and the test failed with RuntimeError. The contract being tested (singular shim returns canonical v6 form when discovery yields a v6 ext-IP) is unchanged; only the mocking strategy needs updating to point at the new internal entry points. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
pull Bot
pushed a commit
to zanachka/proxybroker2
that referenced
this pull request
May 10, 2026
…obes (bluet#220) Closes bluet#220. Stop hiding the dual-stack judge-rejection bug by storing just one ext-IP — discover BOTH v4 and v6 on dual-stack hosts and pass the full set through to Judge.check / _get_anonymity_lvl so set intersection passes whichever family the judge connection used. Why it matters -------------- PR bluet#212 added api64.ipify.org to the endpoint list, but the resolver still picked one host at random and stored a single ext-IP. On a dual-stack host this means the judge response (which can echo whichever family the connection used) is rejected ~50% of the time even though the connection works fine. Same bug all proxy-broker tools have today. The SOTA design — and the only one that actually fixes the bug — probes BOTH families and stores the set. Design ------ - `Resolver._has_local_route(family)` — UDP-connect to a doc-prefix address. Consults the routing table in microseconds without sending any packets. ENETUNREACH/EHOSTUNREACH means "no usable interface". This is the canonical Python idiom (used by urllib3, requests, etc). Critical for v4-only users (~50% of the install base): they pay ZERO startup-latency tax for the IPv6-fix machinery. - `Resolver._probe_family(family)` — single-family probe over an `aiohttp.TCPConnector(family=...)`. Pinning at the SOCKET layer (not the DNS layer) is immune to CDN/CNAME/AAAA-spoof quirks. Tries the existing `_ip_hosts` in random order; defensive wrong-family-response check filters X-Forwarded-For-style leaks. - `Resolver.get_real_ext_ips() -> frozenset[str]` — capability-detect available families, parallel `asyncio.gather` of `_probe_family` for each. Returns a set with 1 entry on single-stack hosts, 2 on dual-stack. Raises RuntimeError when no probe succeeds for any family. - `Resolver.get_real_ext_ip() -> str` — backward-compat shim. Returns one address from the new set, IPv6-preferred (matches Happy Eyeballs default). - `Checker(real_ext_ips=frozenset(...))` — new kwarg accepts the set directly. Legacy `real_ext_ip=str` still accepted and wrapped into a single-element frozenset; legacy `_real_ext_ip` attribute preserved. - `Judge.check(real_ext_ips=...)` — set-aware reception. Legacy `real_ext_ip=str` arg still accepted via the same wrap-into-set path. - `_get_anonymity_lvl(real_ext_ips, ...)` — set intersection against page-extracted IPs. Transparent if ANY of the host's real ext-IPs appears. Accepts set, iterable, str, or None for backward compat. - `Broker._init_judges_pool` (api.py) — now calls `get_real_ext_ips` and passes the set through to Checker. Latency profile --------------- | User class | Old | New (this PR) | |-----------------------|------------|----------------| | v4-only (~50%) | ~500ms | ~500ms (same) | | dual-stack v6 OK | ~500ms* | ~1000ms | | dual-stack v6 broken | ~500ms* | ~500ms | | v6-only (rare) | usually fails | ~500ms | (* but with the dual-stack judge-rejection bug) The capability check fails fast on broken-v6 dual-stack hosts via the routing-table query — no waiting on the v6 probe to time out. Tests ----- +10 resolver tests (capability check + 6 get_real_ext_ips paths + 2 singular shim cases) and +6 checker tests (set-aware _get_anonymity_lvl with v4/v6 leaks, empty set, legacy str compat, 3 Checker.__init__ kwarg variants). 276 pytest pass; ruff clean; opsera 0 critical/high. In-container Docker smoke: v4-only network correctly skips v6 probe and returns single-element set with the host's real v4 ext-IP. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
Summary
Closes #201. Make IPv6 a first-class citizen alongside IPv4 across detection, validation, anonymity comparison, SOCKS5 proxying, and
[v6]:portprovider parsing — replacing the prior situation of broken/half-wired IPv6 stubs scattered across the codebase.This expands #201's original scope (refactor
IPv6Patternregex) into the full IPv6 epic, per the maintainer review: "if we are working on IPv6, maybe just make it right. And we can't leave a half-fixed IPv6 there."Why
Resolver.host_is_ip()is IPv4-only with# TODO: add IPv6 support; users on IPv6-only networks failed at startup. The 700-charIPv6Patternregex (with# nosemgrep) used many capture groups sore.findallreturned tuples, not strings — IPv6 anonymity detection has never worked.c1b60b0(July 2025) tried to add v6 support tohost_is_ip, shipped real bugs (::ffff:192.0.2.1rejected because it contains both.and:), and never made it to master. That branch is the cautionary tale for why partial IPv6 is unsafe.str(ipaddress.ip_address(...))) as the single in-process IP representation and thread it through every comparison site. IPv4 form is identity, so v4 contracts are preserved.Layer summary
resolver.pyhost_is_ipaccepts v4+v6 via stdlibipaddress.get_real_ext_ipreturns canonical form. Addshttps://api64.ipify.org/to default endpoints so IPv6-only networks work.utils.pyIPv6Pattern+# nosemgrep. New narrow_IPV6_CANDIDATE_PATTERN(provably ReDoS-free) feedsipaddress.ip_addressfor validation. Publiccanonicalize_ip(s) -> str ⎮ Nonehelper.get_all_ipreturns canonical form so equivalent encodings collapse.judge.py,checker.pyreal_ext_ip in pageswapped for canonical-form set membership.negotiators.pySocks5NgtremitsATYP=0x04+ 16-byte for IPv6,ATYP=0x01+ 4-byte for IPv4 (regression-tested). SOCKS4 stays v4-only by spec.utils.py,providers.py,api.pyIPv6BracketedPortPattern+find_proxy_pairs(text)helper. Wired intoProvider._find_proxiesandBroker._load.proxy.pyProxy(host=v6)works end-to-end. New_format_host_porthelper brackets v6 per RFC 3986 inas_text,__repr__, andProxy.log.Out of scope (NOT half-fixes; orthogonal feature concerns)
update-geoCLI broken since 2019; need replacement strategy #200; bundled DB has limited v6 coverage. Documented as known limitation.Verification
test_resolverevent_loopfixture errors are unrelated and were present on master before this branch.ruff check --fix,ruff format).tmp/, and Claude config — none introduced by this PR.proxybroker2-ipv6-smoke:latestand:find --types HTTP --limit 2for IPv4 regression — returned 2 working real HTTP proxies (US), confirming no v4 regression.Test plan
docker run --rm bluet/proxybroker2:<test-tag> find --types HTTP --limit 3— verifies v4 pathdocker run --rm bluet/proxybroker2:<test-tag> python -c "from proxybroker import Proxy; print(Proxy('2001:db8::1', 8080))"— verifies v6 construct + repr🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores