From 32f9aede3d6a8ec20609140496930593532c3611 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Thu, 26 Jun 2025 15:36:08 +0100 Subject: [PATCH] Bug 40079: (follow-up) Improve test coverage and fix undef handling This patch modernizes the Scrubber.t unit test with comprehensive coverage following Koha best practices: - Uses subtest structure for logical grouping - Tests all scrubber types (default, comment, note) - Includes security testing for XSS prevention - Covers edge cases and error handling - Tests new list elements added in previous commits Also fixes C4::Scrubber to handle undef parameters cleanly by treating them as 'default' type instead of generating warnings. Test plan: 1. Run prove t/Scrubber.t 2. Verify all tests pass without warnings 3. Confirm comprehensive coverage of module functionality --- C4/Scrubber.pm | 1 + t/Scrubber.t | 218 ++++++++++++++++++++++++++++++++----------------- 2 files changed, 142 insertions(+), 77 deletions(-) diff --git a/C4/Scrubber.pm b/C4/Scrubber.pm index fe406ab8dca..40c0bee3dc1 100644 --- a/C4/Scrubber.pm +++ b/C4/Scrubber.pm @@ -35,6 +35,7 @@ my %scrubbertypes = ( sub new { shift; # ignore our class we are wrapper my $type = (@_) ? shift : 'default'; + $type = 'default' if !defined $type; if ( !exists $scrubbertypes{$type} ) { croak "New called with unrecognized type '$type'"; } diff --git a/t/Scrubber.t b/t/Scrubber.t index bfae79df3b8..cf2de7d0f17 100755 --- a/t/Scrubber.t +++ b/t/Scrubber.t @@ -1,94 +1,158 @@ #!/usr/bin/perl +# Copyright 2025 Koha Development team +# +# This file is part of Koha +# +# Koha is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# Koha is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with Koha; if not, see + use Modern::Perl; -$| = 1; +use Test::More tests => 7; use Test::NoWarnings; -use Test::More tests => 27; +use Test::Exception; use Test::Warn; BEGIN { - use FindBin; - use lib $FindBin::Bin; use_ok('C4::Scrubber'); } -sub pretty_line { - my $max = 54; - (@_) or return "#" x $max . "\n"; - my $phrase = " " . shift() . " "; - my $half = "#" x ( ( $max - length($phrase) ) / 2 ); - return $half . $phrase . $half . "\n"; -} +subtest 'new() constructor tests' => sub { + plan tests => 8; -my ( $scrubber, $html, $result, @types, $collapse ); -$collapse = 1; -@types = qw(default comment note); -$html = q| - - -
- - - I am ITALICS with fake="attribute"
- I am em with fake="attribute"
- I am BOLD
- I am a span w/ style. Bad style. - I am a span trying to inject a link: <a href="badlink.html"> link </a> -
- - I am a link firing javascript. -
- - ONMOUSEOVER JAVASCRIPT - -
-At the end here, I actually have some regular text. -|; - -ok( $scrubber = C4::Scrubber->new(), "Constructor: C4::Scrubber->new()" ); - -isa_ok( $scrubber, 'HTML::Scrubber', 'Constructor returns HTML::Scrubber object' ); - -warning_like { $scrubber->default() } '', "\$scrubber->default ran without fault."; -warning_like { $scrubber->comment() } '', "\$scrubber->comment ran without fault."; -warning_like { $scrubber->process() } '', "\$scrubber->process ran without fault."; - -ok( $result = $scrubber->scrub($html), "Getting scrubbed text (type: [default])" ); - -foreach (@types) { - ok( $scrubber = C4::Scrubber->new($_), "testing Constructor: C4::Scrubber->new($_)" ); - - warning_like { $scrubber->default() } '', "\$scrubber->default ran without fault."; - warning_like { $scrubber->comment() } '', "\$scrubber->comment ran without fault."; - warning_like { $scrubber->process() } '', "\$scrubber->process ran without fault."; - - ok( $result = $scrubber->scrub($html), "Getting scrubbed text (type: $_)" ); -} + my $scrubber; + lives_ok { $scrubber = C4::Scrubber->new() } 'Constructor with no parameters succeeds'; + isa_ok( $scrubber, 'HTML::Scrubber', 'Constructor returns HTML::Scrubber object' ); + + lives_ok { $scrubber = C4::Scrubber->new('default') } 'Constructor with default type succeeds'; + isa_ok( $scrubber, 'HTML::Scrubber', 'Constructor with default type returns HTML::Scrubber object' ); -#Test for invalid new entry -eval { - C4::Scrubber->new(""); - fail("test should fail on entry of ''"); + lives_ok { $scrubber = C4::Scrubber->new('comment') } 'Constructor with comment type succeeds'; + isa_ok( $scrubber, 'HTML::Scrubber', 'Constructor with comment type returns HTML::Scrubber object' ); + + lives_ok { $scrubber = C4::Scrubber->new('note') } 'Constructor with note type succeeds'; + isa_ok( $scrubber, 'HTML::Scrubber', 'Constructor with note type returns HTML::Scrubber object' ); }; -if ($@) { - pass("Test should have failed on entry of '' (empty string) and it did. YAY!"); -} -eval { - C4::Scrubber->new("Client"); - fail("test should fail on entry of 'Client'"); +subtest 'constructor error handling' => sub { + plan tests => 5; + + my $scrubber; + lives_ok { $scrubber = C4::Scrubber->new(undef) } 'Constructor with undef type succeeds (treated as default)'; + isa_ok( $scrubber, 'HTML::Scrubber', 'Constructor with undef type returns HTML::Scrubber object' ); + + throws_ok { + C4::Scrubber->new(''); + } + qr/New called with unrecognized type/, 'Constructor throws exception for empty string type'; + + throws_ok { + C4::Scrubber->new('invalid_type'); + } + qr/New called with unrecognized type/, 'Constructor throws exception for invalid type'; + + throws_ok { + C4::Scrubber->new('Client'); + } + qr/New called with unrecognized type/, 'Constructor throws exception for Client type'; +}; + +subtest 'default scrubber functionality' => sub { + plan tests => 5; + + my $scrubber = C4::Scrubber->new('default'); + + my $malicious_html = q| + + + + + + + Click me +

Paragraph

+
Content
+ Plain text content + |; + + my $result = $scrubber->scrub($malicious_html); + + unlike( $result, qr/'; + + my $result = $scrubber->scrub($test_html); + + like( $result, qr/Bold<\/b>/, 'Bold tags are preserved' ); + like( $result, qr/Italic<\/i>/, 'Italic tags are preserved' ); + like( $result, qr/Emphasis<\/em>/, 'Em tags are preserved' ); + like( $result, qr/Big<\/big>/, 'Big tags are preserved' ); + like( $result, qr/Small<\/small>/, 'Small tags are preserved' ); + like( $result, qr/Strong<\/strong>/, 'Strong tags are preserved' ); + like( $result, qr/
/, 'Break tags are preserved' ); + + unlike( $result, qr/

/, 'Paragraph tags are removed' ); + unlike( $result, qr//, 'Span tags are removed' ); + unlike( $result, qr/'; + my $safe_result = $scrubber->scrub($malicious_note); + + like( $safe_result, qr/

Safe content<\/p>/, 'Safe content is preserved' ); + unlike( $safe_result, qr/