Page MenuHomePhabricator

Endpoints which do not need to authenticate users should set MW_NO_SESSION
Closed, ResolvedPublic

Description

The old MediaWiki session logic only loaded the session when $wgUser was unstubbed, which could cause problems due to hook calls executing unexpectedly early or late. SessionManager moved session initialization into Setup.php; this means that the session is set up even if $wgUser is never needed. While setting up the session is relatively cheap, there is no reason not to skip it in the cases where we know by the time of calling Setup.php (ie. from the choice of endpoint) that authentication won't be needed.

Endpoints that look like they should not need sessions are: load.php, static.php, opensearch_desc.php, profileinfo.php. Probably also thumb.php which does per-user limiting of thumbnailing attempts, but doing that on a per-IP instead of per-username basis does not seem like much of a loss, and for most MediaWiki installations that's what's happening anyway since thumbnails are served from a different domain.

MW_NO_SESSION has not been used before in web requests, so such a change might trigger new bugs and should be tested cautiously.

Details

SubjectRepoBranchLines +/-
mediawiki/coremaster+6 -0
operations/mediawiki-configmaster+5 -1
mediawiki/coremaster+4 -6
mediawiki/coremaster+10 -0
mediawiki/coremaster+2 -3
mediawiki/coremaster+10 -2
mediawiki/corewmf/1.27.0-wmf.16+4 -1
mediawiki/coremaster+4 -1
mediawiki/coremaster+1 -1
mediawiki/corewmf/1.27.0-wmf.16+3 -1
mediawiki/coremaster+3 -1
mediawiki/coremaster+5 -0
mediawiki/extensions/MobileFrontendwmf/1.27.0-wmf.15+5 -4
mediawiki/extensions/MobileFrontendmaster+5 -4
mediawiki/extensions/Scribuntomaster+4 -1
mediawiki/coremaster+6 -0
mediawiki/coremaster+14 -4
mediawiki/coremaster+7 -2
mediawiki/corewmf/1.27.0-wmf.14+68 -20
mediawiki/corewmf/1.27.0-wmf.14+1 -7
mediawiki/corewmf/1.27.0-wmf.13+73 -21
mediawiki/coremaster+1 -7
mediawiki/coremaster+68 -20
Show related patches Customize query in gerrit

Related Objects

StatusSubtypeAssignedTask
Resolved Deskana
ResolvedAnomie
OpenNone
ResolvedAnomie
OpenNone
ResolvedTgr
ResolvedAnomie
OpenFeatureNone
OpenNone
ResolvedAnomie
ResolvedTgr
ResolvedJdlrobson
Resolved bd808
ResolvedNone
ResolvedTgr
ResolvedNone
ResolvedNone
Resolved bd808
ResolvedLegoktm
DeclinedAnomie
ResolvedLegoktm
DeclinedNone
ResolvedAnomie

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 273372 had a related patch set uploaded (by Anomie):
Log violations of load.php's no-session constraint

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

Change 273373 had a related patch set uploaded (by Anomie):
Enforce load.php's no-session constraint

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

Change 275617 had a related patch set uploaded (by Anomie):
Use RL context language in MFResourceLoaderParsedMessageModule

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

Change 275617 merged by jenkins-bot:
Use RL context language in MFResourceLoaderParsedMessageModule

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

Change 275683 had a related patch set uploaded (by Gergő Tisza):
Use RL context language in MFResourceLoaderParsedMessageModule

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

Change 275683 abandoned by Gergő Tisza:
Use RL context language in MFResourceLoaderParsedMessageModule

Reason:
On second thought not really needed as the "flag" in https://gerrit.wikimedia.org/r/#/c/273372/ will go out with the train.

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

Change 273372 merged by jenkins-bot:
Log violations of load.php's no-session constraint

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

Change 276207 had a related patch set uploaded (by Anomie):
Log a backtrace with "sessions are supposed to be disabled" message

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

Change 276207 merged by jenkins-bot:
Log a backtrace with "sessions are supposed to be disabled" message

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

Change 276231 had a related patch set uploaded (by Anomie):
HttpFunctions: Log in the wiki's content language

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

Change 276232 had a related patch set uploaded (by Anomie):
Log a backtrace with "sessions are supposed to be disabled" message

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

Change 276232 merged by jenkins-bot:
Log a backtrace with "sessions are supposed to be disabled" message

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

Change 276246 had a related patch set uploaded (by Anomie):
Check User::isSafeToLoad() in LanguageConverter

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

Here's a fun one: $wgForceUIMsgAsContentMsg makes Message::inContentLanguage() do nothing, and some wikis have 'mainpage' in that list so Title::newMainPage() will blow up. I think T127920 is our only hope of dealing with that craziness.

Change 276246 merged by jenkins-bot:
Check User::isSafeToLoad() in LanguageConverter

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

Change 276231 merged by jenkins-bot:
HttpFunctions: Log in English

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

Change 276467 had a related patch set uploaded (by Anomie):
Check User::isSafeToLoad() in LanguageConverter

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

Change 276467 merged by jenkins-bot:
Check User::isSafeToLoad() in LanguageConverter

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

Change 276891 had a related patch set uploaded (by Krinkle):
Use context language instead of global wfMessage()

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

Change 280267 had a related patch set uploaded (by Anomie):
ResourceLoader: Don't call Title::newMainPage() when 'mainpage' is in $wgForceUIMsgAsContentMsg

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

Change 280267 merged by jenkins-bot:
resourceloader: Avoid Title::newMainPage() to support $wgForceUIMsgAsContentMsg

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

Change 273373 merged by jenkins-bot:
Enforce load.php's no-session constraint

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

Change 287100 had a related patch set uploaded (by Anomie):
Warn on session access in profileinfo.php and opensearch_desc.php

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

Change 287100 merged by jenkins-bot:
Warn on session access in profileinfo.php and opensearch_desc.php

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

https://gerrit.wikimedia.org/r/#/c/273373/
That will give third parties about six months to fix/upgrade extensions.

Rather, it forces third parties such as translatewiki.net to report issues in extensions they do not maintain and revert this patch locally.

Why was no testing done with the most popular extensions and their developers notified so that they can start fixing the issue early?

Rather, it forces third parties such as translatewiki.net to report issues in extensions they do not maintain and revert this patch locally.

Why was no testing done with the most popular extensions and their developers notified so that they can start fixing the issue early?

Apologies if this caused problems. A notification was sent when the patch was merged, although I guess that's not much help for sites which use the master branch. Invalid use of sessions has raised warnings for a couple months now, but we did fail to adequately notify people about the importance of that.

The current reality is the the WMF is not committing development resources for extensions it itself does not use, so testing the most popular extensions is not an option (unless someone volunteers). Parties interested in changing that should probably work on T127312 or something similar.

If you want to identify further problematic extensions on your site, look for warning-level log events in the session channel with the text Sessions are supposed to be disabled for this entry point. If you have enabled structured logging, the log message will also contain a stack trace of there the session access happened.

I have been updating code quite regularly [1] but the warnings only started happening today. I also reverted 40accc9733a0998610d856874d84c8aec6f64c82 but that did not stop the warnings from showing up in error_log. What am I missing? It would also be helpful if $wgDevelopmentWarnings = true would forward these to error_log so I could possibly notice them in my development instance.

[1]

[13:23:27] -rakkaus:#mediawiki-i18n- [nike] updating translatewiki.net from b20e48d 2016-05-04 13:33:00 +0200 (2 days ago)...
[13:25:43] -rakkaus:#mediawiki-i18n- [nike] updated translatewiki.net to 1b4fbb4 2016-05-06 12:24:16 +0200 (2 minutes ago)

The warnings used to be sent to the session log channel. You can create a custom log handler that turns them into proper PHP warnings.

There are some endpoints that still log such warnings and will be turned into exceptions soon (opensearch_desc.php, profileinfo.php). That is unlikely to break third-party sites though as extensions don't hook into those endpoints.

I also reverted 40accc9733a0998610d856874d84c8aec6f64c82 but that did not stop the warnings from showing up in error_log.

That's weird. Do you have an example of such warnings?

The only other endpoint that throws exceptions currently is static.php which is part of the multiwiki setup and probably not used outside the WMF.

Since the error did not make sense, I restarted HHVM just in case and now the error does not appear anymore. Sorry for the noise.

There is a single instance of the Sessions are disabled for this entry point error in the last 30 days, on wikitech:
https://logstash.wikimedia.org/app/kibana#/doc/logstash-*/logstash-2016.10.28/mediawiki/?id=AVgL2K3tcJvqzGkjW53i

/srv/mediawiki/php-1.28.0-wmf.23/includes/session/SessionManager.php:308
/srv/mediawiki/php-1.28.0-wmf.23/includes/session/SessionManager.php:242
/srv/mediawiki/php-1.28.0-wmf.23/includes/session/SessionManager.php:192
/srv/mediawiki/php-1.28.0-wmf.23/includes/WebRequest.php:735
/srv/mediawiki/php-1.28.0-wmf.23/includes/user/User.php:1199
/srv/mediawiki/php-1.28.0-wmf.23/includes/user/User.php:404
/srv/mediawiki/php-1.28.0-wmf.23/includes/user/User.php:5125
/srv/mediawiki/php-1.28.0-wmf.23/includes/user/User.php:2768
/srv/mediawiki/php-1.28.0-wmf.23/includes/context/RequestContext.php:364
/srv/mediawiki/php-1.28.0-wmf.23/includes/Message.php:364
/srv/mediawiki/php-1.28.0-wmf.23/includes/Message.php:1188
/srv/mediawiki/php-1.28.0-wmf.23/includes/Message.php:802
/srv/mediawiki/php-1.28.0-wmf.23/includes/Message.php:902
/srv/mediawiki/php-1.28.0-wmf.23/includes/exception/MWExceptionRenderer.php:249
/srv/mediawiki/php-1.28.0-wmf.23/includes/exception/MWExceptionRenderer.php:334
/srv/mediawiki/php-1.28.0-wmf.23/includes/exception/MWExceptionRenderer.php:46
/srv/mediawiki/php-1.28.0-wmf.23/includes/exception/MWExceptionHandler.php:71
/srv/mediawiki/php-1.28.0-wmf.23/includes/exception/MWExceptionHandler.php:137
{
  "function": "handleException",
  "class": "MWExceptionHandler",
  "type": "::",
  "args": [
    {
      "class": "DBConnectionError",
      "message": "Cannot access the database: Can't connect to MySQL server on '208.80.154.136' (111) (208.80.154.136)",
      "code": 0,
      "file": "/srv/mediawiki/php-1.28.0-wmf.23/includes/libs/rdbms/database/Database.php:748",
      "trace": [
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/libs/rdbms/loadbalancer/LoadBalancer.php:897",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/libs/rdbms/loadbalancer/LoadBalancer.php:572",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/GlobalFunctions.php:3073",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/cache/MessageCache.php:444",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/cache/MessageCache.php:403",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/cache/MessageCache.php:325",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/cache/MessageCache.php:927",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/cache/MessageCache.php:858",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/cache/MessageCache.php:826",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/cache/MessageCache.php:767",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/Message.php:1188",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/Message.php:802",
        "/srv/mediawiki/php-1.28.0-wmf.23/includes/Message.php:902",
        "/srv/mediawiki/php-1.28.0-wmf.23/opensearch_desc.php:63",
        "/srv/mediawiki/w/opensearch_desc.php:3"
      ]
    }
  ]
}

Change 323057 had a related patch set uploaded (by Gergő Tisza):
Use content language when displaying exceptions / outage messages

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

FYI when I reviewed the throttling code for porting over to Thumbor I also thought that the per-user thing was overkill and did it per-IP instead. It would make complete sense to simplify Mediawiki the same way and switch to per-IP throttling.

All subtasks appear to be resolved. Is this task ready to be closed as well?

Endpoints that look like they should not need sessions are: load.php, static.php, opensearch_desc.php, profileinfo.php. Probably also thumb.php

Done for load.php, still in "warn" mode for static.php, opensearch_desc.php, profileinfo.php. Not done for thumb.php and probably not easily doable as sessions are used for rate limiting and in some cases even for authentication, I think. So at least the three warn endpoints should be promoted to a full ban (after checking that there are no warnings).

I see no log entries in the past 60 days. Let's do it.

Change 464390 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] Enforce no-session constraint in opensearch_desc.php and profileinfo.php

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

Change 464391 had a related patch set uploaded (by Anomie; owner: Anomie):
[operations/mediawiki-config@master] Enforce no-session constraint in static.php

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

Change 464390 merged by jenkins-bot:
[mediawiki/core@master] Enforce no-session constraint in opensearch_desc.php and profileinfo.php

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

Change 464391 merged by jenkins-bot:
[operations/mediawiki-config@master] Enforce no-session constraint in static.php

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

Mentioned in SAL (#wikimedia-operations) [2018-10-11T02:25:09Z] <krinkle@deploy1001> Synchronized w/static.php: T127233 - Ic6acb70 (duration: 00m 49s)

Tagging with MW-1.32-release. (Hopeful).

This is probably something that'd be better for end-users if it changes at once in the 1.32 cycle rather than spanning multiple cycles.

(Hopeful).

Isn't this task done? All the patches have been merged and all the subtasks have been resolved. thumb.php still uses sessions, but it's unclear if we even want to change that, and if we do, we need to support its use cases in some other way, and hard-deprecate, that's two more release cycles at least.

Anomie claimed this task.

Yes, let's call this done. thumb.php currently does use the session data as noted above, changing that can be filed as a separate task if someone wants to follow up on it.

Change 323057 merged by jenkins-bot:

[mediawiki/core@master] Limit language logic when displaying exceptions / outage messages

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