1226123 - Email addresses with an apostrophe in them break the "Send Mail to Bug Assignees" button in buglists

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Luke Jones]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36 Edge/12.10240

Steps to reproduce:

When an bug assignee's email address contains an apostrophe, the generated onclick attribute in the email_addresses contains invalid javascript.


Actual results:

Button contains the following attribute: onclick="document.location='mailto:test'[e-mail]'"


Expected results:

The quote in the email address should be appropriately escaped.

Comment 1

[Luke Jones]

The following diff fixes this specific issue, however there are other buttons on the same page that probably require similar treatment:

diff --git a/template/en/default/list/list.html.tmpl b/template/en/default/list/list.html.tmpl
index 42af501..8999381 100644
--- a/template/en/default/list/list.html.tmpl
+++ b/template/en/default/list/list.html.tmpl
@@ -249,7 +249,7 @@

         [% IF bugowners && user.id %]
           <button type="button" id="email_assignees"
-                  onclick="document.location='mailto:[% bugowners FILTER html %]'">
+                  onclick="document.location='mailto:[% bugowners FILTER js %]'">
             Send Mail to [% terms.Bug %] Assignees</button>
         [% END %]

Comment 2

[Teemu Mannermaa (:wicked)]

Better question is.. why on earth we allow apostrophe in an email address? Is there any valid use for such a thing? I know that we don't have any good standards on email address charsets but we can use some common sense..

Comment 3

[Luke Jones]

Because:

a) Surnames (in particular Irish O'{name}) can contain apostrophes
b) RFC2822 states that such an email address is valid

Comment 4

[u430950]

If the fix is truly as simple as stated in comment 1, I would personally recommend this be included in a branch release for 4.4.x as well.

Comment 5

[Frédéric Buclin]

(In reply to Ryan Wilson [:f1sh] from comment #4)
> If the fix is truly as simple as stated in comment 1, I would personally
> recommend this be included in a branch release for 4.4.x as well.

4.4 is restricted to security fixes only, and email addresses with an apostrophe in them are very rare, hence the 'minor' severity.

When you have to play with filters, it's never "as simple as that". I suspect we need to write |FILTER js FILTER html|, but I would have to check both filters in details to be sure.

Comment 6

[bitout.cecelia]

Hi , I would like to work on this bug . But this is my first project and I can not find the files to remove this bug. So if you can please provide some more information on this?

Comment 7

[Simon Green]

(In reply to bitout.cecelia from comment #6)
> Hi , I would like to work on this bug . But this is my first project and I
> can not find the files to remove this bug. So if you can please provide some
> more information on this?

The files can be checked out from github: https://github.com/bugzilla/bugzilla

Once you have made the change, you can either submit a pull request in Github, or upload a patch to this bug.

-- 
Simon

Comment 8

[Ray Gottsch]

Greetings, If this bug is still available, I would like to work on this as a first project.

Comment 9

[Ray Gottsch]

I have been looking into this bug. I am pretty sure that the only place that needs this fix is the one stated above. Is there more files that people would like me to look through and test or should I submit that change?

Comment 10

[Ray Gottsch]

Attachment: list.html.tmpl

Edited list.html.tmpl adding the needed filter.

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8980901 [details] [diff] [review]
list.html.tmpl

Thanks for looking at this. But we will need you to submit a diff for the changes you made instead of the entire file itself.

https://wiki.mozilla.org/Bugzilla:Patches

and more info at

https://wiki.mozilla.org/Bugzilla:Developers

Comment 12

[Ray Gottsch]

Awesome! Thank you for the information. Sorry I missed that step. I will get back to you this week with a diff.

Comment 13

[Ray Gottsch]

Attachment: patch.diff

Reuploaded the patch as the requested diff.

Comment 14

[David Lawrence [:dkl]]

Comment on attachment 8982029 [details] [diff] [review]
patch.diff

Review of attachment 8982029 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/list/list.html.tmpl
@@ +251,4 @@
>          [% IF bugowners && user.id %]
>            <button type="button" id="email_assignees"
>                    onclick="document.location='mailto:[% bugowners FILTER html %]'">
> +                  onclick="document.location='mailto:[% bugowners FILTER js %]'">

Closer but this will not work as written as it is no longer valid HTML since the offending line is not being removed, just copied underneath. Also we can chain filters since we still want to FILTER html as well. So this change should be written as:

- onclick="document.location='mailto:[% bugowners FILTER html %]'">
+ onclick="document.location='mailto:[% bugowners FILTER html FILTER js %]'">

Please do this and test to make sure it works properly.

dkl

Comment 15

[Arshad Kazmi [:arshadkazmi42]]

Is this still open? Looks like the patch needs some changes here? Can I work on it?

Comment 16

[David Lawrence [:dkl]]

Sure. Feel free to create an updated patch and put up for review.

Comment 17

[Arshad Kazmi [:arshadkazmi42]]

where can I find code for this?

Is it part of mozilla central repo?

Comment 18

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #16)
> Sure. Feel free to create an updated patch and put up for review.

No it's in its own Bugzilla github repo.

https://github.com/bugzilla/bugzilla/blob/5.0/template/en/default/list/list.html.tmpl#L251

You will need to fork the repo and create a diff (patch) for the fix. You can then upload the patch to this bug and someone will review it for you.

dkl

Comment 19

[Arshad Kazmi [:arshadkazmi42]]

For building the project in my local do I have to follow these steps 

https://www.bugzilla.org/docs/4.2/en/html/installation.html

or there is some other way to for development local setup?

Comment 20

[Arshad Kazmi [:arshadkazmi42]]

also is there only 1 line change? as mentioned in Comment 14?
and is there any test written to validate this fix? or it needs to be validated manually?

Comment 21

[Arshad Kazmi [:arshadkazmi42]]

Hey David,
just checking in, did you got any change to check my above comments?

Comment 22

[Arshad Kazmi [:arshadkazmi42]]

created PR here

https://github.com/bugzilla/bugzilla/pull/69

Comment 23

[David Lawrence [:dkl]]

Merged to master branch of Bugzilla repo.