Remove stateful mw.Message#toString behaviour
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Krinkle
	Oct 5 2021, 12:58 AM

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()
// &quot;[[:X]]&quot; and its talk page have been added to your [[Special:Watchlist|watchlist]].
x.toString()
// &quot;[[:X]]&quot; 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:

Details

Subject	Repo	Branch	Lines /-
mediawiki.base: Remove deprecated stateful Message#toString	mediawiki/core	master	5 -43
mediawiki.base: Rewrite old mw.Message documentation	mediawiki/core	master	24 -39
mediawiki.base: Add missing toString param to Message#escaped()	mediawiki/core	wmf/1.38.0-wmf.12	1 -1
mediawiki.base: Add missing toString param to Message#escaped()	mediawiki/core	master	1 -1
mediawiki.base: Clean up unit tests for mw.Message	mediawiki/core	master	119 -188
mediawiki.base: Deprecate stateful use of toString()	mediawiki/core	master	31 -8
mediawiki.base: Require `format` param in internal message parser()	mediawiki/core	master	14 -12
mediawiki.base: Introduce mw.log.makeDeprecated()	mediawiki/core	master	124 -39
mediawiki.base: Add tests for mw.log.deprecate()	mediawiki/core	master	41 -0
wikibase.templates: Simplify with mw.format() instead of mw.Message	mediawiki/extensions/Wikibase	master	13 -54

Customize query in gerrit

Related Objects

Mentioned In: T293953: 1.38.0-wmf.12 deployment blockers
T293952: 1.38.0-wmf.11 deployment blockers
Mentioned Here: T146416: Message -> string transformations should not affect each other
T252946: Report implicit or explict usage of Message::__toString in the phan-taint-check-plugin

Event Timeline

Krinkle created this task.Oct 5 2021, 12:58 AM

Restricted Application added a project: Performance-Team. · View Herald TranscriptOct 5 2021, 12:58 AM

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

Krinkle moved this task from Untriaged to 2021 | MW 1.36 / 1.37 on the Technical-Debt (Deprecation process) board.Oct 5 2021, 12:59 AM

Krinkle added a subscriber: egardner.

DannyS712 awarded a token.Oct 5 2021, 3:36 AM

DannyS712 subscribed.

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

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.4; 2021-10-12).Oct 8 2021, 4:00 AM

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.

Krinkle moved this task from Inbox, needs triage to Backlog: Maintenance, non-prioritized on the Performance-Team board.Oct 10 2021, 12:19 AM

Change 726336 merged by jenkins-bot:

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

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

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.5; 2021-10-19); removed MW-1.38-notes (1.38.0-wmf.4; 2021-10-12).Oct 14 2021, 4:00 AM

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

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.12; 2021-12-06); removed MW-1.38-notes (1.38.0-wmf.5; 2021-10-19).Nov 16 2021, 11:00 PM

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

Krinkle mentioned this in T293952: 1.38.0-wmf.11 deployment blockers.Nov 18 2021, 8:04 PM

Krinkle removed a project: Patch-For-Review.

AntiCompositeNumber mentioned this in T293953: 1.38.0-wmf.12 deployment blockers.Dec 3 2021, 6:23 PM

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

gerritbot added a project: Patch-For-Review.Dec 7 2021, 11:30 PM

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

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.13; 2021-12-13); removed MW-1.38-notes (1.38.0-wmf.12; 2021-12-06).Dec 8 2021, 1:00 AM

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

ReleaseTaggerBot edited projects, added MW-1.38-notes (1.38.0-wmf.12; 2021-12-06); removed MW-1.38-notes (1.38.0-wmf.13; 2021-12-13).Dec 8 2021, 4:00 PM

• Tgr awarded a token.Jan 4 2022, 8:59 AM

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

In T292489#7595098, @Tgr wrote:

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: