From b58a10b3e3de3c53ba540bd7403456e72b6bfbad Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 17 May 2024 22:22:02 -0400 Subject: [PATCH 1/5] header_ok: Add unit tests --- Makefile.am | 1 + t/header_ok.pl | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 t/header_ok.pl diff --git a/Makefile.am b/Makefile.am index 738f301..54c1831 100644 --- a/Makefile.am +++ b/Makefile.am @@ -68,6 +68,7 @@ handwritten_tests = \ t/geturl_connectivity.pl \ t/geturl_response.pl \ t/group_hosts_by.pl \ + t/header_ok.pl \ t/interval_expired.pl \ t/is-and-extract-ipv4.pl \ t/is-and-extract-ipv6.pl \ diff --git a/t/header_ok.pl b/t/header_ok.pl new file mode 100644 index 0000000..e835757 --- /dev/null +++ b/t/header_ok.pl @@ -0,0 +1,67 @@ +use Test::More; +SKIP: { eval { require Test::Warnings; } or skip($@, 1); } +eval { require 'ddclient'; } or BAIL_OUT($@); +my $have_mock = eval { require Test::MockModule; }; + +my $failmsg; +my $module; +if ($have_mock) { + $module = Test::MockModule->new('ddclient'); + # Note: 'mock' is used instead of 'redefine' because 'redefine' is not available in the versions + # of Test::MockModule distributed with old Debian and Ubuntu releases. + $module->mock('failed', sub { $failmsg //= ''; $failmsg .= sprintf(shift, @_) . "\n"; }); +} + +my @test_cases = ( + { + desc => 'malformed not OK', + input => 'malformed', + want => 0, + wantmsg => qr/unexpected/, + }, + { + desc => 'HTTP/1.1 200 OK', + input => 'HTTP/1.1 200 OK', + want => 1, + }, + { + desc => 'HTTP/2 200 OK', + input => 'HTTP/2 200 OK', + want => 1, + }, + { + desc => 'HTTP/3 200 OK', + input => 'HTTP/3 200 OK', + want => 1, + }, + { + desc => '401 not OK', + input => 'HTTP/1.1 401 bad', + want => 0, + wantmsg => qr/authentication failed/, + }, + { + desc => '403 not OK', + input => 'HTTP/1.1 403 bad', + want => 0, + wantmsg => qr/not authorized/, + }, + { + desc => 'other 4xx not OK', + input => 'HTTP/1.1 456 bad', + want => 0, + }, +); + +for my $tc (@test_cases) { + subtest $tc->{desc} => sub { + $failmsg = ''; + is(ddclient::header_ok('host', $tc->{input}), $tc->{want}, 'return value matches'); + SKIP: { + skip('Test::MockModule not available') if !$have_mock; + like($failmsg, $tc->{wantmsg} // qr/^$/, 'fail message matches'); + } + }; +} + +done_testing(); From adbac91be721055b506a60a69a8b74d7673e0549 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 17 May 2024 21:27:59 -0400 Subject: [PATCH 2/5] header_ok: Only keep first line of argument This allows callers to pass the entire response without generating overly long error messages. --- ddclient.in | 1 + t/header_ok.pl | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/ddclient.in b/ddclient.in index 9af3867..a5b2800 100755 --- a/ddclient.in +++ b/ddclient.in @@ -3774,6 +3774,7 @@ sub nic_updateable { ###################################################################### sub header_ok { my ($host, $line) = @_; + $line =~ s/\r?\n.*//s; my $ok = 0; if ($line =~ m%^s*HTTP/.*\s+(\d+)%i) { diff --git a/t/header_ok.pl b/t/header_ok.pl index e835757..5797341 100644 --- a/t/header_ok.pl +++ b/t/header_ok.pl @@ -51,6 +51,12 @@ my @test_cases = ( input => 'HTTP/1.1 456 bad', want => 0, }, + { + desc => 'only first line is logged on error', + input => "HTTP/1.1 404 not found\n\nbody", + want => 0, + wantmsg => qr/(?!body)/, + }, ); for my $tc (@test_cases) { From 7fe7fd0e188aa7071f78dacf96ddbceb8aa9e805 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 17 May 2024 22:24:12 -0400 Subject: [PATCH 3/5] header_ok: Refactor for readability --- ddclient.in | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/ddclient.in b/ddclient.in index a5b2800..cc21be6 100755 --- a/ddclient.in +++ b/ddclient.in @@ -3775,24 +3775,21 @@ sub nic_updateable { sub header_ok { my ($host, $line) = @_; $line =~ s/\r?\n.*//s; - my $ok = 0; - - if ($line =~ m%^s*HTTP/.*\s+(\d+)%i) { - my $result = $1; - - if ($result =~ m/^2\d\d$/) { - $ok = 1; - - } elsif ($result eq '401') { - failed("updating %s: authentication failed (%s)", $host, $line); - } elsif ($result eq '403') { - failed("updating %s: not authorized (%s)", $host, $line); - } - - } else { - failed("updating %s: unexpected line (%s)", $host, $line); + # TODO: What is this leading /s*/? Is it a typo of /\s*/? + my ($result) = ($line =~ qr%^s*HTTP/.*\s+(\d+)%i); + if (!defined($result)) { + failed('updating %s: unexpected HTTP response: %s', $host, $line); + return 0; + } elsif ($result eq '401') { + failed("updating %s: authentication failed (%s)", $host, $line); + return 0; + } elsif ($result eq '403') { + failed("updating %s: not authorized (%s)", $host, $line); + return 0; + } elsif ($result !~ qr/^2\d\d$/) { + return 0; } - return $ok; + return 1; } ###################################################################### From 211d59fccca01a7e63b4b52d9ef22d8d293a2272 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Fri, 17 May 2024 23:21:25 -0400 Subject: [PATCH 4/5] header_ok: Log all non-2xx HTTP status codes --- ddclient.in | 17 ++++++++--------- t/header_ok.pl | 9 +++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/ddclient.in b/ddclient.in index cc21be6..84d8e3d 100755 --- a/ddclient.in +++ b/ddclient.in @@ -3776,17 +3776,16 @@ sub header_ok { my ($host, $line) = @_; $line =~ s/\r?\n.*//s; # TODO: What is this leading /s*/? Is it a typo of /\s*/? - my ($result) = ($line =~ qr%^s*HTTP/.*\s+(\d+)%i); - if (!defined($result)) { + my ($code, $msg) = ($line =~ qr%^s*HTTP/.*\s+(\d+)\s*(?:\s+([^\s].*))?$%i); + if (!defined($code)) { failed('updating %s: unexpected HTTP response: %s', $host, $line); return 0; - } elsif ($result eq '401') { - failed("updating %s: authentication failed (%s)", $host, $line); - return 0; - } elsif ($result eq '403') { - failed("updating %s: not authorized (%s)", $host, $line); - return 0; - } elsif ($result !~ qr/^2\d\d$/) { + } elsif ($code !~ qr/^2\d\d$/) { + my %msgs = ( + '401' => 'authentication failed', + '403' => 'not authorized', + ); + failed('updating %s: %s %s', $host, $code, $msg // $msgs{$code} // ''); return 0; } return 1; diff --git a/t/header_ok.pl b/t/header_ok.pl index 5797341..197cc8d 100644 --- a/t/header_ok.pl +++ b/t/header_ok.pl @@ -35,14 +35,14 @@ my @test_cases = ( want => 1, }, { - desc => '401 not OK', - input => 'HTTP/1.1 401 bad', + desc => '401 not OK, fallback message', + input => 'HTTP/1.1 401 ', want => 0, wantmsg => qr/authentication failed/, }, { - desc => '403 not OK', - input => 'HTTP/1.1 403 bad', + desc => '403 not OK, fallback message', + input => 'HTTP/1.1 403 ', want => 0, wantmsg => qr/not authorized/, }, @@ -50,6 +50,7 @@ my @test_cases = ( desc => 'other 4xx not OK', input => 'HTTP/1.1 456 bad', want => 0, + wantmsg => qr/bad/, }, { desc => 'only first line is logged on error', From 2d4a93d5e79964beaf0ff5a3793c0ce6acb089a3 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Tue, 14 May 2024 23:51:37 -0400 Subject: [PATCH 5/5] header_ok: Fix typo(?) in HTTP response regular expression --- ddclient.in | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ddclient.in b/ddclient.in index 84d8e3d..eaf40e8 100755 --- a/ddclient.in +++ b/ddclient.in @@ -3775,8 +3775,7 @@ sub nic_updateable { sub header_ok { my ($host, $line) = @_; $line =~ s/\r?\n.*//s; - # TODO: What is this leading /s*/? Is it a typo of /\s*/? - my ($code, $msg) = ($line =~ qr%^s*HTTP/.*\s+(\d+)\s*(?:\s+([^\s].*))?$%i); + my ($code, $msg) = ($line =~ qr%^\s*HTTP/.*\s+(\d+)\s*(?:\s+([^\s].*))?$%i); if (!defined($code)) { failed('updating %s: unexpected HTTP response: %s', $host, $line); return 0;