Opened 9 years ago

Closed 5 years ago

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

Download all attachments as: .zip

Change History (14)

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

  • Resolution set to invalid
  • Status changed from new to closed

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

  • Resolution invalid deleted
  • Status changed from closed to reopened

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

Patch

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

  • Has patch set

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

Improved patch (also escapes backslashes)

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

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 8 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

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

Accepting cos the ticket itself is a valid bug.

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

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

Changed 5 years ago by seanbrant

just removes newlines forcing everything into one long string.

comment:6 Changed 5 years ago by adamnelson

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 5 years ago by mtredinnick

  • Keywords easy-picking added
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 5 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 5 years ago by ramiro

  • Resolution set to fixed
  • Status changed from reopened to closed

(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 4 years ago by ramiro

(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