From 09b57501ec47494f6c369d6def05de7c23d40669 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 19 Jun 2024 10:44:34 +0200 Subject: [PATCH] fix(dnscheck): gracefully cast to Target (#1623) Closes https://github.com/ooni/probe/issues/2753 --- docs/design/dd-008-richer-input.md | 24 +++++++++++++------ internal/experiment/dnscheck/dnscheck.go | 8 +++++-- internal/experiment/dnscheck/dnscheck_test.go | 14 +++++++++++ internal/targetloading/targetloading.go | 1 + 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/docs/design/dd-008-richer-input.md b/docs/design/dd-008-richer-input.md index 3dcce571e..7927d7974 100644 --- a/docs/design/dd-008-richer-input.md +++ b/docs/design/dd-008-richer-input.md @@ -33,7 +33,6 @@ the following command runs the `dnscheck` experiment measuring ./miniooni dnscheck -i https://8.8.8.8/dns-query -O HTTP3Enabled=true ``` - Additionally, OONI Run v2 allows to run experiments with options. For example, the following JSON document is equivalent to the previous `miniooni` command: @@ -151,7 +150,7 @@ Solving this problem is crucial because most OONI measurements run automatically in the background with input provided by the backend. Therefore, by enabling richer input, we open up the possibility of answering specific research questions requiring options at scale. For example, richer input would enable us to -study [DNS over HTTP/3 is blocking](https://github.com/ooni/probe/issues/2675)) +study [DNS over HTTP/3 blocking](https://github.com/ooni/probe/issues/2675)) at scale. ## Design choice: deprecating the check-in API @@ -159,8 +158,9 @@ at scale. We originally envisioned distributing richer input through the check-in API but we later realized that this design would be problematic because: -1. it prevents us from having experiments implemented as scripts, a solution -that we heavily explored while researching richer input; +1. it prevents us from having experiments implemented as scripts, [a solution +that we heavily explored while researching richer input]( +https://github.com/bassosimone/2023-12-09-ooni-javascript); 2. the check-in API serves URLs for Web Connectivity, which is the most important experiment we run, which means that changing the component serving @@ -343,12 +343,24 @@ the changes roughly look like this: + if args.Target == nil { + return ErrInputRequired + } -+ input := args.Target.(*target).input ++ input, ok := args.Target.(*target).input ++ if !ok { ++ return ErrInvalidInputType ++ } + options := args.Target.(*target).options // [...] } ``` +Note how we MUST gracefully cast to `*target` (as we did in [probe-cli#1623]( +https://github.com/ooni/probe-cli/pull/1623)) because richer input could +potentially come from ~any source, including the mobile app. While richer input +is anything that fullfills the `model.ExperimentTarget` interface, mobile apps +could, for example, construct a Java class implementing such an interface but we +wouldn't be able to cast such an interface to the `*target` type. Therefore, +unconditionally casting could lead to crashes when integrating new code +and generally makes for a less robust codebase. + ## Next steps This is a rough sequence of next steps that we should expand as we implement @@ -387,5 +399,3 @@ inside of the `targetloader` package to have multiple target loaders. * devise long term strategy for delivering richer input to `oonimkall` from mobile apps, which we'll need as soon as we convert the IM experiments - - diff --git a/internal/experiment/dnscheck/dnscheck.go b/internal/experiment/dnscheck/dnscheck.go index 49eed47d5..b5c1583b2 100644 --- a/internal/experiment/dnscheck/dnscheck.go +++ b/internal/experiment/dnscheck/dnscheck.go @@ -115,6 +115,7 @@ func (m *Measurer) ExperimentVersion() string { // that are used by this experiment to implement its functionality. var ( ErrInputRequired = targetloading.ErrInputRequired + ErrInvalidInputType = targetloading.ErrInvalidInputType ErrInvalidURL = errors.New("the input URL is invalid") ErrUnsupportedURLScheme = errors.New("unsupported URL scheme") ) @@ -125,11 +126,14 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error { measurement := args.Measurement sess := args.Session - // 0. obtain the richer input target, config, and input or panic + // 0. obtain the richer input target, config, and input or fail if args.Target == nil { return ErrInputRequired } - target := args.Target.(*Target) + target, ok := args.Target.(*Target) + if !ok { + return ErrInvalidInputType + } config, input := target.Options, target.URL sess.Logger().Infof("dnscheck: using richer input: %+v %+v", config, input) diff --git a/internal/experiment/dnscheck/dnscheck_test.go b/internal/experiment/dnscheck/dnscheck_test.go index 1deaf3d92..8f706ffa4 100644 --- a/internal/experiment/dnscheck/dnscheck_test.go +++ b/internal/experiment/dnscheck/dnscheck_test.go @@ -50,6 +50,20 @@ func TestExperimentNameAndVersion(t *testing.T) { } } +func TestDNSCheckFailsWithInvalidInputType(t *testing.T) { + measurer := NewExperimentMeasurer() + args := &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(log.Log), + Measurement: new(model.Measurement), + Session: newsession(), + Target: &model.OOAPIURLInfo{}, // not the expected input type + } + err := measurer.Run(context.Background(), args) + if !errors.Is(err, ErrInvalidInputType) { + t.Fatal("expected no input error") + } +} + func TestDNSCheckFailsWithoutInput(t *testing.T) { measurer := NewExperimentMeasurer() args := &model.ExperimentArgs{ diff --git a/internal/targetloading/targetloading.go b/internal/targetloading/targetloading.go index c6c91162c..54540f54a 100644 --- a/internal/targetloading/targetloading.go +++ b/internal/targetloading/targetloading.go @@ -24,6 +24,7 @@ var ( ErrInputRequired = errors.New("no input provided") ErrNoInputExpected = errors.New("we did not expect any input") ErrNoStaticInput = errors.New("no static input for this experiment") + ErrInvalidInputType = errors.New("invalid richer input type") ) // Session is the session according to a [*Loader] instance.