Make User::newFromId(0) not return current user's IP
Open, NormalPublic

Description

User::newFromId(0) is an easy thing to do by mistake, which creates the current user's IP, and then people may leak it. Since this is dangerous we should change this behaviour, and make User::newFromId(0) be 127.0.0.1 or something and make sure people explicitly request an anon IP

csteipp created this task.Aug 27 2015, 11:40 PM
csteipp updated the task description. (Show Details)
csteipp raised the priority of this task from to Normal.
csteipp claimed this task.
csteipp added a project: Security-Team.
csteipp added a subscriber: csteipp.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 27 2015, 11:40 PM

Have we identified why User::newFromId(0) was used? (It sounds kind of snotty when I say it like that, that's not my intention, I just mean - is there some area of the documentation we've identified that suggests using it, or anything. I just wonder because, in all of MediaWiki, that call is used only once outside tests, and only in the installer which is pretty obscure. Even in places where an anon user is needed, the preferred way of doing it is usually new User() )

Have we identified why User::newFromId(0) was used? (It sounds kind of snotty when I say it like that, that's not my intention, I just mean - is there some area of the documentation we've identified that suggests using it, or anything. I just wonder because, in all of MediaWiki, that call is used only once outside tests, and only in the installer which is pretty obscure. Even in places where an anon user is needed, the preferred way of doing it is usually new User() )

I think the issue is we haven't called it out as dangerous in any training / documentation, so it didn't raise security questions when it was used. At least that's my take on it.

It might be worth making the user class a bit more explicit. e.g. Having two new factory methods User::newAnonFromCurrentIP() and User::newGenericAnon() (With the later being equivalent to User::newFromName( '127.0.0.2', false ); ). This would make it very clear what the methods do at a glance. It would also allow separating the use case of, I need a generic user object, so my user-generic cache isn't polluted with user specific details [A rather common case in MediaWiki], and the case of, I need an anon user object for the current requester's IP [An extremely obscure need in MediaWiki].

@Bawolff I think those 2 suggestions are very sensible.
I believe User::newFromName( '127.0.0.1', false ) was essentially the expected behaviour of User::newFromId(0);
Also once introduced some explicit documentation of newFromId saying what will likely happen if you pass 0 id.

Aklapper removed csteipp as the assignee of this task.Dec 9 2017, 1:45 PM

"MediaWiki secure code training" = https://www.mediawiki.org/wiki/Security_for_developers and subpages? Or something else?

Bawolff renamed this task from Add code patterns that could impact privacy to MediaWiki secure code training. to Make User::newFromId(0) not return current user's IP.Sep 4 2018, 2:53 PM
Bawolff updated the task description. (Show Details)