I was going to submit a patch to resolve the first bug, but I noticed that the incorrect code was added in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MediaSearch/+/677030 in response to a security review. Do you have any reason to believe that these messages are not being escaped correctly somewhere else? @AnneT @matthiasmullie

As for the second bug…

The messages are parsed in a ResourceLoader packageFiles callback:

				{
					"name": "resources/data/searchOptions.json",
					"callback": "MediaWiki\\Extension\\MediaSearch\\SearchOptions::getSearchOptions"
				}

It invokes the callback with a ResourceLoaderContext object:

	private function expandPackageFiles( ResourceLoaderContext $context ) {
...
					$callbackResult = ( $fileInfo['callback'] )(
						$context,
						$this->getConfig(),
						$expanded['callbackParam']
					);

ResourceLoaderContext's msg() function uses inLanguage():

	public function msg( $key, ...$params ): Message {
		return wfMessage( $key, ...$params )
			->inLanguage( $this->getLanguage() )
			// Use a dummy title because there is no real title
			// for this endpoint, and the cache won't vary on it
			// anyways.
			->page( PageReferenceValue::localReference( NS_MAIN, 'Dwimmerlaik' ) );
	}

And inLanguage() turns off the interface flag!

	/**
	 * Request the message in any language that is supported.
	 *
	 * As a side effect interface message status is unconditionally
	 * turned off.
...
	 */
	public function inLanguage( $lang ) {
...
		$this->interface = false;
...
	}

This behavior was added in 2010 in commit rMWe5941506f9c3: Tidy up the class, which doesn't explain the reason why, other than asserting that it is "better handling for languages".

In T291590#7373050, @matmarex wrote:

I was going to submit a patch to resolve the first bug, but I noticed that the incorrect code was added in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MediaSearch/+/677030 in response to a security review. Do you have any reason to believe that these messages are not being escaped correctly somewhere else? @AnneT @matthiasmullie

We don't have any specific reason to believe that, this was just in response to security review findings (see the General Security Issues section).

Message does contains setInterfaceMessageFlag whose documentation seems to suggest it exists entirely to restore the interface behavior after setting a language. We could probably add that to the call chain in ResourceLoaderContext? I'm unsure of whether that'd cause side-effects we care about...

In T291590#7373095, @AnneT wrote:

In T291590#7373050, @matmarex wrote:

I was going to submit a patch to resolve the first bug, but I noticed that the incorrect code was added in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MediaSearch/+/677030 in response to a security review. Do you have any reason to believe that these messages are not being escaped correctly somewhere else? @AnneT @matthiasmullie

We don't have any specific reason to believe that, this was just in response to security review findings (see the General Security Issues section).

In my experience, these suggestions from the Security team are never correct (see T242134#5898495). I guess I'll have a closer look at this case to make sure, and submit a patch. Thanks for the reply!

Thanks @matmarex for the summary.

It looks like this recent change (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/713681/) may have caused an underlying problem in MediaSearch to surface?

If that's the case, would it be feasible to revert the DiscussionTools patch long enough for us to fix the underlying problem?

@egardner If we're just fixing the use of parse in MediaSearch, I think it'd be quicker to do that (and backport it) than to revert the DiscussionTools patch.

The other issue with ResourceLoaderContext might require a bit more testing to work out if the simple fix would break anything elsewhere, but I don't think we need to rush that one?

In T291590#7373134, @DLynch wrote:

@egardner If we're just fixing the use of parse in MediaSearch, I think it'd be quicker to do that (and backport it) than to revert the DiscussionTools patch.

Fair enough, I can do this shortly unless someone else beats me to it.

Change 722974 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[mediawiki/extensions/MediaSearch@master] Use text() instead of parse() for MediaSearch UI messages

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

I didn't want to submit a patch until I could test it, and I couldn't reproduce the issue locally – it turns out you need $wgExtraSignatureNamespaces[] = NS_MAIN; in your LocalSettings.php to see it. (Which also implies that these messages are parsed in the context of a main namespace title, which seems to make ansolutely no sense, but oh well.)

In T291590#7373185, @matmarex wrote:

I didn't want to submit a patch until I could test it, and I couldn't reproduce the issue locally – it turns out you need $wgExtraSignatureNamespaces[] = NS_MAIN; in your LocalSettings.php to see it. (Which also implies that these messages are parsed in the context of a main namespace title, which seems to make ansolutely no sense, but oh well.)

We phased out Dwimmerlaik in the API a few years ago in favor of Special:Badtitle/dummy title for API calls set in api.php, it's probably time for ResourceLoader to do the same. As a side-effect this would probably fix this too since no one is adding NS_SPECIAL as an extra signature namespace?

As far as testing the patch, we can actually repro this problem on Beta commons: https://commons.wikimedia.beta.wmflabs.org/w/index.php?search=desert&title=Special:MediaSearch (garbled messages are visible there now).

So if someone wants to review and +2 the patch I just posted, we should be able to confirm on Beta that it fixes the problem before backporting.

Change 722974 merged by jenkins-bot:

[mediawiki/extensions/MediaSearch@master] Use text() instead of parse() for MediaSearch UI messages

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

Change 722990 had a related patch set uploaded (by Catrope; author: Eric Gardner):

[mediawiki/extensions/MediaSearch@wmf/1.38.0-wmf.1] Use text() instead of parse() for MediaSearch UI messages

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

Change 722990 merged by jenkins-bot:

[mediawiki/extensions/MediaSearch@wmf/1.38.0-wmf.1] Use text() instead of parse() for MediaSearch UI messages

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

Mentioned in SAL (#wikimedia-operations) [2021-09-23T00:35:57Z] <catrope@deploy1002> Synchronized php-1.38.0-wmf.1/extensions/MediaSearch/: Use text() instead of parse() for MediaSearch UI messages (T291590) (duration: 01m 08s)

Thanks for the quick patch @egardner (and the review and the backport @Catrope).

In T291590#7373188, @Legoktm wrote:

In T291590#7373185, @matmarex wrote:

I didn't want to submit a patch until I could test it, and I couldn't reproduce the issue locally – it turns out you need $wgExtraSignatureNamespaces[] = NS_MAIN; in your LocalSettings.php to see it. (Which also implies that these messages are parsed in the context of a main namespace title, which seems to make ansolutely no sense, but oh well.)

We phased out Dwimmerlaik in the API a few years ago in favor of Special:Badtitle/dummy title for API calls set in api.php, it's probably time for ResourceLoader to do the same. As a side-effect this would probably fix this too since no one is adding NS_SPECIAL as an extra signature namespace?

(For anyone else wondering about the reference to Dwimmerlaik, see rMW536f98c76003: Kill Dwimmerlaik)