-
Notifications
You must be signed in to change notification settings - Fork 0
🛡️ Sentinel: Fix SSRF via DNS resolution & link-local checks #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
🛡️ Sentinel: Fix SSRF via DNS resolution & link-local checks #80
Conversation
…nk-local)
Description:
- **Vulnerability:** The application previously only validated the hostname string of folder URLs. This allowed attackers to bypass SSRF protections by using domains that resolve to internal or private IP addresses (e.g., `localtest.me` -> `127.0.0.1`). Additionally, link-local addresses (used for cloud metadata services) were not explicitly blocked.
- **Fix:** Enhanced `validate_folder_url` to:
1. Resolve domains to IP addresses using `socket.getaddrinfo`.
2. Check both IP literals and resolved IPs against `is_private`, `is_loopback`, and `is_link_local` checks.
3. Fail closed on DNS resolution errors.
- **Verification:** Added `tests/test_security.py` with test cases for local, private, link-local, and public IPs, as well as DNS failures.
|
👋 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. |
| if ip.is_private or ip.is_loopback: | ||
| log.warning(f"Skipping unsafe URL (private IP): {sanitize_for_log(url)}") | ||
| if ip.is_private or ip.is_loopback or ip.is_link_local: | ||
| log.warning(f"Skipping unsafe URL (private/unsafe IP): {sanitize_for_log(url)}") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| ip = ipaddress.ip_address(ip_str) | ||
| # Block private, loopback, AND link-local (e.g. 169.254.x.x for cloud metadata) | ||
| if ip.is_private or ip.is_loopback or ip.is_link_local: | ||
| log.warning(f"Skipping unsafe URL (domain {hostname} resolves to unsafe IP {ip_str}): {sanitize_for_log(url)}") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| log.warning(f"Skipping unsafe URL (domain {hostname} resolves to unsafe IP {ip_str}): {sanitize_for_log(url)}") | ||
| return False | ||
| except socket.gaierror: | ||
| log.warning(f"Skipping URL (DNS resolution failed): {sanitize_for_log(url)}") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| log.warning(f"Skipping URL (DNS resolution failed): {sanitize_for_log(url)}") | ||
| return False | ||
| except Exception as e: | ||
| log.warning(f"Error resolving domain {sanitize_for_log(hostname)}: {e}") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| import unittest | ||
| from unittest.mock import patch | ||
| import socket | ||
| import ipaddress |
Check warning
Code scanning / Prospector (reported by Codacy)
Unused import ipaddress (unused-import) Warning test
|
|
||
| self.assertFalse(is_valid, "Should reject URL if DNS resolution fails") | ||
|
|
||
| if __name__ == '__main__': |
Check warning
Code scanning / Prospector (reported by Codacy)
expected 2 blank lines after class or function definition, found 1 (E305) Warning test
| # We use getaddrinfo to support both IPv4 and IPv6 | ||
| for _, _, _, _, sockaddr in socket.getaddrinfo(hostname, None): | ||
| ip_str = sockaddr[0] | ||
| ip = ipaddress.ip_address(ip_str) |
Check warning
Code scanning / Pylint (reported by Codacy)
Variable name "ip" doesn't conform to snake_case naming style Warning
| ip = ipaddress.ip_address(ip_str) | ||
| # Block private, loopback, AND link-local (e.g. 169.254.x.x for cloud metadata) | ||
| if ip.is_private or ip.is_loopback or ip.is_link_local: | ||
| log.warning(f"Skipping unsafe URL (domain {hostname} resolves to unsafe IP {ip_str}): {sanitize_for_log(url)}") |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (135/100) Warning
| @@ -0,0 +1,86 @@ | |||
| import unittest | |||
Check warning
Code scanning / Pylint (reported by Codacy)
Missing module docstring Warning test
| # Note: This will execute the top-level code in main.py. | ||
| import main | ||
|
|
||
| class TestSSRFProtection(unittest.TestCase): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing class docstring Warning test
| class TestSSRFProtection(unittest.TestCase): | ||
|
|
||
| @patch('socket.getaddrinfo') | ||
| def test_domain_resolving_to_localhost(self, mock_getaddrinfo): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing method docstring Warning test
| self.assertFalse(is_valid, "Should reject link-local IP literal") | ||
|
|
||
| @patch('socket.getaddrinfo') | ||
| def test_domain_resolving_to_link_local(self, mock_getaddrinfo): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing method docstring Warning test
| self.assertFalse(is_valid, "Should reject domain resolving to link-local IP") | ||
|
|
||
| @patch('socket.getaddrinfo') | ||
| def test_domain_resolving_to_public_ip(self, mock_getaddrinfo): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing method docstring Warning test
| self.assertTrue(is_valid, "Should accept domain resolving to public IP") | ||
|
|
||
| @patch('socket.getaddrinfo') | ||
| def test_dns_resolution_failure(self, mock_getaddrinfo): |
Check warning
Code scanning / Pylint (reported by Codacy)
Missing method docstring Warning test
| pass | ||
| # Not an IP literal, it's a domain. Resolve it. | ||
| try: | ||
| # Resolve hostname to check if it points to a private IP |
Check notice
Code scanning / Pylint (reported by Codacy)
Catching too general exception Exception Note
| import unittest | ||
| from unittest.mock import patch | ||
| import socket | ||
| import ipaddress |
Check notice
Code scanning / Pylint (reported by Codacy)
Unused import ipaddress Note test
| self.assertTrue(is_valid, "Should accept domain resolving to public IP") | ||
|
|
||
| @patch('socket.getaddrinfo') | ||
| def test_dns_resolution_failure(self, mock_getaddrinfo): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
| self.assertFalse(is_valid, "Should reject domain resolving to link-local IP") | ||
|
|
||
| @patch('socket.getaddrinfo') | ||
| def test_domain_resolving_to_public_ip(self, mock_getaddrinfo): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
| self.assertFalse(is_valid, "Should reject link-local IP literal") | ||
|
|
||
| @patch('socket.getaddrinfo') | ||
| def test_domain_resolving_to_link_local(self, mock_getaddrinfo): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
|
|
||
| self.assertFalse(is_valid, "Should reject domain resolving to private IP") | ||
|
|
||
| def test_ip_literal_link_local(self): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
| self.assertFalse(is_valid, "Should reject domain resolving to localhost") | ||
|
|
||
| @patch('socket.getaddrinfo') | ||
| def test_domain_resolving_to_private_ip(self, mock_getaddrinfo): |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Missing function or method docstring Warning test
| import unittest | ||
| from unittest.mock import patch | ||
| import socket | ||
| import ipaddress |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Unused import ipaddress Note test
| log.warning(f"Skipping URL (DNS resolution failed): {sanitize_for_log(url)}") | ||
| return False | ||
| except Exception as e: | ||
| log.warning(f"Error resolving domain {sanitize_for_log(hostname)}: {e}") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| log.warning(f"Skipping unsafe URL (domain {hostname} resolves to unsafe IP {ip_str}): {sanitize_for_log(url)}") | ||
| return False | ||
| except socket.gaierror: | ||
| log.warning(f"Skipping URL (DNS resolution failed): {sanitize_for_log(url)}") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| ip = ipaddress.ip_address(ip_str) | ||
| # Block private, loopback, AND link-local (e.g. 169.254.x.x for cloud metadata) | ||
| if ip.is_private or ip.is_loopback or ip.is_link_local: | ||
| log.warning(f"Skipping unsafe URL (domain {hostname} resolves to unsafe IP {ip_str}): {sanitize_for_log(url)}") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| if ip.is_private or ip.is_loopback: | ||
| log.warning(f"Skipping unsafe URL (private IP): {sanitize_for_log(url)}") | ||
| if ip.is_private or ip.is_loopback or ip.is_link_local: | ||
| log.warning(f"Skipping unsafe URL (private/unsafe IP): {sanitize_for_log(url)}") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances SSRF (Server-Side Request Forgery) protection by adding DNS resolution checks and blocking link-local IP addresses in URL validation. The changes prevent attackers from bypassing security checks using domains that resolve to private/internal IPs or using link-local addresses like 169.254.169.254 (AWS metadata endpoint).
Key changes:
- Added DNS resolution to validate_folder_url to check resolved IPs, not just hostname strings
- Extended IP blocking to include link-local addresses (169.254.0.0/16) via is_link_local check
- Added comprehensive test suite for the new SSRF protections
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| main.py | Enhanced validate_folder_url with DNS resolution logic and link-local IP blocking |
| tests/test_security.py | Added comprehensive unit tests for SSRF protection scenarios including DNS resolution cases |
| .jules/sentinel.md | Documented the security vulnerability, learnings, and prevention measures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| class TestSSRFProtection(unittest.TestCase): | ||
|
|
||
| @patch('socket.getaddrinfo') |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock decorator is patching 'socket.getaddrinfo' globally, but it should patch 'main.socket.getaddrinfo' instead. Since main.py imports socket and calls socket.getaddrinfo, the patch needs to target where it's used (in the main module) rather than the socket module itself. This applies to all test methods that use @patch('socket.getaddrinfo').
|
|
||
| self.assertFalse(is_valid, "Should reject domain resolving to localhost") | ||
|
|
||
| @patch('socket.getaddrinfo') |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock decorator is patching 'socket.getaddrinfo' globally, but it should patch 'main.socket.getaddrinfo' instead. Since main.py imports socket and calls socket.getaddrinfo, the patch needs to target where it's used (in the main module) rather than the socket module itself.
| is_valid = main.validate_folder_url(url) | ||
| self.assertFalse(is_valid, "Should reject link-local IP literal") | ||
|
|
||
| @patch('socket.getaddrinfo') |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock decorator is patching 'socket.getaddrinfo' globally, but it should patch 'main.socket.getaddrinfo' instead. Since main.py imports socket and calls socket.getaddrinfo, the patch needs to target where it's used (in the main module) rather than the socket module itself.
|
|
||
| self.assertFalse(is_valid, "Should reject domain resolving to link-local IP") | ||
|
|
||
| @patch('socket.getaddrinfo') |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock decorator is patching 'socket.getaddrinfo' globally, but it should patch 'main.socket.getaddrinfo' instead. Since main.py imports socket and calls socket.getaddrinfo, the patch needs to target where it's used (in the main module) rather than the socket module itself.
|
|
||
| self.assertTrue(is_valid, "Should accept domain resolving to public IP") | ||
|
|
||
| @patch('socket.getaddrinfo') |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock decorator is patching 'socket.getaddrinfo' globally, but it should patch 'main.socket.getaddrinfo' instead. Since main.py imports socket and calls socket.getaddrinfo, the patch needs to target where it's used (in the main module) rather than the socket module itself.
| import unittest | ||
| from unittest.mock import patch | ||
| import socket | ||
| import ipaddress |
Copilot
AI
Jan 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'ipaddress' is not used.
| import ipaddress |
Enhanced SSRF protection by adding DNS resolution and link-local IP checks to URL validation.
PR created automatically by Jules for task 2097701723962855090 started by @abhimehro