Opened 8 years ago

Closed 7 years ago

#9163 closed (fixed)

CsrfMiddleware needs to reset ETag header

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

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 8 years ago.
remove previous ETags, documentation update
9163_alt_r9362.diff (1.1 KB) - added by Carl Meyer 8 years ago.
alternative patch for HttpResponse to remove ETag; incl test

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by Carl Meyer

Attachment: 9163_r9077.diff added

remove previous ETags, documentation update

comment:1 Changed 8 years ago by Carl Meyer

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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

comment:2 Changed 8 years ago by Carl Meyer

Has patch: set

comment:3 Changed 8 years ago by 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.

comment:4 Changed 8 years ago by Carl Meyer

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 Changed 8 years ago by 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.

comment:6 Changed 8 years ago by Carl Meyer

Triage Stage: UnreviewedDesign decision needed

Changed 8 years ago by Carl Meyer

Attachment: 9163_alt_r9362.diff added

alternative patch for HttpResponse to remove ETag; incl test

comment:7 Changed 7 years ago by Luke Plant

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 Changed 7 years ago by 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.

comment:9 Changed 7 years ago by Luke Plant

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