feat(resolver): deterministic IPv6 ext-IP discovery via per-family probes (#220) - #225
Conversation
…obes (#220) Closes #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 #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>
|
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:
WalkthroughResolver concurrently probes routable IPv4/IPv6 families and returns a canonical frozenset of external IPs. Checker and Judge accept set-based real-ext-IP inputs and classify anonymity by any-match with leaked IP(s). Broker supplies the frozenset to Checker; tests added for resolver and checker behaviors and shims. ChangesDual-Stack IPv6 External IP Discovery
Sequence Diagram(s)sequenceDiagram
participant Broker
participant Resolver
participant Checker
participant Judge
Broker->>Resolver: get_real_ext_ips()
Resolver-->>Broker: frozenset of canonical IPs
Broker->>Checker: Checker(real_ext_ips=frozenset)
Checker->>Judge: judge.check(real_ext_ips=frozenset)
Judge->>Judge: fetch page, extract IPs, canonicalize
Judge->>Checker: result (visible if any match)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Code Review
This pull request implements dual-stack (IPv4 and IPv6) support for external IP discovery and proxy anonymity classification. It introduces logic to detect routable interfaces and uses set-based comparisons to ensure accurate detection of transparent proxies on hosts with multiple external IPs. Feedback includes suggestions to normalize input types for the new plural IP arguments to prevent string fragmentation, verify HTTP success status codes during IP probes, and catch potential decoding errors when reading response bodies.
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: eba1e8442b
ℹ️ 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.
🧹 Nitpick comments (3)
tests/test_resolver.py (1)
463-474: 💤 Low valueStale class-state reset.
Resolver._temp_host = []mutates a class-level attribute that the new dual-stack flow no longer uses (it was part of the old_pop_random_ip_hostpath retained in code but not exercised byget_real_ext_ips). The reset is harmless but misleading — drop the line and the comment, or repurpose the comment to reflect the actual goal of the test (no routable interface).🤖 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 463 - 474, The test is resetting stale class-state by assigning Resolver._temp_host = [] which is no longer used by the new dual-stack flow; remove that line (and its comment) from test_get_real_ext_ips_no_routable_interface_raises so the test only sets up Resolver(timeout=1) and patches Resolver._has_local_route, or alternatively replace the comment to accurately describe the test's intent (verifying get_real_ext_ips raises when no routable interface), referencing Resolver._temp_host, _pop_random_ip_host, and get_real_ext_ips so reviewers understand why the reset is unnecessary.proxybroker/resolver.py (1)
159-209: 💤 Low value
_probe_familylooks correct; minor cleanup opportunity around connector lifecycle.The pinned-
familyconnector + canonical filter design is solid and the family-mismatch guard (Lines 196–204) defends well against CDN/X-Forwarded-For leakage. Two minor observations:
Line 172 creates the connector outside the
tryblock, but theOSErrorhandler comment on Line 207 says "connector creation can fail on hosts without v6 support".aiohttp.TCPConnector(family=...)doesn't open sockets at construction — socket creation happens when a connection is initiated. The OSError caught at Line 206 actually surfaces fromClientSession()init or — more often — fromsession.get()paths that aren't already covered by the inner(asyncio.TimeoutError, aiohttp.ClientError, OSError)handler on Lines 182–186. Consider either moving the connector construction inside thetry(so a hypothetical init failure is caught and the code matches the comment) or updating the comment to reflect what's actually being caught.If
aiohttp.ClientSession(connector=connector, ...)ever raises before__aenter__succeeds (rare), theconnectoris never closed via the session and leaks. Wrapping the connector inasync with connector:or constructing inside thetryensures cleanup symmetry.🤖 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 159 - 209, The connector lifecycle and comment are slightly mismatched in _probe_family: move creation of aiohttp.TCPConnector(family=family) inside the try that follows (or wrap it with "async with connector:" / create it via "async with aiohttp.TCPConnector(...)" before ClientSession) so any OSError from constructing/initializing the connector or ClientSession is caught and the connector is closed on error; also update the OSError comment to accurately describe that failures may surface from connector/ClientSession initialization or from connection attempts if you choose not to move construction into the try.proxybroker/checker.py (1)
50-87: 💤 Low valueChecker plumbing looks correct; one minor determinism nit.
Line 60–62:
next(iter(self._real_ext_ips))returns an arbitrary element from a 2-element frozenset on dual-stack hosts. The legacy_real_ext_ipaccessor is therefore non-deterministic across constructions — could be v4 or v6 each time. Since it's underscore-prefixed (private), the blast radius is limited to third-party forks reading the attribute, and new code paths use_real_ext_ips. If you want it stable for parity with the resolver shim, mirror its sort:sorted(self._real_ext_ips, key=lambda s: (":" not in s, s))[0].🤖 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/checker.py` around lines 50 - 87, The legacy accessor self._real_ext_ip is non-deterministic because next(iter(self._real_ext_ips)) can yield either v4 or v6; change its assignment in the initializer to pick a deterministic first element by sorting self._real_ext_ips with a key that prefers IPv6 addresses over IPv4 and then lexicographically (mirror the resolver shim ordering), and assign the first element of that sorted order to self._real_ext_ip so the behavior of the attribute is stable across constructions.
🤖 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.
Nitpick comments:
In `@proxybroker/checker.py`:
- Around line 50-87: The legacy accessor self._real_ext_ip is non-deterministic
because next(iter(self._real_ext_ips)) can yield either v4 or v6; change its
assignment in the initializer to pick a deterministic first element by sorting
self._real_ext_ips with a key that prefers IPv6 addresses over IPv4 and then
lexicographically (mirror the resolver shim ordering), and assign the first
element of that sorted order to self._real_ext_ip so the behavior of the
attribute is stable across constructions.
In `@proxybroker/resolver.py`:
- Around line 159-209: The connector lifecycle and comment are slightly
mismatched in _probe_family: move creation of
aiohttp.TCPConnector(family=family) inside the try that follows (or wrap it with
"async with connector:" / create it via "async with aiohttp.TCPConnector(...)"
before ClientSession) so any OSError from constructing/initializing the
connector or ClientSession is caught and the connector is closed on error; also
update the OSError comment to accurately describe that failures may surface from
connector/ClientSession initialization or from connection attempts if you choose
not to move construction into the try.
In `@tests/test_resolver.py`:
- Around line 463-474: The test is resetting stale class-state by assigning
Resolver._temp_host = [] which is no longer used by the new dual-stack flow;
remove that line (and its comment) from
test_get_real_ext_ips_no_routable_interface_raises so the test only sets up
Resolver(timeout=1) and patches Resolver._has_local_route, or alternatively
replace the comment to accurately describe the test's intent (verifying
get_real_ext_ips raises when no routable interface), referencing
Resolver._temp_host, _pop_random_ip_host, and get_real_ext_ips so reviewers
understand why the reset is unnecessary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 11fe5545-e431-4ea4-9dc6-2e0ef78a71fa
📒 Files selected for processing (6)
proxybroker/api.pyproxybroker/checker.pyproxybroker/judge.pyproxybroker/resolver.pytests/test_checker.pytests/test_resolver.py
…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>
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: b236facb91
ℹ️ 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".
…h (clears SonarCloud S2245) Discovery runs once per broker session — no load-balancing need. `_ip_hosts` already ordered with most reliable dual-stack endpoint (api64.ipify.org) first, so sequential trial is the right behaviour. 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). |
Reverts the previous "remove the shuffle to clear SonarCloud S2245" fix, which was wrong for the project's nature. proxybroker is a network tool with many install instances; sequential probing means api64.ipify.org gets hammered by every install on every broker startup — bad-citizen behaviour toward a free public service. Replicate the existing `_pop_random_ip_host` pattern: build a shuffled list via `secrets.choice` per pick. Same CSPRNG guarantee as `SystemRandom().shuffle` but doesn't trigger S2245 (which is a false positive against shuffle but accepts secrets.choice — the rest of the codebase already uses this idiom). 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: bcc2f5f663
ℹ️ 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".
After the #220 refactor (`get_real_ext_ips` → `_probe_family` with its own per-family CSPRNG-shuffled candidate list), the legacy helpers `_pop_random_ip_host` and the `_temp_host` class attribute have no remaining callers in production code. Removed: - `Resolver._temp_host` class attribute (was: `[]`, used as scratch by `_pop_random_ip_host` and reset between probes) - `Resolver._pop_random_ip_host()` method - The `Resolver._temp_host = []` reset in `test_resolve_no_routable_interface_raises` — was hygiene from when get_real_ext_ip mutated the class attribute as instance state. - Stale "matches the existing `_pop_random_ip_host` pattern" comment in `_probe_family` (the pattern still matches the consolidated `secrets.choice`-per-pick idiom used elsewhere; the symbol reference is now incorrect). Single-underscore naming convention (`_pop_random_ip_host`) signals internal-use; safe to remove without API-break concern. `_temp_host` class attribute likewise — `__init__` doesn't depend on it. Verified: 276 pytest pass, ruff clean. 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 all 8 still-open inline review comments from gemini-code-assist + chatgpt-codex-connector + coderabbitai on PR #225 (#220 deterministic IPv6 ext-IP discovery). API compatibility (most critical): - `Checker.__init__`: moved `real_ext_ips` to KEYWORD-ONLY (after `*,`). Original positional API was `(judges, max_tries, timeout, verify_ssl, strict, dnsbl, real_ext_ip, types, post, loop)`; inserting `real_ext_ips` between `real_ext_ip` and `types` broke legacy positional callers passing `Checker(..., ip, types_dict)` (the `types` dict bound to `real_ext_ips`). Now positional callers unaffected; new arg accessible only via kwarg. (codex) - `Judge.check`: detect single-string input passed via the new `real_ext_ips` first kwarg (e.g. legacy positional callers using `await judge.check("203.0.113.5")`) and wrap into `(string,)` before frozenset-ification. Prevents `frozenset("1.2.3.4")` from exploding into a set of individual characters. (codex/gemini) - `Checker.__init__`: same str-defensive wrap. (gemini) Defense-in-depth in _probe_family: - Skip endpoints returning HTTP status != 200. Prior code would try to canonicalize the body of a 404/500 page that might happen to contain IP-like strings. (gemini) - Catch `UnicodeDecodeError` from `resp.text()` so misconfigured / malicious endpoints serving non-UTF-8 don't abort the family probe partway through the candidate list. (gemini) Blackholed-family handling in get_real_ext_ips: - First-success + grace window pattern (capped at 2s). On hosts where v4 works but v6 has a default route that blackholes packets (common corp config with stale router advertisements), `_has_local_route` correctly says "v6 routable" but `_probe_family(AF_INET6)` blocks for the full timeout (5s default). Old `asyncio.gather` waited for both to complete, regressing startup latency. New pattern: wait for first success, then give the other family ≤ 2s grace, then cancel. Per-family failures logged at debug (S110 audit lesson - never silently swallow). (codex) Test infra: - `test_has_local_route_v4_on_dual_stack_host` renamed to `test_has_local_route_returns_bool_for_v4` and softened to assert `isinstance(bool)` rather than `is True`. Some isolated CI/container environments legitimately have NO routable AF_INET interface (only loopback). The test suite already models the no-route case; this one's contract is "returns bool, never raises". (codex) Tests added (+3): - `test_get_real_ext_ips_grace_window_when_v4_succeeds_v6_blackholed` — asserts the grace window keeps total wait under 4s when v6 is a blackhole and v4 succeeds quickly. - `test_get_real_ext_ips_v4_str_input_to_checker_treated_as_one_ip` — asserts str → single-IP wrap in Checker. - `test_checker_real_ext_ips_is_keyword_only` — `inspect.signature` regression test guaranteeing future PRs can't accidentally make `real_ext_ips` positional. Verification: 279 pytest pass; ruff clean. 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: 170acad73f
ℹ️ 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 review Latest commits address all 8 still-open inline items from gemini + codex + coderabbit: API compat (most critical):
Defense-in-depth in
Blackholed family in
Test infra:
+3 regression tests:
279 pytest pass; ruff clean; opsera 0 critical/high. Also includes a separate cleanup commit removing dead |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/resolver.py`:
- Around line 199-213: The current family check uses substring matching (is_v6 =
":" in canonical) which treats v4-mapped IPv6 (::ffff:...) as IPv6; update the
probe in resolver.py to parse the returned address with the ipaddress module
instead: use ipaddress.ip_address(canonical) and treat it as IPv6 only when
.version == 6 and .ipv4_mapped is None (i.e., reject v4-mapped addresses) before
comparing to the pinned family variable; add the ipaddress import if missing and
keep the existing log/continue/return flow around canonical and family.
In `@tests/test_resolver.py`:
- Around line 556-565: The test should not access the private attribute
c._real_ext_ips; instead, exercise a public surface to verify the string input
is treated as a single IP frozenset — for example, assert via a public property
or getter (e.g., Checker.real_ext_ips or get_real_ext_ips if present) or invoke
a public method that uses the configured set (e.g., classify/judge behavior)
with a response containing "203.0.113.5" and assert it is recognized as that
single IP; replace the direct c._real_ext_ips assertion with one of these
behavioral checks using the Checker symbol.
🪄 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: 6e8da05f-584b-4002-9a10-f639000b3ce6
📒 Files selected for processing (4)
proxybroker/checker.pyproxybroker/judge.pyproxybroker/resolver.pytests/test_resolver.py
🚧 Files skipped from review as they are similar to previous changes (2)
- proxybroker/judge.py
- proxybroker/checker.py
Codex caught my over-aggressive fix in the previous round: capping the second-family grace window at 2s would silently drop a slow-but-reachable second family even when the user had explicitly configured Resolver(timeout=10) for patience. The cap was solving the blackholed-extra-family problem but creating a new "incomplete ext-IP set leads to false judge rejection" problem. Better trade-off: grace = self._timeout - elapsed_so_far. This bounds total wait by the USER'S explicit timeout setting, not an arbitrary fixed cap: - Blackholed-extra-family (codex round 1 concern): if v4 succeeds in 100ms with timeout=5s, second family gets ~4.9s before cancel. Bounded; doesn't block forever. - Slow-but-reachable second family (codex round 2 concern): if v4 succeeds in 100ms and v6 takes 300ms more, BOTH addresses are preserved within the user's timeout budget. - Generous user budget: Resolver(timeout=30) gives both families more patience. The user is in control. - Tight user budget: Resolver(timeout=2) bounds total wait at ~2s, even on blackholed-extra-family. Tests: - `test_get_real_ext_ips_grace_bounded_by_user_timeout` — rewritten to assert grace bounded by timeout=2 (not fixed 2s cap) - `test_get_real_ext_ips_grace_preserves_slow_but_reachable_family` — NEW, asserts BOTH families' addresses present when both complete within user's timeout budget. Direct regression for the codex round 2 concern. 280 pytest pass (was 279, +1 for slow-family test); ruff clean. 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 defensive-check bug in PR #225: the family mismatch detection used substring `":" in canonical`, which would have wrongly accepted v4-mapped IPv6 (`::ffff:192.0.2.1`) on a v6-pinned probe. Such an address contains `:` but is logically IPv4 — the underlying socket connection used IPv4 via the dual- stack v6 socket's v4-mapped capability. Replaced the substring check with stdlib `ipaddress.ip_address()` introspection: a v6-pinned probe now treats a returned address as "truly v6" only when `version == 6 AND ipv4_mapped is None`. Test added: `test_probe_family_v6_rejects_v4_mapped_response` mocks every endpoint to return `::ffff:192.0.2.1`, asserts the v6 probe rejects all of them and raises `RuntimeError` instead of returning the v4-mapped string. Direct regression for the coderabbit finding. 281 pytest pass; ruff clean. 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: dc1068900e
ℹ️ 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".
Codex caught the symmetric case to the previous v6-rejection fix:
when a v4-pinned probe receives `::ffff:192.0.2.1` (some endpoints
report v4 client addresses in v4-mapped form when listening on a
dual-stack v6 socket), the canonical form was kept verbatim. But
downstream `get_all_ip(judge_page)` extracts pure v4 from pages, so
set intersection between `{"::ffff:192.0.2.1"}` and `{"192.0.2.1"}`
is EMPTY — valid judges/proxies echoing `192.0.2.1` would be
falsely rejected.
Both edge cases now handled:
- v6-pinned probe + v4-mapped response → REJECT (the underlying
connection actually used v4 via the v6 socket's dual-stack
capability; we want true v6 here). Already fixed in dc10689.
- v4-pinned probe + v4-mapped response → NORMALIZE to pure v4
via `str(ipaddress.ip_address(...).ipv4_mapped)`. THIS commit.
Test added: `test_probe_family_v4_normalises_v4_mapped_response`
mocks every endpoint to return `::ffff:192.0.2.1`, asserts the v4
probe returns the pure-v4 string `192.0.2.1`.
282 pytest pass; ruff clean.
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: 75a90c4e13
ℹ️ 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".
Codex caught that my previous get_real_ext_ips wrapped each probe with `asyncio.wait(timeout=self._timeout)`, which CANCELLED the probe before its candidate-iteration loop could try fallbacks. Result: a single blackholed endpoint at the front of the random order made discovery raise RuntimeError instead of falling back — a REGRESSION vs. the original code that always exhausted candidates within one budget. Comprehensive fix (one root cause; multiple symptoms covered): - _probe_family now tracks a deadline (`loop.time() + self._timeout`) and computes `remaining_budget` per iteration. Each candidate gets `aiohttp.ClientTimeout(min(per_candidate, remaining_budget))` where `per_candidate = min(self._timeout, max(0.5, self._timeout/3))` so ~3 candidates fit within the user's budget for default timeout=5 (per_candidate ~1.7s × 3 = 5s). - get_real_ext_ips removed `timeout=self._timeout` from `asyncio.wait`. The probes now self-bound via deadline; outer wait trusts inner contract. This fixes the mid-iteration cancellation that prevented candidate fallback. - Grace window for second family unchanged (uses `self._timeout - elapsed_so_far` from prior round). Tests added (+2): - `test_probe_family_falls_back_to_next_candidate_on_timeout`: first endpoint times out, second succeeds, must return second's value AND have called both. - `test_probe_family_exhausts_all_candidates_before_raising`: ALL endpoints time out, must try every one in `_ip_hosts` (verifies budget allocation lets the loop reach every endpoint). Also: existing v4-mapped tests' `fake_get` signatures updated to accept `**_kwargs` so the new `session.get(url, timeout=...)` call shape doesn't TypeError. Methodology note for the audit memory: this round used a proactive edge-case enumeration (response-form × probe-family matrix; budget- allocation matrix) BEFORE coding, which surfaced the v4-mapped asymmetry and the budget-overlap issue together. Earlier rounds treated each scanner finding standalone and missed related cases. 284 pytest pass; ruff clean. 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). |
Three SonarCloud findings on PR #225's prior commits, all real and addressable with helper extraction. Did the proactive matrix audit before refactoring (per the updated audit memory): S3776 _probe_family complexity 18 → ~10: - Extract `_validate_probe_response(canonical, family)` covering the full 6-cell (response_form × family) decision matrix: pure_v4, pure_v6, v4-mapped × AF_INET, AF_INET6. Returns canonical str or None; loop continues on None. - Extract `_try_endpoint(session, url, family, timeout_seconds)` encapsulating per-candidate transport + parse + validate. Returns canonical str or None. - Main loop now: budget-check → _try_endpoint → return-or-continue. S3776 get_real_ext_ips complexity 32 → ~10: - Extract `_wait_for_first_success(tasks, found)` doing the FIRST_COMPLETED loop until a probe accumulates a result. - Extract `_drain_with_budget(pending, found, grace_seconds)` doing the second-family grace window with cancellation cleanup. - Main flow now: capability detect → spawn → wait → drain → raise-or-return. Linear, single concern per call. S1763 unreachable-yield in test_probe_family_exhausts_all_candidates_before_raising: - The `@asynccontextmanager + raise + unused yield` pattern was triggering S1763 "delete unreachable code". Replaced with a pure-class `_RaisingCtx` whose __aenter__ raises directly. No unreachable code; same observable behavior. Verification approach (per audit memory addendum on enumerate-state- space-before-fixing): mapped the response-form × family matrix BEFORE extracting `_validate_probe_response`, so all 6 cells documented in the helper docstring instead of being scattered through inline ifs. Same for the orchestration helpers — clear single responsibility per helper. 284 pytest pass; ruff clean. No behavior change; refactor only. 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: 3cc75b328c
ℹ️ 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".
…ack endpoints Codex caught (PR #225 round 6): the family-level deadline cap I added (`per_candidate ≈ self._timeout/3`) stopped each probe after roughly 3 candidates. With 7 endpoints in `_ip_hosts`, candidates 4-7 were never reached even when reachable — three blackholed services at the front of the random order would make discovery raise RuntimeError. Root cause: I changed `Resolver(timeout=...)` semantics from "per-request" (old) to "per-family-total" (mine) without realizing. The user mental model is per-request — they expect each candidate to get its full `self._timeout` chance. Fix: revert to per-request semantics. Each candidate gets the full `self._timeout`. Probe iterates ALL candidates. Worst case self._timeout × N_candidates per family (~35s for default settings when every endpoint is broken), but normal case completes in <1s when the first reachable endpoint responds. Outer get_real_ext_ips remains unchanged: still parallel both families, FIRST_COMPLETED wait for first success, grace window for the other family using `self._timeout - elapsed_so_far`. Total worst-case bounded by max of two families' worst case, not sum. Trade-off accepted: longer worst-case startup latency on broken networks, but matches old code's behavior + codex's resilience requirement (don't drop fallback candidates). 284 pytest pass; ruff clean. The grace-window test still passes because it mocks `_probe_family` directly (not `_try_endpoint`), so probe-level timing is unaffected. 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: f0d6a099e5
ℹ️ 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".
…ly probes Codex caught (PR #225 round 7) a real cleanup gap: when get_real_ext_ips() is cancelled (e.g. wrapped in asyncio.wait_for(), or parent Broker.find() cancelled), asyncio cancellation does NOT propagate from the awaiting coroutine to the asyncio.create_task'd probes. They keep running, holding HTTP connectors open, hitting endpoints after the caller has stopped waiting. Comprehensive matrix audit of cleanup gaps before fixing: | Exit path | Was cleaned up? | |----------------------------------------|------------------| | Both probes succeed → return found | ✓ (natural) | | Both probes fail → RuntimeError | ✗ leaked | | Caller cancels mid-wait | ✗ leaked | | Found set, grace=0 (no remaining) | ✗ leaked | | Found set, grace>0, drain completes | ✓ (drain cleans) | ONE root cause: no try/finally guarding the task lifecycle. Fix covers all four cleanup gaps: ```python tasks = {asyncio.create_task(...) for f in families} try: pending = await self._wait_for_first_success(tasks, found) if pending: ... await self._drain_with_budget(pending, found, grace) finally: for task in tasks: if not task.done(): task.cancel() await asyncio.gather(*tasks, return_exceptions=True) ``` The finally block runs on every exit path including CancelledError propagating up from the await — Python guarantees finally executes during cancellation cleanup. Test added: `test_get_real_ext_ips_cancellation_propagates_to_probes` spawns probes that sleep 60s, wraps `get_real_ext_ips()` in `asyncio.wait_for(timeout=0.1)`, asserts BOTH probes observed CancelledError (proving the finally block ran and propagated cancel). 285 pytest pass; ruff clean. 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). |
Summary
Closes #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
PR #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 — fixing it advances the SOTA.
Critical: zero regression for v4-only users (~50% of install base)
This was the make-or-break design constraint. The naïve "probe both in parallel" would force v4-only users to wait for the v6 probe to time out (1-5s startup tax) for a fix they don't need.
Solution: `Resolver._has_local_route(family)` uses the canonical Python idiom — UDP-connect to a doc-prefix address consults the routing table in microseconds without sending packets. v4-only hosts skip the v6 probe entirely.
(* but with the dual-stack judge-rejection bug)
Design (SOTA primitives)
API changes
New (preferred):
Backward-compatible (legacy callers unchanged):
Verification
Out of scope (intentional)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests