← Back to PRs

#19525: security: add SSRF validation for external URLs

by Mozzzaic open 2026-02-17 22:37 View on GitHub →
size: M
## Summary - Add SSRF guard utility (`src/security/ssrf-guard.ts`) for validating outbound URLs - Block private IP ranges: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 127.0.0.0/8, 169.254.0.0/16 - Block cloud metadata endpoints (169.254.169.254) - Block non-HTTP protocols (file://, ftp://, gopher://, etc.) - Resolve DNS before validation to prevent DNS rebinding attacks - Export `validateUrl()` async function and `isPrivateIp()` utility ## Security Impact Prevents Server-Side Request Forgery attacks that could access internal services, cloud instance metadata, or other private resources through crafted webhook/callback URLs. ## Test plan - [x] Unit tests for all blocked IP ranges (IPv4 + IPv6) - [x] Test DNS resolution validation - [x] Verify legitimate external URLs pass - [x] Test cloud metadata endpoint blocking - [x] Test non-HTTP protocol rejection <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR introduces a new SSRF guard utility at `src/security/ssrf-guard.ts`, but the codebase already has a comprehensive, production-hardened SSRF implementation at `src/infra/net/ssrf.ts` and `src/infra/net/fetch-guard.ts`. The new module duplicates this functionality with a weaker implementation, creating two diverging SSRF implementations to maintain. **Key issues found:** - **DNS rebinding not actually prevented**: `validateUrl` resolves DNS and checks the result, but then returns. The caller's subsequent HTTP request will perform a new DNS lookup. Without pinning the resolved IPs to the connection (as `fetchWithSsrFGuard` does via `createPinnedDispatcher`), a DNS rebinding attack can still succeed — an attacker returns a public IP during validation and a private IP during the actual connection (with a short TTL). The JSDoc claim that this "prevents DNS rebinding attacks" is incorrect. - **Missing IP ranges vs. existing implementation**: `isPrivateIp` does not handle IPv4-mapped IPv6 addresses (`::ffff:10.0.0.1`), the CGNAT range `100.64.0.0/10` (RFC 6598), or the deprecated site-local `fec0::/10` range — all of which are covered by the existing `isPrivateIpAddress` in `src/infra/net/ssrf.ts`. - **`localhost` and `.local`/`.internal` hostnames not blocked**: The existing `isBlockedHostname()` function covers `localhost`, `metadata.google.internal`, and `.local`/`.internal` suffixes. This new module skips all hostname-level checks. - **`BLOCKED_IPS` set is dead code**: Both entries (`169.254.169.254` and `fd00:ec2::254`) are already caught earlier by `isPrivateIp()`, making all `BLOCKED_IPS.has()` checks unreachable. - **No call sites**: The new module is not imported or used anywhere in the codebase yet, so none of these protections are active. <h3>Confidence Score: 1/5</h3> - This PR is not safe to merge as-is: it introduces a parallel SSRF implementation that is weaker than the existing one, contains a DNS rebinding bypass, and is not integrated anywhere. - The new `ssrf-guard.ts` module has multiple security gaps compared to the existing `src/infra/net/ssrf.ts`: it lacks IPv4-mapped IPv6 handling (a concrete bypass vector), misses CGNAT and site-local ranges, has no hostname-level blocking for `localhost`/`.local`/`.internal`, and critically does not pin DNS results — meaning the core DNS rebinding protection it claims to provide does not work. The module is also not wired into any request path, so it provides no actual protection. Merging it risks fragmentation of the SSRF defence layer and could lead to the new, weaker implementation being adopted over the robust existing one. - src/security/ssrf-guard.ts requires significant rework before it is safe to use; the existing src/infra/net/ssrf.ts and src/infra/net/fetch-guard.ts should be referenced as the canonical implementation. <sub>Last reviewed commit: 56a3b27</sub> <!-- greptile_other_comments_section --> <sub>(4/5) You can add custom instructions or style guidelines for the agent [here](https://app.greptile.com/review/github)!</sub> <!-- /greptile_comment -->

Most Similar PRs