Page MenuHomePhabricator

PermissionManager should not cache anonymous rights under ID 0
Closed, ResolvedPublic

Description

PermissionManager::getUserPermissions() caches user rights by ID, which means all anonymous or locally-non-existent users share a single cache entry. That will break if rights are granted or revoked based on e.g. IP address (such as in the case of IP blocks).

Solution: use namespaced cache keys. If the user ID is not 0, use 'u:' . $user->getId(). If it is 0, use 'anon:' . $user->getName().

Event Timeline

Tagging CPT for triage. This issue was introduced when factoring out PermissionManager for decoupling.

CCicalese_WMF added a subscriber: CCicalese_WMF.

I believe this was mistagged (API Integration Tests). Putting back in inbox.

I note that the cache in question is an in-process cache, and typically, the rights that get cached are for the current user. So there doesn't seem to be cause of immediate alarm. It is however a conceptual problem that could easily lead to issues, e.g. if the cache is made to persist between requests.

Proposed solution: just use the user name instead of the ID as the key to the cache array.

It probably won't break anything in production but it's not that hard to imagine some extension looking up the rights of some arbitrary anonymous user early on in the request, and that later affecting the context user. So IMO it's a bug, although a fringe one.

Proposed solution: just use the user name instead of the ID as the key to the cache array.

In theory you could have both a logged-in and an anonymous user called 1.2.3.4 on very legacy systems. Also a fringe problem, but should be easy to avoid by namespacing the keys to anon / user.

Change 531312 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] WIP: PermissionManager should not cache anonymous rights under ID 0

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

Change 531312 merged by jenkins-bot:
[mediawiki/core@master] PermissionManager should not cache anonymous rights under ID 0

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

Pchelolo claimed this task.