| Jdx | |
| Feb 24 2022, 9:35 AM |
| F35518530: T302479-8.patch | |
| Sep 13 2022, 2:17 AM |
| F35238527: T302479-7.patch | |
| Jun 14 2022, 6:29 AM |
| F35211704: T302479-6.patch | |
| Jun 6 2022, 12:27 PM |
| F35211705: image.png | |
| Jun 6 2022, 12:27 PM |
| F35211699: T302479-5.patch | |
| Jun 6 2022, 12:23 PM |
| F35211695: image.png | |
| Jun 6 2022, 12:23 PM |
| F35211669: T302479-4.patch | |
| Jun 6 2022, 12:07 PM |
| F35198456: T302479-3.patch | |
| Jun 2 2022, 2:41 AM |
I was blocked on Commons on 4 December 2021: https://commons.wikimedia.org/w/index.php?title=Special:Log/block&page=User%3AJdx
Yet I am still able to mark pages for translation: https://commons.wikimedia.org/wiki/Special:Log?type=&user=Jdx&page=Commons%3AAutoconfirmed+users&wpdate=2022-02-24&tagfilter=&wpfilters%5B%5D=newusers (the latest entry).
It looks a bit like a security hole and should not be allowed.
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.
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:
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:
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.
I'll take this up and address Niklas' comments. I expect the patch (https://gerrit.wikimedia.org/r/c/823618) to land next week, and I can work on this after that.
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.
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.
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.
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