core: validate download URLs and re-check them on redirect - #4035
Open
eliteprox wants to merge 2 commits into
Open
core: validate download URLs and re-check them on redirect#4035eliteprox wants to merge 2 commits into
eliteprox wants to merge 2 commits into
Conversation
- Renamed `TestRejectLocalhostDial` to `TestRejectNonPublicDial` and updated test cases to check for various non-public address types. - Introduced `validateDownloadURL` function to enforce stricter URL validation, rejecting unsafe schemes and path traversal. - Updated `DownloadData` to utilize the new validation and reject non-public addresses in `rejectNonPublicDial`. - Enhanced error handling for private and unsafe URLs in the download process.
…achable Blocking all non-public addresses broke a deliberate upstream decision: orchestrators, broadcasters and object stores routinely run on RFC1918 LAN, Docker and Kubernetes addresses, so refusing them would break those deployments. Restore the loopback-only dial check and the test that asserts private addresses stay reachable while a redirect to loopback is refused. What remains is the defense-in-depth that does not change reachability: reject non-HTTP(S) schemes, embedded credentials and path traversal, and re-run those checks on every redirect hop via CheckRedirect. Also drop the rejectLocalhostDial and localhostBlockedHTTPClient compatibility shims, which had no callers.
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
1 participant
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.
Summary
Follow-up hardening for
core.DownloadData, layered on top of the localhost-blocking fix in 5b7581c. Two gaps remained after that change:This PR adds
validateDownloadURL, applied both to the caller-supplied URL and, viaCheckRedirect, to every redirect hop. It rejects:http/https(file:,gopher:,data:,javascript:, …)http://user:pass@host/)..path segments, including percent-encoded (%2e%2e) and backslash-separated formsDeliberately unchanged
Reachability is not narrowed. The dial control stays loopback-and-unspecified only, and private/RFC1918, link-local, and CGNAT addresses remain reachable — orchestrators, broadcasters, and object stores routinely run on LAN, Docker, and Kubernetes addresses, so blocking those would break real deployments.
TestDownloadDataAllowsPrivateAddressAndBlocksRedirectToLoopbackis kept as the guard on that behavior.DownloadDataAllowLocalhostis also untouched and keeps its existing opt-out semantics.The diff against
masteris purely additive apart from one line:+79 / -1.Test plan
TestValidateDownloadURL— new table test covering each rejected class plus validhttp/httpsURLsTestDownloadDataAllowsPrivateAddressAndBlocksRedirectToLoopback— extended with a/redirect-unsafehop that 302s tofile:///etc/passwd, assertingCheckRedirectrejects it; still asserts a private-address fetch succeeds and a redirect to loopback is refused with zero hits on the protected serverTestRejectLocalhostDial— unchanged, still passesgo test -tags=mainnet,experimental ./core/ -count=1go build -tags=mainnet,experimental ./core/... ./server/...andgofmtclean