Ticket #1598 (closed defect: fixed)

Opened 7 months ago

Last modified 4 months ago

Passing non-ASCII characters as kwargs or nested params to method with @identity.require() produces decoding error when not logged in

Reported by: streawkceur Assigned to: anonymous
Priority: normal Milestone: 1.0.4
Component: Identity Version: 1.0.4b3
Severity: normal Keywords: identity utf utf-8 nested parameters
Cc:

Description

Suppose I have this (minimal) controller: 

class Root(controllers.RootController):
    @expose()
    def test(self, *args, **kwargs):
        return "bla"

    @expose()
    @identity.require(identity.not_anonymous())
    def test2(self, *args, **kwargs):
        return "bla2"

When I call /test?a=ä, all works as expected. But when I call the second handler /test2?a=ä, I get an internal server error. Note: This problem only seems to occur when the user actually is anonymous. 

500 Internal error
[..]
  File "[..]python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\filters\decodingfilter.py", line 50, in decode
    decodedParams[key] = value.decode(enc)
  File "[..]python25\lib\encodings\utf_8.py", line 16, in decode
    return codecs.utf_8_decode(input, errors, True)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe4' in position 0: ordinal not in range(128)

A possibly related problem occurs when calling the handler with "nested" query parameters: /test2?a.b=b 

500 Internal error

The server encountered an unexpected condition which prevented it from
fulfilling the request.

Traceback (most recent call last):
  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\_cphttptools.py", line 103, in _run
    applyFilters('before_main')
  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\filters\__init__.py", line 151, in applyFilters
    method()
  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\filters\decodingfilter.py", line 31, in before_main
    self.decode(enc)
  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\filters\decodingfilter.py", line 50, in decode
    decodedParams[key] = value.decode(enc)
AttributeError: 'dict' object has no attribute 'decode'

The full test project (actually, only the controllers.py has been modified after the quickstart) is attached. It has been tested (only) with TG 1.0.4b2.

Attachments

tg-id-utf-bug.zip (95.4 kB) - added by streawkceur on 10/30/07 15:59:23.
monkey-patch-decodingfilter.patch (0.6 kB) - added by Chris Arndt on 12/18/07 12:02:13.
Fix monkey-patching DecodeFilter? when no 'before_main' filters are present
monkey-decodingfilter.patch (0.9 kB) - added by Chris Arndt on 12/18/07 13:37:50.
Fix typo in MonkeyDecodingFilter?

Change History

10/30/07 15:59:23 changed by streawkceur

  • attachment tg-id-utf-bug.zip added.

11/18/07 03:57:48 changed by streawkceur

Adi has a patch that fixes the problem for my test case: 

IN turbogears\visit\api.py
  IN class VisitFilter(BaseFilter):
    IN def before_main(self):

adding at the end of before_main (line 165 for TG 1.0.0.2):

for k,v in cherrypy.request.params.iteritems():
    cherrypy.request.params[k]=v.encode('utf-8')

See http://groups.google.com/group/turbogears/browse_thread/thread/5bf6fdd53c87b07d

11/19/07 03:38:34 changed by streawkceur

Doesn't work with nested request params (i.e. foo.bar.baz) as they get converted into nested dicts, so the value v may be a dict that doesn't have a method named encode: AttributeError: 'dict' object has no attribute 'encode'.

To fix this fix, you have to do do a recursive conversion: 

    def encode_utf8(params):
        for k, v in params.items():
            if type(v) is dict:
                encode_utf8(v)
            else:
                params[k] = v.encode('utf-8') 
    
    encode_utf8(cherrypy.request.params)

Funnily, you'll then get an error from CP: 

  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\filters\decodingfilter.py", line 50, in decode
    decodedParams[key] = value.decode(enc)
AttributeError: 'dict' object has no attribute 'decode'

But I've got a feeling that the real problems are located in another place, deeper in the system. But unfortunately I don't know much about TG's/CP's internals. Maybe this is ultimately a bug in CP. But I really don't know.

11/25/07 17:26:46 changed by faide

  • status changed from new to closed.
  • resolution set to fixed.

Fixed in r3762. Dicts are not managed because managed or not they make CP fail.

11/26/07 09:36:29 changed by streawkceur

  • status changed from closed to reopened.
  • version changed from 1.0.4b1 to 1.0.4b2.
  • resolution deleted.

Actually, I still get an internal server error for my attached test project when invoking the URL http://localhost:8080/test2?a.b=a

I'm using turbogears-1.0.4b2dev_r3766 

500 Internal error

The server encountered an unexpected condition which prevented it from fulfilling the request.

Traceback (most recent call last):
  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\_cphttptools.py", line 103, in _run
    applyFilters('before_main')
  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\filters\__init__.py", line 151, in applyFilters
    method()
  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\filters\decodingfilter.py", line 31, in before_main
    self.decode(enc)
  File "c:\programme\python25\lib\site-packages\cherrypy-2.2.1-py2.5.egg\cherrypy\filters\decodingfilter.py", line 50, in decode
    decodedParams[key] = value.decode(enc)
AttributeError: 'dict' object has no attribute 'decode'

11/26/07 11:48:17 changed by faide

Hi

This is normal CP does not seem to support dict args in its decoding filters... we cannot change this from here. Do you use such args as a.b=<someaccentedarg> ?

If yes then we need to investigate deeper down into CherryPy?2's decodingfilters...

Florent.

11/26/07 13:21:57 changed by streawkceur

Yes, I do use these args as ToscaWidgets generates them.

ToscaWidgets generates form fields with dots in the names for nested widgets/form fields.

Simple example: Wrap some input fields (a,b,c) in a fieldset (fs). The input fields will have the names fs.a, fs.b, fs.c.

So with this bug you cannot use nested form elements in ToscaWidgets.

11/26/07 20:51:13 changed by faide

  • status changed from reopened to closed.
  • resolution set to fixed.

Fixed for real in #3771.

11/27/07 05:14:01 changed by roger.demetrescu

Correction: it is fixed in [3771]

12/17/07 06:57:02 changed by streawkceur

  • keywords changed from identity utf utf-8 to identity utf utf-8 nested parameters.
  • status changed from closed to reopened.
  • version changed from 1.0.4b2 to 1.0.4b3.
  • resolution deleted.

Still crashes in 3830. Example controller: 

from turbogears import controllers, expose, flash
from turbogears import identity, redirect

class Root(controllers.RootController):
    @expose()
    def index(self):
        return '<html><body><a href="/test_wo_ident?a.b=c">Won\'t crash</a>, <a href="/test_with_ident?a.b=c">Will crash</a></body></html>'
    
    @expose()
    def test_wo_ident(self, **kwargs):
        return "works"
    
    @expose()
    @identity.require(identity.not_anonymous())
    def test_with_ident(self, **kwargs):
        return "crashes"

...
  File "/usr/lib/python2.4/site-packages/CherryPy-2.2.1-py2.4.egg/cherrypy/filters/decodingfilter.py", line 50, in decode
    decodedParams[key] = value.decode(enc)
AttributeError: 'dict' object has no attribute 'decode'

12/17/07 09:32:03 changed by chrisz

I can reproduce this. The unit tests have a test function (identity.tests.test_identity.test_decode_filter) that does the same thing as you were doing. One difference was that the unit test tested only the case with a successful login, while you use a wrong login. I have now handled the case of a wrong login in r3831 as well. The strange thing is that this still did not fix your bug, though the unit test does the same thing and it works. Maybe a difference between the testing environment and the standard environment? Will look into that later. Maybe somebody else has an idea.

12/17/07 10:40:35 changed by chrisz

Looks like the infamous monkey_before_main function is not called when you don't start from the test environment.

12/17/07 14:48:48 changed by Chris Arndt

  • summary changed from Identity: Passing non-ASCII characters as kwargs to a handler with @identity.require(identity.not_anonymous()) produces internal server error to Passing non-ASCII characters as kwargs or nested params to method with @identity.require() produces decoding error when not logged in.

12/17/07 15:37:55 changed by streawkceur

This one (maybe obviously) also crashes when called with nested params: 

@expose()
def test(self, **kwargs):
    from turbogears.identity import IdentityFailure
    raise IdentityFailure, "boom"

So it's probably not the decorator but a raised identity exception that leads to the internal server error.

12/17/07 19:49:43 changed by faide

Hey Streawkceur,

This one we hope it is the good one :) please try this revision r3835. I tested with a test project the 3 exposed methods you proposed and all pass the test (manual test).

I did not have time to look at the automated tests yet to see why them fooled us.

Cheers, Florent.

PS: I keep this open because I want to make sure we have tests that validate something so we can guarantee we won't have regressions in the future.

12/18/07 12:01:25 changed by Chris Arndt

Hey Florent, great work! One issue though: your code for monky-patching DecodeFilter breaks when there is no 'before_main' key in cherrypy.filters._filterhooks. This happens when cherrypy.filters.init() isn't called. It should be called by cherrypy._cpengine.Engine.setup() but somehow this is not called (or called later?) when using turbogears.testutil.create_request(). The upshot is that your code works when running the CherryPy? server normally but breaks when you run the test suite in <your_package>/tests/test_controllers.py.

A simple patch is to use _filterhooks.get('before_main', []') for the for loop in turbogears.visit.api.start_extension (patch attached).

12/18/07 12:02:13 changed by Chris Arndt

  • attachment monkey-patch-decodingfilter.patch added.

Fix monkey-patching DecodeFilter? when no 'before_main' filters are present

12/18/07 12:36:35 changed by faide

Thanks Chris,

This may be the reason why the tests did work differently than the server...

Could you apply this yourself to the 1.0 branch ? I won't have access to SVN 'till 22h30 or so and I suspect I won't have much time for coding tonight.

Florent.

12/18/07 13:37:50 changed by Chris Arndt

  • attachment monkey-decodingfilter.patch added.

Fix typo in MonkeyDecodingFilter?

12/18/07 13:41:30 changed by Chris Arndt

New patch that fixes a typo in MonkeyDecodingFilter and incorporates the previous patch.

I'll submit this to SVN, but we still need better tests. For example, the typo wasn't caught by the existing tests, I only stunbled accross this while testing #242.

12/18/07 13:56:16 changed by Chris Arndt

Applied in r2826. The commit msg is somewhat garbled, because I didn't escape the backticks in the shell properly ;-(

12/19/07 02:59:24 changed by Felix.Schwarz

I think you meant r3836 actually.

12/19/07 04:16:47 changed by Chris Arndt

Oops, of course. Off by one error ;-)

01/20/08 09:28:17 changed by faide

  • status changed from reopened to closed.
  • resolution set to fixed.

Fixed in 1.0.4. Fixed also the tests to really test if the params are unicode. r3974 & r3979