Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #76688 - Disallow excessive parameters after options array #3424

Closed
wants to merge 2 commits into from

Conversation

pmmaga
Copy link
Contributor

@pmmaga pmmaga commented Aug 1, 2018

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.

@pmmaga pmmaga changed the base branch from master to PHP-7.3 August 1, 2018 21:10
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

domain = NULL;
secure_null = 1;
httponly_null = 1;
php_error_docref(NULL, E_WARNING, "Arguments passed after the options array are ignored");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they should be ignored. This should be a return false condition at least.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On closer consideration, I agree with @nikic. We should treat the functions like they were overloaded (i.e. support two different signatures), and let them fail on excess arguments.

@pmmaga pmmaga changed the title Fix #76688 - Ignore excessive parameters after options array Fix #76688 - Disallow excessive parameters after options array Aug 3, 2018
@pmmaga
Copy link
Contributor Author

pmmaga commented Aug 3, 2018

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.

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine now.

@cmb69
Copy link
Member

cmb69 commented Aug 5, 2018

Since it wasn't added before, please add a NEWS entry for the samesite cookie.

Should we really duplicate info in NEWS and UPGRADING?

@pmmaga
Copy link
Contributor Author

pmmaga commented Aug 6, 2018

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)

@cmb69
Copy link
Member

cmb69 commented Aug 7, 2018

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.

@cmb69
Copy link
Member

cmb69 commented Aug 8, 2018

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--

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we use a --EXPECTHEADERS-- empty section, what are we trying to achieve?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@php-pulls
Copy link

Comment on behalf of cmb at php.net:

Applied via a16aee6.

@php-pulls php-pulls closed this Aug 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants