Replace header when sending first of each header

When we call `header()` to send the headers in the Response, set the
`replace` parameter to true for the first of that header name and then
set it to false.

Don't do this if the header is Set-Cookie so we don't break sessions.

Fixes #2282
Fixes #2246

Slim/App.php

@@ -447,8 +447,10 @@ public function respond(ResponseInterface $response): ResponseInterface
         if (!headers_sent()) {
             // Headers
             foreach ($response->getHeaders() as $name => $values) {
+                $first = stripos($name, 'Set-Cookie') === 0 ? false : true;
                 foreach ($values as $value) {
-                    header(sprintf('%s: %s', $name, $value), false);
+                    header(sprintf('%s: %s', $name, $value), $first);
+                    $first = false;
                 }
             }
 
@@ -461,7 +463,7 @@ public function respond(ResponseInterface $response): ResponseInterface
                 $response->getProtocolVersion(),
                 $response->getStatusCode(),
                 $response->getReasonPhrase()
-            ));
+            ), true, $response->getStatusCode());
         }
 
         // Body

composer.json

@@ -48,6 +48,11 @@
             "Slim\\": "Slim"
         }
     },
+    "autoload-dev": {
+        "files": [
+          "tests/Assets/HeaderFunctions.php"
+        ]
+    },
     "scripts": {
         "test": [
             "@phpunit",

tests/AppTest.php

@@ -18,6 +18,7 @@
 use Slim\Error\Renderers\HtmlErrorRenderer;
 use Slim\Exception\HttpMethodNotAllowedException;
 use Slim\Handlers\Strategies\RequestResponseArgs;
+use Slim\HeaderStackTestAsset;
 use Slim\Http\Body;
 use Slim\Http\Environment;
 use Slim\Http\Headers;
@@ -27,8 +28,28 @@
 use Slim\Router;
 use Slim\Tests\Mocks\MockAction;
 
+/**
+ * Emit a header, without creating actual output artifacts
+ *
+ * @param string $value
+ */
+function header($value, $replace = true)
+{
+    \Slim\header($value, $replace);
+}
+
 class AppTest extends TestCase
 {
+    public function setUp()
+    {
+        HeaderStackTestAsset::reset();
+    }
+
+    public function tearDown()
+    {
+        HeaderStackTestAsset::reset();
+    }
+
     public static function setupBeforeClass()
     {
         // ini_set('log_errors', 0);
@@ -1569,6 +1590,78 @@ public function testResponseWithStreamReadYieldingLessBytesThanAsked()
         $this->expectOutputString(str_repeat('.', Mocks\SmallChunksStream::SIZE));
     }
 
+    public function testResponseReplacesPreviouslySetHeaders()
+    {
+        $app = new App();
+        $app->get('/foo', function ($req, $res) {
+            return $res
+                ->withHeader('X-Foo', 'baz1')
+                ->withAddedHeader('X-Foo', 'baz2')
+                ;
+        });
+
+        // Prepare request and response objects
+        $serverParams = Environment::mock([
+            'SCRIPT_NAME' => '/index.php',
+            'REQUEST_URI' => '/foo',
+            'REQUEST_METHOD' => 'GET',
+        ]);
+        $uri = Uri::createFromGlobals($serverParams);
+        $headers = Headers::createFromGlobals($serverParams);
+        $cookies = [];
+        $body = new Body(fopen('php://temp', 'r+'));
+        $req = new Request('GET', $uri, $headers, $cookies, $serverParams, $body);
+        $res = new Response();
+
+        // Invoke app
+        $resOut = $app($req, $res);
+        $app->respond($resOut);
+
+        $expectedStack = [
+            ['header' => 'X-Foo: baz1', 'replace' => true, 'status_code' => null],
+            ['header' => 'X-Foo: baz2', 'replace' => false, 'status_code' => null],
+            ['header' => 'HTTP/1.1 200 OK', 'replace' => true, 'status_code' => 200],
+        ];
+
+        $this->assertSame($expectedStack, HeaderStackTestAsset::stack());
+    }
+
+    public function testResponseDoesNotReplacePreviouslySetSetCookieHeaders()
+    {
+        $app = new App();
+        $app->get('/foo', function ($req, $res) {
+            return $res
+                ->withHeader('Set-Cookie', 'foo=bar')
+                ->withAddedHeader('Set-Cookie', 'bar=baz')
+                ;
+        });
+
+        // Prepare request and response objects
+        $serverParams = Environment::mock([
+            'SCRIPT_NAME' => '/index.php',
+            'REQUEST_URI' => '/foo',
+            'REQUEST_METHOD' => 'GET',
+        ]);
+        $uri = Uri::createFromGlobals($serverParams);
+        $headers = Headers::createFromGlobals($serverParams);
+        $cookies = [];
+        $body = new Body(fopen('php://temp', 'r+'));
+        $req = new Request('GET', $uri, $headers, $cookies, $serverParams, $body);
+        $res = new Response();
+
+        // Invoke app
+        $resOut = $app($req, $res);
+        $app->respond($resOut);
+
+        $expectedStack = [
+            ['header' => 'Set-Cookie: foo=bar', 'replace' => false, 'status_code' => null],
+            ['header' => 'Set-Cookie: bar=baz', 'replace' => false, 'status_code' => null],
+            ['header' => 'HTTP/1.1 200 OK', 'replace' => true, 'status_code' => 200],
+        ];
+
+        $this->assertSame($expectedStack, HeaderStackTestAsset::stack());
+    }
+
     public function testFinalize()
     {
         $method = new \ReflectionMethod('Slim\App', 'finalize');

tests/Assets/HeaderFunctions.php

@@ -0,0 +1,114 @@
+<?php
+/**
+ * This is a direct copy of zend-diactoros/test/TestAsset/Functions.php and is used to override
+ * header() and headers_sent() so we can test that they do the right thing.
+ *
+ * We put these into the Slim namespace, so that Slim\App will use these versions of header() and
+ * headers_sent() when we test its output.
+ */
+namespace Slim;
+
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * This file exists to allow overriding the various output-related functions
+ * in order to test what happens during the `Server::listen()` cycle.
+ *
+ * These functions include:
+ *
+ * - headers_sent(): we want to always return false so that headers will be
+ *   emitted, and we can test to see their values.
+ * - header(): we want to aggregate calls to this function.
+ *
+ * The HeaderStack class then aggregates that information for us, and the test
+ * harness resets the values pre and post test.
+ *
+ * @see       http://github.com/zendframework/zend-diactoros for the canonical source repository
+ * @copyright Copyright (c) 2015-2016 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license   https://github.com/zendframework/zend-diactoros/blob/master/LICENSE.md New BSD License
+ */
+
+/**
+ * Store output artifacts
+ */
+class HeaderStackTestAsset
+{
+    /**
+     * @var string[][]
+     */
+    private static $data = [];
+
+    /**
+     * Reset state
+     */
+    public static function reset()
+    {
+        self::$data = [];
+    }
+
+    /**
+     * Push a header on the stack
+     *
+     * @param string[] $header
+     */
+    public static function push(array $header)
+    {
+        self::$data[] = $header;
+    }
+
+    /**
+     * Return the current header stack
+     *
+     * @return string[][]
+     */
+    public static function stack()
+    {
+        return self::$data;
+    }
+
+    /**
+     * Verify if there's a header line on the stack
+     *
+     * @param string $header
+     *
+     * @return bool
+     */
+    public static function has($header)
+    {
+        foreach (self::$data as $item) {
+            if ($item['header'] === $header) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+}
+
+/**
+ * Have headers been sent?
+ *
+ * @return false
+ */
+function headers_sent()
+{
+    return false;
+}
+
+/**
+ * Emit a header, without creating actual output artifacts
+ *
+ * @param string   $string
+ * @param bool     $replace
+ * @param int|null $statusCode
+ */
+function header($string, $replace = true, $statusCode = null)
+{
+    HeaderStackTestAsset::push(
+        [
+            'header'      => $string,
+            'replace'     => $replace,
+            'status_code' => $statusCode,
+        ]
+    );
+}