From 423d30757f6ab82e3569185b8d495783250e8a36 Mon Sep 17 00:00:00 2001 From: Vitaliy Filippov Date: Wed, 22 Oct 2014 14:55:59 +0400 Subject: [PATCH] Fix html filter errors, adjust 008filter.t --- enter_bug.cgi | 2 +- t/008filter.t | 95 ++++++++-------- .../admin/custom_fields/list.html.tmpl | 4 +- .../en/default/admin/editemailin.html.tmpl | 6 +- .../admin/fieldvalues/control-list.html.tmpl | 2 +- .../default/admin/fieldvalues/list.html.tmpl | 2 +- .../default/admin/params/editparams.html.tmpl | 2 +- template/en/default/bug/checkaccess.html.tmpl | 2 +- .../en/default/bug/create/create.html.tmpl | 2 +- template/en/default/bug/edit.html.tmpl | 2 +- template/en/default/bug/knob.html.tmpl | 8 +- .../bug/process/verify-flags.html.tmpl | 4 +- template/en/default/filterexceptions.pl | 103 +----------------- .../global/choose-classification.html.tmpl | 4 +- template/en/default/global/messages.html.tmpl | 2 +- .../default/global/user-error-page.html.tmpl | 2 +- .../en/default/global/user-error.html.tmpl | 4 +- template/en/default/list/comments.rss.tmpl | 8 +- template/en/default/list/list.html.tmpl | 2 +- .../default/pages/attach-multiple.html.tmpl | 10 +- template/en/default/search/form.html.tmpl | 2 +- .../en/default/worktime/supertime.html.tmpl | 2 +- .../en/default/worktime/todaybugs.html.tmpl | 2 +- 23 files changed, 91 insertions(+), 181 deletions(-) diff --git a/enter_bug.cgi b/enter_bug.cgi index 315f9d0c8..9cc3df683 100755 --- a/enter_bug.cgi +++ b/enter_bug.cgi @@ -276,7 +276,7 @@ if ($cloned_bug_id) # the first comment, if it has one. Either way, make a note # that this bug was cloned from another bug. - my $cloned_comment = $ARGS->{cloned_comment} || 0; + my $cloned_comment = int($ARGS->{cloned_comment}) || 0; my $bug_desc = $cloned_bug->comments({ order => 'oldest_to_newest' }); my ($comment_obj) = grep { $_->{count} == $cloned_comment } @$bug_desc; $comment_obj ||= $bug_desc->[0]; diff --git a/t/008filter.t b/t/008filter.t index e6ae4c13a..c1a869d46 100644 --- a/t/008filter.t +++ b/t/008filter.t @@ -25,7 +25,7 @@ # This test scans all our templates for every directive. Having eliminated # those which cannot possibly cause XSS problems, it then checks the rest -# against the safe list stored in the filterexceptions.pl file. +# against the safe list stored in the filterexceptions.pl file. # Sample exploit code: '>"> @@ -54,12 +54,12 @@ foreach my $path (@Support::Templates::include_paths) { chdir $topdir; # absolute path my @testitems = Support::Templates::find_actual_files($path); chdir $topdir; # absolute path - + next unless @testitems; - + # Some people require this, others don't. No-one knows why. chdir $path; # relative path - + # We load a %safe list of acceptable exceptions. if (!-r "filterexceptions.pl") { ok(0, "$path has templates but no filterexceptions.pl file. --ERROR"); @@ -68,7 +68,7 @@ foreach my $path (@Support::Templates::include_paths) { else { do "filterexceptions.pl"; if (ON_WINDOWS) { - # filterexceptions.pl uses / separated paths, while + # filterexceptions.pl uses / separated paths, while # find_actual_files returns \ separated ones on Windows. # Here, we convert the filter exception hash to use \. foreach my $file (keys %safe) { @@ -81,15 +81,15 @@ foreach my $path (@Support::Templates::include_paths) { } } } - + # We preprocess the %safe hash of lists into a hash of hashes. This allows - # us to flag which members were not found, and report that as a warning, + # us to flag which members were not found, and report that as a warning, # thereby keeping the lists clean. foreach my $file (keys %safe) { my $list = $safe{$file}; $safe{$file} = {}; foreach my $directive (@$list) { - $safe{$file}{$directive} = 0; + $safe{$file}{$directive} = 0; } } @@ -100,9 +100,9 @@ foreach my $path (@Support::Templates::include_paths) { ok(1, "($lang/$flavor) $file is filter-safe"); next; } - + # Read the entire file into a string - open (FILE, "<$file") || die "Can't open $file: $!\n"; + open (FILE, "<$file") || die "Can't open $file: $!\n"; my $slurp = ; close (FILE); @@ -119,29 +119,30 @@ foreach my $path (@Support::Templates::include_paths) { if (!directive_ok($file, $directive)) { # This intentionally makes no effort to eliminate duplicates; to do - # so would merely make it more likely that the user would not + # so would merely make it more likely that the user would not # escape all instances when attempting to correct an error. push(@unfiltered, "$lineno:$directive"); } - } + } my $fullpath = File::Spec->catfile($path, $file); - + if (@unfiltered) { my $uflist = join("\n ", @unfiltered); - ok(0, "($lang/$flavor) $fullpath has unfiltered directives:\n $uflist\n--ERROR"); + ok(0, "($lang/$flavor) $fullpath has unfiltered directives"); + diag(" -- ERRORS: --\n $uflist\n"); } else { # Find any members of the exclusion list which were not found my @notfound; foreach my $directive (keys %{$safe{$file}}) { - push(@notfound, $directive) if ($safe{$file}{$directive} == 0); + push(@notfound, $directive) if ($safe{$file}{$directive} == 0); } if (@notfound) { my $nflist = join("\n ", @notfound); - ok(0, "($lang/$flavor) $fullpath - filterexceptions.pl has extra members:\n $nflist\n" . - "--WARNING"); + ok(0, "($lang/$flavor) $fullpath - filterexceptions.pl has extra members"); + diag(" -- WARNING: --\n $nflist\n"); } else { # Don't use the full path here - it's too long and unwieldy. @@ -155,17 +156,17 @@ sub directive_ok { my ($file, $directive) = @_; # Comments - return 1 if $directive =~ /^[+-]?#/; + return 1 if $directive =~ /^[+-]?#/s; # Remove any leading/trailing + or - and whitespace. - $directive =~ s/^[+-]?\s*//; - $directive =~ s/\s*[+-]?$//; + $directive =~ s/^[+-]?\s*//s; + $directive =~ s/\s*[+-]?$//s; # Empty directives are ok; they are usually line break helpers return 1 if $directive eq ''; # Make sure we're not looking for ./ in the $safe hash - $file =~ s#^\./##; + $file =~ s#^\./##s; # Exclude those on the nofilter list if (defined($safe{$file}{$directive})) { @@ -174,7 +175,7 @@ sub directive_ok { }; # Directives - return 1 if $directive =~ /^(IF|END|UNLESS|FOREACH|PROCESS|INCLUDE| + return 1 if $directive =~ /^(IF|END|UNLESS|FOR|PROCESS|INCLUDE| BLOCK|USE|ELSE|NEXT|LAST|DEFAULT|FLUSH| ELSIF|SET|SWITCH|CASE|WHILE|RETURN|STOP| TRY|CATCH|FINAL|THROW|CLEAR|MACRO|FILTER)/x; @@ -185,45 +186,47 @@ sub directive_ok { } # + - * / - return 1 if $directive =~ /[+\-*\/]/; + return 1 if $directive =~ /[+\-*\/]/s; # Numbers - return 1 if $directive =~ /^[0-9]+$/; + return 1 if $directive =~ /^[0-9]+$/s; # Simple assignments - return 1 if $directive =~ /^[\w\.\$\{\}]+\s+=\s+/; + return 1 if $directive =~ /^[\w\.\$\{\}]+\s*=/s; - # Conditional literals with either sort of quotes + # Conditional literals with either sort of quotes # There must be no $ in the string for it to be a literal - return 1 if $directive =~ /^(["'])[^\$]*[^\\]\1/; - return 1 if $directive =~ /^(["'])\1/; + return 1 if $directive =~ /^(["'])[^\$]*[^\\]\1/s; + return 1 if $directive =~ /^(["'])\1/s; # Special values always used for numbers - return 1 if $directive =~ /^[ijkn]$/; - return 1 if $directive =~ /^count$/; - - # Params - return 1 if $directive =~ /^Param\(/; - - # Hooks - return 1 if $directive =~ /^Hook.process\(/; + return 1 if $directive =~ /^[ijkn]$/s; + return 1 if $directive =~ /^count$/s; + return 1 if $directive =~ /\.id$/s; + return 1 if $directive =~ /(^|\.)bug_id$/s; - # Other functions guaranteed to return OK output - return 1 if $directive =~ /^(time2str|url)\(/; + # Params + return 1 if $directive =~ /^Param\(.*\)$/s; + + # Hooks + return 1 if $directive =~ /^Hook.process\(.*\)$/s; + + # Other functions guaranteed to return safe output + return 1 if $directive =~ /^(time2str|url|html_select|json|L)\(.*\)$/s; # Safe Template Toolkit virtual methods - return 1 if $directive =~ /\.(length$|size$|push\(|unshift\(|delete\()/; + return 1 if $directive =~ /\.(length$|size$|(push|unshift|delete)\(.*\)$)/s; # Special Template Toolkit loop variable - return 1 if $directive =~ /^loop\.(index|count)$/; - - # Branding terms - return 1 if $directive =~ /^terms\./; - + return 1 if $directive =~ /^loop\.(index|count)$/s; + + # Branding terms, constants + return 1 if $directive =~ /^(terms|constants)\.\w+$/s; + # Things which are already filtered - # Note: If a single directive prints two things, and only one is + # Note: If a single directive prints two things, and only one is # filtered, we may not catch that case. - return 1 if $directive =~ /FILTER\ (html|csv|js|base64|url_quote|css_class_quote| + return 1 if $directive =~ /(FILTER|[^\|]\|)\s*(html|csv|js|base64|url_quote|css_class_quote| ics|quoteUrls|time|uri|xml|lower|html_light| obsolete|inactive|closed|unitconvert| txt|none)\b/x; diff --git a/template/en/default/admin/custom_fields/list.html.tmpl b/template/en/default/admin/custom_fields/list.html.tmpl index faa2466cc..104795089 100644 --- a/template/en/default/admin/custom_fields/list.html.tmpl +++ b/template/en/default/admin/custom_fields/list.html.tmpl @@ -175,7 +175,7 @@ [% BLOCK is_tweakable_values %] [% IF row.can_tweak('value_field_id') || row.value_field_id %] [% IF row.can_tweak('value_field_id') %][% END %] - per-[% row.value_field && row.value_field.description || 'any' %] + per-[% row.value_field && row.value_field.description || 'any' | html %] [% IF row.can_tweak('value_field_id') %][% END %] [% END %] [% END %] @@ -183,7 +183,7 @@ [% BLOCK is_tweakable_visible %] [% IF row.can_tweak('visibility_field_id') || row.visibility_field_id %] [% IF row.can_tweak('visibility_field_id') %][% END %] - per-[% row.visibility_field && row.visibility_field.description || 'any' %] + per-[% row.visibility_field && row.visibility_field.description || 'any' | html %] [% IF row.can_tweak('visibility_field_id') %][% END %] [% END %] [% END %] diff --git a/template/en/default/admin/editemailin.html.tmpl b/template/en/default/admin/editemailin.html.tmpl index d2bf4c9fe..f0ce2b1f3 100644 --- a/template/en/default/admin/editemailin.html.tmpl +++ b/template/en/default/admin/editemailin.html.tmpl @@ -14,13 +14,13 @@ [% IF !mode_add %] -[% FOR f IN fields %] +[% FOR f = fields %] [% IF la != f.address %] - + [% SET la = f.address %] [% END %] - + diff --git a/template/en/default/admin/fieldvalues/control-list.html.tmpl b/template/en/default/admin/fieldvalues/control-list.html.tmpl index a1035693f..b8920da96 100644 --- a/template/en/default/admin/fieldvalues/control-list.html.tmpl +++ b/template/en/default/admin/fieldvalues/control-list.html.tmpl @@ -7,7 +7,7 @@ [% PROCESS global/header.html.tmpl %] -

[% title %]

+

[% title | none %]

diff --git a/template/en/default/admin/fieldvalues/list.html.tmpl b/template/en/default/admin/fieldvalues/list.html.tmpl index b6fca3723..cb381ee48 100644 --- a/template/en/default/admin/fieldvalues/list.html.tmpl +++ b/template/en/default/admin/fieldvalues/list.html.tmpl @@ -77,7 +77,7 @@ Select value for the '[% field.description | html %]' ([% field.name | html %])

Values for the '[% field.description | html %]' ([% field.name | html %]) field

[% IF field.name == "component" || field.name == "version" || field.name == "target_milestone" || field.name == "product" %] -

[% field.description %]s must be edited from a product page. Select a product first.

+

[% field.description | html %]s must be edited from a product page. Select a product first.

[% ELSE %] [% PROCESS admin/table.html.tmpl columns = columns diff --git a/template/en/default/admin/params/editparams.html.tmpl b/template/en/default/admin/params/editparams.html.tmpl index 4544250b4..48edbf8ec 100644 --- a/template/en/default/admin/params/editparams.html.tmpl +++ b/template/en/default/admin/params/editparams.html.tmpl @@ -97,7 +97,7 @@ at least by testing it and filing bugs! -

[% current_panel.desc %]

+

[% current_panel.desc | none %]

This lets you edit the basic operating parameters of [% terms.Bugzilla %]. diff --git a/template/en/default/bug/checkaccess.html.tmpl b/template/en/default/bug/checkaccess.html.tmpl index d8d2708b0..0a0ce4fd6 100644 --- a/template/en/default/bug/checkaccess.html.tmpl +++ b/template/en/default/bug/checkaccess.html.tmpl @@ -19,7 +19,7 @@

[% FOREACH user = user_list %] - + [% END %] diff --git a/template/en/default/bug/create/create.html.tmpl b/template/en/default/bug/create/create.html.tmpl index 5c7b96291..26394cf04 100644 --- a/template/en/default/bug/create/create.html.tmpl +++ b/template/en/default/bug/create/create.html.tmpl @@ -119,7 +119,7 @@ var close_status_array = [
FieldValue
[% f.address %] (add a field value for this address)
[% f.address | html %] (add a field value for this address)
[% field_descs.${f.field} %]:  [% field_descs.${f.field} | html %]:  
[% user.1 %][% user.2 %][% user.1 | html %][% user.2 | html %]
[% defaultcontent = BLOCK %] [% IF cloned_bug_id %] -+++ This [% terms.bug %] was initially created as a clone of [% terms.Bug %] #[% cloned_bug_id %][% IF cloned_comment %] comment [% cloned_comment %][% END %] +++ ++++ This [% terms.bug %] was initially created as a clone of [% terms.Bug %] #[% cloned_bug_id %][% IF cloned_comment %] comment [% cloned_comment | html %][% END %] +++ [% END %] diff --git a/template/en/default/bug/edit.html.tmpl b/template/en/default/bug/edit.html.tmpl index 8a5a1270f..bf21c3440 100644 --- a/template/en/default/bug/edit.html.tmpl +++ b/template/en/default/bug/edit.html.tmpl @@ -408,7 +408,7 @@ document.changeform = document.[% cfname %]; %]
- +