From 1880021e76b8b0067fe54c5e55ccd4647c85d637 Mon Sep 17 00:00:00 2001 From: Rohit Behera <126186063+r0h1tb@users.noreply.github.com> Date: Mon, 10 Aug 2026 11:28:43 +0530 Subject: [PATCH] network_proxy: reject proxy URLs that resolve to a port with no host (#7922) * network_proxy: reject proxy URLs that resolve to a port with no host The host check after placeholder replacement had its arguments swapped: strings.Split("", pUrl.Host)[0] == ":" This splits the empty string using pUrl.Host as the separator, so it returns [""] and element 0 is always "", never ":". The comparison was dead for every possible host value, which meant the "http://:80" case named in the comment directly below it was never rejected. Such a URL was handed back as the proxy, and the failure surfaced later as a confusing dial error instead of the intended message. Only the pUrl.Host == "" half of the condition ever did anything, so "/some/path" was still caught. Use url.URL.Hostname(), which returns the host with any port stripped and is "" for exactly the two cases the comment describes -- ":80" and "" -- while leaving IPv6 literals such as "[::1]:80" and userinfo forms intact. That collapses both clauses into one expression. Also adds tests for this function; the package previously had none. * network_proxy: cover userinfo-only and IPv6 hosts in the tests Differential-tested the old predicate against the new one across 36 URL forms. Every behavioural change is in the same direction -- previously accepted, now rejected -- and all of them are host-less. Nothing that was rejected before is accepted now, and no legitimate host form changes. Two of those forms were worth pinning down in the test: - "http://user:pass@:8080" parses with Host ":8080", so it is just as host-less as "http://:80". The comment in the source doesn't name this variant, but it was accepted before and is rejected now. - IPv6 literals are full of colons, so a repair that split Host on ":" rather than using Hostname() could plausibly reject them. Added "[::1]:8080" and "[2001:db8::1]" as regression guards; both are accepted before and after, which is the point. On unfixed master the port-only and userinfo cases both fail; the IPv6 cases pass on both sides. --- modules/internal/network/networkproxy.go | 2 +- modules/internal/network/networkproxy_test.go | 114 ++++++++++++++++++ 2 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 modules/internal/network/networkproxy_test.go diff --git a/modules/internal/network/networkproxy.go b/modules/internal/network/networkproxy.go index f9deeb43a..7f3988a7f 100644 --- a/modules/internal/network/networkproxy.go +++ b/modules/internal/network/networkproxy.go @@ -78,7 +78,7 @@ func (p ProxyFromURL) ProxyFunc() func(*http.Request) (*url.URL, error) { if err != nil { p.logger.Warn("failed to derive transport proxy from network_proxy URL") pUrl = nil - } else if pUrl.Host == "" || strings.Split("", pUrl.Host)[0] == ":" { + } else if pUrl.Hostname() == "" { // url.Parse does not return an error on these values: // // - http://:80 diff --git a/modules/internal/network/networkproxy_test.go b/modules/internal/network/networkproxy_test.go new file mode 100644 index 000000000..f35428387 --- /dev/null +++ b/modules/internal/network/networkproxy_test.go @@ -0,0 +1,114 @@ +package network + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "go.uber.org/zap" + + "github.com/caddyserver/caddy/v2" +) + +// TestProxyFromURLPlaceholderHostValidation checks that a network_proxy URL +// which resolves to a value without a host is rejected after placeholders are +// applied. url.Parse accepts both "http://:80" and "/some/path" without +// returning an error, so ProxyFunc has to reject them itself. +func TestProxyFromURLPlaceholderHostValidation(t *testing.T) { + for i, tc := range []struct { + name string + url string + hostRepl string + wantHost string + wantErr bool + }{ + { + name: "port only, no host", + url: "http://{proxy.host}:80", + hostRepl: "", + wantErr: true, + }, + { + name: "no scheme or host, path only", + url: "{proxy.host}/some/path", + hostRepl: "", + wantErr: true, + }, + { + // url.Parse puts the userinfo elsewhere, so Host is still just + // ":8080" here. The comment above the check doesn't name this + // form, but it is equally host-less and equally unusable. + name: "userinfo but no host", + url: "http://user:pass@{proxy.host}:8080", + hostRepl: "", + wantErr: true, + }, + { + name: "host and port", + url: "http://{proxy.host}:8080", + hostRepl: "proxy.example.com", + wantHost: "proxy.example.com", + wantErr: false, + }, + { + name: "host without port", + url: "http://{proxy.host}", + hostRepl: "proxy.example.com", + wantHost: "proxy.example.com", + wantErr: false, + }, + { + // Guards against a repair that splits Host on ":" instead of + // using Hostname(): an IPv6 literal is full of colons and must + // not be mistaken for a missing host. + name: "IPv6 literal with port", + url: "http://[{proxy.host}]:8080", + hostRepl: "::1", + wantHost: "::1", + wantErr: false, + }, + { + name: "IPv6 literal without port", + url: "http://[{proxy.host}]", + hostRepl: "2001:db8::1", + wantHost: "2001:db8::1", + wantErr: false, + }, + } { + p := ProxyFromURL{URL: tc.url, logger: zap.NewNop()} + + repl := caddy.NewReplacer() + repl.Set("proxy.host", tc.hostRepl) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/", nil) + req = req.WithContext(context.WithValue(req.Context(), caddy.ReplacerCtxKey, repl)) + + proxyURL, err := p.ProxyFunc()(req) + + if tc.wantErr { + if err == nil { + t.Errorf("Test %d (%s): expected an error for %q but got none (proxy URL: %v)", + i, tc.name, tc.url, proxyURL) + } + if proxyURL != nil { + t.Errorf("Test %d (%s): expected a nil proxy URL for %q, got %v", + i, tc.name, tc.url, proxyURL) + } + continue + } + + if err != nil { + t.Errorf("Test %d (%s): unexpected error for %q: %v", i, tc.name, tc.url, err) + continue + } + if proxyURL == nil { + t.Errorf("Test %d (%s): expected a proxy URL for %q, got nil", i, tc.name, tc.url) + continue + } + if proxyURL.Hostname() != tc.wantHost { + t.Errorf("Test %d (%s): expected host %q, got %q", + i, tc.name, tc.wantHost, proxyURL.Hostname()) + } + } +}