Opened 17 years ago

Closed 13 years ago

Last modified 13 years ago

#2986 closed defect (fixed)

JavaScript (dismissAddAnotherPopup) problem in TextField with newline characters

Reported by: anonymous Owned by: nobody
Component: contrib.admin Version: dev
Severity: normal Keywords: easy-picking, sprintSep2010
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Hi,

I created some models on Django (see below).

When I try to add a new Text, based on my model, from administration interface it works perfect.
But I cannot add a new Text from within Vulnerability model (a popup window opens). There is a JavaScript error.

Because there is a TextField in my model, JavaScript is broken if there is a newline character.

<script type="text/javascript">opener.dismissAddAnotherPopup(window, 5, "Test: testing
with newline
");</script>
# A sample from my models

class Text (models.Model):
	value = models.TextField()
	text_type = models.ForeignKey(TextType)
	language = models.ForeignKey(TextLanguage, default=2)
	def __str__(self):
		return '%s: %s' % (self.text_type, self.value)
	class Admin:
		pass

class Vulnerability (models.Model):
	title = models.CharField(maxlength=255)
	texts = models.ManyToManyField(Text, filter_interface=models.HORIZONTAL)


Attachments (4)

javascript.patch (950 bytes ) - added by Thomas Steinacher <tom@…> 17 years ago.
Patch
new_javascript.patch (973 bytes ) - added by Thomas Steinacher <tom@…> 17 years ago.
Improved patch (also escapes backslashes)
addslashes.patch (2.1 KB ) - added by Thomas Steinacher <tom@…> 17 years ago.
Even better patch. Patches addslashes to escape '\n' and uses it in the admin view. Maybe this should be moved into django.utils.javascript?
remove_newlines.diff (1.3 KB ) - added by Sean Brant 14 years ago.
just removes newlines forcing everything into one long string.

Download all attachments as: .zip

Change History (14)

comment:1 by Simon G. <dev@…>, 17 years ago

Resolution: invalid
Status: newclosed

I've marked this as invalid, since I can't confirm it. If you (or anyone else) is still having this issue, please reopen this ticket and provide some more info.

comment:2 by Thomas Steinacher <tom@…>, 17 years ago

Resolution: invalid
Status: closedreopened

I can confirm this. Consider a model with the following str method:

def __str__(self):
   return 'a\nb'

When trying to add such a model via the popup-window, I am getting the described syntax error.

by Thomas Steinacher <tom@…>, 17 years ago

Attachment: javascript.patch added

Patch

comment:3 by Thomas Steinacher <tom@…>, 17 years ago

Has patch: set

by Thomas Steinacher <tom@…>, 17 years ago

Attachment: new_javascript.patch added

Improved patch (also escapes backslashes)

by Thomas Steinacher <tom@…>, 17 years ago

Attachment: addslashes.patch added

Even better patch. Patches addslashes to escape '\n' and uses it in the admin view. Maybe this should be moved into django.utils.javascript?

comment:4 by Chris Beaven, 17 years ago

Triage Stage: UnreviewedAccepted

Maybe addslashes should just do repr(text)[1:-1]?

Accepting cos the ticket itself is a valid bug.

comment:5 by Thomas Steinacher <tom@…>, 17 years ago

Maybe yes. I noticed that I also have to escape '\r' to avoid JavaScript errors.

by Sean Brant, 14 years ago

Attachment: remove_newlines.diff added

just removes newlines forcing everything into one long string.

comment:6 by Adam Nelson, 14 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by Malcolm Tredinnick, 14 years ago

Keywords: easy-picking added
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I don't like changing the content like this (removing newlines). Partly because it leaves us open to other kinds of injection attacks as well. We have an escapejs filter in the template tags. Let's pull out the functionality from that into django/utils/html.py and call that to escape the string before putting it into Javascript. The reason I want to pull it out is so that Python code calls pure Python, not a template tag (the template tag can call the python function in django/utils/html.py as well). It's mostly just a namespacing issue, but it keeps things consistent with escape() and the escape filter.

comment:8 by andrewwatts, 14 years ago

Keywords: sprintSep2010 added

added patched django/utils/html.py, template tags and testcase from mtredinnicks suggestions. Available at http://github.com/andrewwatts/django/compare/django-2986 with patch available at http://github.com/andrewwatts/django/compare/django-2986.patch

comment:9 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: reopenedclosed

(In [15131]) Fixed #2986 -- Made the JavaScript code that drives related model instance addition in a popup window handle a model representation containing new lines. Also, moved the escapejs functionality yoo django.utils.html so it can be used from Python code. Thanks andrewwatts for the patch.

comment:10 by Ramiro Morales, 13 years ago

(In [15191]) [1.2.X] Fixed #2986 -- Made the JavaScript code that drives related model instance addition in a popup window handle a model representation containing new lines. Also, moved the escapejs functionality to django.utils.html so it can be used from Python code. Thanks andrewwatts for the patch.

Backport of [15131] from trunk

Note: See TracTickets for help on using tickets.
Back to Top