Last modified: 2009-05-08 20:02:15 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T20719, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 18719 - Do not unavoidably HTML-escape link attributes
Do not unavoidably HTML-escape link attributes
Status: RESOLVED INVALID
Product: MediaWiki
Classification: Unclassified
General/Unknown (Other open bugs)
1.15.x
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-08 17:44 UTC by Happy-melon
Modified: 2009-05-08 20:02 UTC (History)
1 user (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch to Xml.php and Linker.php against r50336 (24.62 KB, patch)
2009-05-08 17:44 UTC, Happy-melon
Details
Remove other random stuff that's on my svn checkout from the diff (3.60 KB, patch)
2009-05-08 17:45 UTC, Happy-melon
Details

Description Happy-melon 2009-05-08 17:44:26 UTC
Created attachment 6096 [details]
Patch to Xml.php and Linker.php against r50336

A significant number of the legitimate/unavoidable uses of hardcoded HTML in the codebase are to hardcode links; it is necessary to use hardcoding instead of the functions from Xml.php or Linker.php because all these functions HTML-escape attribute values.  This means it is impossible to use Linker::link() or Xml::tags() to add a link when you need to add javascript onclick handlers or other things that need raw HTML.  The HTML-escaping needs to be avoidable.

This patch adds a parameter through the call stack right down to Xml::expandAttributes() to optionally circumvent the call to Sanitizer::encodeAttribute(); by the time this bubbles up to Linker::link() it's wrapped in one of the available $options values.  Tested.
Comment 1 Happy-melon 2009-05-08 17:45:47 UTC
Created attachment 6097 [details]
Remove other random stuff that's on my svn checkout from the diff

Oops, loaded up the diff in notepad, cleaned out all the unrelated changes, but then uploaded it here before saving it :D
Comment 2 Niklas Laxström 2009-05-08 18:59:40 UTC
Huh? Escaping shouldn't affect onclick etc. at all. If it does you are producing invalid html in the first place.
Comment 3 Happy-melon 2009-05-08 19:22:59 UTC
In goes:

...
$sk->link( SpecialPage::getTitleFor( 'Book', "{$action}_article/" ),
	   wfMsgHtml( "coll-{$action}_page" ),
	   array( 'onclick' => "collectionCall('{$uaction}Article', ['{$action}page', wgNamespaceNumber, wgTitle, $oldid]); return false;",
	          'rel'     => 'nofollow',
		  'title'   => wfMsgHtml( "coll-{$action}_page_tooltip" ), ),
		  $params,
		  array( 'known', 'noclasses', 'noescape' )
         );
...

Out comes:

...
<a href="/w/index.php?title=Special:Book/add_article/&amp;arttitle=Quok&amp;oldid=0" title="Add the current wiki page to your book" onclick="collectionCall(&#039;AddArticle&#039;, [&#039;addpage&#039;, wgNamespaceNumber, wgTitle, 0]); return false;" rel="nofollow">Add page to book</a>
...

(This is a rewrite of trunk/extensions/Collection/Collection.hooks.php, which currently uses loads of <<EOLs and hardcodes just about everything).  I guess the real issue is that what's being passed to onclick is not actually HTML, but JavaScript, and should be treated as such.  But that's a bit of a big ask.
Comment 4 Niklas Laxström 2009-05-08 19:43:18 UTC
That html snippet works fine for me. Unrelated, you should use wfMsg for the title attribute tooltip to avoid double escaping.
Comment 5 Happy-melon 2009-05-08 20:02:15 UTC
Hmm, seems you're right; the fact that MW coughed and died with two screens of stack backtrace was due to an entirely different piece of dubious code :D

Not to demean the work of whoever wrote this extension, it's pretty scrappy in terms of integration with MediaWiki core.  Thanks for that point on the tooltip messages, for instance.  

Looks like this isn't needed.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links