drupal_http_request adds additional \r\n to message body against HTTP specs

Also, there is a bug here that strlen() may not return the correct length, due to mbstring.func_overload. Comments on the PHP site suggest getting the byte count using mb_strlen($string, '8bit'); if it is available.

Here's Jacob's missing screenshot:

First of all, drupal_http_request only supports HTTP 1.0 (RFC 1945).

The proposed patch is incorrect because implode creates for array('line1' => 'value', 'line2' => 'value', 'line3' => 'value') the following request part (spaces/linebreaks for clarity only):

METHOD path HTTP/1.0 CRLF
line 1 : value CRLF
line 2 : value CRLF
line 3 : value

The next few lines then make this

After $request .= "\r\n\r\n"; we have:

METHOD path HTTP/1.0 CRLF
line 1 : value CRLF
line 2 : value CRLF
line 3 : value CRLF
CRLF (the empty line)

Then we add the body.

Can the extra CRLF between header and body you noticed originate from a header value that already had a CRLF?

The CRLF after the entity body is forbidden by the BNF in RFC 1945 (4.1. Full request). drupal_http_request was added in revision 1.304, but I was unable to find the associated issue, so I do not know why the CRLF was added.

Attached patch removes the CRLF epilogue and also allows sending '0' as data.

Patch is against 7

Looks good, but maybe we should just check isset()?

We should check if (isset($data)) since it may be null. That will still allow '0' to be sent as data.

here's a 6.x backport.

NULL will be cast to an empty string; why the need for isset?

Cross-posted. :)

actually, talking with Heine, his version is probably better, since it's simpler and the NULL to string cast causes no problems.

here's a 6.x verison of his.

patch in #14 also applies to 5.x

Committed to 6.x. Moving back to 7.x, let's get this in there really :)

Reposting with a better patch name so it gets picked up by the testing bot. This is also the same as patch #4.

Should we also maybe run a trim() or a preg_replace on the header values sent by drupal_http_request so we don't get extra line feeds where they shouldn't be (prevent the problem from #5)?

In drupal_http_request():

  foreach ($headers as $header => $value) {
    $defaults[$header] = $header .': '. trim($value);
    // Or...
    $defaults[$header] = $header .': '. preg_replace('/[\n\r]/', '', $value);
  }

That should probably be a separate issue, I know, but I just wanted to get some initial feedback.

Well, from reading RCF 1945, it says that "Header fields can be extended over multiple lines by preceding each extra line with at least one SP or HT, though this is not recommended." trim() would be the appropriate since it will remove any line returns or spaces from the ends of the header value string.

Completely messed the last patch up...trying again.

Status back to code needs review.

I dive into this issue, having a problem very close to it:

The code in drupal_http_request adds a default "Content-Length" header that takes strlen($data) as value.
Thus, when issuing a simple GET request with no body content, we still have this "Content-Length: 0" header.
That's a problem for some servers hardened by Apache mod_security who blindly reject GET requests with a Content-Length header (see example here: http://www.modsecurity.org/documentation/modsecurity-apache/1.9.3/html-m... , third rule from the end).
As a result, Aggregator is unable to retrieve a feed from this site : http://attacvillefranche.ouvaton.org/backend.php3 , for example (gets a 403 Forbidden response, due to mod_security rejecting the request).

Examining HTTP/1.0 RFC, I didn't find anything that forbids us to have a Content-Length field with no body following it, but given that it may cause problems with some servers and that it lacks coherence, I would suggest to clean up the code...

I propose to test $data before adding the default "Content-Length" header (see diff attachment).
Sorry for my english and my inexisting programming skills, I hope you nevertheless will find my comment usefull...

I think you can make this simple if($data) - there is no need to compare to an empty string.

I committed the patch in #22 but not the patch in #24. They seem to do different things. Let's keep talking about the patch in #24. (Ideally, we would create a new issue for #24).

Agreed, #24 should be a separate issue.