Opened 16 years ago

Closed 14 years ago

#9163 closed (fixed)

CsrfMiddleware needs to reset ETag header

Reported by: Carl Meyer Owned by: Luke Plant
Component: Contrib apps Version: dev
Severity: Keywords: csrf
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

9163_r9077.diff (1.5 KB ) - added by Carl Meyer 16 years ago.
remove previous ETags, documentation update
9163_alt_r9362.diff (1.1 KB ) - added by Carl Meyer 15 years ago.
alternative patch for HttpResponse to remove ETag; incl test

Download all attachments as: .zip

Change History (11)

by Carl Meyer, 16 years ago

Attachment: 9163_r9077.diff added

remove previous ETags, documentation update

comment:1 by Carl Meyer, 16 years ago

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

comment:2 by Carl Meyer, 16 years ago

Has patch: set

comment:3 by Carl Meyer, 16 years ago

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.

comment:4 by Carl Meyer, 16 years ago

Needs tests: set

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?

comment:5 by Carl Meyer, 15 years ago

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.

comment:6 by Carl Meyer, 15 years ago

Triage Stage: UnreviewedDesign decision needed

by Carl Meyer, 15 years ago

Attachment: 9163_alt_r9362.diff added

alternative patch for HttpResponse to remove ETag; incl test

comment:7 by Luke Plant, 15 years ago

Owner: changed from nobody to 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.

comment:8 by Luke Plant, 15 years ago

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.

comment:9 by Luke Plant, 14 years ago

Resolution: fixed
Status: newclosed

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

Thanks to carljm for report and patch.

Note: See TracTickets for help on using tickets.
Back to Top