- Patch #124492 by mfer, c960657 et al: valid_url() does not support all valid URL characters.

3 files changed, 121 insertions, 5 deletions

diff --git a/includes/common.inc b/includes/common.inc
index 68b26b1e2..4b59171ef 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -1173,7 +1173,7 @@ function valid_email_address($mail) {
  *
  * This function should only be used on actual URLs. It should not be used for
  * Drupal menu paths, which can contain arbitrary characters.
- *
+ * Valid values per RFC 3986.
  * @param $url
  *   The URL to verify.
  * @param $absolute
@@ -1182,12 +1182,21 @@ function valid_email_address($mail) {
  *   TRUE if the URL is in a valid format.
  */
 function valid_url($url, $absolute = FALSE) {
-  $allowed_characters = '[a-z0-9\/:_\-_\.\?\$,;~=#&%\+]';
   if ($absolute) {
-    return (bool)preg_match("/^(http|https|ftp):\/\/" . $allowed_characters . "+$/i", $url);
+    return (bool)preg_match("
+      /^                                                # Start at the beginning of the text
+      (?:ftp|https?):\/\/                               # Look for ftp, http, or https
+      (?:                                               # Userinfo (optional)
+        (?:[\w\.\-\+%!$&'\(\)*\+,;=]+:)*
+        [\w\.\-\+%!$&'\(\)*\+,;=]+@
+      )?
+      (?:[a-z0-9\-\.%]+)                                # The domain
+      (?::[0-9]+)?                                      # Server port number (optional)
+      (?:[\/|\?][\w#!:\.\?\+=&%@!$'~*,;\/\(\)\[\]\-]*)? # The path (optional)
+    $/xi", $url);
   }
   else {
-    return (bool)preg_match("/^" . $allowed_characters . "+$/i", $url);
+    return (bool)preg_match("/^[\w#!:\.\?\+=&%@!$'~*,;\/\(\)\[\]\-]+$/i", $url);
   }
 }
 
diff --git a/modules/profile/profile.test b/modules/profile/profile.test
index 8b17c07a4..58193d40e 100644
--- a/modules/profile/profile.test
+++ b/modules/profile/profile.test
@@ -134,7 +134,9 @@ class ProfileTestFields extends ProfileTestCase {
       'textarea' => $this->randomName(),
       'list' => $this->randomName(),
       'checkbox' => 1,
-      'url' => 'http://www.' . $this->randomName(10). '.org',
+      // An underscore is an invalid character in a domain name. The method randomName can
+      // return an underscore.
+      'url' => 'http://www.' . str_replace('_', '', $this->randomName(10)). '.org',
     );
 
     // For each field type, create a field, give it a value and delete the field.
diff --git a/modules/simpletest/tests/common.test b/modules/simpletest/tests/common.test
index ca4a0459a..c66007709 100644
--- a/modules/simpletest/tests/common.test
+++ b/modules/simpletest/tests/common.test
@@ -523,6 +523,111 @@ class DrupalErrorHandlerUnitTest extends DrupalWebTestCase {
 }
 
 /**
+ * Test for valid_url().
+ */
+class ValidUrlTestCase extends DrupalWebTestCase {
+  function getInfo() {
+    return array(
+      'name' => t('Valid Url'),
+      'description' => t("Performs tests on Drupal's valid url function."),
+      'group' => t('System')
+    );
+  }
+
+  /**
+   * Test valid absolute urls.
+   */
+  function testValidAbsolute() {
+    $url_schemes = array('http', 'https', 'ftp');
+    $valid_absolute_urls = array(
+      'example.com',
+      'www.example.com',
+      'ex-ample.com',
+      '3xampl3.com',
+      'example.com/paren(the)sis',
+      'example.com/index.html#pagetop',
+      'example.com:8080',
+      'subdomain.example.com',
+      'example.com/index.php?q=node',
+      'example.com/index.php?q=node&param=false',
+      'user@www.example.com',
+      'user:pass@www.example.com:8080/login.php?do=login&style=%23#pagetop',
+      '127.0.0.1',
+      'example.org?',
+      'john%20doe:secret:foo@example.org/',
+      'example.org/~,$\'*;',
+      'caf%C3%A9.example.org',
+    );
+
+    foreach ($url_schemes as $scheme) {
+      foreach ($valid_absolute_urls as $url) {
+        $test_url = $scheme . '://' . $url;
+        $valid_url = valid_url($test_url, TRUE);
+        $this->assertTrue($valid_url, t('@url is a valid url.', array('@url' => $test_url)));
+      }
+    }
+  }
+
+  /**
+   * Test invalid absolute urls.
+   */
+  function testInvalidAbsolute() {
+    $url_schemes = array('http', 'https', 'ftp');
+    $invalid_ablosule_urls = array(
+      '',
+      'ex!ample.com',
+    );
+
+    foreach ($url_schemes as $scheme) {
+      foreach ($invalid_ablosule_urls as $url) {
+        $test_url = $scheme . '://' . $url;
+        $valid_url = valid_url($test_url, TRUE);
+        $this->assertFalse($valid_url, t('@url is NOT a valid url.', array('@url' => $test_url)));
+      }
+    }
+  }
+
+  /**
+   * Test valid relative urls.
+   */
+  function testValidRelative() {
+    $valid_relative_urls = array(
+      'paren(the)sis',
+      'index.html#pagetop',
+      'index.php?q=node',
+      'index.php?q=node&param=false',
+      'login.php?do=login&style=%23#pagetop',
+    );
+
+    foreach (array('', '/') as $front) {
+      foreach ($valid_relative_urls as $url) {
+        $test_url = $front . $url;
+        $valid_url = valid_url($test_url);
+        $this->assertTrue($valid_url,t('@url is a valid url.', array('@url' => $test_url)));
+      }
+    }
+  }
+
+  /**
+   * Test invalid relative urls.
+   */
+  function testInvalidRelative() {
+    $invalid_relative_urls = array(
+      'ex^mple',
+      'example<>'
+    );
+
+    foreach (array('', '/') as $front) {
+      foreach ($invalid_relative_urls as $url) {
+        $test_url = $front . $url;
+        $valid_url = valid_url($test_url);
+        $this->assertFALSE($valid_url,t('@url is NOT a valid url.', array('@url' => $test_url)));
+      }
+    }
+  }
+}
+
+/**
  * Tests Simpletest error and exception collecter.
  */
 class DrupalErrorCollectionUnitTest extends DrupalWebTestCase {