Opened 15 years ago

Closed 15 years ago

Last modified 13 years ago

#12151 closed (fixed)

UnicodeEncodeError from django.contrib.comments.views.utils line 41.

Reported by: Mikkel Høgh Owned by: nobody
Component: contrib.comments Version: dev
Severity: Keywords:
Cc: Mikkel Høgh Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I get a lot of server errors like this:

Traceback (most recent call last):

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/core/handlers/base.py", line 92, in get_response
   response = callback(request, *callback_args, **callback_kwargs)

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/contrib/comments/views/utils.py", line 41, in confirmed
   comment = comments.get_model().objects.get(pk=request.GET['c'])

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/manager.py", line 120, in get
   return self.get_query_set().get(*args, **kwargs)

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/query.py", line 299, in get
   clone = self.filter(*args, **kwargs)

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/query.py", line 498, in filter
   return self._filter_or_exclude(False, *args, **kwargs)

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/query.py", line 516, in _filter_or_exclude
   clone.query.add_q(Q(*args, **kwargs))

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/sql/query.py", line 1675, in add_q
   can_reuse=used_aliases)

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/sql/query.py", line 1614, in add_filter
   connector)

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/sql/where.py", line 56, in add
   obj, params = obj.process(lookup_type, value)

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/sql/where.py", line 269, in process
   params = self.field.get_db_prep_lookup(lookup_type, value)

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/fields/__init__.py", line 210, in get_db_prep_lookup
   return [self.get_db_prep_value(value)]

 File "/usr/storage/www/wsgiapps/virtualenvs/mikkel.hoegh.org/lib/python2.5/site-packages/django/db/models/fields/__init__.py", line 361, in get_db_prep_value
   return int(value)

UnicodeEncodeError: 'decimal' codec can't encode character u'\ufffd' in position 46: invalid decimal Unicode string

The cause of this problem is the request containing junk data:

GET:<QueryDict: {u'c': [u'4549                   \t              Result: \ufffd\ufffd\ufffd\ufffd\ufffd \ufffd\ufffd\ufffd']}>,
HTTP_REFERER': 'http://mikkel.hoegh.org/comments/posted/?c=4549+++++++++++++++++++%09++++++++++++++Result:+%ED%E5+%ED%E0%F8%EB%EE%F1%FC+%F4%EE%F0%EC%FB+%E4%EB%FF+%EE%F2%EF%F0%E0%E2%EA%E8;',
'QUERY_STRING': 'c=4549+++++++++++++++++++%09++++++++++++++Result:+%ED%E5+%ED%E0%F8%EB%EE%F1%FC+%F4%EE%F0%EC%FB+%E4%EB%FF+%EE%F2%EF%F0%E0%E2%EA%E8;',
'REQUEST_URI': '/comments/posted/?c=4549+++++++++++++++++++%09++++++++++++++Result:+%ED%E5+%ED%E0%F8%EB%EE%F1%FC+%F4%EE%F0%EC%FB+%E4%EB%FF+%EE%F2%EF%F0%E0%E2%EA%E8;',

I'm not quite sure what whoever making these request is trying to accomplish, but the server error should probably be avoidable with a bit more validation of the request parameters. Any ideas?

Attachments (5)

12151.diff (604 bytes ) - added by Thejaswi Puthraya 15 years ago.
git patch against the latest checkout
12151_tests.diff (1.1 KB ) - added by Thejaswi Puthraya 15 years ago.
tests to reproduce the problem
12151_1.diff (627 bytes ) - added by Thejaswi Puthraya 15 years ago.
patch to fix the issue
12151_2.diff (2.0 KB ) - added by Thejaswi Puthraya 15 years ago.
patch and tests
12151-kmt.diff (1.8 KB ) - added by Karen Tracey 15 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 by Thejaswi Puthraya, 15 years ago

milestone: 1.2
Version: 1.1SVN

I agree this is an error! There is an error (ValueError) if a string is given that cannot be converted into an int and a UnicodeEncodeError as mentioned by you. So I have generalized. If there is any exception, then we return an empty comment.

by Thejaswi Puthraya, 15 years ago

Attachment: 12151.diff added

git patch against the latest checkout

comment:2 by Mikkel Høgh, 15 years ago

Cc: Mikkel Høgh added

comment:3 by Russell Keith-Magee, 15 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

Blanket catching of "Exception" isn't a good idea. Also needs a test.

comment:4 by Russell Keith-Magee, 15 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Thejaswi Puthraya, 15 years ago

I believe I jumped the gun by trying to fix the problem by using the inputs provided in the ticket. After a deeper look, I am unable to reproduce the problem. How does the request.GETc get such an input?

@mikl,
Has the _get_pk_val() method been modified or is there any next method that you are providing? More info might be useful for fixing this problem.

comment:6 by Mikkel Høgh, 15 years ago

@thejaswi_puthraya: The GET parameters are set by the client, so malicious Internet denizens have a lot of fun with sending junk data to webservers to see if they'll find an exploit somewhere. So what I'm driving at here is that junk data in such parameters should not cause internal server errors, since junk request is part of the normal way of life on the web.

comment:7 by Thejaswi Puthraya, 15 years ago

@mikl, Thanks for the report. I was under the impression that it was being generated after the comment was posted. But now I understand that after a comment has been posted and redirected to the success page, some one is trying to resubmit with some garbled data.

I am uploading the tests for identifying the issue and the patch in another file for fixing it.

by Thejaswi Puthraya, 15 years ago

Attachment: 12151_tests.diff added

tests to reproduce the problem

by Thejaswi Puthraya, 15 years ago

Attachment: 12151_1.diff added

patch to fix the issue

comment:8 by Eric Holscher, 15 years ago

Needs tests: unset
Patch needs improvement: unset

Might be useful to have the tests and fix together in one patch, otherwise seems sane enough.

by Thejaswi Puthraya, 15 years ago

Attachment: 12151_2.diff added

patch and tests

comment:9 by Thejaswi Puthraya, 15 years ago

Eric, thanks for the comments. I have combined the tests and the patch. The tests now act as a regression and check if the exception is raised after the patch has been applied. I have a question regarding the test.

After the patch has been applied, we need to check that the exception is not raised. Is there a counterpart for assertRaises to help in such scenarios? I am handling it in the way below

try:
  self.assertRaises(ValueError, someCallable)
except AssertionError:
  pass

I couldn't think of anything better.

by Karen Tracey, 15 years ago

Attachment: 12151-kmt.diff added

comment:10 by Karen Tracey, 15 years ago

Resolution: fixed
Status: newclosed

(In [12681]) Fixed #12151: Ensured the comments code does not cause a server error when a request comes in for a comment specifying an invalid primary key value. Thanks thejaswi_puthraya.

comment:11 by Karen Tracey, 15 years ago

(In [12682]) [1.1.X] Fixed #12151: Ensured the comments code does not cause a server error when a request comes in for a comment specifying an invalid primary key value. Thanks thejaswi_puthraya.

r12681 from trunk.

in reply to:  9 comment:12 by Karen Tracey, 15 years ago

Replying to thejaswi_puthraya:

Is there a counterpart for assertRaises ...?

If you want to ensure code doesn't raise an exception, just execute it and proceed on in the test testing whatever might be appropriate. Any exception raised outside of an assertRaises will cause the test to fail. See the committed test, which fails before the code change and passes after. (I could not quite follow what you tried to do in the test in the patch, and it also did not fail before the proposed code change, so whatever it was was not effective.)

comment:13 by Leo Shklovskii, 15 years ago

Patch needs improvement: set
Resolution: fixed
Status: closedreopened

Regardless of the discussion in #11716, the patch for this bug should catch ValidationError as well since some of the fields in django throw that error on an invalid input and any field can be set as primary_key.`

comment:14 by Karen Tracey, 15 years ago

Resolution: fixed
Status: reopenedclosed

The reported problem here has been fixed. The failure case posted to #11716 pointed out a different place where the comments code needed to deal with invalid PK data submitted to it, and that has been done in r12800/r12801. It doesn't appear to be all that easy to cause a ValidationError to be raised in the place identified here, since the model involved here is the comments model itself, not the model comments are being attached to. You'd need to have a comments model with a non-autofield PK, I think. I don't even know if that is feasible. If it is, it deserves its own ticket, as it's a bit more of a complex situation than the relatively simple case of invalid submitted data covered by this ticket.

comment:15 by Jacob, 13 years ago

milestone: 1.2

Milestone 1.2 deleted

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