wikimedia/mediawiki-extensions-Echo
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/Echo synced 2024-11-23 15:36:58 +00:00
Code Issues Packages Projects Releases Wiki Activity

Disallow anonymous non-IP agents, handle truncated names

Browse source 
Why:
* Echo stores agents by their user id or by the name if the user
  is not registered. This works for IPs since the "event_agent_ip"
  field has limit of 39 bytes (32× [0-9A-F] + 7× colon for IPv6).
* However, it's possible to hold a user identity that is not
  an IP address, but the user name has not been or cannot be
  registered (e.g., external users). Echo wouldn't validate this
  and would attempt to insert the user name into "event_agent_ip",
  possibly causing silent truncation and data corruption.

What:
* Do not let events with such agents be saved. For now, log an
  error in the production. Wikibase, the only known source of this
  problem, has already been fixed.
* In runtime, replace every possibly corrupted user name with
  a placeholder to avoid unexpected null values and exceptions
  in production.

Bug: T367638
Change-Id: Ic2bd218b10651d13da9e9aea54dd2d668a33d946
Depends-On: I03b4367355dc5a3fc0c14aad5fdf19fbcd0caa3d
Depends-On: I92eb93983e81708b289e9f7d837884d539dade0b
This commit is contained in:
Matěj Suchánek 2024-08-25 14:07:20 +02:00
parent eff1de4fdf
commit 11b9e66f9f
2 changed files with 38 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

29
includes/Model/Event.php
View file

@ -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

10
tests/phpunit/integration/EventIntegrationTest.php
View file

@ -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' );