Found this while reviewing https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/743494 which moves the vulnerable code. Should that patch be put on hold until the fix is public, or quietly updated to fix the vulnerability (and then the security patch only needs to be applied to the current WMF branch)?

In T298019#7579985, @Tgr wrote:

T298019.patch1 KBDownload

+2, patch LGTM.

In T298019#7579990, @Tgr wrote:

Found this while reviewing https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/743494 which moves the vulnerable code. Should that patch be put on hold until the fix is public, or quietly updated to fix the vulnerability (and then the security patch only needs to be applied to the current WMF branch)?

Since the existing patch brings attention to the vulnerable code, I'd say to apply the patch to prod and fix as part of the patch you linked.

16:38 <urbanecm> !log Deploy security patch for T298019
16:38 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

Patch deployed with a slightly amended commit message. This is the deployed .patch file:

We've resolved many issues like this in public in the past (recent examples:  1   2 ).

I think it's much more practical, and not very risky, to resolve them in public. Or if there's a good reason not to do that, please let me know and I'll keep similar issues private in the future too.

In T298019#7580571, @Urbanecm_WMF wrote:

16:38 <urbanecm> !log Deploy security patch for T298019
16:38 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

Patch deployed with a slightly amended commit message. This is the deployed .patch file:

Thanks!

In T298019#7580579, @matmarex wrote:

We've resolved many issues like this in public in the past (recent examples:  1   2 ).

I think it's much more practical, and not very risky, to resolve them in public. Or if there's a good reason not to do that, please let me know and I'll keep similar issues private in the future too.

I'd say in most cases, this is likely fine, especially if patches can get merged on a Monday before the train cut. It's also fine to deploy these as security patches, as @Urbanecm has done, though I'm not sure we even need to track these for the supplemental security release as we have in the past. I guess that's worth further consideration on the part of the Security-Team.

In T298019#7580761, @sbassett wrote:

[...]

In T298019#7580579, @matmarex wrote:

We've resolved many issues like this in public in the past (recent examples:  1   2 ).

I think it's much more practical, and not very risky, to resolve them in public. Or if there's a good reason not to do that, please let me know and I'll keep similar issues private in the future too.

I'd say in most cases, this is likely fine, especially if patches can get merged on a Monday before the train cut. It's also fine to deploy these as security patches, as @Urbanecm_WMF has done, though I'm not sure we even need to track these for the supplemental security release as we have in the past. I guess that's worth further consideration on the part of the Security-Team.

For the record, this task was under internal discussion by the Growth team. The decision was to proceed with a security patch deploy (to ensure production is fixed), and to update https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/743494 (which moves the vulnerable code around) to also fix the vulnerability.

This means the security patch will likely not be needed with next train deploy (cc @Sgs to confirm that).

In T298019#7580795, @Urbanecm_WMF wrote:

For the record, this task was under internal discussion by the Growth team. The decision was to proceed with a security patch deploy (to ensure production is fixed), and to update https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/743494 (which moves the vulnerable code around) to also fix the vulnerability.

This means the security patch will likely not be needed with next train deploy (cc @Sgs to confirm that).

Great, thanks.

@sbassett the patch did not get merged so please do carry it over to the next branch. We'll merge it this week.

In T298019#7594873, @Tgr wrote:

@sbassett the patch did not get merged so please do carry it over to the next branch. We'll merge it this week.

Ok, that's fine. It's tracked at T276237 and for the next supplemental release at T297839. And it's still on wmf.13 and wmf.16. So feel free to merge in gerrit whenever.

@sbasset was merged as part of rEGREc7ae734fb063: SECURITY: Fix several i18n XSS issues in suggested edits. Please don't carry over the secret patch to wmf.17.

In T298019#7611943, @Tgr wrote:

@sbasset was merged as part of rEGREc7ae734fb063: SECURITY: Fix several i18n XSS issues in suggested edits. Please don't carry over the secret patch to wmf.17.

Sounds good, thanks for updating T276237. I don't personally carry anything over. The patch should fail to apply when Release-Engineering-Team tries to roll out wmf.17 to group 0 today (they always double-check this anyways, in case there's a conflict or erroneous git am).