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

[Mikkel Høgh]

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?

[Thejaswi Puthraya]

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.

[Thejaswi Puthraya]

git patch against the latest checkout

[Russell Keith-Magee]

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

[Thejaswi Puthraya]

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.

[Mikkel Høgh]

@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.

[Thejaswi Puthraya]

@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.

[Thejaswi Puthraya]

tests to reproduce the problem

[Thejaswi Puthraya]

patch to fix the issue

[Eric Holscher]

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

[Thejaswi Puthraya]

patch and tests

[Thejaswi Puthraya]

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.

[Karen Tracey]

(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.

[Karen Tracey]

(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.

[Karen Tracey]

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.)

[Leo Shklovskii]

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.`

[Karen Tracey]

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.

[Jacob]

Milestone 1.2 deleted