🛡️ Sentinel: [HIGH] Fix SSRF protection to explicitly block link-local IPs#721
🛡️ Sentinel: [HIGH] Fix SSRF protection to explicitly block link-local IPs#721
Conversation
…l IPs ===== ELIR ===== PURPOSE: Explicitly blocks link-local IP addresses (e.g., 169.254.169.254) in the _is_safe_ip validation. SECURITY: Defense-in-depth to prevent SSRF bypasses via OS network stacks unpredictably routing link-local addresses, protecting metadata services. FAILS IF: An attacker tries to sync a rule URL resolving to a metadata service. VERIFY: Ensure tests/test_ssrf_link_local.py correctly blocks 169.254.169.254 and test_ssrf_loopback.py continues to block 127.0.0.1. MAINTAIN: The checks for is_loopback, is_unspecified, is_private, and is_link_local must all remain explicitly defined before the fallback to is_global. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com>
|
👋 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. |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
github-actions
bot
added
documentation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Pull request overview
This PR hardens SSRF protections by explicitly rejecting link-local IP addresses (e.g., 169.254.169.254) during URL/hostname validation to reduce reliance on ipaddress.is_global.
Changes:
- Add an explicit
ip.is_link_localblock in_is_safe_ip. - Add a regression test ensuring domains resolving to a link-local IP are rejected.
- Document the SSRF learning/prevention note in
.jules/sentinel.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
main.py |
Adds explicit link-local IP rejection in _is_safe_ip used by hostname/URL validation. |
tests/test_ssrf_link_local.py |
Adds a unit test for blocking a hostname that resolves to 169.254.169.254. |
.jules/sentinel.md |
Records the security rationale and prevention guidance for link-local SSRF protection. |
Comments suppressed due to low confidence (1)
main.py:1084
- The
_is_safe_ipdocstring is now out of sync with behavior: the function explicitly blocks private and link-local addresses (and returnsip.is_globalotherwise). Please update the docstring to reflect the full set of rejected address types so future reviewers don’t miss a security-relevant check.
if ip.is_link_local:
return False
if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped:
return _is_safe_ip(ip.ipv4_mapped)
if isinstance(ip, ipaddress.IPv4Address) and ip in _CGNAT_NETWORK:
return False
return ip.is_global
| if ip.is_loopback: | ||
| return False |
There was a problem hiding this comment.
_is_safe_ip checks ip.is_loopback twice (once earlier and again here). The second check is redundant and makes the blocklist logic harder to audit; please remove the duplicate conditional to keep the security checks unambiguous.
| if ip.is_loopback: | |
| return False | |
| # Keep one authoritative loopback check so the blocklist stays easy to audit. |
| url = "https://metadata.example.com/config.json" | ||
| result = main.validate_folder_url(url) | ||
| self.assertFalse(result, "Should block domain resolving to link-local IP") |
There was a problem hiding this comment.
This test covers the DNS-resolution path, but the new ip.is_link_local protection also applies to IP-literal hosts (e.g., https://169.254.169.254/...) via validate_hostname’s IP parsing branch. Please add a second assertion (or test) that a URL containing a link-local IP literal is blocked, to prevent regressions in the direct-IP path.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves SSRF mitigation by explicitly checking for link-local IP addresses in the _is_safe_ip function and includes a new test case for this scenario. A review comment identifies a redundant is_loopback check that should be removed for better code clarity.
| if ip.is_loopback: | ||
| return False | ||
| if ip.is_link_local: | ||
| return False |
There was a problem hiding this comment.
The is_loopback check is duplicated here. It is already performed on lines 1072-1073. Removing the redundant check improves code clarity and performance.
| if ip.is_loopback: | |
| return False | |
| if ip.is_link_local: | |
| return False | |
| if ip.is_link_local: | |
| return False |
🚨 Severity: HIGH
💡 Vulnerability: Implicit reliance on
is_globalfor preventing SSRF to link-local addresses (like169.254.169.254AWS metadata).🎯 Impact: OS-specific network stacks may unexpectedly route these locally or to metadata services, allowing SSRF bypasses.
🔧 Fix: Explicitly check and block
ip.is_link_localin_is_safe_ip.✅ Verification:
tests/test_ssrf_link_local.pyruns successfully.===== ELIR =====
PURPOSE: Explicitly blocks link-local IP addresses (e.g., 169.254.169.254) in the
_is_safe_ipvalidation.SECURITY: Defense-in-depth to prevent SSRF bypasses via OS network stacks unpredictably routing link-local addresses, protecting metadata services.
FAILS IF: An attacker tries to sync a rule URL resolving to a metadata service.
VERIFY: Ensure
tests/test_ssrf_link_local.pycorrectly blocks 169.254.169.254 andtest_ssrf_loopback.pycontinues to block 127.0.0.1.MAINTAIN: The checks for
is_loopback,is_unspecified,is_private, andis_link_localmust all remain explicitly defined before the fallback tois_global.PR created automatically by Jules for task 12037346137075826038 started by @abhimehro