Page MenuHomePhabricator

SessionManager should not emit Set-Cookies on session renewal
Open, Needs TriagePublic

Description

Session renewal (see SessionBackend::renew()) happens when the session store object is near expiry. It doesn't have anything to do with cookie expiration (which in practice is either bound to the browser session, or long enough that it's OK not to renew it) so there is not point emitting cookies, and this is the main source of session cookies being emitted at unpredictable times. We should get rid of it.

Event Timeline

I was hoping this could be done simply by throwing out the if ( $this->persist ) block from SessionBackend::renew() but unfortunately that doesn't really have any effect; the cookies will still be output due to the dirty metadata flag. Not sure if there's an easy fix for this... maybe just suppress setting cookies when they would make no difference compared to the existing cookies? But then, we'd have to deal with SameSite on top of that. Ugh.

Change 633021 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] SessionManager: Allow disabling persisting session metadata on renew

https://gerrit.wikimedia.org/r/633021

We wanted this on the next train but TBH I'm not sure how useful it will be. Looking at the logs, session renewals are responsible for something like 15% of Set-Cookie headers, and I don't think we can easily avoid the rest, so overall this won't improve things that much.