diff --git a/includes/Model/Event.php b/includes/Model/Event.php index cf83b3884..e6a007961 100644 --- a/includes/Model/Event.php +++ b/includes/Model/Event.php @@ -15,10 +15,12 @@ use MediaWiki\Logger\LoggerFactory; use MediaWiki\MediaWikiServices; use MediaWiki\Revision\RevisionRecord; use MediaWiki\Title\Title; +use MediaWiki\User\ActorStore; use MediaWiki\User\User; use MediaWiki\User\UserIdentity; use RuntimeException; use stdClass; +use Wikimedia\IPUtils; use Wikimedia\Rdbms\IDBAccessObject; /** @@ -189,6 +191,19 @@ class Event extends AbstractEntity implements Bundleable { throw new InvalidArgumentException( "Invalid user parameter" ); } + // TODO: when no errors are logged in production, turn this into an exception + if ( !$obj->agent->isRegistered() && !IPUtils::isValid( $obj->agent->getName() ) ) { + LoggerFactory::getInstance( 'Echo' )->error( + 'Invalid IP agent: {username} for event type {event_type}', + [ + 'username' => $obj->agent->getName(), + 'event_type' => $obj->type, + 'exception' => new \RuntimeException, + ] + ); + return false; + } + // RevisionStore returns UserIdentityValue now, convert to User for passing to hooks. if ( !$obj->agent instanceof User ) { $obj->agent = $services->getUserFactory()->newFromUserIdentity( $obj->agent ); @@ -391,7 +406,19 @@ class Event extends AbstractEntity implements Bundleable { if ( $row->event_agent_id ) { $this->agent = User::newFromId( (int)$row->event_agent_id ); } elseif ( $row->event_agent_ip ) { - $this->agent = User::newFromName( (string)$row->event_agent_ip, false ); + // Due to an oversight, non-existing users could be inserted as IPs. + // This wouldn't cause problems if there wasn't the limit of 39 bytes for + // the database field, leading to silent truncation of long user names. + // Ignoring such entries and setting the agent to null could cause + // exceptions in presentation models, hence we accept the name if it's + // definitely not been truncated, otherwise return a fallback user. + if ( IPUtils::isValid( $row->event_agent_ip ) + || strlen( $row->event_agent_ip ) < 39 + ) { + $this->agent = User::newFromName( $row->event_agent_ip, false ); + } else { + $this->agent = User::newFromName( ActorStore::UNKNOWN_USER_NAME, false ); + } } // Lazy load the title from getTitle() so that we can do a batch-load diff --git a/tests/phpunit/integration/EventIntegrationTest.php b/tests/phpunit/integration/EventIntegrationTest.php index d5d27ea70..8e72404a7 100644 --- a/tests/phpunit/integration/EventIntegrationTest.php +++ b/tests/phpunit/integration/EventIntegrationTest.php @@ -52,6 +52,16 @@ class EventIntegrationTest extends MediaWikiIntegrationTestCase { ); } + public function testEventNotCreatedForUnknownUser() { + $this->clearHook( 'BeforeEchoEventInsert' ); + + $event = Event::create( [ + 'type' => 'welcome', + 'agent' => UserIdentityValue::newAnonymous( 'Anonymous user' ), + ] ); + $this->assertFalse( $event ); + } + public function testEventInsertionDeferred() { $this->clearHook( 'BeforeEchoEventInsert' ); $this->clearHook( 'PageSaveComplete' );