133425 - Audit show_bug.html.tmpl for FILTER usage

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Andreas Franke (gone)]

With the templatized show_bug, I just got a summary truncated, probably because
it contains quoted text: "..." 

<justdave> that whole template should probably be audited for filter usage.
<justdave> just about nothing from "bug.*" should be allowed to be displayed
without filtering

Comment 1

[Andreas Franke (gone)]

So here is what I did to make it work again, at least in summaries:

1. In CGI.pl, in sub html_quote, I added a line for replacing " with " :

sub html_quote {
    my ($var) = (@_);
    $var =~ s/\&/\&/g;
    $var =~ s/</\&lt;/g;
    $var =~ s/>/\&gt;/g;
    $var =~ s/"/\&quot;/g;
    return $var;
}

2. In show_bug.html.tmpl, I added FILTER html basically everywhere (following
   the examples in query.atml), i.e. in places like this: value="[% ... %]"
   and in places like this: <td>[% ... %]</td>

   I probably missed some, and maybe there are some that shouldn't be FILTERed,
   but here they are (the lines may be off a bit because I have other changes
   in that file, so take it with a grain of salt):

63c64
<         [% bug.reporter %]
---
>         [% bug.reporter FILTER html%]
91c92
<           <a href="describecomponents.cgi?product=[% bug.product %]">
---
>           <a href="describecomponents.cgi?product=[% bug.product FILTER uri
%]">
98,99c99,100
<             <option value="[% x %]"
<               [% " selected" IF x == bug.component %]>[% x %]</option>
---
>             <option value="[% x FILTER html %]"
>               [% " selected" IF x == bug.component %]>[% x FILTER html
%]</option>
118c119
<           <option value="[% c %]">[% c %]</option>
---
>           <option value="[% c FILTER html %]">[% c FILTER html %]</option>
136c137
<       <td>[% bug.bug_status %]</td>
---
>       <td>[% bug.bug_status FILTER html %]</td>
151c152
<       <td>[% bug.resolution %]</td>
---
>       <td>[% bug.resolution FILTER html %]</td>
167c168
<       <td>[% bug.assigned_to %]</td>
---
>       <td>[% bug.assigned_to FILTER html %]</td>
190c196
<          <input name="qa_contact" value="[% bug.qa_contact %]" size="60">
---
>          <input name="qa_contact" value="[% bug.qa_contact FILTER html %]"
size="60">
198c204
<         [% IF bug.url %]
---
>         [% IF bug.bug_file_loc %]
206c212
<       <input name="bug_file_loc" value="[% bug.bug_file_loc %]" size="60">
---
>       <input name="bug_file_loc" value="[% bug.bug_file_loc FILTER html %]"
size="60">
215c221
<       <input name="short_desc" value="[% bug.short_desc %]" size="60">
---
>       <input name="short_desc" value="[% bug.short_desc FILTER html %]"
size="60">
225c231
<         <input name="status_whiteboard" value="[% bug.status_whiteboard %]"
---
>         <input name="status_whiteboard" value="[% bug.status_whiteboard FILTER
html %]"
238c244
<         <input name="keywords" value="[% bug.keywords.join(', ') %]"
---
>         <input name="keywords" value="[% bug.keywords.join(', ') FILTER html
%]"
354c360
<   Leave as <b>[% bug.bug_status %]&nbsp;[% bug.resolution %]</b>
---
>   Leave as <b>[% bug.bug_status FILTER html %]&nbsp;[% bug.resolution FILTER
html %]</b>
381c387
<         <b>[% bug.resolution %]</b>)<br>
---
>         <b>[% bug.resolution FILTER html %]</b>)<br>
407c413
<              onchange="if ((this.value != '[% bug.assigned_to_email %]') &&
---
>              onchange="if ((this.value != '[% bug.assigned_to_email FILTER js
%]') &&
411c417
<              value="[% bug.assigned_to_email %]">
---
>              value="[% bug.assigned_to_email FILTER html %]">
523,524c529,530
<         <option value="[% x %]"
<           [% " selected" IF x == bug.${selname} %]>[% x %]</option>
---
>         <option value="[% x FILTER html %]"
>           [% " selected" IF x == bug.${selname} %]>[% x FILTER html
%]</option>

(There's one place where I added FILTER js, but I'm not sure if that's
necessary. And there's the bug.url -> bug.bug_file_loc fix.)

Comment 2

[Andreas Franke (gone)]

See also bug 133423 for auditing global/header. Are there other templates where
this needs to be done?

Comment 3

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This patch fixes all FILTERs, the 'urlbase' problem and the bug_file_loc
mistake. The changes are trivial to review.

Gerv

Comment 4

[Gervase Markham [:gerv]]

Taking.

Gerv

Comment 5

[Andreas Franke (gone)]

Comment on attachment 76167 [details] [diff] [review]
Patch v.1

You need to patch html_quote in CGI.pl, too, to replace
 " with " . I think I tried adding html filters
without that change first, but it didn't work at all
if a summary had quotes in it.

>+[% bug.short_desc = bug.short_desc FILTER html %]
>+

For consistency, I think we shouldn't do tricks like this.
Instead, it should be FILTERed when it is used, i.e. the
title filtering should be done in global/header (see bug
133423). Alternatively, _all_ bug.* that need to be 
filtered could be handled as above, but this has problems:
it doesn't work easily for lists/arrays, and you don't 
have easy access to the unfiltered value any more (though
I don't know if that's used anywhere).

> [% UNLESS header_done %]
>   [% INCLUDE global/header 
>-    title = "Bug $bug.bug_id - $bug.short_desc"
>+    title = "Bug $bug.bug_id - $bug.short_desc" 
>     h1 = "Bugzilla Bug $bug.bug_id"
>-    h2 = bug.short_desc
>+    h2 = bug.short_desc 

You added whitespace at the end of these lines!?

>       <td>
>         <select name="component">
>           [% FOREACH x = component_ %]
>-            <option value="[% x %]"
>+            <option value="[% x FILTER html %]"
>               [% " selected" IF x == bug.component_ %]>[% x %]</option>

should be [% x FILTER html %]

>@@ -212,7 +216,8 @@
>       <b>Summary:</b>
>     </td>
>     <td colspan="7">
>-      <input name="short_desc" value="[% bug.short_desc %]" size="60">
>+      <input name="short_desc" 
>+             value="[% bug.short_desc FILTER html %]" size="60">
>     </td>
>   </tr>

And here you filter it again?? This is correct, but then the lines
at the beginning are obsolete, aren't they? Or can you explain?

>     <select name="[% selname %]">
>       [% FOREACH x = ${selname} %]
>-        <option value="[% x %]"
>+        <option value="[% x FILTER html %]"
>           [% " selected" IF x == bug.${selname} %]>[% x %]</option>

should be [% x FILTER html %]

Also, you forgot to change this:
411c417
<	       value="[% bug.assigned_to_email %]">
---
>              value="[% bug.assigned_to_email FILTER html %]">
right after the onchange=... fix.

Other than that, nice cleanup :-)

Comment 6

[Gervase Markham [:gerv]]

> You need to patch html_quote in CGI.pl, too, to replace
>  " with " . 

Ah, yes. I was wrong - we stopped using the Template Toolkit HTML filter for
performance reasons.


> >+[% bug.short_desc = bug.short_desc FILTER html %]
> >+
> 
> For consistency, I think we shouldn't do tricks like this.

The problem is that it's hard to call global/header in a sane way otherwise. But
if that template does its own filtering, then we can just remove this line.

> You added whitespace at the end of these lines!?

Oops.


> >-      <input name="short_desc" value="[% bug.short_desc %]" size="60">
> >+      <input name="short_desc" 
> >+             value="[% bug.short_desc FILTER html %]" size="60">
> 
> And here you filter it again?? This is correct, but then the lines
> at the beginning are obsolete, aren't they? Or can you explain?

Consistency. But the correct solution is the one above, so never mind this.

> Also, you forgot to change this:
> 411c417
> < value="[% bug.assigned_to_email %]">

No, I didn't forget - it's an email address and so does not contain any
characters which need escaping. So I didn't bother. Is that reasonable?

Gerv

Comment 7

[Andreas Franke (gone)]

See also bug 133406. (I missed that one when filing this.)

> > >+[% bug.short_desc = bug.short_desc FILTER html %]
> > >+
> > 
> > For consistency, I think we shouldn't do tricks like this.
>
> The problem is that it's hard to call global/header in a sane way otherwise. 
> But if that template does its own filtering, then we can just remove this 
> line.

What is hard in fixing the FILTERing in global/header, too? If you have this one
and the one below, and you make html_quote escape quotes, then you are
double-escaping, aren't you? Have you tested this?

> > < value="[% bug.assigned_to_email %]">
> 
> No, I didn't forget - it's an email address and so does not contain any
> characters which need escaping. So I didn't bother. Is that reasonable?

Yes, that sounds reasonable. Maybe there should be some comments somewhere about
which bug.* values don't need escaping.

Comment 8

[Bradley Baetz (:bbaetz)]

+ is valid in an email address. Although thtas uri escaping, mind you.

comments.tmpl needs email uri escaped, and name html escaped too, I think, BTW.

Comment 9

[David D. Kilzer (ddk)]

As long as we're auditing show_bug.html.tmpl, could we make 'filter'
all uppercase (line 173) to match its usage everywhere else?

This patch fragment was from v1.2 -> v1.3 of
template/default/show/show_bug.html.tmpl

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/webtools/bugzilla/template/default/show&command=DIFF_FRAMESET&file=show_bug.html.tmpl&rev1=1.2&rev2=1.3&root=/cvsroot

The exact patch is not attached to any bug, though Bug 133372
was filed for this specific issue.

@@ -170,7 +170,7 @@
       [% IF Param("usetargetmilestone") && bug.target_milestone %]
         <td align="right">
           <b>
-            <a href="[% bug.milestoneurl %]">Target Milestone</a>:
+            <a href="[% bug.milestoneurl filter uri %]">Target Milestone</a>:
           </b>
         </td>
         [% PROCESS select selname = "target_milestone" %]

Comment 10

[David D. Kilzer (ddk)]

Ack!  Nevermind.  This was fixed in v1.4 of show_bug.html.tmpl.

Comment 11

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Here's version 2. It fixes the review comments, and adds a few more filters I
missed. 

We have two options to avoid double-filtering: 
- things should be filtered at the point of printing
- things should be filtered at the point of them entering the template system

I think the first is easiest, and it doesn't break if you rearrange templates.
Let's stick with that for the moment.

Gerv

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 76283 [details] [diff] [review]
Patch v.2

>Index: template/default/show/show_bug.html.tmpl

>@@ -222,21 +226,21 @@

>-          <A HREF="describekeywords.cgi">Keywords:</A>
>+          <A href="describekeywords.cgi">Keywords:</a>

You lowercased the "href" but not the "a"...

fix that when you check it in.

r= justdave

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 76283 [details] [diff] [review]
Patch v.2

Actually, you lowercased the </a> but not the <A>

You have also included the change to use votes.cgi rather than showvotes.cgi.

Remove that, too

r=bbaetz

Comment 14

[Gervase Markham [:gerv]]

Fixed.

Checking in template/default/show/show_bug.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/show_bug.html.tmpl,v 
<--  show_bug.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.138; previous revision: 1.137
done

Gerv

Comment 15

[Andreas Franke (gone)]

Attachment: more fun

I think you missed some... These are the three places where I had added filters
in my tree, but that didn't get checked in with the last patch.

Comment 16

[Andreas Franke (gone)]

Reopening for attachment 76389 [details] [diff] [review]...

Comment 17

[David D. Kilzer (ddk)]

*** Bug 133283 has been marked as a duplicate of this bug. ***

Comment 18

[Gervase Markham [:gerv]]

Comment on attachment 76389 [details] [diff] [review]
more fun

You're right; the CC list probably doesn't need filtering, but it can't hurt.

2xr=gerv. Please check in.

Gerv

Comment 19

[Dave Miller [:justdave]]

Checking in template/default/show/show_bug.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/show/show_bug.html.tmpl,v 
<--  show_bug.html.tmpl
new revision: 1.7; previous revision: 1.6
done