r89278 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r89277‎ | r89278 | r89279 >
Date:18:57, 1 June 2011
Author:mah
Status:resolved (Comments)
Tags:
Comment:
Fix Bug #29231: PhpHttpRequest doesn't support HTTPS

Tested fix with PHPUnit Tests that aren't currently executed by
CruiseControl b/c they're marked “broken” till someone comes up with
some reasonable unit tests that will work for everyone.
Modified paths:
  • /trunk/phase3/includes/HttpFunctions.php (modified) (history)
  • /trunk/phase3/tests/phpunit/includes/HttpTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/HttpTest.php
——@@ -17,7 +17,9 @@
1818 static $has_proxy = false;
1919 static $proxy = "http://hulk:8080/";
2020 var $test_geturl = array(
 21+ "ftp://ftp.kernel.org/",
2122 "http://www.example.com/",
 23+ "https://secure.wikimedia.org/",
2224 "http://pecl.php.net/feeds/pkg_apc.rss",
2325 "http://toolserver.org/~jan/poll/dev/main.php?page=wiki_output&id=3",
2426 "http://meta.wikimedia.org/w/index.php?title=Interwiki_map&action=raw",
——@@ -540,7 +542,7 @@
541543 return array(
542544 array( false, '¿non sens before!! http://a', 'Allow anything before URI' ),
543545
544 - # (ftp|http|https) - only three schemes allowed
 546+ # (ftp|http|https) - only three schemes allowed
545547 array( true, 'http://www.example.org/' ),
546548 array( true, 'https://www.example.org/' ),
547549 array( true, 'ftp://www.example.org/' ),
Index: trunk/phase3/includes/HttpFunctions.php
——@@ -713,7 +713,9 @@
714714 $this->postData = wfArrayToCGI( $this->postData );
715715 }
716716
717 - if ( $this->parsedUrl['scheme'] != 'http' ) {
 717+ if ( $this->parsedUrl['scheme'] != 'http' &&
 718+ $this->parsedUrl['scheme'] != 'ftp' &&
 719+ $this->parsedUrl['scheme'] != 'https' ) {
718720 $this->status->fatal( 'http-invalid-scheme', $this->parsedUrl['scheme'] );
719721 }
720722

Follow-up revisions

RevisionCommit summaryAuthorDate
r89292Tweak HttpTest following r89278:...brion21:25, 1 June 2011
r89452follow up r89278 — remove FTP support.mah02:59, 4 June 2011
r89812MFT r89278, r89452. Also had to grab r83360.demon02:56, 10 June 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   19:41, 3 June 2011

Hmm should we actually deliberately support ftp in the Http request class? We may get unexpected return codes etc at best; most things don't seem to expect it.

#Comment by MarkAHershberger (talk | contribs)   20:09, 3 June 2011

I'm willing to change the Request class name to URI (or what have you), but I think supporting “generic” requests is better. Better still would be a way for extensions to begin to plug in handlers for different sort of URIs.

But I realize my liberal “support any URI” policy may not be what MW wants to do.

#Comment by Brion VIBBER (talk | contribs)   20:41, 3 June 2011

Adding protocols increases the security attack surface and doesn't have an obvious practical benefit.

Already, lots of stuff calling Http or MwHttpRequest assumes, well, HTTP or HTTPS and they assume semantics based on that like:

  • the notion of redirects
  • meaning and validity of return codes
#Comment by 😂 (talk | contribs)   00:01, 4 June 2011

+1. Unless we've got a compelling use case to add FTP or other protocols, lets just stick to Http(s)

Status & tagging log