WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 9 months ago

#11286 closed defect (bug) (fixed)

Normal User Input Causes Status 500

Reported by: miqrogroove Owned by: ocean90
Milestone: 4.1 Priority: normal
Severity: normal Version: 2.8.4
Component: Comments Keywords: has-patch commit
Focuses: Cc:

Description

To reproduce: Click the Submit Comment button on any post with no other input.

Expected Result: Status 200 or 403 with feedback to user.

Actual Result: Status 500 with feedback to user.

Status 500 means the server is at fault for an unexpected error condition, which is not the case here. It is the incorrect response to send, and it is alarming to see it show up in a server log without an error message.

Patch should be ready shortly...

Attachments (8)

fix-http-status-in-comments.patch (1.6 KB) - added by miqrogroove 6 years ago.
fix-http-status-in-comments-2.patch (2.3 KB) - added by solarissmoke 6 years ago.
fix-http-status-in-comments.2.patch (3.6 KB) - added by miqrogroove 6 years ago.
Dropkick blank commenters back to the post page. Prefer 400 status.
new-function-idea.diff (739 bytes) - added by miqrogroove 6 years ago.
Proposal for wp_forbid() to be used instead of top-of-file 500 status
fix-http-status-in-comments.3.patch (3.6 KB) - added by solarissmoke 5 years ago.
fix-http-status-in-comments.4.patch (3.7 KB) - added by miqrogroove 5 years ago.
refreshed proper
fix-http-status-in-comments.11286.patch (2.7 KB) - added by mackensen 12 months ago.
Refreshed, with some changes
11286.diff (2.7 KB) - added by johnbillion 10 months ago.

Download all attachments as: .zip

Change History (60)

comment:1 @miqrogroove6 years ago

  • Keywords has-patch added
  • Priority changed from normal to high

comment:2 @nacin6 years ago

See also #10551.

comment:3 @nacin6 years ago

  • Priority changed from high to normal

You can't really wp_die() on a form validation error, then fix it by passing a 200 response code. I'm pretty sure there's another ticket somewhere that discusses better handling for comment validation, for 3.0, but I can't find it.

comment:4 @dd326 years ago

But at the same time, a 200 error for an error conditional isnt right.

See also: #7246: defect (bug): Comments too quick should return 403 not 500 error (closed: fixed)

comment:5 follow-ups: @miqrogroove6 years ago

But at the same time, a 200 error for an error conditional isnt right.

There is no relevant error at the HTTP layer. Just as you would not set status 403 for comment moderation, in this case the client and server have done nothing wrong. The server should return OK for the request.

comment:6 in reply to: ↑ 5 @nacin6 years ago

Replying to miqrogroove:

The server should return OK for the request.

Bigger picture, we should stop these validation errors from wp_die()-ing and instead try to offer some real feedback.

That's tough to do because it would be a theme-side response -- it sounds like a good case of the new theme support registration.

More immediate picture -- if you're A) omitting fields marked as required, B) submitting an invalid email address, or C) submitting an empty comment, the client *did* do something wrong, which is why they're getting a wp_die error thrown at them. I'm thinking the HTTP response should be a generic bad request (400), which is more relevant than 200. I'd find 200 only relevant if it wasn't taking the user off to a display-error-and-die page.

comment:7 @miqrogroove6 years ago

I will concede that anything is better than 500 at this point. But we are still confusing the HTTP and HTML layers. If the client returns a zero-byte string for a "required" field, then that is a completely valid response at the HTTP layer, and I think it is inappropriate to throw Bad Request for valid requests. If the client returns an entity omitting form fields entirely, then in that case you're at least in a gray area and should use 403.

comment:8 @nacin6 years ago

Replying to miqrogroove:

I will concede that anything is better than 500 at this point. But we are still confusing the HTTP and HTML layers. If the client returns a zero-byte string for a "required" field, then that is a completely valid response at the HTTP layer

I agree, only up until the point where the script calls wp_die(). You shouldn't terminate a script and show a failure message yet send 200. It just doesn't sit right.

If the client returns an entity omitting form fields entirely, then in that case you're at least in a gray area and should use 403.

In that case, my opinion is that a completely blank form submission it should send the user back to the page from whence they came, with a 200. No feedback there is better than unnecessary feedback. (Unnecessary - wp_die(). Useful feedback - inline.)

comment:10 in reply to: ↑ description @solarissmoke6 years ago

  • Cc solarissmoke added

Replying to miqrogroove:

With regard to your proposed patch, you also need to modify wp-includes/comments.php, because that is where duplicate comments are checked. Amendment attached.

comment:11 @solarissmoke6 years ago

Sorry, I meant wp-includes/comment.php

comment:12 @westi6 years ago

  • Milestone changed from 2.9 to 3.0

We are not going to change this for 2.9 as we are too close to release.

Moving to 3.0

comment:13 @miqrogroove6 years ago

In between beta and release there is usually a part where all the known bugs get fixed. Fix it or close it. Punting these things is a waste of everyone's time.

comment:14 @dd326 years ago

In between beta and release there is usually a part where all the known bugs get fixed. Fix it or close it. Punting these things is a waste of everyone's time.

In WordPress that period is usually reserves for major bugs, and those which are regressions from the previous version, This is in order to attempt to make sure the final release does not work worse than the previous.

Due to this being a change in the output behaviour of WordPress, It should be dealt with during the development period.

I'm not saying this is the absolute best proceedure, I am mearly stating that this is how its currently done. The next release of WordPress is only just around the corner, The core developers are focusing only on show-stopping regressions. This is not a regression from 2.8.x

comment:15 @solarissmoke6 years ago

  • Cc solarissmoke removed

comment:16 @miqrogroove6 years ago

For the benefit of people who actually care about fixing the bugs, could you please do us a favor and explain those policies in the Bug Hunt and Beta announcements? If I had known you weren't going to fix any of the bugs in 2.8.x then I would have waited until you were ready to do so.

comment:17 @Denis-de-Bernardy6 years ago

  • Cc Denis-de-Bernardy added

comment:19 @hakre6 years ago

my fault, xref: #5998

@miqrogroove6 years ago

Dropkick blank commenters back to the post page. Prefer 400 status.

comment:20 follow-up: @miqrogroove6 years ago

Patch refreshed. This might get expanded further.

@miqrogroove6 years ago

Proposal for wp_forbid() to be used instead of top-of-file 500 status

comment:21 in reply to: ↑ 20 ; follow-up: @solarissmoke6 years ago

Replying to [miqrogroove]:

Prefer 400 status

The HTTP Specification says that 400 is for when "the request could not be understood by the server due to malformed syntax". So whilst this might be an appropriate response for invalid inputs (e.g. bad email address), I don't think it should be used in wp-includes/comment.php for duplicate comments and comment flooding. There 403 is more appropriate.

comment:22 in reply to: ↑ 21 @miqrogroove6 years ago

Replying to solarissmoke:

There 403 is more appropriate.

Personally, I would prefer 409 for anti-flood and duplicates. The 400s were an attempt to accommodate nacin's position.

comment:23 @aaroncampbell5 years ago

  • Cc aaron@… added

comment:24 @solarissmoke5 years ago

Refreshing the patch in view of other changes to wp-comments-post.

It would be really nice if this is fixed in the 3.0 beta, because the way things are currently, commenters get a blank screen with no explanation message if the manage to post to a trashed/draft/protected comment. This isn't very user-friendly (aside from the status header issue).

comment:25 @nacin5 years ago

I like fix-http-status-in-comments -- any objections to this? -- then let's consider wp_forbid for 3.1 (via #10551).

comment:26 @miqrogroove5 years ago

Agreed. Let me know if you need a proper refresh. 3.patch doesn't look right.

comment:27 @nacin5 years ago

  • Keywords needs-refresh added

If it doesn't look right, then yeah, a refresh would help.

@miqrogroove5 years ago

refreshed proper

comment:28 @miqrogroove5 years ago

  • Keywords commit added; needs-refresh removed

Patch Refreshed

comment:29 @nacin5 years ago

Looks fine. The first part of the diff, we add four strings to replace exit calls. Those are just sanity checks for someone either modifying HTML, or with the form loaded before the post was, say, moved to the trash (but posted after). We'll never be generating a public page for any of those conditions with a form that posts to wp-comments-post.php. So not sure if we need to add new strings for those.

comment:30 follow-up: @miqrogroove5 years ago

When I got into that file I couldn't find a reason for having an error message on one case and not on the other four, so I made it consistent. I could re-organize the if block so that it's only one string. Was that your thinking?

comment:31 in reply to: ↑ 30 @nacin5 years ago

Replying to miqrogroove:

I could re-organize the if block so that it's only one string. Was that your thinking?

Or just use the same generic error string. It's extremely edge and shouldn't happen, so let's make the translators work less. If we can use a string that would already be used in a more public way, that's additional points.

comment:32 @nacin5 years ago

  • Keywords early added
  • Milestone changed from 3.0 to 3.1

Let's do this w/ #10551.

comment:33 @nacin5 years ago

  • Milestone changed from Awaiting Triage to Future Release

comment:34 @miqrogroove4 years ago

What do we need to do to get this ticket closed? Just refresh the patch and commit?

comment:36 in reply to: ↑ 5 @bpetty3 years ago

  • Cc bpetty added

Replying to miqrogroove:

There is no relevant error at the HTTP layer. Just as you would not set status 403 for comment moderation, in this case the client and server have done nothing wrong. The server should return OK for the request.

This is entirely correct. Form validation errors (even duplicate comment rejections) should always return 200. It's the body of the request that is responsible for giving the response whether it's valid or not.

The only valid case I can see for anything other than 200 here is if authentication was required for submitting comments, and the client wasn't authenticated (or authenticated with an account without proper permissions), so just 403. This is only because in that case, the form shouldn't even be processed, it's the actual POST request being rejected, not the form values.

comment:37 @mdgl3 years ago

  • Version changed from 2.8.4 to trunk

I agree totally with bpetty (comment:36) that comment validation errors should just return a HTTP response code of 200, also with nacin (#18630 - first comment) that big changes are really needed to the way that we post comments.

The current mechanism (HTTP POST to /wp-comments-post.php followed by redirect back to original page) is somewhat antediluvian and I believe today's users will expect to interact directly with the comment form on the page, perhaps with JS for validation and submission. That, however has the potential to impact greatly on themes.

As an interim measure, I propose that the calls to wp_die() that originate from wp_allow_comment() are moved just to the "top level applications" that submit comments (i.e. wp-comments-post.php, admin and XML-RPC). This would involve a little refactoring of functions wp_new_comment() and wp_allow_comment() to return an error status/message to be checked by callers before deciding whether they should die or not. We should not really be aborting execution deep down inside such library functions.

This would at least localise the problem (#11286, #17159, #10551) and give themes/plugins a fighting chance of customising comment submission if they want (#18630).

comment:38 @SergeyBiryukov3 years ago

  • Version changed from trunk to 2.8.4

Version number indicates when the bug was initially introduced/reported.

comment:39 follow-up: @miqrogroove3 years ago

Three years on, still hacking core files to fix this problem with every version.

http://codex.wordpress.org/images/b/b3/donthack.jpg

comment:40 @knutsp3 years ago

  • Cc knut@… added

comment:41 in reply to: ↑ 39 @bpetty3 years ago

Replying to miqrogroove:

Three years on, still hacking core files to fix this problem with every version.

Returning 403 codes as opposed to 500 is definitely better, but there's still a lot more room for improvement here.

First, I still think these should actually be responses with code 200. Sysadmins (and automated blacklist services like fail2ban) keep an eye on server logs for 403 to identify brute force attempts, and other malicious activity, and this just makes their job harder to correctly identify real threats when it's just legitimate visitors accidentally double-clicking the comment submit button for example.

Besides that though, we could be returning a much more useful variety of error codes to the frontend to distinguish the types of errors (besides just 403 used for all of them) so it knows better how to handle the response besides showing the visitor a localized message, and frontend code having no idea why it failed.

Anyway, if this is changed too quickly here, plugins and themes will need to be fixed to handle this (it breaks backwards compat), and we don't want to do that twice. So I really believe this should wait off until the core team has some time to start looking at some of the long overdue comment system improvements that have been briefly touched on in a number of open tickets related to comments (maybe 3.6, but maybe not even until 3.7) - most of those tickets have already been mentioned here.

For that matter, this patch as it stands could do a better job at maintaining backwards compatibility itself.

Last edited 3 years ago by bpetty (previous) (diff)

comment:42 @miqrogroove2 years ago

This remains the number 1 item on my WordPress bug list. When we fail as a community to fix these problems, it really discourages upgrades.

comment:43 @SergeyBiryukov18 months ago

#27248 was marked as a duplicate.

@mackensen12 months ago

Refreshed, with some changes

comment:44 @mackensen12 months ago

I've refreshed the patch and made a couple changes to the previous patches:

  • Validation errors: 200. I agree completely w/bpetty. This isn't a 4xx error: the request was understood and the client received a response.
  • Closed comments, login required: 403
  • Duplicate comment: 409. I think this is better than a 403 as it's more specific. The RFC suggests this is a permissible use.
  • Too many comments: 429

I know it's too late for 4.0 but I'd like to see this gain some traction. For anyone using monitoring tools the default behavior is counter-intuitive and problematic.

comment:45 @wonderboymusic10 months ago

  • Milestone changed from Future Release to 4.1

comment:46 @johnbillion10 months ago

In 30355:

Allow the response code to be passed as a shorthand to the $title or $args parameter of wp_die(), for brevity.

See #10551 and #11286
Props nacin

@johnbillion10 months ago

comment:47 @johnbillion10 months ago

  • Keywords early removed

11286.diff is a refresh of mackensen's patch, after r30355.

I'm happy with all of the response code changes in the patch. This can go in in the next few days unless someone objects.

comment:48 @mdgl10 months ago

No objection to the current patch but it still offends (!) that we're calling wp_die() from within a library function (see comment:37). Any interest in a further patch to localise these calls and make the functions more re-usable? I have some time this week. Really, though it's about time we re-did the whole comments subsystem...

comment:49 @nacin10 months ago

I'm confortable with the latest patch.

Any other changes should occur in a new ticket.

comment:50 @mdgl10 months ago

OK, after pretending to be Sergey for a bit I discovered we already have #14452, #16979 and #18630 related to the issue I'm raising.

I started creating a fix adopting the idiom used by wp_set_comment_status() which enables the optional return of a WP_Error object rather than dying. This works for wp_new_comment() and wp_allow_comment() but unfortunately not for check_comment_flood_db() which is called indirectly via an action (yech - who thought *that* was a good idea?). Apart from using an ugly global variable, it's unclear how to enable the behaviour of that function to be modified and for it to return an error message whilst preserving complete backward compatibility.

A more wholesale cleanup of the comments subsystem should surely be on the cards for a future 4.x release!

comment:51 @mdgl9 months ago

Patch uploaded to #18630 that would allow these HTTP response codes to be localised completely within wp-comments-post.php.

comment:52 @ocean909 months ago

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

In 30579:

Comments: Use proper HTTP response codes for validation errors.

props miqrogroove, solarissmoke, mackensen.
fixes #11286.

Note: See TracTickets for help on using tickets.