Re: [PHP-DEV] Re: header() removes all header of the same name.

[Yasuo Ohgaki]

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.

[Yasuo Ohgaki]

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.

[Yasuo Ohgaki]

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.

[Stephen Reay]

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