feat(networking): port ToolHive's networking package into core - #223
Merged
Conversation
Adds toolhive-core/networking, ported from ToolHive's pkg/networking so the two stop maintaining separate copies of the same security-critical logic. The private-IP/NAT64 CIDR table in particular is easy to get subtly wrong, and forking it across two independently released repos gives the compiler no way to keep them in sync. Package name and exported identifiers are preserved so ToolHive's migration is an import swap for its ~47 call sites. Three adaptations were required: - The reverse dependency is gone. ToolHive's IsLocalhost delegates to pkg/oauthproto.IsLoopbackHost; toolhive-core must never import toolhive, so that body is inlined. Behavior is identical, and the separate, more thorough IsLoopbackHost (SplitHostPort + ParseIP) is untouched. - INSECURE_DISABLE_URL_VALIDATION now flows through env.Reader instead of a bare os.Getenv. A shared library must not disable a security check based on a process-wide env var whose provenance it cannot see. Existing exported signatures are unchanged; WithEnvReader and the *WithReader variants are additive. - The dial guard and DisableKeepAlives are now inseparable. The guard checks the resolved address per dial, so a pooled connection skips it on later requests -- which is why it must be paired with keep-alives off. ToolHive ships that exact bug in its JWKS client (pkg/auth/token.go), while two other call sites get it right. WithDisableKeepAlives is therefore deleted: the branch that installs the guard is the branch that disables keep-alives, so the unsafe combination is unreachable rather than merely discouraged. Not ported: port.go, which is the sole gopsutil consumer (dragging in ebitengine/purego for one Connections() call) and is local-port allocation plus OAuth pre-registered-client logic, not egress policy; and IsBridgeMode with the NetworkIsolation* messages, which are container network-mode concerns. Coverage 89.3%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Fail closed in AddressReferencesPrivateIp when net.ParseIP can't parse the host, instead of silently treating it as safe. - Turn ErrPrivateIpAddress into a real sentinel error (ErrPrivateIPAddress) so callers can use errors.Is, matching ErrRedirectRefused. - Wire SameHostRedirectPolicy into Build() whenever WithTokenFromFile is used: oauth2.Transport re-adds the bearer token on every redirect hop, so an unset CheckRedirect lets a malicious/compromised server replay the token to an attacker-controlled host on a 30x. This same gap is confirmed live and unmitigated upstream in toolhive's pkg/networking today (pkg/auth/token.go's RFC 7662 and JWKS/OIDC clients). - Add NewHostScopedClientBuilderWithReader so callers can inject an env.Reader for testing; the existing constructor becomes a thin wrapper with unchanged behavior. - Fix a dangling doc-comment reference to a package that only exists in the main toolhive repo, not here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Port port.go/port_test.go from toolhive's pkg/networking to keep the package in one place rather than split across two repos. Preserves all 7 exported symbols (IsAvailable, FindAvailable, FindOrUsePort, ValidateCallbackPort, IsPreRegisteredClient, GetProcessOnPort, ParsePortSpec) with unchanged signatures so toolhive can later swap its import path with zero call-site changes. GetProcessOnPort pulls in github.com/shirou/gopsutil/v4 for cross-platform process-on-port lookup. FindAvailable/IsAvailable have a bind-then-close-then-return race: confirmed still live and unfixed upstream (toolhive#6141 tracked fixing the 7 production call sites and was closed without a fix landing). Rather than port that gap forward silently, add a race-free alternative alongside the preserved API: FindAvailableListener and FindOrUseListener return a still-open *net.TCPListener instead of a bare int, so nothing else can grab the port before the caller binds it. A concurrency test with 20 goroutines confirms no two ever get the same port. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- FindOrUsePort/FindOrUseListener: an explicit port outside 1-65535
(e.g. -1, 65536) was falling into the "port busy, pick another one"
branch and silently returning an unrelated random port. For
pre-registered OAuth callback configs this could conceal a bad
redirect URI. Reject out-of-range explicit ports up front; 0 still
means auto-select.
- ParsePortSpec: validate containerPort (1-65535) and hostPort
(0-65535) instead of only checking they parse as integers, so specs
like "-1:0", "70000:8001", "8000:99999" now fail with an attributed
error instead of surfacing a confusing failure later. Host port 0 is
intentionally still passed through unchanged, since Docker's own
PortBinding.HostPort treats "0" as dynamic allocation.
- WithEnvReader: normalize a nil reader to &env.OSReader{} instead of
storing it as-is, matching the fallback ValidatingTransport already
had. Fixes a nil-interface-method panic in
NewHostScopedClientBuilderWithReader(..., nil).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rdimitrov
approved these changes
Aug 10, 2026
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
2 participants
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
Ports the
networkingpackage fromstacklok/toolhive'spkg/networkinginto toolhive-core: a hardened outbound HTTP client (SSRF egress policy — private-IP/link-local dial blocking, same-host redirect policy), a body-capped JSON fetch helper, HTTP error mapping, URL/issuer validation helpers, and port/listener allocation utilities.networking/fetch.go,http_client.go,http_error.go,utilities.go,port.go(+ tests)ErrPrivateIpAddressconverted to a real sentinel error (errors.Is-compatible)Build()now installs a same-host redirect policy wheneverWithTokenFromFileis used, closing a bearer-token-replay-on-redirect gap — confirmed to also exist unmitigated upstream intoolhive'spkg/auth/token.goNewHostScopedClientBuilderWithReaderadded for env-reader dependency injection (additive, existing constructor unchanged)FindOrUsePort/FindOrUseListener/ParsePortSpecnow reject out-of-range ports instead of silently substituting an unrelated portWithEnvReadernormalizes a nil reader instead of panickingFindAvailableListener,FindOrUseListener) added alongside the portedFindAvailable/FindOrUsePort, which have a known bind-then-close-then-return race that is still unfixed upstreamCloses #221.
Test plan
task lint— 0 issuestask test(race detector) — all packages passtask license-check— clean🤖 Generated with Claude Code