Page MenuHomePhabricator

Uncaught TypeError when associating revision with deleted user
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • As user A (who is registered for an event that has collaborative contributions enabled), make an edit
  • As user B, Revdelete the author of the edit (author A)
  • As the organizer of the event, using the "Add an edit" button implemented in T403727, try to add that edit to any event

What happens?:
The following uncaught TypeError is thrown:

2025-12-08 21:16:35 08a2c2d906e7 my_wiki: [0ed6824c12f25aa53f0d80fe] /w/rest.php/campaignevents/v0/event_registration/1106/edits/my_wiki/2198   TypeError: MediaWiki\Extension\CampaignEvents\MWEntity\CampaignsCentralUserLookup::newFromUserIdentity(): Argument #1 ($userIdentity) must be of type MediaWiki\User\UserIdentity, null given, called in /var/www/html/w/extensions/CampaignEvents/src/EventContribution/EventContributionValidator.php on line 133
#0 /var/www/html/w/extensions/CampaignEvents/src/EventContribution/EventContributionValidator.php(133): MediaWiki\Extension\CampaignEvents\MWEntity\CampaignsCentralUserLookup->newFromUserIdentity()
#1 /var/www/html/w/extensions/CampaignEvents/src/Rest/EventContributionsHandler.php(47): MediaWiki\Extension\CampaignEvents\EventContribution\EventContributionValidator->validateAndSchedule()
#2 /var/www/html/w/includes/Rest/SimpleHandler.php(41): MediaWiki\Extension\CampaignEvents\Rest\EventContributionsHandler->run()
#3 /var/www/html/w/includes/Rest/Module/Module.php(452): MediaWiki\Rest\SimpleHandler->execute()
#4 /var/www/html/w/includes/Rest/Module/Module.php(301): MediaWiki\Rest\Module\Module->executeHandler()
#5 /var/www/html/w/includes/Rest/Router.php(485): MediaWiki\Rest\Module\Module->execute()
#6 /var/www/html/w/includes/Rest/Router.php(444): MediaWiki\Rest\Router->doExecute()
#7 /var/www/html/w/includes/Rest/EntryPoint.php(207): MediaWiki\Rest\Router->execute()
#8 /var/www/html/w/includes/MediaWikiEntryPoint.php(184): MediaWiki\Rest\EntryPoint->execute()
#9 /var/www/html/w/rest.php(25): MediaWiki\MediaWikiEntryPoint->run()
#10 {main}

That's because Revision::getUser() can return null when the revision's author is deleted (we are using FOR_PUBLIC in the audience check, which is correct, so any kind of deletion will do). However, the code assumes it to be non-null.

What should have happened instead?:
Not sure... Perhaps we should fail with a user-friendly error, or maybe we could also allow adding the revision anyway (the record will be semi-deleted in EventContributionComputeMetrics anyway, based on the revision visibility). I believe this is a product question.
The association should go through anyway (and the user's name will later show up as "deleted user"), see T412063#11469854.
The association should fail with the following error message: This edit has been deleted and cannot be associated with the event.. See T412063#11526133

Other information (browser name/version, screenshots, etc.):
Note that this is already reproducible on current master using the API endpoint for associating edits. (And potentially, also making an edit, leaving the association dialog open, revdeleting the edit in a different tab, then confirming the association).

Event Timeline

Note, this would technically be a Wikimedia-production-error, but nobody has triggered it yet (high barrier due to having to make explicit API request and this being a new feature)

Daimona added a subscriber: ifried.

Discussed 2025-12-17: @ifried recommends allowing the association anyway. So, this task can now be worked on.

While working on this, I realized that I made a mistake, and there's actually an important difference between the deletion happening before or after the association. When you try to associate an edit with an event, we need to verify that the association is allowed; amongst other things, this includes checking that the edit author is an event participant, and that the association is being done by either the author themself, or an event organizer. However, we cannot perform these checks when the user has been deleted: otherwise, when it's the organizer performing the association, we would leak to them whether the author of the edit is a participant of their event. The scenario would be something like:

  • Alice organizes an event
  • Bob registers for the event
  • Bob makes an edit, but does not associate it with the event
  • Some time later, the edit's author is revdeleted
  • Alice knows that Bob participated in her event, but she did not see that edit. She now comes across that edit from a page history.
  • Alice can try to associate the edit with the event, and if successful, she will know that it's Bob who made the edit

Note that a few variations of the scenario are possible (for example, Alice could try that with multiple events). I also don't think it's a very common scenario, but it's still a potential info leak in principle, and also an unnecessary one.

Skipping the relevant permission checks conditionally is a bit messy. So, I wonder if we could actually make a u-turn and forbid the association in this case; for this, we would only need the text of an error message. Additionally, I would suggest applying the same failure mode to every kind of revision deletion, so not just the author, but any part of the revision being deleted (content, summary). To give an example, the message could be something like "This edit has been deleted and cannot be associated with the event". @ifried

Thank you for running through this scenario, @Daimona! If I understand the situation correctly, you're proposing that we prevent an organizer (and maybe also the participant who made the edit?) from adding an edit to an event if the author was revdeleted. Like you wrote, this scenario is highly unlikely, but it should be prevented regardless to preserve privacy.

You also bring up if we should prevent the adding of *any* edit that has been revision deleted, whether it is the edit or the author. I think that makes sense, since from my understanding, the edit information is only visible to certain people, like admins. It is not generally accessible, so it doesn't seem like a good contender for a collaborative editing event.

So, yes, let's go ahead and forbid adding any revdeleted edit to an event. The text you proposed also sounds good: "This edit has been deleted and cannot be associated with the event."

Thank you!

Change #1228519 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] EventContributionValidator: guard against deleted revision author

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

Change #1228519 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] EventContributionValidator: guard against deleted revision author

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