Poked around a bit and found two more missing block checks (in Special:ManageMessageGroups and ApiAggregateGroups) that would allow groups to be created, deleted, or modified by blocked users. This patch should fix all three.

@Nikerabbit: Would you maybe know how could review the attached patch? TIA! :-/

Thanks @AntiCompositeNumber. I've reviewed the patch and I'm wondering if we should add a check in ManageGroupSynchronizationCacheActionApi as well. I've also updated the patch to apply cleanly with the latest master branch.

I'm unfamiliar with that section of the code, and I don't think my local test environment is set up to test it. But reading the code, it looks like it would make sense to also have the same test there.

These checks look good, but I think we should also add a check that if a user is blocked from editing a page, then they cannot submit translations for that page. I've updated the patch-set to do this:

Submitting another patch after some testing. Ready for code review:

In absence of the someone like @Nikerabbit reviewing this, happy to stamp my +2 on T302479#8001202.

@sbassett I think this dropped off the radar, could we get the patch deployed?

This fell off my radar. I think the patch is fine except I'm not fully confident about the changes to PageTranslationHooks (btw, there is patch to move this class, so this patch will need to be rebased very soon). My concerns are:

1. It moves the fast return away from the top. Since this is the "expensive" hook variant, that is probably okay.
2. It adds a new feature that if user is blocked from editing a translatable page, this block is automatically considered for the translations as well This makes sense to me, however it may be better to do it as a separate patch and cover translatable bundles. Some docs would also be nice since now the hook callback is doing multiple things and the existing docs don't cover this new feature.
3. For non-translatable pages it seems to duplicate checking which should be done by MediaWiki core. I think that part should be removed with confirmation that core actually takes care of it.

In T302479#8183826, @Legoktm wrote:

In absence of the someone like @Nikerabbit reviewing this, happy to stamp my +2 on T302479#8001202.

@sbassett I think this dropped off the radar, could we get the patch deployed?

Sounds like, per @Nikerabbit, we'd want to at least wait until https://gerrit.wikimedia.org/r/c/823618 gets merged, then rebase the patch. But their other points indicate the patch should likely be revisited in certain ways? I don't have enough domain knowledge to make that call. I'd note that we generally try to keep security patches as simple as possible and really just solve the specific issue at hand, with follow-up (and often public) patches for cleanup and related features. I'm happy to help deploy the above patch during a security window, but we should at least confirm the aforementioned issues and how we'd like to manage them at this time.

Here's an updated patch:

I'll create a separate task (non security) for,

New feature that if user is blocked from editing a translatable page, this block is automatically considered for the translations as well This makes sense to me, however it may be better to do it as a separate patch and cover translatable bundles. Some docs would also be nice since now the hook callback is doing multiple things and the existing docs don't cover this new feature.

Uh so I made a mistake and pushed this patch out to Gerrit while working on a subsequent task. I don't have the option of deleting a patchset from Gerrit.

In T302479#8231100, @abi_ wrote:

Uh so I made a mistake and pushed this patch out to Gerrit while working on a subsequent task. I don't have the option of deleting a patchset from Gerrit.

The patch is deleted now. Thanks to Antoine.

It was online for about 3 hours. During that time I had updated the latest patchset to remove all changes, and also removed the commit message. The patch was submitted in WIP so no email notifications would have gone out.

In T302479#8231388, @abi_ wrote:

The patch is deleted now. Thanks to Antoine.

It was online for about 3 hours. During that time I had updated the latest patchset to remove all changes, and also removed the commit message. The patch was submitted in WIP so no email notifications would have gone out.

Ok, thanks for letting us know and thanks, @hashar, for deleting the patch. While the commit message was somewhat innocuous, the content of the patch could obviously let an attacker know that the various translation layers could be abused by bypassing certain blocks, which are not currently checked. While that piece isn't great, I'm not sure I'd rate this issue more than a low risk for now, as the most obvious attack vectors would seem to be abuse at a limited scale? If I've missed the consideration of other vectors (realistic DoS, etc.) please let me know. Otherwise, we should likely plan to deploy this patch either this week (though I'd prefer to avoid the remaining trains) or during next Monday's security window. Would a member of the Language-Team be available to help test post-deploy?

The patch from T302479#8231007 was deployed to 1.40.0-wmf.1. No issues in logstash. Now tracking at T276237 and T311785.

In T302479#8246785, @Mstyles wrote:

The patch from T302479#8231007 was deployed to 1.40.0-wmf.1. No issues in logstash. Now tracking at T276237 and T311785.

Thanks, I've verified that marking the page for translation (Special:PageTranslation) and adding, editing and deleting an aggregate group (AggregateGroupsActionApi) works as expected. I'll deploy this patch on translatewiki.

In T302479#8247009, @abi_ wrote:

Thanks, I've verified that marking the page for translation (Special:PageTranslation) and adding, editing and deleting an aggregate group (AggregateGroupsActionApi) works as expected. I'll deploy this patch on translatewiki.

Great, thanks for the additional testing, @abi_.

Change 838940 had a related patch set uploaded (by Mstyles; author: Abijeet Patro):

[mediawiki/extensions/Translate@master] Adds missing block checks to various pages and API

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

Change 838824 had a related patch set uploaded (by Mstyles; author: Abijeet Patro):

[mediawiki/extensions/Translate@REL1_39] Adds missing block checks to various pages and API

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

Change 838824 merged by jenkins-bot:

[mediawiki/extensions/Translate@REL1_39] Adds missing block checks to various pages and API

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

Change 838940 merged by jenkins-bot:

[mediawiki/extensions/Translate@master] Adds missing block checks to various pages and API

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