WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 8 weeks ago

#11286 new defect (bug)

Normal User Input Causes Status 500

Reported by: miqrogroove Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 2.8.4
Component: Comments Keywords: has-patch commit early
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 (6)

fix-http-status-in-comments.patch (1.6 KB) - added by miqrogroove 4 years ago.
fix-http-status-in-comments-2.patch (2.3 KB) - added by solarissmoke 4 years ago.
fix-http-status-in-comments.2.patch (3.6 KB) - added by miqrogroove 4 years ago.
Dropkick blank commenters back to the post page. Prefer 400 status.
new-function-idea.diff (739 bytes) - added by miqrogroove 4 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 4 years ago.
fix-http-status-in-comments.4.patch (3.7 KB) - added by miqrogroove 4 years ago.
refreshed proper

Download all attachments as: .zip

Change History (49)

comment:1 miqrogroove4 years ago

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

comment:2 nacin4 years ago

See also #10551.

comment:3 nacin4 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 dd324 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: miqrogroove4 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 nacin4 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 miqrogroove4 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 nacin4 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 solarissmoke4 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 solarissmoke4 years ago

Sorry, I meant wp-includes/comment.php

comment:12 westi4 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 miqrogroove4 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 dd324 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 solarissmoke4 years ago

  • Cc solarissmoke removed

comment:16 miqrogroove4 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-Bernardy4 years ago

  • Cc Denis-de-Bernardy added

comment:19 hakre4 years ago

my fault, xref: #5998

miqrogroove4 years ago

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

comment:20 follow-up: miqrogroove4 years ago

Patch refreshed. This might get expanded further.

miqrogroove4 years ago

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

comment:21 in reply to: ↑ 20 ; follow-up: solarissmoke4 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 miqrogroove4 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 aaroncampbell4 years ago

  • Cc aaron@… added

comment:24 solarissmoke4 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 nacin4 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 miqrogroove4 years ago

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

comment:27 nacin4 years ago

  • Keywords needs-refresh added

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

miqrogroove4 years ago

refreshed proper

comment:28 miqrogroove4 years ago

  • Keywords commit added; needs-refresh removed

Patch Refreshed

comment:29 nacin4 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: miqrogroove4 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 nacin4 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 nacin4 years ago

  • Keywords early added
  • Milestone changed from 3.0 to 3.1

Let's do this w/ #10551.

comment:33 nacin3 years ago

  • Milestone changed from Awaiting Triage to Future Release

comment:34 miqrogroove3 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 bpetty21 months 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 mdgl20 months 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 SergeyBiryukov20 months ago

  • Version changed from trunk to 2.8.4

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

comment:39 follow-up: miqrogroove17 months 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 knutsp17 months ago

  • Cc knut@… added

comment:41 in reply to: ↑ 39 bpetty17 months 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 17 months ago by bpetty (previous) (diff)

comment:42 miqrogroove9 months 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 SergeyBiryukov8 weeks ago

#27248 was marked as a duplicate.

Note: See TracTickets for help on using tickets.