Page MenuHomePhabricator

Don't set an anonymous user's name to their IP address if IP Masking is enabled
Open, Needs TriagePublic

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.