@BlankEclair thank you for reporting. I'll follow up with further information soon

@BlankEclair - thank you for this very detailed analysis, and the video, and the patch! This is extremely helpful. (And I had no idea that there were this many unescaped messages and other strings!) Unfortunately, I didn't see the patch until now - by which time I had already written up fixes on my own for most of these issues. Although in many cases our fixes are (as maybe could be expected) very similar to one another's, and in some cases your fixes are a little more elegant. Anyway, I'm going through the issues piecemeal, and here is the first of my changes:

eee69a311d12

It takes care of all the JS-related issues, as far as I know; and it basically has the same approach you took, of replacing hardcoded HTML.

By the way, the one issue I can't reproduce is #6, with cargo-dynamictables-searchcolumn - it's possible that this is really just one of the other issues in disguise, or something. Maybe after the rest after the rest are fixed, we can see if this is still a problem.

Here's another change, which I think fixes issues 14 and 15, and also some new long-overdue i18n messages: 62b150dca2bb

In T372211#10067397, @Yaron_Koren wrote:

It takes care of all the JS-related issues, as far as I know; and it basically has the same approach you took, of replacing hardcoded HTML.

XSS 5 is still reproducible. To repeat the example path: /wiki/Special:CargoQuery?tables=cargo&fields=_pageName&format=dynamic+table&header_tooltips=<script>alert("awawa (xss 5)")</script>
XSS 6 is also reproducible, but that's understandable since you couldn't figure it out.

Also, you appended the wrong variable here (viewTableText instead of viewTableTag): https://phabricator.wikimedia.org/diffusion/ECRG/browse/master/libs/ext.cargo.recreatedata.js;62b150dca2bbe90ff9544959315650d739a5e98e$83

In T372211#10067397, @Yaron_Koren wrote:

By the way, the one issue I can't reproduce is #6, with cargo-dynamictables-searchcolumn - it's possible that this is really just one of the other issues in disguise, or something. Maybe after the rest after the rest are fixed, we can see if this is still a problem.

You can't use <> to break out of the input element, but quotes are not escaped. Therefore, you could use something like " style="position: fixed; top: 0; left: 0; height: 100%; width: 100%;" onmouseover="alert('xss in mediawiki:cargo-dynamictables-searchcolumn uwu (xss 6)') to break out of the attribute value and add extra attributes to the element.

Sorry - by "JS-related issues", I meant issues where the fix needs to be in the JavaScript code, rather than in PHP.

Thanks for pointing out that incorrect variable! That was due to a last-minute renaming. I fixed it at 2da55512ff85.

And thanks for pointing out the issue with #6. I had no idea that apparently neither Message::parse(), not Html::element(), escapes quotes... that was very good to know.

I just checked in what I think is the final fix for these XSS issues, based in part on your patch, and of course on your other feedback here, at, 1144d41c0d80. Please let me know if everything is indeed fixed!

In T372211#10069721, @Yaron_Koren wrote:

Sorry - by "JS-related issues", I meant issues where the fix needs to be in the JavaScript code, rather than in PHP.

Basically DOM-based XSSes, right?

Also, using Message::parse() is safe as element values since it's basically rendered wikitext. This means that you didn't need to replace ->parse() with ->escaped() in https://phabricator.wikimedia.org/rECRG1144d41c0d804b38ddd331617236bf6ea43f9e48#change-pzEF3usdN8Mg (which leads to this bug):

Also, XSS 12 and XSS 13 are not patched.

Thanks for pointing out these problems. Clearly I was over-optimistic before! I believe I've now taken care of the remiaining issues in dfe25479c450 and 8ca19783349d - though of course I could be wrong.

As for parse() vs. escaped() - I was told a while ago that escaped() is better to use than parse(), if the purpose is just to avoid XSS leaks, because it runs faster. Although obviously sometimes parse() is needed, as we've seen.

/wiki/Special:PageValues/Cargo/allowedvalues is still vulnyable to XSS 13:

The issue with the system message cargo-cargotables-deletereplacement not having its wikitext parsed is still present. Also, Special:DeleteCargoTable/cargo (when deleting) has double-escaped entities:

(But yes, Message::escaped() is (probably marginally) faster than Message::parse() since the former only needs to retrieve and escape the message, while the latter needs to put it through the parser).

It never ends! Thank you for your continued patience. I think I've fixed everything now with d60319b19027 and 59af70a2ec87, but I'm looking forward to finding out...

Great!