Fix #76688 - Disallow excessive parameters after options array#3424
Fix #76688 - Disallow excessive parameters after options array#3424pmmaga wants to merge 2 commits intophp:PHP-7.3from
Conversation
ext/session/session.c
Outdated
| domain = NULL; | ||
| secure_null = 1; | ||
| httponly_null = 1; | ||
| php_error_docref(NULL, E_WARNING, "Arguments passed after the options array are ignored"); |
There was a problem hiding this comment.
I don't think they should be ignored. This should be a return false condition at least.
pmmaga
changed the title
|
As suggested, I've changed it so it returns false and the cookie is not set in this case. PS: Since it wasn't added before, please add a NEWS entry for the samesite cookie. |
Should we really duplicate info in NEWS and UPGRADING? |
|
Given that the information ends up in two different places (ie. http://php.net/migration72 and http://php.net/ChangeLog-7.php#7.2.0) I think it makes sense to mention it on both. If we pick one or the other, the changelog would miss some important entries. (Unless there is a manual task that I am not aware of) |
|
To my knowledge, only few of the entries in UPGRADING/the migration guide are listed in NEWS/the changelog. I'm not really sure, how this is supposed to be handled; I've just asked on internals. |
|
According to Stas' reply which apparently matches our practise, I've added a respective NEWS entry. If there are no objections (and nobody beats me to it), I shall merge this PR on the weekend (so it'll make it into PHP 7.3.0beta2). |
| var_dump(headers_list()); | ||
|
|
||
| --EXPECTHEADERS-- | ||
|
|
There was a problem hiding this comment.
When we use a --EXPECTHEADERS-- empty section, what are we trying to achieve?
There was a problem hiding this comment.
This is so the test is executed with the CGI binary. With the CLI the headers are ignored and the return of headers_list() would be empty.
|
Comment on behalf of cmb at php.net: Applied via a16aee6. |
5 participants
Link for bugsnet: https://bugs.php.net/bug.php?id=76688
I can create a different PR for the first commit but that's something that should also be fixed. There is no good reason to not set the cookie even if no valid option was found in the array.