Allow integers as cookie names

[nosilver4u]

Fixes https://github.com/WordPress/Requests/issues/845 so that an exception isn't thrown for legitimate numeric cookie names.

Changes for both the constructor and parse() methods:

• Modifies the check(s) for $name, to allow both string and integer cookie names.
• Changes the exception thrown to indicate a string OR int is permitted.
• Updates doc blocks to allow cookie names to be integers.
• Modifies unit tests for $name parameter to allow strings and integers, and to validate the updated exception messages.

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

• [x] I have checked there is no other PR open for the same change.

This is a:

• [x] Bug fix
• [ ] New feature
• [ ] Documentation improvement
• [ ] Code quality improvement

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

• [x] This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
• [x] I have commented my code, particularly in hard-to-understand areas.
• [x] I have added unit tests to accompany this PR.
• [x] The (new/existing) tests cover this PR 100%.
• [x] I have (manually) tested this code to the best of my abilities.
• [x] My code follows the style guidelines of this project.

[schlessera]

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:

• a cookie-pair is a cookie-name followed by = followed by a cookie-value
• a cookie-name is a token as defined in RFC 2616, section 2.2

Then, in section 2.2 of RFC 2616, we have the following specification:

• a token is a series of <any CHAR except CTLs or separators>
• a CHAR is <any US-ASCII character (octets 0 - 127)>
• a CTL is <any US-ASCII control character (octets 0 - 31) and DEL (127)>
• a separator is "(" | ")" | "<" | ">" | "@" | "," | ";" | ":" | "\" | <"> | "/" | "[" | "]" | "?" | "=" | "{" | "}" | SP | HT
• a SP is <US-ASCII SP, space (32)>
• a HT is <US-ASCII HT, horizontal-tab (9)>
• a <"> is <US-ASCII double-quote mark (34)>

The above provides specific rules on how to parse a cookie for validity. I suggest we add something like InputValidator::is_valid_rfc2616_token() that we then use to replace the current is_string() validation check in the cookie constructor.

[jrfnl]

PR #849 has been opened for the input validation functionality. Once that is accepted, solving this should be more straight forward.

[jrfnl]

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

[nosilver4u]

@jrfnl Thanks for the update, I'll take a look later this week and see what I can do!

[nosilver4u]

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()?

[jrfnl]

@nosilver4u I've just rebased the PR without changes to allow the PR to get a running (and hopefully passing!) build.

[schlessera]

Tests are failing right now, we're already looking into this and will push a change soon.

[jrfnl]

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.

That is a correct change as the cookie name should always be a string, though with the current PR, an allowance is added for it being passed as an integer. The name should still be a string for further processing though.

[jrfnl]

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()?

Well, the tests for the InputValidator::is_valid_rfc2616_token() method don't need to be duplicated, but we do need to make sure that method is used to do the input validation. Commit https://github.com/WordPress/Requests/pull/847/commits/bb82ab043cd9ab891da1256010c2e159f1b31dbe fixes this up.

[schlessera]

@nosilver4u Unless you have any other feedback or objections, we'll merge this as it currently is in the coming days. Thanks for your work on this!

[nosilver4u]

I've verified my local test still works with the changes, so it all looks good to me!

[jrfnl]

Thanks for testing @nosilver4u !

I've rebased to get rid of the backmerge and get a passing build again. Will merge once the build has passed.

[nosilver4u]

Thanks for testing @nosilver4u !

I've rebased to get rid of the backmerge and get a passing build again. Will merge once the build has passed.

Yeah, sorry for the noise, I was just trying to get my repo up to date and didn't realize it was pushing that upstream!

[jrfnl]

CodeCov is having trouble. Merging now anyway.