CSRFMiddleware should take session key from request.session

[Bob Thomas]

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.

[Bob Thomas]

Get session key from request.session

[Jacob]

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.

[Bob Thomas]

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.

[Jacob]

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

[Luke Plant]

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).

[Luke Plant]

Updated patch (but please see my comment)

[Jacob]

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.

[Luke Plant]

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

[Jacob]

Milestone 1.2 deleted