fix(security): block SSRF to internal/metadata hosts in webhook URLs (#656)#706
Conversation
…656) Webhook URL validation checked the scheme only. An authenticated user could point the outbound webhook at http://169.254.169.254 (cloud IMDS), http://localhost, or any RFC1918 address; the /notifications/test endpoint fires it immediately and returns the HTTP status — a blind-to-semi-blind SSRF primitive. - _validate_webhook_url now resolves the host and rejects loopback, link-local, RFC1918/private, reserved, multicast and unspecified addresses (IPv4, IPv6, and IPv4-mapped IPv6). Covers both save (PUT) and test (POST). - Opt-out via CODEFRAME_ALLOW_PRIVATE_WEBHOOKS=1 for self-hosted operators with legitimate internal webhook targets. - send_event no longer follows redirects (allow_redirects=False) — closes the public-target-302-to-metadata bypass. - Host resolution runs off the event loop (run_in_threadpool) so a slow resolver can't stall request handling. Known limitation: resolve-and-check at save/test time, not a request-time pinned connector, so DNS rebinding (public at save, private at fire) is out of scope; redirect-blocking mitigates the most practical variant.
|
Warning Review limit reached
More reviews will be available in 5 minutes and 21 seconds. Learn how PR review limits work. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Code Review: fix(security): SSRF host validation for webhook URLs. This is a well-scoped security fix. The threat model is clearly stated, the defense-in-depth approach (host validation + redirect blocking) is correct, and limitations are honestly documented. WHAT IS SOLID: (1) Defense in depth: host validation at save/test time AND allow_redirects=False at fire time — one without the other leaves a gap. (2) IPv4-mapped IPv6 handling: ip.ipv4_mapped unwrap for ::ffff:169.254.169.254 is easy to forget and is correctly included. (3) Blocking DNS off the event loop via run_in_threadpool in both PUT and POST handlers is correct. (4) CODEFRAME_ALLOW_PRIVATE_WEBHOOKS read at request time (no restart needed) is the right ergonomic for self-hosted operators. ISSUES TO ADDRESS: (1) Missing test for IPv4-mapped IPv6 metadata IP. The code explicitly handles ::ffff:169.254.169.254 but there is no test for it. This is the trickiest case and the one most likely to be silently broken by a refactor. Please add test_rejects_ipv4_mapped_ipv6_metadata that PUTs http://[::ffff:169.254.169.254]/ and expects a 400. (2) test_rejects_localhost makes a real DNS lookup. localhost resolution is environment-dependent — some CI images return ::1, others 127.0.0.1, and hardened setups may not resolve it at all (which would allow it through under the current unresolvable-means-not-reachable policy). The IP literal tests already cover loopback. Either remove the localhost test or mock socket.getaddrinfo. (3) The 400 error body advertises CODEFRAME_ALLOW_PRIVATE_WEBHOOKS=1 as the bypass. Fine for self-hosted, but operators who expose this API externally are advertising the escape hatch. Worth a conscious decision — the hint may belong in logs or docs. MINOR / NON-BLOCKING: (4) ponytail: in the docstring is non-standard — suggest Upgrade path: or Known limitation: to match codebase conventions. (5) test_accepts_http now uses example.com which incurs a live DNS lookup in tests — a routable IP literal like 203.0.113.1 (TEST-NET-3) avoids CI network-restriction risk. (6) test_allows_private_host_when_opted_in only covers 127.0.0.1 — a second variant with RFC1918 or link-local would confirm the bypass is general. SUMMARY: Approve with item 1 added. Items 2 and 5 are latent CI flakiness risks. Items 3, 4, 6 are low-priority. |
Address advisory review feedback: - Add IPv4-mapped IPv6 metadata test (::ffff:169.254.169.254) — the trickiest case, previously only exercised in the demo. - Make the localhost test hermetic by mocking socket.getaddrinfo instead of depending on CI-image resolution of 'localhost'. - test_accepts_http uses a global IP literal (8.8.8.8) — no live DNS; TEST-NET ranges count as private under ipaddress so they can't serve as 'public'. - Opt-in test also covers an RFC1918 host, not just loopback. - Reword 'ponytail:' docstring note to 'Known limitation:'.
|
Follow-up review (continuing from my prior comment) All three issues I flagged as mandatory or CI-flakiness risks have been addressed in this revision. Prior issues resolved:
One small non-blocking observation: 8.8.8.8 is routable and correctly passes the is_private check, so the test works. If you want to make the intent explicit for future readers, a brief comment noting that TEST-NET-3 (203.0.113.x) would be the semantic choice but ipaddress marks it as private, so a real public IP is needed, would help — but the code is correct as-is. Overall: The implementation is sound: defense-in-depth (host validation at save and test time, plus redirect blocking at fire time), IPv4-mapped IPv6 unwrapping, blocking DNS lookup moved off the event loop, documented DNS-rebinding limitation, operator escape hatch. The test suite is now hermetic. Approve. All mandatory findings from the prior review are resolved. Ready to merge. |
|
Third-pass review (continuing from prior comments) The diff is unchanged from what I reviewed in my second pass — all mandatory items resolved, no new changes since approval. Status: No new findings. Prior approval stands. To summarize the review history for this PR:
The implementation is complete and sound. Ready to merge. |
1 participant
Closes #656.
Problem
Outbound-webhook URL validation checked the scheme only. An authenticated user could set the webhook to
http://169.254.169.254/...(cloud IMDS),http://localhost:8000/..., or any RFC1918 address — and/notifications/testfires it immediately and returns the HTTP status, giving a blind-to-semi-blind SSRF primitive against internal services and cloud metadata.Fix
_validate_webhook_url,settings_v2.py): after scheme/netloc checks, resolve the host and rejectis_private | is_loopback | is_link_local | is_reserved | is_multicast | is_unspecified— IPv4, IPv6, and IPv4-mapped IPv6 (::ffff:169.254.169.254). IP literals are checked directly; DNS names are resolved and every returned address checked. Both the save (PUT) and test (POST) endpoints flow through this, so both are covered.CODEFRAME_ALLOW_PRIVATE_WEBHOOKS=1for self-hosted operators with legitimate internal targets (the issue's "unless explicitly opted in"). Off by default.send_eventnow sendsallow_redirects=False— a public target can no longer302 → 169.254.169.254.run_in_threadpoolso a slow resolver can't stall request handling.Acceptance criteria
Tests
tests/ui/test_settings_notifications.py(save + test endpoints) andtests/notifications/test_webhook_send_event.py(no-redirect). 41 passing locally;ruffclean.Known limitation
Resolve-and-check at save/test time, not a request-time pinned connector — so DNS rebinding (host resolves public at save, private at fire) is out of scope. Redirect-blocking mitigates the most practical variant. Upgrade path noted in code: pin the resolved IP through a custom aiohttp connector in
send_event.Review
Cross-family review via
codex review; both findings (redirect bypass P1, blocking DNS P2) addressed in this PR.