Wikimedia production has $wgRelatedArticlesDescriptionSource = 'wikidata';, I believe, so I don't think that environment is currently affected by this issue.

In T396413#10897393, @sbassett wrote:

Wikimedia production has $wgRelatedArticlesDescriptionSource = 'wikidata';, I believe, so I don't think that environment is currently affected by this issue.

I tried setting up Wikibase locally and using {{SHORTDESC:}} to set a description with HTML, but nothing at all was being displayed in the card. Not sure whether I set up something incorrectly or Wikibase just strips all HTML tags from the short description.

Additional reproduction steps: if your $wgScriptPath isn’t /w, add the following to your LocalSettings:

$wgRelatedArticlesUseCirrusSearchApiUrl = wfScript( 'api' );

Apparently the extension hard-codes /w/api.php for some reason.

I’m afraid the XSS is reproducible with the Wikibase/Wikidata description source by

• having an item with a description like <img src="" onerror="alert(1)"> (note: <script>alert(1)</script> won’t do, the script gets injected into the DOM but not executed)
• connecting a wiki page to it via sitelink
• declaring that wiki page as a related article

(Site note for @sbassett, as far as I can tell my volunteer account didn’t get an email for that subscription. I probably would’ve only seen this next time someone commented on here, if I hadn’t come across it in the list of new tasks anyway.)

Regarding the “Cause” section, my impression is that the missing escaping should take place in step 5, i.e. in the RelatedArticles function. The vulnerability was probably introduced in Simplify the RelatedArticles extension to use Codex CSS components, prior to which the description / extract was set in CardView.js using .text() (letting the browser escape it).

Suggested patch:

diff --git c/resources/ext.relatedArticles.readMore/RelatedArticles.js w/resources/ext.relatedArticles.readMore/RelatedArticles.js
index 997fabd0fa..f48fd61d62 100644
--- c/resources/ext.relatedArticles.readMore/RelatedArticles.js
+++ w/resources/ext.relatedArticles.readMore/RelatedArticles.js
@@ -28,4 +28,4 @@ const RelatedArticles = ( options ) => [
 						<span class="cdx-card__text">
-							<span class="cdx-card__text__title">${ card.label }</span>
-							<span class="cdx-card__text__description">${ card.description }</span>
+							<span class="cdx-card__text__title">${ mw.html.escape( card.label ) }</span>
+							<span class="cdx-card__text__description">${ mw.html.escape( card.description ) }</span>
 						</span>

It’s not the most elegant (ideally this should probably be ported to Vue), but I think for a security patch it should be good enough (and avoid the Git conflicts that a larger refactoring would bring).

(Pulling into our board to reflect that I spent time on it, but review should probably be done by RelatedArticles devs.)

Alternative patch that escapes stuff more comprehensively:

I haven’t been able to test all of this (in particular, I haven’t been able to get my local API to return a page image yet), so it’s possible some of this is broken.

diff --git c/resources/ext.relatedArticles.readMore/RelatedArticles.js w/resources/ext.relatedArticles.readMore/RelatedArticles.js
index 997fabd0fa..c1209f32d5 100644
--- c/resources/ext.relatedArticles.readMore/RelatedArticles.js
+++ w/resources/ext.relatedArticles.readMore/RelatedArticles.js
@@ -15,11 +15,11 @@ const RelatedArticles = ( options ) => [
 			`<aside class="noprint">`,
 				( options.heading ) ?
-				`<h2 class="read-more-container-heading">${ options.heading }</h2>` : ``,
+				`<h2 class="read-more-container-heading">${ mw.html.escape( options.heading ) }</h2>` : ``,
 				`<ul class="read-more-container-card-list">`,
-					options.cards.map( ( card ) => `<li title="${ card.label }">
-					<a href="${ card.url }" ${ options.clickEventName ? `data-event-name="${ options.clickEventName }"` : '' }><span class="cdx-card">
+					options.cards.map( ( card ) => `<li title="${ mw.html.escape( card.label ) }">
+					<a href="${ mw.html.escape( card.url ) }" ${ options.clickEventName ? `data-event-name="${ mw.html.escape( options.clickEventName ) }"` : '' }><span class="cdx-card">
 						<span class="cdx-card__thumbnail cdx-thumbnail">
 						${ ( card.thumbnail && card.thumbnail.url ) ?
-							`<span class="cdx-thumbnail__image" style="background-image: url('${ card.thumbnail.url }')"></span>` :
+							`<span class="cdx-thumbnail__image" style="background-image: url('${ mw.html.escape( card.thumbnail.url ) }')"></span>` :
 							`<span class="cdx-thumbnail__placeholder">
 								<span class="cdx-thumbnail__placeholder__icon"></span>
@@ -27,6 +27,6 @@ const RelatedArticles = ( options ) => [
 						</span>
 						<span class="cdx-card__text">
-							<span class="cdx-card__text__title">${ card.label }</span>
-							<span class="cdx-card__text__description">${ card.description }</span>
+							<span class="cdx-card__text__title">${ mw.html.escape( card.label ) }</span>
+							<span class="cdx-card__text__description">${ mw.html.escape( card.description ) }</span>
 						</span>
 					</a>

I can look at the patches today.

Partial demo in production: Abraham Abraham (Q4668740) erroneously has a German description containing &amp;amp;. If I go to an arbitrary dewiki page; use the browser dev tools to add class="read-more-container" to an arbitrary node in the content; run mw.config.set('wgRelatedArticles', {'Abraham Abraham': ''}); and then run await mw.loader.using('ext.relatedArticles.readMore.bootstrap') – then the related card will show:

I think any much better demo is going to be hard to put together without making changes on-wiki that could potentially give a hint towards this task.

Thanks, @Lucas_Werkmeister_WMDE for the attention here. And sorry for the weird sub issues.

Anyhow, the proposed patch from T396413#10900136 LGTM and seems fairly comprehensive. If @Jdlrobson-WMF thinks it looks safe and reasonable, I think we should plan to get this deployed to Wikimedia production maybe today or tomorrow, or certainly by Thursday when everything should be on 1.45.0-wmf.5. I'm also happy to OS any production-testing that we might want to perform before or after deploying the security patch.

In T396413#10900136, @Lucas_Werkmeister_WMDE wrote:

Alternative patch that escapes stuff more comprehensively:

0001-SECURITY-Escape-card-details.patch2 KBDownload

I haven’t been able to test all of this (in particular, I haven’t been able to get my local API to return a page image yet), so it’s possible some of this is broken.

diff --git c/resources/ext.relatedArticles.readMore/RelatedArticles.js w/resources/ext.relatedArticles.readMore/RelatedArticles.js
index 997fabd0fa..c1209f32d5 100644
--- c/resources/ext.relatedArticles.readMore/RelatedArticles.js
+++ w/resources/ext.relatedArticles.readMore/RelatedArticles.js
@@ -15,11 +15,11 @@ const RelatedArticles = ( options ) => [
 			`<aside class="noprint">`,
 				( options.heading ) ?
-				`<h2 class="read-more-container-heading">${ options.heading }</h2>` : ``,
+				`<h2 class="read-more-container-heading">${ mw.html.escape( options.heading ) }</h2>` : ``,
 				`<ul class="read-more-container-card-list">`,
-					options.cards.map( ( card ) => `<li title="${ card.label }">
-					<a href="${ card.url }" ${ options.clickEventName ? `data-event-name="${ options.clickEventName }"` : '' }><span class="cdx-card">
+					options.cards.map( ( card ) => `<li title="${ mw.html.escape( card.label ) }">
+					<a href="${ mw.html.escape( card.url ) }" ${ options.clickEventName ? `data-event-name="${ mw.html.escape( options.clickEventName ) }"` : '' }><span class="cdx-card">
 						<span class="cdx-card__thumbnail cdx-thumbnail">
 						${ ( card.thumbnail && card.thumbnail.url ) ?
-							`<span class="cdx-thumbnail__image" style="background-image: url('${ card.thumbnail.url }')"></span>` :
+							`<span class="cdx-thumbnail__image" style="background-image: url('${ mw.html.escape( card.thumbnail.url ) }')"></span>` :
 							`<span class="cdx-thumbnail__placeholder">
 								<span class="cdx-thumbnail__placeholder__icon"></span>
@@ -27,6 +27,6 @@ const RelatedArticles = ( options ) => [
 						</span>
 						<span class="cdx-card__text">
-							<span class="cdx-card__text__title">${ card.label }</span>
-							<span class="cdx-card__text__description">${ card.description }</span>
+							<span class="cdx-card__text__title">${ mw.html.escape( card.label ) }</span>
+							<span class="cdx-card__text__description">${ mw.html.escape( card.description ) }</span>
 						</span>
 					</a>

+ 1. Can confirm this works for text extracts and doesn't impact thumbnails but I'd need to look more closely for Wikidata descriptions (haven't been able to replicate this yet). It's definitely not expected that descriptions render as HTML

However, I think we might need a more complete solution before making this public. Note, this looks like it would also impact anything using TextExtracts which is commonly used by various apps on toolforge and https://global-search.toolforge.org/?q=extracts%5B%27%22%5D&regex=1&namespaces=8&title= so we should consider a fix for TextExtracts too (this HTML it generates should be trusted and sanitized and I'm not sure why it isn't here).
https://ur.wikipedia.org/wiki/%D9%85%DB%8C%DA%88%DB%8C%D8%A7%D9%88%DB%8C%DA%A9%DB%8C:Gadget-showIntroOnHover.js for example.

@Lucas_Werkmeister_WMDE where might Wikidata descriptions be rendered as raw HTML - can they contain <strong> tags for example?

This is also reproducible with 'pagedescription' as the description source along with Description2's parser function.

I am going to be out from tomorrow so cc'ing @Catrope who should be able to help with my absence. I think Lucas's patch is good to go, but I would recommend quickly assessing whether there is any impact outside RelatedArticles before applying the fix and making this public.

In T396413#10902065, @Jdlrobson-WMF wrote:

...I would recommend quickly assessing whether there is any impact outside RelatedArticles before applying the fix and making this public.

We'd plan to deploy this as a security patch to Wikimedia production. We'd want to hold off on making it public in gerrit until the next supplemental security release.

This patch looks good to go. Let me know if I can help with deploying it.

In T396413#10901525, @Jdlrobson-WMF wrote:

Note, this looks like it would also impact anything using TextExtracts which is commonly used by various apps on toolforge and https://global-search.toolforge.org/?q=extracts%5B%27%22%5D&regex=1&namespaces=8&title= so we should consider a fix for TextExtracts too (this HTML it generates should be trusted and sanitized and I'm not sure why it isn't here).

The explaintext parameter is explicitly set to 1 by RelatedArticles, which causes TextExtracts to return unsanitized HTML. Without this parameter, TextExtracts would return safe HTML.

To add onto the above, this also affects a skin (Citizen) that seem to use similar code, where this is also reproducible with TextExtracts, Description2 and Wikibase: https://codesearch.wmcloud.org/search/?q=prop+\%2B%3D+'\|extracts'&files=&excludeFiles=
If necessary, I'll wait with reporting it until this task is fixed in WMF prod, since the skin is 3rd party and the fix would be pushed to GitHub within a short time.
Considering this, it's not unlikely that more extensions have similar vulnerabilities as well.

In T396413#10901525, @Jdlrobson-WMF wrote:

@Lucas_Werkmeister_WMDE where might Wikidata descriptions be rendered as raw HTML - can they contain <strong> tags for example?

Wikidata descriptions should always be plain text – the only kind of similar case I can think of is when they’re shown in search results (wrapped in <span class="searchmatch">):

In T396413#10902085, @sbassett wrote:

In T396413#10902065, @Jdlrobson-WMF wrote:

...I would recommend quickly assessing whether there is any impact outside RelatedArticles before applying the fix and making this public.

We'd plan to deploy this as a security patch to Wikimedia production. We'd want to hold off on making it public in gerrit until the next supplemental security release.

Scott, as citizen is affected by similar (see T396413#10902346 above), how would you like to handle this? I don't really want Citizen releasing a patch for the same bug to help anyone find the issue but it's not managed by WMF / on gerrit so harder to co-ordinate for the supplemental. https://phabricator.wikimedia.org/p/alistair3149/ Is the citizen developer.

In T396413#10904575, @RhinosF1 wrote:

Scott, as citizen is affected by similar (see T396413#10902346 above), how would you like to handle this? I don't really want Citizen releasing a patch for the same bug to help anyone find the issue but it's not managed by WMF / on gerrit so harder to co-ordinate for the supplemental. https://phabricator.wikimedia.org/p/alistair3149/ Is the citizen developer.

It's something to keep an eye on, but I think we need to keep this issue protected in line with Wikimedia's standard protocol for core/bundled code. The security team is planning to get this patch deployed this Thursday during or just after the late backport window. I'm fine with subscribing the Citizen maintainer here after that deployment, but we'll also want to caution them that they should not publicly-disclose this issue in any way until the end of this month, when the Wikimedia core/bundled security release should be made available.

Patch deployed to Wikimedia production by @Mstyles.

looks like our part is done on this so I'm removing it from our board

Adding the Citizen maintainer as discussed before (T396413#10905186), since Citizen is affected by a similar issue

Thanks for adding me to the task. I will patch it within Citizen as well.

Like @Jdlrobson-WMF mentioned, there are also numerous community tools that trusts and uses the API output that comes from these extensions. Because of that, we should probably patch the upstream extensions to sanitize the API input to minimize the impact, at the very least for Wikidata and TextExtracts.

Change #1166026 had a related patch set uploaded (by Jly; author: Jly):

[mediawiki/extensions/RelatedArticles@REL1_43] SECURITY: Escape card details

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

Change #1166027 had a related patch set uploaded (by Jly; author: Jly):

[mediawiki/extensions/RelatedArticles@REL1_42] SECURITY: Escape card details

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

Change #1166029 had a related patch set uploaded (by Jly; author: Jly):

[mediawiki/extensions/RelatedArticles@REL1_39] SECURITY: Escape card details

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

Change #1166029 abandoned by Jly:

[mediawiki/extensions/RelatedArticles@REL1_39] SECURITY: Escape card details

Reason:

Not supported in REL1_39

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

Change #1166027 abandoned by Jly:

[mediawiki/extensions/RelatedArticles@REL1_42] SECURITY: Escape card details

Reason:

Unsupported in REL1_42

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

The security patch needed a few further changes to pass CI, can someone review them while Jon is on vacation?

Change #1166024 merged by jenkins-bot:

[mediawiki/extensions/RelatedArticles@master] SECURITY: Escape card details

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

Change #1166025 merged by jenkins-bot:

[mediawiki/extensions/RelatedArticles@REL1_44] SECURITY: Escape card details

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

Change #1166026 merged by jenkins-bot:

[mediawiki/extensions/RelatedArticles@REL1_43] SECURITY: Escape card details

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