I also tried to check our VisualEditor integration, but that didn't load at all, with the following error:

Uncaught Error: Syntax error, unrecognized expression: [accesskey="<script>alert('accesskey-save')</script>"><script>alert('accesskey-save')</script><x y="()"]

Should we have an extra task for that?

In T386963#10568873, @Michael wrote:

I also tried to check our VisualEditor integration, but that didn't load at all, with the following error:

Uncaught Error: Syntax error, unrecognized expression: [accesskey="<script>alert('accesskey-save')</script>"><script>alert('accesskey-save')</script><x y="()"]

Should we have an extra task for that?

Probably. That feels like an issue in VisualEditor itself, but I'm unsure if it also has security implications.

In T386963#10570431, @Urbanecm_WMF wrote:

Fix is below:

T386963.patch1 KBDownload

@Michael (or other engineers), do you mind giving it a +2?

I'm a bit surprised that we only adjust the method for the description to parse. Are the others escaped client-side?

In T386963#10570790, @Michael wrote:

I'm a bit surprised that we only adjust the method for the description to parse. Are the others escaped client-side?

That is correct. As far as I can see, only the description is displayed using $( '<p>' ).html( this.taskTypeData.messages.description ). All other parts are displayed using the text() method, for example:

$( tagName )
	.addClass( 'suggested-edits-task-explanation-heading' )
	.text( this.taskTypeData.messages.name )

Relevant code seems to be modules/ext.growthExperiments.Homepage.SuggestedEdits/TaskExplanationWidget.js.

I wouldn't mind moving the escaping to a single location (all server side or all client side), but that should be probably done in a separate (public) patch.

In T386963#10571408, @Urbanecm_WMF wrote:

In T386963#10570790, @Michael wrote:

I'm a bit surprised that we only adjust the method for the description to parse. Are the others escaped client-side?

That is correct. As far as I can see, only the description is displayed using $( '<p>' ).html( this.taskTypeData.messages.description ). All other parts are displayed using the text() method, for example:

$( tagName )
	.addClass( 'suggested-edits-task-explanation-heading' )
	.text( this.taskTypeData.messages.name )

Relevant code seems to be modules/ext.growthExperiments.Homepage.SuggestedEdits/TaskExplanationWidget.js.

I wouldn't mind moving the escaping to a single location (all server side or all client side), but that should be probably done in a separate (public) patch.

Thanks! That explanation and the fact that I did not observe any alerts from any of the other message keys is good enough for me. I'm hereby giving a virtual +2 to @Urbanecm_WMF's change.

10:08 <urbanecm> !log Deployed security patch for T386963
10:09 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log

Deployed to production.

@sbassett, is it OK to backport in Gerrit and publish the task?

In T386963#10574228, @Urbanecm_WMF wrote:

@sbassett, is it OK to backport in Gerrit and publish the task?

Sure.

Change #1122163 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Fix XSS in Suggested edits

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

Change #1122163 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] SECURITY: Fix XSS in Suggested edits

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