header() removes all header of the same name.

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

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?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

(Apologies for the dupe, re-sending for the list.)

If the replace flag was fixed, isn’t this then just a case of making sure userland sets replace to false if they want existing set-cookie headers retained?

Removing the ability to write a custom Set-Cookie header introduces a bigger problem than the current one, IMO.

Hi all,

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?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

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.

<?php
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=xxxxxxx

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
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

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

Hi all,

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.

<?php
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=xxxxxxx

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
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

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.

<?php
ob_start();
session_start();
header_remove('Set-Cookie');
header('Set-Cookie: PHPSESSID=xxxx 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?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

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
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

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.

<?php
ob_start();
session_start();
header_remove('Set-Cookie');
header('Set-Cookie: PHPSESSID=xxxx 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
yohgaki@ohgaki.net

8 years ago by Andrea Faulds — view source

unread

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/

8 years ago by Stanislav Malyshev — view source

unread

Hi!

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)

We have the flag, so if it doesn't work it should be fixed. Also, one
should use setcookie() for cookies, usually.

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)

I don't think we should do either.

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.

I don't think it's a good idea. If somebody is using header(), it should
work like header() works. If you don't like how it works, use setcookie.

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

You are using setcookie() wrong here. See: http://php.net/setcookie

--
Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

I posted an an idea for preventing accidental cookie deletion.
'Set-Cookie' is a HTTP header, but provide dedicated functions for it. I pasted
it with a little modification.
What do you think?

Bottom line is I would like to prevent lost session ID by header()
in the future.

Implement cookie_*() functions in 7.x, then prohibit 'Set-Cookie' for
header() in 8.x

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)

We have the flag, so if it doesn't work it should be fixed. Also, one
should use setcookie() for cookies, usually.

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_list() - Mostly the same as headers_list()
cookie_remove([string $name]) - Mostly the same as header_remove()
Remove 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.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

Hi All,

Just to make my earlier point of view crystal clear: As a purely userland party and someone maintaining a PHP framework, I don’t think it’s acceptable to limit which headers header()/header_remove() can operate on, particularly when the problem you’re trying to ‘solve’ is simply incorrect use of the functions available. It is possible to achieve any outcome desired with correct use of the header, session and cookie functions (and assuming the $replace argument to header() works correctly).

I still believe the way to solve this issue is with better information about usage, not by removing existing functionality.

So, please do not consider this to be an acceptable solution.

Cheers

Stephen

Hi Stas,

I posted an an idea for preventing accidental cookie deletion.
'Set-Cookie' is a HTTP header, but provide dedicated functions for it. I pasted
it with a little modification.
What do you think?

Bottom line is I would like to prevent lost session ID by header()
in the future.

Implement cookie_*() functions in 7.x, then prohibit 'Set-Cookie' for
header() in 8.x

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)

We have the flag, so if it doesn't work it should be fixed. Also, one
should use setcookie() for cookies, usually.

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_list() - Mostly the same as headers_list()
cookie_remove([string $name]) - Mostly the same as header_remove()
Remove 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.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

If the replace flag was fixed, isn’t this then just a case of making sure userland sets replace to false if they want existing set-cookie headers retained?

Yes and no.

If users use the replace flag correctly, then it will work. However, I
don't expect users set replace flag correctly. If replace flag's
default was opposite, it would work better.

Removing the ability to write a custom Set-Cookie header introduces a bigger problem than the current one, IMO.

OK. Let's just fix the replace flag and document removing 'Set-Cookie'
header by header() may result in unwanted results.

Everyone is ok with this?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

Hi Yasuo,

I agree there are probably a lot using the default, but I think it’s reasonable to expect anyone using a header(‘Set-Cookie:..’); call rather than setcookie() to be aware of the 2nd argument for header(), so this solution sounds good to me.

Cheers

Stephen

Hi Stephen,

If the replace flag was fixed, isn’t this then just a case of making sure userland sets replace to false if they want existing set-cookie headers retained?

Yes and no.

If users use the replace flag correctly, then it will work. However, I
don't expect users set replace flag correctly. If replace flag's
default was opposite, it would work better.

Removing the ability to write a custom Set-Cookie header introduces a bigger problem than the current one, IMO.

OK. Let's just fix the replace flag and document removing 'Set-Cookie'
header by header() may result in unwanted results.

Everyone is ok with this?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

Just to make my earlier point of view crystal clear: As a purely userland party and someone maintaining a PHP framework, I don’t think it’s acceptable to limit which headers header()/header_remove() can operate on, particularly when the problem you’re trying to ‘solve’ is simply incorrect use of the functions available. It is possible to achieve any outcome desired with correct use of the header, session and cookie functions (and assuming the $replace argument to header() works correctly).

I still believe the way to solve this issue is with better information about usage, not by removing existing functionality.

So, please do not consider this to be an acceptable solution.

The idea is to separate HTTP header handling functions.

header*() for any HTTP headers except 'Set-Cookie'
cookie*() for only 'Set-Cookie' header

Developer does not lose anything.

As you mention, custom 'Set-Cookie' header is for advanced users.
Those people wouldn't have problems with new API.

With this new API, we'll have standard confirming function names for
cookie functions as a bonus, too. (I'm enthusiastic to clean up and
have consistent function names as you might know.)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

Hi Yasuo,

Hi Stephen,

Just to make my earlier point of view crystal clear: As a purely userland party and someone maintaining a PHP framework, I don’t think it’s acceptable to limit which headers header()/header_remove() can operate on, particularly when the problem you’re trying to ‘solve’ is simply incorrect use of the functions available. It is possible to achieve any outcome desired with correct use of the header, session and cookie functions (and assuming the $replace argument to header() works correctly).

I still believe the way to solve this issue is with better information about usage, not by removing existing functionality.

So, please do not consider this to be an acceptable solution.

The idea is to separate HTTP header handling functions.

header*() for any HTTP headers except 'Set-Cookie'

cookie*() for only 'Set-Cookie' header

Developer does not lose anything.

The Developer loses what s/he has now - a fully functional header() function.

As you mention, custom 'Set-Cookie' header is for advanced users.
Those people wouldn't have problems with new API.

Those people shouldn’t have problems using header() properly.

With this new API, we'll have standard confirming function names for
cookie functions as a bonus, too. (I'm enthusiastic to clean up and
have consistent function names as you might know.)

I don’t have a problem with improved function names. I have a problem with removing functionality that is basically only ever going to be used by advanced developers, because it has slightly non obvious behaviour that could be fixed by a simple documentation note.

Please understand: no “solution" where header() loses the ability to write any arbitrary header will be acceptable in my opinion.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Cheers

Stephen

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

Please understand: no “solution" where header() loses the ability to write any arbitrary header will be acceptable in my opinion.

Thank you for feedback.
I'll include vote option for prohibiting 'Set-Cookie' for header*()

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Niklas Keller — view source

unread

2016-10-20 10:28 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Stephen,

On Thu, Oct 20, 2016 at 5:23 PM, Stephen Reay php-lists@koalephant.com
wrote:

Please understand: no “solution" where header() loses the ability to
write any arbitrary header will be acceptable in my opinion.

Thank you for feedback.
I'll include vote option for prohibiting 'Set-Cookie' for header*()

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--

Hi Yasuo,

same here, it's not acceptable to limit header and restrict set_cookie.
Just think about all those frameworks that would have to specialcase
setting headers now and have to use the cookie API then.

If you want to protect the session cookie header, why not simply set it
right before the first output? That'd make it also non-overrideable, but
leaves header() intact. But I guess it's harder to implement.

8 years ago by Yasuo Ohgaki — view source

unread

Hi Niklas,

same here, it's not acceptable to limit header and restrict set_cookie.
Just think about all those frameworks that would have to specialcase setting
headers now and have to use the cookie API then.

If you want to protect the session cookie header, why not simply set it
right before the first output? That'd make it also non-overrideable, but
leaves header() intact. But I guess it's harder to implement.

Although, I prefer to have completely separate API, we have to
implement vote result. So vote no for "Disabling 'Set-Cookie' for
header*()" vote option.

Regarding about delaying session cookie header, it is possible to use
output buffer to delay output so that session module can send HTTP
header at request shutdown. However, it will break almost all session
enabled applications that require immediate output. Therefore, it's
easy to implement, but not possible for this reason.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Niklas Keller — view source

unread

2016-10-20 11:57 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

same here, it's not acceptable to limit header and restrict set_cookie.
Just think about all those frameworks that would have to specialcase
setting
headers now and have to use the cookie API then.

If you want to protect the session cookie header, why not simply set it
right before the first output? That'd make it also non-overrideable, but
leaves header() intact. But I guess it's harder to implement.

Although, I prefer to have completely separate API, we have to
implement vote result. So vote no for "Disabling 'Set-Cookie' for
header*()" vote option.

I don't have a vote. But this breaks BC. It might remove surprisings when
using sessions, but having header() not being able to set set-cookie
headers adds new surprisings.

Regarding about delaying session cookie header, it is possible to use
output buffer to delay output so that session module can send HTTP
header at request shutdown. However, it will break almost all session
enabled applications that require immediate output. Therefore, it's
easy to implement, but not possible for this reason.

I meant squeeze in right before output or on first flush() call. There must
be a thing that sets a "already output" flag that prevents further headers.
We could use that mechanism to buffer all headers and just send them out
there and have a hook for the session module.

Regards, Niklas

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

Hi Niklas,

There is even a userland hook for the specific functionality you mention: header_register_callback().

But I would argue that no fix is necessary. If you as a developer call session_start(), and then later call header(‘Set-Cookie:…’) with replace left as true, I think it’s safe to assume you’re either doing it deliberately, or that you’ll go read the documentation on sessions and header() to discover the problem. (Or possibly file a bug which will be marked as not-a-bug and refer you to the documentation).

The only solution that retains full control for the developer, is no change. Any “magic” about “untouchable” cookie headers (e.g. forcing the session cookie header after userland cookie headers) takes away options for the developer.

If anything the BC break being discussed should be to invert the default value for the $replace argument to header().

Cheers

Stephen

2016-10-20 11:57 GMT+02:00 Yasuo Ohgaki <yohgaki@ohgaki.net mailto:yohgaki@ohgaki.net>:
Hi Niklas,

same here, it's not acceptable to limit header and restrict set_cookie.
Just think about all those frameworks that would have to specialcase setting
headers now and have to use the cookie API then.

If you want to protect the session cookie header, why not simply set it
right before the first output? That'd make it also non-overrideable, but
leaves header() intact. But I guess it's harder to implement.

Although, I prefer to have completely separate API, we have to
implement vote result. So vote no for "Disabling 'Set-Cookie' for
header*()" vote option.

I don't have a vote. But this breaks BC. It might remove surprisings when using sessions, but having header() not being able to set set-cookie headers adds new surprisings.

Regarding about delaying session cookie header, it is possible to use
output buffer to delay output so that session module can send HTTP
header at request shutdown. However, it will break almost all session
enabled applications that require immediate output. Therefore, it's
easy to implement, but not possible for this reason.

I meant squeeze in right before output or on first flush() call. There must be a thing that sets a "already output" flag that prevents further headers. We could use that mechanism to buffer all headers and just send them out there and have a hook for the session module.

Regards, Niklas

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net mailto:yohgaki@ohgaki.net

--

<http://www.php.net/unsub.php

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

The only solution that retains full control for the developer, is no
change. Any “magic” about “untouchable” cookie headers (e.g. forcing the
session cookie header after userland cookie headers) takes away options for
the developer.

My cookie*() functions proposal allows developers to remove header by
cookie_remove() and can send any cookie header by cookie_custom().
Therefore, developers have full control if they have to.

The only pain is that users may have to use cookie*() functions if we
disallow header('Set-Cookie') which will be a vote option. If there is
fully functional cookie*() functions, it will mitigate wrong
header('Set-Cookie') usage regardless of the vote result, hopefully.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

Hi Yasuo,

As with Niklas, I have no vote, so my only option to prevent what I consider to be a bad decision, is to post to this thread and hope that enough of those who do have voting rights, reject the proposal.

I understand what you’re proposing. But honestly I don’t even agree with the premise that there is a problem that needs to be fixed.

Did you know that if you manually set the Content-Length header to less than the actual body length, many browsers (Safari, Chrome, and FF definitely) will stop reading/processing the response at the length you specify?
So should we also prevent setting Content-Length via header() ?

Honestly I don’t understand how this is still a discussion. The developer failed to set the $replace argument to false. At most I would expect this to result in a documentation note warning about the use of header(‘Set-Cookie…’).

I appreciate you trying to make improvements, and I’d definitely be in favour of the function naming cleanup you mentioned earlier, but all of this “protected” headers and extra function calls, because someone forgot to type “, false” seems ridiculous to me honestly.

Cheers

Stephen

Hi Stephen,

The only solution that retains full control for the developer, is no
change. Any “magic” about “untouchable” cookie headers (e.g. forcing the
session cookie header after userland cookie headers) takes away options for
the developer.

My cookie*() functions proposal allows developers to remove header by
cookie_remove() and can send any cookie header by cookie_custom().
Therefore, developers have full control if they have to.

The only pain is that users may have to use cookie*() functions if we
disallow header('Set-Cookie') which will be a vote option. If there is
fully functional cookie*() functions, it will mitigate wrong
header('Set-Cookie') usage regardless of the vote result, hopefully.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Ptephen,

As with Niklas, I have no vote, so my only option to prevent what I consider to be a bad decision, is to post to this thread and hope that enough of those who do have voting rights, reject the proposal.

It's okay, but aren't I and framework developers on the same side?

I don't want to get bug report that session lost or some important
cookie lost somehow.

Framework developers don't want to get bug report that user logouts or
some important cookie lost somehow.

It's better that we have header*() and cookie*() function for
dedicated purpose, isn't it?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

Hi Yasuo,

Hi Ptephen,

As with Niklas, I have no vote, so my only option to prevent what I consider to be a bad decision, is to post to this thread and hope that enough of those who do have voting rights, reject the proposal.

It's okay, but aren't I and framework developers on the same side?

Apparently not. I expect people using my work to read the documentation. I don’t remove mysql support because they don’t understand why a “DELETE * FROM foo” query deleted all their data. Similarly I don’t see why it’s a bug to remove headers when you leave $replace to the default of true.

I don't want to get bug report that session lost or some important
cookie lost somehow.

Why is your concern so focussed on solving problems for inexperienced developers, who are effectively using functions incorrectly, at the expense of experienced developers who are doing the right thing?
This response effectively encourages bad behaviour (did the reporter even check the docs for header() to see why it’s replacing the session cookie?

How many bug reports do you think you’re going to get saying “header() no longer supports set-cookie headers”.

Framework developers don't want to get bug report that user logouts or
some important cookie lost somehow.

It's better that we have header*() and cookie*() function for
dedicated purpose, isn't it?

Having dedicated functions for setting cookies, yes that makes sense.
Removing that functionality from header() makes no sense. A cookie is still a header.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

I don't want to get bug report that session lost or some important
cookie lost somehow.

Why is your concern so focussed on solving problems for inexperienced developers, who are effectively using functions incorrectly, at the expense of experienced developers who are doing the right thing?
This response effectively encourages bad behaviour (did the reporter even check the docs for header() to see why it’s replacing the session cookie?

The root cause of misuse is header() and setcookie() difference even
if both manipulate HTTP header.

header() - Removes HTTP headers previously defined by default.
setcookie() - Appends 'Set-Cookie' HTTP header by default. Unlike
header(), no remove feature at all.

API design is inappropriate, IMHO.
I would like to help users by providing reasonable/expectable APIs.
Current header() and setcookie() behavior is reasonable for a
individual feature, but mixing them seems not nice.

There are 3 people not in favor of 'Set-Cookie' protections in header()
Having consistent standard confirming function name means more to me,
I may remove 'Set-Cookie' header vote option, if nobody really cares
it, since I would like to have smooth RFC process.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

I didn't answer this question and would like to make my point of view clear.

Why is your concern so focussed on solving problems for inexperienced developers, who are effectively using functions incorrectly, at the expense of experienced developers who are doing the right thing?

The reason why I'm focusing on problems for inexperienced developers
is productivity with PHP.
IMHO, it is better to remove gocha whenever it is possible.

It's okay to read manual and search net to solve "obvious problem in
code". However, if 10K developers spend 10 hours to solve a problem,
100K hours of productivity with PHP is lost. The change may have small
impact, but small things add up. As long as there is reasonable
alternative way to implement advanced behaviors and small impact on
existing codes, it is better to provide easy and safe default
behaviors.

Making PHP easy to use and a productive language worths in the long
run. This is the reason why some of my proposals are focusing on
making PHP easy to use and safe to use by default.

e.g. Provide correct and safe session management by default, prevent
insane session usage and raise errors for them, add DbC support,
make uniqid more unique, consistent function names, disallow script
inclusion attacks, keep/improve URL rewriter rather than depreciating it
(URL rewriter is very useful to keep private site private, i.e. Disallow
all cross site requests, therefore disallow CSRF, XSS completely.
PHP 7.1 has dedicated output buffer and setting for user URL rewrite.
It's easier and safer to use with PHP 7.1), etc.

PHP is popular because it is easy to use and productive. Let's keep
this and improve! Other languages/platforms are catching up.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Christoph M. Becker — view source

unread

As with Niklas, I have no vote, so my only option to prevent what I consider to be a bad decision, is to post to this thread and hope that enough of those who do have voting rights, reject the proposal.

I understand what you’re proposing. But honestly I don’t even agree with the premise that there is a problem that needs to be fixed.

Did you know that if you manually set the Content-Length header to less than the actual body length, many browsers (Safari, Chrome, and FF definitely) will stop reading/processing the response at the length you specify?
So should we also prevent setting Content-Length via header() ?

Honestly I don’t understand how this is still a discussion. The developer failed to set the $replace argument to false. At most I would expect this to result in a documentation note warning about the use of header(‘Set-Cookie…’).

I appreciate you trying to make improvements, and I’d definitely be in favour of the function naming cleanup you mentioned earlier, but all of this “protected” headers and extra function calls, because someone forgot to type “, false” seems ridiculous to me honestly.

Full ACK.

--
Christoph M. Becker

8 years ago by Niklas Keller — view source

unread

2016-10-20 13:41 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Stephen,

On Thu, Oct 20, 2016 at 8:24 PM, Stephen Reay php-lists@koalephant.com
wrote:

The only solution that retains full control for the developer, is no
change. Any “magic” about “untouchable” cookie headers (e.g. forcing the
session cookie header after userland cookie headers) takes away options
for
the developer.

My cookie*() functions proposal allows developers to remove header by
cookie_remove() and can send any cookie header by cookie_custom().
Therefore, developers have full control if they have to.

The only pain is that users may have to use cookie*() functions if we
disallow header('Set-Cookie') which will be a vote option. If there is
fully functional cookie*() functions, it will mitigate wrong
header('Set-Cookie') usage regardless of the vote result, hopefully.

What about extensions to the set-cookie header? Take SameSite as a
recent example. The setcookie API doesn't cover that. Besides that, the
current setcookie API is awful, people just added more and more
parameters.

Before we even discuss disallowing header("set-cookie"), we should have a
sane cookie API, e.g. one that like setcookie($name, $value, $flags).

That's also the way we implemented it in Aerys (
https://github.com/amphp/aerys/blob/9a7327f062aa678408dfe4f4c3c7f479db16f187/lib/Response.php#L49-L58).
It's a simple wrapper around addHeader to make life easier, but it
doesn't restrict developers to call setHeader and replace all
set-cookie headers.

Regards, Niklas

8 years ago by Yasuo Ohgaki — view source

unread

Hi Niklas,

2016-10-20 13:41 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Stephen,

On Thu, Oct 20, 2016 at 8:24 PM, Stephen Reay php-lists@koalephant.com
wrote:

The only solution that retains full control for the developer, is no
change. Any “magic” about “untouchable” cookie headers (e.g. forcing the
session cookie header after userland cookie headers) takes away options
for
the developer.

My cookie*() functions proposal allows developers to remove header by
cookie_remove() and can send any cookie header by cookie_custom().
Therefore, developers have full control if they have to.

The only pain is that users may have to use cookie*() functions if we
disallow header('Set-Cookie') which will be a vote option. If there is
fully functional cookie*() functions, it will mitigate wrong
header('Set-Cookie') usage regardless of the vote result, hopefully.

What about extensions to the set-cookie header? Take SameSite as a
recent example. The setcookie API doesn't cover that. Besides that, the
current setcookie API is awful, people just added more and more
parameters.

Before we even discuss disallowing header("set-cookie"), we should have a
sane cookie API, e.g. one that like setcookie($name, $value, $flags).

Sure I'll. I don't like it either.
Array can be used for this.

That's also the way we implemented it in Aerys
(https://github.com/amphp/aerys/blob/9a7327f062aa678408dfe4f4c3c7f479db16f187/lib/Response.php#L49-L58).
It's a simple wrapper around addHeader to make life easier, but it doesn't
restrict developers to call setHeader and replace all set-cookie
headers.

The function does it :)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Niklas and all,

Before we even discuss disallowing header("set-cookie"), we should have a
sane cookie API, e.g. one that like setcookie($name, $value, $flags).

That's also the way we implemented it in Aerys
(https://github.com/amphp/aerys/blob/9a7327f062aa678408dfe4f4c3c7f479db16f187/lib/Response.php#L49-L58).
It's a simple wrapper around addHeader to make life easier, but it doesn't
restrict developers to call setHeader and replace all set-cookie
headers.

We choose current API for reason. It does not look pretty.
This is patch allow array config for 3rd param for setcookie().

https://gist.github.com/yohgaki/b86e07cd450777422c1a467166cd2fd3

I suppose some of us will have opinions having this kind of code(s).

Any comments?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Before we even discuss disallowing header("set-cookie"), we should have a
sane cookie API, e.g. one that like setcookie($name, $value, $flags).

That's also the way we implemented it in Aerys
(https://github.com/amphp/aerys/blob/9a7327f062aa678408dfe4f4c3c7f479db16f187/lib/Response.php#L49-L58).
It's a simple wrapper around addHeader to make life easier, but it doesn't
restrict developers to call setHeader and replace all set-cookie
headers.

We choose current API for reason. It does not look pretty.
This is patch allow array config for 3rd param for setcookie().

https://gist.github.com/yohgaki/b86e07cd450777422c1a467166cd2fd3

I suppose some of us will have opinions having this kind of code(s).

Any comments?

Execution example.

[yohgaki@dev github-php-src]$ cat t13.php
<?php
setcookie('A', 'B', ['httponly'=>1, 'path'=>'foo',
'expires'=>time()+999, 'secure'=>1, 'domain'=>'example.com']);
setcookie('A', 'B', ['httponly'=>1] );
setcookie('A', 'B', 999);
setcookie('A', 'B', time()+999);

echo 'OK';
[yohgaki@dev github-php-src]$ ./php-cgi t13.php
X-Powered-By: PHP/7.2.0-dev
Set-Cookie: A=B; expires=Fri, 21-Oct-2016 02:55:31 GMT; Max-Age=999;
path=foo; domain=example.com; secure; HttpOnly
Set-Cookie: A=B; HttpOnly
Set-Cookie: A=B; expires=Thu, 01-Jan-1970 00:16:39 GMT; Max-Age=-1477016533
Set-Cookie: A=B; expires=Fri, 21-Oct-2016 02:55:31 GMT; Max-Age=999
Content-type: text/html; charset=UTF-8

OK

If PHP has named parameter, we don't need this patch.
Dose anyone working on named parameter?

One issue of this patch is strict types. It ruins strictly typed
parameter because array option parameters won't be checked by PHP.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stephen Reay — view source

unread

Is it normal to alter (or support multiple) function signatures like this, when you want to improve the name and improve the signature? Wouldn’t you just leave setcookie() as-is, introduce the new cookie_* functions, and then deprecate set cookie later? (ala mysql => mysqli)

As for the specifics - I kind of like.. Niklas (I think?) suggestion where the flags array accepts either key => value pairs, or non-keyed flag values. Any non-string key is ignored and the value used as a ‘flag’ (e.g. HttpOnly, Secure). Any non-string value would be casted to string.

This would obviously require slightly different usage by developers - the user would need to send a date/time (either a string or object that will cast to a string in the right format) instead of a timestamp for expires. If you want to special-case it, you could type-check for an instance of \DateTimeInterface and run ->format(\DateTime::COOKIE) instead of just casting to string, but I don’t think I’d consider that to be essential really. If the user can generate a UNIX timestamp, they should be able to format it to RFC1123 themselves too, no?

While you’re looking at this. DateTime::COOKIE (and DATE_COOKIE) seem to be using RFC850 format, but with a 4-digit year. Besides being a bit of a mis-match of formats, RFC850 is considered “obsolete” now, and perhaps should be replaced by RFC1123 (basically, no dashes, short day name).

Cheers

Stephen

Before we even discuss disallowing header("set-cookie"), we should have a
sane cookie API, e.g. one that like setcookie($name, $value, $flags).

That's also the way we implemented it in Aerys
(https://github.com/amphp/aerys/blob/9a7327f062aa678408dfe4f4c3c7f479db16f187/lib/Response.php#L49-L58).
It's a simple wrapper around addHeader to make life easier, but it doesn't
restrict developers to call setHeader and replace all set-cookie
headers.

We choose current API for reason. It does not look pretty.
This is patch allow array config for 3rd param for setcookie().

https://gist.github.com/yohgaki/b86e07cd450777422c1a467166cd2fd3

I suppose some of us will have opinions having this kind of code(s).

Any comments?

Execution example.

[yohgaki@dev github-php-src]$ cat t13.php
<?php
setcookie('A', 'B', ['httponly'=>1, 'path'=>'foo',
'expires'=>time()+999, 'secure'=>1, 'domain'=>'example.com']);
setcookie('A', 'B', ['httponly'=>1] );
setcookie('A', 'B', 999);
setcookie('A', 'B', time()+999);

echo 'OK';
[yohgaki@dev github-php-src]$ ./php-cgi t13.php
X-Powered-By: PHP/7.2.0-dev
Set-Cookie: A=B; expires=Fri, 21-Oct-2016 02:55:31 GMT; Max-Age=999;
path=foo; domain=example.com; secure; HttpOnly
Set-Cookie: A=B; HttpOnly
Set-Cookie: A=B; expires=Thu, 01-Jan-1970 00:16:39 GMT; Max-Age=-1477016533
Set-Cookie: A=B; expires=Fri, 21-Oct-2016 02:55:31 GMT; Max-Age=999
Content-type: text/html; charset=UTF-8

OK

If PHP has named parameter, we don't need this patch.
Dose anyone working on named parameter?

One issue of this patch is strict types. It ruins strictly typed
parameter because array option parameters won't be checked by PHP.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

Is it normal to alter (or support multiple) function signatures like this, when you want to improve the name and improve the signature? Wouldn’t you just leave setcookie() as-is, introduce the new cookie_* functions, and then deprecate set cookie later? (ala mysql => mysqli)

I'm lazy enough not to add new function entry point because the patch
is to show how it will look like.

Making aliases make life a little easier for both user and developer.
I don't think we will deprecate (I mean deprecate and remove in the
future) old functions. It will be there to avoid needless
incompatibility.

As for the specifics - I kind of like.. Niklas (I think?) suggestion where the flags array accepts either key => value pairs, or non-keyed flag values. Any non-string key is ignored and the value used as a ‘flag’ (e.g. HttpOnly, Secure). Any non-string value would be casted to string.

This would obviously require slightly different usage by developers - the user would need to send a date/time (either a string or object that will cast to a string in the right format) instead of a timestamp for expires. If you want to special-case it, you could type-check for an instance of \DateTimeInterface and run ->format(\DateTime::COOKIE) instead of just casting to string, but I don’t think I’d consider that to be essential really. If the user can generate a UNIX timestamp, they should be able to format it to RFC1123 themselves too, no?

I have an idea to enable HttpOnly by default. Most applications will
be safer by this change without any problems, but some applications
may need to disable HttpOnly. So it's better to stick with key =>
value pairs in case we change the default.

I cannot think of date generation use case. Is there good use case?

While you’re looking at this. DateTime::COOKIE (and DATE_COOKIE) seem to be using RFC850 format, but with a 4-digit year. Besides being a bit of a mis-match of formats, RFC850 is considered “obsolete” now, and perhaps should be replaced by RFC1123 (basically, no dashes, short day name).

Good idea. It's not updated since 90's, I guess. The same topic pops
up on occasion.

https://tools.ietf.org/html/rfc6265#section-5.1.1
Do we have this algorithm somewhere already?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Lester Caine — view source

unread

Is it normal to alter (or support multiple) function signatures like this, when you want to improve the name and improve the signature? Wouldn’t you just leave setcookie() as-is, introduce the new cookie_* functions, and then deprecate set cookie later? (ala mysql => mysqli)

Or perhaps a new object model for managing header that can be developed
along side the procedural model ... what is being proposed for
setcookie() certainly seems more in line with that style of programming?

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

8 years ago by Yasuo Ohgaki — view source

unread

Hi Niklas,

016-10-20 11:57 GMT+02:00 Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Niklas,

same here, it's not acceptable to limit header and restrict
set_cookie.
Just think about all those frameworks that would have to specialcase
setting
headers now and have to use the cookie API then.

If you want to protect the session cookie header, why not simply set it
right before the first output? That'd make it also non-overrideable, but
leaves header() intact. But I guess it's harder to implement.

Although, I prefer to have completely separate API, we have to
implement vote result. So vote no for "Disabling 'Set-Cookie' for
header*()" vote option.

I don't have a vote. But this breaks BC. It might remove surprisings when
using sessions, but having header() not being able to set set-cookie
headers adds new surprisings.

That's why we have minor releases and document.

Besides, I noted "add API 7.x, disallow header('Set-Cookie') by 8.0".
There are years to prepare for the change even if we decide to do so.

Regarding about delaying session cookie header, it is possible to use
output buffer to delay output so that session module can send HTTP
header at request shutdown. However, it will break almost all session
enabled applications that require immediate output. Therefore, it's
easy to implement, but not possible for this reason.

I meant squeeze in right before output or on first flush() call. There must
be a thing that sets a "already output" flag that prevents further headers.
We could use that mechanism to buffer all headers and just send them out
there and have a hook for the session module.

The reason why I think of this new API proposal is to remove
core(main/SAPI.c) dependency to session. Your idea requires the
dependency (main/SAPI.c) like my the other proposal. Technically, it's
possible and it can be simpler by detecting and preventing session ID
deletion/modification.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Stanislav Malyshev — view source

unread

Hi!

The idea is to separate HTTP header handling functions.

header*() for any HTTP headers except 'Set-Cookie'

cookie*() for only 'Set-Cookie' header

This does not look like a good design. First of all, HTTP spec allows
multiple instances of any header. Second, making function with unobvious
gotcha branch is usually bad design. Third, we are solving non-existing
problem here - people should just use existing functions correctly and
everything would be fine.
Let's not spend any more time on this.

--
Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stats,

The idea is to separate HTTP header handling functions.

header*() for any HTTP headers except 'Set-Cookie'

cookie*() for only 'Set-Cookie' header

This does not look like a good design. First of all, HTTP spec allows
multiple instances of any header. Second, making function with unobvious
gotcha branch is usually bad design. Third, we are solving non-existing
problem here - people should just use existing functions correctly and
everything would be fine.
Let's not spend any more time on this.

OK. 4 people not in favor of Set-Cookie restriction for header*().

What about API clean up?
Since we have setcookie()/setrawcookie() already, we may clean up
current cookie API

e.g.

cookie_set/setcookie($name, [$value, [array $options]])
(Keep current signature also)
cookie_set_raw/setrawcookie($name, [$value, [array $options]])
(Keep current signature also)
cookie_remove($name), cookie_list()
(These may be optional to you)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Guy Marriott — view source

unread

FWIW Yasuo, I also think this is a bad idea. If you remove the ability to
set cookie headers with the header function then the function needs a
more appropriate name - perhaps headerExceptCookie.

That makes 5 people opposed - 100% of the individuals who have responded in
this thread.

Hi Stats,

On Fri, Oct 21, 2016 at 5:54 AM, Stanislav Malyshev smalyshev@gmail.com
wrote:

The idea is to separate HTTP header handling functions.

header*() for any HTTP headers except 'Set-Cookie'

cookie*() for only 'Set-Cookie' header

This does not look like a good design. First of all, HTTP spec allows
multiple instances of any header. Second, making function with unobvious
gotcha branch is usually bad design. Third, we are solving non-existing
problem here - people should just use existing functions correctly and
everything would be fine.
Let's not spend any more time on this.

OK. 4 people not in favor of Set-Cookie restriction for header*().

What about API clean up?
Since we have setcookie()/setrawcookie() already, we may clean up
current cookie API

e.g.

cookie_set/setcookie($name, [$value, [array $options]])
(Keep current signature also)

cookie_set_raw/setrawcookie($name, [$value, [array $options]])
(Keep current signature also)

cookie_remove($name), cookie_list()
(These may be optional to you)

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Rick Widmer — view source

unread

FWIW Yasuo, I also think this is a bad idea. If you remove the ability to
set cookie headers with the header function then the function needs a
more appropriate name - perhaps headerExceptCookie.

That makes 5 people opposed - 100% of the individuals who have responded in
this thread.

8 years ago by Stanislav Malyshev — view source

unread

Hi!

What about API clean up?
Since we have setcookie()/setrawcookie() already, we may clean up
current cookie API

e.g.

cookie_set/setcookie($name, [$value, [array $options]])
(Keep current signature also)

cookie_set_raw/setrawcookie($name, [$value, [array $options]])
(Keep current signature also)

These two already exist, and I see little value in changing their names
or introducing a slightly different form of it.

cookie_remove($name), cookie_list()
(These may be optional to you)

Isn't it the same as setcookie with empty value? But in general, I don't
have much objection to this.

--
Stas Malyshev
smalyshev@gmail.com

8 years ago by Yasuo Ohgaki — view source

unread

Hi Stephen,

Hi Stephen,

Is it normal to alter (or support multiple) function signatures like this, when you want to improve the name and improve the signature? Wouldn’t you just leave setcookie() as-is, introduce the new cookie_* functions, and then deprecate set cookie later? (ala mysql => mysqli)

I'm lazy enough not to add new function entry point because the patch
is to show how it will look like.

Making aliases make life a little easier for both user and developer.
I don't think we will deprecate (I mean deprecate and remove in the
future) old functions. It will be there to avoid needless
incompatibility.

I’m not quite sure I understand. I thought the whole point of new standardised function names is to allow eventual deprecation and removal of the inconsistent/confusing names?

In PHP4, we might have removed some functions on occasion. I don't
remember well. We have removed really unneeded functions in PHP5 like
register_globals related functions. I renamed number of pgsql
functions in PHP4, but I don't remove any aliases and I probably will
not.

Alias removal is very unlikely but possible. However, we have RFC for
such changes. Chance for removing setcookie() is almost none. We
cannot remove even problematic uniqid() probably.

Regarding to have array parameters, session_start() is made to accept
array parameter like this, but session_start() accepts void parameter
before. Array is used because it's very annoying to have setcookie()
like parameters.

As for the specifics - I kind of like.. Niklas (I think?) suggestion where the flags array accepts either key => value pairs, or non-keyed flag values. Any non-string key is ignored and the value used as a ‘flag’ (e.g. HttpOnly, Secure). Any non-string value would be casted to string.

This would obviously require slightly different usage by developers - the user would need to send a date/time (either a string or object that will cast to a string in the right format) instead of a timestamp for expires. If you want to special-case it, you could type-check for an instance of \DateTimeInterface and run ->format(\DateTime::COOKIE) instead of just casting to string, but I don’t think I’d consider that to be essential really. If the user can generate a UNIX timestamp, they should be able to format it to RFC1123 themselves too, no?

I have an idea to enable HttpOnly by default. Most applications will
be safer by this change without any problems, but some applications
may need to disable HttpOnly. So it's better to stick with key =>
value pairs in case we change the default.

That’s a good point. So perhaps anything with a boolean value becomes a keyword only? (i.e. keep it generic, with as little special-case handling of keywords/named values as possible)

Oops, I mixed up session cookie and other cookie. I do have an idea to
set HttpOnly by default for session ID cookie, but not for other
cookies set by setcookie(). Rather than HttpOnly, we may have to
consider enabling "Secure" for setcookie() by default in near future.
Almost all web will enable TLS soon.

https://github.com/amphp/aerys/blob/9a7327f062aa678408dfe4f4c3c7f479db16f187/lib/Response.php#L49-L58
This is interface, so I cannot tell what it mean by "unkeyed values".
['HttpOnly'] means enable 'HttpOnly', probably.
(I'm willing to add new function entry points for consistency, but I
think API itself should be as simple as possible. It may sounds
contradictory with my filter validation improvement RFC, but the
proposal was made so that only implements mandatory API changes for
the purpose)

I would like to keep minimum feature, consistency and expandability.
'option_name' => 'option_value' format should be kept access all
functions. We do change some flags on occasion. Not only flipping
true/false default, but also bool to integer. i.e. on/off flags to
number of int constant options.

I cannot think of date generation use case. Is there good use case?

What do you mean? You can’t think of a situation where the developer would be setting a cookie expiry date, or you can’t think of a reason to cast a date time object to a string?

OK. You would like to use DateTime object's string.

Although we don't follow current RFC, but RFC has very specific rule
for cookie's expiration time range and format. Allowing string
expiration date is problematic and DateTime::timestamp() can be used
to set proper UNIX timestamp. So current way is good.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

header() removes all header of the same name.

I'm about to create new feature request as follows:

Protect session ID and other cookies from header(), header_remove()

Protect Set-Cookie headers from header() and header_remove()

Thanks.

I'm about to create new feature request as follows:

Protect session ID and other cookies from header(), header_remove()

Protect Set-Cookie headers from header() and header_remove()

I'm about to create new feature request as follows:

Protect session ID and other cookies from header(), header_remove()

Protect Set-Cookie headers from header() and header_remove()

-- Lester Caine - G8HFL

Protect session ID and other cookies from `header()`, `header_remove()`

Protect Set-Cookie headers from `header()` and `header_remove()`

Protect session ID and other cookies from `header()`, `header_remove()`

Protect Set-Cookie headers from `header()` and `header_remove()`

Protect session ID and other cookies from `header()`, `header_remove()`

Protect Set-Cookie headers from `header()` and `header_remove()`

--
Lester Caine - G8HFL