Page MenuHomePhabricator

Remove stateful mw.Message#toString behaviour
Closed, ResolvedPublic

Description

Currently:

var x = mw.message('addedwatchtext', 'X');
x.toString()
// "[[:X]]" and its talk page have been added to your [[Special:Watchlist|watchlist]].
x.text()
// "[[:X]]" and its talk page have been added to your [[Special:Watchlist|watchlist]].

x.escaped()
// "[[:X]]" and its talk page have been added to your [[Special:Watchlist|watchlist]].
x.toString()
// "[[:X]]" and its talk page have been added to your [[Special:Watchlist|watchlist]].
var x = mw.message('echo-badge-count', 42);
x.toString()
// "42"
x.text()
// "42"

x.plain()
// "{{PLURAL:42|42|100={{formatnum:99}}+}}"
x.toString()
// "{{PLURAL:42|42|100={{formatnum:99}}+}}"

We're recently finished removal of the same mess in wfMessage/Message.php:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 726289 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.base: Clean up unit tests for mw.Message

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

Change 726290 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.base: Require `format` param in internal message parser()

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

Change 726291 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] [WIP] mediawiki.base: Deprecate stateful use of toString()

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

Change 726336 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.base: Add tests for mw.log.deprecate()

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

Change 726337 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.base: Introduce mw.log.makeDeprecated()

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

Change 727437 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/Wikibase@master] wikibase.templates: Simplify with mw.format() instead of mw.Message

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

Change 727437 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] wikibase.templates: Simplify with mw.format() instead of mw.Message

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

Krinkle triaged this task as Medium priority.Oct 8 2021, 5:42 PM
Krinkle moved this task from Inbox to Accepted Enhancement on the MediaWiki-ResourceLoader board.

Change 726336 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.base: Add tests for mw.log.deprecate()

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

Change 726337 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.base: Introduce mw.log.makeDeprecated()

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

Change 726290 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.base: Require `format` param in internal message parser()

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

Change 726289 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.base: Clean up unit tests for mw.Message

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

Change 726291 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.base: Deprecate stateful use of toString()

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

This deprecation warning seems to be working incorrectly. I see it on every call to mw.message( ... ).escaped().

Change 744895 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.base: Add missing toString param to Messags#escaped()

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

Confirmed. Thanks. Fix incoming.

Change 744804 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@wmf/1.38.0-wmf.12] mediawiki.base: Add missing toString param to Messags#escaped()

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

Change 744895 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.base: Add missing toString param to Message#escaped()

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

Change 744804 merged by jenkins-bot:

[mediawiki/core@wmf/1.38.0-wmf.12] mediawiki.base: Add missing toString param to Message#escaped()

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

Would it be possible to also remove string autoconversion (toString() without an argument)? Or make it use a safer method? That would require a BC break but it would make application code much easier to review for correctness. (And with the PHP implementation using parse as the default format, it's easy for a developer to assume the JS one works the same way, and create a vulnerability.)

Change 765344 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.base: Remove deprecated stateful Message#toString

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

Change 765551 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] mediawiki.base: Rewrite old mw.Message documentation

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

Would it be possible to also remove string autoconversion (toString() without an argument)? Or make it use a safer method? That would require a BC break but it would make application code much easier to review for correctness. (And with the PHP implementation using parse as the default format, it's easy for a developer to assume the JS one works the same way, and create a vulnerability.)

I recognise the difference in expectation compared to PHP. However I think there is similarly a difference in expectation within JavaScript itself. Fundamentally, when on the server-side, the only path outward is an HTTP response, where for most request contexts, the output buffer must be encoded as HTML. The lowest common denominator is HTML, and thus often the format of choice to exchange between classes and across responsibility boundaries is a string of HTML. And even when it isn't, it's reasonable to offer support for it, since eventually everything would end up as HTML.

This is not the case in JavaScript. The life of HTML stops as soon as the main thread spins up and interacts with a DOM. In the DOM text is be read and written as text. There is no escaping going on there, not in our code, and not in the browser natively. Dealing with a string of HTML in JavaScript should be rare. Every once in a while you'll face an HTML template or parsed wikitext, but by and large information is either strings of text or DOM objects.

HTML strings are sufficiently rare that I consider it as an immediate red flag and drop my condidence in any JS code if I see it trying to escape text as HTML. In PHP, the act of escaping feels right. In JS, I feel the opposite. This is almost never needed, and rarely the most efficient, secure, or idiomatic way of doing something. And given how common this is, it isn't unusual to have libraries and other APIs that in fact mandate plain text (they might not even support taking an HTML string). E.g. underlying native interfaces like elem.setAttribute(), elem.title =, elem.textContent =, input.value = all require text. Once you go to HTML, there isn't an obvious way back.

mw.msg() is the most access pattern for messages afaik, which is also text-formatted. So it seems sensible for mw.message().toString() to do the same when logged or concatenated somewhere. See also strings inside any JSON object one might get from a REST API.

Change 765551 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.base: Rewrite old mw.Message documentation

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

Change 765344 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.base: Remove deprecated stateful Message#toString

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