Page MenuHomePhabricator

With $wgReadOnly set and when $wgSessionCacheType = CACHE_DB, ApiQueryTokens will return invalid tokens
Open, Needs TriagePublic

Description

With $wgReadOnly set and when $wgSessionCacheType = CACHE_DB, ApiQueryTokens will return invalid tokens. The token will result in a code: "badtoken" error when used, and action=checktoken confirms they are invalid.

I suspect that this is because it can't persist the session in the database (ApiQueryTokens::getToken calls $session->persist()). There is no error checking for this at all, anywhere, as far as I can see.

ApiQueryTokens should probably return a code: "readonly" error in this case. It's not like the tokens are useful for much anyway if the site is readonly.

This doesn't always happen; I suppose it works if a session is already persisted? I've had trouble with this error disappearing when I tried to reproduce it, and reappearing when I gave up.

Event Timeline

matmarex created this task.Apr 20 2017, 7:46 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 20 2017, 7:46 PM
Kghbln added a subscriber: Kghbln.EditedApr 20 2017, 8:19 PM

Just by chance I stumbled into a similar problem today. With $wgReadOnly set and when $wgSessionCacheType = CACHE_DB generating thumbs with $wgGenerateThumbnailOnParse = false; does not work. This became apparent after upgrading from 1.23.16 to 1.27.2.

Anomie added a subscriber: Anomie.

I suppose it works if a session is already persisted?

It probably works when no token needs to be set into the session.

There is no error checking for this at all, anywhere, as far as I can see.

And adding error checking is not necessarily as straightforward as you might think. ->persist() won't necessarily try to save anything depending on whether the session is already persisted and whether something has an open ->delaySave(). Nor will any of the other stuff that writes the token secret into the session. A save to the SqlBagOStuff might not actually be attempted until after the response is sent to the client and PHP is shutting down.

To properly handle this sort of situation, we'd probably need BagOStuff to have an "isReadOnly" method of some sort to indicate that saves aren't going to work, then SessionManager would need to put the session in read-only mode if its BagOStuff is read only, which would need to cause any attempt to write to the session (probably in SessionBackend->dirty() among other places) to throw an exception of some sort.

The API probably wouldn't actually need to do anything except maybe return a better error code.

It's not like the tokens are useful for much anyway if the site is readonly.

Logging in to be able to view pages with your own preferences, user scripts, and so on?

Could ApiQueryTokens::getToken() try to check the validity of the token it just generated, and if it's invalid, return an error? Or would that be checking against some cached data and always return that the token is valid?

It's not like the tokens are useful for much anyway if the site is readonly.

Logging in to be able to view pages with your own preferences, user scripts, and so on?

If $wgSessionCacheType = CACHE_DB, then you can't login while the site is readonly anyway.

Or would that be checking against some cached data and always return that the token is valid?

Yes. The problem isn't in the generation of the tokens, it's that the saving it to the session is silently ignored.

If $wgSessionCacheType = CACHE_DB, then you can't login while the site is readonly anyway.

But with other types you can log in fine, so making action=tokens not work when the site is readonly would be unnecessary there. And I'd really rather not have ApiQueryTokens hard-coding in checks for "$wgSessionCacheType === CACHE_DB".