Steps to reproduce: put <script>alert("oops!")</script> into an item label. Search on Special:Nearby. When that item is found, the script will be executed.

This is probably due to the label being treated as a display title somewhere, without the proper escaping. DisplayTitle is always safe html. Labels are not, they are unsafe plain text.

Tested with the following settings:

$wgMFNearby = true;
$wgMFQueryPropModules[] = 'pageterms';
$wgMFSearchAPIParams['wbptterms'] = array( 'label' );
$wgMFSearchGenerator = array(
	'name' => 'wbsearch',
	'prefix' => 'wbs'
);

It does not seem to matter whether $wgMFSearchGenerator is set or not, though.

Found during T85368.

The problem seems to be the following line in Page.js:

displayTitle = terms && terms.label ? terms.label[0] : pageprops.displaytitle;

pageprops.displaytitle is HTML by definition. terms.label[0] is plain text and needs escaping.

The issue was apparently introduced by I49cf14db7857b077b8536ac654c04a4af2f4d2ee see https://gerrit.wikimedia.org/r/#/c/246282/12/resources/mobile.startup/Page.js

Patch in Gerrit: https://gerrit.wikimedia.org/r/#/c/255540/

In T119707#1833722, @daniel wrote:

Steps to reproduce: put <script>alert("oops!")</script> into an item label. Search on Special:Nearby. When that item is found, the script will be executed.

This is probably due to the label being treated as a display title somewhere, without the proper escaping. DisplayTitle is always safe html. Labels are not, they are unsafe plain text.

After a first look, this seems to be the problem. Normally we escape things in our hogan templates (which is the nearest "part" of the code before the html is generated/outputted. If we escape the displayTitle property or Page.js there, we would double escape the displaytitle returned by the api.

Would it be a problem to return safe html for the label inside Wikibase? Otherwise we need to escape it client side (which means in _any_ client, which uses these labels). I assume you already escape the labels in Wikibase frontend code? If you can't return safe html in Wikibase, I'll upload a change to escape the label in MobileFrontend (IIRC, Security related tasks needs to upload a git patch to the task, which will be imported into gerrit, after they're approved, right?).

copied to tin.eqiad.wmnet:/srv/patches/extensions/MobileFrontend/T119707.patch

The displayTitle also falls back in JS to page title which is also plain text, not only to wikidata label.

Adding a few more wmf people. Jon is out on vacation, iirc.

! In T119707#1833886, @Florian wrote:
Normally we escape things in our hogan templates (which is the nearest "part" of the code before the html is generated/outputted. If we escape the displayTitle property or Page.js there, we would double escape the displaytitle returned by the api.

Would it be a problem to return safe html for the label inside Wikibase?

That would be odd and inconsistent. The API generally returns plain text for everything, unless explicitly stated otherwise. Core's dispalytitle pageprop is a rare exception.

The API caters to all kinds of clients, many of which don't ever need HTML. Forcing them to unescape our labels is not a good idea conceptually, and it would of course be a breaking change to the API.

So yes, that would be a problem.

Otherwise we need to escape it client side (which means in _any_ client, which uses these labels).

Escaping should always be done just before output. So yes, escaping should happen in the clients. All clients. Because the clients know for what target format they need escaping. The API cannot possibly know that.

I assume you already escape the labels in Wikibase frontend code?

Of yourse, just like core escapes page titles when outputting them, etc.

Btw, this problem isn't specific to MF interaction with wikibase. YOu can also use a regular page title for XSS injections: MF makes displaytitle fall back to the plain page title without escaping.

If you can't return safe html in Wikibase, I'll upload a change to escape the label in MobileFrontend (IIRC, Security related tasks needs to upload a git patch to the task, which will be imported into gerrit, after they're approved, right?).

My patch hopefully covers all instances where displaytitle is set to something unsafe in MF. There's actually quite a few places where this happens.

Adding @jernandez and @bmansurov.

In T119707#1833884, @daniel wrote:

Patch in Gerrit: https://gerrit.wikimedia.org/r/#/c/255540/

I can't access the patch for some reason.

@bmansurov: I've added you as a reviewer on the change.

In T119707#1834092, @bmansurov wrote:

In T119707#1833884, @daniel wrote:

Patch in Gerrit: https://gerrit.wikimedia.org/r/#/c/255540/

I can't access the patch for some reason.

It's a draft, so only those of us in the 'security' group and people explicitly listed as reviewers can see it via gerrit - obviously *anyone* can still see the actual commit itself if they want.