Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSRF protection problem is back #26

Closed
zacharytamas opened this issue Aug 21, 2011 · 22 comments
Closed

CSRF protection problem is back #26

zacharytamas opened this issue Aug 21, 2011 · 22 comments

Comments

@zacharytamas
Copy link

I am developing using the dev version of Django in which your middleware fix for the CSRF no longer works. You are not allowed to modify the method attribute of the request.

However, I was reading the source of the CSRF middleware thinking I would subclass it and override the behavior if Facebook was present. Perhaps in the FB middleware add an attribute to the request which my subclassed and overridden process_view() would detect.

But this wasn't even necessary. In the CSRF middleware the first logic is to look for csrf_processing_done on the request, if it's True it stops right there and doesn't engage the CSRF protection. In your middleware you can add this to the request yourself like this:

if request.method == 'POST' and 'signed_request' in request.POST:
request.csrf_processing_done = True

@zacharytamas
Copy link
Author

Accidentally closed issue, sorry.

@zacharytamas
Copy link
Author

Oh god this is just embarrassing, it's 3am, I should sleep. Here, I fixed it. Be kind. =)

@jgorset
Copy link
Owner

jgorset commented Aug 21, 2011

Haha. Don't worry about it, @zacharytamas.

I am developing using the dev version of Django in which your middleware fix for the CSRF no longer works. You are not allowed to modify the method attribute of the request.

I'm unable to reproduce or derive this from the trunk... am I missing something?

@jgorset jgorset closed this as completed Aug 21, 2011
@jgorset jgorset reopened this Aug 21, 2011
@zacharytamas
Copy link
Author

I wasn't able to derive it either except on three different setups I got a
500 for not being able to set the attribute. From reading the csrf
middleware It appears that using the method I ended up using is actually
what they had in mind. So that you could handle the csrf processing on your
own in certain cases and let their middleware know you already took care of
it.

@jgorset
Copy link
Owner

jgorset commented Aug 21, 2011

I wasn't able to derive it either except on three different setups I got a
500 for not being able to set the attribute.

I see. That's odd - I'm able to set it just fine in Django v1.4.0 alpha. Please provide the exception (w/stack trace) that produced the HTTP 500 Internal Server Error response.

From reading the csrf
middleware It appears that using the method I ended up using is actually
what they had in mind. So that you could handle the csrf processing on your
own in certain cases and let their middleware know you already took care of
it.

You're right that this would remedy this particular issue, but the request method isn't overriden so much to get around CSRF protection as to conform the Facebook platform to the HTTP standard and ensure that requests aren't randomly HTTP POST.

@tijs
Copy link

tijs commented Aug 24, 2011

I'm noticing that when people visit one of my apps pages with a form directly from an email or wall post (i.e. apps.facebook.com/myapp/somepage/someid) they end up at the correct page but the facebook POST triggers the Django form validation and they end up seeing form errors because of the empty fields...

Should your CSRF workaround take care of that as well, and is that thus broken too or should i work around that problem in some other way. Can i recognize the facebook POST easily and just do a pass in those cases for instance?

@tijs
Copy link

tijs commented Aug 24, 2011

I tried doing this in the view, as in your own code:

    if request.method == 'POST':
        if 'signed_request' in request.POST:
            # skip the form.is_valid() stuff and just show the view

But that has nog effect, i guess the signed_request bit is no longer in the request after the middleware had it's way with it...?

@jgorset
Copy link
Owner

jgorset commented Aug 24, 2011

I'm noticing that when people visit one of my apps pages with a form directly from an email or wall post (i.e. apps.facebook.com/myapp/somepage/someid) they end up at the correct page but the facebook POST triggers the Django form validation and they end up seeing form errors because of the empty fields...

Fandjango overrides the method of POST requests that contain a signed request to GET, but leaves it otherwise intact:

# fandjango/middleware.py, line 39

if request.method == 'POST' and 'signed_request' in request.POST:
    request.method = 'GET'

Which versions of Django and Fandjango are you using, @Tini?

@mbaechtold
Copy link

@jgorset: I'm using Django 1.2.5 and a very old version of Fandjango (3.0.1). If I remember correctly, I had to create a view with @csrf_exempt that simply redirected to my main view. A coworker is actually working on a project with Django 1.3 and the latest version of Fandjango; so far no problems. To be honest, I don't really understand what this issue is about...

@jgorset
Copy link
Owner

jgorset commented Aug 24, 2011

I'm terribly sorry, @Tini, I didn't mean to mention you. I intended to mention @tijs.

@mbaechtold
Copy link

@jgorset: haha, never mind, no prob.

@tijs
Copy link

tijs commented Aug 24, 2011

@jgorset fandjango 3.7.2 and Django 1.3, any idea what else i could try?

@jgorset
Copy link
Owner

jgorset commented Aug 24, 2011

[...] any idea what else i could try?

Hmms... not really... this is very strange. Could you please verify that the Facebook Platform's request to your application contains a signed_request in its body? It would seem likely that it doesn't, since Fandjango isn't overriding the request method and the workaround you demonstrated can't seem to find it.

@tijs
Copy link

tijs commented Aug 24, 2011

Yeah it's in there but the request method -is- changed to GET which is why my setup was failing:

    if 'signed_request' in request.POST:
        request.POST = None

    form = PredictionForm(request.POST or None, topic=topic)

This now works while this:

if request.method == 'POST' and 'signed_request' in request.POST:
    request.POST = None

Was never reached...

So i guess the fandjango CSRF fix still works like a charm this is just an extra gotcha when working with facebook + django... :)

@jgorset
Copy link
Owner

jgorset commented Aug 24, 2011

I'm not sure I understand... if the request method is indeed overriden from POST to GET, why would your form be validated?

I'm noticing that when people visit one of my apps pages with a form directly from an email or wall post (i.e. apps.facebook.com/myapp/somepage/someid) they end up at the correct page but the facebook POST triggers the Django form validation and they end up seeing form errors because of the empty fields...

@tijs
Copy link

tijs commented Aug 24, 2011

It wasn't, that was my mistake, it was this shortcut of initializing the form in a one-liner that was tripping me up:

 form = PredictionForm(request.POST or None, topic=topic)

Since request.POST was not None (it had the signed_request thingie) the form was initialized with empty default values. Which triggered the form error. I picked up that shortcut recently which is why i wasn't looking there...

@jgorset
Copy link
Owner

jgorset commented Aug 24, 2011

I see. Hmm. That makes a pretty strong case for discarding the signed request!

@tijs
Copy link

tijs commented Aug 24, 2011

I guess so, it's not actually -needed- anywhere right?

@jgorset
Copy link
Owner

jgorset commented Aug 24, 2011

I guess so, it's not actually -needed- anywhere right?

I think not, so I fixed it; b01282c.

@tijs
Copy link

tijs commented Aug 24, 2011

Great, made my life a bit easier again :)

@jgorset
Copy link
Owner

jgorset commented Aug 29, 2011

I'm going to close this issue pending a response from @zacharytamas.

@jgorset jgorset closed this as completed Aug 29, 2011
@zacharytamas
Copy link
Author

My issue seemed to be from an old Apache configuration that was using mod_python instead of mod_wsgi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants