break the code in a different way

Because I am dropping the requests again, we break again the tests with the redirects. I could possibly fix it by putting requests back again but I am not super happy about doing this because that would cause the DSL to do some strange work and I'd honestly rather not do this.
ooni · Nov 24, 2023 · ff42f3c · ff42f3c
1 parent 94f9fd7
commit ff42f3c
Show file tree

Hide file tree

Showing 5 changed files with 188 additions and 137 deletions.
diff --git a/internal/experiment/webconnectivitylte/analysishttpcore.go b/internal/experiment/webconnectivitylte/analysishttpcore.go
@@ -73,6 +73,18 @@ func (tk *TestKeys) analysisHTTPToplevel(logger model.Logger) {
 		return
 	}
 
+	// Make sure that the request is "final". This means that the request
+	// is not a redirect. If it's a redirect we can't compare to the
+	// final request by the TH. If you find this choice tricky, here's
+	// why we're doing this. We only save request entries when we actually
+	// attempt doing HTTP. So, if there is a redirect with a TCP or TLS
+	// error, there's no request to compare to. Yet, this means that a
+	// previous request MAY be considered as final and compared against
+	// the TH response, which would obviously be wrong.
+	if httpRedirectIsRedirect(finalRequest.Response.Code) {
+		return
+	}
+
 	// fallback to the HTTP diff algo.
 	tk.analysisHTTPDiff(logger, finalRequest, &ctrl)
 }
diff --git a/internal/experiment/webconnectivitylte/cleartextflow.go b/internal/experiment/webconnectivitylte/cleartextflow.go
@@ -8,7 +8,6 @@ package webconnectivitylte
 
 import (
 	"context"
-	"errors"
 	"io"
 	"net"
 	"net/http"
@@ -132,17 +131,19 @@ func (t *CleartextFlow) Run(parentCtx context.Context, index int64) error {
 	tcpDialer := trace.NewDialerWithoutResolver(t.Logger)
 	tcpConn, err := tcpDialer.DialContext(tcpCtx, "tcp", t.Address)
 	t.TestKeys.AppendTCPConnectResults(trace.TCPConnects()...)
-	t.TestKeys.AppendNetworkEvents(trace.NetworkEvents()...)
+	t.TestKeys.AppendNetworkEvents(trace.NetworkEvents()...) // BUGFIX: EXPLAIN
 	if err != nil {
-		// TODO(bassosimone): document why we're adding a request to the heap here
-		t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
-			trace,
-			"tcp",
-			t.Address,
-			"",
-			httpReq,
-			err,
-		))
+		/*
+			// TODO(bassosimone): document why we're adding a request to the heap here
+			t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
+				trace,
+				"tcp",
+				t.Address,
+				"",
+				httpReq,
+				err,
+			))
+		*/
 		ol.Stop(err)
 		return err
 	}
@@ -155,15 +156,17 @@ func (t *CleartextFlow) Run(parentCtx context.Context, index int64) error {
 
 	// Determine whether we're allowed to fetch the webpage
 	if t.PrioSelector == nil || !t.PrioSelector.permissionToFetch(t.Address) {
-		// TODO(bassosimone): document why we're adding a request to the heap here
-		t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
-			trace,
-			"tcp",
-			t.Address,
-			"",
-			httpReq,
-			errors.New("http_request_canceled"), // TODO(bassosimone): define this error
-		))
+		/*
+			// TODO(bassosimone): document why we're adding a request to the heap here
+			t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
+				trace,
+				"tcp",
+				t.Address,
+				"",
+				httpReq,
+				errors.New("http_request_canceled"), // TODO(bassosimone): define this error
+			))
+		*/
 		ol.Stop("stop after TCP connect")
 		return errNotPermittedToFetch
 	}
@@ -306,31 +309,29 @@ func (t *CleartextFlow) maybeFollowRedirects(ctx context.Context, resp *http.Res
 	if !t.FollowRedirects || !t.NumRedirects.CanFollowOneMoreRedirect() {
 		return // not configured or too many redirects
 	}
-	switch resp.StatusCode {
-	case 301, 302, 307, 308:
-		location, err := resp.Location()
-		if err != nil {
-			return // broken response from server
-		}
-		t.Logger.Infof("redirect to: %s", location.String())
-		resolvers := &DNSResolvers{
-			CookieJar:    t.CookieJar,
-			DNSCache:     t.DNSCache,
-			Domain:       location.Hostname(),
-			IDGenerator:  t.IDGenerator,
-			Logger:       t.Logger,
-			NumRedirects: t.NumRedirects,
-			TestKeys:     t.TestKeys,
-			URL:          location,
-			ZeroTime:     t.ZeroTime,
-			WaitGroup:    t.WaitGroup,
-			Referer:      resp.Request.URL.String(),
-			Session:      nil, // no need to issue another control request
-			TestHelpers:  nil, // ditto
-			UDPAddress:   t.UDPAddress,
-		}
-		resolvers.Start(ctx)
-	default:
-		// no redirect to follow
+
+	locationx, err := httpRedirectLocation(resp)
+	if err != nil || locationx.IsNone() {
+		return
+	}
+	location := locationx.Unwrap()
+
+	t.Logger.Infof("redirect to: %s", location.String())
+	resolvers := &DNSResolvers{
+		CookieJar:    t.CookieJar,
+		DNSCache:     t.DNSCache,
+		Domain:       location.Hostname(),
+		IDGenerator:  t.IDGenerator,
+		Logger:       t.Logger,
+		NumRedirects: t.NumRedirects,
+		TestKeys:     t.TestKeys,
+		URL:          location,
+		ZeroTime:     t.ZeroTime,
+		WaitGroup:    t.WaitGroup,
+		Referer:      resp.Request.URL.String(),
+		Session:      nil, // no need to issue another control request
+		TestHelpers:  nil, // ditto
+		UDPAddress:   t.UDPAddress,
 	}
+	resolvers.Start(ctx)
 }
diff --git a/internal/experiment/webconnectivitylte/httpfailure.go b/internal/experiment/webconnectivitylte/httpfailure.go
diff --git a/internal/experiment/webconnectivitylte/httpredirect.go b/internal/experiment/webconnectivitylte/httpredirect.go
@@ -0,0 +1,65 @@
+package webconnectivitylte
+
+import (
+	"net/http"
+	"net/url"
+
+	"github.com/ooni/probe-cli/v3/internal/optional"
+)
+
+/*
+// TODO(bassosimone): document this func
+func newArchivalHTTPRequestResultWithError(trace *measurexlite.Trace, network, address, alpn string,
+	req *http.Request, err error) *model.ArchivalHTTPRequestResult {
+	duration := trace.TimeSince(trace.ZeroTime())
+	return measurexlite.NewArchivalHTTPRequestResult(
+		trace.Index(),
+		duration,
+		network,
+		address,
+		alpn,
+		network, // TODO(bassosimone): get rid of this duplicate field?
+		req,
+		nil,
+		0,
+		nil,
+		err,
+		duration,
+	)
+}
+*/
+
+// httpRedirectIsRedirect returns true if the status code contains a redirect.
+func httpRedirectIsRedirect(status int64) bool {
+	switch status {
+	case 301, 302, 307, 308:
+		return true
+	default:
+		return false
+	}
+}
+
+// httpRedirectLocation returns a possibly-empty redirect URL or an error. More specifically:
+//
+// 1. the redirect URL is empty and the error nil if there's no redirect in resp;
+//
+// 2. the redirect URL is non-empty and the error is nil if there's a redirect in resp;
+//
+// 3. the error is non-nil if there's an error.
+//
+// Note that this function MAY possibly attempt to reconstruct the full redirect URL
+// in cases in which we're dealing with a partial redirect such as '/en_US/index.html'.
+func httpRedirectLocation(resp *http.Response) (optional.Value[*url.URL], error) {
+	if !httpRedirectIsRedirect(int64(resp.StatusCode)) {
+		return optional.None[*url.URL](), nil
+	}
+
+	location, err := resp.Location()
+	if err != nil {
+		return optional.None[*url.URL](), err
+	}
+
+	// TODO(https://github.com/ooni/probe/issues/2628): we need to handle
+	// the case where the redirect URL is incomplete
+	return optional.Some(location), nil
+}
diff --git a/internal/experiment/webconnectivitylte/secureflow.go b/internal/experiment/webconnectivitylte/secureflow.go
@@ -9,7 +9,6 @@ package webconnectivitylte
 import (
 	"context"
 	"crypto/tls"
-	"errors"
 	"io"
 	"net"
 	"net/http"
@@ -177,19 +176,21 @@ func (t *SecureFlow) Run(parentCtx context.Context, index int64) error {
 	tcpDialer := trace.NewDialerWithoutResolver(t.Logger)
 	tcpConn, err := tcpDialer.DialContext(tcpCtx, "tcp", t.Address)
 	t.TestKeys.AppendTCPConnectResults(trace.TCPConnects()...)
-	t.TestKeys.AppendNetworkEvents(trace.NetworkEvents()...)
+	t.TestKeys.AppendNetworkEvents(trace.NetworkEvents()...) // BUGFIX: EXPLAIN
 	if err != nil {
-		// TODO(bassosimone): document why we're adding a request to the heap here
-		if t.PrioSelector != nil {
-			t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
-				trace,
-				"tcp",
-				t.Address,
-				"",
-				httpReq,
-				err,
-			))
-		}
+		/*
+			// TODO(bassosimone): document why we're adding a request to the heap here
+			if t.PrioSelector != nil {
+				t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
+					trace,
+					"tcp",
+					t.Address,
+					"",
+					httpReq,
+					err,
+				))
+			}
+		*/
 		ol.Stop(err)
 		return err
 	}
@@ -220,17 +221,19 @@ func (t *SecureFlow) Run(parentCtx context.Context, index int64) error {
 	tlsConn, err := tlsHandshaker.Handshake(tlsCtx, tcpConn, tlsConfig)
 	t.TestKeys.AppendTLSHandshakes(trace.TLSHandshakes()...)
 	if err != nil {
-		// TODO(bassosimone): document why we're adding a request to the heap here
-		if t.PrioSelector != nil {
-			t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
-				trace,
-				"tcp",
-				t.Address,
-				"",
-				httpReq,
-				err,
-			))
-		}
+		/*
+			// TODO(bassosimone): document why we're adding a request to the heap here
+			if t.PrioSelector != nil {
+				t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
+					trace,
+					"tcp",
+					t.Address,
+					"",
+					httpReq,
+					err,
+				))
+			}
+		*/
 		ol.Stop(err)
 		return err
 	}
@@ -241,17 +244,19 @@ func (t *SecureFlow) Run(parentCtx context.Context, index int64) error {
 
 	// Determine whether we're allowed to fetch the webpage
 	if t.PrioSelector == nil || !t.PrioSelector.permissionToFetch(t.Address) {
-		// TODO(bassosimone): document why we're adding a request to the heap here
-		if t.PrioSelector != nil {
-			t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
-				trace,
-				"tcp",
-				t.Address,
-				"",
-				httpReq,
-				errors.New("http_request_canceled"), // TODO(bassosimone): define this error
-			))
-		}
+		/*
+			// TODO(bassosimone): document why we're adding a request to the heap here
+			if t.PrioSelector != nil {
+				t.TestKeys.PrependRequests(newArchivalHTTPRequestResultWithError(
+					trace,
+					"tcp",
+					t.Address,
+					"",
+					httpReq,
+					errors.New("http_request_canceled"), // TODO(bassosimone): define this error
+				))
+			}
+		*/
 		ol.Stop("stop after TLS handshake")
 		return errNotPermittedToFetch
 	}
@@ -434,33 +439,29 @@ func (t *SecureFlow) maybeFollowRedirects(ctx context.Context, resp *http.Respon
 	if !t.FollowRedirects || !t.NumRedirects.CanFollowOneMoreRedirect() {
 		return // not configured or too many redirects
 	}
-	switch resp.StatusCode {
-	case 301, 302, 307, 308:
-		location, err := resp.Location()
-		if err != nil {
-			return // broken response from server
-		}
-		// TODO(https://github.com/ooni/probe/issues/2628): we need to handle
-		// the case where the redirect URL is incomplete
-		t.Logger.Infof("redirect to: %s", location.String())
-		resolvers := &DNSResolvers{
-			CookieJar:    t.CookieJar,
-			DNSCache:     t.DNSCache,
-			Domain:       location.Hostname(),
-			IDGenerator:  t.IDGenerator,
-			Logger:       t.Logger,
-			NumRedirects: t.NumRedirects,
-			TestKeys:     t.TestKeys,
-			URL:          location,
-			ZeroTime:     t.ZeroTime,
-			WaitGroup:    t.WaitGroup,
-			Referer:      resp.Request.URL.String(),
-			Session:      nil, // no need to issue another control request
-			TestHelpers:  nil, // ditto
-			UDPAddress:   t.UDPAddress,
-		}
-		resolvers.Start(ctx)
-	default:
-		// no redirect to follow
+
+	locationx, err := httpRedirectLocation(resp)
+	if err != nil || locationx.IsNone() {
+		return
+	}
+	location := locationx.Unwrap()
+
+	t.Logger.Infof("redirect to: %s", location.String())
+	resolvers := &DNSResolvers{
+		CookieJar:    t.CookieJar,
+		DNSCache:     t.DNSCache,
+		Domain:       location.Hostname(),
+		IDGenerator:  t.IDGenerator,
+		Logger:       t.Logger,
+		NumRedirects: t.NumRedirects,
+		TestKeys:     t.TestKeys,
+		URL:          location,
+		ZeroTime:     t.ZeroTime,
+		WaitGroup:    t.WaitGroup,
+		Referer:      resp.Request.URL.String(),
+		Session:      nil, // no need to issue another control request
+		TestHelpers:  nil, // ditto
+		UDPAddress:   t.UDPAddress,
 	}
+	resolvers.Start(ctx)
 }