Adding a patch for this:

In T347704#9210907, @sbassett wrote:

Thanks, though we do not ever consider message-layer XSS's to be high risk - many of them we just publicly merge in gerrit. I think we can likely get this deployed during this Monday's security deployment window.

Sounds good, will keep the security risk advice in mind :)

I found a lot more XSS vectors in PageTriage via messages while browsing, so here is a patch that should fix everything I found (the previous/removed comment had a errenous patch):

In T347704#9212310, @Soda wrote:

I found a lot more XSS vectors in PageTriage via messages while browsing, so here is a patch that should fix everything I found (the previous/removed comment had a errenous patch):

0001-Prevent-multiple-messages-from-causing-XSS.patch6 KBDownload

Deployed

In T347704#9217957, @Mstyles wrote:

Deployed

I dropped this from /srv/patches due to T347726#9218657.

@Mstyles What's the status of this task, are there any steps/fixes I need to wrt to @taavi's comments ?

In T347704#9374516, @Soda wrote:

@Mstyles What's the status of this task, are there any steps/fixes I need to wrt to @taavi's comments ?

At the very least, we'd need an updated patch that applies to wmf/1.42.0-wmf.7 and/or current master. The one above doesn't, even via a 3-way merge attempt:

error: modules/ext.pageTriage.views.list/ext.pageTriage.listControlNav.js: No such file or directory
error: modules/ext.pageTriage.views.list/ext.pageTriage.listView.js: No such file or directory

Then we'd need a little code review. After that, we could deploy it during the next security deployment window (typically Monday afternoons in North America).

I've updated the patch to apply on the latest master

In T347704#9376021, @Soda wrote:

I've updated the patch to apply on the latest master

0001-Prevent-XSS-via-messages-in-PageTriage.patch7 KBDownload

CR +1. Patch applies to current ext:PageTriage@master and seems correct at a glance from a security perspective (essentially just passing affected mw.msg() calls through .text() first). It'd be nice though if someone with a bit more familiarity with ext:PageTriage (@kostajh, @Umherirrender, @MPGuy2824) could provide +1 or +2 before we attempt another security-deploy with this updated patch.

I am not familiar with js code, but it looks like using other message function would be easier (mw.message( 'key' ).escaped(). Adding extra span tag could break user scripts.

mw.msg( 'key' ) is just a shortcut for mw.message( 'key' ).text(), so that would be minimal functional change.

The change in ToolView.js also adds the label tag, which is a good idea, but more as just a security patch.

So just can give CR +1 here.

In T347704#9379456, @sbassett wrote:

In T347704#9376021, @Soda wrote:

I've updated the patch to apply on the latest master

0001-Prevent-XSS-via-messages-in-PageTriage.patch7 KBDownload

CR +1. Patch applies to current ext:PageTriage@master and seems correct at a glance from a security perspective (essentially just passing affected mw.msg() calls through .text() first). It'd be nice though if someone with a bit more familiarity with ext:PageTriage (@kostajh, @Umherirrender, @MPGuy2824) could provide +1 or +2 before we attempt another security-deploy with this updated patch.

Adding @jsn.sherman and @Scardenasmolinar as two engineers who have worked on this code recently and might want to comment.

Thanks, @kostajh. At some point we should probably just verify whether ?uselang=x-xss is producing any results anymore.

@kostajh thanks for tagging; @Scardenasmolinar and I will look at this together on Wednesday.

I can give a CR +2 for this. I agree with @Umherirrender's feedback, but this doesn't appear to break anything in manual testing, and this interface that is being patched is going away soon.

In T347704#9403912, @jsn.sherman wrote:

I can give a CR +2 for this. I agree with @Umherirrender's feedback, but this doesn't appear to break anything in manual testing, and this interface that is being patched is going away soon.

Ok, we can try to get this deployed during next Monday's security deployment window. cc: @Mstyles.

@sbassett sounds good!!

In T347704#9376021, @Soda wrote:

I've updated the patch to apply on the latest master

0001-Prevent-XSS-via-messages-in-PageTriage.patch7 KBDownload

This patch was deployed, however there was an issue with the scap deploy command on host, so I've filed a phab ticket. If someone could verify the issue is fixed in production, that would be great.

I did a quick test on test.wikipedia.org (making sure some benign bolding does not get rendered) and it appears to not get rendered (i.e. the issue appears to be fixed) :)

@Soda thank you! This will go out in the next Security release for extensions which will be early January 2024

Change 989177 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Prevent XSS via messages in PageTriage

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

In T347704#9449944, @gerritbot wrote:

Change 989177 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] Prevent XSS via messages in PageTriage

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

Fix was merged into master last week, but the patch was still in the deployment server, it's been removed now.