Fix: DNS-based SSRF bypass in guardrail webhook URL validation

[Deez-Automations]

Summary

The Labs guardrail feature lets a session register a webhook_url that the server later POSTs to (GuardrailHookService.invoke, finbot/guardrails/service.py) during hook invocations.

A note on the linked issue first: it states there's no validation at all and suggests importing is_ssrf_safe() from finbot/apps/ctf/routes/profile.py. That function doesn't exist in the current codebase (it would come from #507, which is still open/unmerged) — worth flagging so nobody goes looking for it. There is existing validation already wired in: validate_webhook_url() in finbot/core/data/repositories.py is already called from LabsGuardrailConfigRepository.upsert() and already wrapped in a 422 response. It has its own passing test suite with an intentional, documented contract: in DEBUG mode (default for local dev), private/loopback addresses are allowed so contributors can test webhooks against local servers; in production (DEBUG=False), they're blocked via a hardcoded network list.

The actual gap: that check only ever inspected the hostname via ipaddress.ip_address(hostname), which only succeeds for a bare IP literal typed directly into the URL. Any normal hostname raises ValueError, which was silently swallowed — so a hostname that isn't itself an IP literal but resolves via DNS to 127.0.0.1, 169.254.169.254 (cloud metadata), or any RFC1918 range was never checked, in either DEBUG or production mode. That's the real, exploitable bypass this PR closes.

Comparison against another open fix for the same issue

Another contributor has an independent, already-open PR (#538) for this same issue, and this comparison isn't a clean "ours is better" — it genuinely isn't, on its own. The two PRs found different, non-overlapping halves of the same vulnerability:

• This PR originally fixed only the DNS-resolution bypass in validate_webhook_url() above (plus a self-caught event-loop DoS, see below). It did not touch endpoint authentication.
• fix(security): Block SSRF in guardrail webhook URL and require authentication on write endpoints #538 found and fixed something this PR's first pass missed entirely: all 5 write endpoints (PUT, POST /toggle, POST /rotate-secret, DELETE, POST /test) used get_session_context, which accepts anonymous temporary sessions — meaning an unauthenticated visitor could register a webhook and, via /test, trigger an immediate outbound request to it. That's a second, independent path into the same "unauthenticated SSRF" issue, and it was still true on main as of this PR's last commit before this update.

Rather than leave that gap open and just note it, I've added the authentication fix from #538's approach into this PR too (see "Also added" below), so this PR now closes both halves. Credit to #538 for finding the half this PR missed — flagging clearly rather than quietly absorbing it.

Fix

• validate_webhook_url(): when the hostname isn't a literal IP, it's now resolved and every returned address is checked against the blocked-ranges list (IPv4-mapped IPv6 addresses are unwrapped first). The existing DEBUG-mode carve-out is untouched — same gating, same intentional behavior, confirmed by a new test that DNS resolution is never even triggered in DEBUG mode.
• Added 100.64.0.0/10 (RFC 6598 carrier-grade NAT, used internally by several cloud providers) and ::/128 to the blocked list.
• Fixed an uncaught UnicodeError on malformed/overlong hostnames (socket.getaddrinfo raises UnicodeEncodeError for those, not socket.gaierror — confirmed directly, not assumed).
• DNS resolution is synchronous and unbounded, and invoke() runs on every guardrail hook call. Calling it inline there would let an attacker-controlled DNS target block the event loop for every concurrent session, not just the one making the request. Added validate_webhook_url_async(), which offloads resolution to a worker thread via asyncio.to_thread bounded by asyncio.wait_for(timeout=2s), and wired it into both the registration route and invoke() (the latter as defense-in-depth re-validation immediately before the actual request, narrowing the DNS-rebinding TOCTOU window rather than fully closing it — full closure would mean pinning the resolved IP and connecting directly to it instead of letting httpx re-resolve independently, which felt like a larger, separate change).
• LabsGuardrailConfigRepository.upsert() gained a skip_url_validation flag so the route (which now validates asynchronously first) doesn't also run the internal synchronous check redundantly — that would've meant a second, unbounded DNS resolution performed while holding an open DB session, which is worse than what this PR fixes, not better. Default behavior (no flag) is unchanged for any other caller.

Also added: authentication on write endpoints

All 5 write endpoints now use get_authenticated_session_context instead of get_session_context, matching the pattern this codebase already uses elsewhere (finbot/apps/ctf/routes/profile.py's own GET/PUT split). The 2 read-only endpoints (GET, GET /activity) stay anonymous-accessible — no state-changing side effect. 10 new tests in test_guardrail_route_security.py confirm all 5 write endpoints return 401 for an anonymous session, both read endpoints still allow one, and 3 endpoints still work correctly for a genuinely authenticated session.

Review process

This went through two full review passes before opening the PR, and the first one caught a real problem in my own patch worth being upfront about: the initial version of the DNS-resolution fix called socket.getaddrinfo synchronously, inline, directly inside invoke(). Since invoke() fires on every guardrail hook call — a hot path, not an occasional one — that would have let an attacker register a webhook pointing at an unresponsive DNS target and freeze the entire event loop for every concurrent session on that worker, for as long as the OS resolver takes to give up (often 15-30s+). That's a self-contained HIGH-severity DoS, introduced in the course of fixing the SSRF, not present before this PR and not something I'd anticipated going in.

The asyncio.to_thread + asyncio.wait_for offload described above, the skip_url_validation flag, the UnicodeError fix, and the test_does_not_block_the_event_loop test that actually proves non-blocking behavior (rather than just asserting a mock was called) all came out of catching and closing that specific finding. A second review pass confirmed it was fixed correctly and surfaced no new blocking issues.

Known residual gaps, noted rather than silently left

• The thread offload uses the process's default ThreadPoolExecutor (there's no dedicated pool configured anywhere in this codebase). Under sustained abuse — many concurrent registrations/invocations pointed at an unresponsive DNS target — the shared pool could still saturate, which would affect other unrelated asyncio.to_thread/run_in_executor consumers in the app. The event loop itself is genuinely protected (that was the primary risk), but a dedicated bounded executor for this specific check would close the second-order resource-exhaustion angle. Didn't want to bundle a broader executor-management change into this fix without discussion, but happy to follow up if that's wanted.
• The defense-in-depth re-check in invoke() narrows the DNS-rebinding window but doesn't fully close it (two independent getaddrinfo calls, milliseconds apart, both attacker-influenceable via a low-TTL record). Full closure needs IP-pinning (resolve once, connect to that address directly) rather than re-validating and letting httpx re-resolve on its own — scoping that as a possible follow-up rather than folding it into this PR.

Test plan

• Extended the existing tests/unit/labs/test_guardrail_config.py::TestValidateWebhookUrl with cases for: hostname resolving to a blocked/metadata IP, mixed safe+unsafe resolved addresses, a hostname resolving to a genuinely public IP, an unresolvable hostname, IPv4-mapped IPv6, an overlong/malformed hostname, and confirmation that DNS resolution never fires in DEBUG mode
• New TestValidateWebhookUrlAsync class: correct delegation for safe/unsafe URLs, a timeout test, and a test that actually proves non-blocking behavior (runs a slow mocked resolution concurrently with a trivial coroutine via asyncio.gather and asserts the trivial one completes first)
• New test for skip_url_validation in both directions (default still validates; explicit skip bypasses)
• New test in test_guardrail_service.py confirming invoke() rejects a webhook resolving to a blocked address and never calls httpx.AsyncClient.post
• 10 new tests in test_guardrail_route_security.py for the authentication fix (see above)
• pytest tests/unit/labs/ -q — 99/99 passing
• Locally merge-tested solo against fresh origin/main: clean
• Locally merge-tested pairwise against Fix: orchestrator confirms payment to vendor even when it fails #574, Fix: guardrail payload corruption before signing + missing after_tool for complete_task #576, Fix: Dark Lab tool override silently drops parameters/inputSchema #577, Fix: profile share-card cache key omits fields that affect the render #578, fix: guard vendor_id/invoice_id=None in chat assistant tool callables (#418) #579, fix: reject whitespace-only task_summary in orchestrator context capture (#459) #580: clean, except a real, expected overlap with Fix: guardrail payload corruption before signing + missing after_tool for complete_task #576 — both PRs independently add a new test method at the same line in tests/unit/labs/test_guardrail_service.py::TestWebhookInvocation (this PR's test_rejects_webhook_url_resolving_to_blocked_address, Fix: guardrail payload corruption before signing + missing after_tool for complete_task #576's test_invoke_never_raises_even_on_internal_failure). Not a logical conflict — both tests are valid and independent, resolved by keeping both. Flagging for whoever merges second, so it's expected rather than surprising.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.