r83360 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r83359‎ | r83360 | r83361 >
Date:02:43, 6 March 2011
Author:bawolff
Status:resolved (Comments)
Tags:
Comment:
(bug 27854) Http::isValidURI is way to lax. This is a much simplified regex that accepts a subset of the previous
regex, but also accepts ftps because both cURL and php support it. It no longer accepts thing like 'foo http://bar bax'
which was my main concern

Note the previous regex kind of looks more restrictive, but is not since saying "anything not containing a space
optionally followed by anything not containing a bunch of characters including a space" is the same as saying anything
with no spaces. See also r83296. This obviously doesn't catch all cases, but I personally think its sufficient.
At the very least it is a very significant improvement over the previous version that caught almost nothing.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/HttpFunctions.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/HttpFunctions.php
——@@ -117,16 +117,17 @@
118118 }
119119
120120 /**
121 - * Checks that the given URI is a valid one
 121+ * Checks that the given URI is a valid one. Hardcoding the
 122+ * protocols, because we only want protocols that both cURL
 123+ * and php support.
122124 *
123125 * @param $uri Mixed: URI to check for validity
124126 * @returns Boolean
125127 */
126128 public static function isValidURI( $uri ) {
127129 return preg_match(
128 - '/(ftp|http|https):\/\/(\w+:{0,1}\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?/',
129 - $uri,
130 - $matches
 130+ '/^(f|ht)tps?:\/\/[^\/\s]\S*$/D',
 131+ $uri
131132 );
132133 }
133134 }
Index: trunk/phase3/RELEASE-NOTES
——@@ -161,6 +161,7 @@
162162 incorrect revision ID is passed.
163163 * Trim the form field for uploading by url to remove extra spaces which could
164164 cause confusing error messages.
 165+* (bug 27854) Http::isValidURI is way too lax.
165166
166167 === API changes in 1.18 ===
167168 * (bug 26339) Throw warning when truncating an overlarge API result

Follow-up revisions

RevisionCommit summaryAuthorDate
r89812MFT r89278, r89452. Also had to grab r83360.demon02:56, 10 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r83296Tests for bug 27854 - Http::isValidURI is to lax...hashar16:15, 5 March 2011

Comments

#Comment by MarkAHershberger (talk | contribs)   21:56, 10 March 2011

Is there an ftps protocol? There is an sftp protocol, but I don't think Curl or PHP support it.

#Comment by MarkAHershberger (talk | contribs)   21:56, 10 March 2011

Is there an ftps protocol? There is an sftp protocol, but I don't think Curl or PHP support it.

#Comment by MarkAHershberger (talk | contribs)   22:03, 10 March 2011

Yes, supported in Curl (http://en.wikipedia.org/wiki/CURL) and php (http://php.net/manual/en/wrappers.ftp.php)

#Comment by Bawolff (talk | contribs)   16:49, 16 March 2011

I just discovered that we don't actually use that feature of php, and actually the php version of this only supports http (no https, ftp, or ftps). We just open a tcp connection, send an http request, and look at the result, so my commit summary is wrong...

#Comment by Bawolff (talk | contribs)   18:19, 16 March 2011

I lied we only do that for proxies. But we still only support plain http for PhpHttpRequest.

Status & tagging log