Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #2036 (closed defect: fixed)

Opened 11 years ago

Last modified 10 years ago

TG gives 500 server error instead of 403 or 404

Reported by: chrisz Owned by: Chris Arndt
Priority: normal Milestone: 1.1
Component: TurboGears Version: 1.0.7
Severity: minor Keywords: needs tests, JSON, NoApplicableMethods, SecureResource


In TG 1.x, when you have a JSON controller (e.g. for an autocomplete field) that is part of a SecureResource (i.e. a login protected part of your site), and you try to access that controller without being logged in, then you get a 500 server error instead of a 403 Forbidden error.

The reason is that TG tries to redirect the request to the login page, but the login page does not accept JSON.

Any ideas for a simple fix? I guess we have to either modify the login method or change IdentityFailure in tg.identity.exceptions so that it checks whether JSON was requested.

The other question is why TG raises 500 server error when no applicable controllers are found. Shouldn't it better return a 404 error? That could be achieved by catching NoApplicableMethods in the expose function in tg.controllers and raising cherrypy.HTTPError(404) in this case (maybe with a few precautions, because NoApplicableMethods could also stem from some nested function call).


Test2036-1.0.tar.bz2 Download (100.6 KB) - added by Chris Arndt 10 years ago.
Test2036-1.1.tar.bz2 Download (100.0 KB) - added by Chris Arndt 10 years ago.
New version of example project using HTTP basic authentication

Change History

comment:1 Changed 11 years ago by faide

Maybe have a login handler that accepts JSON and if JSON is requied only does one thing: return a predefined info like this pseudo-code:

    return {auth_error:"You be logged in before doing %s" % previous_url}

The ajax caller would just need to scan ALL answers at all time to make sure no auth_error is present in the payload and if so it should redirect (js reload or such...) the browser to the login page by itself using HTML instead of JSON.

comment:2 Changed 11 years ago by faide

thoughts ?

comment:3 Changed 11 years ago by chrisz

My current fix for tbe "500 instead of 403" problem is to add allow_json=True to the login controller, and then the following just before the method returns:

        if request.params.get('tg_format'):
            # JSON or anything different from HTML
            raise HTTPError(403)

A good fix for the "500 instead of 404" problem is still wanted.

comment:4 Changed 10 years ago by Chris Arndt

I don't think that IdentityFailure should act differently if JSON was requested. If you set a failure_url it might actually yield a URL which points to a controller which is able to handle JSON.

I think our suggested fix to the login method in comment 3 is sensible.

For the second problem, we should open a separate ticket.

comment:5 Changed 10 years ago by Chris Arndt

Actually, I can't reproduce the problem. I have build an example project that calls a sub-controller method via "loadJSONDoc", which has a expose('json') decorator and the sub-controller is protected via SecureResource. When I'm not loged in, I get a 401 response.

See the attached tarball Test2036-1.0.tar.bz2 with the example project. Start the project an click on the "Get time" link to test. Should give a JS alert box with the HTTP status code. Then dd a user and log in to see it working.

Changed 10 years ago by Chris Arndt

comment:6 Changed 10 years ago by chrisz

Thanks for the test package which gives me a 401 message, too.

It seems the problem only appears when you add the tg_format=json query parameter. You can reproduce this with the test package by setting href="/sub/time?tg_format=json" in the remote_link() call.

comment:7 Changed 10 years ago by Chris Arndt

Ok, I can now reproduce the 500 error too. Here's the exception traceback for reference:

2009-09-19 18:21:29,014 cherrypy.msg INFO HTTP: Page handler: <bound method Root.login of <test2036.controllers.Root object at 0x2294c10>>
Traceback (most recent call last):
  File "/Users/chris/lib/tg11svnpy25/lib/python2.5/site-packages/CherryPy-2.3.0-py2.5.egg/cherrypy/_cphttptools.py", line 121, in _run
  File "/Users/chris/lib/tg11svnpy25/lib/python2.5/site-packages/CherryPy-2.3.0-py2.5.egg/cherrypy/_cphttptools.py", line 264, in main
    body = page_handler(*virtual_path, **self.params)
  File "<string>", line 3, in login
  File "/Users/chris/work/turbogears/svn/branches/1.1/turbogears/controllers.py", line 356, in expose
    *args, **kw)
  File "<generated code>", line 0, in run_with_transaction
  File "build/bdist.macosx-10.3-fat/egg/peak/rules/core.py", line 153, in __call__
    return self.body(*args, **kw)
  File "/Users/chris/work/turbogears/svn/branches/1.1/turbogears/database.py", line 435, in sa_rwt
    retval = func(*args, **kw)
  File "/Users/chris/work/turbogears/svn/branches/1.1/turbogears/controllers.py", line 240, in _expose
  File "<generated code>", line 0, in _expose
  File "build/bdist.macosx-10.3-fat/egg/peak/rules/core.py", line 214, in __call__
    raise self.__class__(*self.args+(args,kw))  # XXX
NoApplicableMethods: ((<function login at 0x228caf0>, 'application/json', False, 
  <test2036.controllers.Root object at 0x2294c10>), {'tg_format': u'json', 'forward_url': None})
Request Headers:
  COOKIE: tg-visit=4d9464b318a5cb1389d99b71351a1976ca51cdea
  ACCEPT-CHARSET: ISO-8859-1,utf-8;q=0.7,*;q=0.7
  USER-AGENT: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; de; rv: Gecko/20090824 Firefox/3.5.3
  CONNECTION: keep-alive
  REFERER: http://localhost:8080/
  HOST: localhost:8080
  ACCEPT: application/json
  Remote-Addr: ::1
  ACCEPT-LANGUAGE: de-de,de;q=0.8,en;q=0.5,en-us;q=0.3
  ACCEPT-ENCODING: gzip,deflate
::1 - - [19/Sep/2009:16:21:29 +0000] "GET /sub/time?tg_format=json HTTP/1.1" 500 2238
 "http://localhost:8080/" "Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; de; 
 rv: Gecko/20090824 Firefox/3.5.3"

comment:8 Changed 10 years ago by Chris Arndt

As to the question of catching NoApplicableMethods in expose: is it possible with PEAK-Rules to check which implementation of a generic method will be called without actually calling it? So the NoApplicable Methods exception could be caught in the lookup phase and not when calling the function?

I feel this is just another instance confirming my dislike for generic methods. They're just too complex and a pita to debug...

comment:9 Changed 10 years ago by chrisz

For me, the main problem is that they are not very well maintained and documented. I hope we will have something like that in the standard lib one day (see PEP3124).

Maybe you can check without calling, but that would not be performant. Can you have a look at r6660? This should solve the second problem with 500 instead of 404.

The first problem (500 instead of 401/403) still needs to be solved (we now have 404 instead of 401/403).

comment:10 Changed 10 years ago by chrisz

I have expanded this patch now in r6661 so that it will not raise "not found" (404), but the error status that has already been set by the identity provider (401). This solves the first problem.

comment:11 Changed 10 years ago by Chris Arndt

Could you add a source code comment which explains what is happening in that new except clause? The code isn't very self-explanatory...

comment:12 Changed 10 years ago by chrisz

Ok, done in r6662.

comment:13 Changed 10 years ago by chrisz

Btw, I think the error code 401 used by TG 1.1 is in fact even more appropriate than the 403 code originally suggested in this ticket.

comment:14 Changed 10 years ago by Chris Arndt

Hmm, not sure about the 401 error code. I just sent a mail to to trunk ML about this topic.

comment:15 Changed 10 years ago by Chris Arndt

  • Keywords needs tests, added

Ok, I just added support to IdentityFailure for initiating HTTP Basic authentication in r6665. It will now only return a 401 status (and an appropriate WWW-Authenticate header) if the config setting identity.http_basic_auth is True and status 403 otherwise.

Changed 10 years ago by Chris Arndt

New version of example project using HTTP basic authentication

comment:16 Changed 10 years ago by chrisz

I have now added a test for the original issue in r6671 and adapted the tests to your change in r6665.

comment:17 Changed 10 years ago by Chris Arndt

  • Owner changed from faide to Chris Arndt
  • Status changed from new to assigned

Oops! Thanks!

comment:18 Changed 10 years ago by Chris Arndt

  • Status changed from assigned to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.