From a3bb17c1ab73c4a25f3e9126de2fdfe956a55a7d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 12 Apr 2024 16:40:19 +0200 Subject: [PATCH 1/2] fix(webconnectivitylte): handle too many redirects Closes https://github.com/ooni/probe/issues/2685 --- .../webconnectivitylte/analysisclassic.go | 2 +- .../webconnectivitylte/cleartextflow.go | 8 +- .../experiment/webconnectivitylte/measurer.go | 2 +- .../webconnectivitylte/redirects.go | 2 +- .../webconnectivitylte/redirects_test.go | 16 ++++ .../webconnectivitylte/secureflow.go | 12 ++- internal/netemx/httpbin.go | 19 ++++- internal/netemx/httpbin_test.go | 82 +++++++++++++++++++ internal/webconnectivityqa/redirect.go | 44 ++++++++++ internal/webconnectivityqa/testcase.go | 2 + 10 files changed, 177 insertions(+), 12 deletions(-) create mode 100644 internal/experiment/webconnectivitylte/redirects_test.go diff --git a/internal/experiment/webconnectivitylte/analysisclassic.go b/internal/experiment/webconnectivitylte/analysisclassic.go index 0db70b7c5b..168c70a85e 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 2824fccad5..6b482c42c4 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 66b3af2a11..9b61e5512e 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 882a05d0be..5a2e238ee4 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 0000000000..03c9b56d23 --- /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 fef37d4a4a..ba22846220 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 2cb6a7b990..d563c4b3e2 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 64630fee17..b85cd27898 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 bf814b4332..7081c44344 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: 8192, // StatusExperimentHTTP + 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: 8192, // StatusExperimentHTTP + XDNSFlags: 0, + XBlockingFlags: 0, + Accessible: false, + Blocking: false, + }, + } +} diff --git a/internal/webconnectivityqa/testcase.go b/internal/webconnectivityqa/testcase.go index fbaf991254..19e4fd1776 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(), From 7fce0aeb6d80e2d434078a795b564eeb2196f61b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 12 Apr 2024 16:44:55 +0200 Subject: [PATCH 2/2] x --- internal/webconnectivityqa/redirect.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/webconnectivityqa/redirect.go b/internal/webconnectivityqa/redirect.go index 7081c44344..8a2d24a1c0 100644 --- a/internal/webconnectivityqa/redirect.go +++ b/internal/webconnectivityqa/redirect.go @@ -405,7 +405,7 @@ func redirectWithMoreThanTenRedirectsAndHTTP() *TestCase { DNSExperimentFailure: nil, DNSConsistency: "consistent", HTTPExperimentFailure: `unknown_failure: stopped after too many redirects`, - XStatus: 8192, // StatusExperimentHTTP + XStatus: 0, XDNSFlags: 0, XBlockingFlags: 0, Accessible: false, @@ -427,7 +427,7 @@ func redirectWithMoreThanTenRedirectsAndHTTPS() *TestCase { DNSExperimentFailure: nil, DNSConsistency: "consistent", HTTPExperimentFailure: `unknown_failure: stopped after too many redirects`, - XStatus: 8192, // StatusExperimentHTTP + XStatus: 0, XDNSFlags: 0, XBlockingFlags: 0, Accessible: false,