WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 7 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 8 years ago.
938b.diff (605 bytes) - added by Nazgul 8 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Ozh9 years ago

  • Patch set to No

comment:2 ryan9 years ago

  • Patch changed from No to Yes

comment:3 skippy9 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?

comment:4 markjaquith9 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...

comment:5 davidhouse9 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?:)?("|')?#

Nazgul8 years ago

comment:6 Nazgul8 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.

comment:7 foolswisdom8 years ago

  • Owner skippy deleted
  • Status changed from assigned to new

Clearing owner = skippy

Nazgul8 years ago

comment:8 Nazgul8 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.

comment:9 markjaquith8 years ago

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

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

comment:10 foolswisdom8 years ago

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

comment:11 anonymous7 years ago

  • Milestone 2.0.5 deleted

Milestone 2.0.5 deleted

Note: See TracTickets for help on using tickets.