159631 - Param('maintainer') should use FILTER html_light in the html templates

Status: NEW

(Bugzilla :: User Interface, defect)

Description

[Tobias Burnus]

bug/create/create.html.tmpl and global/code-error.html.tmpl contain
[% Param("maintainer") %] and I happen to use this as maintainer:
  Bugzilla Maintainer <[e-mail]>
this causes that the email adress is not shown.

Patch to come.

Comment 1

[Tobias Burnus]

Attachment: Add the FILTER html.

Comment 2

[Gervase Markham [:gerv]]

HTML-filtering is not necessarily the correct solution to this. Imagine the
following as Param('maintainer'):

<a href="mailto:[e-mail]">[e-mail]</a>

The correct solution is to label this param as "is HTML" or "may contain HTML"
in the description.

Gerv

Comment 3

[Tobias Burnus]

> HTML-filtering is not necessarily the correct solution to this. Imagine the
> following as Param('maintainer'):
>  <a href="mailto:[e-mail]">[e-mail]</a>
which doesn't work well for the email templates. In the templates itself it
occures only in the body though.

At least in defparams.pl (whinemail) it is used in the mail header:
   From: %maintainer
(in the email templates:  From: bugzilla-admin-daemon).

I don't see a simple solution where both <a href="mailto:"> and "foo <bar@foo>"
is possible. :-(

Comment 4

[Gervase Markham [:gerv]]

<thinks>

How about this. We define the param as: 

"an RFCXXXX-complaint email address, e.g. "Joe Bloggs <[e-mail]>".

We then put it as-is into the mail templates. 
For the HTML, we do something like the following:

[% maintainer =~ s/([a-z1-9\.-]+@[a-z1-9\.-])/<a href="mailto:$1">$1</a>/ %]

i.e. replace the email address with a link. We can copy the regexps from our
autolinkification code. In fact, we can just do:

[% quoteUrls(maintainer) %]

and The Right Thing should happen.

Gerv

Comment 5

[Tobias Burnus]

I failed to find a RFC which defines this 'name <foo@bar>' syntax.

RFC 822 explains the 'foo@bar' format in detail, but I cannot find the spec for
'name' part. (In an example they use 'From:  George Jones <[e-mail]>' so
it might be the correct RFC).

The RFC 1522 causes a problem though:
  From: =?ISO-8859-1?Q?Olle_J=E4rnefors?= <[e-mail]>

I don't know how we want to handle this one. Maybe using a encoding parameter
and let it enter as 'Olle Jörnefors <...>' and do the translation later on.
This would then be part of the/a big encoding patch for emails and http headers.

Comment 6

[Gervase Markham [:gerv]]

> I don't know how we want to handle this one.

Say we don't support RFC1522?

We have two ways of approaching this:
- make it as functional as possible while still preserving the simplicity of a
  single textbox
- make it fully functional, no matter how complicated the prefs to define it get

I'd opt for option 1 - in other words, if you want to put non-ASCII characters
in there, tough. Do your own custom Bugzilla mod :-)

Gerv

Comment 7

[Matthew Tuck [:CodeMachine]]

Separate into two parameters - real name and email.  Format in whatever way is
relevant for the template.

Comment 8

[Gervase Markham [:gerv]]

Do we really need two prefs for defining something as simple as who the
administrator is?

Here's another solution: don't use the admin's real name for sending email with.
All the other email comes from bugzilla-daemon - why should whinemail be different?

Then, we can just use HTML in the box.

Gerv 

Comment 9

[Gervase Markham [:gerv]]

Attachment: Patch v.1

How about this?

Gerv

Comment 10

[Bradley Baetz (:bbaetz)]

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

You can't do this - token emails use this, as does the xml bug export, and so
on.

Comment 11

[Gervase Markham [:gerv]]

Pants.

Then we're back to my question in comment #6.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 92961 [details] [diff] [review]
Add the FILTER html.

Cleaning up old review.

Gerv

Comment 13

[Dave Miller [:justdave]]

The User Interface component now belongs to Gerv.  Reassigning all UNCONFIRMED
and NEW (but not ASSIGNED) bugs currently owned by Myk (the previous component
owner) to Gerv.

Comment 14

[Dave Miller [:justdave]]

Reassigning back to Myk.  That stuff about Gerv taking over the User Interface
component turned out to be short-lived.  Please pardon our confusion, and I'm
very sorry about the spam.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

As Gerv says, "maintainer" should be an email address (or simple, not
RFC-compliant, name-address combo).  We don't need to make things more
complicated and shouldn't.

Comment 16

[Gervase Markham [:gerv]]

Myk: the problem with a name/address combo is that you can't use it in a
mailto:. Or can you?

Gerv

Comment 17

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 19

[Frédéric Buclin]

I think FILTER html_light should fix this problem.

Comment 20

[Justin B]

Attachment: 0001-Bug-159631-Update-Param-maintainer-in-HTML-Templates.patch

Proposed Patch

Comment 21

[Gervase Markham [:gerv]]

(In reply to Justin B from comment #20)
> Created attachment 8822288 [details] [diff] [review]
> 0001-Bug-159631-Update-Param-maintainer-in-HTML-Templates.patch

Hi Justin,

Thanks for the patch! Can you tell us how you tested it? For example, the html_light filter's function seems to depend a lot on whether the html_desc feature is enabled or disabled. Did you test both configurations?

Gerv

Comment 22

[Justin B]

Hi Gerv - I tested with plain email addresses and name/email combo. But noob question here, what/where is the html_desc feature?

Comment 23

[Gervase Markham [:gerv]]

(In reply to Justin B from comment #22)
> Hi Gerv - I tested with plain email addresses and name/email combo. But noob
> question here, what/where is the html_desc feature?

No idea - I've never heard of it before. I just read the code and found that the behaviour gated on it (and you should have spotted that too, perhaps :-). Dylan: can you shed any light on what this is, and how to turn it on/off?

Gerv

Comment 24

[Justin B]

Ah, my mistake. From reading the past comments I assumed that moving to html_light was already a recommended solution that just needed implementing (I'm a new contributor and just trying to be helpful by working on bug list for first-timers. :) )

Ok, so looks like this feature is enabled if checksetup.pl finds the needed Perl modules (HTML::Parser and HTML::Scrubber). I'll take another look...

Dylan: Am I on the right track here? Is there an easier way of disabling the html_desc feature that doesn't involve hiding/deleting/renaming these two Perl modules?

Comment 25

[Dylan Hardison [:dylan] (he/him)]

(In reply to Justin B from comment #24)
> Ah, my mistake. From reading the past comments I assumed that moving to
> html_light was already a recommended solution that just needed implementing
> (I'm a new contributor and just trying to be helpful by working on bug list
> for first-timers. :) )
> 
> Ok, so looks like this feature is enabled if checksetup.pl finds the needed
> Perl modules (HTML::Parser and HTML::Scrubber). I'll take another look...
> 
> Dylan: Am I on the right track here? Is there an easier way of disabling the
> html_desc feature that doesn't involve hiding/deleting/renaming these two
> Perl modules?

As a hack to test this patch, add:

return 0 if $feature_name eq 'html_desc';

to https://github.com/bugzilla/bugzilla/blob/master/Bugzilla.pm#L274

Comment 26

[Dylan Hardison [:dylan] (he/him)]

Meanwhile, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1330048 as a more general solution to this, as I would accept such a solution as well.

Comment 27

[Dylan Hardison [:dylan] (he/him)]

Since you're the patch author, it makes sense to assign this bug to you.

Comment 28

[Dylan Hardison [:dylan] (he/him)]

If this course of action proves to be too difficult, we could consider making "html_light" a non-optional feature for future releases. Let me know how you're doing on this.

Thanks!

Comment 29

[Justin B]

So after additional testing, using FILTER html_light on a maintainer like: 

Darth Vader <[e-mail]>

Without html_desc: Darth Vader <[e-mail]>
With html_desc:    Darth Vader


html_desc uses HTML::Scrubber which looks for allowed html tags, and I don't see an obvious way to allow an email address (correct me if i'm wrong)

So in the html_light_quote function, what if we revised the logic where it checks for the 'html_desc' feature:

if (!Bugzilla->feature('html_desc') 
    || Bugzilla->params->{'maintainer'} =~ m/<[a-z1-9\.-]+@[a-z1-9\.-]+>/ ) {

this will fall back to the regex-based filtering and allow 'name <email>' formatting

Or... does this warrant it's own filter?

Comment 30

[Dylan Hardison [:dylan] (he/him)]

If it looks like an email, escape the <'s. No sense doing crazy gymnastics.

Comment 31

[Gervase Markham [:gerv]]

Justin: back to you :-)

Gerv

Comment 32

[Firefox Bug Husbandry Bot]

Removing good-first-bug keyword because team does not have bandwidth to mentor at the moment.