Index: includes/common.inc =================================================================== RCS file: /cvs/drupal/drupal/includes/common.inc,v retrieving revision 1.1154 diff -u -p -r1.1154 common.inc --- includes/common.inc 29 Apr 2010 05:33:43 -0000 1.1154 +++ includes/common.inc 29 Apr 2010 20:12:29 -0000 @@ -1199,10 +1199,21 @@ function flood_is_allowed($name, $thresh */ /** - * Prepare a URL for use in an HTML attribute. Strips harmful protocols. - */ -function check_url($uri) { - return filter_xss_bad_protocol($uri, FALSE); + * Strip harmful protocols (e.g. 'javascript:') from a URL and optionally make it safe for output to HTML. + * + * @param $uri + * A plain-text URI that might contain harmful protocols. + * @param $check_plain + * (optional) Whether to also call check_plain() so that the result can be + * safely output to HTML. Defaults to TRUE, but can be set to FALSE when the + * result of this function will not be output to HTML (e.g. to a plain-text + * e-mail instead), or when it will be passed to another function that expects + * a plain-text string instead of an HTML-encoded string (e.g. l(), t(), or + * drupal_attributes()). + */ +function check_url($uri, $check_plain = TRUE) { + $uri = filter_xss_bad_protocol($uri, FALSE); + return $check_plain ? check_plain($uri) : $uri; } /** @@ -1440,25 +1451,36 @@ function _filter_xss_attributes($attr) { } /** - * Processes an HTML attribute value and ensures it does not contain an URL with a disallowed protocol (e.g. javascript:). + * Strips harmful URL protocols from a text or HTML string (e.g. javascript:). + * + * This function is usually called as part of some other sanitization process + * (e.g., check_url(), filter_xss(), or check_markup()) rather than on its own. + * Most code should call one of those functions instead. One exception is that + * url() and url_is_external() call this function directly, not as part of + * sanitization, but in order to determine if a URL is external, and not wanting + * to treat unknown protocols as external. * * @param $string - * The string with the attribute value. - * @param $decode - * Whether to decode entities in the $string. Set to FALSE if the $string - * is in plain text, TRUE otherwise. Defaults to TRUE. + * The string containing a URL. + * @param $html_encoded + * (optional) Whether $string is already encoded for HTML (for example, if it + * was parsed from an HTML document). If TRUE, the returned string will also + * be escaped for HTML. If FALSE, the returned string will not be + * HTML-escaped, so check_plain() will need to be called prior to outputting + * to a page. Defaults to TRUE. + * * @return - * Cleaned up and HTML-escaped version of $string. + * Cleaned up version of $string. */ -function filter_xss_bad_protocol($string, $decode = TRUE) { +function filter_xss_bad_protocol($string, $html_encoded = TRUE) { static $allowed_protocols; if (!isset($allowed_protocols)) { $allowed_protocols = array_flip(variable_get('filter_allowed_protocols', array('ftp', 'http', 'https', 'irc', 'mailto', 'news', 'nntp', 'rtsp', 'sftp', 'ssh', 'telnet', 'webcal'))); } - // Get the plain text representation of the attribute value (i.e. its meaning). - if ($decode) { + // If the string is HTML-encoded, decode it. + if ($html_encoded) { $string = decode_entities($string); } @@ -1483,7 +1505,12 @@ function filter_xss_bad_protocol($string } } while ($before != $string); - return check_plain($string); + // If the string was HTML-encoded and we decoded it, re-encode it. + if ($html_encoded) { + $string = check_plain($string); + } + + return $string; } /** @@ -1979,7 +2006,7 @@ function url($path = NULL, array $option // Note: we could use url_is_external($path) here, but that would // require another function call, and performance inside url() is critical. $colonpos = strpos($path, ':'); - $options['external'] = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == check_plain($path)); + $options['external'] = ($colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == $path); } // Preserve the original path before altering or aliasing. @@ -2099,7 +2126,7 @@ function url_is_external($path) { $colonpos = strpos($path, ':'); // Only call the slow filter_xss_bad_protocol if $path contains a ':' // before any / ? or #. - return $colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == check_plain($path); + return $colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && filter_xss_bad_protocol($path, FALSE) == $path; } /** @@ -2154,6 +2181,12 @@ function drupal_attributes(array $attrib * internal links output by modules should be generated by this function if * possible. * + * This function is not responsible for stripping bad protocols from URLs + * that come from user input. Use the following to ensure such URLs are safe: + * @code + * check_url($path, FALSE); + * @endcode + * * @param $text * The link text for the anchor tag. * @param $path Index: includes/form.inc =================================================================== RCS file: /cvs/drupal/drupal/includes/form.inc,v retrieving revision 1.457 diff -u -p -r1.457 form.inc --- includes/form.inc 29 Apr 2010 03:34:00 -0000 1.457 +++ includes/form.inc 29 Apr 2010 19:40:32 -0000 @@ -2870,7 +2870,7 @@ function theme_textfield($variables) { if ($element['#autocomplete_path'] && drupal_valid_path($element['#autocomplete_path'])) { drupal_add_js('misc/autocomplete.js'); $class[] = 'form-autocomplete'; - $extra = ''; + $extra = ''; } _form_set_class($element, $class); @@ -2892,7 +2892,7 @@ function theme_textfield($variables) { function theme_form($variables) { $element = $variables['element']; // Anonymous div to satisfy XHTML compliance. - $action = $element['#action'] ? 'action="' . check_url($element['#action']) . '" ' : ''; + $action = $element['#action'] ? 'action="' . check_plain($element['#action']) . '" ' : ''; return '
\n
" . $element['#children'] . "\n
\n"; } Index: includes/theme.inc =================================================================== RCS file: /cvs/drupal/drupal/includes/theme.inc,v retrieving revision 1.594 diff -u -p -r1.594 theme.inc --- includes/theme.inc 28 Apr 2010 20:00:33 -0000 1.594 +++ includes/theme.inc 29 Apr 2010 19:40:32 -0000 @@ -1840,7 +1840,7 @@ function theme_item_list($variables) { * - url: The url for the link. */ function theme_more_help_link($variables) { - return '
' . t('More help', array('@link' => check_url($variables['url']))) . '
'; + return '
' . l(t('More help'), $variables['url']) . '
'; } /** @@ -1854,7 +1854,7 @@ function theme_more_help_link($variables function theme_feed_icon($variables) { $text = t('Subscribe to @feed-title', array('@feed-title' => $variables['title'])); if ($image = theme('image', array('path' => 'misc/feed.png', 'alt' => $text))) { - return '' . $image . ''; + return l($image, $variables['url'], array('html' => TRUE, 'attributes' => array('class' => array('feed-icon'), 'title' => $text))); } } @@ -1904,7 +1904,7 @@ function theme_html_tag($variables) { * - title: A descriptive verb for the link, like 'Read more'. */ function theme_more_link($variables) { - return '
' . t('More', array('@link' => check_url($variables['url']), '@title' => $variables['title'])) . '
'; + return '
' . l(t('More'), $variables['url'], array('attributes' => array('title' => $variables['title']))) . '
'; } /** @@ -2158,7 +2158,7 @@ function template_preprocess_html(&$vari if (theme_get_setting('toggle_favicon')) { $favicon = theme_get_setting('favicon'); $type = theme_get_setting('favicon_mimetype'); - drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => check_url($favicon), 'type' => $type)); + drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => check_url($favicon, FALSE), 'type' => $type)); } // Construct page title. @@ -2342,7 +2342,7 @@ function template_preprocess_maintenance if (theme_get_setting('toggle_favicon')) { $favicon = theme_get_setting('favicon'); $type = theme_get_setting('favicon_mimetype'); - drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => check_url($favicon), 'type' => $type)); + drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => check_url($favicon, FALSE), 'type' => $type)); } global $theme; Index: modules/comment/comment.tokens.inc =================================================================== RCS file: /cvs/drupal/drupal/modules/comment/comment.tokens.inc,v retrieving revision 1.11 diff -u -p -r1.11 comment.tokens.inc --- modules/comment/comment.tokens.inc 20 Apr 2010 09:48:06 -0000 1.11 +++ modules/comment/comment.tokens.inc 29 Apr 2010 19:40:32 -0000 @@ -172,7 +172,7 @@ function comment_tokens($type, $tokens, break; case 'homepage': - $replacements[$original] = $sanitize ? filter_xss_bad_protocol($comment->homepage) : $comment->homepage; + $replacements[$original] = $sanitize ? check_url($comment->homepage) : $comment->homepage; break; case 'title': Index: modules/search/search.pages.inc =================================================================== RCS file: /cvs/drupal/drupal/modules/search/search.pages.inc,v retrieving revision 1.18 diff -u -p -r1.18 search.pages.inc --- modules/search/search.pages.inc 13 Apr 2010 15:23:03 -0000 1.18 +++ modules/search/search.pages.inc 29 Apr 2010 19:40:32 -0000 @@ -90,7 +90,7 @@ function template_preprocess_search_resu */ function template_preprocess_search_result(&$variables) { $result = $variables['result']; - $variables['url'] = check_url($result['link']); + $variables['url'] = check_plain($result['link']); $variables['title'] = check_plain($result['title']); $info = array(); Index: modules/simpletest/tests/common.test =================================================================== RCS file: /cvs/drupal/drupal/modules/simpletest/tests/common.test,v retrieving revision 1.110 diff -u -p -r1.110 common.test --- modules/simpletest/tests/common.test 22 Apr 2010 21:41:09 -0000 1.110 +++ modules/simpletest/tests/common.test 29 Apr 2010 19:40:32 -0000 @@ -82,7 +82,10 @@ class CommonURLUnitTest extends DrupalWe $text = $this->randomName(); $path = ""; $link = l($text, $path); - $sanitized_path = check_url(url($path)); + // Not check_url(), because l() is not responsible for stripping bad + // protocols. Code that calls l() with a path that comes from user input + // needs to do that. + $sanitized_path = check_plain(url($path)); $this->assertTrue(strpos($link, $sanitized_path) !== FALSE, t('XSS attack @path was filtered', array('@path' => $path))); } @@ -314,7 +317,7 @@ class CommonXssUnitTest extends DrupalUn public static function getInfo() { return array( 'name' => 'String filtering tests', - 'description' => 'Confirm that check_plain() and filter_xss() work correctly, including invalid multi-byte sequences.', + 'description' => 'Confirm that check_plain(), filter_xss(), and check_url() work correctly, including invalid multi-byte sequences.', 'group' => 'System', ); } @@ -341,6 +344,20 @@ class CommonXssUnitTest extends DrupalUn $text = check_plain("