r50472 MediaWiki - Code Review archive

Is this doing some kind of double-encoding of parameters? It looks totally wrong...

r50479 is some sort of follow-up, not sure exactly what for.

The return of $wgRequest->getVal() is going to be urlencoded, surely? I can't see any attempt in the call stack to *un*encode it before it gets back to this level, unless it's *really* deep. And it's silly to build the entire URL just to be able to use Title::newFromURL().

r50479 seems to be reverting to injecting the extra JavaScript by direct <script> elements; I had previously called $wgOut->addScriptFile(). Why didn't that work?

The code that inserts the script is part of the portlet code in the sidebar. This HTML code is sometimes retrieved and inserted via XMLHttpRequests, so the normal output handler of MediaWiki is not used in this method.

I'll have a look at this.

The double escaping was introduced in the diff form bug18735: An already escaped URL (from Title::get*URL()) was passed to Xml::tags(). In one of the two relevant places, the 'noescape' option was passed to Xml::tags(), but I guess that's wrong (e.g. the output of wfMsg() is another value passed to Xml::tags() which needs to be escaped). I've committed a change in r50511, where the URLs are decoded and then passed to Xml::tags() which does the usual escaping.

The 'noescape' option was left over from [https://bugzilla.wikimedia.org/show_bug.cgi?id=18719 bug18719], which I was convinced was unnecessary. Maybe not...

urldecode() is unnecessary because it's done inside PHP. Title::newFromUrl() is identical to Title::newFromText() except for a random ancient hack that breaks titles with question marks in them. It does not decode anything. Using it is always incorrect. These two errors mostly cancel each other out as of r50511 except for the question marks.

This extension clearly needs a review from top to bottom by someone experienced with MediaWiki. The impression I get on scanning the code is that there are lots of obvious errors like these, as well as scary code, in the sense that there is potential for XSS. I'm wondering if it should be disabled on Wikimedia for a week or so post-scap, until we have time to do that.

No one from enwiki is going to complain about that; we've already disabled it by hiding the sidebar with CSS. It needs a lot of cleanup from the frontend as well as the back.

I don't intend to sound harsh, but:

• We're very well aware of the discussion on en.wp and we are working on ways to improving the usability and other aspects of the extension (I guess that's what you mean with "lot of cleanup from the frontend"). But IMHO, this discussion doesn't belong here.
• If the PHP code needs "a lot of cleanup" – especially wrt security issues – I'd be very happy to hear about more concrete suggestions where to improve the code, as I wrote in my reply to Tim.
• The problems we are (were) discussing for this changeset here were introduced by your patch (though of course I was the one who applied it and thus I am the one to blame b/c I apparently didn't review it carefully enough). Please don't get me wrong: I'm very thankful for the improvements contained in your patch! But these particular problems weren't present before this changeset and AFAIK they have been fixed in the current version of the extension.
• The "Create a book" portlet might be currently hidden in en.wp, but the extension is still installed and can be accessed. For example, the "PDF version" link still exists and the Special:Book page is still accessible. So if there really are any security issues with the Collection code, hiding some HTML via CSS doesn't help very much.

Don't get me wrong, the fact that it's my patch is the reason I'm here. I suppose in the abstract, 'not using core MediaWiki functionality at all' *is* a way of 'not using the wrong MediaWiki functionality', but I believe (hope certainly) that this patch is at worst a step sideways. Certainly the plethora of hardcoded HTMl is *not* the best solution. I'm with you all the way on your comment below: if Title::newFromUrl() is deprecated, it should be marked as such, else people like you or I will be inclined to use it in good faith.

As you say, the how-to-improve-usability discussion doesn't belong here; all I meant by this comment was that, since we've done our best to 'remove' it already, no one is going to miss it if it's take off for a few weeks for an overhaul (by "it needs a lot of cleanup", I mean the UI in general, not the code specifically). Although I admit I don't really see what the benefit of that is: it's not like it's a server which can't be worked on and used at the same time. Either there are security vulnerabilities, in which case it should be disabled *now* and left that way until they're fixed, or there are only usability issues, in which case there's no harm in leaving it installed but largely disabled until said issues are resolved. As you say, the PDF version link is useful quite apart from the full collections system.

OK, full ACK here :)

If using newFromUrl() is "always incorrect" why does it still exist without any such comment in Title.php and why is it used in several places in MediaWiki code? Anyway, I've changed all three occurrences of newFromUrl() into newFromText() in r51464.

I'm a bit puzzled by your comment though: Aren't the problems introduced in this changeset here fixed with r50511 and r50542? If not, what are the two errors that "mostly cancel each other out" in the current version of the extension?

And regarding your last paragraph: Of course, I'd be more than thankful for all pointers to "scary code" and will happily fix it!

I added such a comment to Title::newFromURL() in r51326 during the course of the current batch of code review, when I realised how many people were using it.

I think this is all fixed now.