Double Content-Type headers added to request if context->http->header is a multiline string

[pharrison-weiss]

Description

When using file_get_contents to post an HTTP/HTTPS request, context->http->header can either be a string or an array of strings. If a string, it may ignore the header containing a Content-Type line and add an additional one.

The following code:

<?php
$header = "Authentication: Bearer XYZ" . PHP_EOL;
$header .= 'Content-Type: application/json' . PHP_EOL;

$httpoptions = [
 'method' => 'POST',
 'ignore_errors' => true,
 'content' => json_encode(["message" => "Hello world"]),
 'header' => $header,
];

$context = stream_context_create(['http' => $httpoptions]);

$result = file_get_contents("http://some-test-url/", false, $context);

Resulted in this request (captured using netcat):

POST / HTTP/1.1
Host: some-test-url
Connection: close
Content-Length: 25
Authentication: Bearer XYZ
Content-Type: application/json
Content-Type: application/x-www-form-urlencoded

{"message":"Hello world"}

But I expected this output instead:

POST / HTTP/1.1
Host: some-test-url
Connection: close
Content-Length: 25
Authentication: Bearer XYZ
Content-Type: application/json

{"message":"Hello world"}

Commentary:

There is a warning, "file_get_contents(): Content-type not specified assuming application/x-www-form-urlencoded", that is on some occasions issued (although not, oddly enough, for the code we tracked down this issue as applying to.)

The issue goes away if you build context->http->header as an array.

While arguably building the header as a multiline string seems (always seemed) odd to me, it's frequently quoted in examples across the Internet (which is probably how we ended up doing it) - several examples here: https://www.php.net/manual/en/function.stream-context-create.php

I doubt there are any backward compatibility issues that would be caused by a straight fix to this.

PHP Version

PHP 8.1.2

Operating System

Ubuntu 22.04

[iluuu1994]

Hi @pharrison-weiss. HTTP uses CRLF line endings. If you replace PHP_EOL with "\r\n", this works correctly.

[pharrison-weiss]

Thanks, not sure where I read to use PHP_EOL but we're switching to arrays at this point in any case.

I'd still consider it an issue because (1) the other headers are being picked up by most web servers so it's not going to be immediately obvious there's an issue (indeed, we were alerted to the issue because a certain major CRM provider's API was updated to validate the Content-Type field, and it's complaining about there being two Content-Types) and (2) it's silent and extremely difficult to see why it's failing (unless there's some way to dump sent headers that I'm unaware of?) But I guess it's changing from a "Good configuration is failing" to "Bad configuration isn't being rejected/properly warned about" type thing. Which under certain circumstances wouldn't be a major issue, it's just, like I said, almost impossible to debug short of doing what we did above with Netcat, and warnings will only be issued under some circumstances and then only a misleading one.

[iluuu1994]

Tools are often forgiving, but the standard is clear. I'll pass this along to @bukka to decide whether we want to do the same.

[bukka]

I did a bit of checking and thinking about this. Currently, we don't do any validation of request header values. In this case, it's not actually strictly wrong, because RFC 9110 places the requirement on the recipient, not the sender. Specifically, this is defined in section 5.5:

Field values containing CR, LF, or NUL characters are invalid and dangerous, due to the varying ways that implementations might parse and interpret those characters; a recipient of CR, LF, or NUL within a field value MUST either reject the message or replace each of those characters with SP before further processing or forwarding of that message. Field values containing other CTL characters are also invalid; however, recipients MAY retain such characters for the sake of robustness when they appear within a safe context (e.g., an application-specific quoted string that will not be processed by any downstream HTTP parser).

Unless I missed something in the RFC, this is not strictly prohibited on the sender side. So I don't think we can treat this as a bug. The thing is, there might be some testing applications that are used to test receivers by intentionally sending invalid headers. If we change this, we could potentially break those — which is something we want to avoid in a bug-fix release if we don’t have to.

That said, I do think we should introduce some validation by default as a feature (targeting the master branch). Not doing any validation is dangerous, can lead to unexpected results, and may even have security implications. (While the security issue would technically be on the recipient side in this case, it's better to prevent it on the sender side anyway.) To allow such testing tools to continue working, we should add a boolean context option that allows disabling this validation.