Leaking only to shell holders, but..

This is the relevant piece of code from CentralAuth:

$data = [ 'gu_name' => $newname ];
if ( $type === GlobalRenameRequest::VANISH ) {
	// Vanish requests need to remove user's email
	$data[ 'gu_email' ] = '';
}
$dbw->newUpdateQueryBuilder()
	->update( 'globaluser' )
	->set( $data )
	->where( [ 'gu_name' => $oldname ] )
	->caller( __METHOD__ )
	->execute();

(Partial) dupe of T104500: Old versions of sensitive user data (email, password hashes) can remain in database indefinitely due to local and global DB not being kept in sync?

GlobalRenameUser::getJob doesn't access $options['type'], so the default parameter in LocalRenameUserJob (used for vanishing as well) is GlobalRenameRequest::RENAME when it should be GlobalRenameRequest::VANISH. That should be sufficient to address this, because then we'll hit the code path that uses User::newSystemUser( $renamedUser->getName(), [ 'steal' => true ] );

In T418122#11640588, @kostajh wrote:

01-T418122.patch3 KBDownload

LGTM, +2

In T418122#11640535, @kostajh wrote:

GlobalRenameUser::getJob doesn't access $options['type'], so the default parameter in LocalRenameUserJob (used for vanishing as well) is GlobalRenameRequest::RENAME when it should be GlobalRenameRequest::VANISH. That should be sufficient to address this, because then we'll hit the code path that uses User::newSystemUser( $renamedUser->getName(), [ 'steal' => true ] );

Note this has wider effects than email: It also scrubs gu_password and user_password, making the actions irreversible. FWIW, this came up in the context of https://meta.wikimedia.org/wiki/Talk:Account_vanishing#c-Cabayi-20260223143200-Reversal_/_Unvanishing, where the reversibility is being discussed. (No objections towards deploying that though, I just want to make sure those consequences are known/expected in general)

In T418122#11640803, @Urbanecm wrote:

In T418122#11640535, @kostajh wrote:

GlobalRenameUser::getJob doesn't access $options['type'], so the default parameter in LocalRenameUserJob (used for vanishing as well) is GlobalRenameRequest::RENAME when it should be GlobalRenameRequest::VANISH. That should be sufficient to address this, because then we'll hit the code path that uses User::newSystemUser( $renamedUser->getName(), [ 'steal' => true ] );

Note this has wider effects than email: It also scrubs gu_password and user_password, making the actions irreversible. FWIW, this came up in the context of https://meta.wikimedia.org/wiki/Talk:Account_vanishing#c-Cabayi-20260223143200-Reversal_/_Unvanishing, where the reversibility is being discussed. (No objections towards deploying that though, I just want to make sure those consequences are known/expected in general)

I think that's OK, since that was already happening on the central wiki?

Are we still ok with the patch from T418122#11640588? It seems reasonable to me. If folks are still ok with this approach for now, we could likely get this deployed during today's security deployment window.

The local password tables are empty on CentralAuth wikis anyway. The first system-user-ification will scrub the password from globaluser; that seems like a good thing from a privacy / data removal rights perspective.

In T418122#11641296, @sbassett wrote:

Are we still ok with the patch from T418122#11640588? It seems reasonable to me. If folks are still ok with this approach for now, we could likely get this deployed during today's security deployment window.

Yes, I think we should do it.

Deployed to 1.46.0-wmf.16. Looks stable.

This change caused vanished accounts to be labelled as "system users" in the wiki interface, e.g. on Special:UserRights, which was filed as T420848: AccountVanishing is marking endusers as system users. I was quite confused what's happening until I found this task with its security patch.

A few people in that discussion pointed out that we had some ambiguous old decisions about *not* removing passwords from vanished users. I filed T421525: Should vanishing an account remove its password and other login credentials? with a sketch of the history, so that we can once again and finally decide what should happen.

Any objections for this patch to go through Gerrit?

In T418122#11777791, @Reedy wrote:

Any objections for this patch to go through Gerrit?

Not from me.

Change #1267098 had a related patch set uploaded (by Reedy; author: Kosta Harlan):

[mediawiki/extensions/CentralAuth@master] SECURITY: Forward 'type' option to LocalRenameUserJob for vanishes

https://gerrit.wikimedia.org/r/1267098

Change #1267098 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@master] SECURITY: Forward 'type' option to LocalRenameUserJob for vanishes

https://gerrit.wikimedia.org/r/1267098

Change #1267108 had a related patch set uploaded (by Reedy; author: Kosta Harlan):

[mediawiki/extensions/CentralAuth@REL1_45] SECURITY: Forward 'type' option to LocalRenameUserJob for vanishes

https://gerrit.wikimedia.org/r/1267108

Change #1267110 had a related patch set uploaded (by Reedy; author: Kosta Harlan):

[mediawiki/extensions/CentralAuth@REL1_44] SECURITY: Forward 'type' option to LocalRenameUserJob for vanishes

https://gerrit.wikimedia.org/r/1267110

Change #1267111 had a related patch set uploaded (by Reedy; author: Kosta Harlan):

[mediawiki/extensions/CentralAuth@REL1_43] SECURITY: Forward 'type' option to LocalRenameUserJob for vanishes

https://gerrit.wikimedia.org/r/1267111

Change #1267108 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@REL1_45] SECURITY: Forward 'type' option to LocalRenameUserJob for vanishes

https://gerrit.wikimedia.org/r/1267108

Change #1267110 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@REL1_44] SECURITY: Forward 'type' option to LocalRenameUserJob for vanishes

https://gerrit.wikimedia.org/r/1267110

Change #1267111 merged by jenkins-bot:

[mediawiki/extensions/CentralAuth@REL1_43] SECURITY: Forward 'type' option to LocalRenameUserJob for vanishes

https://gerrit.wikimedia.org/r/1267111