tbleher@svn.wikimedia.org wrote:

Revision: 29651 Author: tbleher Date: 2008-01-12 14:33:50 +0000 (Sat, 12 Jan 2008)

Log Message:

Security fix: escape all URLs, as they are written directly into the HTML of the page.

Modified Paths:

trunk/extensions/ProofreadPage/ProofreadPage.php

Modified: trunk/extensions/ProofreadPage/ProofreadPage.php

--- trunk/extensions/ProofreadPage/ProofreadPage.php 2008-01-12 13:44:08 UTC (rev 29650) +++ trunk/extensions/ProofreadPage/ProofreadPage.php 2008-01-12 14:33:50 UTC (rev 29651) @@ -69,16 +69,16 @@ $prev_name = "$page_namespace:$name/" . ( $pagenr - 1 ); $next_name = "$page_namespace:$name/" . ( $pagenr + 1 ); $index_name = "$index_namespace:$name";

• 	$prev_url = ( $pagenr == 1 ) ? '' : Title::newFromText( $prev_name )->getFullURL();
  

• 	$next_url = ( $pagenr == $count ) ? '' : Title::newFromText( $next_name )->getFullURL();
  

• 	$index_url = Title::newFromText( $index_name )->getFullURL();
  

• 	$prev_url = ( $pagenr == 1 ) ? '' : Title::newFromText( $prev_name )->escapeFullURL();
  

• 	$next_url = ( $pagenr == $count ) ? '' : Title::newFromText( $next_name )->escapeFullURL();
  

• 	$index_url = Title::newFromText( $index_name )->escapeFullURL();
  return array( $index_url, $prev_url, $next_url );
  

  } return $err; }

  $index_title = $ref_title;

• $index_url = $index_title->getFullURL();

• $index_url = $index_title->escapeFullURL(); $rev = Revision::newFromTitle( $index_title ); $text = $rev->getText();

@@ -93,13 +93,13 @@ if( ($i>0) && ($i<count($links[1])) ){ $prev_title = Title::newFromText( $links[1][$i-1] ); if(!$prev_title) return $err;

• $prev_url = $prev_title->getFullURL();
  

• $prev_url = $prev_title->escapeFullURL();
  

  } else $prev_url = ''; if( ($i>=0) && ($i+1<count($links[1])) ){ $next_title = Title::newFromText( $links[1][$i+1] ); if(!$next_title) return $err;

• $next_url = $next_title->getFullURL();
  

• $next_url = $next_title->escapeFullURL();
  

  } else $next_url = '';

This is incorrect. The URLs in question are destined for JavaScript, not plain HTML, so you need to use Xml::escapeJsString() or Xml::encodeJsVar(), not htmlspecialchars(). Additionally, escaping should always be done as close to the output as possible, to make security review easier and mistakes less likely. wfPRNavigation() should return the unescaped URLs, and its caller should escape them according to what kind of output it is doing.

-- Tim Starling