-
Notifications
You must be signed in to change notification settings - Fork 495
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
Allow integers as cookie names #847
base: develop
Are you sure you want to change the base?
Allow integers as cookie names #847
Conversation
Modify data validation checks, exception messages, and docblocks to allow cookie names to be integers. This affects both the constructor and the parse() method. Also modified unit tests to allow strings and integers, and to validate the updated exception messages.
It looks like our cookie parsing has been too loose in terms of what it expects. However, instead of now adding integers and still not properly reflecting the RFC specification, I'd suggest we take a close look at what the RFC for cookies actually states and implement that instead (which will include integers, but also other characters that should work but currently don't). In section 4.1.1 of the RFC 6265, the syntax sepcification states the following:
Then, in section 2.2 of RFC 2616, we have the following specification:
The above provides specific rules on how to parse a cookie for validity. I suggest we add something like |
PR #849 has been opened for the input validation functionality. Once that is accepted, solving this should be more straight forward. |
@nosilver4u PR #849 has now been merged, which should make it more straight-forward and stable to make the change you are proposing. Would you like to update your PR to start using the method as introduced in #849 ? Or do you expect us to get your PR to a mergable state ? |
@jrfnl Thanks for the update, I'll take a look later this week and see what I can do! |
Updated the Cookie token/name validation to use the new InputValidator::is_valid_rfc2616_token() method, updated the exception messages, and modified the test cases to look for the updated exceptions.
I updated the validation to use the new InputValidator::is_valid_rfc2616_token() method from #849. One thing that could potentially be improved would be to use some of the test data written by @schlessera in the constructor/parse tests. However, I wasn't sure if it was appropriate to re-use the IsValidRfc2616TokenTest::dataInvalidValues() method. For example, would it make sense to add a dataInvalidNameValues() method to the ConstructorTest class that is basically just a wrapper for IsValidRfc2616TokenTest::dataInvalidValues()? |
Fixes #845 so that an exception isn't thrown for legitimate numeric cookie names.
Changes for both the constructor and parse() methods:
I have kept Cookie->name as a string, and cast $name to a string within the constructor, just in case some (external) code would be doing a strict comparison against the name, and expecting a string value. If that's not desirable, I can revert that change.
Pull Request Type
This is a:
Context
Currently, multiple plugins, and possibly even WP core are throwing errors if someone has a cookie with a numeric name. Would love to see this issue go away, so that folks don't have to resort to deleting cookies in the dev tools of their browser.
Detailed Description
This issue typically comes up in plugins that are executing 'async' requests, such that the existing cookies ($_COOKIES) are copied into the 'cookies' parameter for a GET/POST request. One could sanitize the numeric key names at that point, but PHP will cast the indices back to integers, and then an exception will still be thrown.
Quality assurance