Environment variables prefixed with CONTENT_ may overwrite HTTP-Headers provided by the client

[bcremer]

Bug Report

Q	A
Version(s)	all

Summary

Environment variables prefixed with CONTENT_ may overwrite HTTP-Headers provided by the client

Current behavior

This may be entirely expected behavior but I have not found a spec or warning in the documentation.
It's a least surprising to find unrelated environment variables as request header and potentially a security problem when comparing the env-variable against the header during authentication.

Environment variables prefixed with CONTENT_ will be populated as http-headers:

I stumbled about this behavior in a project with the following .env-file:

CONTENT_API_PASSWORD=password-for-content-api

The .env file is loaded into the $_SERVER superglobal via Dotenv\Dotenv::createImmutable().

The request object is created via \Laminas\Diactoros\ServerRequestFactory::fromGlobals.
The request object now contains content-api-password as request-header $request->getHeader('content-api-password').

Environment variables prefixed with CONTENT_ max overwrite client provided HTTP-Headers:

If the same header name is provided by an http-client the $_SERVER superglobal contains a HTTP_CONTENT_API_PASSWORD key.

In the current implementation the order is relevant. Whatever key is defined later in $_SERVER will be used to populate the HTTP-Header.

How to reproduce

        $server = [
            'HTTP_CONTENT_API_PASSWORD' => 'password_from_header',
            'CONTENT_API_PASSWORD' => 'password_from_env',
        ];

        $request = ServerRequestFactory::fromGlobals($server);

        $this->assertSame('password_from_env', $request->getHeader('content-api-password')[0]);


        $server = [
            'CONTENT_API_PASSWORD' => 'password_from_env',
            'HTTP_CONTENT_API_PASSWORD' => 'password_from_header',
        ];

        $request = ServerRequestFactory::fromGlobals($server);

        $this->assertSame('password_from_header', $request->getHeader('content-api-password')[0]);

Expected behavior

        $server = [
            'HTTP_CONTENT_API_PASSWORD' => 'password_from_header',
            'CONTENT_API_PASSWORD' => 'password_from_env',
        ];

        $request = ServerRequestFactory::fromGlobals($server);

        $this->assertSame('password_from_header', $request->getHeader('content-api-password')[0]);


        $server = [
            'CONTENT_API_PASSWORD' => 'password_from_env',
            'HTTP_CONTENT_API_PASSWORD' => 'password_from_header',
        ];

        $request = ServerRequestFactory::fromGlobals($server);

        $this->assertSame('password_from_header', $request->getHeader('content-api-password')[0]);

[bcremer]

If my understanding of https://tools.ietf.org/html/rfc3875#section-4.1.2 is correct only CONTENT_LENGTH and CONTENT_TYPE should be copied from $_SERVER to the headers in https://github.com/laminas/laminas-diactoros/blob/2.5.1/src/functions/marshal_headers_from_sapi.php#L54

Edit:
And maybe CONTENT_MD5, see: https://github.com/ralouphie/getallheaders/blob/develop/src/getallheaders.php#L14

[weierophinney]

I agree we should make these changes.

Primary issue is... the MarshalHeadersFromSapiTest has a few test cases that fail if I make this change, as it was explicitly testing that things like CONTENT_FOO and CONTENT_BAZ in the $_SERVER array were being converted to headers. As such, this is potentially a BC break, as we'd be changing existing behavior.

I'm going to ask the TSC if the change can be done in next minor (2.6.0) as I'd call the current behavior incorrect, or if we absolutely need to do a new major (3.0.0).

[weierophinney]

One suggestion by @boesing is to potentially add a feature flag that is toggled via an ENV variable, and have it default to existing behavior when not present. This would allow you to opt-in to the new behavior, while keeping it backwards compatible by default. Next major version would remove the bad behavior of the current version.

I'll start work on that shortly.