324359 - BiDi control characters can disfigure e-mail notifications

Status: ASSIGNED

(Bugzilla :: Email Notifications, defect)

Description

[Marc Schumann [:Wurblzap]]

This is a follow-up bug to bug 319331, comment 22.

Until we have a proper BiDi balancing function as proposed in bug 320273, the only way to protect against unbalanced BiDi is to remove BiDi switching characters. Bug 319331 does this for HTML; this bug here is to take care of e-mail notifications.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch

I wish I were sure passing the whole message through the filter was safe... I'm not, so I'm taking this less general approach.

Comment 2

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.0.1

Unrotted.

Comment 3

[Teemu Mannermaa (:wicked)]

Comment on attachment 267946 [details] [diff] [review]
Patch 1.0.1

It seems this doesn't address all emails sent, especially missing is new/changed bugmail and whinemail.

Would it be good idea to filter BiDi already from output of identity and such methods from Bugzilla::User objects? That way they wouldn't have to be filtered everywhere they are used.

Bonus points for changing filtering test to make sure necessary email fields get filtered in the future. Now it just skips all text templates.

One thing I noted while testing this. Even after bug 319331, testcase at
http://landfill.bugzilla.org/bugzilla-tip/show_bug.cgi?id=3197#c1 shows that date has switched around and part of real name has moved to righ side of date. I first thought we had regressed mailto BiDi fix but looks like that's a problem with the actual characters in real name that are meant to be displayed from right to left and thus browser do just that. I guess there's not much we can do about that with filtering? Do you have a bug open to address this with span tags or something?

>Index: Bugzilla/Template.pm
>===================================================================
>+            # Bug 324359
>+            txt => \&Bugzilla::Util::remove_bidi_disruptions,

There's already a txt filter that was introduced by bug 351994. I'm not sure if you can use it or create new one specifically for BiDi email filtering.

>Index: Bugzilla/Util.pm
>===================================================================
>+sub remove_bidi_disruptions {

Please add perldoc for this new sub.

>Index: Bugzilla/Template.pm
>-                if (Bugzilla->params->{'utf8'}) {
>Index: Bugzilla/Util.pm
>+    if (Param('utf8')) {

This change obviously doesn't work as Param() is no more.

>Index: template/en/default/request/email.txt.tmpl
>===================================================================
> To: [% to %]

Why this is not filtered?

>+Subject: [% flag.type.name FILTER txt %] [%+ subject_status %]: [[% terms.Bug %] [%+ bug.bug_id %]] [% bug.short_desc %]

Why bug description is not filtered?

>+[% user.identity FILTER txt %] has [% statuses.${flag.status} %] [%+ to_identity FILTER txt %] [%+ flag.type.name %]:

Why this flag type name is not filtered and one from above is?

Comment 4

[Frédéric Buclin]

The 2.22 branch is restricted to security bugs -> 3.0

Comment 5

[Frédéric Buclin]

The Bugzilla 3.0 branch is now locked to security bugs and dataloss fixes only. This bug doesn't fit into one of these two categories and is retargetted to 3.2 as part of a mass-change. To catch bugmails related to this mass-change, use lts081207 in your email client filter.

Comment 6

[Frédéric Buclin]

Bugzilla 3.2 is restricted to security bugs only. Moreover, this bug is either assigned to nobody or got no traction for several months now. Rather than retargetting it at each new release, I'm clearing the target milestone and the bug will be retargetted to some sensible release when someone starts fixing this bug for real (Bugzilla 3.8 more likely).