Comment form submission with invalid fields incorrectly returns 200 OK response

[westonruter]

This follows up on #11286.

I just noticed that a bunch of error responses are returning 200 status code:

• comment_author_column_length
• comment_author_email_column_length
• comment_author_url_column_length
• comment_content_column_length
• require_name_email
• require_valid_email
• require_valid_comment

The origin of this seems to be this comment:

• Validation errors: 200. I agree completely w/bpetty. This isn't a 4xx error: the request was understood and the client received a response.

I don't think this is right. Sure the request was understood, but it was understood to be incorrect.

Should they not be 400 (Bad Request)? The argument against 400 was this comment:

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.

Nothing here specifically arguing against using 400 for invalid fields.

Also, more recently the REST API sends 400 status code responses in \WP_REST_Comments_Controller::create_item() for these errors:

• rest_comment_exists
• rest_invalid_comment_type
• rest_comment_content_invalid
• rest_comment_author_data_required
• comment_author_column_length
• comment_author_email_column_length
• comment_author_url_column_length
• comment_content_column_length
• comment_flood

The exceptions are comment_duplicate which returns ​409 (Conflict), and the generic DB error rest_comment_failed_create which returns with a 500 error code, as expected.

For the length errors, a ​413 (Payload Too Large) code may be more specific. Also, for comment flooding, what about the status ​429 (Too Many Requests)? The REST API should perhaps be updated to use this instead of 400.

All this to say, to align with the REST API the comment form submission errors should use 400+ error codes instead of 200.

[westonruter]

Use 400 status instead of 200 status for invalid comment form submission responses to align with REST API: ​https://github.com/westonruter/wordpress-develop/pull/2

[miqrogroove]

Code 400 signifies a malformed HTTP header that the server cannot process. I would never use this for form validation except where the payload is missing or cannot be parsed at all.

[westonruter]

Replying to miqrogroove:

Code 400 signifies a malformed HTTP header that the server cannot process. I would never use this for form validation except where the payload is missing or cannot be parsed at all.

Then why does the WordPress REST API use the 400 response code for invalid comment requests?

[miqrogroove]

From what I know of this, REST errors are conveyed within the API's response entity. The HTTP status code reflects success or failure of handling the HTTP request. Departing from those standards would not seem to be of much benefit, but not impossible either.

[TimothyBlynJacobs]

Code 400 signifies a malformed HTTP header that the server cannot process. I would never use this for form validation except where the payload is missing or cannot be parsed at all.

This is no longer true.

From the RFC: ​https://tools.ietf.org/html/rfc7231#section-6.5.1

The 400 (Bad Request) status code indicates that the server cannot or
will not process the request due to something that is perceived to be
a client error (e.g., malformed request syntax, invalid request
message framing, or deceptive request routing).

Relevant SO discussion following that RFC: ​https://stackoverflow.com/q/16133923

From what I know of this, REST errors are conveyed within the API's response entity.

This is partially correct. The error code, equivalent to WP_Error::get_error_code(), is returned in the response entity, but the equivalent status code, for instance 400 is also returned as the HTTP status.

Taking a brief look at wp_die usages there are already cases of sending a 40x response status code.

wp_die( __( 'This site is no longer available.' ), '', array( 'response' => 410 ) );
wp_die( __( 'A post ID mismatch has been detected.' ), __( 'Sorry, you are not allowed to edit this item.' ), 400 );

Departing from those standards would not seem to be of much benefit,

In an API context, this can be extremely helpful. A number of HTTP libraries, for instance Guzzle, offer to reject 40x or 50x response codes and treat it as an error. Additionally, it allows determining that a request failed without having to scrape the output.

Then why does the WordPress REST API use the 400 response code for invalid comment requests?

One such difference is that the WordPress REST API is a REST API and returning a 200 status code for an error response is strongly discouraged.

I guess the question is whether wp-comment-post.php would be seen as an HTTP API of some kind?

[westonruter]

Replying to TimothyBlynJacobs:

One such difference is that the WordPress REST API is a REST API and returning a 200 status code for an error response is strongly discouraged.

I guess the question is whether wp-comment-post.php would be seen as an HTTP API of some kind?

The context for how I came across this is “Ajaxifying” the comment form submission, specifically in the context of AMP. The amp-form component will render responses in the submit-success element when a 200 code is received, and it will render responses in the submit-error element when a 400+ code is received. This doesn't work properly with the WordPress comment form because 200 is returned even for invalid form submissions. So this ​workaround is currently required in a custom wp_die handler:

<?php
if ( 200 === $status_code && isset( $pagenow ) && 'wp-comments-post.php' === $pagenow ) {
        $status_code = 400;
}

Full PR: ​https://github.com/ampproject/amp-wp/pull/2425

I suppose it could just use the REST API for creating the comment in the first place, but rest_allow_anonymous_comments is disabled by default. So using the wp-comment-post.php endpoint will be the most reliable.

[westonruter]

Just noticed that Jetpack's comments module was just updated to send 4xx response status codes: ​https://github.com/Automattic/jetpack/pull/12428

[TimothyBlynJacobs]

Making this change still seems logical to me, but removing the REST API focus since it isn't really impacted.