php-pm / php-pm-httpkernel

HttpKernel adapter for use of Symfony and Laravel frameworks with PHP-PM
MIT License
246 stars 72 forks source link

Duplicate `Set-Cookie` headers on Laravel #103

Closed adrianfalleiro closed 6 years ago

adrianfalleiro commented 6 years ago

When using this HttpKernel adapter with the Laravel bootstrap I am seeing duplicate Set-Cookie headers being sent.

screen shot 2018-05-02 at 1 05 01 pm

Is this supposed to happen? It doesn't seem to break anything in my testing so far though.

From what I can see, this is occurring because of line 276 of PHPPM\Bridges\HttpKernel in which an array of cookies is being merged with existing Set-Cookie headers on the SymfonyResponse object:

$headers['Set-Cookie'] = array_merge((array)$headers['Set-Cookie'], $cookies);

Does this need to happen? You're already merging $nativeHeaders with all of the SymfonyResponse object headers on line 247:

$headers = array_merge($nativeHeaders, $syResponse->headers->allPreserveCase());

I've created a demo project to show the issue: https://github.com/adrianfalleiro/laravel-5.6-ppm-example

All you need to do is composer install and then ./vendor/bin/ppm start. If you inspect the headers on the home page you should be able to see duplicate Set-Cookie headers sent.

Also for context I am running this on macOS 10.12.6 with PHP 7.2.4 installed (via Homebrew).

dzubchik commented 6 years ago

And, there must be array_merge_recursive function. So I really don't get why we should merge cookies twice.

Index: Bridges/HttpKernel.php
<+>UTF-8
===================================================================
--- Bridges/HttpKernel.php  (revision 086def550ccf72aee48bd62107cb2aa343cfebf6)
+++ Bridges/HttpKernel.php  (revision 0329324c219a8d892bed2a09fc2e481382e38e6c)
@@ -104,7 +104,7 @@
         if ($this->application instanceof TerminableInterface) {
             $this->application->terminate($syRequest, $syResponse);
         }
-        
+
         if ($this->application instanceof Kernel) {
             $this->application->terminate($syRequest, $syResponse);
         }
@@ -244,34 +244,10 @@
         // operates on a clean header.
         header_remove();

-        $headers = array_merge($nativeHeaders, $syResponse->headers->allPreserveCase());
+        $syHeaders = $syResponse->headers->allPreserveCase();
+        $headers = array_merge_recursive($nativeHeaders, $syHeaders);
         $cookies = [];

-        /** @var Cookie $cookie */
-        foreach ($syResponse->headers->getCookies() as $cookie) {
-            $cookieHeader = sprintf('%s=%s', $cookie->getName(), $cookie->getValue());
-
-            if ($cookie->getPath()) {
-                $cookieHeader .= '; Path=' . $cookie->getPath();
-            }
-            if ($cookie->getDomain()) {
-                $cookieHeader .= '; Domain=' . $cookie->getDomain();
-            }
-
-            if ($cookie->getExpiresTime()) {
-                $cookieHeader .= '; Expires=' . gmdate('D, d-M-Y H:i:s', $cookie->getExpiresTime()). ' GMT';
-            }
-
-            if ($cookie->isSecure()) {
-                $cookieHeader .= '; Secure';
-            }
-            if ($cookie->isHttpOnly()) {
-                $cookieHeader .= '; HttpOnly';
-            }
-
-            $cookies[] = $cookieHeader;
-        }
-
         if (isset($headers['Set-Cookie'])) {
             $headers['Set-Cookie'] = array_merge((array)$headers['Set-Cookie'], $cookies);
         } else {

After this everything works as expected.

marcj commented 6 years ago

@dzubchik because you completely got rid of all cookies set by Symfony Response instance. AFAIK allPreserveCase() does not build cookie header entries of header->cookies.

marcj commented 6 years ago

oh, well, I just saw, Symfony\Component\HttpFoundation\Cookie has __toString which does it already. :+1:

dzubchik commented 6 years ago

Before this session cookie was not installed.

adrianfalleiro commented 6 years ago

@dzubchik When I tested your code block above on a Laravel project authentication fails. I cannot figure out why, but it looks to be failing because the XSRF tokens in the POST body and session do not match.

Instead the below seemed to work for me

 protected function mapResponse(SymfonyResponse $syResponse, $stdout='')
 {
    ...
-  $headers = array_merge($nativeHeaders, $syResponse->headers->allPreserveCase());
+  $headers = array_merge($nativeHeaders, $syResponse->headers->allPreserveCaseWithoutCookies());
    ...    
 }

And then iterating over $syResponse->headers->getCookies() and adding them to the PSR-7 response as it currently does.

I'm not really sure why this works...as allPreserveCase() does exactly the same thing.

mathieudz commented 6 years ago

See also #112 for fixing the session cookie. That PR also got rid of duplicate cookies for me.

mathieudz commented 6 years ago

This issue should be retested now that #112 has been merged.

adrianfalleiro commented 6 years ago

Thanks, I’ll retest my original code

andig commented 6 years ago

Please reopen if issue persists.