Closed
Bug 133423
Opened 23 years ago
Closed 23 years ago
Audit templates for FILTER usage
Categories
(Bugzilla :: User Interface, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: afranke, Assigned: bbaetz)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
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.
Assignee | ||
Comment 1•23 years ago
|
||
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 | ||
Comment 2•23 years ago
|
||
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.
Assignee | ||
Comment 3•23 years ago
|
||
Comment 4•23 years ago
|
||
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-
Assignee | ||
Comment 5•23 years ago
|
||
I'd prefer using the variable directly, but I don't know how. I'll post to the TT list, and ask.
Comment 6•23 years ago
|
||
This is a bunch of potential security holes if we don't clean this up. marking as a blocker.
Severity: normal → blocker
Priority: -- → P1
Assignee | ||
Comment 7•23 years ago
|
||
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 8•23 years ago
|
||
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 9•23 years ago
|
||
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+
Assignee | ||
Comment 10•23 years ago
|
||
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 &, too I'll do that before checking in.
Assignee | ||
Comment 11•23 years ago
|
||
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: 23 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•