r65822 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r65821‎ | r65822 | r65823 >
Date:01:21, 3 May 2010
Author:jeroendedauw
Status:deferred (Comments)
Tags:
Comment:
Fixed security issues
Modified paths:
  • /trunk/extensions/Storyboard/specials/Story/Story_body.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Storyboard/specials/Story/Story_body.php
@@ -121,7 +121,7 @@
122122 global $wgTitle;
123123 $wgOut->addWikiMsg(
124124 'storyboard-canedit',
125 - $this->getTitle( $story->story_title )->getFullURL( array( 'action' => 'edit' ) )
 125+ htmlspecialchars( $this->getTitle( $story->story_title )->getFullURL( array( 'action' => 'edit' ) ) )
126126 );
127127 }
128128 }
@@ -146,10 +146,10 @@
147147
148148 if ( $story->story_author_image != '' && $story->story_image_hidden != 1 ) {
149149 $story->story_author_image = htmlspecialchars( $story->story_author_image );
150 - $wgOut->addHTML( "<img src='$story->story_author_image' class='story-image'>" );
 150+ $wgOut->addHTML( "<img src=\"$story->story_author_image\" class='story-image'>" );
151151 }
152152
153 - $wgOut->addWikiText( $story->story_text );
 153+ $wgOut->addWikiText( $story->story_text );
154154
155155 // If the user that submitted the story was logged in, create a link to his/her user page.
156156 if ( $story->story_author_id ) {
@@ -171,9 +171,9 @@
172172 // FIXME: this button is a temporary solution untill the SkinTemplateNavigation on special pages issue is fixed.
173173 if ( $wgUser->isAllowed( 'storyreview' ) ) {
174174 $editMsg = htmlspecialchars( wfMsg( 'edit' ) );
175 - $editUrl = $this->getTitle( $story->story_title )->getLocalURL( 'action=edit' );
 175+ $editUrl = htmlspecialchars( $this->getTitle( $story->story_title )->getLocalURL( 'action=edit' ) );
176176 $wgOut->addHtml(
177 - "<button type='button' onclick=\"window.location='$editUrl'\">$editMsg</button>"
 177+ "<button type='button' onclick='window.location=\"$editUrl\"'>$editMsg</button>"
178178 );
179179 }
180180
@@ -205,7 +205,7 @@
206206 $minLen = $wgRequest->getVal( 'minlength' );
207207 if ( !is_int( $minLen ) ) $minLen = $egStoryboardMinStoryLen;
208208
209 - $formBody = "<table width='$width'>";
 209+ $formBody = "<table width=\"$width\">";
210210
211211 // The current value will be selected on page load with jQuery.
212212 $formBody .= '<tr>' .
@@ -384,9 +384,10 @@
385385
386386 $wgOut->addHTML( $formBody );
387387
 388+ $state = htmlspecialchars( $story->story_state );
388389 $wgOut->addInlineScript( <<<EOT
389390 jQuery(document).ready(function() {
390 - jQuery("#storystate option[value='$story->story_state']").attr('selected', 'selected');
 391+ jQuery('#storystate option[value="$state"]').attr('selected', 'selected');
391392
392393 jQuery("#storyform").validate({
393394 messages: {

Follow-up revisions

RevisionCommit summaryAuthorDate
r65839Follow up to r65822jeroendedauw07:32, 3 May 2010

Comments

#Comment by Nikerabbit (talk | contribs)   06:41, 3 May 2010
+ htmlspecialchars( $this->getTitle( $story->story_title )->getFullURL( array( 'action' => 'edit' ) ) )

That looks double escaping to me: first by htmlspecialchars and then the wiki parser.

#Comment by Jeroen De Dauw (talk | contribs)   06:48, 3 May 2010

Can you confirm it gets escaped by the parser? I'll remove it then.

#Comment by Nikerabbit (talk | contribs)   06:53, 3 May 2010

If it didn't, we would have a bigger security hole. Of course what the parser does is not equivalent to htmlspecialchars, but should suffice.

#Comment by Jeroen De Dauw (talk | contribs)   07:33, 3 May 2010

Fixed in follow-up

Status & tagging log