[backport] fix(webconnectivitylte): handle too many redirects (#1550)

Closes ooni/probe#2685
ooni · Apr 12, 2024 · 7569bd8 · 7569bd8
1 parent 22381f6
commit 7569bd8
Show file tree

Hide file tree

Showing 10 changed files with 177 additions and 12 deletions.
diff --git a/internal/experiment/webconnectivitylte/analysisclassic.go b/internal/experiment/webconnectivitylte/analysisclassic.go
@@ -25,7 +25,7 @@ func analysisEngineClassic(tk *TestKeys, logger model.Logger) {
 	tk.analysisClassic(model.GeoIPASNLookupperFunc(geoipx.LookupASN), logger)
 }
 
-func (tk *TestKeys) analysisClassic(lookupper model.GeoIPASNLookupper, logger model.Logger) {
+func (tk *TestKeys) analysisClassic(lookupper model.GeoIPASNLookupper, _ model.Logger) {
 	// Since we run after all tasks have completed (or so we assume) we're
 	// not going to use any form of locking here.
 

diff --git a/internal/experiment/webconnectivitylte/cleartextflow.go b/internal/experiment/webconnectivitylte/cleartextflow.go
@@ -282,6 +282,9 @@ func (t *CleartextFlow) httpTransaction(ctx context.Context, network, address, a
 	}
 	if err == nil && httpRedirectIsRedirect(resp) {
 		err = httpValidateRedirect(resp)
+		if err == nil && t.FollowRedirects && !t.NumRedirects.CanFollowOneMoreRedirect() {
+			err = ErrTooManyRedirects
+		}
 	}
 
 	finished := trace.TimeSince(trace.ZeroTime())
@@ -319,10 +322,7 @@ func (t *CleartextFlow) httpTransaction(ctx context.Context, network, address, a
 
 // maybeFollowRedirects follows redirects if configured and needed
 func (t *CleartextFlow) maybeFollowRedirects(ctx context.Context, resp *http.Response) {
-	if !t.FollowRedirects || !t.NumRedirects.CanFollowOneMoreRedirect() {
-		return // not configured or too many redirects
-	}
-	if httpRedirectIsRedirect(resp) {
+	if t.FollowRedirects && httpRedirectIsRedirect(resp) {
 		location, err := resp.Location()
 		if err != nil {
 			return // broken response from server

diff --git a/internal/experiment/webconnectivitylte/measurer.go b/internal/experiment/webconnectivitylte/measurer.go
@@ -121,7 +121,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 		Domain:                  URL.Hostname(),
 		IDGenerator:             NewIDGenerator(),
 		Logger:                  sess.Logger(),
-		NumRedirects:            NewNumRedirects(5),
+		NumRedirects:            NewNumRedirects(10),
 		TestKeys:                tk,
 		URL:                     URL,
 		ZeroTime:                measurement.MeasurementStartTimeSaved,

diff --git a/internal/experiment/webconnectivitylte/redirects.go b/internal/experiment/webconnectivitylte/redirects.go
@@ -19,5 +19,5 @@ func NewNumRedirects(n int64) *NumRedirects {
 // CanFollowOneMoreRedirect returns true if we are
 // allowed to follow one more redirect.
 func (nr *NumRedirects) CanFollowOneMoreRedirect() bool {
-	return nr.count.Add(-1) > 0
+	return nr.count.Add(-1) >= 0
 }
diff --git a/internal/experiment/webconnectivitylte/redirects_test.go b/internal/experiment/webconnectivitylte/redirects_test.go
@@ -0,0 +1,16 @@
+package webconnectivitylte
+
+import "testing"
+
+func TestNumRedirects(t *testing.T) {
+	const count = 10
+	nr := NewNumRedirects(count)
+	for idx := 0; idx < count; idx++ {
+		if !nr.CanFollowOneMoreRedirect() {
+			t.Fatal("got false with idx=", idx)
+		}
+	}
+	if nr.CanFollowOneMoreRedirect() {
+		t.Fatal("got true after the loop")
+	}
+}
diff --git a/internal/experiment/webconnectivitylte/secureflow.go b/internal/experiment/webconnectivitylte/secureflow.go
@@ -9,6 +9,7 @@ package webconnectivitylte
 import (
 	"context"
 	"crypto/tls"
+	"errors"
 	"io"
 	"net"
 	"net/http"
@@ -305,6 +306,9 @@ func (t *SecureFlow) newHTTPRequest(ctx context.Context) (*http.Request, error)
 	return httpReq, nil
 }
 
+// ErrTooManyRedirects indicates we have seen too many HTTP redirects.
+var ErrTooManyRedirects = errors.New("stopped after too many redirects")
+
 // httpTransaction runs the HTTP transaction and saves the results.
 func (t *SecureFlow) httpTransaction(ctx context.Context, network, address, alpn string,
 	txp model.HTTPTransport, req *http.Request, trace *measurexlite.Trace) (*http.Response, []byte, error) {
@@ -337,6 +341,9 @@ func (t *SecureFlow) httpTransaction(ctx context.Context, network, address, alpn
 	}
 	if err == nil && httpRedirectIsRedirect(resp) {
 		err = httpValidateRedirect(resp)
+		if err == nil && t.FollowRedirects && !t.NumRedirects.CanFollowOneMoreRedirect() {
+			err = ErrTooManyRedirects
+		}
 	}
 
 	finished := trace.TimeSince(trace.ZeroTime())
@@ -374,10 +381,7 @@ func (t *SecureFlow) httpTransaction(ctx context.Context, network, address, alpn
 
 // maybeFollowRedirects follows redirects if configured and needed
 func (t *SecureFlow) maybeFollowRedirects(ctx context.Context, resp *http.Response) {
-	if !t.FollowRedirects || !t.NumRedirects.CanFollowOneMoreRedirect() {
-		return // not configured or too many redirects
-	}
-	if httpRedirectIsRedirect(resp) {
+	if t.FollowRedirects && httpRedirectIsRedirect(resp) {
 		location, err := resp.Location()
 		if err != nil {
 			return // broken response from server

diff --git a/internal/netemx/httpbin.go b/internal/netemx/httpbin.go
@@ -1,8 +1,11 @@
 package netemx
 
 import (
+	"fmt"
 	"net"
 	"net/http"
+	"strconv"
+	"strings"
 
 	"github.com/ooni/netem"
 )
@@ -29,7 +32,7 @@ func HTTPBinHandlerFactory() HTTPHandlerFactory {
 // Any other request URL causes a 404 respose.
 func HTTPBinHandler() http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.Header().Add("Date", "Thu, 24 Aug 2023 14:35:29 GMT")
+		w.Header().Set("Date", "Thu, 24 Aug 2023 14:35:29 GMT")
 
 		// missing address => 500
 		address, _, err := net.SplitHostPort(r.RemoteAddr)
@@ -44,6 +47,20 @@ func HTTPBinHandler() http.Handler {
 		secureRedirect := r.URL.Path == "/broken-redirect-https"
 
 		switch {
+		// redirect with count
+		case strings.HasPrefix(r.URL.Path, "/redirect/"):
+			count, err := strconv.Atoi(strings.TrimPrefix(r.URL.Path, "/redirect/"))
+			if err != nil {
+				w.WriteHeader(http.StatusBadRequest)
+				return
+			}
+			if count <= 0 {
+				w.WriteHeader(http.StatusOK)
+				return
+			}
+			w.Header().Set("Location", fmt.Sprintf("/redirect/%d", count-1))
+			w.WriteHeader(http.StatusFound)
+
 		// broken HTTP redirect for clients
 		case cleartextRedirect && client:
 			w.Header().Set("Location", "http://")

diff --git a/internal/netemx/httpbin_test.go b/internal/netemx/httpbin_test.go
@@ -25,6 +25,88 @@ func TestHTTPBinHandler(t *testing.T) {
 		}
 	})
 
+	t.Run("/redirect/{n} with invalid number", func(t *testing.T) {
+		req := &http.Request{
+			URL:        &url.URL{Scheme: "https://", Path: "/redirect/antani"},
+			Body:       http.NoBody,
+			Close:      false,
+			Host:       "httpbin.com",
+			RemoteAddr: net.JoinHostPort("8.8.8.8", "54321"),
+		}
+		rr := httptest.NewRecorder()
+		handler := HTTPBinHandler()
+		handler.ServeHTTP(rr, req)
+		result := rr.Result()
+		if result.StatusCode != http.StatusBadRequest {
+			t.Fatal("unexpected status code", result.StatusCode)
+		}
+	})
+
+	t.Run("/redirect/0", func(t *testing.T) {
+		req := &http.Request{
+			URL:        &url.URL{Scheme: "https://", Path: "/redirect/0"},
+			Body:       http.NoBody,
+			Close:      false,
+			Host:       "httpbin.com",
+			RemoteAddr: net.JoinHostPort("8.8.8.8", "54321"),
+		}
+		rr := httptest.NewRecorder()
+		handler := HTTPBinHandler()
+		handler.ServeHTTP(rr, req)
+		result := rr.Result()
+		if result.StatusCode != http.StatusOK {
+			t.Fatal("unexpected status code", result.StatusCode)
+		}
+	})
+
+	t.Run("/redirect/1", func(t *testing.T) {
+		req := &http.Request{
+			URL:        &url.URL{Scheme: "https://", Path: "/redirect/1"},
+			Body:       http.NoBody,
+			Close:      false,
+			Host:       "httpbin.com",
+			RemoteAddr: net.JoinHostPort("8.8.8.8", "54321"),
+		}
+		rr := httptest.NewRecorder()
+		handler := HTTPBinHandler()
+		handler.ServeHTTP(rr, req)
+		result := rr.Result()
+		if result.StatusCode != http.StatusFound {
+			t.Fatal("unexpected status code", result.StatusCode)
+		}
+		location, err := result.Location()
+		if err != nil {
+			t.Fatal(err)
+		}
+		if location.Path != "/redirect/0" {
+			t.Fatal("unexpected location.Path", location.Path)
+		}
+	})
+
+	t.Run("/redirect/2", func(t *testing.T) {
+		req := &http.Request{
+			URL:        &url.URL{Scheme: "https://", Path: "/redirect/2"},
+			Body:       http.NoBody,
+			Close:      false,
+			Host:       "httpbin.com",
+			RemoteAddr: net.JoinHostPort("8.8.8.8", "54321"),
+		}
+		rr := httptest.NewRecorder()
+		handler := HTTPBinHandler()
+		handler.ServeHTTP(rr, req)
+		result := rr.Result()
+		if result.StatusCode != http.StatusFound {
+			t.Fatal("unexpected status code", result.StatusCode)
+		}
+		location, err := result.Location()
+		if err != nil {
+			t.Fatal(err)
+		}
+		if location.Path != "/redirect/1" {
+			t.Fatal("unexpected location.Path", location.Path)
+		}
+	})
+
 	t.Run("/broken-redirect-http with client address", func(t *testing.T) {
 		req := &http.Request{
 			URL:        &url.URL{Scheme: "http://", Path: "/broken-redirect-http"},

diff --git a/internal/webconnectivityqa/redirect.go b/internal/webconnectivityqa/redirect.go
@@ -391,3 +391,47 @@ func redirectWithBrokenLocationForHTTPS() *TestCase {
 		},
 	}
 }
+
+// redirectWithMoreThanTenRedirectsAndHTTP is a scenario where the redirect
+// consists of more than ten redirects for http:// URLs.
+func redirectWithMoreThanTenRedirectsAndHTTP() *TestCase {
+	return &TestCase{
+		Name:      "redirectWithMoreThanTenRedirectsAndHTTP",
+		Flags:     TestCaseFlagNoV04,
+		Input:     "http://httpbin.com/redirect/15",
+		LongTest:  true,
+		ExpectErr: false,
+		ExpectTestKeys: &TestKeys{
+			DNSExperimentFailure:  nil,
+			DNSConsistency:        "consistent",
+			HTTPExperimentFailure: `unknown_failure: stopped after too many redirects`,
+			XStatus:               0,
+			XDNSFlags:             0,
+			XBlockingFlags:        0,
+			Accessible:            false,
+			Blocking:              false,
+		},
+	}
+}
+
+// redirectWithMoreThanTenRedirectsAndHTTPS is a scenario where the redirect
+// consists of more than ten redirects for https:// URLs.
+func redirectWithMoreThanTenRedirectsAndHTTPS() *TestCase {
+	return &TestCase{
+		Name:      "redirectWithMoreThanTenRedirectsAndHTTPS",
+		Flags:     TestCaseFlagNoV04,
+		Input:     "https://httpbin.com/redirect/15",
+		LongTest:  true,
+		ExpectErr: false,
+		ExpectTestKeys: &TestKeys{
+			DNSExperimentFailure:  nil,
+			DNSConsistency:        "consistent",
+			HTTPExperimentFailure: `unknown_failure: stopped after too many redirects`,
+			XStatus:               0,
+			XDNSFlags:             0,
+			XBlockingFlags:        0,
+			Accessible:            false,
+			Blocking:              false,
+		},
+	}
+}
diff --git a/internal/webconnectivityqa/testcase.go b/internal/webconnectivityqa/testcase.go
@@ -90,6 +90,8 @@ func AllTestCases() []*TestCase {
 		redirectWithConsistentDNSAndThenEOFForHTTPS(),
 		redirectWithConsistentDNSAndThenTimeoutForHTTP(),
 		redirectWithConsistentDNSAndThenTimeoutForHTTPS(),
+		redirectWithMoreThanTenRedirectsAndHTTP(),
+		redirectWithMoreThanTenRedirectsAndHTTPS(),
 
 		successWithHTTP(),
 		successWithHTTPS(),