From ad2ba3ed0dca43afa3fb6128e998a8399b562bab Mon Sep 17 00:00:00 2001 From: Aaron Jorbin Date: Tue, 7 Feb 2023 18:52:24 +0000 Subject: [PATCH] Comments: Improve rel attribute usage in comments. Internal links should be followed and it should be easier to modify other rel attributes on comments. This adds a helper function for determining if a URL is internal and also adds some new filters to make it easy to modify rel attributes in comments. Props thomasplevy, desrosj, sabernhardt, benish74, samiamnot, galbaras, jorbin. Fixes #53290, #56444. git-svn-id: https://develop.svn.wordpress.org/trunk@55289 602fd350-edb4-49c9-b593-d223f7449a82 --- package.json | 2 +- src/wp-includes/comment-template.php | 41 +++++++-- src/wp-includes/formatting.php | 87 +++++++++++-------- src/wp-includes/link-template.php | 60 +++++++++++++ .../tests/formatting/makeClickable.php | 47 +++++++--- .../tests/formatting/wpRelNofollow.php | 40 --------- tests/phpunit/tests/formatting/wpRelUgc.php | 43 +-------- 7 files changed, 184 insertions(+), 136 deletions(-) diff --git a/package.json b/package.json index df81c6d13e..35c0e049b4 100644 --- a/package.json +++ b/package.json @@ -176,7 +176,7 @@ "env:cli": "node ./tools/local-env/scripts/docker.js run cli", "env:logs": "node ./tools/local-env/scripts/docker.js logs", "env:pull": "node ./tools/local-env/scripts/docker.js pull", - "test:php": "node ./tools/local-env/scripts/docker.js run -T php composer update -W && node ./tools/local-env/scripts/docker.js run php ./vendor/bin/phpunit", + "test:php": "node ./tools/local-env/scripts/docker.js run -T php composer update -W && node ./tools/local-env/scripts/docker.js run php ./vendor/bin/phpunit --group formatting", "test:e2e": "node ./tests/e2e/run-tests.js", "test:visual": "node ./tests/visual-regression/run-tests.js", "sync-gutenberg-packages": "grunt sync-gutenberg-packages", diff --git a/src/wp-includes/comment-template.php b/src/wp-includes/comment-template.php index ba6851e554..b5cbdc5cbe 100644 --- a/src/wp-includes/comment-template.php +++ b/src/wp-includes/comment-template.php @@ -218,14 +218,45 @@ function get_comment_author_email_link( $linktext = '', $before = '', $after = ' * @return string The comment author name or HTML link for author's URL. */ function get_comment_author_link( $comment_ID = 0 ) { - $comment = get_comment( $comment_ID ); - $url = get_comment_author_url( $comment ); - $author = get_comment_author( $comment ); + $comment = get_comment( $comment_ID ); + $comment_ID = ! empty( $comment->comment_ID ) ? $comment->comment_ID : (string) $comment_ID; + $url = get_comment_author_url( $comment ); + $author = get_comment_author( $comment ); if ( empty( $url ) || 'http://' === $url ) { $return = $author; } else { - $return = "$author"; + $rel_parts = array( 'ugc' ); + if ( ! wp_is_internal_link( $url ) ) { + $rel_parts = array_merge( + $rel_parts, + array( 'external', 'nofollow' ) + ); + } + + /** + * Filters the rel attributes of the comment author's link. + * + * @since 6.2.0 + * + * @param string[] $rel_parts An array of strings representing the rel + * tags which will be joined into the anchor's + * rel attribute. + * @param WP_Comment $comment The comment object + */ + $rel_parts = apply_filters( 'comment_author_link_rel', $rel_parts, $comment ); + + $rel = implode( ' ', $rel_parts ); + $rel = esc_attr( $rel ); + // empty space before rel necessary for later sprintf. + $rel = ! empty( $rel ) ? sprintf( ' rel="%s"', $rel ) : ''; + + $return = sprintf( + '%3$s', + $url, + $rel, + $author + ); } /** @@ -239,7 +270,7 @@ function get_comment_author_link( $comment_ID = 0 ) { * @param string $author The comment author's username. * @param string $comment_ID The comment ID as a numeric string. */ - return apply_filters( 'get_comment_author_link', $return, $author, $comment->comment_ID ); + return apply_filters( 'get_comment_author_link', $return, $author, $comment_ID ); } /** diff --git a/src/wp-includes/formatting.php b/src/wp-includes/formatting.php index d598d354d3..bb822b24b5 100644 --- a/src/wp-includes/formatting.php +++ b/src/wp-includes/formatting.php @@ -2917,24 +2917,9 @@ function _make_url_clickable_cb( $matches ) { return $matches[0]; } - if ( 'comment_text' === current_filter() ) { - $rel = 'nofollow ugc'; - } else { - $rel = 'nofollow'; - } + $rel_attr = _make_clickable_rel_attr( $url ); + return $matches[1] . "$url" . $suffix; - /** - * Filters the rel value that is added to URL matches converted to links. - * - * @since 5.3.0 - * - * @param string $rel The rel value. - * @param string $url The matched URL being converted to a link tag. - */ - $rel = apply_filters( 'make_clickable_rel', $rel, $url ); - $rel = esc_attr( $rel ); - - return $matches[1] . "$url" . $suffix; } /** @@ -2965,17 +2950,8 @@ function _make_web_ftp_clickable_cb( $matches ) { return $matches[0]; } - if ( 'comment_text' === current_filter() ) { - $rel = 'nofollow ugc'; - } else { - $rel = 'nofollow'; - } - - /** This filter is documented in wp-includes/formatting.php */ - $rel = apply_filters( 'make_clickable_rel', $rel, $dest ); - $rel = esc_attr( $rel ); - - return $matches[1] . "$dest$ret"; + $rel_attr = _make_clickable_rel_attr( $dest ); + return $matches[1] . "{$dest}{$ret}"; } /** @@ -2994,6 +2970,48 @@ function _make_email_clickable_cb( $matches ) { return $matches[1] . "$email"; } +/** + * Helper function used to build the "rel" attribute for a URL when creating an anchor using make_clickable(). + * + * @since 6.2.0 + * + * @param string $url The URL. + * @return string The rel attribute for the anchor or an empty string if no rel attribute should be added. + */ +function _make_clickable_rel_attr( $url ) { + + $rel_parts = array(); + $scheme = strtolower( wp_parse_url( $url, PHP_URL_SCHEME ) ); + $nofollow_schemes = array_intersect( wp_allowed_protocols(), array( 'https', 'http' ) ); + + // Apply "nofollow" to external links with qualifying URL schemes (mailto:, tel:, etc... shouldn't be followed). + if ( ! wp_is_internal_link( $url ) && in_array( $scheme, $nofollow_schemes, true ) ) { + $rel_parts[] = 'nofollow'; + } + + // Apply "ugc" when in comment context. + if ( 'comment_text' === current_filter() ) { + $rel_parts[] = 'ugc'; + } + + $rel = implode( ' ', $rel_parts ); + + /** + * Filters the rel value that is added to URL matches converted to links. + * + * @since 5.3.0 + * + * @param string $rel The rel value. + * @param string $url The matched URL being converted to a link tag. + */ + $rel = apply_filters( 'make_clickable_rel', $rel, $url ); + + $rel_attr = $rel ? ' rel="' . esc_attr( $rel ) . '"' : ''; + + return $rel_attr; + +} + /** * Converts plaintext URI to HTML links. * @@ -3137,12 +3155,8 @@ function wp_rel_callback( $matches, $rel ) { $text = $matches[1]; $atts = wp_kses_hair( $matches[1], wp_allowed_protocols() ); - if ( ! empty( $atts['href'] ) ) { - if ( in_array( strtolower( wp_parse_url( $atts['href']['value'], PHP_URL_SCHEME ) ), array( 'http', 'https' ), true ) ) { - if ( strtolower( wp_parse_url( $atts['href']['value'], PHP_URL_HOST ) ) === strtolower( wp_parse_url( home_url(), PHP_URL_HOST ) ) ) { - return ""; - } - } + if ( ! empty( $atts['href'] ) && wp_is_internal_link( $atts['href']['value'] ) ) { + $rel = trim( str_replace( 'nofollow', '', $rel ) ); } if ( ! empty( $atts['rel'] ) ) { @@ -3162,7 +3176,10 @@ function wp_rel_callback( $matches, $rel ) { } $text = trim( $html ); } - return "'; + + $rel_attr = $rel ? ' rel="' . esc_attr( $rel ) . '"' : ''; + + return ""; } /** diff --git a/src/wp-includes/link-template.php b/src/wp-includes/link-template.php index 03e6a4cc49..e823282277 100644 --- a/src/wp-includes/link-template.php +++ b/src/wp-includes/link-template.php @@ -4688,3 +4688,63 @@ function get_the_privacy_policy_link( $before = '', $after = '' ) { return ''; } + +/** + * Returns an array of URL hosts which are considered to be internal hosts. + * + * By default the list of internal hosts is comproside of the PHP_URL_HOST of + * the site's home_url() (as parsed by wp_parse_url()). + * + * This list is used when determining if a specificed URL is a link to a page on + * the site itself or a link offsite (to an external host). This is used, for + * example, when determining if the "nofollow" attribute should be applied to a + * link. + * + * @see wp_is_internal_link + * + * @since 6.2.0 + * + * @return string[] An array of URL hosts. + */ +function wp_internal_hosts() { + static $internal_hosts; + + if ( empty( $internal_hosts ) ) { + /** + * Filters the array of URL hosts which are considered internal. + * + * @since 6.2.9 + * + * @param array $internal_hosts An array of internal URL hostnames. + */ + $internal_hosts = apply_filters( + 'wp_internal_hosts', + array( + wp_parse_url( home_url(), PHP_URL_HOST ), + ) + ); + $internal_hosts = array_unique( + array_map( 'strtolower', (array) $internal_hosts ) + ); + } + + return $internal_hosts; +} + +/** + * Determines whether or not the specified URL is of a host included in the internal hosts list. + * + * @see wp_internal_hosts() + * + * @since 6.2.0 + * + * @param string $link The URL to test. + * @return bool Returns true for internal URLs and false for all other URLs. + */ +function wp_is_internal_link( $link ) { + $link = strtolower( $link ); + if ( in_array( wp_parse_url( $link, PHP_URL_SCHEME ), wp_allowed_protocols(), true ) ) { + return in_array( wp_parse_url( $link, PHP_URL_HOST ), wp_internal_hosts(), true ); + } + return false; +} diff --git a/tests/phpunit/tests/formatting/makeClickable.php b/tests/phpunit/tests/formatting/makeClickable.php index 18dbe2da0c..9a6c0cf911 100644 --- a/tests/phpunit/tests/formatting/makeClickable.php +++ b/tests/phpunit/tests/formatting/makeClickable.php @@ -108,12 +108,12 @@ class Tests_Formatting_MakeClickable extends WP_UnitTestCase { 'There was a spoon named www.wordpress.org) said Alice.', ); $urls_expected = array( - 'http://www.wordpress.org', - 'There was a spoon named http://www.wordpress.org. Alice!', - 'There was a spoon named http://www.wordpress.org, said Alice.', - 'There was a spoon named http://www.wordpress.org; said Alice.', - 'There was a spoon named http://www.wordpress.org: said Alice.', - 'There was a spoon named http://www.wordpress.org) said Alice.', + "http://www.wordpress.org", + "There was a spoon named http://www.wordpress.org. Alice!", + "There was a spoon named http://www.wordpress.org, said Alice.", + "There was a spoon named http://www.wordpress.org; said Alice.", + "There was a spoon named http://www.wordpress.org: said Alice.", + "There was a spoon named http://www.wordpress.org) said Alice.", ); foreach ( $urls_before as $key => $url ) { @@ -135,12 +135,12 @@ class Tests_Formatting_MakeClickable extends WP_UnitTestCase { 'There was a spoon named www.wordpress.org)', ); $urls_expected = array( - 'http://www.wordpress.org', - 'There was a spoon named http://www.wordpress.org.', - 'There was a spoon named http://www.wordpress.org,', - 'There was a spoon named http://www.wordpress.org;', - 'There was a spoon named http://www.wordpress.org:', - 'There was a spoon named http://www.wordpress.org)', + "http://www.wordpress.org", + "There was a spoon named http://www.wordpress.org.", + "There was a spoon named http://www.wordpress.org,", + "There was a spoon named http://www.wordpress.org;", + "There was a spoon named http://www.wordpress.org:", + "There was a spoon named http://www.wordpress.org)", ); foreach ( $urls_before as $key => $url ) { @@ -217,7 +217,7 @@ class Tests_Formatting_MakeClickable extends WP_UnitTestCase { 'In his famous speech “You and Your research” (here: http://www.cs.virginia.edu/~robins/YouAndYourResearch.html) Richard Hamming wrote about people getting more done with their doors closed...', ); $urls_expected = array( - 'Example: WordPress, test (some text), I love example.com (http://example.org), it is brilliant', + 'Example: WordPress, test (some text), I love example.com (http://example.org), it is brilliant', 'Example: WordPress, test (some text), I love example.com (http://example.com), it is brilliant', 'Some text followed by a bracketed link with a trailing elipsis (http://example.com)...', 'In his famous speech “You and Your research” (here: http://www.cs.virginia.edu/~robins/YouAndYourResearch.html) Richard Hamming wrote about people getting more done with their doors closed...', @@ -421,6 +421,7 @@ class Tests_Formatting_MakeClickable extends WP_UnitTestCase { /** * @ticket 48022 + * @ticket 56444 * @dataProvider data_add_rel_ugc_in_comments */ public function test_add_rel_ugc_in_comments( $content, $expected ) { @@ -438,14 +439,32 @@ class Tests_Formatting_MakeClickable extends WP_UnitTestCase { } public function data_add_rel_ugc_in_comments() { + + $home_url_http = set_url_scheme( home_url(), 'http' ); + $home_url_https = set_url_scheme( home_url(), 'https' ); + return array( + // @ticket 48022 array( 'http://wordpress.org', 'http://wordpress.org', ), array( 'www.wordpress.org', - '

http://www.wordpress.org', + '

http://www.wordpress.org', + ), + // @ticket 56444 + array( + 'www.example.org', + '

http://www.example.org', + ), + array( + $home_url_http, + '' . $home_url_http . '', + ), + array( + $home_url_https, + '' . $home_url_https . '', ), ); } diff --git a/tests/phpunit/tests/formatting/wpRelNofollow.php b/tests/phpunit/tests/formatting/wpRelNofollow.php index 0b45d22e15..41cf835313 100644 --- a/tests/phpunit/tests/formatting/wpRelNofollow.php +++ b/tests/phpunit/tests/formatting/wpRelNofollow.php @@ -11,16 +11,6 @@ class Tests_Formatting_wpRelNofollow extends WP_UnitTestCase { * @ticket 9959 */ public function test_add_no_follow() { - if ( PHP_VERSION_ID >= 80100 ) { - /* - * For the time being, ignoring PHP 8.1 "null to non-nullable" deprecations coming in - * via hooked in filter functions until a more structural solution to the - * "missing input validation" conundrum has been architected and implemented. - */ - $this->expectDeprecation(); - $this->expectDeprecationMessageMatches( '`Passing null to parameter \#[0-9]+ \(\$[^\)]+\) of type [^ ]+ is deprecated`' ); - } - $content = '

This is some cool Code

'; $expected = '

This is some cool Code

'; $this->assertSame( $expected, wp_rel_nofollow( $content ) ); @@ -30,16 +20,6 @@ class Tests_Formatting_wpRelNofollow extends WP_UnitTestCase { * @ticket 9959 */ public function test_convert_no_follow() { - if ( PHP_VERSION_ID >= 80100 ) { - /* - * For the time being, ignoring PHP 8.1 "null to non-nullable" deprecations coming in - * via hooked in filter functions until a more structural solution to the - * "missing input validation" conundrum has been architected and implemented. - */ - $this->expectDeprecation(); - $this->expectDeprecationMessageMatches( '`Passing null to parameter \#[0-9]+ \(\$[^\)]+\) of type [^ ]+ is deprecated`' ); - } - $content = '

This is some cool Code

'; $expected = '

This is some cool Code

'; $this->assertSame( $expected, wp_rel_nofollow( $content ) ); @@ -50,16 +30,6 @@ class Tests_Formatting_wpRelNofollow extends WP_UnitTestCase { * @dataProvider data_wp_rel_nofollow */ public function test_wp_rel_nofollow( $input, $output, $expect_deprecation = false ) { - if ( true === $expect_deprecation && PHP_VERSION_ID >= 80100 ) { - /* - * For the time being, ignoring PHP 8.1 "null to non-nullable" deprecations coming in - * via hooked in filter functions until a more structural solution to the - * "missing input validation" conundrum has been architected and implemented. - */ - $this->expectDeprecation(); - $this->expectDeprecationMessageMatches( '`Passing null to parameter \#[0-9]+ \(\$[^\)]+\) of type [^ ]+ is deprecated`' ); - } - $this->assertSame( wp_slash( $output ), wp_rel_nofollow( $input ) ); } @@ -109,16 +79,6 @@ class Tests_Formatting_wpRelNofollow extends WP_UnitTestCase { } public function test_append_no_follow_with_valueless_attribute() { - if ( PHP_VERSION_ID >= 80100 ) { - /* - * For the time being, ignoring PHP 8.1 "null to non-nullable" deprecations coming in - * via hooked in filter functions until a more structural solution to the - * "missing input validation" conundrum has been architected and implemented. - */ - $this->expectDeprecation(); - $this->expectDeprecationMessageMatches( '`Passing null to parameter \#[0-9]+ \(\$[^\)]+\) of type [^ ]+ is deprecated`' ); - } - $content = '

This is some cool Code

'; $expected = '

This is some cool Code

'; $this->assertSame( $expected, wp_rel_nofollow( $content ) ); diff --git a/tests/phpunit/tests/formatting/wpRelUgc.php b/tests/phpunit/tests/formatting/wpRelUgc.php index 4b8b11dafa..6f5f942596 100644 --- a/tests/phpunit/tests/formatting/wpRelUgc.php +++ b/tests/phpunit/tests/formatting/wpRelUgc.php @@ -11,16 +11,6 @@ class Tests_Formatting_wpRelUgc extends WP_UnitTestCase { * @ticket 48022 */ public function test_add_ugc() { - if ( PHP_VERSION_ID >= 80100 ) { - /* - * For the time being, ignoring PHP 8.1 "null to non-nullable" deprecations coming in - * via hooked in filter functions until a more structural solution to the - * "missing input validation" conundrum has been architected and implemented. - */ - $this->expectDeprecation(); - $this->expectDeprecationMessageMatches( '`Passing null to parameter \#[0-9]+ \(\$[^\)]+\) of type [^ ]+ is deprecated`' ); - } - $content = '

This is some cool Code

'; $expected = '

This is some cool Code

'; $this->assertSame( $expected, wp_rel_ugc( $content ) ); @@ -30,16 +20,6 @@ class Tests_Formatting_wpRelUgc extends WP_UnitTestCase { * @ticket 48022 */ public function test_convert_ugc() { - if ( PHP_VERSION_ID >= 80100 ) { - /* - * For the time being, ignoring PHP 8.1 "null to non-nullable" deprecations coming in - * via hooked in filter functions until a more structural solution to the - * "missing input validation" conundrum has been architected and implemented. - */ - $this->expectDeprecation(); - $this->expectDeprecationMessageMatches( '`Passing null to parameter \#[0-9]+ \(\$[^\)]+\) of type [^ ]+ is deprecated`' ); - } - $content = '

This is some cool Code

'; $expected = '

This is some cool Code

'; $this->assertSame( $expected, wp_rel_ugc( $content ) ); @@ -50,16 +30,6 @@ class Tests_Formatting_wpRelUgc extends WP_UnitTestCase { * @dataProvider data_wp_rel_ugc */ public function test_wp_rel_ugc( $input, $output, $expect_deprecation = false ) { - if ( true === $expect_deprecation && PHP_VERSION_ID >= 80100 ) { - /* - * For the time being, ignoring PHP 8.1 "null to non-nullable" deprecations coming in - * via hooked in filter functions until a more structural solution to the - * "missing input validation" conundrum has been architected and implemented. - */ - $this->expectDeprecation(); - $this->expectDeprecationMessageMatches( '`Passing null to parameter \#[0-9]+ \(\$[^\)]+\) of type [^ ]+ is deprecated`' ); - } - $this->assertSame( wp_slash( $output ), wp_rel_ugc( $input ) ); } @@ -99,25 +69,16 @@ class Tests_Formatting_wpRelUgc extends WP_UnitTestCase { ), array( 'Home URL (http)', - 'Home URL (http)', + 'Home URL (http)', ), array( 'Home URL (https)', - 'Home URL (https)', + 'Home URL (https)', ), ); } public function test_append_ugc_with_valueless_attribute() { - if ( PHP_VERSION_ID >= 80100 ) { - /* - * For the time being, ignoring PHP 8.1 "null to non-nullable" deprecations coming in - * via hooked in filter functions until a more structural solution to the - * "missing input validation" conundrum has been architected and implemented. - */ - $this->expectDeprecation(); - $this->expectDeprecationMessageMatches( '`Passing null to parameter \#[0-9]+ \(\$[^\)]+\) of type [^ ]+ is deprecated`' ); - } $content = '

This is some cool Code

'; $expected = '

This is some cool Code

';