Page MenuHomePhabricator

LoggedOut cookie not set anymore
Closed, ResolvedPublic

Description

In my testing, LoggedOut is not set anymore. I'm not sure if this is intentional. My understanding is that this is supposed to be used to bypass pages that were cached by the browser when you were logged in.

Impact

(Unconfirmed). I suspect that now, when you view article X, and are logged in, and then log out, and view the same pages you have viewed before, it will appear as if you are logged in, because the browser is getting a "304 Not Modified" response from the server allowing the b browser to use its cache. This would be incorrect, potentially confusing, and might seem insecure to users.

See also:

Event Timeline

It's not intentional; it's supposed to be set in CookieSessionProvider::setLoggedOutCookie. But I guess that's never called since the (anonymous) session is not persisted on logout.

If that is indeed the case then it has been broken since January, when SessionManager was deployed (specifically, the User::doLogout() changes in a73c5b7395a07d490f7052fd3b2491ebd656b190).

More likely is that the fix for T127436 (6d4436c9) caused the cookie to no longer be set since the session is no longer persisted by the time it would be sent to the browser.

I'm not sure that this is a problem anymore, though: the intention seems to have been to fix T2063 without clearing/resetting the session cookie, but since T127436 we do clear the session cookie so the Vary header should handle varying the cache now.

From 990d7679/7f64539 it would seem that clearing the session is not enough, or not wanted. Maybe @tstarling can clarify?

Related: T33639

OTOH, setting the LoggedOut cookie again might well re-break T127436, since it's included in the Key header. I'd hope browsers are smart enough to respect at least the Vary header for their local caching.

So.... should this be declined? Sounds like there is no consensus that the advantages outweigh?

So.... should this be declined? Sounds like there is no consensus that the advantages outweigh?

Anyone willing to make a decision?

I think we're waiting for Tim's opinion in light of 990d7679/7f64539.

@tstarling: Could you provide your opinion please? See last comment by anomie. Thanks.

aaron triaged this task as Lowest priority.Jun 6 2019, 9:20 AM

I suspect that now, when you view article X, and are logged in, and then log out, and view the same pages you have viewed before, it will appear as if you are logged in

That does not seem to be the case, on multiple levels:

  • I can't actually produce a 304 (I'm testing in Chrome 74 on Ubuntu): if I log out, the following load of a previously visited page will be a 200, and only the following ones 304s. Not sure why that happens, it seems like the server shouldn't be able to differentiate but somehow it does.
  • There is a Vary: ..., Cookie, ... header on all responses, and logged-in and logged out requests have different cookies so I imagine that splits the cache on the browser side as well, and the browser doesn't use the cached 200 that was the response for a request with a session cookie to render a 304 response given for a request with no session cookie.
  • All responses to logged-in requests have Expires: Thu, 01 Jan 1970 00:00:00 GMT which I think discourages browser caching anyway.

This sounds to me like the cookie is actually not needed. If that is the case, the related code should be deleted.

If this is not the case and we still need the cookie, the code needs to be fixed.

The cookie has been defunct for almost a decade. The code should be deleted.

Change #1203251 had a related patch set uploaded (by Ori; author: Ori):

[mediawiki/core@master] Remove defunct LoggedOut cookie

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

Change #1203252 had a related patch set uploaded (by Ori; author: Ori):

[mediawiki/core@master] Remove defunct LoggedOut cookie

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

Change #1203251 abandoned by Ori:

[mediawiki/core@master] Remove defunct LoggedOut cookie

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

[…]

  • All responses to logged-in requests have Expires: Thu, 01 Jan 1970 00:00:00 GMT which I think discourages browser caching anyway.

It does not.

Expires with a past date is the same as max-age=0, which in turn is the same as a expires in the future or max-age=100 and then waiting for that to be in the past / expire. The result is that browsers should revalidate it via the IMS/INM header and may then receive an empty HTTP 304 response instead a full HTTP 200 re-download.

Unless something extreme like Cache-Control: no-store is used, a browser will store it, and then use it offline (if fresh) or use it it revalidation (if expired).

This is all a good thing, because logged-in users can do and do benefit from HTTP 304 when re-visiting articles that haven't been edited/touched since your last visit. Changing cookies when logging-in or logging-out effecitvely invalidates the local browser cache, because we set Vary: Cookie which means the internal cache key will not be based solely on the URL but also on the cookie values.

Varnish has this snippet, if we are dropping the cookie, we should probably drop it as well:

// Users that just logged out, should not get a 304 for their
// (locally cached) logged in pages.
if (req.http.If-Modified-Since && req.http.Cookie ~ "LoggedOut") {
    unset req.http.If-Modified-Since;
}

Do we understand why the cookie is defunct? It's called unconditionally in persistSession() as

$this->setLoggedOutCookie( $session->getLoggedOutTimestamp(), $request );

so I guess $session->getLoggedOutTimestamp() always returns 0? Should we get rid of that (and generally the loggedOut field in session metadata) as well? The other thing it gets used for is setting User::$mTouched for anonymous users.

setLoggedOutTimestamp() is called on logout, but since we are unpersisting the session on logout, I'm not sure what that's supposed to achieve.

Change #1217757 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/CentralAuth@master] Remove LoggedOut cookie logic

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

Change #1203252 merged by jenkins-bot:

[mediawiki/core@master] Remove defunct LoggedOut cookie

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

Change #1217774 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[operations/puppet@production] Remove LoggedOut cookie handling

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

Change #1217790 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[operations/mediawiki-config@master] Remove LoggedOut cookie logic

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

Change #1217757 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] Remove LoggedOut cookie logic

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

Change #1217790 merged by jenkins-bot:

[operations/mediawiki-config@master] Remove LoggedOut cookie logic

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

Mentioned in SAL (#wikimedia-operations) [2025-12-18T21:06:55Z] <tgr@deploy2002> Started scap sync-world: Backport for [[gerrit:1217790|Remove LoggedOut cookie logic (T142542)]], [[gerrit:1219588|Turn on Parsoid Read Views on itwiki (T413084)]], [[gerrit:rOPUP121728222dda|Logos: Handle missing responsive URLs, manually modify thumbnail sizes to avoid $wgThumbnailSteps (T405169)]]

Mentioned in SAL (#wikimedia-operations) [2025-12-18T21:08:54Z] <tgr@deploy2002> pppery, tgr, cscott: Backport for [[gerrit:1217790|Remove LoggedOut cookie logic (T142542)]], [[gerrit:1219588|Turn on Parsoid Read Views on itwiki (T413084)]], [[gerrit:rOPUP121728222dda|Logos: Handle missing responsive URLs, manually modify thumbnail sizes to avoid $wgThumbnailSteps (T405169)]] synced to the testservers (see https://wikitech.wikimedia.org/wiki/Mwdebug). Changes can now be verified there.

Mentioned in SAL (#wikimedia-operations) [2025-12-18T21:13:23Z] <tgr@deploy2002> Finished scap sync-world: Backport for [[gerrit:1217790|Remove LoggedOut cookie logic (T142542)]], [[gerrit:1219588|Turn on Parsoid Read Views on itwiki (T413084)]], [[gerrit:rOPUP121728222dda|Logos: Handle missing responsive URLs, manually modify thumbnail sizes to avoid $wgThumbnailSteps (T405169)]] (duration: 06m 28s)

Change #1217774 merged by RLazarus:

[operations/puppet@production] Remove LoggedOut cookie handling

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

matmarex claimed this task.

Looks all done in MediaWiki and WMF configs.