Fixes setcookie to only send one Set-Cookie header per name

[ralt]

According to the IETF RFC 6265, only one Set-Cookie header should be sent per cookie name.

Fixes https://bugs.php.net/bug.php?id=67736

[ralt]

I'm not sure as to whether prevent the second set-cookie from being added, or just put a single warning. Right now, it just puts a warning, but the message is wrong (should be "should not"). I'm not sure what my future commit should be: fix the behavior, or fix the comment?

[Majkl578]

Fixes? I fail to see how this fixes the problem, it only throws E_WARNING, but does not fix anything.
(Edit: Sent before reading your comment.)

[Majkl578]

Actually, I would expect it to be "last one wins", so the latter cookie overrides previous ones. It is IIRC also how browsers handle cookies with same name.

[Danack]

Fixes? I fail to see how this fixes the problem, it only throws E_WARNING, but does not fix anything.

It does not fix anything because the PHP code does not have a bug. The RFC does not forbid sending multiple cookies with the same name:

Servers SHOULD NOT include more than one Set-Cookie header field in the same response with the same cookie-name. (See Section 5.2 for how user agents handle this case.)"

Although it is definitely not recommended, the RFC does not say MUST NOT.

Even if it was forbidden by the RFC, it is not PHP's job to enforce conformance to RFCs. That is up to the programmer using PHP to not send improper data.

The idea of having setcookie magically modifying the cookie header that is sent is nuts. It would be adding more magic behaviour to PHP to cover up the fact that the programmer who is calling setcookie() has a bug in their code.

Adding a warning is enough. It tells people that they are doing something that is probably unwise, but is technically allowed. Doing anything more is not only the wrong thing to do anyway, but introduces a backwards compatibility break for no good reason.

If someone is unaware they are calling setcookie() twice with the same name, the error will alert them and allow them to fix their code.

If someone is deliberately calling setcookie() twice with the same name (e.g. for a legacy application that knows how to handle multiple cookies with the same name) then they will need to continue using the current behaviour.

Changing the behaviour just because you think it would be 'better' is not a good idea. There needs to be a clear reason for changing the behaviour, and I don't think there is one here.

[hikari-no-yume]

"Cannot" is misleading (implies header wasn't set). This should say "should not".

[datibbaw]

Could you add a test case that triggers the warning?

[ralt]

I've added 2 test cases: one with display_errors set to off showing that the behavior doesn't change (i.e. 2 set-cookie headers are sent), and another with display_errors set to on showing that the warning is thrown. Since the warning is thrown, only one header can be sent in this case though.

[hikari-no-yume]

Why bother having a display_errors=0 test? We already test that display_errors works in other places.

[ralt]

@TazeTSchnitzel to make sure the behavior of having 2 set-cookie headers is kept intact. When display_errors is set to on, you get the warning, so the 2nd header can't be set. (because headers are already sent.)

[hikari-no-yume]

@ralt Wait, what? Why does changing that ini setting affect behaviour?!

[ralt]

@TazeTSchnitzel because when the warnings are shown, the headers can't be sent anymore after that.

[hikari-no-yume]

@ralt Oh, right, I see what you're saying. Because the warning was outputted, setting a header would fail.

[ralt]

Exactly! That was hard to say, my bad.

[yohgaki]

I've fixed similar issue in session module before. IIRC, I didn't use hash to detect already sent header to minimize changes. It may be better to rewrite session module to use the hash if this is going to be merged.

Anyway, isn't it better to overwrite old setcookie() silently? It would be the programmer's intention almost always.

[ralt]

@yohgaki w.r.t. the behavior, see #795 (comment)

[pierrejoye]

A couple of comments:

• could you remove WS changes pls?
• it should use php_error_docref instead of zend_error and the doc should be updated, if the behavior changes (let see the outcome of the discussions on internals)
• I am not sure about that either, must not != should not
• it is very unlikely to make it in 5.4/5/6 as it changes internal exposed structure

[ralt]

@pierrejoye

• Why should I remove the WS changes? AFAIK the general opinion is to clean up trailing whitespaces where you're working. (See http://markmail.org/message/52abxyjjjpzonzst#query:+page:1+mid:cqfzaod7khipvps6+state:results ) If you simply want to see the diff without WS changes, add ?w=1 to the URL. Example: https://github.com/php/php-src/pull/795/files?w=1 (git provides this kind of option (git diff -b) locally too.)
• What's the difference between php_error_docref and zend_error? Why should I use one over the other?

[pierrejoye]

WS must be done in separate commits, ease merges and co (3rd parties too)
On Sep 6, 2014 12:42 PM, "Florian Margaine" notifications@github.com
wrote:

@pierrejoye https://github.com/pierrejoye

• Why should I remove the WS changes? AFAIK, the general opinion is to
  clean up where you're working. (See
  http://markmail.org/message/52abxyjjjpzonzst#query:+page:1+mid:cqfzaod7khipvps6+state:results
  ) If you simply want to see the diff without WS changes, add ?w=1 to the
  URL. Example: https://github.com/php/php-src/pull/795/files?w=1 (git
  provides this kind of option when using git diff locally too.)
• What's the difference between php_error_docref and zend_error? Why
  should I use one over the other?

—
Reply to this email directly or view it on GitHub
#795 (comment).

[pierrejoye]

Doc ref is for ext and co, provides links to the doc too (not sure about Ze)
On Sep 6, 2014 12:42 PM, "Florian Margaine" notifications@github.com
wrote:

@pierrejoye https://github.com/pierrejoye

• Why should I remove the WS changes? AFAIK, the general opinion is to
  clean up where you're working. (See
  http://markmail.org/message/52abxyjjjpzonzst#query:+page:1+mid:cqfzaod7khipvps6+state:results
  ) If you simply want to see the diff without WS changes, add ?w=1 to the
  URL. Example: https://github.com/php/php-src/pull/795/files?w=1 (git
  provides this kind of option when using git diff locally too.)
• What's the difference between php_error_docref and zend_error? Why
  should I use one over the other?

—
Reply to this email directly or view it on GitHub
#795 (comment).

[yohgaki]

I understand it valid to send multiple cookies even if there might be different behavior browser to browser.

There was similar issue when browser is sending cookies to server. There could be multiple cookies for a single URL and old browsers sent multiple cookies. Current browsers only send outstanding cookie for the browser to avoid confusion. (I have different opinion for this behavior, though)

PHP could be behave like this for the same name and the same attribute cookies. Allowing to send multiple cookies is better to be optional, since latter one is the outstanding cookie almost always. The behavior depends on browser implementation. It's better to remove uncertainty as much as possible, IMHO.

BTW, the patch should deal with "attributes" also. The same name cookie is completely valid if attribute differs and should not raise any error.

[ralt]

@pierrejoye I've fixed the patch to remove whitespaces and to use php_error_doc_ref.

[ralt]

@jpauli fixed your comments.

[Kubo2]

I agree with @Majkl578, it should adopt „the latter the winner“. But also agree with @yohgaki that it could be optional, dealing with changed behavior. That's my opinion. :-)

@TazeTSchnitzel, @ralt: but you can also get both generated warnings in test, using output buffering functions, can't you? Like below:

--TEST--
testing multiple setcookie() calls with the same 
name (intentionally with display_errors On)
--INI--
display_errors=1
--FILE--
<?php
ob_start();

setcookie('foo', 'bar');

// called with the same name
setcookie('foo', 'second call');
setcookie('foo', 'third call');

ob_end_flush();

?>
--EXPECTF--

Warning: setcookie(): should not be used twice with the same name in %s

Warning: setcookie(): should not be used twice with the same name in %s

[yohgaki]

Just an additional note for this patch. I have code like

for all combination of $path, $domain, $secure, $httponly
setcookie('foo', '', 1, $path, $domain, $secure, $httponly)

to remove unwanted cookies set by someone else. There should be a way to allow this kind of code w/o any errors.

[hikari-no-yume]

@yohgaki perhaps we could add unsetcookie?

[jpauli]

@TazeTSchnitzel Yup , that would be the solution. Also, the Warning error message for setcookie() with the same cookie is somehow discutable.
I guess we should RFC those ideas

[hikari-no-yume]

I mean, using setcookie with '' just feels hackish to me.

[Kubo2]

@TazeTSchnitzel:

Adding unsetcookie() is not that bad idea, but after adding that there may grow up custom implementations like following

<?php

function _setCookie(...$attrs) {
    unsetcookie($attrs[0]);
    return setcookie(...$attrs);
}

to enwrap trivially expected „the latter the winner“ behavior and beginners, inspired in that many tutorials, will start using non-breaking

unsetcookie('foo');
setcookie('foo', 'new value');

calls wherever they will be „setting a cookie“ because of expecting supposed behavior which will be disapproved by that tutorials. And their codes natively using cookies heavily will grow up becasue of using two calls instead of one trivial. Do you want to accept this?

---

But it could be good idea to adding unsetcookie() and simultaneously adopting expected „the latter the winner“ behavior of setcookie(), couldn't it?

[coladict]

Perhaps it would be better if instead of sending E_WARNING and not setting to have it issue E_NOTICE and override the previous cookie data. That way it will behave as expected if your intention is to update a cookie and at the same time provide a log trace in case it's overridden somewhere you don't want it to be.

[hikari-no-yume]

@coladict If we emit an E_NOTICE and do so after setting the cookie, it wouldn't prevent setting the header, yeah.

[ralt]

@TazeTSchnitzel only if it is the last setcookie though. Further calls wouldn't be set. Shouldn't it be more consistent and fail right away?

[hikari-no-yume]

@ralt Typically, E_NOTICE is hidden in production, so it wouldn't stop sending further headers, right?

[ralt]

Well, E_WARNING is also typically disabled in production. I don't have a
strong opinion on whether it should be a warning or a notice though.

[yohgaki]

@TazeTSchnitzel unsetcookie() would be nice to have. Having '' value is hacky and we may try to remove as much as it can when an option is specified. e.g. combinations of domain, path, attributes(httponly, secure)

Trying to remove all cookies is the best effort, since there is no way to remove all cookies for a name. It's much better than nothing.

[Danack]

This appears to be duplicated by #849

[hikari-no-yume]

@Danack Look more carefully. One targets 5.6, the other master.

[yohgaki]

RFC compliance is not good always. For example, I have code that try to remove all offending cookies. It is used to prevent session fixation via adoptive session or DoS. (I described in previous reply)

//Remove all cookies as many as possible. i.e. Try all combinations of $path, $domain, $secure , $httponly. Actual code is omitted
setcookie('PHPSESS', '', 1, $path, $domain, $secure , $httponly);

http://tools.ietf.org/html/rfc6265#page-8
4.1.1. Syntax
Servers SHOULD NOT include more than one Set-Cookie header field in
the same response with the same cookie-name. (See Section 5.2 for
how user agents handle this case.)

It say server SHOULD NOT. However, algorithm described in Section 5.2 does not ignore multiple cookies of the same name.

I don't object this if I can remove all cookies of the same name by single API.

Suggestion:

• Allow users to try to remove all possible cookies. e.g. Add unsetcookie()
• Target this change 7.0 or later.

We should consider how these could be used with session.

This code will produce multiple set-cookie headers of the same name. unsetcookie() and session_start() should not be called at once to avoid race condition. unsetcookie() should be called from a dedicated request. This applies to setcookie() also. e.g.

This should not be done as well.

This should be in the document otherwise there may be number of bug reports complaining "setcookie() cannot send cookie and raise error".

unsetcookie() should accept these options

• $domain : if omitted, use current domain in the request uri.
• $path : if omitted, use current path in the request uri.
• $simple : if true, simply send cookie without combinations of attributes.

Please note that unsetcookie() must try all domain/path combinations.
e.g.
If $path is "/myapp/login/reset_cookie/index.php", unsetcookie() should send
setcookie('foo', '', 1 , "/");
setcookie('foo', '', 1 , "/myapp/");
setcookie('foo', '', 1 , "/myapp/login/");
setcookie('foo', '', 1 , "/myapp/login/reset_cookie/");
There will be long list of set-cookie headers because it's combinations of $path, $domain, $secure, $httponly.

Anyway, even when PHP forbid multiple cookies of the same name, we may use header() to send multiple set-cookie headers. I don't object as long as this is targeted for 7.0. I think I've removed multiple set-cookie headers from session module, but there may be leftover. Please test this patch with session extensively.

[php-pulls]

Comment on behalf of krakjoe at php.net:

Since this PR targets a security fix only branch of PHP, I'm closing the PR.