BaseHttp server crash since last SVN update

Safestring is also an string ;)

Another patch, oneliner

(In [6780]) Fixed some type checks in the development server. Fixed #6063.

This fix only works for the BaseHTTPServer. It is not possible to use FastCGI with flup because there is also an assert inside the BaseFCGIServer write() method:

assert type(data) is str, 'write() argument must be string'

The fix should ensure that only str types are sent to the write() method.

No, the fix really shouldn't do that. SafeString? is a subclass of str and therefore perfectly safe to pass in. I'll make the same change to the fcgi server as I did to base server.

Oh, I see what you're saying: the assert is in flup itself. That's just broken. They should be using isinstance() so that valid subclassing is supported. :-(

This is going to unbelievably hard to fix in Django and we're not actually doing anything wrong. Needs some thought, but filing a bug report with flup would be a help.

Replying to mtredinnick:

Oh, I see what you're saying: the assert is in flup itself. That's just broken. They should be using isinstance() so that valid subclassing is supported. :-( This is going to unbelievably hard to fix in Django and we're not actually doing anything wrong. Needs some thought, but filing a bug report with flup would be a help.

Ok, I will send a change request to flup.

Allan Saddi gave the input that the usage of a string subclass is maybe not spec-compliant:
http://trac.saddi.com/flup/ticket/28

Can somebody please give me a short example of how to repeat this problem. I can't see how a SafeString instance is leaking out of HttpResponse.

To reproduce the problem it is enough to run trunk copy of django as fastcgi server and setup some flat page for example.

The change that causes the problem was introduced in [6778], [6777] works fine.

Did [6807] fix this by any chance? I still can't see from reading the code what is causing the problem and I'm not easily able to set up the whole fcgi stuff on my laptop at the moment. So if somebody could test that commit and see, I'd appreciate it.

For anybody wanting to help out here, the thing to work out is where the SafeData is coming from. HttpResponse.content should always have type "str", not SafeString, so where is the bad type leaking from? We aren't about to revert [6778], since it solves another problem, thus we need to plug the leak after we find it.

I can confirm this problem still exists in [6844].

In newforms-admin, I worked around the problem by replacing source:django/branches/newforms-admin/django/template/__init__.py with the version from [6777].

Replying to mtredinnick:

For anybody wanting to help out here, the thing to work out is where the SafeData is coming from. HttpResponse.content should always have type "str", not SafeString, so where is the bad type leaking from? We aren't about to revert [6778], since it solves another problem, thus we need to plug the leak after we find it.

Well, it looks like Template render() method returns SafeString which in turn is passed to HttpResponse constructor. The most sensible point of ensuring only strings are leaving Django realm seems to me the HttpResponse's constructor (the content should be rendered and escaped anyway, so there's no need to have SafeString here).

Naive patch on HttpResponse to convert content to str (just to visualize the idea)

Problem still exists. It is easy to reproduce with flup (manage.py runfcgi) --- any 404 triggers the error.

patch that is working for me

Hi, I've just posted a very simple patch that is working for me, please check if you get this error message somewhere else.

To be honest, I don't really see a problem with isinstance.

The isinstance doc says, that if given a type as a second option, it checks whether type matches:

>>> print isinstance.__doc__
isinstance(object, class-or-type-or-tuple) -> bool

Return whether an object is an instance of a class or of a subclass thereof.
With a type as second argument, return whether that is the object's type.
The form using a tuple, isinstance(x, (A, B, ...)), is a shortcut for
isinstance(x, A) or isinstance(x, B) or ... (etc.).

I belive that we do not have any problem with it, but asaddi from http://trac.saddi.com/flup/ticket/28 has :) so there always is a choice... convince him, include modified version of flup withing django tree (painful) or switch to different implementation :) IMHO easiest is to patch django...

The problem with isinstance() is that it doesn't test purely is something is the type, but also (correctly and in a natural Python fashion) allows subclasses. I will argue that the WSGI spec is completely broken by insisting on a str type in that place, rather than also permitting something that is a subclass of string. Which part of duck-typing don't they understand? However, that is beside the point and we should comply with the spec whenever possible.

Hear, hear! We can sit and complain about the standard, or we can easily fix it on our side. I agree with Malcolm totally --- let's fix it to comply to the spec. If the spec is updated, we'll update our code as well.

(In [6895]) Fixed #6063 -- Caught one place in HttpResponse that was not returning a str type (violation of WSGI spec). Thanks, janusz.harkot@gmail.com.