fix(outbound): keep dial pinning for hops the environment proxy declines

pkg/gotenberg/outbound.go

@@ -11,6 +11,7 @@ import (
 	"net/http"
 	"net/netip"
 	"net/url"
+	"os"
 	"strings"
 	"time"
 
@@ -187,6 +188,12 @@ type OutboundDecision struct {
 // is stored.
 type outboundDecisionKey struct{}
 
+// outboundProxiedKey is the context key under which [outboundRoundTripper]
+// records that the environment proxy will carry this request, so that the
+// dialer knows the address it receives is the proxy's rather than the
+// destination's.
+type outboundProxiedKey struct{}
+
 // decideConfig carries optional settings for [DecideOutbound] and
 // [FilterOutboundURL]. See [DecideOption] for how callers configure it.
 type decideConfig struct {
@@ -364,6 +371,10 @@ type outboundRoundTripper struct {
 	allowList []*regexp2.Regexp
 	denyList  []*regexp2.Regexp
 	opts      []DecideOption
+
+	// proxyFunc mirrors the transport's own proxy resolution. It is nil unless
+	// the environment proxy is enabled.
+	proxyFunc func(*url.URL) (*url.URL, error)
 }
 
 // RoundTrip validates req.URL and delegates to the base transport.
@@ -379,6 +390,18 @@ func (rt *outboundRoundTripper) RoundTrip(req *http.Request) (*http.Response, er
 	}
 
 	ctx := context.WithValue(req.Context(), outboundDecisionKey{}, decision)
+
+	// A request the proxy will not carry is dialed directly, so it still gets
+	// pinned. Without this, enabling the environment proxy would silently drop
+	// DNS-rebinding protection for every NO_PROXY host, and for all traffic
+	// when no proxy variable is set at all.
+	if rt.proxyFunc != nil {
+		proxyURL, proxyErr := rt.proxyFunc(req.URL)
+		if proxyErr == nil && proxyURL != nil {
+			ctx = context.WithValue(ctx, outboundProxiedKey{}, true)
+		}
+	}
+
 	return rt.base.RoundTrip(req.WithContext(ctx))
 }
 
@@ -396,24 +419,34 @@ func (rt *outboundRoundTripper) RoundTrip(req *http.Request) (*http.Response, er
 //
 // When enableEnvironmentProxy is true, the client routes through the proxy
 // defined by the standard HTTP_PROXY, HTTPS_PROXY, and NO_PROXY variables,
-// including any credentials embedded in those URLs. In that mode the proxy
-// owns DNS and egress, so destination dial pinning does not apply; the URL
-// allow/deny and IP-class validation still runs. Callers gate this behind
-// their module's opt-in flag. See
+// including any credentials embedded in those URLs. Dial pinning does not apply
+// to a hop the proxy carries, since the proxy owns DNS and egress there; a hop
+// the proxy declines, such as a NO_PROXY host, is dialed directly and stays
+// pinned. The URL allow/deny and IP-class validation runs either way. Callers
+// gate this behind their module's opt-in flag. See
 // https://github.com/gotenberg/gotenberg/issues/1592.
 func NewOutboundHttpClient(timeout time.Duration, allowList, denyList []*regexp2.Regexp, enableEnvironmentProxy bool, opts ...DecideOption) *http.Client {
 	base := http.DefaultTransport.(*http.Transport).Clone()
 
+	var proxyFunc func(*url.URL) (*url.URL, error)
+
 	if enableEnvironmentProxy {
 		// Route through the operator's proxy (standard env vars, credentials
-		// included). NO_PROXY hosts get a direct, unpinned dial.
-		// httpproxy.FromEnvironment reads the environment now rather than
-		// caching it process-wide like http.ProxyFromEnvironment.
-		proxyFunc := httpproxy.FromEnvironment().ProxyFunc()
+		// included). httpproxy.FromEnvironment reads the environment now rather
+		// than caching it process-wide like http.ProxyFromEnvironment.
+		proxyFunc = httpproxy.FromEnvironment().ProxyFunc()
 		base.Proxy = func(req *http.Request) (*url.URL, error) {
 			return proxyFunc(req.URL)
 		}
-		base.DialContext = outboundDialer.DialContext
+		// Only a hop the proxy actually carries skips pinning: there the dial
+		// targets the proxy, not the destination, and the proxy owns DNS. A hop
+		// the proxy declines is dialed directly and stays pinned.
+		base.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+			if proxied, _ := ctx.Value(outboundProxiedKey{}).(bool); proxied {
+				return outboundDialer.DialContext(ctx, network, addr)
+			}
+			return secureDialContext(ctx, network, addr)
+		}
 	} else {
 		// Default: ignore any proxy environment variables and pin the dial to
 		// the IPs resolved during validation, closing the DNS-rebinding
@@ -431,10 +464,60 @@ func NewOutboundHttpClient(timeout time.Duration, allowList, denyList []*regexp2
 			allowList: allowList,
 			denyList:  denyList,
 			opts:      opts,
+			proxyFunc: proxyFunc,
 		},
 	}
 }
 
+// environmentProxyVariables are the variables golang.org/x/net/http/httpproxy
+// reads, in the casing precedence it applies.
+var environmentProxyVariables = []string{
+	"HTTP_PROXY", "http_proxy",
+	"HTTPS_PROXY", "https_proxy",
+	"ALL_PROXY", "all_proxy",
+}
+
+// ValidateEnvironmentProxyVariables checks that every proxy variable currently
+// set can be parsed as a proxy URL.
+//
+// httpproxy discards a parse error and falls back to a direct connection, so an
+// operator who mistypes a proxy URL would silently lose the egress path they
+// meant to enforce. Modules exposing an environment proxy flag call this from
+// their Validate so that startup fails loudly instead.
+//
+// Values are never included in the error: a proxy URL may carry credentials.
+func ValidateEnvironmentProxyVariables() error {
+	var err error
+
+	for _, name := range environmentProxyVariables {
+		if os.Getenv(name) == "" {
+			continue
+		}
+
+		if !isUsableProxyURL(os.Getenv(name)) {
+			err = errors.Join(err, fmt.Errorf("environment variable %s is not a usable proxy URL; unset it, or set it to a value like 'http://user:password@host:3128'", name))
+		}
+	}
+
+	return err
+}
+
+// isUsableProxyURL mirrors httpproxy's own parsing: a URL with a proxy scheme,
+// or anything that becomes one once a scheme is prefixed.
+func isUsableProxyURL(value string) bool {
+	proxyURL, err := url.Parse(value)
+	if err == nil {
+		switch proxyURL.Scheme {
+		case "http", "https", "socks5", "socks5h":
+			return true
+		}
+	}
+
+	// httpproxy retries bare values such as "host:3128" with a scheme.
+	_, err = url.Parse("http://" + value)
+	return err == nil
+}
+
 // secureDialContext consumes the [OutboundDecision] stashed in ctx by
 // [outboundRoundTripper]. When the decision is to bypass (allow-list
 // match), it dials directly. When the decision contains pinned IPs, it
@@ -580,3 +663,14 @@ type bufferedConn struct {
 func (c *bufferedConn) Read(b []byte) (int, error) {
 	return c.r.Read(b)
 }
+
+// CloseWrite half-closes the underlying connection. Embedding [net.Conn] hides
+// the method, so a CONNECT splice over this connection could never signal EOF
+// to the upstream and both sides waited for the other until a timeout.
+func (c *bufferedConn) CloseWrite() error {
+	cw, ok := c.Conn.(interface{ CloseWrite() error })
+	if !ok {
+		return fmt.Errorf("underlying %T does not support half-close", c.Conn)
+	}
+	return cw.CloseWrite()
+}

pkg/gotenberg/outbound_envproxy_test.go

@@ -0,0 +1,155 @@
+package gotenberg
+
+import (
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"net/netip"
+	"strings"
+	"testing"
+)
+
+func TestValidateEnvironmentProxyVariables(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		env     map[string]string
+		wantErr bool
+		// wantIn is a substring the error must name, so that an operator can
+		// find the offending variable.
+		wantIn string
+	}{
+		{
+			name: "nothing set",
+			env:  map[string]string{},
+		},
+		{
+			name: "well formed URL",
+			env:  map[string]string{"HTTP_PROXY": "http://proxy.example.com:3128"},
+		},
+		{
+			name: "credentials are accepted",
+			env:  map[string]string{"HTTPS_PROXY": "http://user:password@proxy.example.com:3128"},
+		},
+		{
+			name: "bare host and port is accepted, as httpproxy prefixes a scheme",
+			env:  map[string]string{"HTTP_PROXY": "proxy.example.com:3128"},
+		},
+		{
+			name: "socks5 is accepted",
+			env:  map[string]string{"ALL_PROXY": "socks5://proxy.example.com:1080"},
+		},
+		{
+			name: "lowercase variables are checked too",
+			env:  map[string]string{"http_proxy": "http://proxy.example.com:3128"},
+		},
+		{
+			name:    "unparseable URL",
+			env:     map[string]string{"HTTP_PROXY": "http://proxy.example.com:3128/%zz"},
+			wantErr: true,
+			wantIn:  "HTTP_PROXY",
+		},
+		{
+			name:    "the failing variable is named",
+			env:     map[string]string{"HTTPS_PROXY": "://%zz"},
+			wantErr: true,
+			wantIn:  "HTTPS_PROXY",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			for _, name := range environmentProxyVariables {
+				t.Setenv(name, "")
+			}
+			for name, value := range tc.env {
+				t.Setenv(name, value)
+			}
+
+			err := ValidateEnvironmentProxyVariables()
+			if tc.wantErr && err == nil {
+				t.Fatal("expected an error, got none")
+			}
+			if !tc.wantErr && err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if tc.wantIn != "" && !strings.Contains(err.Error(), tc.wantIn) {
+				t.Errorf("error %q does not name %q", err, tc.wantIn)
+			}
+		})
+	}
+}
+
+// TestValidateEnvironmentProxyVariables_DoesNotLeakCredentials pins that a
+// proxy URL, which may embed a password, never reaches the error text.
+func TestValidateEnvironmentProxyVariables_DoesNotLeakCredentials(t *testing.T) {
+	for _, name := range environmentProxyVariables {
+		t.Setenv(name, "")
+	}
+	t.Setenv("HTTP_PROXY", "http://admin:hunter2@proxy.example.com:3128/%zz")
+
+	err := ValidateEnvironmentProxyVariables()
+	if err == nil {
+		t.Fatal("expected an error, got none")
+	}
+	if strings.Contains(err.Error(), "hunter2") {
+		t.Errorf("error leaks the proxy password: %q", err)
+	}
+	if strings.Contains(err.Error(), "admin") {
+		t.Errorf("error leaks the proxy username: %q", err)
+	}
+}
+
+// TestNewOutboundHttpClient_EnvironmentProxyPinsDirectHops is the regression
+// test for the dial-pinning gap: with the environment proxy enabled but no
+// proxy applicable to the request, the dial must still go through the pinning
+// dialer rather than a plain one.
+//
+// The request targets a hostname that only the stub resolver knows, so a plain
+// dial would hand that unresolvable name to the OS and fail. Only a pinned dial,
+// which substitutes the address resolved during validation, can connect.
+// See https://github.com/gotenberg/gotenberg/issues/1592.
+func TestNewOutboundHttpClient_EnvironmentProxyPinsDirectHops(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusNoContent)
+	}))
+	defer srv.Close()
+
+	_, port, err := net.SplitHostPort(strings.TrimPrefix(srv.URL, "http://"))
+	if err != nil {
+		t.Fatalf("split server address: %v", err)
+	}
+
+	const host = "pinned-only.invalid"
+
+	// NO_PROXY covers the destination, so httpproxy declines it and the
+	// transport dials directly. That direct dial is the hop that used to lose
+	// pinning.
+	for _, name := range environmentProxyVariables {
+		t.Setenv(name, "")
+	}
+	t.Setenv("HTTP_PROXY", "http://proxy.invalid:3128")
+	t.Setenv("NO_PROXY", host)
+
+	withStubResolver(t, func(string) ([]netip.Addr, error) {
+		return []netip.Addr{netip.MustParseAddr("127.0.0.1")}, nil
+	})
+
+	client := NewOutboundHttpClient(0, nil, nil, true)
+	rt, ok := client.Transport.(*outboundRoundTripper)
+	if !ok {
+		t.Fatalf("transport is %T, want *outboundRoundTripper", client.Transport)
+	}
+	if rt.proxyFunc == nil {
+		t.Fatal("proxyFunc is nil, want the environment proxy to be resolved per request")
+	}
+
+	resp, err := client.Get("http://" + net.JoinHostPort(host, port))
+	if err != nil {
+		t.Fatalf("GET failed, so the direct hop was not pinned: %v", err)
+	}
+	defer func() {
+		_ = resp.Body.Close()
+	}()
+
+	if resp.StatusCode != http.StatusNoContent {
+		t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusNoContent)
+	}
+}

pkg/modules/api/api.go

@@ -380,6 +380,13 @@ func (a *Api) Validate() error {
 		err = errors.Join(err, errors.New("IP must be a valid IP address"))
 	}
 
+	if a.downloadFromCfg.enableEnvironmentProxy {
+		proxyErr := gotenberg.ValidateEnvironmentProxyVariables()
+		if proxyErr != nil {
+			err = errors.Join(err, fmt.Errorf("--api-download-from-enable-environment-proxy is set: %w", proxyErr))
+		}
+	}
+
 	if (a.tlsCertFile != "" && a.tlsKeyFile == "") || (a.tlsCertFile == "" && a.tlsKeyFile != "") {
 		err = errors.Join(err,
 			errors.New("both TLS certificate and key files must be set"),

pkg/modules/chromium/chromium.go

@@ -669,6 +669,13 @@ func (mod *Chromium) Validate() error {
 		return fmt.Errorf("chromium-max-concurrency must be between 1 and 6, got %d", mod.maxConcurrency)
 	}
 
+	if mod.args.enableEnvironmentProxy {
+		proxyErr := gotenberg.ValidateEnvironmentProxyVariables()
+		if proxyErr != nil {
+			return fmt.Errorf("--chromium-enable-environment-proxy is set: %w", proxyErr)
+		}
+	}
+
 	_, err := os.Stat(mod.args.binPath)
 	if os.IsNotExist(err) {
 		return fmt.Errorf("Chromium binary does not exist at %q; check the CHROMIUM_BIN_PATH environment variable: %w", mod.args.binPath, err)

pkg/modules/libreoffice/api/api.go

@@ -522,6 +522,13 @@ func (a *Api) Validate() error {
 		err = errors.Join(err, fmt.Errorf("unoconverter binary does not exist at %q; check the UNOCONVERTER_BIN_PATH environment variable: %w", a.args.unoBinPath, statErr))
 	}
 
+	if a.args.proxyOptions.enableEnvironmentProxy {
+		proxyErr := gotenberg.ValidateEnvironmentProxyVariables()
+		if proxyErr != nil {
+			err = errors.Join(err, fmt.Errorf("--libreoffice-enable-environment-proxy is set: %w", proxyErr))
+		}
+	}
+
 	return err
 }
 

pkg/modules/webhook/webhook.go

@@ -1,6 +1,7 @@
 package webhook
 
 import (
+	"fmt"
 	"sync/atomic"
 	"time"
 
@@ -107,10 +108,25 @@ func (w *Webhook) AsyncCount() int64 {
 	return w.asyncCount.Load()
 }
 
+// Validate checks the module's configuration.
+func (w *Webhook) Validate() error {
+	if !w.enableEnvironmentProxy {
+		return nil
+	}
+
+	err := gotenberg.ValidateEnvironmentProxyVariables()
+	if err != nil {
+		return fmt.Errorf("--webhook-enable-environment-proxy is set: %w", err)
+	}
+
+	return nil
+}
+
 // Interface guards.
 var (
 	_ gotenberg.Module        = (*Webhook)(nil)
 	_ gotenberg.Provisioner   = (*Webhook)(nil)
+	_ gotenberg.Validator     = (*Webhook)(nil)
 	_ api.MiddlewareProvider  = (*Webhook)(nil)
 	_ api.AsynchronousCounter = (*Webhook)(nil)
 )