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

@@ -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

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())
+		}
+	}
+}