fix(notifications): close DNS-rebinding SSRF in webhook delivery

[sjhddh]

Summary

• Security: The NotificationService had a TOCTOU SSRF vulnerability in its webhook delivery path. validate_config() resolved the hostname and checked it against blocked ranges, but then passed the original URL to urllib.request.urlopen() which performed a second, independent DNS lookup. An attacker controlling a domain with TTL=0 could resolve it to a public IP at validation time, then rebind the DNS to an internal address (e.g. 169.254.169.254 AWS metadata, 10.x.x.x, loopback) before the actual HTTP POST fires.

• Fix: The hostname is now resolved once in validate_config(), which returns the validated IP string. _post_once() receives that pre-validated IP and connects to it directly via http.client.HTTPConnection/HTTPSConnection, eliminating any second DNS query. For HTTPS, a raw TCP socket is opened to the IP and then TLS-wrapped with server_hostname=<original_hostname> so SNI and certificate validation still use the domain name.

Attack scenario (before fix)

1. Attacker registers evil.example.com with TTL=1, initially pointing to 203.0.113.1 (public IP).
2. Agent operator configures https://evil.example.com/webhook as a push notification URL.
3. validate_config() resolves evil.example.com → 203.0.113.1 ✅ passes block-list check.
4. Attacker flips DNS: evil.example.com → 169.254.169.254 (AWS IMDSv1).
5. urllib.request.urlopen() resolves again → hits instance metadata service 🔴.

Changes

• validate_config() now returns the resolved IP string (was None).
• New private helper _resolve_and_check_ip() centralises DNS resolution + block-list check.
• _post_once() accepts a resolved_ip parameter and connects to it directly.
• HTTPS path opens a raw TCP socket to the IP then wraps it with TLS using the original hostname for SNI/cert verification.
• Removed urllib import (no longer needed); added http.client and ssl.
• All existing unit tests for validate_config continue to pass (mock patches socket.getaddrinfo); _post_once tests use patch.object so the signature change is transparent.

Test plan

• tests/unit/utils/test_notifications.py — all existing tests pass without modification
• Verify validate_config({"url": "http://localhost/webhook"}) raises ValueError (loopback blocked)
• Verify validate_config({"url": "https://example.com/webhook"}) returns a public IP string
• Verify _post_once uses the returned IP and not the original hostname for TCP connection

🤖 Generated with Claude Code

Summary by CodeRabbit

• Refactor
  • Enhanced notification delivery reliability and security by implementing direct IP-based connections with improved hostname validation and blocked-range checking.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f1062a79-25f3-4138-ba09-8b3333df0ead

📥 Commits

Reviewing files that changed from the base of the PR and between 98cfad0 and f6af319.

📒 Files selected for processing (1)

• bindu/utils/notifications.py

---

📝 Walkthrough

Walkthrough

Refactored the notification delivery system to resolve hostnames upfront and bypass urllib in favor of direct socket connections with explicit IP targeting. DNS resolution now happens during config validation rather than during HTTP connection, with custom TLS and HTTP client handling replacing standard library approaches.

Changes

Cohort / File(s)

Summary

Notification Delivery Refactor bindu/utils/notifications.py

Added _resolve_and_check_ip() helper for hostname resolution and validation. Modified validate_config() to return resolved IP string instead of None. Updated send_event() to pass resolved IP through the delivery chain. Reworked _post_with_retry() and _post_once() to accept and utilize resolved_ip parameter. Replaced urllib.request.urlopen with direct socket connections: HTTPS uses http.client.HTTPSConnection with custom SNI wrapping over resolved IP; HTTP uses http.client.HTTPConnection. Updated error handling from urllib-specific exceptions to OSError and http.client.HTTPException.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 A rabbit hops through DNS with care,
Resolving first what's hiding there.
Sockets dance where urllib once crept,
TLS secrets safely kept!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description provides a clear summary of the vulnerability and fix. However, it is incomplete against the repository template—missing Critical sections: Change Type, Scope, Security Impact details (permissions, secrets, network calls, auth changes), Verification steps, Human Verification, Compatibility/Migration, and Checklist.	Complete the pull request description by filling in all required template sections: Change Type (mark 'Security hardening' and 'Bug fix'), Scope, detailed Security Impact assessment, Verification environment and steps, Human Verification, Compatibility assessment, and confirmation of the checklist items.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main security fix—closing a DNS-rebinding SSRF vulnerability in webhook delivery.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[raahulrahl]

Solid fix for a real TOCTOU SSRF vulnerability. Implementation is clean, uses stdlib correctly, and eliminates the DNS rebinding attack vector.