diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 0000000..893dbd7 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,4 @@ +## 2025-01-26 - Critical SSRF in Webhook Client +**Vulnerability:** `send_to_discord_with_retry` accepted arbitrary URLs, allowing Server-Side Request Forgery (SSRF). A malicious user could probe internal services or cloud metadata services. +**Learning:** The validation function `validate_webhook_url` existed but was not called in the main sending function. Also, `validate_webhook_url` had a fallback lenient check that could be bypassed. +**Prevention:** Always enforce input validation at the point of use. Avoid "lenient" fallback checks for security-critical inputs like URLs. diff --git a/tests/test_utils.py b/tests/test_utils.py index be674c8..4546dc3 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -13,7 +13,8 @@ import unittest sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) from utils.sanitizer import sanitize_json_for_export -from utils.discord_api import validate_webhook_url, sanitize_webhook_for_logging +from utils.discord_api import validate_webhook_url, sanitize_webhook_for_logging, send_to_discord_with_retry +from unittest.mock import patch, MagicMock class TestSanitizer(unittest.TestCase): @@ -118,6 +119,45 @@ class TestWebhookValidation(unittest.TestCase): is_valid, message = validate_webhook_url("https://example.com") self.assertFalse(is_valid) + def test_bypass_attempt_url(self): + """Should reject URLs that attempt to bypass validation.""" + # This URL contains 'discord' and 'webhook' but is not hosted on discord.com + bypass_url = "http://evil-site.com/discord/webhook" + is_valid, message = validate_webhook_url(bypass_url) + self.assertFalse(is_valid) + + def test_localhost_url(self): + """Should reject localhost URLs (SSRF protection).""" + is_valid, message = validate_webhook_url("http://localhost:8080/admin") + self.assertFalse(is_valid) + + +class TestSSRFPrevention(unittest.TestCase): + """Tests for SSRF prevention mechanisms.""" + + def test_send_to_discord_validates_url(self): + """Should raise ValueError for invalid URLs before sending request.""" + malicious_url = "http://localhost:8080/admin/delete" + + # We don't need to mock requests.post because it should fail before calling it + with self.assertRaises(ValueError) as cm: + send_to_discord_with_retry(malicious_url, data={"content": "test"}) + + self.assertIn("Invalid webhook URL", str(cm.exception)) + + @patch('requests.post') + def test_send_to_discord_allows_valid_url(self, mock_post): + """Should allow valid Discord URLs.""" + valid_url = "https://discord.com/api/webhooks/123/abc" + + mock_response = MagicMock() + mock_response.status_code = 200 + mock_post.return_value = mock_response + + send_to_discord_with_retry(valid_url, data={"content": "test"}) + + mock_post.assert_called_once() + class TestWebhookSanitization(unittest.TestCase): """Tests for webhook URL sanitization for logging.""" diff --git a/utils/discord_api.py b/utils/discord_api.py index dd49293..98e5abc 100644 --- a/utils/discord_api.py +++ b/utils/discord_api.py @@ -45,10 +45,6 @@ def validate_webhook_url(url: str) -> Tuple[bool, str]: if re.match(pattern, url, re.IGNORECASE): return True, "Valid Discord webhook URL" - # More lenient check - if "discord" in url.lower() and "webhook" in url.lower(): - return True, "Appears to be a Discord webhook URL" - return False, "URL does not appear to be a valid Discord webhook URL" @@ -345,7 +341,13 @@ def send_to_discord_with_retry( Raises: requests.exceptions.RequestException: If all retries fail + ValueError: If the webhook URL is invalid """ + # Validate URL to prevent SSRF + is_valid, error_msg = validate_webhook_url(webhook_url) + if not is_valid: + raise ValueError(f"Invalid webhook URL: {error_msg}") + last_exception = None for attempt in range(max_retries):