Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#37863 closed defect (bug) (fixed)

HTTP/1.1 defined header in wp-comments-post.php

Reported by: tnash's profile tnash Owned by: johnbillion's profile johnbillion
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.6
Component: Comments Keywords:
Focuses: Cc:

Description

if ( 'POST' != $_SERVER['REQUEST_METHOD'] ) {
        header('Allow: POST');
        header('HTTP/1.1 405 Method Not Allowed');
        header('Content-Type: text/plain');
        exit;
}

Should ideally be protocol agnostic, to support HTTP/2 in line with other files this should probably be

if ( 'POST' != $_SERVER['REQUEST_METHOD'] ) {
$protocol = $_SERVER['SERVER_PROTOCOL'];
       	if ( ! in_array( $protocol, array( 'HTTP/1.1', 'HTTP/2', 'HTTP/2.0' ) ) ) {
                $protocol = 'HTTP/1.0';
       	}
        header('Allow: POST');
        header("$protocol 405 Method Not Allowed");
        header('Content-Type: text/plain');
        exit;
}

in line with other files, which can't use wp_get_server_protocol(); prior to wp_load.php

Change History (7)

#1 @johnbillion
8 years ago

  • Milestone changed from Awaiting Review to 4.7

Thanks for the report, Tim!

#2 @johnbillion
8 years ago

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

In 38432:

Comments: Add support for all HTTP protocol versions when returning a 405 from wp-comments-post.php.

Fixes #37863
Props tnash

#3 @johnbillion
8 years ago

#37869 was marked as a duplicate.

#4 @kobashicomputing
8 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Read my original feedback to the support forum for which the originator of this bug report decided to create a ticket for.

The code above is wrong.

1) Having a fallback to HTTP 1.0 is NEVER GUARANTEED to be supported on the HTTP server. The only way you as Wordpress is going to know what HTTP version is supported is through $_SERVER[SERVER_PROTOCOL]. Making the assumption that the HTTP server you are running on without even knowing it supports 1.0 is guessing.

2) There is no future support in place with this code. The code above falls apart as newer HTTP versions come out (HTTP 2.01, HTTP 3, etc) which then results in returning HTTP 1.0 error messages.

3) Further, RFC 2145 says:

"An HTTP server SHOULD send a response version equal to the highest
version for which the server is at least conditionally compliant, and
whose major version is less than or equal to the one received in the
request. An HTTP server MUST NOT send a version for which it is not
at least conditionally compliant. A server MAY send a 505 (HTTP
Version Not Supported) response if cannot send a response using the
major version used in the client's request."

You must trust what is returned in $_SERVER['SERVER_PROTOCOL'].

<?php
if ('POST' != $_SERVER['REQUEST_METHOD']) {
  header('Allow: POST');
  header($_SERVER['SERVER_PROTOCOL'] . ' 405 Method Not Allowed');
  header('Content-Type: text/plain');
  exit;
}
?>

If someone visits the post comment URL with different HTTP Methods (DELETE, GET, PUT, etc.) and you tell them you support HTTP 1.0, you are misleading them, especially if it is a service that now is going to think only HTTP 1.0 is the way to post comments.

Version 1, edited 8 years ago by SergeyBiryukov (previous) (next) (diff)

#5 @SergeyBiryukov
8 years ago

@tnash, if the ticket was indeed based on the support forums thread, it would be nice to link to it in the description, as well as mention the ticket in the thread :)

Replying to kobashicomputing:

You must trust what is returned in $_SERVER['SERVER_PROTOCOL'].

If that's the case, the code of wp_get_server_protocol() introduced in [34894] should also be corrected.

A similar code is also used in wp-admin/load-scripts.php and wp-admin/load-styles.php.

#6 @dd32
8 years ago

If I remember correctly; we've encountered misconfigured servers which pass SERVER_PROTOCOL as null or an invalid form (which is why we also support HTTP/2). In older versions of nginx it was also common to see the client connection as 1.1, but SERVER_PROTOCOL supplied to PHP as 1.0.

I don't see any major issue with falling back to HTTP/1.0 in the event that a non-1.1/2/2.0 protocol is presented (as current) despite it not fitting the strict spec, we can add support for HTTP/2.1 or HTTP/3.0 when it's released in a decade, applications in the real world handle other applications responding with a lower version number, as part of their back-compat and iterative nature of the protocols.

If we were to change something here, I'd suggest it should respect anything passed in the format of HTTP/[0-9.]+ falling back to HTTP/1.0 otherwise.

#7 @johnbillion
8 years ago

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

Based on @dd32's comments above, I'm re-closing this. As noted, incorrect values for SERVER_PROTOCOL are not uncommon, and the current array of protocol version numbers will serve us for quite a few years to come.

Note: See TracTickets for help on using tickets.