(In reply to comment #0)

Created an attachment (id=6068) [details]
Patch to Pager.php, against r49962

I'm currently plugging away at formatting [[Special:Allmessages]] using the
TablePager class, from Pager.php. In order to avoid some horrible hacks
(returning a value of """foo" rowspan="2""" to fill into something like """<td
class="$class">""")

I'm surprised that that even works: it's a sanitization bug.

happy.melon.wiki wrote:

You mean the hack? TBH I didn't actually try it, I knew what I needed and what was available in TablePager, and gave up in disgust. Whoever wrote that comment in CodeReview was right, the function wasn't particularly accessible. The positioning of the $this->mCurrentRow=$row; line is just stupid.

Or do you mean the patch? :-S

(In reply to comment #2)

You mean the hack? TBH I didn't actually try it, I knew what I needed and what
was available in TablePager, and gave up in disgust. Whoever wrote that comment
in CodeReview was right, the function wasn't particularly accessible. The
positioning of the $this->mCurrentRow=$row; line is just stupid.

Or do you mean the patch? :-S

No, I meant the HTML injection hack. I hadn't even looked at the patch yet. Now that I have:

$td = Xml::openElement( 'td', $this->getCellAttrs($field,$value) );
$s .= "$td $formatted </td>";

You should simply use:

$s .= Xml::element( 'td', $this->getCellAttrs( $field, $value ), $formatted );

(or Xml::tags() if $formatted contains HTML and should not be escaped; I couldn't tell from the patch).

return array( 'class' => htmlspecialchars( $this->getRowClass( $row ) ) );

You don't need to (and shouldn't) use htmlspecialchars() here: the Xml:: functions will handle proper escaping. This way, you'll probably end up double-escaping stuff. The same applies to getCellAttrs().

happy.melon.wiki wrote:

I tried the first method and got PHP's version of a compile error (unexpected character on the last line); I'm not sure either why it didn't work ($formatted could in principle contain raw HTML, but would that really *kill* the program?); but doing it that way definitely did fix it.

happy.melon.wiki wrote:

Updated with Roan's htmlspecialchars() fixes

attachment bug18620-2.txt ignored as obsolete

(In reply to comment #4)

I tried the first method and got PHP's version of a compile error (unexpected
character on the last line); I'm not sure either why it didn't work

My fragment looks correct, I'm not sure your error is related. Please try again, as your current patch still doesn't use Xml::element() or Xml::tags() on that line.

($formatted
could in principle contain raw HTML, but would that really *kill* the
program?); but doing it that way definitely did fix it.

No, but it would show the HTML as text in the browser rather than actually treating it as HTML, so you'd have a bug in case $formatted is *supposed* to contain HTML.

happy.melon.wiki wrote:

updated, use Xml::tags(); still against r49962

I guess you're right; tried and it worked fine. It definitely needs to be tags(); using elements escapes things like links. htmlspecialchars is the thing to use to disable tags, right? (Not relevant for this, but will be in my rewrite of SpecialAllmessages).

Attached:

(In reply to comment #7)

Created an attachment (id=6070) [details]
updated, use Xml::tags(); still against r49962

I guess you're right; tried and it worked fine. It definitely needs to be
tags(); using elements escapes things like links.

Patch looks OK, committed in r50046

htmlspecialchars is the thing
to use to disable tags, right? (Not relevant for this, but will be in my
rewrite of SpecialAllmessages).

Yes, but you probably want to use Xml:: functions there as well, as they sanitize stuff for you.