Revision deleting a file does not update fr_deleted
Closed, ResolvedPublicSecurity
Actions

Assigned To

Authored By

	Zabe
	Apr 19 2026, 5:58 PM

Project Tags

Referenced Files

	F76781964: 0001-SECURITY-Correctly-support-new-file-tables-in-RevDel.patch
	Apr 20 2026, 2:31 PM

	F76776101: T423821.patch
	Apr 20 2026, 1:52 PM

	F76763098: 0001-SECURITY-Correctly-support-new-file-tables-in-RevDel.patch
	Apr 20 2026, 12:02 PM

	F76646997: 0001-RevDelFileItem-Correctly-support-new-file-tables.patch
	Apr 19 2026, 6:07 PM

Subscribers

Description

RevDelFileItem is missing the corresponding write new code. This results in T423065.

Details

Risk Rating: Medium
Author Affiliation: Wikimedia Communities

Related Changes in Gerrit:

Subject	Repo	Branch	Lines +/-
Correctly support new file tables in RevDelFileItem	mediawiki/core	REL1_44	+26 -9
Correctly support new file tables in RevDelFileItem	mediawiki/core	REL1_45	+26 -9
Correctly support new file tables in RevDelFileItem	mediawiki/core	master	+26 -9

Customize query in gerrit

Related Objects

Mentioned In: T424553: Fix fr_deleted drifts in WMF production
T423065: Stuck-hidden file / Deleted file revisions displaying improperly
Mentioned Here: T423654: Expectation (readQueryTime <= 5) by MediaWiki\Actions\ActionEntryPoint::execute not met (actual: {actualSeconds}) in trx #{trxId}:{query}
T423065: Stuck-hidden file / Deleted file revisions displaying improperly

Event Timeline

Zabe created this task.Apr 19 2026, 5:58 PM

Restricted Application added a project: Product Safety and Integrity. · View Herald TranscriptApr 19 2026, 5:58 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Zabe mentioned this in T423065: Stuck-hidden file / Deleted file revisions displaying improperly.Apr 19 2026, 5:59 PM

Zabe added projects: DBA, MediaWiki-File-management.

Novem_Linguae subscribed.Apr 19 2026, 6:00 PM

Zabe updated the task description. (Show Details)Apr 19 2026, 6:00 PM

Zabe added a subscriber: gerritbot.

We can probably just push this through Gerrit now that prod is not affected by this anymore.

0001-RevDelFileItem-Correctly-support-new-file-tables.patch2 KBDownload

Zabe moved this task from Triage to In progress on the DBA board.Apr 19 2026, 6:07 PM

Virtual +2, For future and after clean up of the old code, we can rely on fr_id instead of fr_name but that's not a blocker.

In T423821#11837067, @Zabe wrote:

We can probably just push this through Gerrit now that prod is not affected by this anymore.

0001-RevDelFileItem-Correctly-support-new-file-tables.patch2 KBDownload

if the change introducing the bug is not new, we still need to be careful about third parties but also at the same time. No third party is using the new file system I think (READ_NEW should be quite recent I assume)

$wgFileSchemaMigrationStage is not yet documented to actually support read new. The problem is actually that this is even a WRITE_NEW issue. If any third party wiki already set their wiki to WRITE_BOTH, they will have deviations in their filerevision table.

The next thing I just noticed is that the drift in fr_deleted also exposes a lot of rows in the wiki replicas which should be hidden.

Zabe claimed this task.Apr 19 2026, 6:38 PM

Do we need to write a maintenance script to fix fr_deleted fields that got out of sync with oi_deleted?

Might want to edit the ticket to include why this is a security issue. The bug in T423065 resulted in hidden files being clickable, but clicking on them gave an error message, so the hidden files were not actually viewable.

In T423821#11837295, @Novem_Linguae wrote:

Do we need to write a maintenance script to fix fr_deleted fields that got out of sync with oi_deleted?

Yes.

In T423821#11837303, @Novem_Linguae wrote:

Might want to edit the ticket to include why this is a security issue. The bug in T423065 resulted in hidden files being clickable, but clicking on them gave an error message, so the hidden files were not actually viewable.

It also did affect the other kinds of revision deletions. For example did I hide your username on the file you linked in your task (https://test.wikipedia.org/w/index.php?title=Special:Log&logid=461602) and it simply did not work.

Generally, _deleted rows are used in all kinds of permission checks and none of them will work here, so a bit of caution seems like a good idea.

0001-SECURITY-Correctly-support-new-file-tables-in-RevDel.patch2 KBDownload

Deployed this as a security patch for now (nothing changed, I just fixed the commit message).

Thanks!

Zabe added a comment.Apr 20 2026, 1:52 PM

This comment was removed by Zabe.

Meh. I thought I tested it, but my local wiki was set to write old, smart. Here a fixed version.

0001-SECURITY-Correctly-support-new-file-tables-in-RevDel.patch2 KBDownload

Instead of querying file table, you can use ->getFileIdFromName() instead. Much simpler.

In T423821#11839318, @Zabe wrote:

Meh. I thought I tested it, but my local wiki was set to write old, smart. Here a fixed version.

0001-SECURITY-Correctly-support-new-file-tables-in-RevDel.patch2 KBDownload

Did this also get deployed and replace the original patch? Or no? Also, can we just push this up to gerrit? Similar to T423654? I think the Security-Team is fine with that, cc: @Catrope.

In T423821#11845598, @sbassett wrote:

In T423821#11839318, @Zabe wrote:

Meh. I thought I tested it, but my local wiki was set to write old, smart. Here a fixed version.

0001-SECURITY-Correctly-support-new-file-tables-in-RevDel.patch2 KBDownload

Did this also get deployed and replace the original patch? Or no?

No I removed the first version and waited for a review of the new one.

Also, can we just push this up to gerrit? Similar to T423654? I think the Security-Team is fine with that, cc: @Catrope.

We can push it through Gerrit if folks are fine with it.

In T423821#11845699, @Zabe wrote:

Also, can we just push this up to gerrit? Similar to T423654? I think the Security-Team is fine with that, cc: @Catrope.

We can push it through Gerrit if folks are fine with it.