Page MenuHomePhabricator

Audit attemptSave, onEditFilter and onEditFilterMergedContent implementations to see which ones check for IP user specifically
Open, Needs TriagePublic

Description

Context

The parent task proposes to create the temp user after the initial core edit constraints run, but before the attemptSave, EditFilter, and EditFilterMergedContent hooks run.

Proposal

  • Audit all implementations of the three pre-edit hooks, and see which ones check for an IP user specifically
  • Work out which of those implementations could be easily converted to handle a temporary account as the $user

Event Timeline

kostajh renamed this task from Audit :attemptSave; onEditFilter; onEditFilterMergedContent implementations to see which ones check for IP user specifically to Audit attemptSave, onEditFilter and onEditFilterMergedContent implementations to see which ones check for IP user specifically.Mar 12 2024, 12:48 PM
kostajh updated the task description. (Show Details)

Here are the results of the investigation. I've audited all handlers in codesearch, not just WMF. For each one, I've made reasonable attempts to check if the User object is needed or not. This includes going at least one level deep to see if there were any RequestContext::getMain or similar calls. I did not go down code paths where it didn't seem to make sense to have a hidden user dependency, e.g., for code like $this->isConfigPage( $title ). Some extensions have particularly complex code that I did not fully investigate, so I would consider these to be partial results.

EditPage::attemptSave

Registered handlers

  • AccessControl (handler): doesn't need user.
  • DiscussionThreading (handler): doesn't need user.
  • MsCatSelect (handler): doesn't need user.
  • WikiEditor (handler): logs the edit attempt (using EventLogging). This includes the user who's attempting the edit, and it checks for anons specifically. Not really related to this task, but changing the order in which this hook is run would have an impact on the attempted edit logging, as @kostajh already discovered while working on T359405.
  • TemplateStylesExtender (handler): runs a permission check on the context user. The user right being checked is configurable, but it defaults to editinterface, which by default is only given to sysops. Therefore, this code shouldn't be affected by temp accounts unless the wiki is configured so that everyone can edit the interface; I would argue that in this scenario, the sysadmins would have much more pressing problems to solve.

EditFilter

Registered handlers

  • Athena (handler): uses the context user in calculateAthenaValue. It retrieves the user age and then uses it in some calculations to determine the probability that the edit is spam. Anons are special-cased, so it all boils down to how the extension treats anons vs newly (as in, < 1 second ago) registered accounts. I didn't investigate any deeper.
  • Drafts (handler): does not need the user.
  • JsonData (handler): does not need the user.
  • SpamBlacklist (handler): does not need the user.
  • SpamRegex (handler): does not need the user.
  • TitleBlacklist (handler): does not need the user.
  • AutomatedValues (handler): does not need the user.
  • WikibaseExport (handler): does not need the user.
  • WikibaseRDF (handler): does not need the user.
  • mediawiki-moderation (handler): does not need the user.

EditFilterMergedContent

Registered handlers

  • AbuseFilter (handler): widely discussed in T334623 and the various subtasks of T307060.
  • AkismetKlik (handler): sends the username and user's email to an external service for spam checking. How said service handles anons vs temp accounts, I don't know. The handler also checks the bypassakismet user right, which is not assigned to anybody by default (and at any rate, I wouldn't expect it to be assigned to all users, as that would defeat its purpose).
  • ConfirmEdit (handler): uses the User object further down the chain. SimpleCaptcha::confirmEditMerged calls ::doConfirmEdit, which calls shouldCheck and passCaptchaLimitedFromRequest. The former checks the skipcaptcha right (sysops and bots by default), and calls WikiPage::prepareContentForEdit; we may want to make sure that prepareContentForEdit is given the same user by core and ConfirmEdit, also for performance (but is this the case today?). passCaptchaLimitedFromRequest uses the rate limiter. In general, I believe that this extension would need to be investigated more thoroughly.
  • EventLogging (handler): does not need the user.
  • Gadgets (handler): does not need the user.
  • GrowthExperiments (handler): does not need the user.
  • JsonConfig (handler): does not need the user.
  • LinkedWiki (handler): does not need the user.
  • MediaUploader (handler): only checks whether the user is MediaUploader's system user (done by comparing the username), which would not be affected by the proposed change.
  • Newsletter (handler): checks the rate limiter for the fake newsletter right. The default rate is by user, so it would start applying the limit on temporary accounts.
  • RealMe (handler): does not need the user.
  • Scribunto (handler): does not need the user.
  • SpamBlacklist (handler): similar complexity to AbuseFilter. Checks user rights, edit stash, and then logs the hit. This is known to already have problems (T358806), and it needs to be investigated further.
  • Translate (handler 1, handler 2): the first handler checks whether the user has the translate-manage right (not assigned by default), or if they are the FuzzyBot user. Neither of these should affect anons or temp accounts. The second handler does not need the user.
  • UploadWizard (handler): does not need the user.