fix(webconnectivitylte): include network events

internal/experiment/webconnectivitylte/cleartextflow.go

@@ -124,7 +124,14 @@ 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()...)
-	defer t.TestKeys.AppendNetworkEvents(trace.NetworkEvents()...) // here to include "connect" events
+	defer func() {
+		// BUGFIX: we must call trace.NetworkEvents()... inside the defer block otherwise
+		// we miss the read/write network events. See https://github.com/ooni/probe/issues/2674.
+		//
+		// Additionally, we must register this defer here because we want to include
+		// the "connect" event in case connect has failed.
+		t.TestKeys.AppendNetworkEvents(trace.NetworkEvents()...)
+	}()
 	if err != nil {
 		ol.Stop(err)
 		return err

internal/experiment/webconnectivitylte/secureflow.go

@@ -131,7 +131,14 @@ 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()...)
-	defer t.TestKeys.AppendNetworkEvents(trace.NetworkEvents()...) // here to include "connect" events
+	defer func() {
+		// BUGFIX: we must call trace.NetworkEvents()... inside the defer block otherwise
+		// we miss the read/write network events. See https://github.com/ooni/probe/issues/2674.
+		//
+		// Additionally, we must register this defer here because we want to include
+		// the "connect" event in case connect has failed.
+		t.TestKeys.AppendNetworkEvents(trace.NetworkEvents()...)
+	}()
 	if err != nil {
 		ol.Stop(err)
 		return err

internal/webconnectivityqa/badssl.go

@@ -16,7 +16,7 @@ func badSSLWithExpiredCertificate() *TestCase {
 			// nothing
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency:        "consistent",
 			HTTPExperimentFailure: "ssl_invalid_certificate",
 			XStatus:               16, // StatusAnomalyControlFailure
@@ -38,7 +38,7 @@ func badSSLWithWrongServerName() *TestCase {
 			// nothing
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency:        "consistent",
 			HTTPExperimentFailure: "ssl_invalid_hostname",
 			XStatus:               16, // StatusAnomalyControlFailure
@@ -59,7 +59,7 @@ func badSSLWithUnknownAuthorityWithConsistentDNS() *TestCase {
 			// nothing
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency:        "consistent",
 			HTTPExperimentFailure: "ssl_unknown_authority",
 			XStatus:               16, // StatusAnomalyControlFailure
@@ -88,7 +88,7 @@ func badSSLWithUnknownAuthorityWithInconsistentDNS() *TestCase {
 
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency:        "inconsistent",
 			HTTPExperimentFailure: "ssl_unknown_authority",
 			XStatus:               9248, // StatusExperimentHTTP | StatusAnomalyTLSHandshake | StatusAnomalyDNS

internal/webconnectivityqa/checker.go

@@ -0,0 +1,61 @@
+package webconnectivityqa
+
+import (
+	"errors"
+	"strings"
+
+	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/must"
+	"github.com/ooni/probe-cli/v3/internal/netxlite"
+	"github.com/ooni/probe-cli/v3/internal/x/dslx"
+)
+
+// Checker checks whether a measurement is correct.
+type Checker interface {
+	Check(mx *model.Measurement) error
+}
+
+// ReadWriteEventsExistentialChecker fails if there are zero network events.
+type ReadWriteEventsExistentialChecker struct{}
+
+var _ Checker = &ReadWriteEventsExistentialChecker{}
+
+// ErrCheckerNoReadWriteEvents indicates that a checker did not find any read/write events.
+var ErrCheckerNoReadWriteEvents = errors.New("no read or write events")
+
+// ErrCheckerUnexpectedWebConnectivityVersion indicates that the version is unexpected
+var ErrCheckerUnexpectedWebConnectivityVersion = errors.New("unexpected Web Connectivity version")
+
+// Check implements Checker.
+func (*ReadWriteEventsExistentialChecker) Check(mx *model.Measurement) error {
+	// we don't care about v0.4
+	if strings.HasPrefix(mx.TestVersion, "0.4.") {
+		return nil
+	}
+
+	// make sure it's v0.5
+	if !strings.HasPrefix(mx.TestVersion, "0.5.") {
+		return ErrCheckerUnexpectedWebConnectivityVersion
+	}
+
+	// serialize and reparse the test keys
+	var tk *dslx.Observations
+	must.UnmarshalJSON(must.MarshalJSON(mx.TestKeys), &tk)
+
+	// count the read/write events
+	var count int
+	for _, ev := range tk.NetworkEvents {
+		switch ev.Operation {
+		case netxlite.ReadOperation, netxlite.WriteOperation:
+			count++
+		default:
+			// nothing
+		}
+	}
+
+	// make sure there's at least one network event
+	if count <= 0 {
+		return ErrCheckerNoReadWriteEvents
+	}
+	return nil
+}

internal/webconnectivityqa/checker_test.go

@@ -0,0 +1,112 @@
+package webconnectivityqa_test
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/ooni/probe-cli/v3/internal/experiment/webconnectivitylte"
+	"github.com/ooni/probe-cli/v3/internal/mocks"
+	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/must"
+	"github.com/ooni/probe-cli/v3/internal/optional"
+	"github.com/ooni/probe-cli/v3/internal/webconnectivityqa"
+)
+
+func TestConfigureCustomCheckers(t *testing.T) {
+	tc := &webconnectivityqa.TestCase{
+		Name:      "",
+		Input:     "",
+		ExpectErr: false,
+		ExpectTestKeys: &webconnectivityqa.TestKeys{
+			Accessible: true,
+			Blocking:   nil,
+		},
+		Checkers: []webconnectivityqa.Checker{&webconnectivityqa.ReadWriteEventsExistentialChecker{}},
+	}
+	measurer := &mocks.ExperimentMeasurer{
+		MockExperimentName: func() string {
+			return "web_connectivity"
+		},
+		MockExperimentVersion: func() string {
+			return "0.5.28"
+		},
+		MockRun: func(ctx context.Context, args *model.ExperimentArgs) error {
+			args.Measurement.TestKeys = &webconnectivitylte.TestKeys{
+				Accessible: optional.Some(true),
+				Blocking:   nil,
+			}
+			return nil
+		},
+	}
+	err := webconnectivityqa.RunTestCase(measurer, tc)
+	if !errors.Is(err, webconnectivityqa.ErrCheckerNoReadWriteEvents) {
+		t.Fatal("unexpected error", err)
+	}
+}
+
+func TestReadWriteEventsExistentialChecker(t *testing.T) {
+	type testcase struct {
+		name    string
+		version string
+		tk      string
+		expect  error
+	}
+
+	cases := []testcase{{
+		name:    "with Web Connectivity v0.4",
+		version: "0.4.3",
+		tk:      `{}`,
+		expect:  nil,
+	}, {
+		name:    "with Web Connectivity v0.6",
+		version: "0.6.0",
+		tk:      `{}`,
+		expect:  webconnectivityqa.ErrCheckerUnexpectedWebConnectivityVersion,
+	}, {
+		name:    "with read/write network events",
+		version: "0.5.28",
+		tk:      `{"network_events":[{"operation":"read"},{"operation":"write"}]}`,
+		expect:  nil,
+	}, {
+		name:    "without network events",
+		version: "0.5.28",
+		tk:      `{"network_events":[]}`,
+		expect:  webconnectivityqa.ErrCheckerNoReadWriteEvents,
+	}, {
+		name:    "with no read/write network events",
+		version: "0.5.28",
+		tk:      `{"network_events":[{"operation":"connect"},{"operation":"close"}]}`,
+		expect:  webconnectivityqa.ErrCheckerNoReadWriteEvents,
+	}}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			var tks map[string]any
+			must.UnmarshalJSON([]byte(tc.tk), &tks)
+
+			meas := &model.Measurement{
+				TestKeys:    tks,
+				TestVersion: tc.version,
+			}
+
+			err := (&webconnectivityqa.ReadWriteEventsExistentialChecker{}).Check(meas)
+
+			switch {
+			case tc.expect == nil && err == nil:
+				return
+
+			case tc.expect == nil && err != nil:
+				t.Fatal("expected", tc.expect, "got", err)
+
+			case tc.expect != nil && err == nil:
+				t.Fatal("expected", tc.expect, "got", err)
+
+			case tc.expect != nil && err != nil:
+				if err.Error() != tc.expect.Error() {
+					t.Fatal("expected", tc.expect, "got", err)
+				}
+			}
+		})
+	}
+}

internal/webconnectivityqa/cloudflare.go

@@ -1,20 +1,15 @@
 package webconnectivityqa
 
-import "github.com/ooni/probe-cli/v3/internal/netemx"
-
 // cloudflareCAPTCHAWithHTTP obtains the cloudflare CAPTCHA using HTTP.
 func cloudflareCAPTCHAWithHTTP() *TestCase {
 	// See https://github.com/ooni/probe/issues/2661 for an explanation of why
 	// here for now we're forced to declare "http-diff".
 	return &TestCase{
-		Name:  "cloudflareCAPTCHAWithHTTP",
-		Flags: TestCaseFlagNoV04,
-		Input: "http://www.cloudflare-cache.com/",
-		Configure: func(env *netemx.QAEnv) {
-			// nothing
-		},
+		Name:      "cloudflareCAPTCHAWithHTTP",
+		Flags:     TestCaseFlagNoV04,
+		Input:     "http://www.cloudflare-cache.com/",
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency:  "consistent",
 			StatusCodeMatch: false,
 			BodyLengthMatch: false,
@@ -31,14 +26,11 @@ func cloudflareCAPTCHAWithHTTP() *TestCase {
 // cloudflareCAPTCHAWithHTTPS obtains the cloudflare CAPTCHA using HTTPS.
 func cloudflareCAPTCHAWithHTTPS() *TestCase {
 	return &TestCase{
-		Name:  "cloudflareCAPTCHAWithHTTPS",
-		Flags: TestCaseFlagNoV04,
-		Input: "https://www.cloudflare-cache.com/",
-		Configure: func(env *netemx.QAEnv) {
-			// nothing
-		},
+		Name:      "cloudflareCAPTCHAWithHTTPS",
+		Flags:     TestCaseFlagNoV04,
+		Input:     "https://www.cloudflare-cache.com/",
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency:  "consistent",
 			StatusCodeMatch: false,
 			BodyLengthMatch: false,

internal/webconnectivityqa/control.go

@@ -42,7 +42,7 @@ func controlFailureWithSuccessfulHTTPWebsite() *TestCase {
 
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			ControlFailure: "unknown_failure: httpapi: all endpoints failed: [ connection_reset; connection_reset; connection_reset; connection_reset;]",
 			XStatus:        8, // StatusAnomalyControlUnreachable
 			Accessible:     nil,
@@ -87,7 +87,7 @@ func controlFailureWithSuccessfulHTTPSWebsite() *TestCase {
 
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			ControlFailure: "unknown_failure: httpapi: all endpoints failed: [ connection_reset; connection_reset; connection_reset; connection_reset;]",
 			XStatus:        1,  // StatusSuccessSecure
 			XBlockingFlags: 32, // AnalysisBlockingFlagSuccess

internal/webconnectivityqa/dnsblocking.go

@@ -20,7 +20,7 @@ func dnsBlockingAndroidDNSCacheNoData() *TestCase {
 			env.ISPResolverConfig().RemoveRecord("www.example.com")
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSExperimentFailure:  "android_dns_cache_no_data",
 			HTTPExperimentFailure: "android_dns_cache_no_data",
 			DNSConsistency:        "inconsistent",
@@ -52,7 +52,7 @@ func dnsBlockingNXDOMAIN() *TestCase {
 			env.ISPResolverConfig().RemoveRecord("www.example.com")
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSExperimentFailure:  "dns_nxdomain_error",
 			HTTPExperimentFailure: "dns_nxdomain_error",
 			DNSConsistency:        "inconsistent",
@@ -76,7 +76,7 @@ func dnsBlockingBOGON() *TestCase {
 			env.ISPResolverConfig().AddRecord("www.example.com", "", "10.10.34.35")
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			HTTPExperimentFailure: "generic_timeout_error",
 			DNSExperimentFailure:  nil,
 			DNSConsistency:        "inconsistent",

internal/webconnectivityqa/dnshijacking.go

@@ -26,7 +26,7 @@ func dnsHijackingToProxyWithHTTPURL() *TestCase {
 
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency:  "inconsistent",
 			BodyLengthMatch: true,
 			BodyProportion:  1,
@@ -63,7 +63,7 @@ func dnsHijackingToProxyWithHTTPSURL() *TestCase {
 
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency:  "inconsistent",
 			BodyLengthMatch: true,
 			BodyProportion:  1,
@@ -97,7 +97,7 @@ func dnsHijackingToLocalhostWithHTTP() *TestCase {
 
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency: "inconsistent",
 			XDNSFlags:      5,  // AnalysisFlagDNSBogon | AnalysisDNSFlagUnexpectedAddrs
 			XBlockingFlags: 33, // AnalysisBlockingFlagDNSBlocking | AnalysisBlockingFlagSuccess
@@ -125,7 +125,7 @@ func dnsHijackingToLocalhostWithHTTPS() *TestCase {
 
 		},
 		ExpectErr: false,
-		ExpectTestKeys: &testKeys{
+		ExpectTestKeys: &TestKeys{
 			DNSConsistency: "inconsistent",
 			XDNSFlags:      5,  // AnalysisFlagDNSBogon | AnalysisDNSFlagUnexpectedAddrs
 			XBlockingFlags: 33, // AnalysisBlockingFlagDNSBlocking | AnalysisBlockingFlagSuccess