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() )

In T110620#1583373, @Bawolff wrote:

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.

@daniel This might tie in with the Authority model that you're working on. Specifically, I'm wondering if it would make sense both for cache-poisioning reasons and for privacy reasons, to prevent accidental use of the IP address in any way during a response unless it is for a permission/block/rate-limit etc use case.

This should then also prevent e.g. something like "My talk page" from rendering as an IP link and then cached for a random person who visited it, when configured wgUseCdn enabled.

Also caused T263368, see T263368#6478406

This can easily be fixed in UserFactory::newFromId(). Passing $id = 0 just doesn't make sense there. The only question is what we should do if it does happen. Throw InvalidArgumentException and crash hard? Or return the reserved "Unknown user"? Or return an invalid user with an empty name (which may spread and cause corruption)?

Note that the use case of constructing IP based anon users for use with blocks is covered by UserFactory::newAnonymous()

Proposal: Current behaviour with hard deprecation, then after a week of checking prod logs backport the hard-deprecation to 1.37 and immediately change in master to a hard crash.

Change 785821 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/core@master] user: Emit warning & log in prod when `UserFactory::newFromId(0)`

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

@Krinkle, this is still happening, I've made a patch to emit warnings & log in production. Let me know if that is enough to investigate the issue.

Change 790759 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/extensions/Flow@master] tests: Hide deprecation warning when calling `UserFactory::newFromId(0)`

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

Change 790759 abandoned by D3r1ck01:

[mediawiki/extensions/Flow@master] tests: Hide deprecation warning when calling `UserFactory::newFromId(0)`

Reason:

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

Change 785821 abandoned by D3r1ck01:

[mediawiki/core@master] Emit warnings in prod when `UserFactory::newFromId(0)` is invoked

Reason:

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