Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with the new Cookie management #11208

Closed
1 of 3 tasks
stefanozoffoli opened this issue Sep 18, 2017 · 4 comments
Closed
1 of 3 tasks

Issues with the new Cookie management #11208

stefanozoffoli opened this issue Sep 18, 2017 · 4 comments
Assignees
Milestone

Comments

@stefanozoffoli
Copy link

stefanozoffoli commented Sep 18, 2017

This is a (multiple allowed):

  • bug

  • enhancement

  • feature-discussion (RFC)

  • CakePHP Version: 3.5.2

  • Platform and Target: Apache/2.4.12 (Win32) OpenSSL/1.0.1l PHP/5.6.8

What you did

I tried to port my previous "remember me" cookie implementation (based on Cookie Component) to the new Cookie management.

My codebase is pretty simple:

$this->response = $this->response->withCookie(
    (new Cookie('urmc'))
        ->withValue([
            'user_id' => $user_id,
            'token' => $token,
        ])
        ->withExpiry(new DateTime('+1 month'))
        ->withHttpOnly(true)
);

return $this->redirect('/');

I had 3 problems:

Cookie with redirect

If I set the cookie before a redirect, it doesn't set any cookie.
I tried to debug ResponseEmitter.php: in the function emitHeaders() it seems to resets the value of the variable $cookies right after setting a header of type 'Location: ...'.
For debug purpose I tried to temporarily remove the redirect like this:

foreach ($response->getHeaders() as $name => $values) {
    if (strtolower($name) === 'set-cookie') {
        $cookies = array_merge($cookies, $values);
        continue;
    }
    if (strtolower($name) === 'location') {
        continue;
    }
    ...

and then $cookies correctly had the cookie i setted.

Cookie with value of type array

After I removed the redirect, it gave me another error:

setcookie() expects parameter 2 to be string, array given

This is because it calls Response->convertCookieToArray() that doesn't convert values of type array to string.
But I saw that Cookie() object can handle values of type array by using an internal variable $isExpanded... is this correct?
I expected that I could set a cookie with value of type array without the need to explicitly call json_encode()

Encrypted Cookie

I don't know if I'm using it incorrectly, but EncryptedCookieMiddleware only decodes my cookie and doesn't encode it.
This is my code (in routes.php):

Router::scope('/', function (RouteBuilder $routes) {
    ...

    $routes->registerMiddleware('csrf', new CsrfProtectionMiddleware());
    $routes->registerMiddleware('cookies', new EncryptedCookieMiddleware(['urmc'], Configure::read('Security.cookieKey')));

    $routes->applyMiddleware('csrf', 'cookies');

    ...

    $routes->fallbacks(DashedRoute::class);
});

Maybe I didn't understand really well how middleware works, but there is only a __invoke() function that calls both decodeCookies() in the request and encodeCookies() in the response...
But I set the cookie after that, right? So the cookie doesn't encode because the __invoke() function has already been called?

Thank you

@markstory
Copy link
Member

Cookies should be sent and encoded when responses contain Redirect headers. I'll try to reproduce this locally.

@markstory markstory self-assigned this Sep 18, 2017
@markstory markstory added this to the 3.5.3 milestone Sep 18, 2017
@markstory
Copy link
Member

I took a look at this and was able to reproduce the issue you were having. The dropped cookies on redirect were likely caused by the setcookie() expects parameter 2 to be string, array given warning you were seeing when the redirect was not present. This is something I'll get fixed.

I was not able to reproduce the missing cookie data when the EncryptedCookieMiddleware is being used. In my setup, I got an cookie with encrypted data in the response headers as expected.

but there is only a __invoke() function that calls both decodeCookies() in the request and encodeCookies() in the response...

Yes there is only one function in that middleware. It handles processing the incoming request, calling the 'next' middleware, and then encrypting cookies in the response generated by other middleware.

But I set the cookie after that, right? So the cookie doesn't encode because the __invoke() function has already been called?

If your cookie is being set in a controller action, then that would happen inside of $next() from the cookie middleware's point of view, so the cookies should be set already.

markstory added a commit that referenced this issue Sep 19, 2017
Add a new method to get a cookie value as a string. I considered
changing how `getValue()` works but thought that had bigger
compatibility implications. Instead I've modified the
interface/implementation. Normally I would shy away from changing
interfaces, but this is a very new class and the likelihood of there be
user-space implementations is quite low.

Refs #11208
@markstory
Copy link
Member

#11209 will solve the problems I was able to reproduce here.

@stefanozoffoli
Copy link
Author

stefanozoffoli commented Sep 19, 2017

Thank you Mark,

I applied your changes and It correctly fixes all the issues I had (for these i commentend directly in the pull request). 😄

For completeness I had the issue with EncryptedCookieMiddleware because I hadn't added that middleware in every scope, so it was my fault!

o0h pushed a commit to o0h/cakephp that referenced this issue Nov 16, 2017
Add a new method to get a cookie value as a string. I considered
changing how `getValue()` works but thought that had bigger
compatibility implications. Instead I've modified the
interface/implementation. Normally I would shy away from changing
interfaces, but this is a very new class and the likelihood of there be
user-space implementations is quite low.

Refs cakephp#11208
o0h pushed a commit to o0h/cakephp that referenced this issue Nov 16, 2017
o0h pushed a commit to o0h/cakephp that referenced this issue Dec 30, 2017
Add a new method to get a cookie value as a string. I considered
changing how `getValue()` works but thought that had bigger
compatibility implications. Instead I've modified the
interface/implementation. Normally I would shy away from changing
interfaces, but this is a very new class and the likelihood of there be
user-space implementations is quite low.

Refs cakephp#11208
o0h pushed a commit to o0h/cakephp that referenced this issue Dec 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants