Right, this relates to the comment

// TODO Should use undeleteIfAllowed instead.

that I left in SpecialUndelete. I didn't do that at first because the permission-checking logic in SpecialUndelete is quite complex and fragmentated (see SpecialUndelete::isAllowed and all the places where it's used). I didn't realize that this way, permissions are not checked for the associated talk page.

In T305680#7840541, @Daimona wrote:

Right, this relates to the comment

// TODO Should use undeleteIfAllowed instead.

that I left in SpecialUndelete. I didn't do that at first because the permission-checking logic in SpecialUndelete is quite complex and fragmentated (see SpecialUndelete::isAllowed and all the places where it's used). I didn't realize that this way, permissions are not checked for the associated talk page.

I completely missed that comment. And yes, this is all over the place and hard to "properly" fix without doing some more refactoring.
I've made some changes here https://gerrit.wikimedia.org/r/c/mediawiki/core/+/778494. Let me know what you think

And now I'm just realizing this is a security task. Not sure what to do since I've already uploaded the patch to gerrit. This is a bit of an edge case so I hope it is not a big deal

I think if we just want a quick fix that can also be quickly deployed, replacing undeleteUnsafe with undeleteIfAllowed should be enough. Some checks will be performed twice, but twice is better than never. As for fixing it properly... I don't know how difficult it would be.

In T305680#7840690, @Daimona wrote:

I think if we just want a quick fix that can also be quickly deployed, replacing undeleteUnsafe with undeleteIfAllowed should be enough. Some checks will be performed twice, but twice is better than never. As for fixing it properly... I don't know how difficult it would be.

Good point. I've abandoned the other one and I'll put up the new patch soon (the right way this time :S)

In T305680#7840626, @dmaza wrote:

I completely missed that comment. And yes, this is all over the place and hard to "properly" fix without doing some more refactoring.
I've made some changes here https://gerrit.wikimedia.org/r/c/mediawiki/core/+/778494. Let me know what you think
...
And now I'm just realizing this is a security task. Not sure what to do since I've already uploaded the patch to gerrit. This is a bit of an edge case so I hope it is not a big deal
...
Good point. I've abandoned the other one and I'll put up the new patch soon (the right way this time :S)

I mean, the change set and the associated files were already up on gerrit and are still publicly viewable. So it's likely fine to just push this through gerrit at this point, since we can't walk any of that back. I also don't personally feel this is a risky enough issue to worry about it potentially being publicly known for a few days prior to any merge/deploy. If there are stronger concerns about this, we can always send the patch up to gerrit and pick it to 1.39.0-wmf.6 and deploy it today. But if it causes any problems, it'll have to come off and wait for further testing/review.

In T305680#7841223, @sbassett wrote:

I mean, the change set and the associated files were already up on gerrit and are still publicly viewable. So it's likely fine to just push this through gerrit at this point, since we can't walk any of that back. I also don't personally feel this is a risky enough issue to worry about it potentially being publicly known for a few days prior to any merge/deploy. If there are stronger concerns about this, we can always send the patch up to gerrit and pick it to 1.39.0-wmf.6 and deploy it today. But if it causes any problems, it'll have to come off and wait for further testing/review.

In that case let's continue in gerrit and avoid deploying on a Friday. No need to bother/involve other folks if there is no urgency.

Change 778547 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] SpecialUndelete: check permissions again when undeleting

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

In T305680#7841394, @dmaza wrote:

In that case let's continue in gerrit and avoid deploying on a Friday. No need to bother/involve other folks if there is no urgency.

Sounds good. I'll note that I am around and happy to deploy today. I'd just note that if we see any spikes on an mwdebug or in logstash, the patch will have to get reverted immediately. Unfortunately, we do see this behavior with security patches on occasion (T285159#7812740 for a recent example.) Otherwise, getting it merged before Monday afternoon and having ride next week's train should work fine.

Change 778547 merged by jenkins-bot:

[mediawiki/core@master] SpecialUndelete: check permissions again when undeleting

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

I can no longer undelete the associated talk page if I am blocked from that talk page.

Instead, I get the below error and neither the content page nor its talk page are undeleted.

Some or all of the undeletion failed:

 * You cannot undelete this page as you are not allowed to edit this page.
 * You cannot undelete this page as there is no existing page with this name and you are not allowed to create this page.
 * Your username or IP address is blocked from doing this. You may still be able to do other things on this site, such as editing certain pages. You can view the full block details at account contributions.

The block was made by <blocking admin>.

The reason given is <block reason>.

    Start of block: <time block made>
    Expiration of block: <time block expires>
    Intended blockee: <blocked user>
    Block ID <block id>

I tested this with a few different types of blocks for files and for regular articles.

The same is also true if I am protected from creating an already deleted talk page.

Moving into this sprint for visibility.

Test Environments:

• https://en.wikipedia.beta.wmflabs.org MediaWiki 1.39.0-alpha (2425e00) 06:50, 11 April 2022.
• local docker MediaWiki 1.39.0-alpha (2425e00) 06:50, 11 April 2022 (for testing the protected scenario).

@sbassett I've found the very same issue when deleting a file. Only that this time it can only be done via the API (even less likely to be exploited IMO).

Is it ok to also handle it in this task and via gerrit if we merge it today before the cut?

In T305680#7844675, @dmaza wrote:

@sbassett I've found the very same issue when deleting a file. Only that this time it can only be done via the API (even less likely to be exploited IMO).

Is it ok to also handle it in this task and via gerrit if we merge it today before the cut?

Yes, that's fine. Let's definitely try to get it merged today.

Change 778640 had a related patch set uploaded (by SBassett; author: Daimona Eaytoy):

[mediawiki/core@REL1_38] SpecialUndelete: check permissions again when undeleting

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

Change 779075 had a related patch set uploaded (by Dmaza; author: Dmaza):

[mediawiki/core@master] SpecialDelete: check permissions again when deleting

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

Change 778640 merged by jenkins-bot:

[mediawiki/core@REL1_38] SpecialUndelete: check permissions again when undeleting

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

Change 779075 merged by jenkins-bot:

[mediawiki/core@master] SpecialDelete: check permissions again when deleting

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

I could not delete or undelete via the API when blocked from the associated talk page.

I tried for pages in as many different namespaces as I could find on beta[1].

I could not test the UI, as we have reverted that patch.

When attempting to delete with the deletetalk parameter, the API returned the response:

{
    "error": {
        "code": "blocked",
        "info": "You have been blocked from editing this page.",
        "blockinfo": {
            ...
        },
...
}

This is with the exception of trying to delete a File, where the API returned:

{
    "error": {
        "code": "cantdelete",
        "info": "The page or file \"File:Before required.png\" could not be deleted. It may have already been deleted by someone else.",
        "*": "See https://en.wikipedia.beta.wmflabs.org/w/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at <https://lists.wikimedia.org/postorius/lists/mediawiki-api-announce.lists.wikimedia.org/> for notice of API deprecations and breaking changes."
    },
    "servedby": "deployment-mediawiki11"
}

When attempting to undelete with the undeletetalk parameter, the API returned:

{
    "error": {
        "code": "undelete-cantedit",
        "info": "You cannot undelete this page as you are not allowed to edit this page.",
        "*": "See https://en.wikipedia.beta.wmflabs.org/w/api.php for API usage. Subscribe to the mediawiki-api-announce mailing list at <https://lists.wikimedia.org/postorius/lists/mediawiki-api-announce.lists.wikimedia.org/> for notice of API deprecations and breaking changes."
    },
    "servedby": "deployment-mediawiki11"
}

Test environment: https://en.wikipedia.beta.wmflabs.org and https://commons.wikimedia.beta.wmflabs.org MediaWiki 1.39.0-alpha (fd4387d) 09:33, 12 April 2022.

Notes

1. Namespaces included:
   • Talk
   • File_talk
   • User_talk
   • Wikipedia_talk
   • MediaWiki_talk
   • Template_talk
   • Help_talk
   • Category_talk
   • Flow_test_talk
   • Portal_talk
   • Draft_talk
   • Module_talk
   • Config_talk
   • Data_talk (commons)
   • GWToolset_talk (commons)
   • TimedText_talk (commons)
   • Translations_talk (commons)
   • Creator_talk (commons)
   • Institution_talk (commons)
   • Campaign_talk (commons)

I'm not seeing anything obvious that should prevent this task from becoming public at this point. If anyone sees anything, please let me know.

In T305680#7851643, @sbassett wrote:

I'm not seeing anything obvious that should prevent this task from becoming public at this point. If anyone sees anything, please let me know.

Nope, I think we are good

Change 780634 had a related patch set uploaded (by MusikAnimal; author: Dmaza):

[mediawiki/core@REL1_38] SpecialDelete: check permissions again when deleting

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

Change 780634 merged by jenkins-bot:

[mediawiki/core@REL1_38] SpecialDelete: check permissions again when deleting

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