r53195 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r53194‎ | r53195 | r53196 >
Date:21:39, 13 July 2009
Author:btongminh
Status:reverted (Comments)
Tags:
Comment:
Add the 403 fix from r49833 to RawPage as well.
Modified paths:
  • /trunk/phase3/includes/RawPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/RawPage.php
——@@ -111,7 +111,7 @@
112112 function view() {
113113 global $wgOut, $wgScript;
114114
115 - if( isset( $_SERVER['SCRIPT_URL'] ) ) {
 115+ if( isset( $_SERVER['SCRIPT_NAME'] ) ) {
116116 # Normally we use PHP_SELF to get the URL to the script
117117 # as it was called, minus the query string.
118118 #
——@@ -122,16 +122,16 @@
123123 #
124124 # If in this mode, use SCRIPT_URL instead, which mod_rewrite
125125 # provides containing the "before" URL.
126 - $url = $_SERVER['SCRIPT_URL'];
 126+ $url = $_SERVER['SCRIPT_NAME'];
127127 } else {
128 - $url = $_SERVER['PHP_SELF'];
 128+ $url = $_SERVER['URL'];
129129 }
130130
131131 if( $url == '' ) {
132132 # This will make the next check fail with a confusing error
133133 # message, so we should mention it separately.
134134 wfHttpError( 500, 'Internal Server Error',
135 - "\$_SERVER['PHP_SELF'] is not set. Perhaps you're using CGI" .
 135+ "\$_SERVER['URL'] is not set. Perhaps you're using CGI" .
136136 " and haven't set cgi.fix_pathinfo = 1 in php.ini?" );
137137 return;
138138 }

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r49833API: (bug 13049) This'll hopefully fix the 403 Forbidden error in api.php for...catrope19:50, 24 April 2009

Comments

#Comment by Simetrical (talk | contribs)   00:17, 14 July 2009

Are you certain this doesn't break some setups? Different installations tend to set these variables differently in an unpredictable fashion, in my experience. There are tons of possible setups out there. It seems a lot safer to fall back to these extra ones only if the currently-used ones aren't set; the current ones have been working okay for a long time for most people, and we shouldn't break those.

#Comment by Tim Starling (talk | contribs)   14:17, 16 August 2009

See my comments on r49833. It appears to break the security check entirely, exposing an XSS vulnerability in IE<=3 (*gasp*) and rendering most MW installations with cgi.fix_pathinfo=0 unusable. The $url=$_SERVER['URL'] branch is unreachable which is lucky because otherwise it would issue a notice in any webserver other than IIS.

Of course IE 3 probably has a hundred documented buffer overflows, but that's not the point, is it?

Status & tagging log