458329 - Canned responses don't fill review textarea properly if they contain double quotes

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect)

Description

[Michael Morgan [:morgamic]]

Not sure if this was the problem, but something like this wouldn't fill the textarea:

In order to prevent conflicts with other addons that may be installed
by users, you need to wrap your "loose" variables and functions within
a JavaScript object. You can see an example of how to do this @
http://blogger.ziesemer.com/2007/10/respecting-javascript-global-namespace.html.
Thanks.

It would truncate on 'your'.

Comment 1

[Les Orchard [:lorchard]]

Need some more details here, like a URL or what canned responses where and in what textarea.

Comment 2

[u278084]

This is happening on another canned response. The work around seems to be using single quotes. But still a bug regardless:

STR:
1. View a pending queue from the editor's tools.
2. Select an addon to review
3. Hit 'Request Super Review'
4. From canned responses, choose remote xul or javascript.

At the bottom, you will see :
Option 2: Load it into an iframe marked type=

Comment 3

[Les Orchard [:lorchard]]

Attachment: patch (-p1) to properly escape quotes and line breaks in the select options for canned responses

Now that I've had a chance to see how the review process works, I see the problem here.  Select option values need some extra escaping to prevent breaking out of the value attribute.  Patch attached

Comment 4

[Les Orchard [:lorchard]]

Also, fwiw: The better place to do this is in the CakePHP selectTag helper itself, but I'm hesitant to do that and see it clobbered if we upgrade CakePHP.  Might be a good upstream fix to submit if it's not already handled.  Might be a good follow up bug

Comment 5

[Les Orchard [:lorchard]]

Comment on attachment 355605 [details] [diff] [review]
patch (-p1) to properly escape quotes and line breaks in the select options for canned responses

I can has review?

Comment 6

[Fred Wenzel [:wenzel]]

(In reply to comment #4)
> Also, fwiw: The better place to do this is in the CakePHP selectTag helper
> itself, but I'm hesitant to do that and see it clobbered if we upgrade CakePHP.

Hm, do you *want* to do it in the html helper? We subclass it anyway in views/helpers/addons_html.php. If you overwrote selectTag() there and added this, it may not hurt. Let me know what you think.

Comment 7

[Les Orchard [:lorchard]]

Attachment: patch (-p1) to properly escape select options in the AMO local version of HTML helper

Following wenzel's advice, I dropped a modified version of selectTag into the AMO local subclass of the Cake HTML helper.  On the upside, this should fix not only the current bug, but all other bugs like this one.  On the downside, it changes the selectTag helper everywhere - while I don't think it should break anything, it does have a wide reach

Comment 8

[Nick Nguyen [:osunick]]

Is this also the reason why "Option 2: Load it into an iframe marked type=" doesn't complete to "content"?

Comment 9

[Les Orchard [:lorchard]]

(In reply to comment #8)
> Is this also the reason why "Option 2: Load it into an iframe marked type="
> doesn't complete to "content"?

Yes, quotes aren't escaped, and so <option value=""> tags containing canned responses with quotes are broken in the HTML.

Comment 10

[Les Orchard [:lorchard]]

Comment on attachment 360174 [details] [diff] [review]
patch (-p1) to properly escape select options in the AMO local version of HTML helper

r? for rdoherty

Comment 11

[Ryan Doherty (:rdoherty)]

Comment on attachment 360174 [details] [diff] [review]
patch (-p1) to properly escape select options in the AMO local version of HTML helper

Looks good and works.

Comment 12

[Les Orchard [:lorchard]]

checked in as r22132

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

Attachment: Post-fix screenshot

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on preview.AMO