Placing unexpected data in MediaWiki:GrowthMentors.json causes internal errors
Closed, ResolvedPublicSecurity
Actions

Assigned To

Authored By

	Xaosflux
	Feb 19 2025, 3:06 PM

Description

See similar report at T384244

Steps:

Manually edit MediaWiki:GrowthMentors.json
Put in an unsupported value for "weight" (e.g. "6")
Go to Special:ManageMentors
FAIL with error:

[6b90cbed-9d66-4ca7-a164-356855f13272] /wiki/Special:ManageMentors LogicException: Weight 6 is not supported

Backtrace:

from /srv/mediawiki/php-1.44.0-wmf.17/extensions/GrowthExperiments/includes/Specials/SpecialManageMentors.php(129)
#0 /srv/mediawiki/php-1.44.0-wmf.17/extensions/GrowthExperiments/includes/Specials/SpecialManageMentors.php(180): GrowthExperiments\Specials\SpecialManageMentors->formatWeight(GrowthExperiments\Mentorship\Mentor)
#1 /srv/mediawiki/php-1.44.0-wmf.17/extensions/GrowthExperiments/includes/Specials/SpecialManageMentors.php(242): GrowthExperiments\Specials\SpecialManageMentors->getMentorAsHtmlRow(GrowthExperiments\Mentorship\Mentor, int)
#2 /srv/mediawiki/php-1.44.0-wmf.17/extensions/GrowthExperiments/includes/Specials/SpecialManageMentors.php(324): GrowthExperiments\Specials\SpecialManageMentors->getMentorsTableBody(array)
#3 /srv/mediawiki/php-1.44.0-wmf.17/extensions/GrowthExperiments/includes/Specials/SpecialManageMentors.php(503): GrowthExperiments\Specials\SpecialManageMentors->getMentorsTable(array)
#4 /srv/mediawiki/php-1.44.0-wmf.17/includes/specialpage/SpecialPage.php(729): GrowthExperiments\Specials\SpecialManageMentors->execute(null)
#5 /srv/mediawiki/php-1.44.0-wmf.17/includes/specialpage/SpecialPageFactory.php(1734): MediaWiki\SpecialPage\SpecialPage->run(null)
#6 /srv/mediawiki/php-1.44.0-wmf.17/includes/actions/ActionEntryPoint.php(503): MediaWiki\SpecialPage\SpecialPageFactory->executePath(string, MediaWiki\Context\RequestContext)
#7 /srv/mediawiki/php-1.44.0-wmf.17/includes/actions/ActionEntryPoint.php(145): MediaWiki\Actions\ActionEntryPoint->performRequest()
#8 /srv/mediawiki/php-1.44.0-wmf.17/includes/MediaWikiEntryPoint.php(202): MediaWiki\Actions\ActionEntryPoint->execute()
#9 /srv/mediawiki/php-1.44.0-wmf.17/index.php(58): MediaWiki\MediaWikiEntryPoint->run()
#10 /srv/mediawiki/w/index.php(3): require(string)
#11 {main}

Unknown if this also causes MORE problems elsewhere

Details

Risk Rating: High
Author Affiliation: Wikimedia Communities

Related Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	SECURITY: Validate mentor's weight is within the expected range	mediawiki/extensions/GrowthExperiments	master	+12 -0

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Resolved	Release	dduvall	T386222 1.44.0-wmf.27 deployment blockers
		Resolved	Security	Urbanecm_WMF	T386826 Placing unexpected data in MediaWiki:GrowthMentors.json causes internal errors

Event Timeline

Xaosflux created this task.Feb 19 2025, 3:06 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 19 2025, 3:06 PM

Xaosflux added a project: GrowthExperiments-Mentorship.Feb 19 2025, 3:07 PM

Xaosflux added a subscriber: Urbanecm_WMF.

Restricted Application added a project: Growth-Team. · View Herald TranscriptFeb 19 2025, 3:07 PM

sbassett edited projects, added SecTeam-Processed, Vuln-DoS; removed Security-Team.Feb 19 2025, 5:58 PM

Urbanecm_WMF added subscribers: Michael, Sgs, Cyndymediawiksim and 3 others.Feb 25 2025, 1:29 PM

Etonkovidova moved this task from Inbox to Needs Discussion on the Growth-Team board.Feb 26 2025, 10:46 PM

Urbanecm_WMF claimed this task.Feb 27 2025, 9:02 AM

Urbanecm_WMF edited projects, added Growth-Team (Current Sprint); removed Growth-Team.

Global search advises there is no invalid value stored right now: https://global-search.toolforge.org/?q=%5C%22weight%5C%22%3A+%5B%5E0124%5D&regex=1&namespaces=8&title=GrowthMentors.*

T386826.patch4 KBDownload

In T386826#10590421, @Urbanecm_WMF wrote:

T386826.patch4 KBDownload

I've tried this and it does fix the bug. Given that the search confirms that there is no invalid weight currently stored anywhere, it also does not introduce new ones. So. I'm giving this my +2.

Deployed to production:

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

Urbanecm_WMF updated the task description. (Show Details)Feb 28 2025, 12:28 PM

Urbanecm_WMF added a subscriber: gerritbot.

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

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Validate mentor's weight is within the expected range

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

gerritbot added a project: Patch-For-Review.Feb 28 2025, 12:29 PM

Change #1123631 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Validate mentor's weight is within the expected range

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

Fixed, deployed, backported to master. Should be working now.

@sbassett Would it be possible to publish the task, please?

Urbanecm_WMF triaged this task as High priority.Feb 28 2025, 12:59 PM

Jdforrester-WMF added a project: MW-1.44-notes (1.44.0-wmf.19; 2025-03-04).Feb 28 2025, 2:41 PM

The fix is in place - an invalid value e.g. an attempt to save 6 for weight on MediaWiki:GrowthMentors.json will display the following error message and such an edit cannot be saved:

Screen Shot 2025-02-28 at 9.52.58 AM.png (1×2 px, 174 KB)

sbassett changed Author Affiliation from N/A to Wikimedia Communities.Feb 28 2025, 6:54 PM

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".

sbassett changed the edit policy from "Custom Policy" to "All Users".

sbassett changed Risk Rating from N/A to High.

Maintenance_bot removed a project: Patch-For-Review.Feb 28 2025, 7:30 PM

Checked on testwiki wmf.19 - the issue is fixed. For entering invalid numerical values for weight on MediaWiki:GrowthMentors.json the error message is displayed and the form is not saved.

Screen Shot 2025-03-06 at 3.05.52 PM.png (1×1 px, 306 KB)

The general error message is displayed for all other non-valid input values:

Screen Shot 2025-03-06 at 3.06.24 PM.png (1×1 px, 331 KB)

Michael mentioned this in T390333: [beta-enwiki] Special:ManageMentors doesn't list all mentors .Apr 1 2025, 2:04 PM

Patch 03-T386826.patch is currently failing to apply for the most recent code in the mainline branch of extensions/GrowthExperiments. This is blocking MediaWiki release 1.44.0-wmf.27(T386222)

If the patch needs to be rebased

To unblock the release, a new version of the patch can be placed at the right location in the deployment server with the following Scap command:

REVISED_PATCH=<path_to_revised_patch>
scap update-patch --message-body 'Rebase to solve merge conflicts with mainline code' /srv/patches/1.44.0-wmf.27/extensions/GrowthExperiments/03-T386826.patch "$REVISED_PATCH"

If the patch has been made public

To unblock the release, the patch can be removed for the right version from the deployment server with the following Scap command:

scap remove-patch --message-body 'Remove patch already made public' /srv/patches/1.44.0-wmf.27/extensions/GrowthExperiments/03-T386826.patch

(Note that if patches for the version don't exist yet, they will be created and the patch you specified removed)

I'm somehow confused by what you wrote, @SecurityPatchBot...

Anyway: The patch here was published (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/1123631) and is in master branch, so there is no need to rebase it or anything. It fails to be applied because of https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/1139075, which modifies the file, resulting in a merge conflict.

I'm confused as to why the bot claims it should be removed from the deployment server, since:

[urbanecm@deploy1003 /srv/patches (master)]$ ls
1.44.0-wmf.24  1.44.0-wmf.25
[urbanecm@deploy1003 /srv/patches (master)]$

the patch is not there to begin with (for wmf.27), so it cannot be removed from that version. It _is_ included in wmf.25 and wmf.24 (despite the master branch version being already in both versions on itself) though. In the GE directory, there are patches for T384244 and T386963 (both are already in master too).

According to the bot's instructions, I should run scap remove-patch --message-body 'Remove patch already made public' /srv/patches/1.44.0-wmf.27/extensions/GrowthExperiments/03-T386826.patch. I tried that, but that:

inits the version directory (which I guess is OK),
removes the patch for this task (which is intended),
keeps patches for T384244 and T386963 (which is confusing, as (a) the bot does not complain on either of those tasks and (b) those patches should probably not be kept indefinitely as well)

I would like some guidance regarding this. Is it expected of patching teams to remove the patch from future directories by running this scap command when they backport a patch to Gerrit? Is there a reason why we don't have a script that would automatically remove patches for which Git claims that "No changes -- Patch already applied."?

Lowering priority, as this is not a blocker to deployment (given the intended resolution is clear here). I'm happy to run the command, but I am curious about the other patches that apply cleanly, but are merged already.

Urbanecm_WMF mentioned this in T386222: 1.44.0-wmf.27 deployment blockers.Apr 28 2025, 7:21 AM

We did not run the train last week (wmf.26). The system that cut the new branch (wmf.27) attempts to apply the patches from the previous deployment (wmf.25), but since those fail to apply, the script aborts and the patches are never copied to a new /srv/patches/1.44.0-wmf.27.

The output of the scap script for GrowthExperiments:

01:57:44 [ALREADY APPLIED] /srv/jenkins-agent/workspace/Branch cut test patches/work/patches/branch_cut_pretest/extensions/GrowthExperiments/01-T384244.patch
01:57:44 [ALREADY APPLIED] /srv/jenkins-agent/workspace/Branch cut test patches/work/patches/branch_cut_pretest/extensions/GrowthExperiments/02-T386963.patch
01:57:44 [FAILED] /srv/jenkins-agent/workspace/Branch cut test patches/work/patches/branch_cut_pretest/extensions/GrowthExperiments/03-T386826.patch

@Urbanecm_WMF already investigated those have all been merged in master (and released in wmf.24/wmf.25). So essentially we do not need those patches anymore and they should have been removed. That can be done using the command reported above by the bot but using the latest deployment (wmf.25), instead of the new one that does not exist yet:

scap remove-patch --message-body 'Remove patch already made public' /srv/patches/1.44.0-wmf.25/extensions/GrowthExperiments/01-T384244.patch
scap remove-patch --message-body 'Remove patch already made public' /srv/patches/1.44.0-wmf.25/extensions/GrowthExperiments/02-T386963.patch
scap remove-patch --message-body 'Remove patch already made public' /srv/patches/1.44.0-wmf.25/extensions/GrowthExperiments/03-T386826.patch

The bot was implemented to handle patches that fail to apply, it wasn't meant to modify/delete other patches.

Any patch that is still applying without conflicts just gets carried over to the next version, the bot will never ping about them. So far patches have normally been deleted manually once they go public, it seems that didn't happen in this case.

It could be helpful that the bot has some support for that case, but bear in mind that right now the bot can only do changes to patches on the deployment server when it's run manually from them (meaning that public patches could only be removed when some operation is executed for a patch with problems) Alternatively the patch could start running in a systemd timer, but that is more involved and would be harder to find the time to make it happen.

@Urbanecm_WMF if that sounds like something helpful, could you create a task for the feature request and add details there?

I have removed all three patches of GrowthExperiments using the commands from above ( T386826#10771660 ) . I have poked the other tasks about the patches removal

In T386826#10771750, @jnuche wrote:

So far patches have normally been deleted manually once they go public, it seems that didn't happen in this case.

I don't know if this is officially codified in any security release doc, be it the primary/core mw release or the supplemental release. The Security-Team does our best to manually track current security patches under the long-running T276237 and then garbage-collect outdated patches during future security deployments. Probably not the greatest process, I know.

	F58627682: Screen Shot 2025-03-06 at 3.06.24 PM.png
	Mar 6 2025, 11:12 PM

	F58627677: Screen Shot 2025-03-06 at 3.05.52 PM.png
	Mar 6 2025, 11:12 PM

	F58520509: Screen Shot 2025-02-28 at 9.52.58 AM.png
	Feb 28 2025, 6:26 PM

	F58518236: T386826.patch
	Feb 28 2025, 11:22 AM

Placing unexpected data in MediaWiki:GrowthMentors.json causes internal errorsClosed, ResolvedPublicSecurityActions

Description

Details

Related ObjectsSearch...

Event Timeline

Placing unexpected data in MediaWiki:GrowthMentors.json causes internal errors
Closed, ResolvedPublicSecurity
Actions

Related Objects
Search...