fix(netxlite): do not mutate outgoing requests (#508)

I have recently seen a data race related our way of
mutating the outgoing request to set the host header.

Unfortunately, I've lost track of the race output,
because I rebooted my Linux box before saving it.

Though, after inspecting why and and where we're mutating
outgoing requets, I've found that:

1. we add the host header when logging to have it logged,
which is not a big deal since we already emit the URL
rather than just the URL path when logging a request, and
so we can safely zap this piece of code;

2. as a result, in measurements we may omit the host header
but again this is pretty much obvious from the URL itself
and so it should not be very important (nonetheless,
avoid surprises and keep the existing behavior);

3. when the User-Agent header is not set, we default to
a `miniooni/0.1.0-dev` user agent, which is probably not
very useful anyway, so we can actually remove it.

Part of https://github.com/ooni/probe/issues/1733 (this diff
has been extracted from https://github.com/ooni/probe-cli/pull/506).

internal/engine/legacy/netx/oldhttptransport/httptransport.go

@@ -22,11 +22,6 @@ func New(roundTripper http.RoundTripper) *Transport {
 // RoundTrip executes a single HTTP transaction, returning
 // a Response for the provided Request.
 func (t *Transport) RoundTrip(req *http.Request) (resp *http.Response, err error) {
-	// Make sure we're not sending Go's default User-Agent
-	// if the user has configured no user agent
-	if req.Header.Get("User-Agent") == "" {
-		req.Header["User-Agent"] = nil
-	}
 	return t.roundTripper.RoundTrip(req)
 }
 

internal/engine/netx/httptransport/saver.go

@@ -52,7 +52,7 @@ type SaverMetadataHTTPTransport struct {
 // RoundTrip implements RoundTripper.RoundTrip
 func (txp SaverMetadataHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	txp.Saver.Write(trace.Event{
-		HTTPHeaders: req.Header,
+		HTTPHeaders: txp.CloneHeaders(req),
 		HTTPMethod:  req.Method,
 		HTTPURL:     req.URL.String(),
 		Transport:   txp.Transport,
@@ -72,6 +72,19 @@ func (txp SaverMetadataHTTPTransport) RoundTrip(req *http.Request) (*http.Respon
 	return resp, err
 }
 
+// CloneHeaders returns a clone of the headers where we have
+// also set the host header, which normally is not set by
+// golang until it serializes the request itself.
+func (txp SaverMetadataHTTPTransport) CloneHeaders(req *http.Request) http.Header {
+	header := req.Header.Clone()
+	if req.Host != "" {
+		header.Set("Host", req.Host)
+	} else {
+		header.Set("Host", req.URL.Host)
+	}
+	return header
+}
+
 // SaverTransactionHTTPTransport is a RoundTripper that saves
 // events related to the HTTP transaction
 type SaverTransactionHTTPTransport struct {

internal/engine/netx/httptransport/saver_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"io"
 	"net/http"
+	"net/url"
 	"strings"
 	"testing"
 	"time"
@@ -429,3 +430,35 @@ func TestSaverBodyResponseReadError(t *testing.T) {
 		t.Fatal("invalid Time")
 	}
 }
+
+func TestCloneHeaders(t *testing.T) {
+	t.Run("with req.Host set", func(t *testing.T) {
+		req := &http.Request{
+			Host: "www.example.com",
+			URL: &url.URL{
+				Host: "www.kernel.org",
+			},
+			Header: http.Header{},
+		}
+		txp := httptransport.SaverMetadataHTTPTransport{}
+		header := txp.CloneHeaders(req)
+		if header.Get("Host") != "www.example.com" {
+			t.Fatal("did not set Host header correctly")
+		}
+	})
+
+	t.Run("with only req.URL.Host set", func(t *testing.T) {
+		req := &http.Request{
+			Host: "",
+			URL: &url.URL{
+				Host: "www.kernel.org",
+			},
+			Header: http.Header{},
+		}
+		txp := httptransport.SaverMetadataHTTPTransport{}
+		header := txp.CloneHeaders(req)
+		if header.Get("Host") != "www.kernel.org" {
+			t.Fatal("did not set Host header correctly")
+		}
+	})
+}

internal/engine/netx/httptransport/useragent.go

@@ -1,9 +0,0 @@
-package httptransport
-
-import (
-	"github.com/ooni/probe-cli/v3/internal/netxlite"
-)
-
-// UserAgentTransport is a transport that ensures that we always
-// set an OONI specific default User-Agent header.
-type UserAgentTransport = netxlite.UserAgentTransport

internal/engine/netx/netx.go

@@ -252,7 +252,6 @@ func NewHTTPTransport(config Config) HTTPRoundTripper {
 		txp = httptransport.SaverTransactionHTTPTransport{
 			RoundTripper: txp, Saver: config.HTTPSaver}
 	}
-	txp = &httptransport.UserAgentTransport{HTTPTransport: txp}
 	return txp
 }
 

internal/engine/netx/netx_test.go

@@ -489,11 +489,7 @@ func TestNewTLSDialerWithNoTLSVerifyAndNoConfig(t *testing.T) {
 
 func TestNewVanilla(t *testing.T) {
 	txp := netx.NewHTTPTransport(netx.Config{})
-	uatxp, ok := txp.(*httptransport.UserAgentTransport)
-	if !ok {
-		t.Fatal("not the transport we expected")
-	}
-	if _, ok := uatxp.HTTPTransport.(*http.Transport); !ok {
+	if _, ok := txp.(*http.Transport); !ok {
 		t.Fatal("not the transport we expected")
 	}
 }
@@ -546,11 +542,7 @@ func TestNewWithByteCounter(t *testing.T) {
 	txp := netx.NewHTTPTransport(netx.Config{
 		ByteCounter: counter,
 	})
-	uatxp, ok := txp.(*httptransport.UserAgentTransport)
-	if !ok {
-		t.Fatal("not the transport we expected")
-	}
-	bctxp, ok := uatxp.HTTPTransport.(httptransport.ByteCountingTransport)
+	bctxp, ok := txp.(httptransport.ByteCountingTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
@@ -566,11 +558,7 @@ func TestNewWithLogger(t *testing.T) {
 	txp := netx.NewHTTPTransport(netx.Config{
 		Logger: log.Log,
 	})
-	uatxp, ok := txp.(*httptransport.UserAgentTransport)
-	if !ok {
-		t.Fatal("not the transport we expected")
-	}
-	ltxp, ok := uatxp.HTTPTransport.(*netxlite.HTTPTransportLogger)
+	ltxp, ok := txp.(*netxlite.HTTPTransportLogger)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
@@ -587,11 +575,7 @@ func TestNewWithSaver(t *testing.T) {
 	txp := netx.NewHTTPTransport(netx.Config{
 		HTTPSaver: saver,
 	})
-	uatxp, ok := txp.(*httptransport.UserAgentTransport)
-	if !ok {
-		t.Fatal("not the transport we expected")
-	}
-	stxptxp, ok := uatxp.HTTPTransport.(httptransport.SaverTransactionHTTPTransport)
+	stxptxp, ok := txp.(httptransport.SaverTransactionHTTPTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}

internal/netxlite/http.go

@@ -31,16 +31,6 @@ type httpTransportLogger struct {
 var _ HTTPTransport = &httpTransportLogger{}
 
 func (txp *httpTransportLogger) RoundTrip(req *http.Request) (*http.Response, error) {
-	host := req.Host
-	if host == "" {
-		host = req.URL.Host
-	}
-	req.Header.Set("Host", host) // anticipate what Go would do
-	return txp.logTrip(req)
-}
-
-// logTrip is an HTTP round trip with logging.
-func (txp *httpTransportLogger) logTrip(req *http.Request) (*http.Response, error) {
 	txp.Logger.Debugf("> %s %s", req.Method, req.URL.String())
 	for key, values := range req.Header {
 		for _, value := range values {
@@ -149,15 +139,10 @@ func NewOOHTTPBaseTransport(dialer Dialer, tlsDialer TLSDialer) HTTPTransport {
 
 // WrapHTTPTransport creates a new HTTP transport using
 // the given logger for logging.
-//
-// The returned transport will set a default user agent if the
-// request has not already set a user agent.
 func WrapHTTPTransport(logger Logger, txp HTTPTransport) HTTPTransport {
-	return &httpUserAgentTransport{
-		HTTPTransport: &httpTransportLogger{
-			HTTPTransport: txp,
-			Logger:        logger,
-		},
+	return &httpTransportLogger{
+		HTTPTransport: txp,
+		Logger:        logger,
 	}
 }
 
@@ -248,23 +233,6 @@ func (c *httpTLSConnWithReadTimeout) Read(b []byte) (int, error) {
 	return c.TLSConn.Read(b)
 }
 
-// httpUserAgentTransport is a transport that ensures that we always
-// set an OONI specific default User-Agent header.
-type httpUserAgentTransport struct {
-	HTTPTransport
-}
-
-const defaultHTTPUserAgent = "miniooni/0.1.0-dev"
-
-func (txp *httpUserAgentTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	if req.Header.Get("User-Agent") == "" {
-		req.Header.Set("User-Agent", defaultHTTPUserAgent)
-	}
-	return txp.HTTPTransport.RoundTrip(req)
-}
-
-var _ HTTPTransport = &httpUserAgentTransport{}
-
 // NewHTTPTransportStdlib creates a new HTTPTransport that uses
 // the Go standard library for all operations, including DNS
 // resolutions and TLS handshakes.

internal/netxlite/http_test.go

@@ -6,7 +6,6 @@ import (
 	"io"
 	"net"
 	"net/http"
-	"net/url"
 	"strings"
 	"testing"
 	"time"
@@ -51,39 +50,6 @@ func TestHTTPTransportLogger(t *testing.T) {
 			}
 		})
 
-		t.Run("we add the host header", func(t *testing.T) {
-			foundHost := &atomicx.Int64{}
-			txp := &httpTransportLogger{
-				Logger: log.Log,
-				HTTPTransport: &mocks.HTTPTransport{
-					MockRoundTrip: func(req *http.Request) (*http.Response, error) {
-						if req.Header.Get("Host") == "www.google.com" {
-							foundHost.Add(1)
-						}
-						return nil, io.EOF
-					},
-				},
-			}
-			req := &http.Request{
-				Header: http.Header{},
-				URL: &url.URL{
-					Scheme: "https",
-					Host:   "www.google.com",
-					Path:   "/",
-				},
-			}
-			resp, err := txp.RoundTrip(req)
-			if !errors.Is(err, io.EOF) {
-				t.Fatal("not the error we expected")
-			}
-			if resp != nil {
-				t.Fatal("expected nil response here")
-			}
-			if foundHost.Load() != 1 {
-				t.Fatal("host header was not added")
-			}
-		})
-
 		t.Run("with success", func(t *testing.T) {
 			var count int
 			lo := &mocks.Logger{
@@ -224,8 +190,7 @@ func TestNewHTTPTransport(t *testing.T) {
 		d := &mocks.Dialer{}
 		td := &mocks.TLSDialer{}
 		txp := NewHTTPTransport(log.Log, d, td)
-		ua := txp.(*httpUserAgentTransport)
-		logger := ua.HTTPTransport.(*httpTransportLogger)
+		logger := txp.(*httpTransportLogger)
 		if logger.Logger != log.Log {
 			t.Fatal("invalid logger")
 		}
@@ -425,62 +390,6 @@ func TestHTTPTLSDialerWithReadTimeout(t *testing.T) {
 	})
 }
 
-func TestHTTPUserAgentTransport(t *testing.T) {
-	t.Run("CloseIdleConnections", func(t *testing.T) {
-		var called bool
-		txp := &httpUserAgentTransport{
-			HTTPTransport: &mocks.HTTPTransport{
-				MockCloseIdleConnections: func() {
-					called = true
-				},
-			},
-		}
-		txp.CloseIdleConnections()
-		if !called {
-			t.Fatal("not called")
-		}
-	})
-
-	t.Run("RoundTrip", func(t *testing.T) {
-		t.Run("without an user-agent", func(t *testing.T) {
-			var ua string
-			txp := &httpUserAgentTransport{
-				HTTPTransport: &mocks.HTTPTransport{
-					MockRoundTrip: func(req *http.Request) (*http.Response, error) {
-						ua = req.Header.Get("User-Agent")
-						return &http.Response{}, nil
-					},
-				},
-			}
-			txp.RoundTrip(&http.Request{Header: make(http.Header)})
-			if ua != defaultHTTPUserAgent {
-				t.Fatal("not the expected user-agent")
-			}
-		})
-
-		t.Run("with an user-agent", func(t *testing.T) {
-			var ua string
-			expected := "antani/1.0"
-			txp := &httpUserAgentTransport{
-				HTTPTransport: &mocks.HTTPTransport{
-					MockRoundTrip: func(req *http.Request) (*http.Response, error) {
-						ua = req.Header.Get("User-Agent")
-						return &http.Response{}, nil
-					},
-				},
-			}
-			txp.RoundTrip(&http.Request{
-				Header: http.Header{
-					"User-Agent": {expected},
-				},
-			})
-			if ua != expected {
-				t.Fatal("not the expected user-agent")
-			}
-		})
-	})
-}
-
 func TestNewHTTPTransportStdlib(t *testing.T) {
 	// What to test about this factory?
 	txp := NewHTTPTransportStdlib(log.Log)

internal/netxlite/legacy.go

@@ -32,7 +32,6 @@ type (
 	TLSHandshakerLogger       = tlsHandshakerLogger
 	DialerSystem              = dialerSystem
 	TLSDialerLegacy           = tlsDialer
-	UserAgentTransport        = httpUserAgentTransport
 	AddressResolver           = resolverShortCircuitIPAddr
 )