Tagging Moderator-Tools-Team as the team that implemented the feature (@Samwalton9-WMF, adding you to this task FYI)

Hiding the target of all relevant log entries does not hide the prior existence of a page. That information is still (intentionally) publicly available through other means, e.g., the API.

In T385792#10529352, @JJMC89 wrote:

Hiding the target of all relevant log entries does not hide the prior existence of a page. That information is still (intentionally) publicly available through other means, e.g., the API.

If you're referring to the API methods that I assume you're referring to, that's currently the subject of security issue {T356495}, and the fact that that task hasn't yet been resolved doesn't imo mean that this additional infoleak shouldn't be rectified.
If you're referring to other API methods, I'd appreciate it if you could let me know what they are, so that I can comment on them/their relation to this task more specifically.

I don't have time to investigate in detail right now but it would be good to confirm if this is solving a real security gap, or if it's possible to know a revdel'd page existed through other means, before we proceed.

In T385792#10531807, @Samwalton9-WMF wrote:

I don't have time to investigate in detail right now but it would be good to confirm if this is solving a real security gap, or if it's possible to know a revdel'd page existed through other means, before we proceed.

From my perspective as the task filer, I'm not personally aware of other means that allow for revealing the previous existence of log-deleted/log-suppressed titles that aren't themselves security issues (given that MediaWiki confirming this would - as far as I can tell - be a leak of the information that had been rev-deleted/suppressed). However, I'll let others confirm whether or not my understanding in this regard is correct.

I'm moving this to our code review request queue; though note that we are waiting for more feedback on whether this is a change that must be made.

I don't mean to unduly push anyone in any direction and/or rush anything, but - if this is determined to be a security issue - it would IMO be better to merge a fix prior to the release of MW 1.44, to avoid having to include it in a security release at a later date/so that the issue’s not present in any numbered release of MediaWiki. (Tagging with MW-1.44-release for increased visibility.)

(FWIW, even if this is determined to not be a security issue, I think I'd still support making the proposed change -- in a situation where there's no previous public deletion log entry for a page in question, it could IMO be confusing for a new page at that title to be tagged as 'Recreated' without an obvious/clear reason for it to have been tagged in this way.)

(tagging MediaWiki-API-Platform-Team as @Maintenance_bot would have done if it could see this task)

(responding to workboard movements)
FWIW, my current proof-of-concept patch needs an update to reflect the changes that have occurred to the codebase since then. Let me know if you’d like me to prepare an updated patch (based on what I’ve done in the current one).

@sbassett Is this a security issue that needs to be addressed?

In T385792#10765620, @Kgraessle wrote:

@sbassett Is this a security issue that needs to be addressed?

Yes, it's a low-risk Vuln-Infoleak. Though since the affected code is a part of core, it should be security-deployed (typically during the Monday windows) and tracked for the next security release (T389302). I'd give a CR+1 to @A_smart_kitten's patch from T385792#10528761, but it'd be nice if someone else could give a CR+2 here on the private task, and then we can stage it for a security-deploy.

To be clear, I'm fairly sure that the current patch isn't ready for deployment, due to changes in the relevant code since this task was filed. As it sounds like a patch for this issue is likely desired now, though, I can get to working on an updated one & uploading it here when I next get an opportunity (hopefully in the next several days). If a patch is desired/needed sooner, though, any engineer is of course welcome to build on my existing patch/to code a new one, & I'll try and leave a review for it :)

[…] and tracked for the next security release (T389302).

I am probably misunderstanding something, but - as this issue hasn't yet (to my knowledge) been present in a public MediaWiki release, couldn't the fix be merged into master (after being security-deployed) & released with MW 1.44?

In T385792#10766583, @A_smart_kitten wrote:

To be clear, I'm fairly sure that the current patch isn't ready for deployment, due to changes in the relevant code since this task was filed. As it sounds like a patch for this issue is likely desired now, though, I can get to working on an updated one & uploading it here when I next get an opportunity (hopefully in the next several days). If a patch is desired/needed sooner, though, any engineer is of course welcome to build on my existing patch/to code a new one, & I'll try and leave a review for it :)

Doesn't seem like there were many conflicts? Just the new use statement for MediaWiki\ChangeTags\ChangeTags from https://gerrit.wikimedia.org/r/q/I3cf44dfe5 and the new test from https://gerrit.wikimedia.org/r/q/I0a4ddc418. This revised patch seems to apply, though let me know if I missed something:

I am probably misunderstanding something, but - as this issue hasn't yet (to my knowledge) been present in a public MediaWiki release, couldn't the fix be merged into master (after being security-deployed) & released with MW 1.44?

Generally speaking, we like to hold security issues for mw core and bundled extensions for the proper security releases that @Reedy does at the end of each quarter, unless there's an extremely compelling reason to release and backport them early.

But if it was only introduced in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1091761 which hasn't made it into a release yet...

@A_smart_kitten I went ahead and refactored your PR to make sure we're always checking the most recent deletion log entry and added one additional test.

I do have a question around the behavior of this with an article that has multiple deletion log records; some of which are hidden and some of which are not.
In this case we wouldn't be tagging the page as recreated if the most recent log entry was hidden, but we would if the most recent log entry was not hidden.
I'm not sure if this is the desired behavior though because when I go to recreate the page I still see the following:

@A_smart_kitten @jsn.sherman @Scardenasmolinar
If you all can give this a look and +2 whenever you get the chance, and thanks @A_smart_kitten for all your work on this!

I went ahead and refactored your PR to make sure we're always checking the most recent deletion log entry [...]

Only just seen this so I still have to fully read it over (I'll probably use some time over the weekend to do so, and to remind myself of the code we're dealing with here!), but a couple of things come to me right now:

• I'm confused as to why we always want to check the most recent deletion log entry? As you say, this produces potentially confusing behaviour when the title of the page has been hidden from the most recent deletion log entry but not from an older one. (Am I missing something here? /gen)
• Regarding the $deletionLogEntry->log_deleted == 0 check: from my perspective, I think it should be fine to apply the 'Recreated' tag in some circumstances where log_deleted is not 0; e.g. in a circumstance where the identity of the user deleting the page has been hidden but the title of the page is still publicly viewable.

In T385792#10766815, @Reedy wrote:

But if it was only introduced in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1091761 which hasn't made it into a release yet...

Oh, yeah, if it never made it to a release, then I guess it's fine to release it early, once we're patched in production.

In T385792#10766740, @sbassett wrote:

In T385792#10766583, @A_smart_kitten wrote:

To be clear, I'm fairly sure that the current patch isn't ready for deployment, due to changes in the relevant code since this task was filed. As it sounds like a patch for this issue is likely desired now, though, I can get to working on an updated one & uploading it here when I next get an opportunity (hopefully in the next several days). If a patch is desired/needed sooner, though, any engineer is of course welcome to build on my existing patch/to code a new one, & I'll try and leave a review for it :)

Doesn't seem like there were many conflicts? Just the new use statement for MediaWiki\ChangeTags\ChangeTags from https://gerrit.wikimedia.org/r/q/I3cf44dfe5 and the new test from https://gerrit.wikimedia.org/r/q/I0a4ddc418.

Apologies @sbassett, I think you might be right! I don't recall exactly what was in my mind a few months ago when I wrote in T385792#10528761 that the patch would "probably need to be rebased/modified slightly".
Looking at it now, though, my immediate thoughts are that I might have been referring to:

• resolving any merge conflicts caused by https://gerrit.wikimedia.org/r/1117255,
• removing the $deleteLogEntry->setDeleted( LogPage::DELETED_ACTION ); call from this line of the above-linked patch (as IIUC it causes the test to check for the presence of the current infoleak-ing behaviour), and
• renaming the new test from testDoUpdatesTagsEditAsRecreatedWhenDeletedLogEntryAndUndelete to testDoUpdatesDoesNotTagEditAsRecreatedWhenDeletionLogEntryAndUndelete (to be clear on terminology, given that this test is checking for the absence of the 'Recreated' tag, and given that the tag shouldn't be applied for a log entry with its action deleted).

The latter two are probably (IIUC) technically not blockers for the patch to be security-deployed, though IMO it'd be nice to get them done beforehand (especially as the test's setDeleted() call would - IIUC - cause CI to fail if the patch was pushed through Gerrit).

In T385792#10768409, @A_smart_kitten wrote:

• I'm confused as to why we always want to check the most recent deletion log entry? As you say, this produces potentially confusing behaviour when the title of the page has been hidden from the most recent deletion log entry but not from an older one. (Am I missing something here? /gen)

Something else that's occurred to me about this: if we're only checking the most recent deletion log entry, that (IIUC) has the potential to introduce a separate infoleak (albeit in a bit of an edge case); as the software could then be making a decision on whether or not to apply a publicly-visible tag based on deletion log entries that aren't themselves publicly visible, which could then (in certain circumstances) reveal that the most recent deletion log entry for a given page has been hidden. (I can elaborate with a proof-of-concept if it'd be helpful, but I didn't want to unnecessarily lengthen this already-long Phabricator comment!)

In T385792#10772118, @A_smart_kitten wrote:

In T385792#10766740, @sbassett wrote:

In T385792#10766583, @A_smart_kitten wrote:

To be clear, I'm fairly sure that the current patch isn't ready for deployment, due to changes in the relevant code since this task was filed. As it sounds like a patch for this issue is likely desired now, though, I can get to working on an updated one & uploading it here when I next get an opportunity (hopefully in the next several days). If a patch is desired/needed sooner, though, any engineer is of course welcome to build on my existing patch/to code a new one, & I'll try and leave a review for it :)

Doesn't seem like there were many conflicts? Just the new use statement for MediaWiki\ChangeTags\ChangeTags from https://gerrit.wikimedia.org/r/q/I3cf44dfe5 and the new test from https://gerrit.wikimedia.org/r/q/I0a4ddc418.

Apologies @sbassett, I think you might be right! I don't recall exactly what was in my mind a few months ago when I wrote in T385792#10528761 that the patch would "probably need to be rebased/modified slightly".
Looking at it now, though, my immediate thoughts are that I might have been referring to:

• resolving any merge conflicts caused by https://gerrit.wikimedia.org/r/1117255,
• removing the $deleteLogEntry->setDeleted( LogPage::DELETED_ACTION ); call from this line of the above-linked patch (as IIUC it causes the test to check for the presence of the current infoleak-ing behaviour), and
• renaming the new test from testDoUpdatesTagsEditAsRecreatedWhenDeletedLogEntryAndUndelete to testDoUpdatesDoesNotTagEditAsRecreatedWhenDeletionLogEntryAndUndelete (to be clear on terminology, given that this test is checking for the absence of the 'Recreated' tag, and given that the tag shouldn't be applied for a log entry with its action deleted).

The latter two are probably (IIUC) technically not blockers for the patch to be security-deployed, though IMO it'd be nice to get them done beforehand (especially as the test's setDeleted() call would - IIUC - cause CI to fail if the patch was pushed through Gerrit).

---

In T385792#10768409, @A_smart_kitten wrote:

• I'm confused as to why we always want to check the most recent deletion log entry? As you say, this produces potentially confusing behaviour when the title of the page has been hidden from the most recent deletion log entry but not from an older one. (Am I missing something here? /gen)

Something else that's occurred to me about this: if we're only checking the most recent deletion log entry, that (IIUC) has the potential to introduce a separate infoleak (albeit in a bit of an edge case); as the software could then be making a decision on whether or not to apply a publicly-visible tag based on deletion log entries that aren't themselves publicly visible, which could then (in certain circumstances) reveal that the most recent deletion log entry for a given page has been hidden. (I can elaborate with a proof-of-concept if it'd be helpful, but I didn't want to unnecessarily lengthen this already-long Phabricator comment!)

Ok, yeah I think I understand now and this makes more sense. Disregard my patch, and thanks for clarifying.

I'm good to +2
the T385792-rev2.patch

Noting that I have a rev3 version of the patch that addresses the notes in my comment above (and also namespaces the use statement for LogPage); I'm just finishing checking it over and hopefully I'll have it uploaded here in the next couple hours if not before.

Revised version of my original patch/the -rev2 patch, to address the notes in my comment above (and also to add a namespace to the use LogPage statement, following https://gerrit.wikimedia.org/r/1128558). Apologies for the delay, I ended up discovering T392832 while testing - and apologies as well for posting this revised patch after the -rev2 version was given a +2!

git diff between the codebase with the -rev2 patch applied and the codebase with the -rev3 patch applied:

diff --git a/includes/Storage/DerivedPageDataUpdater.php b/includes/Storage/DerivedPageDataUpdater.php
index 8e0f9792f37..c66618a1ee0 100644
--- a/includes/Storage/DerivedPageDataUpdater.php
+++ b/includes/Storage/DerivedPageDataUpdater.php
@@ -23,7 +23,6 @@ namespace MediaWiki\Storage;
 use InvalidArgumentException;
 use LogicException;
 use MediaWiki\ChangeTags\ChangeTags;
-use LogPage;
 use MediaWiki\ChangeTags\ChangeTagsStore;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Content\Content;
@@ -41,6 +40,7 @@ use MediaWiki\HookContainer\HookRunner;
 use MediaWiki\JobQueue\JobQueueGroup;
 use MediaWiki\JobQueue\Jobs\ParsoidCachePrewarmJob;
 use MediaWiki\Language\Language;
+use MediaWiki\Logging\LogPage;
 use MediaWiki\MainConfigNames;
 use MediaWiki\Page\Event\PageRevisionUpdatedEvent;
 use MediaWiki\Page\PageIdentity;
diff --git a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
index 11064393b64..24c18e2e19a 100644
--- a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
+++ b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
@@ -1438,7 +1438,7 @@ class DerivedPageDataUpdaterTest extends MediaWikiIntegrationTestCase {
 	 * @covers \MediaWiki\Storage\DerivedPageDataUpdater::doUpdates()
 	 * @covers \MediaWiki\Storage\DerivedPageDataUpdater::maybeAddRecreateChangeTag
 	 */
-	public function testDoUpdatesTagsEditAsRecreatedWhenDeletedLogEntryAndUndelete() {
+	public function testDoUpdatesDoesNotTagEditAsRecreatedWhenDeletionLogEntryAndUndelete() {
 		$page = $this->getPage( __METHOD__ );
 		$title = $this->getTitle( __METHOD__ );
 		$user = $this->getMutableTestUser()->getUser();
@@ -1457,7 +1457,6 @@ class DerivedPageDataUpdaterTest extends MediaWikiIntegrationTestCase {
 		$deleteLogEntry = new ManualLogEntry( 'delete', 'delete' );
 		$deleteLogEntry->setPerformer( $this->getTestUser()->getUser() );
 		$deleteLogEntry->setTarget( $title );
-		$deleteLogEntry->setDeleted( LogPage::DELETED_ACTION );
 		$logId = $deleteLogEntry->insert( $this->getDb() );
 		$deleteLogEntry->publish( $logId );
 		// undelete the page

Ok, yeah I think I understand now and this makes more sense. Disregard my patch, and thanks for clarifying.

No problem! Thanks for your review :)

In T385792#10773266, @A_smart_kitten wrote:

Revised version of my original patch/the -rev2 patch, to address the notes in my comment above (and also to add a namespace to the use LogPage statement, following https://gerrit.wikimedia.org/r/1128558). Apologies for the delay, I ended up discovering T392832 while testing - and apologies as well for posting this revised patch after the -rev2 version was given a +2!

T385792-rev3.patch7 KBDownload

---

git diff between the codebase with the -rev2 patch applied and the codebase with the -rev3 patch applied:

diff --git a/includes/Storage/DerivedPageDataUpdater.php b/includes/Storage/DerivedPageDataUpdater.php
index 8e0f9792f37..c66618a1ee0 100644
--- a/includes/Storage/DerivedPageDataUpdater.php
+++ b/includes/Storage/DerivedPageDataUpdater.php
@@ -23,7 +23,6 @@ namespace MediaWiki\Storage;
 use InvalidArgumentException;
 use LogicException;
 use MediaWiki\ChangeTags\ChangeTags;
-use LogPage;
 use MediaWiki\ChangeTags\ChangeTagsStore;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Content\Content;
@@ -41,6 +40,7 @@ use MediaWiki\HookContainer\HookRunner;
 use MediaWiki\JobQueue\JobQueueGroup;
 use MediaWiki\JobQueue\Jobs\ParsoidCachePrewarmJob;
 use MediaWiki\Language\Language;
+use MediaWiki\Logging\LogPage;
 use MediaWiki\MainConfigNames;
 use MediaWiki\Page\Event\PageRevisionUpdatedEvent;
 use MediaWiki\Page\PageIdentity;
diff --git a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
index 11064393b64..24c18e2e19a 100644
--- a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
+++ b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php
@@ -1438,7 +1438,7 @@ class DerivedPageDataUpdaterTest extends MediaWikiIntegrationTestCase {
 	 * @covers \MediaWiki\Storage\DerivedPageDataUpdater::doUpdates()
 	 * @covers \MediaWiki\Storage\DerivedPageDataUpdater::maybeAddRecreateChangeTag
 	 */
-	public function testDoUpdatesTagsEditAsRecreatedWhenDeletedLogEntryAndUndelete() {
+	public function testDoUpdatesDoesNotTagEditAsRecreatedWhenDeletionLogEntryAndUndelete() {
 		$page = $this->getPage( __METHOD__ );
 		$title = $this->getTitle( __METHOD__ );
 		$user = $this->getMutableTestUser()->getUser();
@@ -1457,7 +1457,6 @@ class DerivedPageDataUpdaterTest extends MediaWikiIntegrationTestCase {
 		$deleteLogEntry = new ManualLogEntry( 'delete', 'delete' );
 		$deleteLogEntry->setPerformer( $this->getTestUser()->getUser() );
 		$deleteLogEntry->setTarget( $title );
-		$deleteLogEntry->setDeleted( LogPage::DELETED_ACTION );
 		$logId = $deleteLogEntry->insert( $this->getDb() );
 		$deleteLogEntry->publish( $logId );
 		// undelete the page

---

Ok, yeah I think I understand now and this makes more sense. Disregard my patch, and thanks for clarifying.

No problem! Thanks for your review :)

Looks great to me, thanks for all your work.
+2

I think we can try to get the -rev3 patch out during today's security deployment window.

Ok, the -rev3 patch was deployed to Wikimedia production: https://sal.toolforge.org/log/q4xefpYB8tZ8Ohr0EzQ4. If someone could test when they have a chance, that would be great.

Security-Team, based on T385792#10768460, are we okay to push the -rev3 patch to master at some point in time for MW-1.44-release?

In T385792#10783494, @A_smart_kitten wrote:

Security-Team, based on T385792#10768460, are we okay to push the -rev3 patch to master at some point in time for MW-1.44-release?

Yes, that should be fine. I've subbed @gerritbot to this task.

Thanks @sbassett! To be clear, should I push it myself/would that normally be done by a Security-Team member? Just to avoid accidentally stepping on each others' toes 😅

In T385792#10784044, @A_smart_kitten wrote:

Thanks @sbassett! To be clear, should I push it myself/would that normally be done by a Security-Team member? Just to avoid accidentally stepping on each others' toes 😅

You can push a change set to master in gerrit, if you'd like. In general, I think the primary role of the Security-Team for these things is to make the determination as to whether a core patch like this should be early-released or not. And then anyone can push up the change set(s).

Fair enough :) I'll push the patch to master in Gerrit when I get a chance to do so; hopefully later today, if not then some time tomorrow. When I do push it, I'll leave a note on Gerrit that the Security-Team has signed-off on uploading it to Gerrit (given the normal rules about patching security issues, to hopefully prevent any worries about an inadvertent disclosure!).

Change #1140545 had a related patch set uploaded (by A smart kitten; author: A smart kitten):

[mediawiki/core@master] SECURITY: Don't add the 'Recreated' tag if the target of a title's deletion log entry is hidden

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

Change #1140545 merged by jenkins-bot:

[mediawiki/core@master] SECURITY: Don't add the 'Recreated' tag if the target of a title's deletion log entry is hidden

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

thanks @SecurityPatchBot for the alert, but this is presumably just because the patch has been merged into master :) presumably someone just needs to remove the patch from the server now.

(side note @jnuche, if the bot is flagging a closed security task as a train blocker, should it also reopen the task in question for additional visibility? i’m just thinking to avoid the potential risk of people seeing an already-closed train-blocker-subtask, and - based on its 'closed' status - thinking that the train-blocking issue has already been resolved.)

thanks @SecurityPatchBot for the alert, but this is presumably just because the patch has been merged into master :) presumably someone just needs to remove the patch from the server now.

Yeah, the bot considers that possibility and provides a command for that in its message; note the message mentions version 1.45.0-wmf.1 because that's what was in the Release Version field of T386223, I've changed it now to 1.44.0-wmf.28.

(side note @jnuche, if the bot is flagging a closed security task as a train blocker, should it also reopen the task in question for additional visibility? i’m just thinking to avoid the potential risk of people seeing an already-closed train-blocker-subtask, and - based on its 'closed' status - thinking that the train-blocking issue has already been resolved.)

That makes sense to me. I've created T393361 to follow up.

@Reedy FYI, you subscribed GerritBot (the project) rather than @gerritbot (the account); which (from a brief test) I believe may have given the GerritBot project's members visibility of this hidden task.

In T385792#10965130, @A_smart_kitten wrote:

@Reedy FYI, you subscribed GerritBot (the project) rather than @gerritbot (the account); which (from a brief test) I believe may have given the GerritBot project's members visibility of this hidden task.

Shouldn't be an issue as we were patched in prod for some time and the patch was already public in gerrit.

Sure, just thought I should mention it just in case :)

Thanks @sbassett :)