Page MenuHomePhabricator

Don't set an anonymous user's name to their IP address if IP Masking is enabled
Closed, DeclinedPublic

Description

Background

After IP Masking is enabled, new users:

  • Should not have their IP address shown in the UI as their name
  • Should not be stored with their IP address as their name in the actor table (even if this never makes it into the UI)

User::getName sets the name of an anonymous user to their IP address:

/**
 * Get the user name, or the IP of an anonymous user
 * @return string User's name or IP address
 */
public function getName(): string {
	if ( $this->isItemLoaded( 'name', 'only' ) ) {
		// Special case optimisation
		return $this->mName;
	}

	$this->load();
	if ( $this->mName === false ) {
		// Clean up IPs
		$this->mName = IPUtils::sanitizeIP( $this->getRequest()->getIP() );
	}

	return $this->mName;
}

ActorStore inserts a user into the actor table via acquireActorId and createNewActor.

What should we do

We can:

  1. Solve T336187: [S] Investigate: Creating temp user on non-EditPage actions, which should catch most cases, but perhaps not all
  2. First log a warning if this branch of User::getNameis hit, or if an IP actor is inserted, with IP Masking enabled. (A warning rather than an error so that we can detect when this is happening without causing disruption.)
  3. Deploy to test wikis and early adopters, and monitor for these warnings
  4. After we're confident we've fixed almost all cases, throw errors [ i ]
  5. Continue to monitor for these errors, and fix any more cases that arise

After step (4) we can confidently tell users that their IP address will not be shown in the user interface or stored in the DB [ ii ]. Until then, with IP Masking enabled, it shouldn't but it might.

[ i ] Anonymous users may need to be created, and their name set, in a situation when we don't want to create a temporary user (for example for permissions checks). For this reason, we should set some placeholder name in User::getName. Since this will never be stored in the database, presumably there is no need to disambiguate.

[ ii ] We should check whether the IP address is ever set by some means other than calling User::getName, e.g. by creating a new user and passing the IP address as a name, and fix those cases. We should also check whether the actor table is inserted into from anywhere else, and reroute those calls via ActorStore.

Event Timeline

I'm not sure if there is anything to do with getName:

  1. We need to handle a case where mName is not yet set, for example, a request before a session is fully initialized.
  2. Acquiring and stashing a temp account name for every request seems wasteful and also complicated to implement, because of the request lifecycle
  3. We can't set mName to a placeholder value that looks like a temp account name, because then other places in code only check if the username looks like a temp account, so a lot of code would assume the user is logged-in as a temp account user
  4. Setting mName to something that matches the reserved pattern but not the temp account match pattern might work, but then there is no disambiguation between the name for requests from different IP addresses. (That probably doesn't matter in practice, though) The larger problem, though, is that once mName is populated with anything that is not an IP address, various places in MediaWiki will want to treat the User object as a logged-in user.

It seems easier to handle cases like T358683#9595978 by replacing the IP address with a placeholder name as part of returning the data.

@Dreamy_Jazz @mszabo @STran thoughts?

Niharika moved this task from Backlog to Someday / Maybe on the Temporary accounts board.

High technical effort. Low priority at the moment.