T09 · Insecure Skill Coding Practices
- Location
references/code_review_service.py:49- Finding
Incomplete SSRF Protection for Pull Request Diff Retrieval
- Content
View full analysis
None: """Validate that a diff URL is safe to fetch (prevents SSRF).""" parsed = urlparse(url) if parsed.scheme not in ALLOWED_DIFF_SCHEMES: raise ValueError(f"Unsupported URL scheme '{parsed.scheme}' for diff URL. Allowed: {ALLOWED_DIFF_SCHEMES}") hostname = parsed.hostname or "" # Block internal/private network addresses if hostname in ("localhost", "127.0.0.1", "0.0.0.0", "::1") or hostname.startswith("10.") or hostname.startswith("192.168.") or hostname.startswith("172."): if not os.getenv("ALLOW_INTERNAL_DIFF_URLS"): raise ValueError(f"Diff URL points to internal address '{hostname}', which is blocked for security") if not hostname: raise ValueError("Diff URL has no hostname") ``` The validated URL is subsequently fetched as follows: ```python if diff_url: _validate_diff_url(diff_url) try: resp = requests.get(diff_url, timeout=REQUEST_TIMEOUT) resp.raise_for_status() diff_content = resp.text ``` ### Technical Analysis The validation checks only the textual hostname supplied in the URL. It does not resolve the hostname and inspect all resulting IP addresses. Consequently, it does not reliably reject destinations in loopback, private, link-local, reserved, unspecified, multicast, or other non-public address ranges. Specific bypass conditions include: - Link-local addresses such as `169.254.169.254`, commonly used by cloud metadata services. - Other loopback representations not explicitly listed. - Hostnames that resolve to private or loopback addresses. - DNS rebinding, where the hostname resolves differently bet ...[truncated 1658 chars]- Remediation
View remediation
