Opened 10 years ago

Closed 6 years ago

Last modified 6 years ago

#2986 closed defect (fixed)

JavaScript (dismissAddAnotherPopup) problem in TextField with newline characters

Reported by: anonymous Owned by: nobody
Component: contrib.admin Version: master
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: UI/UX:

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@…> 10 years ago.
Patch
new_javascript.patch (973 bytes) - added by Thomas Steinacher <tom@…> 10 years ago.
Improved patch (also escapes backslashes)
addslashes.patch (2.1 KB) - added by Thomas Steinacher <tom@…> 10 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 7 years ago.
just removes newlines forcing everything into one long string.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by Simon G. <dev@…>

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 Changed 10 years ago by Thomas Steinacher <tom@…>

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.

Changed 10 years ago by Thomas Steinacher <tom@…>

Attachment: javascript.patch added

Patch

comment:3 Changed 10 years ago by Thomas Steinacher <tom@…>

Has patch: set

Changed 10 years ago by Thomas Steinacher <tom@…>

Attachment: new_javascript.patch added

Improved patch (also escapes backslashes)

Changed 10 years ago by Thomas Steinacher <tom@…>

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 Changed 10 years ago by Chris Beaven

Triage Stage: UnreviewedAccepted

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

Accepting cos the ticket itself is a valid bug.

comment:5 Changed 10 years ago by Thomas Steinacher <tom@…>

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

Changed 7 years ago by Sean Brant

Attachment: remove_newlines.diff added

just removes newlines forcing everything into one long string.

comment:6 Changed 7 years ago by Adam Nelson

Triage Stage: AcceptedReady for checkin

comment:7 Changed 6 years ago by Malcolm Tredinnick

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 Changed 6 years ago by andrewwatts

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 Changed 6 years ago by Ramiro Morales

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 Changed 6 years ago by Ramiro Morales

(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