diff --git a/internal/engine/legacy/netx/oldhttptransport/httptransport.go b/internal/engine/legacy/netx/oldhttptransport/httptransport.go index 40d7058..1d934f7 100644 --- a/internal/engine/legacy/netx/oldhttptransport/httptransport.go +++ b/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) } diff --git a/internal/engine/netx/httptransport/saver.go b/internal/engine/netx/httptransport/saver.go index bf56e69..3141327 100644 --- a/internal/engine/netx/httptransport/saver.go +++ b/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 { diff --git a/internal/engine/netx/httptransport/saver_test.go b/internal/engine/netx/httptransport/saver_test.go index f8eec7e..6ede801 100644 --- a/internal/engine/netx/httptransport/saver_test.go +++ b/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") + } + }) +} diff --git a/internal/engine/netx/httptransport/useragent.go b/internal/engine/netx/httptransport/useragent.go deleted file mode 100644 index 1ce4bcc..0000000 --- a/internal/engine/netx/httptransport/useragent.go +++ /dev/null @@ -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 diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index 09454f9..97eec27 100644 --- a/internal/engine/netx/netx.go +++ b/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 } diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index d93818b..9f1c0bf 100644 --- a/internal/engine/netx/netx_test.go +++ b/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") } diff --git a/internal/netxlite/http.go b/internal/netxlite/http.go index 35aaf59..c9fb9c5 100644 --- a/internal/netxlite/http.go +++ b/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. diff --git a/internal/netxlite/http_test.go b/internal/netxlite/http_test.go index 7da878c..417799b 100644 --- a/internal/netxlite/http_test.go +++ b/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) diff --git a/internal/netxlite/legacy.go b/internal/netxlite/legacy.go index dbf2116..bd41fe6 100644 --- a/internal/netxlite/legacy.go +++ b/internal/netxlite/legacy.go @@ -32,7 +32,6 @@ type ( TLSHandshakerLogger = tlsHandshakerLogger DialerSystem = dialerSystem TLSDialerLegacy = tlsDialer - UserAgentTransport = httpUserAgentTransport AddressResolver = resolverShortCircuitIPAddr )