Send browser-like User-Agent when fetching RSS feeds #16
marvin8
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-2-feed-user-agent"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Some feeds (notably The Driven,
https://thedriven.io/feed/) reject requests with httpx2's default User-Agent with403 Forbidden, so their articles were never imported. The same URL returns 200 with a browser User-Agent (verified in the Sept 2026 probe recorded on issue #2).fetch_feednow sends a browser-like User-Agent (Firefox on Linux, the exact string the probe verified works)YUNJIN_HTTP_USER_AGENTenvironment variable, read at call time; empty/whitespace values fall back to the defaultSSRF guard (WuMing findings, maintainer-approved)
_validate_feed_url()runs before every fetch:http/httpsscheme allow-list (always enforced), host resolution blocking private/loopback/link-local/reserved/unspecified/multicast addresses (IPv4-mapped IPv6 unwrapped, mixed resolution rejected, unresolvable hosts fail closed)_fetch_feed_response): every hop is re-validated through the same guard, with a missing-Locationerror and a 5-redirect cap; blocked targets are never fetchedYUNJIN_ALLOW_PRIVATE_FEED_HOSTSopt-out_validate_feed_url's docstringCloses #2
WuMing
Found 4 issue(s). See inline comments below.
@ -20,0 +32,4 @@User-Agent string to send with feed fetch requests."""return os.environ.get("YUNJIN_HTTP_USER_AGENT", _DEFAULT_USER_AGENT)code [LOW]
If YUNJIN_HTTP_USER_AGENT is set but empty (a common misconfiguration), os.environ.get returns an empty string and fetch_feed sends an empty User-Agent, which may still be rejected with 403. Use
os.environ.get("YUNJIN_HTTP_USER_AGENT") or _DEFAULT_USER_AGENTso empty values fall back to the default.@ -43,2 +60,3 @@logger.info(f"Fetching feed: {feed.url}")response = httpx2.get(feed.url, timeout=30.0, follow_redirects=True)response = httpx2.get(feed.url,security [HIGH]
A10: The feed URL is passed directly to httpx2.get without allow-list validation. If untrusted users can create feeds, this permits SSRF to internal services and cloud metadata. Validate URL scheme/host and block private IP ranges.
@ -75,0 +93,4 @@fetch_feed(db_conn, feed)headers = mock_get.call_args.kwargs["headers"]assert headers["User-Agent"].startswith("Mozilla/5.0")code [MEDIUM]
This default-header test does not isolate YUNJIN_HTTP_USER_AGENT, so if that environment variable is set in CI or locally, the assertion will fail. Also, startswith only verifies a prefix, so it would not catch an accidental change to the rest of the verified UA string. Patch/clear the env var for this test and assert equality to the expected default (e.g. import _DEFAULT_USER_AGENT).
tests [LOW]
The test only asserts the User-Agent starts with 'Mozilla/5.0', a loose check that passes for any browser-like string (e.g. Chrome, an older Firefox version, or another Mozilla UA). Since the PR's whole premise is that one specific probe-verified Firefox string is required to avoid the 403 from thedriven.io, a regression that swaps in a different browser UA would not be caught. Assert against the exact expected value (or the
_DEFAULT_USER_AGENTconstant) so the verified string is locked in.tests/test_fetcher.py✅ Fixed in commit
f9b1172— the test now runs underpatch.dict(os.environ, …, clear=True)withYUNJIN_HTTP_USER_AGENTremoved, and asserts equality against the imported_DEFAULT_USER_AGENTinstead of astartswithprefix check. This also resolves the duplicate finding on the loose prefix assertion.tests/test_fetcher.py✅ Fixed in commit
f9b1172— same finding as the duplicate comment above; the assertion is now exact equality against_DEFAULT_USER_AGENT, so the probe-verified Firefox string is locked in.src/yunjin/services/fetcher.py✅ Fixed in commit
15952f7—_user_agent()now usesos.environ.get("YUNJIN_HTTP_USER_AGENT") or _DEFAULT_USER_AGENT, with a regression test asserting the fallback for an empty value.src/yunjin/services/fetcher.py⏸️ Escalated to Marvin8 for a threat-model decision rather than fixed in this PR: yunjin is currently single-operator (feeds are added via the CLI and OPML import; there is no untrusted-user feed-creation path), the SSRF surface predates this PR, and blocking private ranges would break a legitimate deployment case — fetching feeds from LAN hosts such as a self-hosted FreshRSS. A decision on scope (scheme allow-list only, or full private-IP blocking with an opt-out) is pending.
WuMing
Found 4 issue(s). See inline comments below.
@ -20,0 +33,4 @@User-Agent string to send with feed fetch requests."""return os.environ.get("YUNJIN_HTTP_USER_AGENT") or _DEFAULT_USER_AGENTcode [LOW]
The fallback only treats an empty string as missing. A whitespace-only value such as ' ' is truthy and will be returned as the User-Agent, likely causing requests to be rejected. Consider stripping the value before applying the fallback: os.environ.get('YUNJIN_HTTP_USER_AGENT', '').strip() or _DEFAULT_USER_AGENT.
@ -43,2 +61,3 @@logger.info(f"Fetching feed: {feed.url}")response = httpx2.get(feed.url, timeout=30.0, follow_redirects=True)response = httpx2.get(feed.url,security [HIGH]
A10: feed.url is passed directly to httpx2.get without an allow-list or SSRF protection. If the feed URL is user-controlled, this permits requests to internal services or cloud metadata endpoints. Validate schemes and resolve/block private or link-local addresses, or route through an egress proxy.
@ -75,0 +91,4 @@env_without_override = os.environ.copy()env_without_override.pop("YUNJIN_HTTP_USER_AGENT", None)with patch.dict(os.environ, env_without_override, clear=True):code [LOW]
Using patch.dict(os.environ, env_without_override, clear=True) clears the entire environment for the duration of the test. This is more invasive than necessary and can cause flaky behavior if any code under test or a library depends on other environment variables. Prefer a targeted patch that only overrides/removes YUNJIN_HTTP_USER_AGENT, or use pytest's monkeypatch.delenv.
@ -75,0 +97,4 @@fetch_feed(db_conn, feed)headers = mock_get.call_args.kwargs["headers"]assert headers["User-Agent"] == _DEFAULT_USER_AGENTtests [LOW]
The assertion compares against
_DEFAULT_USER_AGENTimported from the module under test, so it is self-referential with respect to the constant: if_DEFAULT_USER_AGENTis changed to a non-browser or broken value, this test still passes. Assert against an independent expected value (e.g. a literal string or a check that it containsMozilla/5.0andFirefox), and keep the_DEFAULT_USER_AGENTcomparison only for the fallback/override tests where the constant is genuinely the expected outcome.src/yunjin/services/fetcher.py✅ Fixed in commit
224f2b6—_user_agent()now usesos.environ.get("YUNJIN_HTTP_USER_AGENT", "").strip() or _DEFAULT_USER_AGENT, with a regression test for a whitespace-only value falling back to the default.tests/test_fetcher.py✅ Fixed in commit
224f2b6— the default-header test now usesmonkeypatch.delenv("YUNJIN_HTTP_USER_AGENT", raising=False)instead of the environment-widepatch.dict(clear=True).tests/test_fetcher.py✅ Fixed in commit
224f2b6— the tests now assert against an independent literal copy of the probe-verified string defined in the test class (with a comment citing the issue #2 probe), so a broken or non-browser default would fail loudly and force re-verification.src/yunjin/services/fetcher.py⏸️ Still escalated to Marvin8 for a threat-model decision (see my reply to the earlier report of this finding): yunjin is single-operator with no untrusted feed-creation path, the surface predates this PR, and private-range blocking would break LAN feed sources. Not actioned in this PR without a product call.
WuMing
Found 1 issue(s). See inline comments below.
@ -43,2 +62,3 @@logger.info(f"Fetching feed: {feed.url}")response = httpx2.get(feed.url, timeout=30.0, follow_redirects=True)response = httpx2.get(feed.url,security [HIGH]
A10: feed.url is passed directly to httpx2.get without an allow-list or SSRF guard. If feed URLs are user-controlled, an attacker can target internal services or cloud metadata endpoints. Restrict schemes and validate/resolve hosts against private, loopback, and link-local ranges.
src/yunjin/services/fetcher.py⏸️ Same finding as my two earlier ⏸️ replies — escalated to Marvin8 for a threat-model decision (single-operator tool, LAN feed sources are a legitimate case). All other findings from waves 1–2 are fixed; this one is intentionally not actioned in this PR without a product call.
src/yunjin/services/fetcher.py✅ Fixed in commit
4b26273, per the maintainer-approved approach:_validate_feed_url()now runs before every fetch —http/httpsscheme allow-list (always), host resolution with blocking of private, loopback, link-local, reserved, unspecified, and multicast addresses (IPv4-mapped IPv6 unwrapped, mixed results rejected, unresolvable hosts fail closed). Opt-out for LAN feed sources viaYUNJIN_ALLOW_PRIVATE_FEED_HOSTS. Known limitations (documented in the docstring): redirects are followed without re-validation, and validation is TOCTOU vs. the client's own resolution; closing those needs a custom transport and is out of scope here. 18 new tests, all with mocked resolution.WuMing
Found 5 issue(s). See inline comments below.
@ -45,0 +157,4 @@response = httpx2.get(feed.url,timeout=30.0,follow_redirects=True,security [HIGH]
A10: Redirects are followed without re-applying the SSRF guard. An attacker-controlled feed URL can pass validation on a public host and then redirect httpx to an internal address such as 169.254.169.254 or a LAN service. Re-validate every redirect target or disable redirects and handle them manually with the same scheme and address checks.
@ -39,3 +151,4 @@"""now = datetime.now()_validate_feed_url(feed.url)security [HIGH]
A10: The SSRF guard resolves the hostname during validation, but the actual HTTP request resolves it again separately. An attacker controlling DNS can return a public address during validation and a private/reserved address when httpx fetches it (DNS rebinding), bypassing the guard. Pin the validated IP address(es) and issue the request using those addresses.
@ -72,6 +82,111 @@ class TestFetchFeed:saved_articles = articles.get_articles_by_feed(db_conn, feed.id)assert len(saved_articles) == 2def test_fetch_feed_sends_user_agent_header(self, db_conn, monkeypatch):tests [MEDIUM]
This test (and the three User-Agent tests that follow at lines 108, 131 and 154) mock
httpx2.getandfeedparser.parsebut never mock DNS, whilefetch_feednow calls_validate_feed_url, which performs a realsocket.getaddrinfo("example.com"). The tests therefore depend on external DNS/network availability and will error withFetchError: ... unresolvable hostin an offline or sandboxed CI, which makes them flaky. Patchsocket.getaddrinfo(asTestFeedUrlValidationdoes) with a public address for these cases.@ -299,0 +430,4 @@return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (ip_address, 0))]return [(socket.AF_INET6, socket.SOCK_STREAM, 6, "", (ip_address, 0, 0, 0))]def test_allows_public_address(self):tests [LOW]
test_allows_public_addresscontains no assertion statement: it only calls_validate_feed_urland relies on the absence of an exception. The test would still pass if validation were silently turned into a no-op, so it cannot distinguish "allowed" from "not validated at all". Assert that the guard actually ran, e.g. by asserting thesocket.getaddrinfopatch was called with the expected hostname, and/or assert the function returns None.@ -299,0 +479,4 @@with pytest.raises(FetchError, match="unresolvable"):_validate_feed_url("https://example.invalid/feed.xml")def test_opt_out_allows_private_host(self):tests [LOW]
test_opt_out_allows_private_hosthas no assertion — it only relies on_validate_feed_urlnot raising inside the patch contexts. Because_validate_feed_urlreturnsNoneand the opt-out path returns early, the test would pass even if the opt-out were removed and the address were treated as public. Add an explicit check, e.g. assert the patchedsocket.getaddrinfowas called (proving the opt-out bypassed resolution) or that the call returnedNone.tests/test_fetcher.py✅ Fixed in commit
a6674a5— the test now assertssocket.getaddrinfowas called once with("example.com", None), proving the guard ran rather than silently no-oping.tests/test_fetcher.py✅ Fixed in commit
a6674a5— with a correction to the suggested assertion: the opt-out path returns before resolution, so the meaningful check ismock_getaddrinfo.assert_not_called()— it proves the opt-out short-circuited validation instead of the host merely passing validation.tests/test_fetcher.py✅ Fixed in commit
a6674a5—TestFetchFeednow has an autousepublic_dnsfixture that patchessocket.getaddrinfoto a fixed public address for every test in the class (the private-URL test overrides it locally), so no fetch test touches live DNS.src/yunjin/services/fetcher.py⏸️ Escalated to Marvin8: closing the rebinding window requires pinning the validated address at the transport layer (httpx2's top-level
get()accepts no transport; this means switchingfetch_feedto aClientwith a custom transport that rewrites connections to the validated IP while preserving SNI/Host). This is the TOCTOU limitation documented in_validate_feed_url's docstring and in the maintainer-approved scope decision — for a single-operator aggregator the attacker must already control DNS for a feed hostname the operator chose to add. Decision pending: implement transport pinning vs. accept the documented limitation.src/yunjin/services/fetcher.py⏸️ Escalated to Marvin8 alongside the rebinding finding — this is the second documented limitation in
_validate_feed_url's docstring. The fix shape (disable auto-redirects, loop manually withurljoin-resolved targets through the same guard) is well understood and cheaper than transport pinning; awaiting the maintainer's call on whether it lands in this PR or a follow-up.src/yunjin/services/fetcher.py✅ Fixed in commit
bd78da3—_fetch_feed_response()now follows redirects manually (follow_redirects=False), resolving eachLocationwithurljoinand re-running the full_validate_feed_urlguard before every hop; missingLocationheaders and chains beyond_MAX_FEED_REDIRECTS(5) raiseFetchError. Redirect targets resolving to blocked addresses are never fetched (asserted by test).src/yunjin/services/fetcher.py🔴 Declined by maintainer decision — accepted as a documented limitation. Pinning the validated address requires moving
fetch_feedonto ahttpx2.Clientwith a custom transport that rewrites connections to the validated IP while preserving SNI/Host (the top-levelget()accepts no transport), which is disproportionate for this project: yunjin is a single-operator aggregator, so exploiting the rebinding window requires controlling DNS for a feed hostname the operator deliberately added. The limitation and its rationale are documented in_validate_feed_url's docstring; it can be revisited in a dedicated issue if the deployment model changes.WuMing
Found 1 issue(s). See inline comments below.
@ -27,0 +159,4 @@"""headers = {"User-Agent": _user_agent()}for _redirect_number in range(_MAX_FEED_REDIRECTS + 1):response = httpx2.get(url, timeout=30.0, follow_redirects=False, headers=headers)security [MEDIUM]
A10: The SSRF guard resolves the hostname in _validate_feed_url, but httpx2.get resolves it independently when opening the connection. A DNS rebinding attack can return a public address during validation and a private/link-local address during the actual request, bypassing the guard and reaching internal services or cloud metadata endpoints. Remediate by pinning the HTTP client to the validated IP addresses or using a custom transport that resolves and connects with the same result.
src/yunjin/services/fetcher.py⏸️ Standing maintainer decision — see the 🔴 reply to the earlier report of this finding: declined as a documented limitation (transport-level pinning is disproportionate for a single-operator aggregator; the limitation and rationale are in
_validate_feed_url's docstring and can be revisited in a dedicated issue if the deployment model changes). Noting the severity was re-assessed HIGH → MEDIUM; no further action planned on this PR. This finding will re-appear on each new push since WuMing has no wontfix marker.