Admin site: Appropriateness of HTTP 500 when using non-allowed query strings
3point2
change list pages. The syntax follows queryset field lookups, for
example http://mysite.com/admin/myapp/mymodel/?field__exact=test.
Lookups that are not specified on the ModelAdmin's list_filter option
raise a SuspiciousOperation exception. This is done to prevent a
normal user from obtaining sensitive information (e.g. password
hashes).
In production use, I'm not sure that returning an HTTP code of 500
(internal server error) and emailing the server admins is an
appropriate response to a user manipulating the query string.
I think that 403 (forbidden) would be more accurate. In my mind, 500
suggests that something went wrong on the server, for example an
unexpected condition or exception in the application code. In this
situation, this is not the case. Django is deliberately forbidding a
user from accessing information for which they have not been
authorized.
Any thoughts?
Tai Lee
Cheers.
Tai.
Julien Phalip
I agree that no 500 response should be returned only by passing improper querystring parameters, unless those parameters match a custom list filter and that filter raises an unhandled exception. There is already a system in place to avoid this problem though, so if you've found an edge case could you please create a new ticket in Trac with a test case?
Thanks a lot,
Julien
3point2
500 for ANY field lookup on a related model that is not in the
list_filter option.
To test, simply create a model that has a ForeignKey to another model
and hook it up into the admin site. Don't include any list_filter
options. Then craft a valid query string on the change list page that
queries a field on the related model. You will get an HTTP 500.
For example:
myapp/models.py:
class MyModel(models.Model):
parent = ForeignKey(AnotherModel)
myapp/admin.py
admin.site.register(MyModel)
then visit http://localhost:8000/admin/myapp/mymodel/?parent__pk=1 and
you will get a SuspiciousOperation exception with a return code of
500.
Just to be clear, I'm not against the SuspiciousOperation exception
being raised. I just think it should use a more appropriate HTTP
status code, like 403.
On Apr 11, 11:47 am, Julien Phalip <jpha...@gmail.com> wrote:
> On Apr 10, 2012, at 4:34 AM, 3point2 wrote:
>
>
>
>
>
>
>
>
>
> > The admin site allows the use of certain query strings to filter
> > change list pages. The syntax follows queryset field lookups, for
> > raise a SuspiciousOperation exception. This is done to prevent a
> > normal user from obtaining sensitive information (e.g. password
> > hashes).
>
> > In production use, I'm not sure that returning an HTTP code of 500
> > (internal server error) and emailing the server admins is an
> > appropriate response to a user manipulating the query string.
>
> > I think that 403 (forbidden) would be more accurate. In my mind, 500
> > suggests that something went wrong on the server, for example an
> > unexpected condition or exception in the application code. In this
> > situation, this is not the case. Django is deliberately forbidding a
> > user from accessing information for which they have not been
> > authorized.
>
> > Any thoughts?
>
> I agree that no 500 response should be returned only by passing improper querystring parameters, unless those parameters match a custom list filter and that filter raises an unhandled exception. There is already a system in place to avoid this problem though, so if you've found an edge case could you please create a new ticket in Trac with a test case?
>
> Thanks a lot,
>
> Julien
>
> 1KViewDownload
Alex Ogier
If a query string references a foreign key that isn't in list_filter then it can hardly be a "valid query string". This isn't an authorization problem ("You lack permission to perform that operation"), it's a real fatal error ("You asked us for something we don't understand/support").
From a security standpoint, it's best to leak as little as possible about structure and relations when you reach undefined behavior. Special-casing this particular unhandled exception may leak information. Much better to play dumb and handle it like every other unhandled exception.
It's not a code path you should ever reach in normal use, only when someone is getting crafty with the admin URLs. A 400 response suggests that there is a fixable error somewhere, and there isn't.
Best,
Alex Ogier
--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Julien Phalip
> Julien, I'm not describing an edge case. Django will return an HTTP
> 500 for ANY field lookup on a related model that is not in the
> list_filter option.
>
> To test, simply create a model that has a ForeignKey to another model
> and hook it up into the admin site. Don't include any list_filter
> options. Then craft a valid query string on the change list page that
> queries a field on the related model. You will get an HTTP 500.
>
> For example:
>
> myapp/models.py:
>
> class MyModel(models.Model):
> parent = ForeignKey(AnotherModel)
>
> myapp/admin.py
>
> admin.site.register(MyModel)
>
> then visit http://localhost:8000/admin/myapp/mymodel/?parent__pk=1 and
> you will get a SuspiciousOperation exception with a return code of
> 500.
>
> Just to be clear, I'm not against the SuspiciousOperation exception
> being raised. I just think it should use a more appropriate HTTP
> status code, like 403.
Thanks for providing a test case. It kind of is an edge case as it requires some specific unusual conditions to be reproduced. But anyways, I've verified that this behavior has been in place in Django for a long time (at least since 1.2). Also it doesn't seem to be tested at all. I do agree that a 500 isn't appropriate here. However I don't think a 403 is appropriate either. Instead it should probably redirect you to the changelist with the querystring ?e=1, just like other unhandled exceptions.
Again, this behavior currently isn't tested, so more thoughts should probably be put in this. There's enough material to open a ticket though. Could you do that and provide a recap of this discussion?
Regards,
Julien