Block SSRF to non-public addresses via connect-time IP guard#83
Open
paskal wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the unauthenticated POST /api/extract pipeline against SSRF by blocking connections to non-public IP ranges, including protections against DNS rebinding and redirect-to-internal, and makes the behavior configurable for trusted intranet deployments.
Changes:
- Added a connect-time IP guard (
net.Dialer.Control) and public-host prevalidation to block loopback/private/link-local and other special-purpose ranges. - Wired the guard through the default HTTP retriever, image fetching, and
mainwith a default-on posture plus an--allow-private-networksopt-out. - Added tests covering blocked IP classification, retriever blocking behavior, and upfront host validation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the new ALLOW_PRIVATE_NETWORKS / --allow-private-networks configuration. |
| main.go | Enables SSRF blocking by default and wires the opt-out flag through service configuration. |
| extractor/safedial.go | Implements connect-time dial guard, public-host validation, and guarded transport (proxy-disabled). |
| extractor/retriever.go | Adds BlockPrivateNetworks flag and uses the guarded transport when enabled. |
| extractor/retriever_test.go | Adds tests for blocked IP ranges, dial-time blocking, and hostname validation. |
| extractor/readability.go | Adds BlockPrivateNetworks plumbing and enforces prevalidation for Cloudflare path parity. |
| extractor/pics.go | Applies the guarded transport to image fetches when blocking is enabled. |
| extractor/pics_test.go | Adds a test ensuring image fetches are blocked on loopback when enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+58
to
+63
| // getImageSize loads image to get size | ||
| func (f *UReadability) getImageSize(url string) (size int) { | ||
| httpClient := &http.Client{Timeout: 30 * time.Second} | ||
| if f.BlockPrivateNetworks { | ||
| httpClient.Transport = safeTransport(30 * time.Second) | ||
| } |
Member
Author
There was a problem hiding this comment.
Fixed. Image probes now share a single lazily-built client (and its guarded transport) via imageClient() instead of constructing one per <img>, so enabling the guard by default no longer churns transports/FDs. Pushed in 83f51e0. (The sibling PR #85 refactors the same image path for streaming; on merge the shared client is the common end state.)
POST /api/extract is unauthenticated and the extractor fetched any URL it was given with no host validation, so a caller could reach http://169.254.169.254/... (cloud metadata), localhost and other internal hosts; image extraction amplified it by following <img src> into more internal fetches. The page retriever and image fetcher reject targets that resolve to loopback, private, link-local, unspecified, multicast and other IANA special-purpose ranges (0.0.0.0/8, CGNAT, benchmarking, documentation, reserved, broadcast) via a net.Dialer Control hook that checks the actual IP at connect time, so it also covers redirect-to-internal and DNS rebinding; the guarded transport disables env proxies so validation can't be bypassed through a proxy. extractWithRules additionally validates the resolved host up front, so the Cloudflare path (which fetches remotely and isn't covered by the dialer) enforces the same policy. Image probes share a single lazily-built client (and its guarded transport) rather than constructing one per <img>, so enabling the guard by default doesn't churn transports/FDs; the probe request no longer forces Connection: close, letting the shared client reuse connections. Enabled by default; operators opt out with --allow-private-networks for trusted intranet extraction. The default HTTPRetriever inherits the flag from UReadability so package-level callers are guarded too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
POST /api/extractis unauthenticated and the extractor fetched any URL it was given with no host validation, so a caller could reachhttp://169.254.169.254/…(cloud metadata),localhostand other internal hosts; image extraction amplified it by following<img src>into more internal fetches.What changed
Both the page retriever and the image fetcher reject targets that resolve to loopback, private, link-local, unspecified, multicast and other IANA special-purpose ranges (
0.0.0.0/8, CGNAT, benchmarking, documentation, reserved, broadcast). The check runs in anet.DialerControlhook against the actual IP at connect time, so it also covers redirect-to-internal and DNS rebinding. The guarded transport disables env proxies (HTTP_PROXY/HTTPS_PROXY) so validation can't be bypassed by routing through a proxy.extractWithRulesadditionally validates the resolved host up front, so the Cloudflare Browser Rendering path (which fetches remotely and isn't covered by the dialer) enforces the same policy.Enabled by default; operators opt out with
--allow-private-networks(ALLOW_PRIVATE_NETWORKS) for trusted intranet extraction, documented in the README config table. The defaultHTTPRetrieverinherits the flag fromUReadabilityso package-level callers are guarded too.Tests cover the blocked ranges (
TestIsBlockedIP), the retriever guard (TestHTTPRetriever_BlockPrivateNetworks) and the upfront host validation includinglocalhostand the metadata IP (TestValidatePublicHost).One of three PRs splitting the earlier #82; gated with an xhigh Codex review over the final diff.