r99802 MediaWiki - Code Review archive

This had been discussed and reviewed by Tim Starling via email a few weeks back. Peter - can you please add Tim's code review notes in a comment so they're public?

How secure it is depends mostly on how you use it in templates. With $wgRawHtml enabled, as it is on wikimediafoundation.org, it would be possible to construct a template which passes a user input string through to an unsafe context, such as an onclick attribute.

The performance also concerns me. In the past, pages which are linked from banners have had a very high request rate, and this special page doesn't have any caching. If you linked directly to it from banners on all wikis, the site would probably go down.

Can you give me some more details about the problem you're trying to solve? Were there a large number of landing pages created last year? If so, can you give me links to some of them?

-- Tim Starling

In re performance, my understanding is that we planned to cache it. Clearly I missed something that is preventing this page from being cached then?

The extension would be deployed on donate.wikimedia.org which will also have $wgRawHtml enabled. We are aware of the issue with the templates, but user accounts on donate.wikimedia.org are meant to be heavily restricted to only those people who need it for the fundraiser. Am I correct in my thinking that this is an underlying issue with $wgRawHtml and that this extension does not exacerbate the vulnerability?

The problem that we are trying to solve is indeed the number of pages that have to be created for this year's fundraiser. Last year there was not much of a push for localization, but this year we are attempting, with the GlobalCollect integration, to be able to accept as many currencies and payment methods as possible. Each of these will need to be localized by country depending on what is available there. Our intent here was to break the landing pages into two major sections, the appeal and the form. If we can localize a form or set of forms once for each country, and easily swap out the appeal with the extension, we can avoid having to make another static page for each combination of form and template.

Doing A/B testing for each of the ~200 countries for payment methods would result in ~400 pages per appeal version (we are often also wanting to test subtle changes in appeals). This extension would allow us to create the 400 forms and then just swap out the appeals as we desired. Additionally, we could do the same type of swapping for any part of the landing page template if we desired.

-- Peter Gehres

Special pages are uncached by default.

Normally, only people with user accounts can edit template parameters. The extension makes it so that template parameters can come from URLs, so anyone who can write a URL can edit template parameters. This could lead to an XSS vulnerability if the template is constructed in a particular way. For example, if you had:

<a href="{{{target}}}">Click here</a>

Then a URL with a target parameter could cause arbitrary JavaScript to be executed when the link is clicked, using a target starting with "javascript:". The escaping that the extension does doesn't help in this particular context. If a logged-in user can be tricked into clicking such a link, their user account could be compromised by the attacker who constructed the URL.

-- Tim Starling

I definitely agree that there is a potential for XSS with this. We had previously decided to just be careful with this and escape the parameters before use as well as not use them in a potentially unsafe way. Do you think that running the parameters through a [a-zA-Z0-9_-]+ regex would eliminate most, if not all, of the potential for XSS?

From #wikimedia-dev, 2011/10/20

[01:12am] pgehres: TimStarling: if you'd like to chat about the extension more synchronously, I'm around [01:12am] TimStarling: pgehres: I think running the parameters through a restrictive regex would prevent security issues [01:13am] TimStarling: but it would limit what you can do with the extension [01:13am] TimStarling: if you tried to put text in there for display, it wouldn't be long before you started missing your punctuation [01:13am] pgehres: TimStarling: yes, but I think we will generally be loading other templates for the appeal text since it will need to be localized [01:14am] pgehres: I am willing to sacrifice some flexible for security [01:14am] TimStarling: if you just made 400 pages, and didn't use the extension at all, it would be pretty secure [01:15am] pgehres: That is definitely true, but we would need to make many times that many [01:15am] pgehres: Its about 400 per appeal [01:15am] TimStarling: fair enough [01:16am] TimStarling: obviously page creation can be automated, but that won't make sense as a solution above a few thousand pages [01:18am] pgehres: we did think about bots as well, but we are already passing country and language in the query string and it seemed more natural to do this and do even more switches in wiki-markup (as well as less fragile during the fundraiser) [01:19am] pgehres: I will go ahead and implement the regex if you think that's a reasonable solution [01:20am] TimStarling: yes, I think it will work, barring some really unlikely template constructions [01:20am] pgehres: okay, thank you very much for looking at this [01:20am] TimStarling: <script>eval(base64_decode('{{{value}}}')); </script>