Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#12151 closed (fixed)

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

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

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 5 years ago.
git patch against the latest checkout
12151_tests.diff (1.1 KB) - added by thejaswi_puthraya 4 years ago.
tests to reproduce the problem
12151_1.diff (627 bytes) - added by thejaswi_puthraya 4 years ago.
patch to fix the issue
12151_2.diff (2.0 KB) - added by thejaswi_puthraya 4 years ago.
patch and tests
12151-kmt.diff (1.8 KB) - added by kmtracey 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by thejaswi_puthraya

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.1 to SVN

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.

Changed 5 years ago by thejaswi_puthraya

git patch against the latest checkout

comment:2 Changed 4 years ago by mikl

  • Cc mikl added

comment:3 Changed 4 years ago by russellm

  • 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 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 4 years ago by 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.

comment:6 Changed 4 years ago by mikl

@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 Changed 4 years ago by 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.

Changed 4 years ago by thejaswi_puthraya

tests to reproduce the problem

Changed 4 years ago by thejaswi_puthraya

patch to fix the issue

comment:8 Changed 4 years ago by ericholscher

  • Needs tests unset
  • Patch needs improvement unset

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

Changed 4 years ago by thejaswi_puthraya

patch and tests

comment:9 follow-up: Changed 4 years ago by 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.

Changed 4 years ago by kmtracey

comment:10 Changed 4 years ago by kmtracey

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

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

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

comment:12 in reply to: ↑ 9 Changed 4 years ago by kmtracey

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 Changed 4 years ago by Leo

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened

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 Changed 4 years ago by kmtracey

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

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 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.