Ticket #938 (closed defect (bug): fixed)
maximum number of links in comment "exploit"
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Priority: | low | Milestone: | |
| Component: | General | Version: | 1.5.1 |
| Severity: | minor | Keywords: | has-patch |
| 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
Change History
- Keywords bg|2nd-opinion added
- Owner changed from anonymous to skippy
- Status changed from new to assigned
- Priority changed from normal to low
Should check_comment check non-http links? FTP / https / ??
Is this a big deal?
comment:4
markjaquith — 7 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
davidhouse — 6 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?:)?("|')?#
- 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
foolswisdom — 5 years ago
- Owner skippy deleted
- Status changed from assigned to new
Clearing owner = skippy
- 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
markjaquith — 5 years ago
- Status changed from new to closed
- Resolution set to fixed
comment:10
foolswisdom — 5 years ago
[4300] same fix for branches/2.0 -> 2.0.5
