Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#9356 closed (invalid)

CSRFMiddleware should take session key from request.session

Reported by: bthomas Owned by: nobody
Component: Contrib apps Version: 1.0
Severity: Keywords:
Cc: jacob.kaplanmoss@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:


Since CSRFMiddleware depends on SessionMiddleware, it should use request.session to get the session key. That way, SessionMiddleware can ensure that there is a valid session ID present before the CSRF hash is generated.

Attachments (2)

csrf_fix.diff (1.0 KB) - added by bthomas 8 years ago.
Get session key from request.session
ticket_9356.2.diff (6.7 KB) - added by lukeplant 7 years ago.
Updated patch (but please see my comment)

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by bthomas

Get session key from request.session

comment:1 Changed 7 years ago by jacob

  • milestone set to 1.1
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago by jacob

  • Triage Stage changed from Accepted to Design decision needed

I actually think there's a good reason for this, but I can't recall why right now. So bumping back to DDN in case someone else can confirm if I'm right or mistaken.

comment:3 Changed 7 years ago by bthomas

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

The reasoning behind this was discussed in more detail on #9977, and the patch on that ticket includes the fix for this. Since #9977 is not going to be included in 1.1 (and possibly not ever), either myself or Luke need to make a better patch for this bug based on his improved fix in the #9977 patch.

Also needs tests.

comment:4 Changed 7 years ago by jacob

Ah, OK, I'd remembered the discussion but not the conclusion. Thanks.

comment:5 Changed 7 years ago by lukeplant

I've coded up a patch for this, extracting a bit of an early patch from #9977, but I'm not sure why we really need this. I'm particularly concerned because the patch would add a database hit (read) to every request (two for POST requests), which seems unavoidable unless we want to mess around with internals of the SessionStore object.

The middleware currently uses values from cookies (incoming and outgoing), hence avoiding database hits. If the incoming cookie is not a valid session, then:

  • Incoming checks: CsrfViewMiddleware will check the session cookie is accompanied by a CSRF token. The only time this will fail is if the user has manually messed around with the session cookie. Otherwise the middleware is harmlessly checking that an expired session is matched by an unneeded CSRF token.

  • Outgoing processing: as far as the session framework is concerned, there is no session. Any access to the session during the view function will create an outgoing cookie, which the CSRF middleware will notice, which will then do the right thing. If there is no session created, then CsrfResponseMiddleware will harmlessly insert a token that corresponds to the session cookie the user has (which is being ignored by Django).

What real world problem is this bug about? If there isn't one, I'm not inclined to add a database hit for every request to fix it.

The only other thing this patch would fix is the potential abuse of middleware to generate hashes of (SECRET_KEY + user supplied arbitrary string), which is much more easily addressed by adding some salt in the form of a prefix to the SECRET_KEY. A quick check of instances of SECRET_KEY suggests that this abuse is not currently exploitable in Django, so I don't think we need to address this right now (and when we do, this is not the only bit of code we should fix - we should add unique prefixes to every bit of code that uses SECRET_KEY).

Changed 7 years ago by lukeplant

Updated patch (but please see my comment)

comment:6 Changed 7 years ago by jacob

  • milestone changed from 1.1 to 1.2

I pretty much agree with Luke's comment, and so I'm going to push this to 1.2. There's a good chance that it'll then get closed as a dup of #9977, but that's a move for later.

comment:7 Changed 7 years ago by lukeplant

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

CsrfMiddleware no longer depends on the session, so this isn't valid any more.

comment:8 Changed 5 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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