Merge pull request #823 from christianlouis/sentinel-fix-ssrf-dns-resolution-16520734505214840647

🛡️ Sentinel: [HIGH] Fix SSRF bypass on DNS resolution failure

.jules/sentinel.md

@@ -2,6 +2,11 @@
 **Vulnerability:** The `_test_webdav_connection` function had a custom SSRF check that failed to resolve DNS names, allowing attackers to bypass the check by providing a domain that resolves to an internal IP (e.g., `127.0.0.1`).
 **Learning:** DNS resolution is required for robust SSRF protection when validating URLs provided by users.
 **Prevention:** Use a centralized `is_private_ip` function (now in `app/utils/network.py`) that resolves the hostname to its IPs and checks if any are private.
+## 2026-03-22 - B310: urllib.request.urlopen replaced with httpx
+**Vulnerability:** The `_test_webdav_connection` function used `urllib.request.urlopen`, which natively supports dangerous schemes like `file://` or `ftp://` and follows redirects by default, potentially allowing SSRF bypasses or Local File Inclusion.
+**Learning:** `urllib.request` should be avoided for user-supplied URLs. Even when URL schemes are manually validated, `urllib`'s default redirect following behavior can bypass SSRF protections (e.g. redirecting to `127.0.0.1`).
+**Prevention:** Use a modern, safer HTTP client like `httpx` with `follow_redirects=False` when testing user-provided URLs.
+
 ## 2026-03-20 - Safe Path Traversal Prevention in Low-Level Utilities
 **Vulnerability:** The generic file utility `hash_file` in `app/utils/file_operations.py` accepted any file path and was vulnerable to reading arbitrary files via path traversal (e.g., `../../../etc/passwd`) or absolute paths if an attacker could control the `filepath` argument.
 **Learning:** Naively checking for `".." in path` breaks legitimate relative paths used internally by the application. Blocking absolute paths entirely also breaks functionality. Input validation should occur at the API boundary, but for defense-in-depth, low-level utilities must enforce expected boundaries (e.g., the application's `workdir`).

app/utils/network.py

@@ -27,8 +27,8 @@ def is_private_ip(hostname: str) -> bool:
                     return True
             return False
         except (socket.gaierror, socket.error):
-            # Cannot resolve - allow for testing/development
-            # In production, DNS should work properly
-            # Log this for debugging
-            logger.warning(f"Could not resolve hostname: {hostname}")
-            return False  # Changed from True to False to allow external domains in tests
+            # Cannot resolve.
+            # Fail securely: block unresolved domains to prevent DNS rebinding
+            # and SSRF bypasses via unresolvable addresses.
+            logger.warning(f"Could not resolve hostname (blocking securely): {hostname}")
+            return True

tests/test_coverage_polish.py

@@ -520,14 +520,14 @@ class TestURLUploadAdditionalCoverage:
         assert exc_info.value.status_code == 400
 
     def test_is_private_ip_unresolvable_hostname(self):
-        """Cover DNS resolution failure branch (lines 67-72)."""
+        """Cover DNS resolution failure branch blocking unresolvable domains."""
         import socket as _socket
 
         from app.utils.network import is_private_ip
 
         with patch("socket.getaddrinfo", side_effect=_socket.gaierror("nope")):
             result = is_private_ip("nonexistent.invalid.hostname.test")
-            assert result is False
+            assert result is True  # Fail securely by returning True
 
     def test_is_private_ip_hostname_resolves_to_private(self):
         """Cover branch where hostname resolves to a private IP (line 64-65)."""

tests/test_url_upload.py

@@ -698,6 +698,18 @@ class TestURLUploadCoverageGaps:
         assert result is False
         mock_getaddrinfo.assert_called_once()
 
+    @patch("app.utils.network.socket.getaddrinfo")
+    def test_is_private_ip_unresolvable_hostname_fails_securely(self, mock_getaddrinfo):
+        """Test that unresolvable hostnames fail securely by blocking access."""
+        import socket
+
+        from app.utils.network import is_private_ip
+
+        mock_getaddrinfo.side_effect = socket.gaierror("Name or service not known")
+
+        result = is_private_ip("unresolvable.example.internal")
+        assert result is True  # Fails securely
+
     @patch("socket.getaddrinfo")
     def test_is_private_ip_hostname_resolves_multiple_ips_all_public(self, mock_getaddrinfo):
         """Test hostname with multiple public IPs returns False (covers 65->61 loop branch)"""