Patch:

-1 on the patch.

The original link in the i18n message has target="_blank" specifically so that clicking on it would open the link in a new tab. The new definition does not include this. I think we shouldn't change that through a security patch in case it was deliberately intended.

If the approach of your patch doesn't allow the above, we can always fix this by defining the message in RawHtmlMessages of extension.json. That would address the security concern by making the message need elevated permissions to edit on the site.

Apart from building the anchor element dynamically, which would require the URL to be hardcoded or extracted to a new message (which in turn could cause another XSS through javascript: links that would have to be avoided), I think adding it to RawHtmlMessages is the best solution for now. This could probably later be fixed in a non-security follow-up patch.

New patch:

In T402076#11091418, @SomeRandomDeveloper wrote:

Patch:

T402076.patch2 KBDownload

+2

Works locally for me (I needed to restart my wiki to get the patch to work in case anyone else is testing this):

In T402076#11094611, @Dreamy_Jazz wrote:

In T402076#11091418, @SomeRandomDeveloper wrote:

Patch:

T402076.patch2 KBDownload

+2

Works locally for me (I needed to restart my wiki to get the patch to work in case anyone else is testing this):

Deployed

Given the size of the QA queue and the low risk that this security issue presents (due to needing at least admin rights to abuse this + comparing to other more important security issues), I think this can skip QA.

Change #1189186 had a related patch set uploaded (by Reedy; author: SomeRandomDeveloper):

[mediawiki/extensions/SecurePoll@master] SECURITY: Parse message instead of inserting it as HTML

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

Change #1189186 merged by jenkins-bot:

[mediawiki/extensions/SecurePoll@master] SECURITY: Parse message instead of inserting it as HTML

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

Looks like this was added in rESPOb6479304fbd3: Prefer a parameter for importing translations in SecurePoll, so no backport needed.

In T402076#11094611, @Dreamy_Jazz wrote:

In T402076#11091418, @SomeRandomDeveloper wrote:

Patch:

T402076.patch2 KBDownload

+2

Works locally for me (I needed to restart my wiki to get the patch to work in case anyone else is testing this):

Question… which patch was +2ed here? Asking because the context of the comments prior to T402076#11094611 would suggest that it maybe might have been intended to be the second version of the patch (ie., the one uploaded in T402076#11093305); but this comment quotes the one that uploaded the first version of the patch — which is now also the one that's been merged in Gerrit.

In T402076#11287276, @A_smart_kitten wrote:

In T402076#11094611, @Dreamy_Jazz wrote:

In T402076#11091418, @SomeRandomDeveloper wrote:

Patch:

T402076.patch2 KBDownload

+2

Works locally for me (I needed to restart my wiki to get the patch to work in case anyone else is testing this):

Question… which patch was +2ed here? Asking because the context of the comments prior to T402076#11094611 would suggest that it maybe might have been intended to be the second version of the patch (ie., the one uploaded in T402076#11093305); but this comment quotes the one that uploaded the first version of the patch — which is now also the one that's been merged in Gerrit.

That is indeed the wrong patch... unfortunately I think it's too late to undo this now from a security perspective, unless the message would be renamed along with its addition to wgRawHtmlMessages

In T402076#11308313, @SomeRandomDeveloper wrote:

That is indeed the wrong patch... unfortunately I think it's too late to undo this now from a security perspective, unless the message would be renamed along with its addition to wgRawHtmlMessages

Maybe I'm missing something (in which case I apologise!), but how come it'd be too late to undo? Couldn't e.g. there be a patch pushed to Gerrit which is effectively a combined (a) revert of F65758960 & (b) application of F65774953?

In T402076#11339572, @A_smart_kitten wrote:

In T402076#11308313, @SomeRandomDeveloper wrote:

That is indeed the wrong patch... unfortunately I think it's too late to undo this now from a security perspective, unless the message would be renamed along with its addition to wgRawHtmlMessages

Maybe I'm missing something (in which case I apologise!), but how come it'd be too late to undo? Couldn't e.g. there be a patch pushed to Gerrit which is effectively a combined (a) revert of F65758960 & (b) application of F65774953?

If you add the message to wgRawHtmlMessages now, it would in theory be possible for sysops to edit it and insert a payload before their wiki upgrades the extension when the next security/MW release happens (or before the change is deployed via the train on WMF wikis). The only way to avoid that is to rename the message

Ahhh yep. good point. ty @SomeRandomDeveloper.
(On that note though, I wonder if security patches like F65774953 should also rename the message as part of the patch? As otherwise, it seems like such a patch wouldn't remediate the XSS in cases where a sysop without editsitejs had already inserted a payload to the message prior to the security release being made available - e.g. if they'd discovered & exploited the XSS themselves before a fix was released.)

On the subject of this task specifically, I guess it's a question for @Dreamy_Jazz / Product Safety and Integrity as to whether anything more should happen here, bearing in mind the comments in T402076#11093158.