WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#938 closed defect (bug) (fixed)

maximum number of links in comment "exploit"

Reported by: Ozh Owned by:
Milestone: Priority: low
Severity: minor Version: 1.5.1
Component: General Keywords: has-patch
Focuses: Cc:

Description

Function 'check_comment' in comment-functions.php counts the number of 'http:' occurences. So a spammer can post as much links as wished if typed this way :
<a href="crapsite.com">link</a>

Attachments (2)

938.diff (606 bytes) - added by Nazgul 10 years ago.
938b.diff (605 bytes) - added by Nazgul 10 years ago.

Download all attachments as: .zip

Change History (13)

#1 @Ozh
11 years ago

  • Patch set to No

#2 @ryan
11 years ago

  • Patch changed from No to Yes

#3 @skippy
11 years ago

  • Keywords bg|2nd-opinion added
  • Owner changed from anonymous to skippy
  • Priority changed from normal to low
  • Status changed from new to assigned

Should check_comment check non-http links? FTP / https / ??

Is this a big deal?

#4 @markjaquith
11 years ago

  • Keywords bg|dev-feedback added

well, the site.com links would be clickable... so they should probably get counted. I don't think https or FTP is going to be a problem, though.

We could count href="http:// and href=" and combine the two, or move to a regex. Also have to consider href='http:// and href=' so it might be time to go the regex route.

Another issue: <a href="http://site.com/">http://site.com/</a> gets penalized twice with the current situation, and if you're using a formatting filter on your comments, those links could get through on their own. Should we be processing the comment text filters before looking for links? That's how it's going to appear on your site...

#5 @davidhouse
11 years ago

The quotes are optional in HTML, so don't require their presence. I.e., <a href=http://example.com>blah</a> works in most browsers. I'd suggest checking with:

#href=('|")?(https?:)?("|')?#

@Nazgul
10 years ago

#6 @Nazgul
10 years ago

  • Keywords 2nd-opinion dev-feedback has-patch added; bg|2nd-opinion bg|dev-feedback removed
  • Milestone set to 2.1

This ticket hasn't been updated in ages, but I think it's still valid.

I've created a patch for it which should cover most of the given scenarios, but some feedback would be appreciated.

#7 @foolswisdom
10 years ago

  • Owner skippy deleted
  • Status changed from assigned to new

Clearing owner = skippy

@Nazgul
10 years ago

#8 @Nazgul
10 years ago

  • Keywords 2nd-opinion dev-feedback removed
  • Milestone changed from 2.1 to 2.0.5

Updated patch, based on feedback on IRC:

[06:29] MarkJaquith: BasB: regarding the \/\/ part (http://) ... you're using pipes to delimit the regex, so you shouldn't have to escape those forward slashes
[06:30] MarkJaquith: oh, and let's make it case insensitive

Also marking this as a 2.0.5 candidate.

#9 @markjaquith
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [4299]) comment link counting improvements from Nazgul. fixes: #938

#10 @foolswisdom
10 years ago

[4300] same fix for branches/2.0 -> 2.0.5

#11 @anonymous
10 years ago

  • Milestone 2.0.5 deleted

Milestone 2.0.5 deleted

Note: See TracTickets for help on using tickets.