WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 43 hours ago

#10377 reopened defect (bug)

Comment fields should have max lengths

Reported by: muriloazevedo Owned by: rachelbaker
Milestone: 4.5 Priority: normal
Severity: normal Version: 2.8
Component: Comments Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Hello, I don't know if exactly it's a bug or use javascript validation, but i think so there's no
treatment for this, or was fix in the new version.

In the section of comments(Wordpress 2.8), we can insert how many characheters we wants, generating an SQL Exception and breaking the all system.

The solution is simple, use the property maxlenght in the tag
<input /> e limit the characters if will be send to database.

Attachments (8)

10377.patch (4.5 KB) - added by westonruter 2 years ago.
Add maxlength to comment textarea and size check in wp-comments-post.php (extends patch from #10377)
10377.2.patch (1.9 KB) - added by rachelbaker 5 months ago.
Refreshed patch
10377.3.patch (3.0 KB) - added by rachelbaker 5 months ago.
10377.4.diff (7.1 KB) - added by rachelbaker 5 weeks ago.
with new function wp_get_comment_column_max_length() and unit tests
10377.5.diff (8.9 KB) - added by rachelbaker 4 weeks ago.
Refresh of 10377.4 with maxlength for all comment template fields and mb_strlen checks where needed
10377.6.diff (1.4 KB) - added by rachelbaker 3 weeks ago.
Fallback to TEXT column with 65525 length and add unit tests
10377.7.diff (4.5 KB) - added by azaozz 3 weeks ago.
10377.8.diff (5.3 KB) - added by azaozz 5 days ago.

Download all attachments as: .zip

Change History (46)

#1 follow-up: @dd32
7 years ago

  • Milestone changed from Unassigned to Future Release

The solution is simple, use the property maxlenght in the tag <input /> e limit the characters if will be send to database.

That doesnt stop people from directly posting the data anyway..

The comment_content field is a TEXT field:

BLOB 
TEXT 
A BLOB or TEXT column with a maximum length of 65535 (2^16 - 1) characters. See section 7.7.1 Silent column specification changes. 
MEDIUMBLOB 
MEDIUMTEXT 
A BLOB or TEXT column with a maximum length of 16777215 (2^24 - 1) characters. See section 7.7.1 Silent column specification changes. 
LONGBLOB 
LONGTEXT 
A BLOB or TEXT column with a maximum length of 4294967295 (2^32 - 1) characters. See section 7.7.1 Silent column specification changes.

so theres a limit of 65,535 characters in a comment at present.. Perhaps the comment handler should throw a error upon a longer comment.

#2 @dd32
7 years ago

also note, That MySQL should actually truncate the data if its longer, not throw an error..

#3 in reply to: ↑ 1 @azaozz
7 years ago

Replying to dd32:

... so there's a limit of 65,535 characters in a comment at present.. Perhaps the comment handler should throw a error upon a longer comment.

There's some basic back-end validation already (duplicate comments, non-empty name and email, etc.). Adding max length for all comment fields should be trivial.

Actually this may be good for most POST requests in the admin including AJAX.

#4 @mrmist
6 years ago

  • Summary changed from Problem in comments! to Comment fields should have max lengths

#5 @n3k4
4 years ago

  • Cc hellokane@… added

@westonruter
2 years ago

Add maxlength to comment textarea and size check in wp-comments-post.php (extends patch from #10377)

#6 @westonruter
2 years ago

  • Cc weston@… added
  • Keywords has-patch added; 'sql exception' comments removed

As noted in the attachment comment, in addition to the $comment_content size check in wp-comments-post.php, I've also patched the comment form to add a maxlength to the textarea itself (this extends a patch from #4332). Patch attached, and branch also pushed to GitHub fork: https://github.com/x-team/WordPress/compare/10377-comment-fields-should-have-max-lengths

#7 @chriscct7
5 months ago

  • Keywords needs-refresh added

@rachelbaker
5 months ago

Refreshed patch

#8 @rachelbaker
5 months ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 4.4
  • Owner set to rachelbaker
  • Status changed from new to accepted

#9 @rachelbaker
5 months ago

  • Keywords commit added

Moving for commit consideration.

#10 @wonderboymusic
5 months ago

is this value conducive to utf8mb4? /cc @pento

#11 @pento
5 months ago

Yep, that's fine. comment_content is a text field, which stores 65535 bytes. strlen() also counts the number of bytes, so we're being consistent.

#12 follow-up: @SergeyBiryukov
5 months ago

Instead of a hardcoded value, seems like we should use $wpdb->get_col_length() to get the actual value, in case someone modified the DB schema for a particular install.

See upgrade_430_fix_comments() for reference.

#13 in reply to: ↑ 12 ; follow-up: @chriscct7
5 months ago

Replying to SergeyBiryukov:

Instead of a hardcoded value, seems like we should use $wpdb->get_col_length() to get the actual value, in case someone modified the DB schema for a particular install.

See upgrade_430_fix_comments() for reference.

I don't know if that's worth the extra db call. Someone could just as easily use the filter to modify it if that's the case

#14 in reply to: ↑ 13 @pento
5 months ago

Replying to chriscct7:

I don't know if that's worth the extra db call. Someone could just as easily use the filter to modify it if that's the case

It's not an extra call - the same call will be done later when the data is actually being written to the DB.

I agree with @SergeyBiryukov, $wpdb->get_col_length() is a better way of making sure it's the right length.

#15 @rachelbaker
5 months ago

  • Keywords needs-refresh added

Thank you all for the feedback and direction. Updated patch forthcoming.

#16 @rachelbaker
5 months ago

  • Keywords commit removed

#17 @rachelbaker
5 months ago

  • Keywords dev-feedback added; needs-refresh removed

I made an attempt at using $wpdb->get_col_length() in the new function wp_get_comment_content_max_length() (isn't that a mouthful).

#18 follow-up: @SergeyBiryukov
5 months ago

The maxlength attribute is probably redundant (and would cause an extra query on front-end), I think just checking the length in wp-comments-post.php would be enough.

#19 in reply to: ↑ 18 @rachelbaker
5 months ago

Replying to SergeyBiryukov:

The maxlength attribute is probably redundant (and would cause an extra query on front-end), I think just checking the length in wp-comments-post.php would be enough.

The issue I see there is that if a commenter was to leave a 65525+ comment they wouldn't know it was too long until they press submit. Including "max-length" in the comment form template would at least give the commenter feedback that they have reached some sort of limit when they try to type the 65526 character.

#20 @azaozz
5 months ago

Don't think there's a problem to have hard-coded maxlength. 65k characters is a lot of text, about 11,000 words or 44 printed pages :)

We already have a filter 'comment_form_default_fields'. Can add an array with hard-coded lengths for the (very rare) cases when somebody wants comments longer than that. Then run the same filter on checking the POST lengths. Or maybe have a function similar to wp_get_comment_content_max_length() that will filter and return the hard-coded values.

Also: what about maxlength and strlen() for the other comment fields? If we are adding that for the textarea would be good to add everywhere.

Last edited 5 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by dd32. View the logs.


4 months ago

#22 @rachelbaker
4 months ago

  • Keywords needs-refresh added; dev-feedback removed

#23 @rachelbaker
4 months ago

  • Milestone changed from 4.4 to Future Release
  • Owner rachelbaker deleted
  • Status changed from accepted to assigned

Punting out of 4.4

#24 @rachelbaker
2 months ago

  • Milestone changed from Future Release to 4.5

@rachelbaker
5 weeks ago

with new function wp_get_comment_column_max_length() and unit tests

#25 follow-up: @rachelbaker
5 weeks ago

  • Keywords has-unit-tests dev-feedback added; needs-refresh removed

In 10377.4.diff:

  • hardcoded maxlength attribute on the comment_field, the markup here can be modified via the comment_form_defaults filter (1)
  • added wp_get_comment_column_max_length() which returns the max column length for a given column name, and is filterable (2)
  • added logic in wp_handle_comment_submission() to return a WP_Error when the comment_author, comment_author_url, or comment_content values exceed the max length of their columns
  • added unit tests for the error conditions in wp_handle_comment_submission()
  1. We could add hardcoded attributes like this to the two other comment form inputs. Open to second opinions here.
  2. Is there a good way to convert characters to bytes?

#26 in reply to: ↑ 25 @azaozz
5 weeks ago

Replying to rachelbaker:

10377.4.diff is starting to look good.

  1. We could add hardcoded attributes like this to the two other comment form inputs.

Thinking this is a must. Having maxlength on the other three ( Name, Email, Website ) text fields will improve UX if these limits are reached.

  1. Is there a good way to convert characters to bytes?

If you want to check the byte length of a string, mb_strlen( $string, '8bit' ) should work well. That's the proper way to test it when the "real" mb_strlen() exists. If it doesn't exist, the compat function will return strlen( $string ).

Also this looks like a typo:

} elseif ( is_array( $col_length ) && isset( $col_length['length'] ) && (int) $col_length > 0 ) {

should probably be:

} elseif ( is_array( $col_length ) && isset( $col_length['length'] ) && intval( $col_length['length'] ) > 0 ) {
Last edited 5 weeks ago by azaozz (previous) (diff)

@rachelbaker
4 weeks ago

Refresh of 10377.4 with maxlength for all comment template fields and mb_strlen checks where needed

#27 follow-up: @rachelbaker
4 weeks ago

@azaozz Can you take a look at 10377.5.diff when you have time? I corrected my conditional, added maxlength checks for all comments-template fields, and compare the byte length on tinytext and text columns.

#28 in reply to: ↑ 27 @azaozz
4 weeks ago

Replying to rachelbaker:

10377.5.diff looks great :)

#29 @rachelbaker
4 weeks ago

  • Owner set to rachelbaker
  • Resolution set to fixed
  • Status changed from assigned to closed

In 36272:

Comments: Restrict the maximum characters for input fields within the comments template.

Added hardcoded maxlength attributes on the author, author_email, author_url, and comment_field input markup. These can be modified via the comment_form_defaults filter. Added logic in wp_handle_comment_submission() to return a WP_Error when the comment_author, comment_author_url, or comment_content values exceed the max length of their columns. Introduces wp_get_comment_column_max_length() which returns the max column length for a given column name, and is filterable. Unit tests included for the error conditions in wp_handle_comment_submission()

Fixes #10377.

Props westonruter rachelbaker.

#30 @ocean90
4 weeks ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests dev-feedback removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[36272] broke comments on w.org because $wpdb->get_col_length() is returning false and falls back to 255. Looking at upgrade_430_fix_comments() the fallback should be 65535.

Not sure yet why it returns false, but the method returns also false in case $wpdb->is_mysql is false.

Edit: $wpdb->is_mysql is false via HyperDB.

Last edited 4 weeks ago by ocean90 (previous) (diff)

@rachelbaker
3 weeks ago

Fallback to TEXT column with 65525 length and add unit tests

#31 @rachelbaker
3 weeks ago

@ocean90 In 10377.6.diff I changed the default fallback to be 65525, 65535 - 10. This matches the logic in upgrade_430_fix_comments(). Unit tests for each text-based column type for the comments table included in test_wp_get_comment_column_max_length().

#32 @rachelbaker
3 weeks ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

This ticket was mentioned in Slack in #core by ocean90. View the logs.


3 weeks ago

#34 @rachelbaker
3 weeks ago

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

In 36325:

Comments: Use TEXT column type in fallback for wp_get_comment_column_max_length().

Fixes #10377.

@azaozz
3 weeks ago

#35 @azaozz
3 weeks ago

broke comments on w.org because $wpdb->get_col_length() is returning false and falls back to 255. Looking at upgrade_430_fix_comments() the fallback should be 65535.
...the method returns also false in case $wpdb->is_mysql is false.

Not sure if the default lengths for email, name, URL should be 65525. We should be using "sane" values :)

How about we go back to the idea of having an array with the default lengths and override it with the actual lengths from the DB (if we can get them)? Something like 10377.7.diff (needs to fix the unit tests).

#36 @rachelbaker
9 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@azaozz 10377.7.diff makes more sense. Reopened this.

@azaozz
5 days ago

#37 @azaozz
5 days ago

In 10377.8.diff:

  • Don't check for type when the column length is not an array.
  • Remove unneeded test. Now we always have lengths.

#38 @DrewAPicture
43 hours ago

In 36499:

Docs: Add a changelog entry for the introduction of maxlength character limits for the 'author', 'email', and 'url' fields in comment_form().

Introduced in [36272].

See #10377. See #32246.

Note: See TracTickets for help on using tickets.