Page MenuHomePhabricator

Provide a method (in MW's Sanitiser, or in WikiLambda?) that works like removeSomeTags() but also allows links through
Closed, ResolvedPublic

Description

Options:

  1. Add a new method to Sanitiser
  2. Use the extraTags flag in Sanitiser::removeSomeTags but then post-process to only allow some things(?)
  3. Add a new flag to Sanitiser::removeSomeTags
  4. Add a sub-class of Sanitiser in WikiLambda that behaves differently
  5. ???

Event Timeline

(from slack):

Sanitizer::removeSomeTags() isn't really meant for cleaning up untrusted HTML. It's really meant for cleaning up the author's displaytitle, which might have links and spans and other unsuitable things, and creating something suitable for the top <h1>, where <b> and <i> and <sup> and <sub> are ok but most other things are not. But the input to ::removeSomeTags() is parsed wikitext, so has already had most of the "bad stuff" removed.

If you really want to allow completely user-authored HTML strings, you need a real sanitization library, that will deal with style attributes and href allow/block lists and attempts at breaking out of its own DOM tree and etc etc.

You can use Remex to build that, but even the allow-list of tags and attributes in Sanitizer is probably not exactly the right fit for your application: it assumes (for example) that <a> tags are ok because wikitext bans <a> tags so the only way the sanitizer will see an <a> tag is because it was generated by wikitext, and we already validated the title/interwiki link/external link during the wikitext parse.

Another example: various methods in the sanitizer will allow data-mw-* tags, but from untrusted html strings you should not. In untrusted html you should also restrict the class and IDs allowed so as not to conflict with similar classes used by MediaWiki. You have to worry about spoofing attacks, where the untrusted HTML is used to mimic trusted parts of the mediawiki UX, like a login dialog, etc, or might have a link to a temporary account that lacks the usual grey background marking such, etc etc.

Technically attrCallback to Sanitizer::removeSomeTags() would probably do some of what you want to do with <a> tags, but it's marked @internal. And RemexRemoveTagHandler is marked @internal as well. I think you'd probably be better off cloning RemexRemoveTagHandler and simplifying it and starting with a very small allow-list of tags and attributes: enough to demonstrate your MVP, but (initially) little else.

That should make security review easier, and I think it is ultimately more secure than hoping that the list of tags and attributes from Sanitizer will map cleanly. There are a lot of wikitext-specific assumptions built in to Sanitizer that aren't immediately obvious --- for example, that attributes have all been normalized to be double quoted and with normalized character references. Methods in Sanitizer aren't typically labeled clearly as to which assume that and which don't.

wikimedia/css-sanitizer seems to work well for Extension:TemplateData; that's probably a reasonable reusable solution for WF as well, although my recollection was that there are some corner cases there in how it deals with standalone style sheets -vs- embedded style attributes.

tl;dr it's probably not too hard to use wikimedia/remex-html and wikimedia/css-sanitizer to write a very simple sanitization library that allows a small set of known-safe HTML/CSS, and that's probably a better starting point than using the Sanitizer class from mediawiki-core.

Change #1167971 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/WikiLambda@master] WikifunctionsPFragmentHandler: Introduce a bespoke sanitiser, for more control

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

Change #1167972 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/WikiLambda@master] WikifunctionsPFragmentSanitiserTokenHandler: Demonstrate dropping of data- attributes

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

Change #1167973 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/WikiLambda@master] WikifunctionsPFragmentHandler: Drop pointless sanitised-away style attributes

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

Change #1167974 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/WikiLambda@master] [WIP] WikifunctionsPFragmentSanitiserTokenHandler: Enable local links

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

Change #1167971 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] WikifunctionsPFragmentHandler: Introduce a bespoke sanitiser, for more control

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

Change #1167972 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] WikifunctionsPFragmentSanitiserTokenHandler: Demonstrate dropping of data- attributes

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

Change #1167973 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] WikifunctionsPFragmentHandler: Drop pointless sanitised-away style attributes

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

DSantamaria changed the task status from Open to In Progress.Jul 14 2025, 2:23 PM

Change #1167974 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] WikifunctionsPFragmentSanitiserTokenHandler: Enable local links

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