fix(webconnectivitylte): handle malformed redirect URLs

internal/experiment/webconnectivitylte/cleartextflow.go

@@ -269,6 +269,9 @@ func (t *CleartextFlow) httpTransaction(ctx context.Context, network, address, a
 		reader := io.LimitReader(resp.Body, maxbody)
 		body, err = StreamAllContext(ctx, reader)
 	}
+	if err == nil && httpRedirectIsRedirect(resp) {
+		err = httpValidateRedirect(resp)
+	}
 	finished := trace.TimeSince(trace.ZeroTime())
 	t.TestKeys.AppendNetworkEvents(measurexlite.NewAnnotationArchivalNetworkEvent(
 		trace.Index(), finished, "http_transaction_done", trace.Tags()...,
@@ -297,14 +300,11 @@ 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:
+	if httpRedirectIsRedirect(resp) {
 		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,
@@ -324,7 +324,5 @@ func (t *CleartextFlow) maybeFollowRedirects(ctx context.Context, resp *http.Res
 			UDPAddress:   t.UDPAddress,
 		}
 		resolvers.Start(ctx)
-	default:
-		// no redirect to follow
 	}
 }

internal/experiment/webconnectivitylte/httpredirect.go

@@ -0,0 +1,46 @@
+package webconnectivitylte
+
+import (
+	"errors"
+	"net/http"
+
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
+)
+
+// httpRedirectIsRedirect returns whether this response is a redirect
+func httpRedirectIsRedirect(resp *http.Response) bool {
+	switch resp.StatusCode {
+	case 301, 302, 307, 308:
+		return true
+	default:
+		return false
+	}
+
+}
+
+var errHTTPValidateRedirectMissingRequest = errors.New("httpValidateRedirect: missing request")
+
+// httpValidateRedirect validates a redirect. In case of failure, the
+// returned error is a [*netxlite.ErrWrapper] instance.
+//
+// See https://github.com/ooni/probe/issues/2628 for context.
+func httpValidateRedirect(resp *http.Response) error {
+	// Implementation note: require the original request to be present otherwise we
+	// cannot distinguish between `/en-US/index.html` (which is legit) and `https://`
+	// (which instead is what we want to prevent from being used).
+	if resp.Request == nil {
+		return errHTTPValidateRedirectMissingRequest
+	}
+	location, err := resp.Location()
+	if err != nil {
+		return err
+	}
+	if location.Host == "" {
+		return &netxlite.ErrWrapper{
+			Failure:    netxlite.FailureHTTPInvalidRedirectLocationHost,
+			Operation:  netxlite.HTTPRoundTripOperation,
+			WrappedErr: nil,
+		}
+	}
+	return nil
+}

internal/experiment/webconnectivitylte/httpredirect_test.go

@@ -0,0 +1,136 @@
+package webconnectivitylte
+
+import (
+	"errors"
+	"fmt"
+	"net/http"
+	"net/url"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
+)
+
+func TestHTTPRedirectIsRedirect(t *testing.T) {
+	type testcase struct {
+		status int
+		expect bool
+	}
+
+	cases := []testcase{{
+		status: 100,
+		expect: false,
+	}, {
+		status: 200,
+		expect: false,
+	}, {
+		status: 300,
+		expect: false,
+	}, {
+		status: 301,
+		expect: true,
+	}, {
+		status: 302,
+		expect: true,
+	}, {
+		status: 304,
+		expect: false,
+	}, {
+		status: 305,
+		expect: false,
+	}, {
+		status: 306,
+		expect: false,
+	}, {
+		status: 307,
+		expect: true,
+	}, {
+		status: 308,
+		expect: true,
+	}, {
+		status: 309,
+		expect: false,
+	}, {
+		status: 400,
+		expect: false,
+	}, {
+		status: 500,
+		expect: false,
+	}}
+
+	for _, tc := range cases {
+		t.Run(fmt.Sprintf("%d", tc.status), func(t *testing.T) {
+			resp := &http.Response{StatusCode: tc.status}
+			got := httpRedirectIsRedirect(resp)
+			if diff := cmp.Diff(tc.expect, got); diff != "" {
+				t.Fatal(diff)
+			}
+		})
+	}
+}
+
+func TestHTTPValidateRedirect(t *testing.T) {
+	type testcase struct {
+		addReq   bool
+		location string
+		expect   error
+	}
+
+	cases := []testcase{{
+		addReq:   false,
+		location: "/en-US/index.html",
+		expect:   errHTTPValidateRedirectMissingRequest,
+	}, {
+		addReq:   true,
+		location: "", // explicitly empty
+		expect:   http.ErrNoLocation,
+	}, {
+		addReq:   true,
+		location: "http://",
+		expect:   errors.New(netxlite.FailureHTTPInvalidRedirectLocationHost),
+	}, {
+		addReq:   true,
+		location: "https://",
+		expect:   errors.New(netxlite.FailureHTTPInvalidRedirectLocationHost),
+	}, {
+		addReq:   true,
+		location: "/en-US/index.html",
+		expect:   nil,
+	}, {
+		addReq:   true,
+		location: "https://web01.example.com/",
+		expect:   nil,
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.location, func(t *testing.T) {
+			resp := &http.Response{Header: http.Header{}}
+			resp.Header.Set("Location", tc.location)
+			if tc.addReq {
+				resp.Request = &http.Request{URL: &url.URL{
+					Scheme: "https",
+					Host:   "www.example.com",
+					Path:   "/",
+				}}
+			}
+
+			got := httpValidateRedirect(resp)
+
+			switch {
+			case tc.expect == nil && got == nil:
+				// all good
+
+			case tc.expect == nil && got != nil:
+				t.Fatal("expected", tc.expect, "got", got)
+
+			case tc.expect != nil && got == nil:
+				t.Fatal("expected", tc.expect, "got", got)
+
+			case tc.expect != nil && got != nil:
+				if diff := cmp.Diff(tc.expect.Error(), got.Error()); diff != "" {
+					t.Fatal(diff)
+				}
+			}
+		})
+	}
+}

internal/experiment/webconnectivitylte/secureflow.go

@@ -324,6 +324,9 @@ func (t *SecureFlow) httpTransaction(ctx context.Context, network, address, alpn
 		reader := io.LimitReader(resp.Body, maxbody)
 		body, err = StreamAllContext(ctx, reader)
 	}
+	if err == nil && httpRedirectIsRedirect(resp) {
+		err = httpValidateRedirect(resp)
+	}
 	finished := trace.TimeSince(trace.ZeroTime())
 	t.TestKeys.AppendNetworkEvents(measurexlite.NewAnnotationArchivalNetworkEvent(
 		trace.Index(), finished, "http_transaction_done", trace.Tags()...,
@@ -352,14 +355,11 @@ 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:
+	if httpRedirectIsRedirect(resp) {
 		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,
@@ -379,7 +379,5 @@ func (t *SecureFlow) maybeFollowRedirects(ctx context.Context, resp *http.Respon
 			UDPAddress:   t.UDPAddress,
 		}
 		resolvers.Start(ctx)
-	default:
-		// no redirect to follow
 	}
 }

internal/experiment/webconnectivityqa/redirect.go

@@ -4,6 +4,7 @@ import (
 	"github.com/apex/log"
 	"github.com/ooni/netem"
 	"github.com/ooni/probe-cli/v3/internal/netemx"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
 // redirectWithConsistentDNSAndThenConnectionRefusedForHTTP is a scenario where the redirect
@@ -350,7 +351,7 @@ func redirectWithConsistentDNSAndThenTimeoutForHTTPS() *TestCase {
 func redirectWithBrokenLocationForHTTP() *TestCase {
 	return &TestCase{
 		Name:     "redirectWithBrokenLocationForHTTP",
-		Flags:    TestCaseFlagNoLTE,
+		Flags:    TestCaseFlagNoV04,
 		Input:    "http://httpbin.com/broken-redirect-http",
 		LongTest: true,
 		Configure: func(env *netemx.QAEnv) {
@@ -360,12 +361,12 @@ func redirectWithBrokenLocationForHTTP() *TestCase {
 		ExpectTestKeys: &testKeys{
 			DNSExperimentFailure:  nil,
 			DNSConsistency:        "consistent",
-			HTTPExperimentFailure: "unknown_failure: http: no Host in request URL",
+			HTTPExperimentFailure: netxlite.FailureHTTPInvalidRedirectLocationHost,
 			XStatus:               8192, // StatusExperimentHTTP
 			XDNSFlags:             0,
-			XBlockingFlags:        1, // AnalysisBlockingFlagDNSBlocking
-			Accessible:            nil,
-			Blocking:              nil,
+			XBlockingFlags:        8, // AnalysisBlockingFlagHTTPBlocking
+			Accessible:            false,
+			Blocking:              "http-failure",
 		},
 	}
 }
@@ -377,7 +378,7 @@ func redirectWithBrokenLocationForHTTP() *TestCase {
 func redirectWithBrokenLocationForHTTPS() *TestCase {
 	return &TestCase{
 		Name:     "redirectWithBrokenLocationForHTTPS",
-		Flags:    TestCaseFlagNoLTE,
+		Flags:    TestCaseFlagNoV04,
 		Input:    "https://httpbin.com/broken-redirect-https",
 		LongTest: true,
 		Configure: func(env *netemx.QAEnv) {
@@ -387,12 +388,12 @@ func redirectWithBrokenLocationForHTTPS() *TestCase {
 		ExpectTestKeys: &testKeys{
 			DNSExperimentFailure:  nil,
 			DNSConsistency:        "consistent",
-			HTTPExperimentFailure: "unknown_failure: http: no Host in request URL",
+			HTTPExperimentFailure: netxlite.FailureHTTPInvalidRedirectLocationHost,
 			XStatus:               8192, // StatusExperimentHTTP
 			XDNSFlags:             0,
-			XBlockingFlags:        1, // AnalysisBlockingFlagDNSBlocking
-			Accessible:            nil,
-			Blocking:              nil,
+			XBlockingFlags:        8, // AnalysisBlockingFlagHTTPBlocking
+			Accessible:            false,
+			Blocking:              "http-failure",
 		},
 	}
 }