WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 3 months ago

#23050 new defect (bug)

make_clickable incorrectly formats anchors with URL's and spaces in them in comments

Reported by: johnjamesjacoby Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.2
Component: Formatting Keywords: needs-patch has-unit-tests
Focuses: Cc:

Description

When posting a comment, if an anchor tag contains both a URL and some words, make_clickable formats the output incorrectly.

To duplicate, post the following content in a comment:

Hey! <a href="http://wordpress.org">http://wordpress.org is awesome</a> in case you didn't know!

When viewing the comment, you'll get:

Hey!
<a href="#" rel="nofollow"></a>
<a href="http://wordpress.org" rel="nofollow">http://wordpress.org</a> is awesome in case you didn't know.

Attachments (4)

fix.make.clickable.23050.diff (470 bytes) - added by mikejolley 6 months ago.
Fix for the code which removes links within links
23050-unit-tests.diff (3.6 KB) - added by netweb 6 months ago.
WITHOUT rel="nofollow"
23050-unit-tests-nofollow.diff (3.7 KB) - added by netweb 6 months ago.
WITH rel="nofollow"
23050-ala-23757.diff (928 bytes) - added by beaulebens 4 months ago.
Ignore links within link a la code/pre blocks

Download all attachments as: .zip

Change History (16)

comment:1 netweb16 months ago

  • Cc netweb added

comment:2 miqrogroove16 months ago

  • Version changed from 3.5 to 3.4.2

Confirmed in 3.5 as well as in 3.4.2.

mikejolley6 months ago

Fix for the code which removes links within links

comment:3 mikejolley6 months ago

Attached http://core.trac.wordpress.org/attachment/ticket/23050/fix.make.clickable.23050.diff which fixed the link within link code. Before it would only work if the closing link tags were side by side. This checks for content between them.

comment:4 netweb6 months ago

  • Keywords has-patch added; needs-patch removed

Awesome, works for me.

Related: bbPress#1932

netweb6 months ago

WITHOUT rel="nofollow"

comment:5 netweb6 months ago

  • Keywords needs-patch added; has-patch removed

Spoke to soon, adding some unit tests (I am pretty sure links should include rel="nofollow")

  • 23050-unit-tests.diff - Resulting links WITHOUT rel="nofollow"
  • 23050-unit-tests-nofollow.diff - Resulting links WITH rel="nofollow"

Current patch attachment:fix.make.clickable.23050.diff​ breaks PHPUnit formatting test.

$ phpunit --group formatting
There was 1 failure:
1) Tests_Formatting_MakeClickable::test_no_links_within_links
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Some text with a link <a href="http://example.com">http://example.com</a>'
+'Some text with a link <a href="http://example.com"><a href="http://example.com" rel="nofollow">http://example.com</a></a>'
$ phpunit --group 23050
There was 1 failure:

1) Tests_Formatting_MakeClickable::test_anchors_with_url_and_words
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Hey! <a href="http://wordpress.org" rel="nofollow">http://wordpress.org is awesome</a> if you have clicks to give!'
+'Hey! <a href="http://wordpress.org">http://wordpress.org is awesome</a> if you have clicks to give!'

netweb6 months ago

WITH rel="nofollow"

comment:6 mikejolley5 months ago

@netweb Personally I would expect existing links to be left alone. The code patched was to fix links within links, the links within links being the invalid ones (which would have nofollow).

Entire string could, after fixing, be ran though something like wp_rel_nofollow which is also in the formatting.php file, but I'm not sure if this is the expected behaviour or not - the function is make_clickable, and makes no mention of formatting links with nofollow.

In fact, in the case of comments where nofollow is important, the content would have already been nofollowed during the pre_comment_content hook.

comment:7 mikejolley5 months ago

  • Cc mike.jolley@… added

comment:8 beaulebens4 months ago

  • Cc beau@… added

comment:9 beaulebens4 months ago

Using a similar approach to #23756 might make sense here. Example patch attached (which includes compacting the initial regex into a single one, vs introducing a third) and moving the $nested_code_pre check in the elseif to the start to avoid doing string operations if unnecessary.

I have not run the unit tests because I don't currently have them set up :)

beaulebens4 months ago

Ignore links within link a la code/pre blocks

comment:10 netweb4 months ago

PHPUnit tests using 23050-ala-23757.diff

phpunit --group "formatting,23050" - 23050-unit-tests-nofollow.diff - Resulting links WITH rel="nofollow"

Time: 11 seconds, Memory: 43.75Mb

There were 2 failures:

1) Tests_Formatting_MakeClickable::test_anchors_with_url_and_words
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Hey! <a href="http://wordpress.org" rel="nofollow">http://wordpress.org is awesome</a> if you have clicks to give!'
+'Hey! <a href="http://wordpress.org">http://wordpress.org is awesome</a> if you have clicks to give!'

C:\xampp\htdocs\develop.wp.nw\tests\phpunit\tests\formatting\MakeClickable.php:390

2) Tests_Formatting_MakeClickable::test_square_brackets
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<a href="http://example.com/?foo%5Bbar%5D=baz" rel="nofollow">http://example.com/?foo%5Bbar%5D=baz</a>'
+'<a href="http://example.com/?foobar=baz" rel="nofollow">http://example.com/?foobar=baz</a>'

C:\xampp\htdocs\develop.wp.nw\tests\phpunit\tests\formatting\MakeClickable.php:434

FAILURES!
Tests: 557, Assertions: 1430, Failures: 2, Incomplete: 1, Skipped: 27.

phpunit --group "formatting,23050" - 23050-unit-tests.diff - Resulting links WITHOUT rel="nofollow"

Time: 6 seconds, Memory: 43.00Mb

OK, but incomplete or skipped tests!
Tests: 557, Assertions: 1433, Incomplete: 1, Skipped: 28.

phpunit --group "formatting" i.e. without any of the 23050 unit tests

Time: 7 seconds, Memory: 43.25Mb

OK, but incomplete or skipped tests!
Tests: 556, Assertions: 1428, Incomplete: 1, Skipped: 28.

comment:11 alex-ye3 months ago

  • Cc nashwan.doaqan@… added

comment:12 nacin3 months ago

  • Keywords has-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

So, looks like there is still some breakage. I don't think we should allow for someone to circumvent nofollow (at least it's not a security issue). Let's see what we can do here. Probably needs more tests and obviously some tweaks to the patch.

I definitely like beauleben's approach, but I'm up for whatever pans out.

Note: See TracTickets for help on using tickets.