Code

Opened 6 years ago

Closed 4 years ago

#9163 closed (fixed)

CsrfMiddleware needs to reset ETag header

Reported by: carljm Owned by: lukeplant
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 carljm 6 years ago.
remove previous ETags, documentation update
9163_alt_r9362.diff (1.1 KB) - added by carljm 5 years ago.
alternative patch for HttpResponse to remove ETag; incl test

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by carljm

remove previous ETags, documentation update

comment:1 Changed 6 years ago by carljm

  • 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 6 years ago by carljm

  • Has patch set

comment:3 Changed 6 years ago by carljm

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 6 years ago by carljm

  • 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 5 years ago by carljm

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 5 years ago by carljm

  • Triage Stage changed from Unreviewed to Design decision needed

Changed 5 years ago by carljm

alternative patch for HttpResponse to remove ETag; incl test

comment:7 Changed 5 years ago by lukeplant

  • Owner changed from nobody to lukeplant

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 5 years ago by lukeplant

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 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

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

Thanks to carljm for report and patch.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.