Security-Team: This is a mild Vuln-DoS. It requires two conditions for it to work: the attacker must be an administrator, plus the victim must have a mentor assigned (and Help Panel enabled). All accounts created since the deployment of Growth features would be vulnerable. I filled it as a security issue out of caution; feel free to publish if you deem it appropriate.

@JJMC89: This is not an issue in EditGrowthConfig (legacy Community Configuration), but in the way how we migrated mentorship to the CommunityConfiguration extension. Updated the tags accordingly.

There are two ways how we can fix this:

1. Finish T367575 (currently marked as stalled)
2. Register StructuredMentorListValidator as a validator with community configuration, and rely on CommunityConfiguration running the validator (instead of running it ourselves). Requires first finishing T384246.

Option 2 is quicker, and it requires relatively little changes, but option 1 is the end goal.

@Sgs @Michael Do you have any thoughts on this?

As discussed with @Urbanecm_WMF, let's go with Option 2. It is faster for now to create that capability in principle as @internal in a public change, and then have us making use of it as part of this security change.

Option 2 implemented in the patch above. @Michael @Sgs please approve via Phab, then I can deploy. T384246 has the matching CommunityConfiguration patch in Gerrit (with no context).

I applied the patch locally and can confirm that it re-enables the previous validation. In particular, I can no longer save a string for the weight, or an integer for the username or message.

Some other mischief seems still possible (saving too long an intro message, etc.) but that does not seem security-relevant.

Thus, I'm giving my virtual +2 to the patch above.

In T384244#10481464, @Michael wrote:

Some other mischief seems still possible (saving too long an intro message, etc.) but that does not seem security-relevant.

Right. The previous code interpreted warnings as blocking, while the new code doesn't do that (instead, we have the validatePermissively concept). We can fix that later, I filled T384445 (privately for now) to ensure we do not forget that.

14:56 <urbanecm> !log Deployed security patch for T384244
14:56 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

Deployed to production (including the corresponding CommunityConfiguration change).

@sbassett Is it ok for us to backport this to Gerrit at this point? Or is there anything else your team would like to do first? For the record, GrowthExperiments is a regular extension (that is not a part of the release tarball).

In T384244#10484015, @Urbanecm_WMF wrote:

@sbassett Is it ok for us to backport this to Gerrit at this point? Or is there anything else your team would like to do first? For the record, GrowthExperiments is a regular extension (that is not a part of the release tarball).

Yes. I'll add it for the next supplemental release as well. Thanks.

Change #1114020 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Register CommunityStructuredMentorListValidator

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

Change #1114020 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Register CommunityStructuredMentorListValidator

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

In T384244#10492949, @sbassett wrote:

In T384244#10484015, @Urbanecm_WMF wrote:

@sbassett Is it ok for us to backport this to Gerrit at this point? Or is there anything else your team would like to do first? For the record, GrowthExperiments is a regular extension (that is not a part of the release tarball).

Yes. I'll add it for the next supplemental release as well. Thanks.

Given the issue was fixed publicly, is it OK to publish the task at this point? It would allow us to publish & fix T384445 as well (which does not have security implications, but would enable anyone to guess the existence of this task as well).

In T384244#10519192, @Urbanecm_WMF wrote:

Given the issue was fixed publicly, is it OK to publish the task at this point?

Yes, totally fine.

In T384244#10519218, @sbassett wrote:

In T384244#10519192, @Urbanecm_WMF wrote:

Given the issue was fixed publicly, is it OK to publish the task at this point?

Yes, totally fine.

Done. Thank you!