From 3b27780836714f8d9440633a9257e66a48c7f5f7 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 5 Nov 2021 13:51:22 +0100 Subject: [PATCH] fix(webconnectivity): ignore any status code <= 0 (#579) This diff changes the algorithm used by webconnectivity's httpanalysis.go to ignore any status code <= 0 rather than just ignoring the == 0 case. Make sure we add test cases for when the control's status code is negative rather than being zero. While there, simplify code where boolean checks could be more compact according to staticcheck. Closes https://github.com/ooni/probe/issues/1825 --- .../webconnectivity/httpanalysis.go | 14 ++-- .../webconnectivity/httpanalysis_test.go | 74 +++++++++++++++++++ .../experiment/webconnectivity/summary.go | 2 +- 3 files changed, 82 insertions(+), 8 deletions(-) diff --git a/internal/engine/experiment/webconnectivity/httpanalysis.go b/internal/engine/experiment/webconnectivity/httpanalysis.go index 757acac..b8f3e00 100644 --- a/internal/engine/experiment/webconnectivity/httpanalysis.go +++ b/internal/engine/experiment/webconnectivity/httpanalysis.go @@ -80,14 +80,14 @@ func HTTPStatusCodeMatch(tk urlgetter.TestKeys, ctrl ControlResponse) (out *bool return // no real status code } measurement := tk.Requests[0].Response.Code - if control == 0 { + if control <= 0 { return // no real status code } - if measurement == 0 { + if measurement <= 0 { return // no real status code } value := control == measurement - if value == true { + if value { // if the status codes are equal, they clearly match out = &value return @@ -110,10 +110,10 @@ func HTTPHeadersMatch(tk urlgetter.TestKeys, ctrl ControlResponse) *bool { if len(tk.Requests) <= 0 { return nil } - if tk.Requests[0].Response.Code == 0 { + if tk.Requests[0].Response.Code <= 0 { return nil } - if ctrl.HTTPRequest.StatusCode == 0 { + if ctrl.HTTPRequest.StatusCode <= 0 { return nil } control := ctrl.HTTPRequest.Headers @@ -203,13 +203,13 @@ func HTTPTitleMatch(tk urlgetter.TestKeys, ctrl ControlResponse) (out *bool) { return } response := tk.Requests[0].Response - if response.Code == 0 { + if response.Code <= 0 { return } if response.BodyIsTruncated { return } - if ctrl.HTTPRequest.StatusCode == 0 { + if ctrl.HTTPRequest.StatusCode <= 0 { return } control := ctrl.HTTPRequest.Title diff --git a/internal/engine/experiment/webconnectivity/httpanalysis_test.go b/internal/engine/experiment/webconnectivity/httpanalysis_test.go index 54264e9..2b6ee9f 100644 --- a/internal/engine/experiment/webconnectivity/httpanalysis_test.go +++ b/internal/engine/experiment/webconnectivity/httpanalysis_test.go @@ -70,6 +70,25 @@ func TestHTTPBodyLengthChecks(t *testing.T) { }, }, lengthMatch: nil, + }, { + name: "control length is negative", + args: args{ + tk: urlgetter.TestKeys{ + Requests: []archival.RequestEntry{{ + Response: archival.HTTPResponse{ + Body: archival.MaybeBinaryValue{ + Value: randx.Letters(768), + }, + }, + }}, + }, + ctrl: webconnectivity.ControlResponse{ + HTTPRequest: webconnectivity.ControlHTTPRequestResult{ + BodyLength: -1, + }, + }, + }, + lengthMatch: nil, }, { name: "match with bigger control", args: args{ @@ -249,6 +268,22 @@ func TestStatusCodeMatch(t *testing.T) { }}, }, }, + }, { + name: "with response status code and -1 as control status code", + args: args{ + tk: urlgetter.TestKeys{ + Requests: []archival.RequestEntry{{ + Response: archival.HTTPResponse{ + Code: 200, + }, + }}, + }, + ctrl: webconnectivity.ControlResponse{ + HTTPRequest: webconnectivity.ControlHTTPRequestResult{ + StatusCode: -1, + }, + }, + }, }, { name: "with only control status code and no response status code", args: args{ @@ -340,6 +375,26 @@ func TestHeadersMatch(t *testing.T) { ctrl: webconnectivity.ControlResponse{}, }, want: nil, + }, { + name: "with negative control status code", + args: args{ + tk: urlgetter.TestKeys{ + Requests: []archival.RequestEntry{{ + Response: archival.HTTPResponse{ + Headers: map[string]archival.MaybeBinaryValue{ + "Date": {Value: "Mon Jul 13 21:10:08 CEST 2020"}, + }, + Code: 200, + }, + }}, + }, + ctrl: webconnectivity.ControlResponse{ + HTTPRequest: webconnectivity.ControlHTTPRequestResult{ + StatusCode: -1, + }, + }, + }, + want: nil, }, { name: "with no uncommon headers", args: args{ @@ -748,6 +803,25 @@ func TestTitleMatch(t *testing.T) { }, }, wantOut: &trueValue, + }, { + name: "when the control status code is negative", + args: args{ + tk: urlgetter.TestKeys{ + Requests: []archival.RequestEntry{{ + Response: archival.HTTPResponse{ + Code: 200, + Body: archival.MaybeBinaryValue{ + Value: "La commUNity di MSN"}, + }, + }}, + }, + ctrl: webconnectivity.ControlResponse{ + HTTPRequest: webconnectivity.ControlHTTPRequestResult{ + StatusCode: -1, + }, + }, + }, + wantOut: nil, }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/engine/experiment/webconnectivity/summary.go b/internal/engine/experiment/webconnectivity/summary.go index 5635275..4ed2a8b 100644 --- a/internal/engine/experiment/webconnectivity/summary.go +++ b/internal/engine/experiment/webconnectivity/summary.go @@ -82,7 +82,7 @@ type Summary struct { // // Because of that, we must preserve the original behaviour. func DetermineBlocking(s Summary) interface{} { - if s.Accessible != nil && *s.Accessible == true { + if s.Accessible != nil && *s.Accessible { return false } return s.BlockingReason