CsrfMiddleware needs to reset ETag header

[Carl Meyer]

If you enable CsrfMiddleware with ConditionalGetMiddleware (or CommonMiddleware + USE_ETAGS setting), spurious "Cross Site Request Forgery" 403 errors will be generated if a user logs out and then logs back in again.

The logout and re-login will cause the user to have a new session id, so their CSRF token will change. If the user then visits any form-containing page that they had previously visited in their old session, where the ETag header for the page was set before the CsrfMiddleware had a chance to add its token to the form, Django will incorrectly tell the browser that the page has not been modified (based on the ETag), even though the page should now contain a different CSRF token. Submission of the form on that page will then cause a 403 because the browser submits the old CSRF token with the form.

Ironically, this is _most_ likely to happen on a view with the never_cache decorator (such as the login view), because never_cache sets the ETag before CsrfMiddleware gets to mangle the form. It can also happen on any other form, if CsrfMiddleware is before ConditionalGetMiddleware/CommonMiddleware in the MIDDLEWARE_CLASSES (so it processes the response later).

The fix is two-fold: 1) CsrfMiddleware should always either recalculate or trash any ETag header it finds on a response where it adds a CSRF token, because that ETag's MD5 sum is no longer correct after the CSRF token is added, and 2) it should be documented that CsrfMiddleware should be placed after ConditionalGetMiddleware and CommonMiddleware in the MIDDLEWARE_CLASSES (otherwise the first fix will result in ETags not working or being double-calculated every time).

It might not be a bad idea to also add a warning in the middleware documentation that any middleware which touches the response content in a significant way needs to take the same precautions in order to work with ETags.

[Carl Meyer]

remove previous ETags, documentation update

[Carl Meyer]

This proposed patch just deletes any previously-set ETag rather than trying to recalculate it.

[Carl Meyer]

Actually, it might be best to fix this directly in the HttpResponse object, so it always kills the ETag header if its content is modified.

[Carl Meyer]

I'm also willing to write tests here, but I'm not sure what's the best thing to test: the specific interaction of CsrfMiddleware with ConditionalGetMiddleware (this will require some plumbing: setting up MIDDLEWARE_CLASSES, using the test client -- doesn't seem like the cleanest approach, or the most generic test)? The fact that CsrfMiddleware modifies response.content without touching the ETag? The fact that HttpResponse allows this to happen?

[Carl Meyer]

Any devs/triagers have opinions on the situation here? I'd be happy to get a fix for this ready for 1.0.1 if it's accepted as a bug and I can get some guidance on the best direction for the fix.

[Carl Meyer]

alternative patch for HttpResponse to remove ETag; incl test

[Luke Plant]

Hmm:

• Adding ETag specific code into core is bad
• Putting it into the CsrfMiddleware doesn't make much sense either, but since it created the mess, it ought to clean it up.

Personally, at the moment I'd go for the latter, and also for deprecating the response post-processing part of CsrfMiddleware in favour of the template tag described in CsrfProtection.

[Luke Plant]

Also, in proposed changes, the CSRF token will be session independent, so the spurious 403 changes shouldn't happen in this case. Fixing the ETag still needs to be done though, I think.

[Luke Plant]

(In [11650]) Fixed #9163 - CsrfMiddleware needs to reset ETag header

Thanks to carljm for report and patch.