From ff42f3cb16e17839cae021b7ef1801c6b908ac18 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 24 Nov 2023 10:15:15 +0100 Subject: [PATCH] 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. --- .../webconnectivitylte/analysishttpcore.go | 12 ++ .../webconnectivitylte/cleartextflow.go | 93 ++++++------- .../webconnectivitylte/httpfailure.go | 28 ---- .../webconnectivitylte/httpredirect.go | 65 +++++++++ .../webconnectivitylte/secureflow.go | 127 +++++++++--------- 5 files changed, 188 insertions(+), 137 deletions(-) delete mode 100644 internal/experiment/webconnectivitylte/httpfailure.go create mode 100644 internal/experiment/webconnectivitylte/httpredirect.go diff --git a/internal/experiment/webconnectivitylte/analysishttpcore.go b/internal/experiment/webconnectivitylte/analysishttpcore.go index 794bfbd871..6113489e44 100644 --- 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 index 4af51d5c82..264e7553c6 100644 --- 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 deleted file mode 100644 index 4d0ef73cf4..0000000000 --- a/internal/experiment/webconnectivitylte/httpfailure.go +++ /dev/null @@ -1,28 +0,0 @@ -package webconnectivitylte - -import ( - "net/http" - - "github.com/ooni/probe-cli/v3/internal/measurexlite" - "github.com/ooni/probe-cli/v3/internal/model" -) - -// 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, - ) -} diff --git a/internal/experiment/webconnectivitylte/httpredirect.go b/internal/experiment/webconnectivitylte/httpredirect.go new file mode 100644 index 0000000000..0af52a21c3 --- /dev/null +++ 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 index fcedfde511..a000af66d0 100644 --- 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) }