🛡️ Sentinel: Fix SSRF vulnerability in URL validation #87
Conversation
Resolved domains in `validate_folder_url` to check if they point to private IP addresses. Previously, only IP literals were checked, allowing domains pointing to localhost/private IPs to bypass validation. This ensures that `folder-url` cannot be used to access internal services. Verified with new test cases covering: - Localhost/Private IP literals (rejected) - Domains resolving to private IPs (rejected) - Domains resolving to public IPs (accepted) - DNS resolution failures (rejected/fail-closed)
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| resolved_ip_str = socket.gethostbyname(hostname) | ||
| resolved_ip = ipaddress.ip_address(resolved_ip_str) | ||
| if resolved_ip.is_private or resolved_ip.is_loopback: | ||
| log.warning(f"Skipping unsafe URL (domain resolves to private IP {resolved_ip_str}): {sanitize_for_log(url)}") |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (130/100) Warning
| resolved_ip_str = socket.gethostbyname(hostname) | ||
| resolved_ip = ipaddress.ip_address(resolved_ip_str) | ||
| if resolved_ip.is_private or resolved_ip.is_loopback: | ||
| log.warning(f"Skipping unsafe URL (domain resolves to private IP {resolved_ip_str}): {sanitize_for_log(url)}") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| # Actually, if we can't resolve it, httpx won't be able to fetch it anyway. | ||
| # So we can just log a warning and return False (or True and let httpx fail). | ||
| # Returning False prevents the request. | ||
| log.warning(f"Could not resolve hostname for validation: {sanitize_for_log(url)}") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| resolved_ip_str = socket.gethostbyname(hostname) | ||
| resolved_ip = ipaddress.ip_address(resolved_ip_str) | ||
| if resolved_ip.is_private or resolved_ip.is_loopback: | ||
| log.warning(f"Skipping unsafe URL (domain resolves to private IP {resolved_ip_str}): {sanitize_for_log(url)}") |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (130/100) Warning
| resolved_ip_str = socket.gethostbyname(hostname) | ||
| resolved_ip = ipaddress.ip_address(resolved_ip_str) | ||
| if resolved_ip.is_private or resolved_ip.is_loopback: | ||
| log.warning(f"Skipping unsafe URL (domain resolves to private IP {resolved_ip_str}): {sanitize_for_log(url)}") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| # Actually, if we can't resolve it, httpx won't be able to fetch it anyway. | ||
| # So we can just log a warning and return False (or True and let httpx fail). | ||
| # Returning False prevents the request. | ||
| log.warning(f"Could not resolve hostname for validation: {sanitize_for_log(url)}") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
🛡️ Sentinel: [HIGH] Fix SSRF vulnerability in folder URL validation
🚨 Severity: HIGH
💡 Vulnerability: The
validate_folder_urlfunction previously only checked if the hostname was a private IP literal. It did not resolve domain names, allowing an attacker to use a domain (e.g.,local.test) that resolves to a private IP (e.g.,127.0.0.1) to bypass the check and perform Server-Side Request Forgery (SSRF).🎯 Impact: An attacker could potentially access internal services or sensitive local endpoints by providing a malicious
folder-url.🔧 Fix: Added DNS resolution using
socket.gethostbynameto resolve domains to their IP addresses before checking if they are private. The check "fails closed" if DNS resolution fails.✅ Verification: Verified with a test script
tests/test_ssrf.pythat mocks DNS resolution to ensure private IPs are rejected and public IPs are accepted.PR created automatically by Jules for task 5603844987171948925 started by @abhimehro