Closed Bug 133423 Opened 22 years ago Closed 22 years ago

Audit templates for FILTER usage

Categories

(Bugzilla :: User Interface, defect, P1)

Product:
Bugzilla
Component:
User Interface
Version:
2.15
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: afranke, Assigned: bbaetz)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

v2
21.15 KB, patch
gerv
: review+
justdave
: review+
Details | Diff | Splinter Review 
<justdave> just about nothing from "bug.*" should be allowed to be displayed
without filtering

This may affect the title passed from show_bug.html.tmpl to global/header, which
is then used unfiltered.
What if you wanted html in there ? the <title> should be html escaped, but h1/h2
should be escaped by the caller.

Taking for 2.16 - this is a regression.
Assignee: myk → bbaetz
Keywords: regression
Target Milestone: --- → Bugzilla 2.16
OK, so I got involved, and audioted every template file.

Notes:

component/product/etc descriptions are html - maybe we should mention that on
the edit pages?

I couldn't work out how to filter at the same time as passing into an INCLUDE,
so I split that up. If you know how, please let me know.

I think this is it. If I missed a class of things to filter, I'd prefer to do
that as a separate bug; if I missed particular changes, then mention that here.
Status: NEW → ASSIGNED
Keywords: patch, review
Summary: Audit global/header for FILTER usage → Audit templates for FILTER usage
Attached patch v1 (obsolete) — — Splinter Review 

    
    

    
  
Comment on attachment 77395 [details] [diff] [review]
v1

>+[% filteredSummary = bugsummary FILTER html %]

Are we using StudlyCaps for variable names? I didn't think we were.
I'd go for filtered_summary . Same for the other occurrences.

>@@ -136,7 +136,7 @@
>     </tr>
>   </table>
> 
>-  [% PROCESS show/comments.tmpl %]
>+  [% PROCESS show/comments.tmpl comments=bug.comments %]
>   
>   <hr>
> [% END %]

This change is already in.

Gerv

        Attachment #77395 -
        Flags: review-

    
    

    
  
I'd prefer using the variable directly, but I don't know how. I'll post to the
TT list, and ask.

    
    

    
  
This is a bunch of potential security holes if we don't clean this up.  marking
as a blocker.
Severity: normal → blocker
Priority: -- → P1

    
  
Blocks: 97495

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2
        — 
          — Splinter Review
      

    


  
myk suggested adding a ; before the variable I want to filter, but that then
ends the INCLUDE, and the later bits just become variable assignments. I'll use
a temp val instead.

He also suggested doing teh filtering in the header template, but h2 can
contain html, so we don't want this. There is a possible problem in that h1
defaults to the value of the title attribute, so we may want to escape the
DEFAULT for the header somehow. What do people think of that?

        Attachment #77395 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 78025 [details] [diff] [review]
v2

>+[% filtered_summary = bugsummary FILTER html %]
> [% INCLUDE global/header 
>   title = "View All Attachments for Bug #$bugid"
>   h1 = "View All Attachments for <a href=\"show_bug.cgi?id=$bugid\">Bug #$bugid</a>"
>-  h2 = bugsummary
>+  h2 = filtered_summary

Can this be done by putting h2 FILTER html as the last entry in the INCLUDE?

The current method is fine, although I agree that it would be nice to 
have something less clunky. So r=gerv.

Gerv

        Attachment #78025 -
        Flags: review+

    
    

    
  
Comment on attachment 78025 [details] [diff] [review]
v2

>Index: template/default/prefs/account.tmpl

>-          <td>[% new_login_name %]</td>
>+          <td>[% new_login_name FILTER html %]</td>

>Index: template/default/show/comments.tmpl

>-      <a href="mailto:[% comment.email %]">[% comment.name %]</a>
>+      <a href="mailto:[% comment.email %]">[% comment.name FILTER html %]</a>

We filter login_name everywhere else, and comment.email is the same data, so we
ought to filter it here, too.  probably uri rather than html, in this case
though.

r= justdave if you fix that.

        Attachment #78025 -
        Flags: review+

    
    

    
  
gerv: No, that FILTERs the actual template, too, so we literatlly print
<html><head> etc.

justdave: It actually needs to be done trhough both - if the email addr
contained an &, then that would need to be escaped to &amp;, too

I'll do that before checking in.

    
    

    
  
Actually, it does only need to be html filtered, because characters which are
illegal in uris are also illegal in email addresses.

Checked in with just html filtering for that bit.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: