fix(poller): close DNS rebinding gap - #62
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe poller’s link verification now resolves each hostname once, validates public addresses, pins the selected IP for TCP connections, and applies the same protection to redirects. Tests cover DNS reuse, mixed-address rejection, and injected-client bypasses. Versions were bumped to 1.11.2. ChangesSSRF-safe link verification
Project metadata and backlog
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Probe
participant PinnedResolver
participant PinnedTransport
participant TCPBackend
Probe->>PinnedResolver: Pin hostname
PinnedResolver->>PinnedResolver: Validate public DNS answers
Probe->>PinnedTransport: Send request
PinnedTransport->>TCPBackend: Connect to pinned public IP
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@BACKLOG.md`:
- Line 10: Update the BACKLOG entry for issue `#51` to unchecked ([ ]) while the
GitHub issue remains open; do not mark it complete until the matching issue is
closed.
In `@src/applytrack/linkcheck.py`:
- Around line 167-186: The test suite must exercise the production wiring
exposed by ssrf_safe_client, not only _PinnedResolver and _PinnedNetworkBackend
directly. Add a lightweight test that constructs the client, verifies its
transport uses a _PinnedNetworkBackend, and confirms that backend is bound to
the same resolver targeted by _PinnedClient.pin; avoid relying on private httpx
internals where possible.
- Around line 259-262: Normalize the hostname to its ASCII IDNA form before
calling _pin_for_request in the initial request flow, instead of passing
parts.hostname directly. Apply the same normalization in the redirect-check
logic so both initial and redirected requests use the key expected by
httpx/httpcore, while preserving existing rejection behavior for invalid or
unpinned hosts.
- Around line 155-164: Update _PinnedTransport.__init__ to preserve the
HTTPTransport pool configuration when replacing _pool, forwarding the relevant
transport options and constructing the httpcore.ConnectionPool with matching
settings while retaining _PinnedNetworkBackend(resolver). Avoid relying on the
private _pool replacement where possible, or ensure the replacement remains
behaviorally equivalent to HTTPTransport defaults and configured knobs.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff75ce23-a655-4a63-bdca-50a93ed18237
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
BACKLOG.mdapi/ApplyTrack.Api/ApplyTrack.Api.csprojpyproject.tomlsrc/applytrack/linkcheck.pysrc/applytrack/poll.pytests/test_linkcheck.py
📜 Review details
🔇 Additional comments (11)
pyproject.toml (1)
7-14: LGTM!api/ApplyTrack.Api/ApplyTrack.Api.csproj (1)
8-8: LGTM!src/applytrack/linkcheck.py (4)
14-18: LGTM!
96-131: LGTM!
134-152: LGTM!
189-211: LGTM!src/applytrack/poll.py (1)
49-49: LGTM!Also applies to: 761-765
tests/test_linkcheck.py (4)
11-23: LGTM!
94-135: LGTM!
137-153: LGTM!
156-169: LGTM!
|
|
||
| ## Security and stability | ||
|
|
||
| - [x] [#51 — Close the Python poller link-check DNS rebinding gap](https://github.com/CryptoJones/OSApplyTrack/issues/51) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep issue #51 unchecked until it is closed.
The file says items may be checked only after their matching issue is closed, but GitHub currently shows issue #51 as Open. Change this to [ ] or close the issue as part of the release workflow. (github.com)
🤖 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 `@BACKLOG.md` at line 10, Update the BACKLOG entry for issue `#51` to unchecked
([ ]) while the GitHub issue remains open; do not mark it complete until the
matching issue is closed.
| class _PinnedTransport(httpx.HTTPTransport): | ||
| """HTTPX transport whose connection pool cannot perform a second DNS lookup.""" | ||
|
|
||
| def __init__(self, resolver: _PinnedResolver) -> None: | ||
| super().__init__(trust_env=False) | ||
| self._pool.close() | ||
| self._pool = httpcore.ConnectionPool( | ||
| ssl_context=httpx.create_ssl_context(trust_env=False), | ||
| network_backend=_PinnedNetworkBackend(resolver), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What are the default Limits values (max_connections, max_keepalive_connections) used by httpx.HTTPTransport, and does httpx.HTTPTransport still store its connection pool on a _pool attribute in the latest release?
💡 Result:
The default limits used by httpx.HTTPTransport are managed by the Limits configuration class, which sets max_connections to 100 and max_keepalive_connections to 20 [1][2][3]. Additionally, the keepalive_expiry default value is 5.0 seconds [1][3]. Yes, httpx.HTTPTransport still stores its connection pool on a _pool attribute in the latest release [4][5][6][7]. This attribute is used internally to hold the underlying httpcore.ConnectionPool (or proxy-specific variants such as httpcore.HTTPProxy or httpcore.SOCKSProxy) [4][6][7]. While this attribute is part of the library's internal implementation and not a public API, it remains present for managing connections within the transport [8].
Citations:
- 1: https://github.com/encode/httpx/blob/master/httpx/_config.py
- 2: https://github.com/encode/httpx/blob/def4778d/httpx/_config.py
- 3: https://deepwiki.com/encode/httpx/6-configuration
- 4: https://github.com/encode/httpx/blob/master/httpx/_transports/default.py
- 5: https://fossies.org/dox/httpx-0.28.1/default_8py_source.html
- 6: https://github.com/encode/httpx/blob/def4778d/httpx/_transports/default.py
- 7: https://github.com/encode/httpx/blob/b5addb64/httpx/_transports/default.py
- 8: Public API for accessing connection pool details encode/httpx#2966
Preserve HTTPTransport pool settings here
_PinnedTransport replaces httpx.HTTPTransport’s pool with a raw httpcore.ConnectionPool, so HTTPTransport defaults and transport knobs no longer apply. It also relies on the internal _pool attribute, which is not part of the public API. Forward the relevant options or build the replacement pool from the same settings to keep behavior stable.
🤖 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 `@src/applytrack/linkcheck.py` around lines 155 - 164, Update
_PinnedTransport.__init__ to preserve the HTTPTransport pool configuration when
replacing _pool, forwarding the relevant transport options and constructing the
httpcore.ConnectionPool with matching settings while retaining
_PinnedNetworkBackend(resolver). Avoid relying on the private _pool replacement
where possible, or ensure the replacement remains behaviorally equivalent to
HTTPTransport defaults and configured knobs.
| class _PinnedClient(httpx.Client): | ||
| """HTTP client that connects only to addresses pinned by its resolver.""" | ||
|
|
||
| def __init__(self, *, timeout: float) -> None: | ||
| self._resolver = _PinnedResolver() | ||
| super().__init__( | ||
| timeout=timeout, | ||
| follow_redirects=False, | ||
| headers=BROWSER_HEADERS, | ||
| transport=_PinnedTransport(self._resolver), | ||
| trust_env=False, | ||
| ) | ||
|
|
||
| def pin(self, host: str) -> bool: | ||
| return self._resolver.pin(host) | ||
|
|
||
|
|
||
| def ssrf_safe_client(*, timeout: float = 12.0) -> httpx.Client: | ||
| """Return a reusable client that pins every validated hostname to one public IP.""" | ||
| return _PinnedClient(timeout=timeout) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No test exercises the _PinnedTransport/_PinnedClient/ssrf_safe_client wiring.
All new tests (test_dns_answer_is_validated_once_then_tcp_uses_the_pinned_ip, test_resolver_rejects_a_hostname_with_any_private_answer) construct _PinnedResolver/_PinnedNetworkBackend directly, bypassing _PinnedTransport/_PinnedClient. The actual production entry point, ssrf_safe_client(), is never constructed or exercised in isolation, so a wiring regression (e.g. the private-_pool reliance flagged above breaking on an httpx upgrade) wouldn't be caught by this suite.
Consider a lightweight test that at least asserts ssrf_safe_client() returns a client whose transport uses a _PinnedNetworkBackend bound to the same resolver .pin() delegates to.
🤖 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 `@src/applytrack/linkcheck.py` around lines 167 - 186, The test suite must
exercise the production wiring exposed by ssrf_safe_client, not only
_PinnedResolver and _PinnedNetworkBackend directly. Add a lightweight test that
constructs the client, verifies its transport uses a _PinnedNetworkBackend, and
confirms that backend is bound to the same resolver targeted by
_PinnedClient.pin; avoid relying on private httpx internals where possible.
| client = client or ssrf_safe_client(timeout=timeout) | ||
| try: | ||
| if not _pin_for_request(client, parts.hostname or ""): | ||
| return LinkStatus(url=url, ok=False, error="refused non-public or unpinned address") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does httpx.URL.host return the Punycode (ASCII) form or the Unicode form for internationalized domain name hostnames, and does it match the host used internally to build the httpcore connection origin?
💡 Result:
In the httpx library, the URL.host property and the internal host used to build the connection (httpcore origin) behave differently regarding internationalized domain names (IDN) [1][2]. 1. Does URL.host return Punycode or Unicode? The URL.host property is designed to return the Unicode (decoded) form of the hostname [1][2]. When you access this property, httpx attempts to decode any Punycode (IDNA-encoded) sequences (those starting with "xn--") into their human-readable Unicode representation [1]. Note that there have been reports of behavior inconsistencies in some versions where Punycode might be returned if the hostname does not strictly start with "xn--" or in specific edge cases [3], but the intended and documented design is to return the decoded Unicode string [1][2]. 2. Does it match the host used internally to build the connection? No, it does not directly match the internal host used to build the httpcore connection origin. For network requests, httpx uses the.raw_host property (or underlying IDNA-encoded bytes) to build the connection [4][1][2]. The.raw_host property is always IDNA-encoded (Punycode) ASCII, as required by DNS and HTTP protocols [1][2]. Summary: - URL.host: Returns the Unicode form (e.g., "中国.icom.museum") [1]. - Internal connection host: Uses the IDNA-encoded Punycode form (e.g., "xn--fiqs8s.icom.museum"), accessible via URL.raw_host [1][2].
Citations:
- 1: https://github.com/encode/httpx/blob/b5addb64/httpx/_urls.py
- 2: https://deepwiki.com/encode/httpx/4.3-url-handling
- 3: URL.host returns Punycode instead of Unicode for some URLs encode/httpx#3333
- 4: Differentiate between 'url.host' and 'url.raw_host' encode/httpx#1590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline src/applytrack/linkcheck.py --view expanded || true
printf '\n== relevant slices ==\n'
sed -n '1,220p' src/applytrack/linkcheck.py | nl -ba | sed -n '1,220p'
printf '\n---\n'
sed -n '220,360p' src/applytrack/linkcheck.py | nl -ba | sed -n '220,360p'
printf '\n== httpx host usages in repo ==\n'
rg -n "raw_host|URL\\(|httpx\\.URL|hostname" src -g '*.py' || trueRepository: CryptoJones/OSApplyTrack
Length of output: 1711
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== lines 90-210 =='
awk 'NR>=90 && NR<=210 {print NR ": " $0}' src/applytrack/linkcheck.py
echo
echo '== lines 242-290 =='
awk 'NR>=242 && NR<=290 {print NR ": " $0}' src/applytrack/linkcheck.py
echo
echo '== lines 290-340 =='
awk 'NR>=290 && NR<=340 {print NR ": " $0}' src/applytrack/linkcheck.pyRepository: CryptoJones/OSApplyTrack
Length of output: 8417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('src/applytrack/linkcheck.py')
text = path.read_text()
for start, end in [(90, 210), (242, 290), (290, 340)]:
print(f'== lines {start}-{end} ==')
for i, line in enumerate(text.splitlines(), 1):
if start <= i <= end:
print(f'{i}: {line}')
print()
PYRepository: CryptoJones/OSApplyTrack
Length of output: 8418
Normalize the pinning key for IDN hosts src/applytrack/linkcheck.py:261-276
urlsplit().hostname can be Unicode for internationalized domains, while httpx/httpcore uses the ASCII raw_host on the connection path. That means the resolver can miss the pinned entry and reject otherwise valid IDN links on the initial request and redirect hops. Use the same IDNA-normalized host key here and in the redirect check.
🤖 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 `@src/applytrack/linkcheck.py` around lines 259 - 262, Normalize the hostname
to its ASCII IDNA form before calling _pin_for_request in the initial request
flow, instead of passing parts.hostname directly. Apply the same normalization
in the redirect-check logic so both initial and redirected requests use the key
expected by httpx/httpcore, while preserving existing rejection behavior for
invalid or unpinned hosts.
1 participant
Summary
Validation
Closes #51
Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/