Bugzilla – Attachment 183537 Details for
Bug 40079
C4::Scrubber "note" profile should allow for list (ul, ol, li, dl, dt, and dd) HTML tags
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 40079: (follow-up) Improve test coverage and fix undef handling
Bug-40079-follow-up-Improve-test-coverage-and-fix-.patch (text/plain), 12.15 KB, created by
Martin Renvoize (ashimema)
on 2025-06-26 14:37:56 UTC
(
hide
)
Description:
Bug 40079: (follow-up) Improve test coverage and fix undef handling
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2025-06-26 14:37:56 UTC
Size:
12.15 KB
patch
obsolete
>From 32f9aede3d6a8ec20609140496930593532c3611 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 <http://www.gnu.org/licenses> >+ > 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| >-<![CDATA[selfdestruct]]]> >-<?php echo(" EVIL EVIL EVIL "); ?> <!-- COMMENT --> >-<hr> <!-- TMPL_VAR NAME="password" --> >-<style type="text/css">body{display:none;}</style> >-<link media="screen" type="text/css" rev="stylesheet" rel="stylesheet" href="css.css"> >-<I FAKE="attribute" > I am ITALICS with fake="attribute" </I><br /> >-<em FAKE="attribute" > I am em with fake="attribute" </em><br /> >-<B> I am BOLD </B><br /> >-<span style="background-image: url(http://hackersite.cn/porno.jpg);"> I am a span w/ style. Bad style.</span> >-<span> I am a span trying to inject a link: <a href="badlink.html"> link </a></span> >-<br> >-<A NAME="evil"> >- <A HREF="javascript:alert('OMG YOO R HACKED');">I am a link firing javascript.</A> >- <br /> >- <A HREF="image/bigone.jpg" ONMOUSEOVER="alert('OMG YOO R HACKED');"> >- <IMG SRC="image/smallone.jpg" ALT="ONMOUSEOVER JAVASCRIPT"> >- </A> >-</A> <br> >-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| >+ <![CDATA[selfdestruct]]]> >+ <?php echo("EVIL EVIL EVIL"); ?> >+ <script>alert('XSS Attack!');</script> >+ <style type="text/css">body{display:none;}</style> >+ <link href="evil.css" rel="stylesheet"> >+ <img src="x" onerror="alert('XSS')"> >+ <a href="javascript:alert('XSS')">Click me</a> >+ <p onclick="alert('XSS')">Paragraph</p> >+ <div style="background:url(javascript:alert('XSS'))">Content</div> >+ Plain text content >+ |; >+ >+ my $result = $scrubber->scrub($malicious_html); >+ >+ unlike( $result, qr/<script/i, 'Script tags are removed' ); >+ unlike( $result, qr/<style/i, 'Style tags are removed' ); >+ unlike( $result, qr/<link/i, 'Link tags are removed' ); >+ unlike( $result, qr/javascript:/i, 'JavaScript URLs are removed' ); >+ like( $result, qr/Plain text content/, 'Plain text content is preserved' ); >+}; >+ >+subtest 'comment scrubber functionality' => sub { >+ plan tests => 10; >+ >+ my $scrubber = C4::Scrubber->new('comment'); >+ >+ my $test_html = >+ '<p>Paragraph</p><b>Bold</b><i>Italic</i><em>Emphasis</em><big>Big</big><small>Small</small><strong>Strong</strong><br><u>Underline</u><hr><span>Span</span><div>Div</div><script>Evil</script>'; >+ >+ my $result = $scrubber->scrub($test_html); >+ >+ like( $result, qr/<b>Bold<\/b>/, 'Bold tags are preserved' ); >+ like( $result, qr/<i>Italic<\/i>/, 'Italic tags are preserved' ); >+ like( $result, qr/<em>Emphasis<\/em>/, 'Em tags are preserved' ); >+ like( $result, qr/<big>Big<\/big>/, 'Big tags are preserved' ); >+ like( $result, qr/<small>Small<\/small>/, 'Small tags are preserved' ); >+ like( $result, qr/<strong>Strong<\/strong>/, 'Strong tags are preserved' ); >+ like( $result, qr/<br>/, 'Break tags are preserved' ); >+ >+ unlike( $result, qr/<p>/, 'Paragraph tags are removed' ); >+ unlike( $result, qr/<span>/, 'Span tags are removed' ); >+ unlike( $result, qr/<script>/, 'Script tags are removed' ); > }; >-if ($@) { >- pass("Test should have failed on entry of 'Client' and it did. YAY!"); >-} > >-my $scrub_text = >- '<div><span><p><b>bold</b><i>ital</i><em>emphatic</em><big>embiggen</big><small>shrink</small><strong>strongbad</strong><br><u>under</u><hr><ol><li>ordered item</li></ol><ul><li>unordered item</li></ul><dl><dt>term</dt><dd>definition</dd></dl></p></span></div>'; >-my $scrub_comment = >- '<b>bold</b><i>ital</i><em>emphatic</em><big>embiggen</big><small>shrink</small><strong>strongbad</strong><br>underordered itemunordered itemtermdefinition'; >-is( C4::Scrubber->new('comment')->scrub($scrub_text), $scrub_comment, "Comment scrubber removes expected elements" ); >-is( >- C4::Scrubber->new('note')->scrub($scrub_text), $scrub_text, >- "Note scrubber removes (additional) expected elements" >-); >+subtest 'note scrubber functionality' => sub { >+ plan tests => 22; >+ >+ my $scrubber = C4::Scrubber->new('note'); >+ >+ my $comprehensive_html = >+ '<div><span><p><b>Bold</b><i>Italic</i><em>Emphasis</em><big>Big</big><small>Small</small><strong>Strong</strong><br><u>Underline</u><hr><ol><li>Ordered item 1</li><li>Ordered item 2</li></ol><ul><li>Unordered item 1</li><li>Unordered item 2</li></ul><dl><dt>Term</dt><dd>Definition</dd></dl></p></span></div>'; >+ >+ my $result = $scrubber->scrub($comprehensive_html); >+ >+ like( $result, qr/<div>/, 'Div tags are preserved' ); >+ like( $result, qr/<span>/, 'Span tags are preserved' ); >+ like( $result, qr/<p>/, 'Paragraph tags are preserved' ); >+ like( $result, qr/<b>Bold<\/b>/, 'Bold tags are preserved' ); >+ like( $result, qr/<i>Italic<\/i>/, 'Italic tags are preserved' ); >+ like( $result, qr/<em>Emphasis<\/em>/, 'Em tags are preserved' ); >+ like( $result, qr/<big>Big<\/big>/, 'Big tags are preserved' ); >+ like( $result, qr/<small>Small<\/small>/, 'Small tags are preserved' ); >+ like( $result, qr/<strong>Strong<\/strong>/, 'Strong tags are preserved' ); >+ like( $result, qr/<br>/, 'Break tags are preserved' ); >+ like( $result, qr/<u>Underline<\/u>/, 'Underline tags are preserved' ); >+ like( $result, qr/<hr>/, 'HR tags are preserved' ); >+ like( $result, qr/<ol>/, 'Ordered list tags are preserved' ); >+ like( $result, qr/<ul>/, 'Unordered list tags are preserved' ); >+ like( $result, qr/<li>/, 'List item tags are preserved' ); >+ like( $result, qr/<dl>/, 'Description list tags are preserved' ); >+ like( $result, qr/<dt>Term<\/dt>/, 'Description term tags are preserved' ); >+ like( $result, qr/<dd>Definition<\/dd>/, 'Description definition tags are preserved' ); >+ >+ is( $result, $comprehensive_html, 'All allowed tags in note scrubber are preserved exactly' ); >+ >+ my $malicious_note = '<p>Safe content</p><script>alert("XSS")</script><iframe src="evil.html"></iframe>'; >+ my $safe_result = $scrubber->scrub($malicious_note); >+ >+ like( $safe_result, qr/<p>Safe content<\/p>/, 'Safe content is preserved' ); >+ unlike( $safe_result, qr/<script>/, 'Script tags are removed from notes' ); >+ unlike( $safe_result, qr/<iframe>/, 'Iframe tags are removed from notes' ); >+}; >-- >2.49.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 40079
:
182985
|
183014
|
183534
|
183535
|
183536
|
183537
|
183583
|
183584
|
183585
|
183586