From 7b32b24df9a88fb45b85a33c2475c97e10d25810 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 12 Oct 2018 13:57:49 -0300 Subject: [PATCH] Bug 21576: Add a developer script to fix missing TT filters See bug 13618 and bug 21526. We need a script to add missing filters, or replace wrong ones. Test plan: - Add unescaped variables to .tt files - prove xt/find-missing-filters.t will warn about them - perl misc/devel/add_missing_filters.pl will add the missing/wrong filters - prove xt/find-missing-filters.t will now be happy Signed-off-by: Owen Leonard Signed-off-by: Martin Renvoize --- misc/devel/add_missing_filters.pl | 93 +++++++++ t/lib/QA/TemplateFilters.pm | 186 +++++++++++++++--- t/template_filters.t | 301 ++++++++++++++++++++---------- xt/find-missing-filters.t | 4 +- 4 files changed, 462 insertions(+), 122 deletions(-) create mode 100755 misc/devel/add_missing_filters.pl diff --git a/misc/devel/add_missing_filters.pl b/misc/devel/add_missing_filters.pl new file mode 100755 index 0000000000..cc1b877e1e --- /dev/null +++ b/misc/devel/add_missing_filters.pl @@ -0,0 +1,93 @@ +#!/usr/bin/perl + +use Modern::Perl; + +use File::Slurp; +use Pod::Usage; +use Getopt::Long; + +use t::lib::QA::TemplateFilters; + +my ( $help, $verbose, @files ); +GetOptions( + 'h|help' => \$help, + 'v|verbose' => \$verbose, +) || pod2usage(1); + +@files = @ARGV; + +pod2usage(1) if $help or not @files; + +my $i; +my $total = scalar @files; +my $num_width = length $total; +for my $file ( @ARGV ) { + if ( $verbose ) { + print sprintf "|%-25s| %${num_width}s / %s (%.2f%%)\r", + '=' x (24*$i++/$total). '>', + $i, $total, 100*$i/+$total; + flush STDOUT; + } + + my $content = read_file( $file ); + my $new_content = t::lib::QA::TemplateFilters::fix_filters($content); + $new_content .= "\n"; + if ( $content ne $new_content ) { + say "$file -- Modified"; + write_file($file, $new_content); + } +} + + +=head1 NAME + +add_missing_filters.pl - Will add the missing filters to the template files given in parameters. + +=head1 SYNOPSIS + +perl misc/devel/add_missing_filters.pl **/*.tt + +/!\ It is highly recommended to execute this script on a clean git install, with all your files and changes committed. + + Options: + -?|--help brief help message + -v|--verbose verbose mode + +=head1 OPTIONS + +=over 8 + +=item B<--help|-?> + +Print a brief help message and exits + +=item B<-v|--verbose> + +Verbose mode. + +=back + +=head1 AUTHOR + +Jonathan Druart + +=head1 COPYRIGHT + +Copyright 2018 Koha Development Team + +=head1 LICENSE + +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 . diff --git a/t/lib/QA/TemplateFilters.pm b/t/lib/QA/TemplateFilters.pm index 2aa94b7dfb..400de8bf70 100644 --- a/t/lib/QA/TemplateFilters.pm +++ b/t/lib/QA/TemplateFilters.pm @@ -26,24 +26,43 @@ our @tt_directives = ( qr{^\s*NEXT}, ); -sub missing_filters { +sub fix_filters { + return _process_tt_content( @_ )->{new_content}; +} + +sub search_missing_filters { + return _process_tt_content( @_ )->{errors}; + +} + +sub _process_tt_content { my ($content) = @_; my ( $use_raw, $has_use_raw ); my @errors; + my @new_lines; my $line_number; for my $line ( split "\n", $content ) { + my $new_line = $line; $line_number++; if ( $line =~ m{\[%[^%]+%\]} ) { # handle exceptions first - $use_raw = 1 - if $line =~ m{|\s*\$raw}; # Is the file use the raw filter? + if ( $line =~ m{\|\s*\$raw} ) { # Is the file use the raw filter? + $use_raw = 1; + } # Do we have Asset without the raw filter? - if ( $line =~ m{^\s*\[% Asset} ) { - push @errors, { error => 'asset_must_be_raw', line => $line, line_number => $line_number } - and next - unless $line =~ m{\|\s*\$raw}; + if ( $line =~ m{^\s*\[% Asset} && $line !~ m{\|\s*\$raw} ) { + push @errors, + { + error => 'asset_must_be_raw', + line => $line, + line_number => $line_number + }; + $new_line =~ s/\)\s*%]/) | \$raw %]/; + $use_raw = 1; + push @new_lines, $new_line; + next; } $has_use_raw++ @@ -59,29 +78,132 @@ sub missing_filters { %\]}gmxs ) { - my $tt_block = $+{tt_block}; + my $tt_block = $+{tt_block}; + my $pre_chomp = $+{pre_chomp}; + my $post_chomp = $+{post_chomp}; + + next if + # It's a TT directive, no filters needed + grep { $tt_block =~ $_ } @tt_directives - # It's a TT directive, no filters needed - next if grep { $tt_block =~ $_ } @tt_directives; + # It is a comment + or $tt_block =~ m{^\#} - next - if $tt_block =~ m{\s?\|\s?\$KohaDates\s?$} + # Already escaped with a special filter + # We could escape it but should be safe + or $tt_block =~ m{\s?\|\s?\$KohaDates\s?$} or $tt_block =~ m{\s?\|\s?\$Price\s?$} - ; # We could escape it but should be safe - next if $tt_block =~ m{^\#}; # Is a comment, skip it - - push @errors, { error => 'missing_filter', line => $line, line_number => $line_number } - if $tt_block !~ m{\|\s?\$raw} # already escaped correctly with raw - && $tt_block !~ m{=} # assignment, maybe we should require to use SET (?) - && $tt_block !~ m{\|\s?ur(l|i)} # already has url or uri filter - && $tt_block !~ m{\|\s?html} # already has html filter - && $tt_block !~ m{^(?\S+)\s+UNLESS\s+(?\S+)} # Specific for [% foo UNLESS bar %] + + # Already escaped correctly with raw + or $tt_block =~ m{\|\s?\$raw} + + # Assignment, maybe we should require to use SET (?) + or $tt_block =~ m{=} + + # Already has url or uri filter + or $tt_block =~ m{\|\s?ur(l|i)} + + # Specific for [% foo UNLESS bar %] + or $tt_block =~ m{^(?\S+)\s+UNLESS\s+(?\S+)} ; + + $pre_chomp = + $pre_chomp + ? $pre_chomp =~ m|-| + ? q|- | + : $pre_chomp =~ m|~| + ? q|~ | + : q| | + : q| |; + $post_chomp = + $post_chomp + ? $post_chomp =~ m|-| + ? q| -| + : $post_chomp =~ m|~| + ? q| ~| + : q| | + : q| |; + + if ( + + # Use the uri filter + # If html filtered or not filtered + $new_line =~ qr{ + \@errors, new_content => $new_content }; } 1; @@ -93,7 +215,8 @@ t::lib::QA::TemplateFilters - Module used by tests and QA script to catch missin =head1 SYNOPSIS my $content = read_file($filename); - my @e = t::lib::QA::TemplateFilters::missing_filters($content); + my $new_content = t::lib::QA::TemplateFilters::fix_filters($content); + my $errors = t::lib::QA::TemplateFilters::search_missing_filters($content); =head1 DESCRIPTION @@ -102,15 +225,26 @@ and to not duplicate the code. =head1 METHODS -=head2 missing_filters +=head2 fix_filters - Take a template content file in parameter and return an array of errors. - An error is a hashref with 2 keys, error and line. + Take a template content file in parameter and return the same content with + the correct (guessed) filters. + It will also add the [% USE raw %] statement if it is needed. + +=head2 search_missing_filters + + Take a template content file in parameter and return an arrayref of errors. + + An error is a hashref with 3 keys, error and line, line_number. * error can be: asset_must_be_raw - When Asset is called without using raw missing_filter - When a TT variable is displayed without filter + wrong_html_filter - When a TT variable is using the html filter when uri (or url) + should be used instead. * line is the line where the error has been found. + * line_number is the line number where the error has been found. + =head1 AUTHORS diff --git a/t/template_filters.t b/t/template_filters.t index 7ea0f54f1d..3c9c5a97a3 100644 --- a/t/template_filters.t +++ b/t/template_filters.t @@ -16,28 +16,130 @@ # along with Koha; if not, see . use Modern::Perl; -use Test::More tests => 1; +use Test::More tests => 5; use t::lib::QA::TemplateFilters; -my $input = < sub { + plan tests => 2; + my $input = < "asset_must_be_raw", + line => '[% Asset.css("css/one.css") %]', + line_number => 1, + }, + { + error => "asset_must_be_raw", + line => '[% Asset.css("js/two.js") %]', + line_number => 2, + } + + ], + ); +}; + +subtest 'Variables must be html escaped' => sub { + plan tests => 2; + + my $input = <Koha › Patrons › [% UNLESS blocking_error %] - Patron details for [% INCLUDE 'patron-title.inc' no_html = 1 %] + [% just_a_var %] [% just_a_var %] A N D [% another_one_on_same_line %] - [% just_a_var_filtered|html %] - [% just_a_var_filtered |html %] - [% just_a_var_filtered| html %] - [% just_a_var_filtered | html %] [% END %] - [% IF ( patron.othernames | html ) %]“[% patron.othernames %]”[% END %] - [% Asset.css("css/datatables.css").raw %] - [% Asset.css("css/datatables.css") | \$raw %] + [% IF ( patron.othernames ) %]“[% patron.othernames %]”[% END %] -[% patron.phone %] -[% patron.emailpro %] [% patron_message.get_column('manager_surname') %] +INPUT + + my $expected = <Koha › Patrons › + [% UNLESS blocking_error %] + [% just_a_var | html %] + [% just_a_var | html %] A N D [% another_one_on_same_line | html %] + [% END %] + [% IF ( patron.othernames ) %]“[% patron.othernames | html %]”[% END %] + +[% patron_message.get_column('manager_surname') | html %] +EXPECTED + + my $new_content = t::lib::QA::TemplateFilters::fix_filters($input); + is( $new_content . "\n", $expected, ); + my $missing_filters = t::lib::QA::TemplateFilters::search_missing_filters($input); + is_deeply( + $missing_filters, + [{ + error => "missing_filter", + line => " [% just_a_var %]", + line_number => 3, + }, + { + error => "missing_filter", + line => " [% just_a_var %] A N D [% another_one_on_same_line %]", + line_number => 4, + }, + { + error => "missing_filter", + line => " [% just_a_var %] A N D [% another_one_on_same_line %]", + line_number => 4, + }, + { + error => "missing_filter", + line => " [% IF ( patron.othernames ) %]“[% patron.othernames %]”[% END %]", + line_number => 6, + }, + { + error => "missing_filter", + line => "[% patron_message.get_column('manager_surname') %]", + line_number => 8 + } + ], + + ); +}; + +subtest 'TT directives, assignments and already filtered variables must not be escaped' => sub { + plan tests => 2; + my $input = < sub { + plan tests => 1; + my $input = < sub { + plan tests => 3; + my $input = <[% patron.phone %] +[% patron.emailpro %] +[% patron.emailpro %] +[% patron.emailpro %] +[% myuri %] +[% myuri %] +[% myurl %] +[% myurl %] INPUT -my @expected_errors = ( - { - error => q{missing_filter}, - line => -q{ [% just_a_var %] A N D [% another_one_on_same_line %]}, - line_number => 6, - }, - { - error => q{missing_filter}, - line => -q{ [% just_a_var %] A N D [% another_one_on_same_line %]}, - line_number => 6, - }, - { - error => q{missing_filter}, - line => -q{ [% IF ( patron.othernames | html ) %]“[% patron.othernames %]”[% END %]}, - line_number => 12, - }, - { - error => q{asset_must_be_raw}, - line => q{ [% Asset.css("css/datatables.css").raw %]}, - line_number => 13, - }, - { - error => q{missing_filter}, - line => q{[% patron.phone %]}, - line_number => 16, - }, - { - error => q{missing_filter}, - line => q{[% patron.phone %]}, - line_number => 16, - }, - { - error => q{missing_filter}, - line => -q{[% patron.emailpro %]}, - line_number => 17, - }, - { - error => q{missing_filter}, - line => -q{[% patron.emailpro %]}, - line_number => 17, - }, - { - error => q{missing_filter}, - line => q{[% patron_message.get_column('manager_surname') %]}, - line_number => 18, - }, - { - error => q{missing_filter}, - line => q{[%- var -%]}, - line_number => 29, - }, - { - error => q{missing_filter}, - line => q{[% - var - %]}, - line_number => 30, - }, - { - error => q{missing_filter}, - line => q{[%~ var ~%]}, - line_number => 31, - }, - { - error => q{missing_filter}, - line => q{[% ~ var ~ %]}, - line_number => 32, - } -); - -my @get = t::lib::QA::TemplateFilters::missing_filters($input); -is_deeply( \@get, \@expected_errors); + # Note: [% myurl %] will be uri escaped, we cannot know url should be used + my $expected = <[% patron.phone | html %] +[% patron.emailpro | html %] +[% patron.emailpro | html %] +[% patron.emailpro | html %] +[% myuri | html %] +[% myuri | html %] +[% myurl | html %] +[% myurl | html %] +EXPECTED + + my $new_content = t::lib::QA::TemplateFilters::fix_filters($input); + is( $new_content . "\n", $expected, ); + + $input = <[% var | html %] +INPUT + my $missing_filters = t::lib::QA::TemplateFilters::search_missing_filters($input); + is_deeply( + $missing_filters, + [ + { + error => "wrong_html_filter", + line => + '[% var | html %]', + line_number => 1 + } + + ], + ); + + $input = <[% var | html %] +INPUT + $missing_filters = t::lib::QA::TemplateFilters::search_missing_filters($input); + is_deeply( $missing_filters, [], ); +}; diff --git a/xt/find-missing-filters.t b/xt/find-missing-filters.t index 6b7e80f4aa..858c8362f2 100755 --- a/xt/find-missing-filters.t +++ b/xt/find-missing-filters.t @@ -52,8 +52,8 @@ find({ wanted => \&wanted, no_chdir => 1 }, @themes ); my @errors; for my $file ( @files ) { my $content = read_file($file); - my @e = t::lib::QA::TemplateFilters::missing_filters($content); - push @errors, { file => $file, errors => \@e } if @e; + my $e = t::lib::QA::TemplateFilters::search_missing_filters($content); + push @errors, { file => $file, errors => $e } if @$e; } is( @errors, 0, "Template variables should be correctly escaped" ) -- 2.19.1