Django

Code

Ticket #9172 (closed: wontfix)

Opened 2 years ago

Last modified 2 years ago

CsrfMiddleware breaks django test client

Reported by: gruffudd Assigned to: nobody
Milestone: Component: Testing framework
Version: 1.0 Keywords:
Cc: davenaff Triage Stage: Unreviewed
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

The very excellent CsrfMiddleware? has one undesired side-effect: It stops the django test client from working. Is there a way of identifying that a POST has originated from django.test.client and, if so, disable the checking?

We're currently removing the CsrfMiddleware? from settings at runtime in our test harness, but that's a bit naughty!

Attachments

9172_r9084.diff (5.3 kB) - added by carljm on 09/22/08 23:48:05.
make test client bypass CsrfMiddleware?
django-trunk-csrf-test-client.patch (5.1 kB) - added by mbertheau on 01/22/09 10:39:33.
Updated patch for current trunk
django-1.0.2-csrf-test-client.patch (3.6 kB) - added by mbertheau on 01/22/09 10:39:55.
Updated patch for 1.0.2
django-trunk-remove-csrf-in-test-client.patch (0.9 kB) - added by mbertheau on 01/23/09 04:50:52.
Remove csrf middleware for tests automatically

Change History

09/22/08 11:50:20 changed by carljm

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

Removing CsrfMiddleware? from settings in your test harness is the best solution. Injecting test-client-specific checks into CsrfMiddleware? itself would be far uglier. I believe this should be closed as wontfix.

09/22/08 16:46:48 changed by russellm

I'm not convinced that this should be a wontfix - a solution of some kind is clearly needed. Ideally, your tests should be able to run on the same configuration as your production site - every difference between production and test environment is potential source for error. However, Carl is also correct - we don't really want to be inserting "If test" logic into the CSRF Middleware (we haven't had to do this anywhere else).

I can't say I have any brilliant ideas off the top of my head, so some deeper thought will be required. Suggestions welcome.

09/22/08 17:02:10 changed by carljm

I'd love to see a nicer solution; I banged my head on this one for quite some time.

You _can_ run your tests on the same configuration as your production site, but then they have to follow the same rules as your production site (which is as it should be, else you aren't really testing). If you're using CsrfMiddleware? that means your tests need to GET the form first, scrape the csrf token, and then use it for the POST (you actually only have to scrape once per session, since Django's Csrf token varies only with session id and SECRET_KEY). If you do that, everything will work fine.

Obviously, that's not a real attractive solution either. But if you want your tests to play by different rules, you should have to say so explicitly, and removing the middleware makes it explicit.

(follow-up: ↓ 5 ) 09/22/08 17:43:29 changed by SmileyChris

Couldn't the test client check for the middleware and fetch (and insert) the token if necessary?

(in reply to: ↑ 4 ) 09/22/08 23:29:47 changed by carljm

Replying to SmileyChris:

Couldn't the test client check for the middleware and fetch (and insert) the token if necessary?

Ok, I'm dumb. I went a little ways down that road, but not far enough. Turns out that's really simple to do; in fact, I just put together a patch that does it, along with accompanying test and doc update.

09/22/08 23:48:05 changed by carljm

  • attachment 9172_r9084.diff added.

make test client bypass CsrfMiddleware?

09/23/08 02:37:42 changed by carljm

  • has_patch set to 1.

09/25/08 00:08:01 changed by davenaff

  • cc set to davenaff.

(follow-up: ↓ 9 ) 09/25/08 00:10:41 changed by davenaff

Maybe I'm missing something, but I don't see the patch attached to this ticket - just a blank diff file.

(in reply to: ↑ 8 ) 09/25/08 00:48:12 changed by russellm

Replying to davenaff:

Maybe I'm missing something, but I don't see the patch attached to this ticket - just a blank diff file.

This is a common problem with Trac; some patches break the patch viewer. If you download the patch, it should be ok.

09/25/08 12:42:51 changed by carljm

I think it's patches that contain the "\ No newline at end of file" marker that seem to give Trac trouble - I could be wrong.

01/22/09 10:39:33 changed by mbertheau

  • attachment django-trunk-csrf-test-client.patch added.

Updated patch for current trunk

01/22/09 10:39:55 changed by mbertheau

  • attachment django-1.0.2-csrf-test-client.patch added.

Updated patch for 1.0.2

01/22/09 10:41:59 changed by mbertheau

I'm using the patch locally and it works fine. I also attached updated versions of this patch for trunk and 1.0.2. Trac still refuses to show the 1.0.2 patch, but it's there and working.

01/22/09 16:08:02 changed by lukeplant

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

The patch does almost the same thing as disabling the CsrfMiddleware? -- it simply makes the incoming POST data match what the middleware wants (whether or not the actual page that a user would see would match what the middleware wants). I cannot see the advantage of using this over disabling the middleware.

I see it this way: presumably you are trusting the middleware to do its job of stopping CSRF attacks in the absence of the token (this is basic stuff, and it has its own tests, after all). What you might not trust is whether the middleware is inserting the token correctly, which is a possibility, and will be more of a problem when/if an updated middleware is used (as is planned), which doesn't insert the token into outgoing pages, but requires a template tag in every form that needs it. In that case, you will want to test *exactly* what happens when a user accesses the page, and check you don't get 403's when you shouldn't. The only way to do this properly is to do browser simulation, using something like twill. If you are using that, it will automatically submit hidden fields, as it must, and will get through the middleware just fine.

In short, the only reason to not disable the middleware for testing is if you are going to go the whole way and do browser simulation, in which case you don't need a patch. Also, the patch introduces a dependency on a contrib app which isn't very nice.

01/23/09 04:50:01 changed by mbertheau

  • status changed from closed to reopened.
  • resolution deleted.

What about doing that automatically?

01/23/09 04:50:52 changed by mbertheau

  • attachment django-trunk-remove-csrf-in-test-client.patch added.

Remove csrf middleware for tests automatically

01/23/09 09:15:09 changed by lukeplant

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

It is simpler to just put the developer in control of the settings being used, and not mess with the settings in case they actually do want the middleware to be on and functioning (as in the case of full browser simulation). This special-casing doesn't add anything that cannot be achieved more simply. Also, in one sense, if you have the middleware enabled in your settings file, and you use the Django test client to test pages with sessions enabled, then your tests ought to fail -- the middleware is designed to stop the kind of access that is done by the test client. You can also manually monkey patch the middleware in/out for specific tests as required.


Add/Change #9172 (CsrfMiddleware breaks django test client)




Change Properties
Action