diff --git a/internal/experiment/webconnectivitylte/analysisclassic.go b/internal/experiment/webconnectivitylte/analysisclassic.go index 0db70b7c5..168c70a85 100644 --- 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 index 2824fccad..6b482c42c 100644 --- 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 index 66b3af2a1..9b61e5512 100644 --- 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 index 882a05d0b..5a2e238ee 100644 --- 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 new file mode 100644 index 000000000..03c9b56d2 --- /dev/null +++ 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 index fef37d4a4..ba2284622 100644 --- 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 index 2cb6a7b99..d563c4b3e 100644 --- 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 index 64630fee1..b85cd2789 100644 --- 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 index bf814b433..8a2d24a1c 100644 --- 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 index fbaf99125..19e4fd177 100644 --- a/internal/webconnectivityqa/testcase.go +++ b/internal/webconnectivityqa/testcase.go @@ -90,6 +90,8 @@ func AllTestCases() []*TestCase { redirectWithConsistentDNSAndThenEOFForHTTPS(), redirectWithConsistentDNSAndThenTimeoutForHTTP(), redirectWithConsistentDNSAndThenTimeoutForHTTPS(), + redirectWithMoreThanTenRedirectsAndHTTP(), + redirectWithMoreThanTenRedirectsAndHTTPS(), successWithHTTP(), successWithHTTPS(),