What about in flow where posts are saved as html, instead of serializing between wikitext & back again?

Edit: I tested, flow has same issue, but is not a stored XSS

I'm not sure what you mean. I don't think that exists yet? When such a thing exists, I would expect it to have a sanitization step which would catch this.

This patch fixes the issue for me, and it doesn't seem to cause any extra spurious './' in resulting wikitext:

In general, the way we handle 'hrefPrefix' is confusing to me. If I remember correctly, extra href prefixes used to be used by Parsoid in the HTML of subpages, so that a link to "C" on a page called "A/B" would actually link to "C" (../C) rather than "A/C" (./C). But it seems that today's Parsoid always outputs links prefixed with ./ and instead uses a <base href="http://…/wiki/"> for links on subpages to point to the right place. Possibly we can remove a lot of that code to make it easier to spot issues like this?

(Somehow it seems that Editing engineers who could review this patch are not in the Security group, so CC-ing some folks)

We use the DOMPurify library in situations where HTML might be shared between clients without being converted to wikitext, although at the moment that is only the experimental collaborative editor.

Note that DomPurify by itself isnt generally sufficient as we also have the goal of preventing leaking IP addresses so anything that could trigger an http request (e.g. images) needs to be filtered too

Patch looks good BTW

@Bawolff how would we go about filtering external images in the client?

In T206357#4646496, @matmarex wrote:

In general, the way we handle 'hrefPrefix' is confusing to me. If I remember correctly, extra href prefixes used to be used by Parsoid in the HTML of subpages, so that a link to "C" on a page called "A/B" would actually link to "C" (../C) rather than "A/C" (./C). But it seems that today's Parsoid always outputs links prefixed with ./ and instead uses a <base href="http://…/wiki/"> for links on subpages to point to the right place. Possibly we can remove a lot of that code to make it easier to spot issues like this?

For reference, Parsoid stopped generating them in 2014 as a result of T72743, and it ignored them in input as well. We should remove everything dealing with hrefPrefix and hardcode ./ everywhere.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/487894

This has been merged and deployed (and fixed) for a long time now, but it has been lingering on the VisualEditor workboard. I think this is because some folks on the Editing team don't have the access to view security tasks (they should get it, but never asked for it) and so no one pushed it along. I'm going to make it public so that we can close it per our usual process.

Change 501040 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Remove support for page title "hrefPrefix" other than './'

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

Change 501041 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/Cite@master] Remove "hrefPrefix" from tests

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

This is the followup/hardening change I proposed here earlier.

Change 501041 merged by Jforrester:
[mediawiki/extensions/Cite@master] Remove "hrefPrefix" from tests

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

Change 501040 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Remove support for page title "hrefPrefix" other than './'

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

Change 517907 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@REL1_33] Remove support for page title "hrefPrefix" other than './'

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

Change 517907 merged by Krinkle:
[mediawiki/extensions/VisualEditor@REL1_33] Remove support for page title "hrefPrefix" other than './'

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