[PHP-DEV] Re: header() removes all header of the same name.
Hi Yasuo, I don't think we should do anything about this beyond maybe warning the user in the manual. header() is a generic function for setting headers, it would be surprising if it had different behaviour for cookies or session cookies. It is possible that use of this function in this way may be deliberate. Thanks. -- Andrea Faulds https://ajf.me/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: header() removes all header of the same name.
Hi all, On Wed, Oct 19, 2016 at 1:34 PM, Yasuo Ohgaki wrote: > > On Wed, Oct 19, 2016 at 12:18 PM, Stephen Reay > wrote: >> I still have an issue with that. I believe the correct behaviour here is >> (assuming the `replace` argument to header() is honoured) what you’re >> seeing. Yes, it might be *unexpected* for new users, but its also *expected* >> by millions of current users/projects. >> >> I would suggest perhaps a warning on the header() docs page, and perhaps an >> example to avoid the issue on the Session handling page. >> >> Leaving it as-is, with improved docs means all functionality is *possible* >> with the right arguments. >> >> Changing to your proposal means advanced use-cases are *impossible* with any >> arguments. >> >> >> I realise you’re trying to remove WTF cases, but I don’t think removing >> advanced capabilities is the way to do that. > > Yes. Even framework developer(?) seems to have current behavior. > > In general, users shouldn't touch session ID. In case of user really > want to modify session ID cookie, following could be done. > > ob_start(); > session_start(); > header_remove('Set-Cookie'); > header('Set-Cookie: PHPSESSID= something'); > ?> > > Make header_remove() able to delete 'Set-Cookie' header. (Current behavior) > Make header() able to send 'Set-Cookie' header. (Current behavior, but > not remove session ID cookie) > > This allows users to send arbitrary session ID cookie when it is > needed really needed, while avoiding accidental session ID cookie > removal. > > What do you think? Another idea for session ID cookie and Set-Cookie header protection. Since we have setcookie() function, how about to have cookie dedicated functions for cookie header manipulation. I'm about to create new feature request as follows: - Protect session ID and other cookies from header(), header_remove() - header() removes any previously defined headers. header('Set-Cookie: something') / header_remove() deletes session ID and other Set-Cookie headers. Cookies should be protected from header()/header_remove(). Instead, create new cookie functions cookie_set() - Set cookie header (setcookie() alias) cookie_set_raw() - Set cookie header (setrawcookie alias) cookie_custom() - Set cookie with custom style. (The same as header(sprintf('Set-Cookie: %s', something)); cookie_remove() - Remove all cookie header. $name parameter is cookie name to be deleted. Protect Set-Cookie headers from header() and header_remove() -- This implementation is cleaner because core to session dependency is not required. It is also good to have naming standard confirming cookie function names. i.e. Cookie functions should be named cookie_*() according to CODING_STANDARDS. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: header() removes all header of the same name.
Hi Stephen, On Wed, Oct 19, 2016 at 1:34 PM, Yasuo Ohgaki wrote: >> I realise you’re trying to remove WTF cases, but I don’t think removing >> advanced capabilities is the way to do that. > > Yes. Even framework developer(?) seems to have current behavior. To be honest, I didn't have idea what's wrong with user provided code when I have a look at the bug report. I had to experiment a little to understand what is going on. Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: header() removes all header of the same name.
Hi Stephen, On Wed, Oct 19, 2016 at 12:18 PM, Stephen Reay wrote: > I still have an issue with that. I believe the correct behaviour here is > (assuming the `replace` argument to header() is honoured) what you’re seeing. > Yes, it might be *unexpected* for new users, but its also *expected* by > millions of current users/projects. > > I would suggest perhaps a warning on the header() docs page, and perhaps an > example to avoid the issue on the Session handling page. > > Leaving it as-is, with improved docs means all functionality is *possible* > with the right arguments. > > Changing to your proposal means advanced use-cases are *impossible* with any > arguments. > > > I realise you’re trying to remove WTF cases, but I don’t think removing > advanced capabilities is the way to do that. Yes. Even framework developer(?) seems to have current behavior. In general, users shouldn't touch session ID. In case of user really want to modify session ID cookie, following could be done. Make header_remove() able to delete 'Set-Cookie' header. (Current behavior) Make header() able to send 'Set-Cookie' header. (Current behavior, but not remove session ID cookie) This allows users to send arbitrary session ID cookie when it is needed really needed, while avoiding accidental session ID cookie removal. What do you think? Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Re: header() removes all header of the same name.
Hi Yasuo, I still have an issue with that. I believe the correct behaviour here is (assuming the `replace` argument to header() is honoured) what you’re seeing. Yes, it might be *unexpected* for new users, but its also *expected* by millions of current users/projects. I would suggest perhaps a warning on the header() docs page, and perhaps an example to avoid the issue on the Session handling page. Leaving it as-is, with improved docs means all functionality is *possible* with the right arguments. Changing to your proposal means advanced use-cases are *impossible* with any arguments. I realise you’re trying to remove WTF cases, but I don’t think removing advanced capabilities is the way to do that. Cheers Stephen > On 19 Oct 2016, at 08:00, Yasuo Ohgaki wrote: > > Hi all, > > On Tue, Oct 18, 2016 at 4:31 PM, Yasuo Ohgaki wrote: >> I understand why header() is made to remove all headers of the same >> name. This is needed in some cases, but it does not work well for some >> cases. >> >> We need to decide what to do with >> https://bugs.php.net/bug.php?id=72997 >> >> There is 2 issues. >> - header() removes all headers of the same name including 'Set-Cookie' >> - header() ignores replace flag. (This one is easy to fix) >> >> Since header() enables 'replace flag' by default, it removes all >> 'Set-Cookie' headers sent previously by default. It can easily disturb >> security related cookies to work. i.e. Session ID cookie, Auto Login >> cookie. This bug would be very hard to find for normal users, too. >> >> Restoring older behavior (Removing only one header) cannot be a >> resolution because it can still disturb security related cookies. >> >> Possible resolutions: >> >> - Prohibit 'Set-Cookie' for header() and force users to use setcookie() >> - Mitigate by disabling replace flag by default. (This is not a good idea, >> IMO) >> >> Both resolution requires BC, but this is better to be fixed ASAP. >> >> Non-BC resolution could be: >> - "Ask users to use setcookie() always for 'Set-Cookie'". >> >> I would like to prohibit 'Set-Cookie' by header() because it may >> remove session ID cookie as well as auto login cookie, etc. If we >> leave released version as it is now, I would like to prohibit >> 'Set-Cookie' by header() in PHP 7.1. >> >> Problem with this may be that user cannot modify 'Set-Cookie' header >> line as user want. >> >> $ php -r 'setcookie("REMEMBERME=value; expires=Sat, 03-Sep-2020 >> 05:38:43 GMT; path=/; domain=aaa");' >> PHP Warning: Cookie names cannot contain any of the following '=,; >> \t\r\n\013\014' in Command line code on line 1 >> >> >> Comments? > > An idea for session ID protection. > > Following code results in lost session always. > > session_start(); > session_regenerate_id(); > header('Set-Cookie: something'); > ?> > > header() function removes all header of the same name, e.g. > Set-Cookie, Expires, etc, by sapi_remove_header(). This could be very > hard to find the cause. > > > This risk can be removed w/o much BC. Only BC is when user is > intentionally trying to delete session ID cookie manually. This would > be very rare. We can add code that excludes session ID cookie in > sapi_remove_header(). > http://lxr.php.net/xref/PHP-MASTER/main/SAPI.c#593 > > To do that, we can search string like following > Set-Cookie: PHPSESSID=xxx > > The only issue is we need session global, i.e. PS(session_name), at > least. It's not nice to have dependency from SAPI.c to session, but it > protects session ID from removed by users by mistake. > > Any comments? > > Regards, > > -- > Yasuo Ohgaki > yohg...@ohgaki.net > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
[PHP-DEV] Re: header() removes all header of the same name.
Hi all, On Tue, Oct 18, 2016 at 4:31 PM, Yasuo Ohgaki wrote: > I understand why header() is made to remove all headers of the same > name. This is needed in some cases, but it does not work well for some > cases. > > We need to decide what to do with > https://bugs.php.net/bug.php?id=72997 > > There is 2 issues. > - header() removes all headers of the same name including 'Set-Cookie' > - header() ignores replace flag. (This one is easy to fix) > > Since header() enables 'replace flag' by default, it removes all > 'Set-Cookie' headers sent previously by default. It can easily disturb > security related cookies to work. i.e. Session ID cookie, Auto Login > cookie. This bug would be very hard to find for normal users, too. > > Restoring older behavior (Removing only one header) cannot be a > resolution because it can still disturb security related cookies. > > Possible resolutions: > > - Prohibit 'Set-Cookie' for header() and force users to use setcookie() > - Mitigate by disabling replace flag by default. (This is not a good idea, > IMO) > > Both resolution requires BC, but this is better to be fixed ASAP. > > Non-BC resolution could be: > - "Ask users to use setcookie() always for 'Set-Cookie'". > > I would like to prohibit 'Set-Cookie' by header() because it may > remove session ID cookie as well as auto login cookie, etc. If we > leave released version as it is now, I would like to prohibit > 'Set-Cookie' by header() in PHP 7.1. > > Problem with this may be that user cannot modify 'Set-Cookie' header > line as user want. > > $ php -r 'setcookie("REMEMBERME=value; expires=Sat, 03-Sep-2020 > 05:38:43 GMT; path=/; domain=aaa");' > PHP Warning: Cookie names cannot contain any of the following '=,; > \t\r\n\013\014' in Command line code on line 1 > > > Comments? An idea for session ID protection. Following code results in lost session always. header() function removes all header of the same name, e.g. Set-Cookie, Expires, etc, by sapi_remove_header(). This could be very hard to find the cause. This risk can be removed w/o much BC. Only BC is when user is intentionally trying to delete session ID cookie manually. This would be very rare. We can add code that excludes session ID cookie in sapi_remove_header(). http://lxr.php.net/xref/PHP-MASTER/main/SAPI.c#593 To do that, we can search string like following Set-Cookie: PHPSESSID=xxx The only issue is we need session global, i.e. PS(session_name), at least. It's not nice to have dependency from SAPI.c to session, but it protects session ID from removed by users by mistake. Any comments? Regards, -- Yasuo Ohgaki yohg...@ohgaki.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php