Merge pull request #12198 from davidyell/deprecate-response-cookie-creation

Deprecate the Response being able to create new Cookies

src/Controller/Component/CookieComponent.php

@@ -15,6 +15,7 @@
 namespace Cake\Controller\Component;
 
 use Cake\Controller\Component;
+use Cake\Http\Cookie\Cookie;
 use Cake\Http\ServerRequestFactory;
 use Cake\I18n\Time;
 use Cake\Utility\CookieCryptTrait;
@@ -310,14 +311,18 @@ class CookieComponent extends Component
         $expires = new Time($config['expires']);
 
         $controller = $this->getController();
-        $controller->response = $controller->response->withCookie($name, [
-            'value' => $this->_encrypt($value, $config['encryption'], $config['key']),
-            'expire' => $expires->format('U'),
-            'path' => $config['path'],
-            'domain' => $config['domain'],
-            'secure' => (bool)$config['secure'],
-            'httpOnly' => (bool)$config['httpOnly']
-        ]);
+
+        $cookie = new Cookie(
+            $name,
+            $this->_encrypt($value, $config['encryption'], $config['key']),
+            $expires,
+            $config['path'],
+            $config['domain'],
+            (bool)$config['secure'],
+            (bool)$config['httpOnly']
+        );
+
+        $controller->response = $controller->response->withCookie($cookie);
     }
 
     /**
@@ -335,14 +340,17 @@ class CookieComponent extends Component
         $expires = new Time('now');
         $controller = $this->getController();
 
-        $controller->response = $controller->response->withCookie($name, [
-            'value' => '',
-            'expire' => $expires->format('U') - 42000,
-            'path' => $config['path'],
-            'domain' => $config['domain'],
-            'secure' => $config['secure'],
-            'httpOnly' => $config['httpOnly']
-        ]);
+        $cookie = new Cookie(
+            $name,
+            '',
+            $expires,
+            $config['path'],
+            $config['domain'],
+            (bool)$config['secure'],
+            (bool)$config['httpOnly']
+        );
+
+        $controller->response = $controller->response->withExpiredCookie($cookie);
     }
 
     /**

src/Controller/Component/CsrfComponent.php

@@ -16,6 +16,7 @@ namespace Cake\Controller\Component;
 
 use Cake\Controller\Component;
 use Cake\Event\Event;
+use Cake\Http\Cookie\Cookie;
 use Cake\Http\Exception\InvalidCsrfTokenException;
 use Cake\Http\Response;
 use Cake\Http\ServerRequest;
@@ -134,13 +135,18 @@ class CsrfComponent extends Component
         $value = hash('sha512', Security::randomBytes(16), false);
 
         $request = $request->withParam('_csrfToken', $value);
-        $response = $response->withCookie($this->_config['cookieName'], [
-            'value' => $value,
-            'expire' => $expiry->format('U'),
-            'path' => $request->getAttribute('webroot'),
-            'secure' => $this->_config['secure'],
-            'httpOnly' => $this->_config['httpOnly'],
-        ]);
+
+        $cookie = new Cookie(
+            $this->_config['cookieName'],
+            $value,
+            $expiry,
+            $request->getAttribute('webroot'),
+            '',
+            (bool)$this->_config['secure'],
+            (bool)$this->_config['httpOnly']
+        );
+
+        $response = $response->withCookie($cookie);
 
         return [$request, $response];
     }

src/Http/Cookie/Cookie.php

@@ -81,7 +81,7 @@ class Cookie implements CookieInterface
      *
      * @var string
      */
-    protected $path = '';
+    protected $path = '/';
 
     /**
      * Domain
@@ -124,7 +124,7 @@ class Cookie implements CookieInterface
         $name,
         $value = '',
         $expiresAt = null,
-        $path = '',
+        $path = '/',
         $domain = '',
         $secure = false,
         $httpOnly = false

src/Http/Middleware/CsrfProtectionMiddleware.php

@@ -14,6 +14,7 @@
  */
 namespace Cake\Http\Middleware;
 
+use Cake\Http\Cookie\Cookie;
 use Cake\Http\Exception\InvalidCsrfTokenException;
 use Cake\Http\Response;
 use Cake\Http\ServerRequest;
@@ -163,13 +164,17 @@ class CsrfProtectionMiddleware
     {
         $expiry = new Time($this->_config['expiry']);
 
-        return $response->withCookie($this->_config['cookieName'], [
-            'value' => $token,
-            'expire' => $expiry->format('U'),
-            'path' => $request->getAttribute('webroot'),
-            'secure' => $this->_config['secure'],
-            'httpOnly' => $this->_config['httpOnly'],
-        ]);
+        $cookie = new Cookie(
+            $this->_config['cookieName'],
+            $token,
+            $expiry,
+            $request->getAttribute('webroot'),
+            '',
+            (bool)$this->_config['secure'],
+            (bool)$this->_config['httpOnly']
+        );
+
+        return $response->withCookie($cookie);
     }
 
     /**

src/Http/Response.php

@@ -2175,7 +2175,7 @@ class Response implements ResponseInterface
     /**
      * Create a new response with a cookie set.
      *
-     * ### Options
+     * ### Data
      *
      * - `value`: Value of the cookie
      * - `expire`: Time the cookie expires in
@@ -2206,6 +2206,11 @@ class Response implements ResponseInterface
         if ($name instanceof Cookie) {
             $cookie = $name;
         } else {
+            deprecationWarning(
+                get_called_class() . '::withCookie(string $name, array $data) is deprecated. ' .
+                'Pass an instance of \Cake\Http\Cookie\Cookie instead.'
+            );
+
             if (!is_array($data)) {
                 $data = ['value' => $data];
             }
@@ -2267,6 +2272,11 @@ class Response implements ResponseInterface
         if ($name instanceof CookieInterface) {
             $cookie = $name->withExpired();
         } else {
+            deprecationWarning(
+                get_called_class() . '::withExpiredCookie(string $name, array $data) is deprecated. ' .
+                'Pass an instance of \Cake\Http\Cookie\Cookie instead.'
+            );
+
             $options += [
                 'path' => '/',
                 'domain' => '',

tests/TestCase/Controller/Component/CookieComponentTest.php

@@ -439,7 +439,7 @@ class CookieComponentTest extends TestCase
         $expected = [
             'name' => 'Testing',
             'value' => '',
-            'expire' => (new Time('now'))->format('U') - 42000,
+            'expire' => '1',
             'path' => '/',
             'domain' => '',
             'secure' => false,

tests/TestCase/Http/Cookie/CookieCollectionTest.php

@@ -483,7 +483,7 @@ class CookieCollectionTest extends TestCase
 
         $cookie = $cookies->get('name');
         $this->assertSame('val', $cookie->getValue());
-        $this->assertSame('', $cookie->getPath(), 'No path on request cookies');
+        $this->assertSame('/', $cookie->getPath());
         $this->assertSame('', $cookie->getDomain(), 'No domain on request cookies');
     }
 }

tests/TestCase/Http/Cookie/CookieTest.php

@@ -72,7 +72,7 @@ class CookieTest extends TestCase
     {
         $cookie = new Cookie('cakephp', 'cakephp-rocks');
         $result = $cookie->toHeaderValue();
-        $this->assertEquals('cakephp=cakephp-rocks', $result);
+        $this->assertEquals('cakephp=cakephp-rocks; path=/', $result);
 
         $date = Chronos::createFromFormat('m/d/Y h:m:s', '12/1/2027 12:00:00');
 
@@ -83,7 +83,7 @@ class CookieTest extends TestCase
             ->withSecure(true);
         $result = $cookie->toHeaderValue();
 
-        $expected = 'cakephp=cakephp-rocks; expires=Tue, 01-Dec-2026 12:00:00 GMT; domain=cakephp.org; secure; httponly';
+        $expected = 'cakephp=cakephp-rocks; expires=Tue, 01-Dec-2026 12:00:00 GMT; path=/; domain=cakephp.org; secure; httponly';
         $this->assertEquals($expected, $result);
     }
 
@@ -213,6 +213,17 @@ class CookieTest extends TestCase
     }
 
     /**
+     * Test default path in cookies
+     *
+     * @return void
+     */
+    public function testDefaultPath()
+    {
+        $cookie = new Cookie('cakephp', 'cakephp-rocks');
+        $this->assertContains('path=/', $cookie->toHeaderValue());
+    }
+
+    /**
      * Test setting httponly in cookies
      *
      * @return void
@@ -550,10 +561,10 @@ class CookieTest extends TestCase
     public function testGetId()
     {
         $cookie = new Cookie('cakephp', 'cakephp-rocks');
-        $this->assertEquals('cakephp;;', $cookie->getId());
+        $this->assertEquals('cakephp;;/', $cookie->getId());
 
         $cookie = new Cookie('CAKEPHP', 'cakephp-rocks');
-        $this->assertEquals('cakephp;;', $cookie->getId());
+        $this->assertEquals('cakephp;;/', $cookie->getId());
 
         $cookie = new Cookie('test', 'val', null, '/path', 'example.com');
         $this->assertEquals('test;example.com;/path', $cookie->getId());

tests/TestCase/Http/Middleware/EncryptedCookieMiddlewareTest.php

@@ -14,6 +14,7 @@
  */
 namespace Cake\Test\TestCase\Http\Middleware;
 
+use Cake\Http\Cookie\Cookie;
 use Cake\Http\Cookie\CookieCollection;
 use Cake\Http\Middleware\EncryptedCookieMiddleware;
 use Cake\Http\Response;
@@ -110,9 +111,9 @@ class EncryptedCookieMiddlewareTest extends TestCase
         $request = new ServerRequest(['url' => '/cookies/nom']);
         $response = new Response();
         $next = function ($req, $res) {
-            return $res->withCookie('secret', 'be quiet')
-                ->withCookie('plain', 'in clear')
-                ->withCookie('ninja', 'shuriken');
+            return $res->withCookie(new Cookie('secret', 'be quiet'))
+                ->withCookie(new Cookie('plain', 'in clear'))
+                ->withCookie(new Cookie('ninja', 'shuriken'));
         };
         $middleware = $this->middleware;
         $response = $middleware($request, $response, $next);

tests/TestCase/Http/ResponseEmitterTest.php

@@ -15,6 +15,7 @@
 namespace Cake\Test\TestCase;
 
 use Cake\Http\CallbackStream;
+use Cake\Http\Cookie\Cookie;
 use Cake\Http\Response;
 use Cake\Http\ResponseEmitter;
 use Cake\TestSuite\TestCase;
@@ -110,7 +111,7 @@ class ResponseEmitterTest extends TestCase
     public function testEmitResponseArrayCookies()
     {
         $response = (new Response())
-            ->withCookie('simple', ['value' => 'val', 'secure' => true])
+            ->withCookie(new Cookie('simple', 'val', null, '/', '', true))
             ->withAddedHeader('Set-Cookie', 'google=not=nice;Path=/accounts; HttpOnly')
             ->withHeader('Content-Type', 'text/plain');
         $response->getBody()->write('ok');

tests/TestCase/Http/ResponseTest.php

@@ -16,12 +16,14 @@ namespace Cake\Test\TestCase\Http;
 
 include_once CORE_TEST_CASES . DS . 'Http' . DS . 'server_mocks.php';
 
+use Cake\Chronos\Chronos;
 use Cake\Http\Cookie\Cookie;
 use Cake\Http\Cookie\CookieCollection;
 use Cake\Http\CorsBuilder;
 use Cake\Http\Exception\NotFoundException;
 use Cake\Http\Response;
 use Cake\Http\ServerRequest;
+use Cake\I18n\FrozenTime;
 use Cake\TestSuite\TestCase;
 use Zend\Diactoros\Stream;
 
@@ -1592,7 +1594,7 @@ class ResponseTest extends TestCase
     public function testWithCookieEmpty()
     {
         $response = new Response();
-        $new = $response->withCookie('testing');
+        $new = $response->withCookie(new Cookie('testing'));
         $this->assertNull($response->getCookie('testing'), 'withCookie does not mutate');
 
         $expected = [
@@ -1615,46 +1617,56 @@ class ResponseTest extends TestCase
     public function testWithCookieScalar()
     {
         $response = new Response();
-        $new = $response->withCookie('testing', 'abc123');
+        $new = $response->withCookie(new Cookie('testing', 'abc123'));
         $this->assertNull($response->getCookie('testing'), 'withCookie does not mutate');
         $this->assertEquals('abc123', $new->getCookie('testing')['value']);
 
-        $new = $response->withCookie('testing', 99);
+        $new = $response->withCookie(new Cookie('testing', 99));
         $this->assertEquals(99, $new->getCookie('testing')['value']);
 
-        $new = $response->withCookie('testing', false);
+        $new = $response->withCookie(new Cookie('testing', false));
         $this->assertFalse($new->getCookie('testing')['value']);
 
-        $new = $response->withCookie('testing', true);
+        $new = $response->withCookie(new Cookie('testing', true));
         $this->assertTrue($new->getCookie('testing')['value']);
     }
 
     /**
-     * Test withCookie() and array data.
+     * Test withCookie() and duplicate data
      *
      * @return void
+     * @throws \Exception
      */
-    public function testWithCookieArray()
+    public function testWithDuplicateCookie()
     {
+        $expiry = new \DateTimeImmutable('+24 hours');
+
         $response = new Response();
-        $cookie = [
-            'name' => 'ignored key',
-            'value' => '[a,b,c]',
-            'expire' => 1000,
-            'path' => '/test',
-            'secure' => true
-        ];
-        $new = $response->withCookie('testing', $cookie);
+        $cookie = new Cookie(
+            'testing',
+            '[a,b,c]',
+            $expiry,
+            '/test',
+            '',
+            true
+        );
+
+        $new = $response->withCookie($cookie);
         $this->assertNull($response->getCookie('testing'), 'withCookie does not mutate');
+
         $expected = [
             'name' => 'testing',
             'value' => '[a,b,c]',
-            'expire' => 1000,
+            'expire' => $expiry,
             'path' => '/test',
             'domain' => '',
             'secure' => true,
             'httpOnly' => false
         ];
+
+        // Match the date time formatting to Response::convertCookieToArray
+        $expected['expire'] = $expiry->format('U');
+
         $this->assertEquals($expected, $new->getCookie('testing'));
     }
 
@@ -1677,15 +1689,18 @@ class ResponseTest extends TestCase
     public function testWithExpiredCookieScalar()
     {
         $response = new Response();
-        $response = $response->withCookie('testing', 'abc123');
+        $response = $response->withCookie(new Cookie('testing', 'abc123'));
         $this->assertEquals('abc123', $response->getCookie('testing')['value']);
 
-        $new = $response->withExpiredCookie('testing');
+        $new = $response->withExpiredCookie(new Cookie('testing'));
 
         $this->assertNull($response->getCookie('testing')['expire']);
-        $this->assertEquals('1', $new->getCookie('testing')['expire']);
+        $this->assertLessThan(FrozenTime::createFromTimestamp(1), $new->getCookie('testing')['expire']);
     }
 
+    /**
+     * @throws \Exception If DateImmutable emits an error.
+     */
     public function testWithExpiredCookieOptions()
     {
         $options = [
@@ -1695,18 +1710,30 @@ class ResponseTest extends TestCase
             'path' => '/custompath/',
             'secure' => true,
             'httpOnly' => true,
-            'expire' => (string)strtotime('+14 days'),
+            'expire' => new \DateTimeImmutable('+14 days'),
         ];
 
+        $cookie = new Cookie(
+            $options['name'],
+            $options['value'],
+            $options['expire'],
+            $options['path'],
+            $options['domain'],
+            $options['secure'],
+            $options['httpOnly']
+        );
+
         $response = new Response();
-        $response = $response->withCookie('testing', $options);
+        $response = $response->withCookie($cookie);
+
+        // Change the timestamp format to match the Response::convertCookieToArray
+        $options['expire'] = $options['expire']->format('U');
         $this->assertEquals($options, $response->getCookie('testing'));
 
-        $new = $response->withExpiredCookie('testing', $options);
+        $expiredCookie = $response->withExpiredCookie($cookie);
 
         $this->assertEquals($options['expire'], $response->getCookie('testing')['expire']);
-        $this->assertEquals('1', $new->getCookie('testing')['expire']);
-        $this->assertEquals('', $new->getCookie('testing')['value']);
+        $this->assertLessThan(Chronos::createFromTimestamp(1), $expiredCookie->getCookie('testing')['expire']);
     }
 
     public function testWithExpiredCookieObject()
@@ -1730,8 +1757,8 @@ class ResponseTest extends TestCase
     public function testGetCookies()
     {
         $response = new Response();
-        $new = $response->withCookie('testing', 'a')
-            ->withCookie('test2', ['value' => 'b', 'path' => '/test', 'secure' => true]);
+        $new = $response->withCookie(new Cookie('testing', 'a'))
+            ->withCookie(new Cookie('test2', 'b', null, '/test', '', true));
         $expected = [
             'testing' => [
                 'name' => 'testing',
@@ -1773,7 +1800,7 @@ class ResponseTest extends TestCase
                 'name' => 'urmc',
                 'value' => '{"user_id":1,"token":"abc123"}',
                 'expire' => null,
-                'path' => '',
+                'path' => '/',
                 'domain' => '',
                 'secure' => false,
                 'httpOnly' => true
@@ -1790,8 +1817,8 @@ class ResponseTest extends TestCase
     public function testGetCookieCollection()
     {
         $response = new Response();
-        $new = $response->withCookie('testing', 'a')
-            ->withCookie('test2', ['value' => 'b', 'path' => '/test', 'secure' => true]);
+        $new = $response->withCookie(new Cookie('testing', 'a'))
+            ->withCookie(new Cookie('test2', 'b', null, '/test', '', true));
         $cookies = $response->getCookieCollection();
         $this->assertInstanceOf(CookieCollection::class, $cookies);
         $this->assertCount(0, $cookies, 'Original response not mutated');

tests/test_app/TestApp/Controller/PostsController.php

@@ -15,6 +15,7 @@
 namespace TestApp\Controller;
 
 use Cake\Event\Event;
+use Cake\Http\Cookie\Cookie;
 
 /**
  * PostsController class
@@ -55,7 +56,7 @@ class PostsController extends AppController
     public function index($layout = 'default')
     {
         $this->Flash->error('An error message');
-        $this->response = $this->response->withCookie('remember_me', 1);
+        $this->response = $this->response->withCookie(new Cookie('remember_me', 1));
         $this->set('test', 'value');
         $this->viewBuilder()->setLayout($layout);
     }