SetCookie::expire does not work: missing Domain part

[djozsef]

SetCookie::expire() does not work for response cookies because browsers expect a "Domain" part in the Set-Cookie header when expiring a cookie.

I have tested it with Firefox 49.0.1 and Chrome 53.0.2785.143 and both behave in the same (standard?) way: the minimum requirement for expiring and flushing a cookie is to have the cookie name, the "Expires" part in the past AND also a "Domain" part in the Set-Cookie header.

I could not really make a patch for this, since in response cookies no domain information is present. Any suggestion how to fix it in a canonical way?

[vincentsys]

Thank you @djozsef for pointing out the cause ot the bug. Related to #22. Somebody looking into this?

[djozsef]

The reason I did not make a patch right away is I wanted to trigger conversation on this. The main problem is, we have to include the domain information in the Set-Cookie header to get the "expiry happen" in the browser. I have not found any traces of automatic domain information retrieval in the response classes so we have to get the user to manually provide it to the SetCookie::expire method in a parameter or set it with ::withDomain().

So I see 2 solutions:
A) we add a required $domain parameter to the ::expire() method's signature and call ::withDomain() imlicitly
B) we warn the user in the documentation he needs to set the domain via ::withDomain() preliminary.

Though I see the option A) more compact I do not like it very much since it kinda breaks the PSR-7 Response concept. On the other hand, option B) is not a real solution, rather a lame workaround correcting the fault of the PSR-7 design.

What do you think? @vincentsys?

If we can live with option A) I'll make the patch for that.

[vincentsys]

Could you please elaborate on this:

I do not like it very much since it kinda breaks the PSR-7 Response concept.

adding a required parameter seems to be the way to go at first viewing.

[simensen]

Would it be possible to share the code you are using to try and make this work? I haven't had a chance to dive into it too closely but it seems to me that the withDomain call could be made on the SetCookie prior to calling expire and that would solve the problem?

$setCookie = $setCookie->withDomain($domain)->expire();

Unless you are talking about FigResponseCookies::expire which I believe would have the problem you are describing.

[djozsef]

@vincentsys I meant that according to PSR-7 the response message does not deal with the host that the subject message will be pushed out through. That is why there is no interface declaration for getHost or so. I think mixing in the host (that is a property of a Request type) here would go against this separation of 'topics' promoted by PSR-7. BUT there is no other way because we need the host information so I agree with you to ask for it via a method parameter.

[vincentsys]

@djozsef Thanks for explaining!. Agreed!
@simensen indeed I am using the FigResponseCookies class as facade (FigResponseCookies::expire).

It looks like the
$setCookie = $setCookie->withDomain($domain)->expire();
approach could/should work. Still, the facades need the missing param (or maybe config/param?).

[simensen]

For this fix, I'd recommend something like this. Happy to hear your thoughts on it:

    /**
     * @param ResponseInterface $response
     * @param string $cookieName
     * @param string $domain
     *
     * @return ResponseInterface
     */
    public static function expire(ResponseInterface $response, $cookieName, $domain = null)
    {
        $setCookie = SetCookie::createExpired($cookieName);

        if ($domain) {
            $setCookie = $setCookie->withDomain($domain);
        }

        return static::set($response, $setCookie);
    }

Since it is a static facade this will hopefully be somewhat BC as the $domain parameter will allow existing calls to FigRequestCookies::expire() without a $domain parameter to work.

Thoughts?

[djozsef]

I like your idea @simensen, it is exactly what was on my mind. Additionally I had a thought what if we check if the $domain parameter implements \Psr\Http\Message\RequestInterface. Tn this case the user could just pass the request object and we could grab the domain with Psr\Http\Message\RequestInterface::getHost(). Something like this:

use Psr\Http\Message\RequestInterface;

...

 /**
     * @param ResponseInterface $response
     * @param string $cookieName
     * @param string|RequestInterface $domain Domain as string or a request object that
     * implements RequestInterface
     *
     * @return ResponseInterface
     */
    public static function expire(ResponseInterface $response, $cookieName, $domain = null)
    {
        $setCookie = SetCookie::createExpired($cookieName);

        if ($domain instanceof RequestInterface) { 
            $setCookie =  $setCookie->withDomain($domain->getHost());
        } elseif ($domain) {
            $setCookie = $setCookie->withDomain($domain);
        }

        return static::set($response, $setCookie);
    }

Am I complicating it too much?

[vincentsys]

@simensen like it!
@djozsef making use of getHost() is a nice idea. But you are relying on a method which needs to be implemented correctly. Accorting to PSR-7: HTTP message interfaces:

In requests, the Host header typically mirrors the host component of the URI, as well as the host used when establishing the TCP connection. However, the HTTP specification allows the Host header to differ from each of the two.

During construction, implementations MUST attempt to set the Host header from a provided URI if no Host header is provided.

[simensen]

$domainOrRequest seems like it might be too much magic. Especially given UriInterface's getHost may return an empty string, I'm not sure what the best way would be to try and do this extra magic. For now, I'd just as soon leave it out.

Would either of you like to create a PR and tests for this change?

[djozsef]

I agree @simensen, too much magic at the moment. I vote for your original code.

Regarding the PR: this code is your baby but if it helps I can make a PR for that.

[simensen]

@djozsef if you are up for it, please go ahead. it is more likely to get done if someone else does it at this point. :) i'm sure i'd get around to it eventually but if either of you need it right now, the best way to make that happen is to implement it so i can merge & tag. :)

[vincentsys]

Bad news(?). I did further testing and it seems the problem is not the domain. Instead it is the withPath setting as described here #22.

Adding the domain makes no difference for me. As soon as I remove the path, I am able to modify and expire the cookie.

[vincentsys]

Ok, another test round. It seems if you set the cookie withDomain you need withDomain to modify/expire it, too! Setting it withPath also requires withPath modifying/expireing it!

[simensen]

Sigh.

Somehow when #4 came through I knew that this was going to be trouble but I couldn't quite put my finger on it. :-/ In order to be expired, cookies need to be defined exactly like they previously were with the exception of the value and the expiration time. So you cannot simply "delete a cookie" knowing just the name.

It would be nice to get @franzliedke feedback on this as hopefully they've used it by now and have overcome some of these issues?

At this point I'm tempted to suggest we try to find a way to rewrite this method entirely or drop it completely.

[franzliedke]

Hmm, sorry about that. Bad to read when you get back from a holiday. ;)

So if I understand this correctly, only the static methods (SetCookie::createExpired() and FigResponseCookies::expire()) don't make any sense, because you'd need to set the domain first.

So, to expire a cookie that was previously created, you would probably write something like the following:

$expireCookie = $this->getCookieTheWayItWasCreated()->expire();

//... and now attach that to the response

I can only suggest to release a new minor version that deprecates these pointless static methods (as people probably couldn't use them properly anyway), and then remove them completely with the next major release.

Again, sorry for the mess - I don't think there's much more we can do here.

[benjaminprojas]

I am also having an issue here. Even when trying to expire using @franzliedke suggestion, it doesn't work for me. The only way I can actually get a cookie to expire is to create it again without a value and set withExpires() to a date in the past:

$responseCookie = SetCookie::create( 'cookie' )
	->withExpires( strtotime( '-2 years' ) )
	->withPath('/')
	->withDomain( COOKIE_DOMAIN )
	->withSecure( true )
	->withHttpOnly( true );
$response = FigResponseCookies::set( $response, $responseCookie );

[franzliedke]

@benjaminprojas Not sure I understand the exact problem here. Are you saying that, for my exception to work, SetCookie::expire() should also set the value to null?

[benjaminprojas]

@franzliedke I wasn't able to get your suggestion to work at all. The only way I could expire a cookie was to "create" a new one with the same name and with an expiration date in the past.

[franzliedke]

@benjaminprojas Can you show us what exactly you tried then? It's hard to guess. ;)

[Tagirijus]

I solved it for me this way: #22 (comment)

[franzliedke]

This is what we do at Flarum, and it works well:
https://github.com/flarum/core/blob/3bf74eaf10135cf683440b56741937a4a2713613/src/Http/CookieFactory.php

[fabswt]

Any plans to fix it?

[franzliedke]

I went ahead with my proposal to deprecate the misleading facade methods in #45.