Closed Bug 136180 Opened 23 years ago Closed 23 years ago

'FILTER html' used in *.txt.tmpl files instead of 'FILTER uri'

Categories

(Bugzilla :: Email Notifications, defect)

Product:
Bugzilla
Component:
Email Notifications
Version:
2.15
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: ddkilzer, Assigned: ddkilzer)

References

Details

(Keywords: regression)

Attachments

(3 files, 7 obsolete files)

Patch v.8 Part 1 Changes to non-template files
2.59 KB, patch
Details | Diff | Splinter Review
Patch v.8 Part 2 Changes to template/default files
7.24 KB, patch
Details | Diff | Splinter Review
Patch v.8 Part 3 Changes to template/en/default files
7.36 KB, patch
gerv
: review+
gerv
: review+
Details | Diff | Splinter Review 
The patch for Bug 133423 added 'FILTER html' to variables in:

  template/default/token/emailchangenew.txt.tmpl
  template/default/token/emailchangeold.txt.tmpl

However, the output of these variables appears as part of a URL
and should be filtered using 'uri' instead of 'html'.  Patch to
follow shortly.
Status: NEW → ASSIGNED
Keywords: patch, regression, review
Target Milestone: --- → Bugzilla 2.16
Attached patch Patch v.1 (obsolete) — — Splinter Review 
Change 'FILTER html' to 'FILTER uri'.
And please recall that I don't have checkin permission, so if
you 2x=r this, please check it in.  :^)
Comment on attachment 78190 [details] [diff] [review]
Patch v.1

Looks good to me. But as producer of the original patch, I want bbaetz to
quickly confirm this is right.

r=gerv.

Gerv
Attachment #78190 - Flags: review+
Comment on attachment 78190 [details] [diff] [review]
Patch v.1

I originally did this bacsed on teh fact that the token can contain &. Since
this isn't html, uri filter is probably best. However, our uri parsing code
doesn't like & in params (products with & aren't really liked either) - should
we just remove & from the list of valid password chars in GerneateRandomToken.
Can someone test if this works?
Attachment #78190 - Flags: review+
URI parsing code should be able to handle any properly-encoded
string of variables and values.  If it can't handle an encoded
'&' (%26) in such a string, I would say that this code is
broken.  However, I haven't looked at the code itself, so I'm
not aware of the other issue you described.

Where is the URI parsing code located?
The problem is that the & isn't encoded because & is a valid thing to have in a
url - the tt docs explictly say so

And so the parsing code fails because & is the separator.
Token.pm uses url_quote before passing the tokens and email addresses to the
templates.  If TT filters are now to be used, Token.pm should be changed to pass
the raw data to the templates.  Token.pm has been re-written in bug 134805 for
this purpose.
So is this blocked on bug 134805 or can I go ahead and check it in?
Could this patch be extended to remove the two url_quote's from
IssueEmailChangeToken, rather than waiting for bug 134805.
Ugh.  After doing some tests with the 'FILTER uri' in TT, I would
have to say it has at best unexpected behavior, and at worst is
broken (at least for what Bugzilla uses it for).

Essentially, 'FILTER uri' is only useful when you have a URI (or
URL) that accidentally contains "unsafe" characters that were not
caught during the encoding of individual variable names and
values.  This means that if you have a variable name or a value
with '&' or '=' in it, 'FILTER uri' will NOT escape it properly.
(Look at the uri_filter() subroutine in Template/Filters.pm.)

The proper way to encode a query string is to create a hash of
variable names (hash keys) and values (hash values), then
assemble them by joining the variable names and values with '=',
then joining each variable-value pair with '&', then joining
that string and the 'http://host/path/file' string with '?'.

When the variable names and values are first joined with '=',
they should each be encoded using something like url_quote()
from CGI.pl that quotes ALL "special" characters.  In fact,
it's better to over-quote (i.e., quote characters that don't
have to be quoted like A..Z, a..z, 0..9,e tc.) than to under-
quote.  The unquoting mechanism will work just fine if 'A'
is encoded into %41 in a URI.

Note that if part of the query string is static text, then
only individual variable names or values need to be quoted
using url_quote() or a similar subroutine.

In looking through the current set of templates, it appears
that there is a mix of using 'FILTER uri' on individual
values, whole URLs, and strings like "&order=$order".  The
only case where 'FILTER uri' is appropriate is on whole URLs
anyway.  The other cases should use a url_quote() filter
(and be separated in some cases).

Should the focus of this bug change to providing a 'url_quote'
filter in the $template object and re-auditing the use of
'FILTER uri' in the templates?

Also note that the manpage for Template::Filters recommends
that full URLs should be passed through the 'html' filter as
well as the 'uri' filter since correct HTML wants the '&' in
query strings escaped to '&' inside HREF attributes.

Oh, and here is a link to RFC 2396 which the Template::Filters
manpage refers to.

  http://www.faqs.org/rfcs/rfc2396.html
> Should the focus of this bug change to providing a 'url_quote'
> filter in the $template object and re-auditing the use of
> 'FILTER uri' in the templates?

That sounds very sensible. I suggest we add a "url" filter which uses
url_quote(), unless anyone thinks it's confusingly similar to the "uri" filter.

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

marking needs-work on behalf of the last several comments on this bug and to
get it off the "to be checked in" list since it won't be.
Attachment #78190 - Flags: review+ → review-
Attached patch Patch v.2 (obsolete) — — Splinter Review 
- Adds 'url_quote' filter to global $template object variable.
  (I thought 'url' was too close to 'uri' and would be too
  confusing.  If someone has a better name than 'url_quote',
  I'd be happy to change it.)

- Fixes use of 'uri' filter to use 'url_quote' as appropriate.
  Original fixes for the token/emailchangenew.txt.tmpl
  andtoken/emailchangeold.txt.tmpl now use 'url_quote' filter.
Attachment #78190 - Attachment is obsolete: true
Blocks: 135817
Comment on attachment 78908 [details] [diff] [review]
Patch v.2

Forgot to remove use of url_quote() from Token.pm.
Attachment #78908 - Flags: review-
Attached patch Patch v.3 (obsolete) — — Splinter Review 
Removes use of url_quote() subroutine to pre-quote $token
and $newtoken before passing to templates.

Note that token/tokencancel.txt.tmpl should not have $token
escaped (since it's not used in a URL).

Also note the remaining use of url_quote() in Token.pm is
used in an inline text message sent to the SENDMAIL file
handle, not a template.
Attachment #78908 - Attachment is obsolete: true
Comment on attachment 79248 [details] [diff] [review]
Patch v.3

Looks sane to me. r=gerv.

Gerv
Attachment #79248 - Flags: review+
The usual nits to follow... 
 
> +        [%- "&order=$order" FILTER html IF order %]&tweak=1">Change Several   
&tweak=1 
Also, I'm not sure if FILTER html does the &order=... => &order=... thing. 
  
>      <a href="buglist.cgi?[% urlquerypart %]&order=  
&amp;order= (but this wasn't part of this patch, only in the diff context...) 
  
> +        <A HREF="buglist.cgi?keywords=[% keyword.name FILTER url_quote %]">  
<a href=  
 
filter html does do & => &amp;

Wouldn't this fail tempalte tests, cause you didn't chacnge the test to use this
new filter too? I'm not in a position to test ATM, though.
Comment on attachment 79248 [details] [diff] [review]
Patch v.3

Marking needs-work per Comment #18.
Attachment #79248 - Flags: review-
Blocks: 135543
Attached patch Patch v.4 (obsolete) — — Splinter Review 
- Added dummy url_quote filter in t/004template.t. (Comment #19)
- Fixed '&tweak=1' to '&amp;tweak=1' in buglist/buglist.html.tmpl.
  (Comment #18)
- Fixed '&order=' to '&amp;order=' in two places in
  buglist/table.tmpl. (Yeah, I know, I should file separate bugs,
  but as long as I'm going to be nitted on these things, I figure
  I could just as well fix the issue everywhere in the whole file.)
  (Comment #18)
- Fixed '<A HREF' to '<a href' in info/describe-keywords.html.tmpl.
  (Comment #18)
Attachment #79248 - Attachment is obsolete: true
Comment on attachment 79628 [details] [diff] [review]
Patch v.4

r=gerv.

Gerv
Attachment #79628 - Flags: review+
Comment on attachment 79628 [details] [diff] [review]
Patch v.4

>Index: template/default/buglist/table.tmpl

>@@ -96,9 +98,9 @@
>+      [% column.name FILTER url_quote html %]

Myk and ddk both tell me on IRC that you can't stack filters like this
without either repeating the word FILTER or using a | between them
Attachment #79628 - Flags: review-
Attached patch Patch v.5 (obsolete) — — Splinter Review 
Fixes double filter with no 'FILTER' or '|' separating them in
template/default/buglist/table.tmpl.
Attachment #79628 - Attachment is obsolete: true
Comment on attachment 79811 [details] [diff] [review]
Patch v.5

Found more abuses of double filters with no separators.

find template -name CVS -prune -o -type f -exec grep FILTER {} /dev/null \; | \
grep -v 'FILTER [^ ]* %\]'
Attachment #79811 - Flags: review-
Attached patch Patch v.6 (obsolete) — — Splinter Review 
- Okay, search I did using 'find' was on a CVS-tip directory,
  and Patch v.5 had already taken care of another use of a
  double filter with no separator.
- Changed use of '|' to 'FILTER' in double filter in
  template/default/buglist/table.tmpl to match use in other
  tempaltes.
Attachment #79811 - Attachment is obsolete: true
Comment on attachment 79820 [details] [diff] [review]
Patch v.6

r=gerv. Although we're going to have to do this again for the new versions of
the templates (bug 135707). :-( We'll try and get that bug out of the way ASAP.
I think this is the only open 2.16 bug which touches a lot of templates.

Gerv
Attachment #79820 - Flags: review+
Note that these templates were changed to alter variables (which
requires an INCLUDE instead of a PROCESS per Bug 135707):

template/default/buglist/buglist.html.tmpl
template/default/buglist/table.tmpl

If the patch should NOT do this, let me know and I'll create yet
another patch.  (I presume that I should use a different variable
name like 'order_filtered' instead of 'order' if this is done.)
The difference between INCLUDE and PROCESS is that if a template is PROCESSed,
changes it makes to variables affect the outer template; if it's INCLUDED, they
don't. So problems arise if the inner template uses var $foo locally, and the
outer one uses the same name for something else.

I don't think your patch changes behaviour like that, does it?

Gerv
Yes, these templates use 'FILTER url_quote' to change the value of
the 'order' variable:

template/default/buglist/buglist.html.tmpl
template/default/buglist/table.tmpl

If these templates were PROCESSed from other templates instead of
INCLUDEd, then the 'order' value would be changed permanently.
Should I fix this in the patch?

I could just as well redo the patch for the new template hierarchy
as well....
> Should I fix this in the patch?

It depends if you think anyone would ever have a use for a non-url-quoted order
variable :-)

Gerv


Shouldn't be now
   templates/en/default
be patched instead of
   templates/default
or both?
Comment on attachment 79820 [details] [diff] [review]
Patch v.6

Will change self-modification of 'order' variable and
provide a patch for the new template/en/default templates tree.
Attachment #79820 - Flags: review-
Instead of modifying the 'order' variable, I now create a new
variable named 'qorder' for use in these templates (similar to
$qorder in buglist.cgi):

template/default/buglist/buglist.html.tmpl
template/default/buglist/table.tmpl

Patch for the new template/en/default directory structure to
follow shortly.
Attachment #79820 - Attachment is obsolete: true
Comment on attachment 80025 [details] [diff] [review]
Patch v.7 for old template/default directory

Ack!  Patch suffers from bit rot.  Will post a new three-part patch for
non-template changes, changes to template/default, and 
changes to template/en/default.
Attachment #80025 - Flags: review-
Changes to non-template files.
Attachment #80025 - Attachment is obsolete: true
Part 2 of Patch v.8.  Contains changes to the files under the
template/default directory structure.
Attachment #80589 - Attachment description: Patch v.8 Changes to non-template files → Patch v.8 Part 1 Changes to non-template files
Part 3 of Patch v.8.  Contains changes to the files under the
template/en/default directory structure.
If you still are unclear as to what the difference between the
'uri' and the 'url_quote' filters are, this comment in 
Bug 138701 should help as it gives some good examples.

http://bugzilla.mozilla.org/show_bug.cgi?id=138701#c2
Comment on attachment 80591 [details] [diff] [review]
Patch v.8 Part 3 Changes to template/en/default files

2xr=gerv. This applies to parts 1 and 3 of this patch only. We shouldn't bother
with part 2 IMO.

Gerv
Attachment #80591 - Flags: review+
Fixed (patches 1 and 3 only.)

Checking in Token.pm;
/cvsroot/mozilla/webtools/bugzilla/Token.pm,v  <--  Token.pm
new revision: 1.9; previous revision: 1.8
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.157; previous revision: 1.156
done
Checking in t/004template.t;
/cvsroot/mozilla/webtools/bugzilla/t/004template.t,v  <--  004template.t
new revision: 1.12; previous revision: 1.11
done
... and loads of templates.

Gerv
Status: ASSIGNED → RESOLVED
Closed: 23 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: