"Unterminated string constant" errors from people with double-quotes in their name

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
Creating/Changing Bugs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Simon King, Assigned: dkl)

Tracking

Bugzilla 4.4
Bug Flags:
approval +

Details

(Whiteboard: [bmo4.0-resolved])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 1.1.4322; .NET CLR 2.0.50727; MS-RTC LM 8; .NET CLR 3.0.04506.30; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; InfoPath.2; .NET4.0C; .NET4.0E)

Steps to reproduce:

1. Enable javascript error notifications in IE (Tools -> Internet Options -> Advanced -> Browsing -> Display a notification about every script error)

2. Visit a bug which has been commented on by a user who has quotes in their display name (such as comment 12 on bug 1000)



Actual results:

I received a Javascript error:

Message: Unterminated string constant
Line: 1296
Char: 43
Code: 0
URI: https://bugzilla.mozilla.org/show_bug.cgi?id=1000

(Note that this error is then followed by a second error which is due to bug 683464)


Expected results:

The page should have displayed without error.

The lines around which the error occurs looks like this:

        <script type="text/javascript"><!--
          addReplyLink(12, 1457413, 'Patrick Xia (\"Octalc0de\")');
          addCollapseLink(12); // -->
        </script>

I have no idea at all why IE objects to this - I would have thought that escaping the double-quotes would be all that is necessary, but apparently not...

Experimenting with the IE developer tools, it seems to be quite happy with this:

    s = 'Patrick Xia (\"Octalc0de\")';

but not this:

    addReplyLink(12, 1457413, 'Patrick Xia ("Octalc0de")');

The bug was introduced in response to bug 653634, which hasn't yet been merged into the main bugzilla tree.
it's failing inside addReplyLink(), however IE is reporting the line number incorrectly.

i suspect we should be using &quot; for the " characters..

-        var escaped_name = name.replace("\'", "\\\'", "g");
+        var escaped_name = name.replace(/'/g, "\\'").replace(/"/g, '&quot;');

IE doesn't support the 3 argument version of replace, so switching to /'/g is also required.


i'll do some more testing before wrapping this into a patch for review.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

6 years ago
I'd just reached the same conclusion and got a mid-air collision when submitting :-)

FWIW, the patch on bug 653634 has moved on since this was incorporated on bmo, and it no longer uses document.write. It might therefore be safer than the current implementation.

(Sorry I don't have a patch for you this time - I'm not actually running this code on my local installation, so can't easily test any changes)
(Assignee)

Comment 3

6 years ago
(In reply to Simon King from comment #2)
> FWIW, the patch on bug 653634 has moved on since this was incorporated on
> bmo, and it no longer uses document.write. It might therefore be safer than
> the current implementation.

This is correct. The new patch does away with document.write entirely so the escaping may become a non-issue. When the patch lands upstream, I will port it over to BMO as well.

dkl
i get the same error message using firefox when clicking on the reply link.  firefox also reports the wrong line number (line 1).
Assignee: nobody → glob
Summary: Unterminated string constant error in IE when viewing comments from people with quotes in their name → "Unterminated string constant" errors from people with double-quotes in their name
Created attachment 557455 [details] [diff] [review]
patch v1

tested in firefox, IE8, chrome, opera & safari.
Attachment #557455 - Flags: review?(dkl)
(Assignee)

Comment 6

6 years ago
Comment on attachment 557455 [details] [diff] [review]
patch v1

bug 653634 was just committed for trunk and I ported the patch to bmo/4.0 as well. So this patch is no longer valid. Please test again on bugzilla-stage-tip.mozilla.org to verify this is still an issue with IE, etc.

dkl
Attachment #557455 - Flags: review?(dkl) → review-
(In reply to David Lawrence [:dkl] from comment #6)
> Please test again on bugzilla-stage-tip.mozilla.org to verify this is still an
> issue with IE, etc.

this problem still exists on bugzilla-stage-tip; you get an 'unterminated string constant' error from all browsers when clicking on the reply link to comment 12 of bug 1000.
(Assignee)

Comment 8

6 years ago
Moving this to upstream as this affects the patch I just committed and will need to be fixed there as well.

dkl
Assignee: glob → create-and-change
Status: NEW → ASSIGNED
Component: General → Creating/Changing Bugs
Product: bugzilla.mozilla.org → Bugzilla
QA Contact: general → default-qa
Version: Current → 4.3
(Assignee)

Comment 9

6 years ago
Created attachment 557858 [details] [diff] [review]
Patch to fix error when name contains double quotes (v1)

This fixes the issue for Firefox. Please verify it also fixes for IE.

dkl
Assignee: create-and-change → dkl
Attachment #557455 - Attachment is obsolete: true
Attachment #557858 - Flags: review?(glob)

Comment 10

6 years ago
Comment on attachment 557858 [details] [diff] [review]
Patch to fix error when name contains double quotes (v1)

>+                [% replyto_name = comment.author.name || comment.author.nick %]                
>+                [% replyto_name = replyto_name.replace('\"', '&quot;') %]
>+                onclick="replyToComment('[% count %]', '[% comment.id %]', '[% replyto_name FILTER js %]'); return false;"

Instead of this, why not simply call [% comment.author.name || comment.author.nick FILTER html FILTER js %]?
Comment on attachment 557858 [details] [diff] [review]
Patch to fix error when name contains double quotes (v1)

(In reply to Frédéric Buclin from comment #10)
> Instead of this, why not simply call [% comment.author.name ||
> comment.author.nick FILTER html FILTER js %]?

indeed; Frédéric's suggestion works and is cleaner.
Attachment #557858 - Flags: review?(glob) → review-
(Assignee)

Comment 12

6 years ago
(In reply to Byron Jones ‹:glob› from comment #11)
> Comment on attachment 557858 [details] [diff] [review]
> Patch to fix error when name contains double quotes (v1)
> 
> (In reply to Frédéric Buclin from comment #10)
> > Instead of this, why not simply call [% comment.author.name ||
> > comment.author.nick FILTER html FILTER js %]?
> 
> indeed; Frédéric's suggestion works and is cleaner.

That is what I was doing originally. 

https://bzr.mozilla.org/bugzilla/trunk/revision/7948

+               onclick="replyToComment('[% count %]', '[% comment.id %]', '[% comment.author.name || comment.author.nick FILTER js %]'); return false;"

The reporter was saying that the change that was going upstream on trunk was originally causing the problem where IE was treating the the escaped quotes incorrectly. 

The FILTER js just converts the " to \" instead of &quot; which I thought was the problem so I updated the patch to do a replace() in the template of \" to &quot;

For my education, you are saying that my original commit does in fact work on IE/Firefox? I did see Firefox fail with my original patch which prompted me to post a workaround here. Or are you saying this patch here on this bug, along with Frederics suggested change (which I am doing replyto_name FILTER js which does the same), is the proper fix?

dkl
(In reply to David Lawrence [:dkl] from comment #12)
> > (In reply to Frédéric Buclin from comment #10)
> > > Instead of this, why not simply call [% comment.author.name ||
> > > comment.author.nick FILTER html FILTER js %]?
> 
> That is what I was doing originally. 
> 
> +               onclick="replyToComment('[% count %]', '[% comment.id %]',
> '[% comment.author.name || comment.author.nick FILTER js %]'); return false;"

the fix involves using both html and js filters, your original patch only uses js.

> Or are you saying this patch here on this bug, along with Frederics suggested change
> (which I am doing replyto_name FILTER js which does the same), is the proper fix?

only frédéric's fix is required.
(Assignee)

Comment 14

6 years ago
Created attachment 558390 [details] [diff] [review]
Patch to fix error when name contains double quotes (v2)

Ah, yep I see it now in Frederics comment where I missed the extra filter before I commented. Here is a patch with his change applied and it does in fact fix the issue for as well.

dkl
Attachment #557858 - Attachment is obsolete: true
Attachment #558390 - Flags: review?(glob)
Comment on attachment 558390 [details] [diff] [review]
Patch to fix error when name contains double quotes (v2)

r=glob
Attachment #558390 - Flags: review?(glob) → review+
Flags: approval?
(Assignee)

Updated

6 years ago
Whiteboard: [bmo4.0-resolved]

Updated

6 years ago
Depends on: 653634
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 5.0
(Assignee)

Comment 16

6 years ago
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified template/en/default/bug/comments.html.tmpl
Committed revision 7954
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.