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

RESOLVED FIXED in Bugzilla 5.0

Status

()

--
minor
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: jonesld, Assigned: arshadkazmi42)

Tracking

({good-first-bug, outreachy})

Bugzilla 5.0
good-first-bug, outreachy

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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'email@address.com'"


Expected results:

The quote in the email address should be appropriately escaped.
(Reporter)

Comment 1

3 years ago
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 %]
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..
(Reporter)

Comment 3

3 years ago
Because:

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

Updated

3 years ago
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Incorrect filter used for email_addresses button → Email addresses with an apostrophe in them break the "Send Mail to Bug Assignees" button in buglists
Target Milestone: --- → Bugzilla 5.0
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

3 years ago
(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.
Keywords: good-first-bug, outreachy
Whiteboard: [good first bug]

Comment 6

2 years ago
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

2 years ago
(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

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

Comment 9

9 months ago
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

8 months ago
Created attachment 8980901 [details] [diff] [review]
list.html.tmpl

Edited list.html.tmpl adding the needed filter.
Attachment #8980901 - Flags: review?(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
Attachment #8980901 - Flags: review?(dkl) → review-

Comment 12

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

Comment 13

8 months ago
Created attachment 8982029 [details] [diff] [review]
patch.diff

Reuploaded the patch as the requested diff.
Attachment #8980901 - Attachment is obsolete: true
Attachment #8982029 - Flags: review?(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
Attachment #8982029 - Flags: review?(dkl) → review-
(Assignee)

Comment 15

4 months ago
Is this still open? Looks like the patch needs some changes here? Can I work on it?
Flags: needinfo?(dkl)
Sure. Feel free to create an updated patch and put up for review.
Assignee: query-and-buglist → arshadkazmi42
Flags: needinfo?(dkl)
(Assignee)

Comment 17

4 months ago
where can I find code for this?

Is it part of mozilla central repo?
Flags: needinfo?(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
Flags: needinfo?(dkl)
(Assignee)

Comment 19

4 months ago
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?
Flags: needinfo?(dkl)
(Assignee)

Comment 20

4 months ago
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?
(Assignee)

Comment 21

4 months ago
Hey David,
just checking in, did you got any change to check my above comments?
Merged to master branch of Bugzilla repo.
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Flags: needinfo?(dkl)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.