Uh oh!
There was an error while loading. Please reload this page.
Fix top-3 review findings: SSRF, textarea CRLF corruption, image fetch - #82
Closed
paskal wants to merge 1 commit into
Closed
Fix top-3 review findings: SSRF, textarea CRLF corruption, image fetch#82paskal wants to merge 1 commit into
paskal wants to merge 1 commit into
Conversation
Block SSRF via user-supplied URLs. Extraction and image fetches now reject targets resolving to loopback, private, link-local or other non-public addresses via a net.Dialer Control hook (checks the actual IP at connect time, so it also covers redirects and DNS rebinding). Gated by BlockPrivateNetworks, wired on in main; default stays permissive so httptest-based tests keep working. `POST /api/extract` was and remains unauthenticated, so this is the real mitigation. Stop corrupting multi-line rule fields. saveRule split match_url, excludes and test_urls on "\n" only, so browser-submitted \r\n left a trailing \r on every entry and blank lines became empty strings. New splitTrimLines trims and drops empties; handlePreview reuses it. Rework image sizing. getImageSize buffered every image fully into memory with a fresh client per image, no context and unbounded per-page goroutines. Now it streams to io.Discard behind an io.LimitReader (10MB cap), shares one client, honors the request context and caps concurrency at 8 probes per page.
There was a problem hiding this comment.
Pull request overview
This PR addresses three high-priority findings by hardening outbound fetching against SSRF, fixing CRLF-related textarea corruption when saving rules, and making image-size probing more resource-efficient within the extractor.
Changes:
- Add optional private-network/loopback blocking at dial-time for page retrieval and image probing (enabled in
main). - Normalize textarea inputs by splitting, trimming, and dropping empty lines to prevent
\r/blank-entry accumulation. - Stream image bytes with a size cap, share a single HTTP client for image probes, propagate request context, and limit concurrent in-flight image probes.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rest/server.go | Replaces ad-hoc textarea splitting with splitTrimLines to trim CRLF and drop empty entries. |
| rest/server_test.go | Adds coverage to ensure CRLF/blank lines are handled correctly when saving rules. |
| main.go | Enables private-network blocking flags for production use in the HTTP retriever and image probing. |
| extractor/safedial.go | Introduces dial-time IP blocking helpers and a transport constructor for SSRF mitigation. |
| extractor/retriever.go | Adds BlockPrivateNetworks to HTTPRetriever and wires safe transport when enabled. |
| extractor/retriever_test.go | Adds tests for loopback blocking behavior and IP classification logic. |
| extractor/readability.go | Plumbs request context into image extraction and adds image-client fields/flag. |
| extractor/pics.go | Refactors image probing to stream bytes with a cap, reuse a client, accept context, and limit concurrent probes. |
| extractor/pics_test.go | Updates callers for the new context signature and adds tests for cancellation/streamed sizing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
46
to
50
| wg.Go(func() { | ||
| size := f.getImageSize(im) | ||
| resCh <- imgInfo{url: im, size: size} | ||
| sem <- struct{}{} | ||
| defer func() { <-sem }() | ||
| resCh <- imgInfo{url: im, size: f.getImageSize(ctx, im)} | ||
| }) |
MemberAuthor
There was a problem hiding this comment.
False positive - sync.WaitGroup.Go was added in Go 1.25 (go.mod is go 1.25.0); it compiles and passes under -race in CI. This PR is superseded by #85, where the same code carries the same reply. No change.
| log.Printf("[WARN] can't create request to get pic from %s", url) | ||
| return 0 | ||
| } | ||
| req.Close = true |
MemberAuthor
Comment on lines
+12
to
+14
| // errBlockedAddress is returned when a fetch targets a non-public address and private-network | ||
| // blocking is enabled. the check runs at connect time against the actual resolved IP, so it also | ||
| // defends against DNS rebinding and redirects that point back at internal hosts. |
MemberAuthor
There was a problem hiding this comment.
Leaving lowercase by design - this repo's convention is lowercase in-code comments (see CLAUDE.md / the surrounding code), so a new sentence starting with lowercase "the check" is consistent house style rather than a typo. No change.
This was referenced Jul 3, 2026
paskal
commented
Jul 3, 2026
MemberAuthor
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 freeto 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.
Fixes the three highest-priority issues from an end-to-end review of the current master.
1. SSRF via user-supplied URLs (security)
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 (getImageSize) amplified it by following<img src>into more internal fetches.Both the page retriever and the image fetcher now reject targets that resolve to loopback, private, link-local, unspecified or multicast addresses. The check runs in a
net.DialerControlhook against the actual IP at connect time, so it also covers redirect-to-internal and DNS rebinding. It is gated behind a newBlockPrivateNetworksflag, wired on inmain; the default stays permissive sohttptest-based tests (which bind loopback) keep working.2. Textarea CRLF corruption on rule save (data integrity)
saveRulesplitmatch_url,excludesandtest_urlson"\n"only. Browsers submit textareas with\r\n, so every stored entry kept a trailing\rand blank lines became empty-string entries, and this junk accumulated on every round-trip through the edit form. NewsplitTrimLinestrims each line and drops empties;handlePreviewreuses it in place of its ad-hoc split-and-trim loop.3. Image sizing wasted resources on every extraction (efficiency)
getImageSizebuffered each image fully into memory viaio.ReadAll, built a freshhttp.Clientper image, ignored the request context, and spawned one unbounded goroutine per<img>on the page. It now streams toio.Discardbehind a 10 MBio.LimitReader, shares a single client, propagates the caller's context, and caps concurrency at 8 probes per page.Tests
Added
TestHTTPRetriever_BlockPrivateNetworks,TestIsBlockedIP,TestServer_SaveRuleTrimsLines, and twogetImageSizesubtests (context cancellation and streamed-byte measurement). Full suite passes with-race,golangci-lintandgo vetclean.Note
SSRF blocking is opt-in (on in production via
main, off by default) to avoid rewriting the loopback-based test suite. If you'd prefer it default-on with the tests adjusted to set an allow flag, that's an easy follow-up.