Page MenuHomePhabricator

Deprecate mw.msg() MediaWiki method
Closed, DeclinedPublic

Description

mw.msg( key ) is a shortcut for mw.message( key ).text(). The latter is fairly self-explanatory, the former is not; you have to memorize what escaping it uses. Using mw.msg() when you should have used mw.message().parse() or mw.message().escapes() results in an XSS vulnerability; from reading the code, this won't be obvious (unlike using mw.message().text() wrongly which is fairly easy even for the untrained reviewer to get suspicious about). MediaWiki developers are regularly tripped by this.

Obscure naming of security-critical functionality increases the chance of vulnerabilities, for no benefit. This shortcut should be deprecated and removed.

Event Timeline

Adding resourceloader tag as mw.msg() is part of ResourceLoader's base module. Not sure if that's the proper use of the tag, feel free to revert back to -general.

I don't see a security problem here. https://www.mediawiki.org/wiki/Security_for_developers#Cross-site_scripting_(XSS) recommends:

Escape as close to the output as possible, so that the reviewer can easily verify that it was done.

Therefor the developer should use

$element.text( mw.message( key ).text() )

or

$element.text( message )

instead of

$element.html( mw.message( key ).escaped() )

or

$element.html( message )

The most common use case is the unescaped mw.message( key ).text() because escaping should be done as close to the output as possible. And mw.msg( key ) is a useful shortcut for the most common use case.

@Tgr Did this some up during onboarding and/or through working with someone less familiar with MW frontend dev, or is this a more general observation about what you believe will result in an easier to use interface?

I'm asking in which this came up as I'd like to better understand exactly how the confusion comes up. I feel slightly hestitant about removing mw.msg() without an equally short replacement as I believe (but am happy to hear otherwise) that this actually makes for an easier to use interface today. I recognise that the "default" could be a bad default, but I'd like to understand that better before I agree.

The way I'm looking at it, the default behaviour of mw.msg() matches the assumptions one would have to make about almost any JavaScript interface. Whether our own utilities like mw.user.getName() or mw.Title#getUrl(), or browsers APIs like location.host, domElement.dataset.foo, or domElement.getAttribute('data-foo'), or HTTP response from an API request, such as from JSON data like data.edit.comment, etc. In each of these, the JavaScript runtime access the native string value of plain text directly, and likewise has to either be sent to an input that accepts the same, e.g. Element.setAttribute/jQuery.attr, Element.textContent/jQuery.text. The alternative, if one is producting raw HTML in JavaScript (that feels rare?), then one would have to escape all of these via mw.html.escape() first, or access a different API that provides an explicitly HTML-enriched version of the data, such mw.message().parse().

Is my mental model for this different from yours? What is it about mw.msg/mw.message().text() that makes it feel like it would be HTML-formatted? Or is this a more general issue that you see unaddressed in other APIs as well?

Krinkle triaged this task as Medium priority.

Thanks, I'll take a look when I get back.

Krinkle changed the task status from Open to Stalled.Jun 15 2023, 5:49 PM
Krinkle lowered the priority of this task from Medium to Low.
Krinkle moved this task from Inbox to Backlog on the MediaWiki-ResourceLoader board.

Is my mental model for this different from yours? What is it about mw.msg/mw.message().text() that makes it feel like it would be HTML-formatted?

mw.message.text() is fine. mw.msg() just seemed unnecessary to me, on grounds that it's always better for security-sensitive code to be explicit than potentially unclear - a reviewer shouldn't have to rely on their intuition / mental model when reasoning about the XSS-proofness of the code.

Anyway, apparently this isn't as self-evident as I thought it were, and I don't feel that strongly about it so let's drop it.