476305 - Clean up and merge HTML filtering code

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Vitaly Fedrushkov]

Anyone remember why do we have separate Bugzilla::Util::html_quote() and Template::Filters::html_filter()?  Both are identical and the former is traceable to fossilized CGI.pl v1.1

The latter is overridden (by bug 120030 and bug 319331) to construct widely used 'FILTER html'.  However, in Perl code html_quote() is used consistently, lacking these overrides.

Comment 1

[Max Kanat-Alexander]

We should probably standardize on one, but in some way that's easy to use inside Perl code.

Comment 2

[Vitaly Fedrushkov]

Intentions:

Inherit Bugzilla::Util::html_quote() from Template::Filters::html_filter(), for possible future changes of the latter.

Move @ and BiDi handling to Bugzilla::Util::html_quote().

In Bugzilla::Template reduce 'html' FILTER to html_quote() call.  Optionally add scalar operator for the same, to reduce need for pieces like http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/webtools/bugzilla/template/en/default/account/created.html.tmpl&rev=1.9&root=/cvsroot#28:

| [% title = BLOCK %] 
|   Request for new user account '[% login FILTER html %]' submitted
| [% END %]
| [% PROCESS global/header.html.tmpl title = title %] 

About @ escaping (per bug 120030 comment 146): it had been working back in 2003, but is a no-op for spammers in 2009.  Using escaped syntax to render <a href="mailto:..."> URL may break at least some clients.  Either we carefully check all html_quote() usage cases or revert this countermeasure entirely as obsolete, in favor of landed bug 219021.

Comment 3

[Vitaly Fedrushkov]

Attachment: patch v1

Patch, according to intentions.

Note that http://www.w3.org/International/questions/qa-bidi-controls suggests Unicode BiDi controls should be replaced by HTML tags.  Current implementation may be questioned if/when any semitic language l10n happens.

Comment 4

[Frédéric Buclin]

(In reply to comment #2)
> add scalar operator for the same

I don't understand what you are trying to do here. Why would foo.html be useful? This would give us two ways to do the same thing, which is IMO a bad idea.

Comment 5

[Marc Schumann [:Wurblzap]]

I think I don't understand either. Shouldn't the new html_quote take care
of <, > and so on, as the old one does?

Comment 6

[Vitaly Fedrushkov]

(In reply to comment #5)
> I think I don't understand either. Shouldn't the new html_quote take care
> of <, > and so on, as the old one does?

Template::Filters::html_filter does exactly this

Comment 7

[Vitaly Fedrushkov]

(In reply to comment #4)
> I don't understand what you are trying to do here. Why would foo.html be
> useful? This would give us two ways to do the same thing, which is IMO a bad
> idea.

compare comment 2 and

| [% PROCESS global/header.html.tmpl 
|    title = "Request for new user account ${login}.html submitted" %]

Comment 8

[Marc Schumann [:Wurblzap]]

Maybe I'm seeing some light now... Is this a filter? Does ${login}.html mean that $login gets passed through the html filter?

Comment 9

[Marc Schumann [:Wurblzap]]

Comment on attachment 363624 [details] [diff] [review]
patch v1

As mentioned on IRC: test 7 fails with this patch. Otherwise, I think this is all right. I'll take a look at an updated patch.

Comment 10

[Frédéric Buclin]

It's not OK to me, from a security point of view. The example in comment 2 will force us to filter the login name, else 008filter.t will complain. With your new example, it's too easy to forget to filter the login name, and I'm sure people is going to abuse this syntax as soon as it's available. Did you think about that? I fixed enough security bugs related to filtering to know it will be a problem.

Comment 11

[Frédéric Buclin]

(In reply to comment #7)
> |    title = "Request for new user account ${login}.html submitted" %]

Also, your example is wrong. If you want it to be .html, then your scalar operators are wrongly named.

Comment 12

[Marc Schumann [:Wurblzap]]

(In reply to comment #11)
> Also, your example is wrong. If you want it to be .html, then your scalar
> operators are wrongly named.

Yeah, he already noticed this (on IRC).

Frédéric raises a valid point. Syntactic sugar is all right, but is in Bugzilla's security context not helpful. It's nice to be able to format_time or somesuch directly, but only this. So html and url_quote filters should *not* support the .FILTER syntax. Or if they do, we shouldn't rely on them, i.e. we shouldn't remove our current filter checks.

SnowyOwl, maybe it's best to keep the scalar filters out of this bug.

Comment 13

[Max Kanat-Alexander]

I'm opposed to the .html form, it won't be caught as a filter by our tests, and it allows too many ways to do the same thing, when we're trying for more consistency.

Comment 14

[Vitaly Fedrushkov]

Attachment: patch v2

OK let's split obvious things from controversial:

This patch is a cleanup only, addressing comment 9.

Comment 15

[Frédéric Buclin]

Comment on attachment 364989 [details] [diff] [review]
patch v2

Works fine. And now FILTER html and html_quote() work consistently. r=LpSolit

Comment 16

[Frédéric Buclin]

Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.102; previous revision: 1.101
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.90; previous revision: 1.89
done
Checking in t/007util.t;
/cvsroot/mozilla/webtools/bugzilla/t/007util.t,v  <--  007util.t
new revision: 1.13; previous revision: 1.12
done