fix(security): block SSRF to internal/metadata hosts in webhook URLs (#656) (#706)

* fix(security): block SSRF to internal/metadata hosts in webhook URLs (#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.

* test(security): harden #656 SSRF tests; reword docstring

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:'.

* docs: document CODEFRAME_ALLOW_PRIVATE_WEBHOOKS env var (#656)

Author: frankbria

CLAUDE.md

@@ -304,6 +304,15 @@ REDIS_URL=redis://localhost:6379
 
 CODEFRAME_API_KEY_SECRET=<secret>     # API key hashing
 
+# Outbound webhook SSRF guard (#656) — default OFF (block)
+CODEFRAME_ALLOW_PRIVATE_WEBHOOKS=1    # Allow webhook URLs whose host resolves to
+                                      # private/loopback/link-local/metadata IPs.
+                                      # Off by default: such hosts are rejected by
+                                      # the notifications save + test endpoints to
+                                      # prevent SSRF (e.g. 169.254.169.254 IMDS).
+                                      # Set for self-hosted ops with legit internal
+                                      # webhook targets (localhost, RFC1918).
+
 # Telemetry (default: off — must be explicitly opted in)
 CODEFRAME_TELEMETRY=on|off            # Force telemetry on or off; overrides ~/.codeframe/telemetry.json
 CODEFRAME_TELEMETRY_ENDPOINT=<url>    # Override collector URL (default: https://telemetry.codeframe.dev/v1/events)

codeframe/notifications/webhook.py

@@ -260,6 +260,10 @@ async def send_event(
                     target_url,
                     json=payload,
                     timeout=aiohttp.ClientTimeout(total=self.timeout),
+                    # SSRF (#656): the URL host is validated before saving, but
+                    # a public target could 302 → 169.254.169.254 / localhost.
+                    # Webhook delivery never needs to follow redirects, so don't.
+                    allow_redirects=False,
                 ) as response:
                     ok = 200 <= response.status < 300
                     if not ok:

codeframe/ui/routers/settings_v2.py

@@ -16,7 +16,10 @@
 config is per-workspace and persisted under .codeframe/notifications_config.json.
 """
 
+import ipaddress
 import logging
+import os
+import socket
 
 from typing import Optional, cast
 
@@ -447,13 +450,85 @@ async def get_notification_settings(
 _ALLOWED_WEBHOOK_SCHEMES = frozenset({"http", "https"})
 
 
+def _allow_private_webhook_hosts() -> bool:
+    """Opt-out for the SSRF host check.
+
+    Off by default (block private/internal targets). Self-hosted operators who
+    legitimately point webhooks at ``localhost`` or an internal service set
+    ``CODEFRAME_ALLOW_PRIVATE_WEBHOOKS=1`` — read at request time so it can be
+    toggled without a restart.
+    """
+    return os.getenv("CODEFRAME_ALLOW_PRIVATE_WEBHOOKS", "").strip().lower() in {
+        "1",
+        "true",
+        "yes",
+        "on",
+    }
+
+
+def _assert_webhook_host_is_public(hostname: str) -> None:
+    """Reject webhook hosts that resolve to internal/metadata/private IPs.
+
+    Blocks the SSRF vector where an authenticated user points the webhook at
+    ``169.254.169.254`` (cloud IMDS), ``127.0.0.1``, or an RFC1918 address —
+    the ``/notifications/test`` endpoint fires the URL immediately and returns
+    the HTTP status, a blind-to-semi-blind SSRF primitive. IP literals are
+    checked directly; DNS names are resolved and every returned address is
+    checked. A host that cannot be resolved is allowed through — it isn't
+    reachable right now, and we don't want to block saving a not-yet-live URL.
+
+    Known limitation: resolve-and-check, not a request-time pinned connector,
+    so DNS rebinding (public at save, private at fire) is out of scope.
+    Upgrade path: pin the resolved IP through a custom aiohttp connector in
+    ``send_event``.
+    """
+    if _allow_private_webhook_hosts():
+        return
+
+    try:
+        candidates = [ipaddress.ip_address(hostname)]
+    except ValueError:
+        try:
+            candidates = [
+                ipaddress.ip_address(info[4][0])
+                for info in socket.getaddrinfo(
+                    hostname, None, proto=socket.IPPROTO_TCP
+                )
+            ]
+        except socket.gaierror:
+            return  # unresolvable now → not reachable now; don't block the save
+
+    for ip in candidates:
+        # ::ffff:169.254.169.254 — judge by the embedded IPv4 address.
+        if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped is not None:
+            ip = ip.ipv4_mapped
+        if (
+            ip.is_private
+            or ip.is_loopback
+            or ip.is_link_local
+            or ip.is_reserved
+            or ip.is_multicast
+            or ip.is_unspecified
+        ):
+            raise HTTPException(
+                status_code=400,
+                detail=api_error(
+                    f"Webhook host resolves to a private/internal address "
+                    f"({ip}); refusing to avoid SSRF. Set "
+                    f"CODEFRAME_ALLOW_PRIVATE_WEBHOOKS=1 to allow internal targets.",
+                    ErrorCodes.VALIDATION_ERROR,
+                ),
+            )
+
+
 def _validate_webhook_url(url: Optional[str]) -> Optional[str]:
     """Normalize and validate a user-supplied webhook URL.
 
     Returns the trimmed URL on success, or ``None`` if empty. Raises
     ``HTTPException(400)`` for non-``http(s)`` schemes — without this guard,
     a user could enter ``file:///...`` or ``ftp://...`` and aiohttp would
-    happily attempt the request (SSRF on local resources).
+    happily attempt the request (SSRF on local resources) — and for hosts that
+    resolve to private/internal/metadata IPs (see ``_assert_webhook_host_is_public``).
     """
     from urllib.parse import urlparse
 
@@ -477,6 +552,8 @@ def _validate_webhook_url(url: Optional[str]) -> Optional[str]:
                 ErrorCodes.VALIDATION_ERROR,
             ),
         )
+    if parsed.hostname:
+        _assert_webhook_host_is_public(parsed.hostname)
     return trimmed
 
 
@@ -494,7 +571,8 @@ async def update_notification_settings(
     the saved URL. The URL is validated for ``http(s)`` scheme to avoid
     ``file://`` / ``ftp://`` SSRF on local resources.
     """
-    url = _validate_webhook_url(body.webhook_url)
+    # Off the event loop: _validate_webhook_url may do a blocking DNS lookup.
+    url = await run_in_threadpool(_validate_webhook_url, body.webhook_url)
     try:
         save_notifications_config(
             workspace,
@@ -553,7 +631,8 @@ async def test_notification_webhook(
     # We discard the return value: ``_validate_webhook_url`` raises
     # ``HTTPException(400)`` on a bad scheme/host, which is the side-effect
     # we want here. The trimmed URL it returns is already in ``url``.
-    _validate_webhook_url(url)
+    # Off the event loop — it may do a blocking DNS lookup.
+    await run_in_threadpool(_validate_webhook_url, url)
 
     svc = WebhookNotificationService(webhook_url=url, timeout=5)
     result = await svc.send_event(format_test_payload())

tests/notifications/test_webhook_send_event.py

@@ -55,6 +55,17 @@ async def test_send_event_returns_not_ok_on_5xx():
     assert result.status_code == 500
 
 
+@pytest.mark.asyncio
+async def test_send_event_does_not_follow_redirects():
+    """SSRF (#656): redirects must not be followed — a public target could
+    302 → 169.254.169.254 / localhost and bypass the save-time host check."""
+    svc = WebhookNotificationService(webhook_url="https://example.com/hook", timeout=5)
+    session = _mock_post(200)
+    with patch("aiohttp.ClientSession", return_value=session):
+        await svc.send_event(format_test_payload())
+    assert session.post.call_args.kwargs["allow_redirects"] is False
+
+
 @pytest.mark.asyncio
 async def test_send_event_no_url_returns_error():
     svc = WebhookNotificationService(webhook_url=None, timeout=5)

tests/ui/test_settings_notifications.py

@@ -136,13 +136,88 @@ def test_accepts_https(self, client):
         assert r.status_code == 200
 
     def test_accepts_http(self, client):
-        """Plain http is allowed for local testing / internal endpoints."""
+        """Plain http is allowed for public endpoints. Uses a global IP literal
+        (no DNS lookup) so the test is hermetic — TEST-NET ranges count as
+        private under ipaddress, so a real routable global IP is used."""
         r = client.put(
             "/api/v2/settings/notifications",
-            json={"webhook_url": "http://localhost:9876/h", "webhook_enabled": True},
+            json={"webhook_url": "http://8.8.8.8:9876/h", "webhook_enabled": True},
         )
         assert r.status_code == 200
 
+    # --- SSRF host validation (#656) ---------------------------------------
+
+    def test_rejects_metadata_ip(self, client):
+        """Cloud IMDS (169.254.169.254) must be rejected — the core SSRF target."""
+        r = client.put(
+            "/api/v2/settings/notifications",
+            json={
+                "webhook_url": "http://169.254.169.254/latest/meta-data/",
+                "webhook_enabled": True,
+            },
+        )
+        assert r.status_code == 400
+
+    def test_rejects_localhost(self, client):
+        """A DNS name resolving to loopback → blocked. getaddrinfo is mocked so
+        the test doesn't depend on how the CI image resolves ``localhost``
+        (some return ::1, some 127.0.0.1, hardened ones not at all)."""
+        with patch(
+            "codeframe.ui.routers.settings_v2.socket.getaddrinfo",
+            return_value=[(2, 1, 6, "", ("127.0.0.1", 0))],
+        ):
+            r = client.put(
+                "/api/v2/settings/notifications",
+                json={
+                    "webhook_url": "http://localhost:9876/h",
+                    "webhook_enabled": True,
+                },
+            )
+        assert r.status_code == 400
+
+    def test_rejects_ipv4_mapped_ipv6_metadata(self, client):
+        """::ffff:169.254.169.254 must be unwrapped to its IPv4 and rejected —
+        the trickiest case, easy to silently break in a refactor."""
+        r = client.put(
+            "/api/v2/settings/notifications",
+            json={
+                "webhook_url": "http://[::ffff:169.254.169.254]/h",
+                "webhook_enabled": True,
+            },
+        )
+        assert r.status_code == 400
+
+    def test_rejects_loopback_ip(self, client):
+        r = client.put(
+            "/api/v2/settings/notifications",
+            json={"webhook_url": "http://127.0.0.1:8000/h", "webhook_enabled": True},
+        )
+        assert r.status_code == 400
+
+    def test_rejects_rfc1918_ip(self, client):
+        r = client.put(
+            "/api/v2/settings/notifications",
+            json={"webhook_url": "http://10.0.0.5/h", "webhook_enabled": True},
+        )
+        assert r.status_code == 400
+
+    def test_rejects_ipv6_loopback(self, client):
+        r = client.put(
+            "/api/v2/settings/notifications",
+            json={"webhook_url": "http://[::1]:8000/h", "webhook_enabled": True},
+        )
+        assert r.status_code == 400
+
+    def test_allows_private_host_when_opted_in(self, client, monkeypatch):
+        """CODEFRAME_ALLOW_PRIVATE_WEBHOOKS=1 is the documented escape hatch."""
+        monkeypatch.setenv("CODEFRAME_ALLOW_PRIVATE_WEBHOOKS", "1")
+        for host in ("http://127.0.0.1:8000/h", "http://10.0.0.5/h"):
+            r = client.put(
+                "/api/v2/settings/notifications",
+                json={"webhook_url": host, "webhook_enabled": True},
+            )
+            assert r.status_code == 200, host
+
 
 class TestNotificationWebhookTest:
     def test_returns_400_when_no_url_configured(self, client):
@@ -218,6 +293,17 @@ def test_rejects_unsafe_stored_url_at_test_time(self, client, workspace):
         r = client.post("/api/v2/settings/notifications/test")
         assert r.status_code == 400
 
+    def test_rejects_metadata_ip_at_test_time(self, client, workspace):
+        """SSRF (#656): a hand-edited config pointing at IMDS must be refused
+        by /test, not fired."""
+        path = workspace.state_dir / "notifications_config.json"
+        path.write_text(
+            '{"webhook_url": "http://169.254.169.254/latest/meta-data/", '
+            '"webhook_enabled": true}'
+        )
+        r = client.post("/api/v2/settings/notifications/test")
+        assert r.status_code == 400
+
     def test_test_works_even_when_enabled_flag_is_false(self, client):
         """The Test button should still work when the user is verifying
         a URL before turning notifications on."""