The patch to maintain-replicas has my +1. Please poke me when you want it applied or - alternately - anyone can do so from root on labstore1001 (the operations/software tree is cloned into root's home):

# ./maintain-replicas.pl -u image

(-u image tells the tool to forcibly recreate the view in all databases where it exists).

@Tgr, sorry for the slow review!

In testing this, I'm seeing something odd with the File: page and caching. When the same user uploads multiple versions of a file, then I block + hide that user, on the File: page, the old versions of the file immediately have the correct "(username removed)" for the username, but the current revision still has the username visible. I can sometimes add purge=1 to the url to get it to correctly hide the username, but sometimes I have to flush memcache.

So it seems like the archived versions of the file are flushing the File object out of cache, but the current version isn't. Any idea how to fix that?

I don't think old versions are cached at all (OldLocalFile::getCacheKey() should disable that). I don't see an easy fix - RevisionDeleteUser uses mass DB updates to set the flags, and there is no equivalent for the cache, so the only thing I can think of is a job that iterates through all images of the user and invalidates them. (Which means that for global suppression, a job will iterate through the wikis and fire off a thousand other jobs which iterate through the images... not sure if that would cause any complications. If it's problematic, we could check if the DB update touched any rows and only fire the job when needed.)

@aaron, any ideas on how to deal with this caching issue?

Why not just have RevisionDeleteUser iterate over the image rows to be changed and call invalidateCache()? That should be able to handle a fair number of edits (and it's not likely such users will have many files). AFAIK, the doLocalSuppression call can already run in jobs. If we just removed $wgCentralAuthWikisPerSuppressJob and always used the queue for simplicity, and pushed the jobs to the local wikis rather than the central queue, the cache purges would work. CentralAuthSuppressUserJob looks like it would work on local wikis.

I made https://gerrit.wikimedia.org/r/212579 to speed up such purges a bit.

Did that, don't really like the result:

@aaron, couldn't we expose the shared repo cache key to other wikis instead?

Fixed a stupid mistake in the Labs replication patch.

@Tgr, I applied

How do you trigger a local suppression block? RevisionDeleteUser::invalidateCache() needs to be called, and right now that's the responsibility of the code that calls the other RevisionDeleteUser methods (and I only did it for CentralAuth). Maybe it should be called by those methods instead; I didn't want to do that because those work when called cross-wiki and invalidateCache does not and that would have been confusing.

Just do a normal block on a username, and there's an option to also
suppress the username, if you have the right (iirc).

@csteipp: updated the patch (it's in the task summary). Looks like undoing a suppress block via Special:Block is currently broken, the patch fixes that as well.

In T90300#1332550, @Tgr wrote:

Did that, don't really like the result:

0003-SECURITY-Invalidate-cached-data-on-user-suppression.patch1 KBDownload

0003-SECURITY-Launch-a-separate-user-suppression-job-for-each-wiki.patch4 KBDownload

@aaron, couldn't we expose the shared repo cache key to other wikis instead?

Yes, though I think we should be careful about cross-wiki code and the existing pattern seems a bit like a mistake. It's already there though and building on it is not intolerable though. In any case it should either fully work crosswiki or not pretend to.

There was a stray comma in the sql patchfile.

@Tgr, thanks for the new patches, I confirmed the issue is fixed now!

I'm a little worried about the move to only using jobs in centralauth-- @aaron, do those run in their own queue so we can be sure that they're going to get run soon after the user is hidden?

If so, I'm happy to go ahead and deploy these.

In T90300#1445764, @csteipp wrote:

@Tgr, thanks for the new patches, I confirmed the issue is fixed now!

I'm a little worried about the move to only using jobs in centralauth-- @aaron, do those run in their own queue so we can be sure that they're going to get run soon after the user is hidden?

If so, I'm happy to go ahead and deploy these.

They don't, just the regular high-priority queue. It might makes sense to leave it that way unless it's too slow. If we have slow jobs in the high priority queue that back up others, then they should be moved the low priority queue rather than making new queue loops, generally speaking.

@jcrespo / @Springle,

This all looks good from the security side, I'd like to start getting the fixes rolled out. Tgr outlined the task description, but basically,

WMF cluster update plan

1. run foreachdb sql.php /home/tgr/T90300-add-img_deleted.sql
2. apply all patches
3. run foreachdb sql.php /home/tgr/T90300-populate-img_deleted.sql
4. run script via foreachwiki eval.php to invalidate cache (if we care enough):

Can we start working on #1?

Unless @Springle is against it, I am ok with the intended work, I would be against the method, specially for commons which has 36 million rows on image and that would disable file uploads for over 1 hour, plus create a 1 hour lag on the slaves. For other wikis with no images or a small number, it is ok (<1000 rows).

I would recommend using pt-online-schema change for the larger tables (this table has a PRIMARY KEY). We can take care of this, with the usual questions (where and when?).

For the updates, make sure they are done in <1 sec steps, perform several of them otherwise.

Re: query optimization, there is a big difference between the IN () and the JOIN version (apparently MySQL is really stupid about handling uncorrelated IN subqueries), so using joins and the (both faster and more correct) username-based joins:

This is fixed on 5.6/10. We recently dumped 5.5 on the slaves, and will do on the masters soon.

In T90300#1492489, @jcrespo wrote:

Unless @Springle is against it, I am ok with the intended work, I would be against the method, specially for commons which has 36 million rows on image and that would disable file uploads for over 1 hour, plus create a 1 hour lag on the slaves. For other wikis with no images or a small number, it is ok (<1000 rows).

I would recommend using pt-online-schema change for the larger tables (this table has a PRIMARY KEY). We can take care of this, with the usual questions (where and when?).

@jcrespo, I'd be happy for you or Sean to do this. It would need to be applied to all wikis, whenever you're able to schedule it.

For the updates, make sure they are done in <1 sec steps, perform several of them otherwise.

I'm not exactly clear on what you're suggesting here-- are you saying setup a limit and repeatedly run this where each run finishes under 1 second? Something like running,

UPDATE image JOIN ipblocks ON img_user_text = ipb_address
SET img_deleted = img_deleted | 12
WHERE ipb_deleted AND img_deleted = 0
LIMIT <something sane that finishes under 1 second>;

Will apply the change, report back when finished.

I'm not exactly clear on what you're suggesting here-- are you saying setup a limit and repeatedly run this where each run finishes under 1 second? Something like running,

UPDATE image JOIN ipblocks ON img_user_text = ipb_address
SET img_deleted = img_deleted | 12
WHERE ipb_deleted AND img_deleted = 0
LIMIT <something sane that finishes under 1 second>;

That would probably work. If you want to be 100% sure of changes being applied in less than 1 second, page using the primary key (LIMIT could take sometimes too much time if a full table scan or large range is done). Take a 1 second pause between updates or monitor the lag during its application. If that is too difficult, ask for help to the DBAs after code is applied.

@csteipp I've deployed it to testwiki. Sincerely, this seems like a very strightforward change (except for large tables), but if you want to do a final check or test my previous comment suggestions there (for example, 1000 rows updated at a time is a good starting point), I will wait a day before applying it to the rest of the wikis.

Deployment update:

The schema change was applied to all wikis in an online fashion (pt-table-checksum), with the exception of commonswiki. The reason for that is that due to the high SELECT rate of that table on commons, the change created high contention due to metadata locking, so I stopped and rolled back the change. Please note that the issue here is the schema change, not the resulting schema.

I am now applying the change in a rolling-update fashion, depooling and repooling one server at a time -hopefully that will avoid the load issues. The issue with this method is that the change takes between 2-4 hours per server and has to be mostly serialized, so it will probably take me over a day to complete it (but is is ongoing right now).

ALTER TABLE image ADD img_deleted tinyint unsigned NOT NULL default 0; (or more precisely, a complex list of operations with the same result but without affecting production) has been run on ALL WMF wikis.

I suppose that concludes the first step.

Be careful with Commons: there, image is a very busy 30-million row table, and even simple operations can generate lag.

In T90300#1514580, @jcrespo wrote:

ALTER TABLE image ADD img_deleted tinyint unsigned NOT NULL default 0; (or more precisely, a complex list of operations with the same result but without affecting production) has been run on ALL WMF wikis.

You missed labswiki, which I just did (as there's only 260 rows there).

In my search to find out why this column had appeared on some wikis but not labswiki, I came across T38497 and T22742 which also suggested adding this column years ago.

@Tgr Just to clarify, are you still working on this/Does anyone know the status?

I think this was just waiting for the patches to be deployed (but that was 3 years ago so they need rebase / re-review now), not sure where / why exactly that got stuck. Are you still interested in getting it out? I'll be mostly on vacation until end of next week, can look at it after that.

@sbassett is the Security team interested in seeing this through? If not, can it be made public? (It's an infoleak, but a very minor one, and not triggered by user action, so public knowledge of the bug wouldn't make much difference.)
It was a nontrivial amount of work, and the patch clears up a significant amount of tech debt in the image table, so I'd like to see it through one way or another (now that I remember it exists... thanks for the reminder, @Aklapper).

@Tgr - moving this to our clinic meeting today. If the work can go through gerrit sooner than later, that sounds like the easier path to get it deployed, especially for what I'd personally consider a fairly low-risk vulnerability.

@Tgr - Security-Team is fine with this being public, given what we perceive to be a fairly minor information disclosure, especially if the work can proceed sooner than later within gerrit. I'll make this public now.

Change 609798 had a related patch set uploaded (by Reedy; owner: CSteipp):
[mediawiki/core@master] Add img_deleted column

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

Seems the img_deleted column was only recently removed from WMF wikis... T250055: Remove image.img_deleted column from production

In T90300#6282177, @sbassett wrote:

@Tgr - Security-Team is fine with this being public, given what we perceive to be a fairly minor information disclosure, especially if the work can proceed sooner than later within gerrit. I'll make this public now.

Thanks! Will try to re-test things in the next week or two.

Change #609798 abandoned by Reedy:

[mediawiki/core@master] Add img_deleted column

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