Make WordPress Core

Opened 11 years ago

Closed 8 years ago

Last modified 5 years ago

#25385 closed defect (bug) (maybelater)

Validate URL for user comments in Comment Form

Reported by: nofearinc's profile nofearinc Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch dev-feedback
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Submitting a comment on a post allows for any sort of non-empty URL website address, including "asdf". Using the is_url function proposal in #12868 by technosailor would help validating various URLs around the core, and adding a verification in wp-comment-post.php would disallow commenting with invalid URLs.

Somewhat related: #10970, #18769.

Attachments (1)

25385.patch (1.4 KB) - added by nofearinc 11 years ago.

Download all attachments as: .zip

Change History (10)

@nofearinc
11 years ago

#1 @SergeyBiryukov
11 years ago

  • Description modified (diff)

#2 follow-up: @johnbillion
11 years ago

Two things.

  1. We should absolutely not introduce another pluggable function. What use case is there for is_url() being pluggable?
  2. The patch needs to account for no URL being entered, and http:// being entered.

#3 in reply to: ↑ 2 @nofearinc
11 years ago

Replying to johnbillion:

  1. We should absolutely not introduce another pluggable function. What use case is there for is_url() being pluggable?

The idea of is_url is not strict itself, and there are use cases where people might rely on a single group of addresses (for example, only .be or only punycode). Or an Intranet site for an organization, validating only *.someuni.edu. But I agree that a filter could be used instead and the function could be moved somewhere else.

Replying to johnbillion:

  1. The patch needs to account for no URL being entered, and http:// being entered.

Initially the original function referred to localhost as being a valid URL according to the RFC, and I would see how this would be helpful for Intranet sites, but I'm not sure if it should be allowed in general.

In my opinion there are several ways we could work this out, and I'm just proposing one of them that is liberal enough but doesn't allow random relative literals as comment URLs.

#4 follow-up: @mark-k
11 years ago

-1 at least in what relates to comments for the following reasons

  1. What is the real life problem that invalid author urls cause?
  2. why only http? what is wrong with an author that have an ftp site and want to use ftp://...
  3. Even when the check passes, it still might not be a valid URL, maybe it has a none existing TLD. The only way to check url validity is to try to "ping" it
  4. If I misspelled and wrote htt://examle.com, I would need to reenter the whole comment. I think that in that case I will not bother. Right now the patch doesn't even return me to the post on which I was commenting. If returned to the right ost then I don't see how you can dislay the error.
  5. The author url data is optional, if we can live with it being empty then why can't we live with it being wrong?

That being said, maybe it is a good idea to just remove the url data if it is invalid, maybe even try to auto-correct problems like in point 4.

#5 in reply to: ↑ 4 @nofearinc
11 years ago

Replying to mark-k:

  1. What is the real life problem that invalid author urls cause?

More spam due to the lack of validation, wrong URLs when someone enters "example.com" and it leads to an inner page, typos while entering/pasting URLs, etc.

Replying to mark-k:

  1. If I misspelled and wrote htt://examle.com, I would need to reenter the whole comment.

Testing with Chrome, Firefox and Opera here the "back" button keeps the original comment content. Might be wrong, but most browsers started doing it at some point of time to prevent that sort of troubles.
You're definitely right about the lack of a useful mechanism to get back though.

Replying to mark-k:

That being said, maybe it is a good idea to just remove the url data if it is invalid, maybe even try to auto-correct problems like in point 4.

That's also one of the possible ways to fix that, yes.

#6 @chriscct7
8 years ago

  • Keywords dev-feedback added

This ticket was mentioned in Slack in #core-comments by rachelbaker. View the logs.


8 years ago

#8 @rachelbaker
8 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

There is no one size fits all solution here.

If the current theme supports html5 for comment-form the type=url attribute is added to the comment_author_url input. You can use the comment_form_default_fields filter to add a pattern attribute with a regex that further restricts what urls should be allowed. Example: pattern="https?://.+"


#9 @schlessera
5 years ago

I propose reopening this and changing the default comment rendering code instead to check whether the URL is valid, and only add it as the href for an <a> element if that is the case. Otherwise, the URL will be displayed as text only, and will not be a clickable link.

Doing it like this means that:

  • The comment submission won't get stuck or be aborted.
  • The URL is stored and can be modified by the site owner if needed.
  • The frontend will not render an invalid (and potentially unsafe) URL as a clickable link.
Note: See TracTickets for help on using tickets.