Merge pull request #187 from BaaRai/fix/customtool-proxy-scheme

Fix doubled scheme in the custom http tool's recording proxy URL

server/customtool.go

@@ -482,7 +482,11 @@ func (s *Server) httpToolTransport(ctx context.Context, key string, spec httpExe
 	if !intercept.GateOn(ctx) {
 		return nil
 	}
-	tr := http.DefaultTransport.(*http.Transport).Clone()
+	base, ok := http.DefaultTransport.(*http.Transport)
+	if !ok {
+		base = &http.Transport{}
+	}
+	tr := base.Clone()
 	// An environment proxy would be dialled instead of the target.
 	tr.Proxy = nil
 	tr.DialContext = intercept.PinnedDialer(key, true)
@@ -494,11 +498,11 @@ func (s *Server) httpToolTransport(ctx context.Context, key string, spec httpExe
 // httpProxyTransport builds a Transport for the http tool's proxy config, or nil
 // (direct). use_recording_proxy routes through the recording proxy + trusts its CA.
 func (s *Server) httpProxyTransport(spec httpExec) *http.Transport {
-	proxyStr := strings.TrimSpace(spec.Proxy)
+	proxyStr := proxyURLString(spec.Proxy)
 	var caFile string
 	if spec.UseRecordingProxy {
-		if addr := s.m.ProxyAddr(); addr != "" {
-			proxyStr = "http://" + addr
+		if addr := proxyURLString(s.m.ProxyAddr()); addr != "" {
+			proxyStr = addr
 			caFile = s.m.ProxyCACert()
 		}
 	}
@@ -521,6 +525,17 @@ func (s *Server) httpProxyTransport(spec httpExec) *http.Transport {
 	return tr
 }
 
+// proxyURLString returns a proxy address as a URL: a bare host:port gets
+// "http://", an address that already has a scheme (as ProxyAddr returns it) is
+// kept as is.
+func proxyURLString(s string) string {
+	s = strings.TrimSpace(s)
+	if s == "" || strings.Contains(s, "://") {
+		return s
+	}
+	return "http://" + s
+}
+
 // ---------- helpers ----------
 
 func identity(s string) string { return s }

server/customtool_dial_test.go

@@ -138,6 +138,52 @@ func TestCustomHTTPGateOffTransportUnchanged(t *testing.T) {
 	}
 }
 
+// The proxy URL of the http tool's transport: an address with a scheme is kept
+// as is, a bare host:port gets "http://"; never a doubled scheme.
+func TestHTTPProxyTransportProxyScheme(t *testing.T) {
+	req := httptest.NewRequest("GET", "http://api.example.com/", nil)
+	for _, c := range []struct {
+		name, globalProxy string
+		spec              httpExec
+		want              string
+	}{
+		{"recording proxy, global proxy with a scheme", "http://127.0.0.1:3128", httpExec{UseRecordingProxy: true}, "http://127.0.0.1:3128"},
+		{"operator proxy with a scheme", "", httpExec{Proxy: "http://10.0.0.5:8080"}, "http://10.0.0.5:8080"},
+		{"operator proxy as bare host:port", "", httpExec{Proxy: " 10.0.0.5:8080 "}, "http://10.0.0.5:8080"},
+	} {
+		s := &Server{m: &Manager{globalProxy: c.globalProxy}}
+		tr := s.httpProxyTransport(c.spec)
+		if tr == nil || tr.Proxy == nil {
+			t.Fatalf("%s: transport %+v, want a proxy transport", c.name, tr)
+		}
+		u, err := tr.Proxy(req)
+		if err != nil || u == nil || u.String() != c.want {
+			t.Fatalf("%s: Proxy(req) = %v, %v; want %s", c.name, u, err, c.want)
+		}
+	}
+}
+
+// passRoundTripper stands in for an http.DefaultTransport that is not an
+// *http.Transport; it hands every request to the original transport.
+type passRoundTripper struct{ next http.RoundTripper }
+
+func (p passRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
+	return p.next.RoundTrip(r)
+}
+
+// With http.DefaultTransport replaced by another RoundTripper, a gated direct
+// call still gets a transport with the pinned dialer instead of a panic.
+func TestHTTPToolTransportDefaultTransportNotStd(t *testing.T) {
+	orig := http.DefaultTransport
+	http.DefaultTransport = passRoundTripper{next: orig}
+	defer func() { http.DefaultTransport = orig }()
+	ctx := intercept.WithTargetScope(context.Background(), intercept.TargetScope{TaskID: "7", Enabled: func() bool { return true }})
+	tr := (&Server{}).httpToolTransport(ctx, "probe", httpExec{URL: "http://api.example.com/"})
+	if tr == nil || tr.DialContext == nil {
+		t.Fatalf("transport %+v, want one with the pinned dialer", tr)
+	}
+}
+
 // A redirect of a pinned origin to a pinned hop: the hop is dialled at the
 // hop's pinned address.
 func TestCustomHTTPRedirectHopDialPinned(t *testing.T) {
@@ -224,10 +270,10 @@ func TestBypass_CustomHTTPProxyPeerNotPinned(t *testing.T) {
 	direct := g.s.buildCustomTool(&db.Tool{Key: "probe_bypass", Kind: "http", Exec: execJSON(t, map[string]any{"url": url})})
 	approveTarget(t, g, task, direct, func() (string, bool) { return callTool(t, direct, gatedCtx(task, 0), `{}`, nil) }, key)
 
-	// httpProxyTransport writes "http://" before the recording proxy address, so
-	// the address is given here without a scheme.
+	// The global proxy carries a scheme, as in production (ProxyAddr returns it
+	// as is with capture off); httpProxyTransport must not write a second one.
 	g.m.mu.Lock()
-	g.m.globalProxy = proxy.Listener.Addr().String()
+	g.m.globalProxy = proxy.URL
 	g.m.mu.Unlock()
 	t.Cleanup(func() {
 		g.m.mu.Lock()