Make WordPress Core

Opened 17 years ago

Closed 16 years ago

Last modified 15 years ago

#938 closed defect (bug) (fixed)

maximum number of links in comment "exploit"

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


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="">link</a>

Attachments (2)

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

Download all attachments as: .zip

Change History (13)

#1 @Ozh
17 years ago

  • Patch set to No

#2 @ryan
17 years ago

  • Patch changed from No to Yes

#3 @skippy
17 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
17 years ago

  • Keywords bg|dev-feedback added

well, the 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=""></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
17 years ago

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


16 years ago

#6 @Nazgul
16 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
16 years ago

  • Owner skippy deleted
  • Status changed from assigned to new

Clearing owner = skippy

16 years ago

#8 @Nazgul
16 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
16 years ago

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

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

#10 @foolswisdom
16 years ago

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

#11 @(none)
15 years ago

  • Milestone 2.0.5 deleted

Milestone 2.0.5 deleted

Note: See TracTickets for help on using tickets.