Handle request headers with numeric keys

[jeshuaborges]

Here, at getpocket.com, we have had a client hit our servers with header keys as integers. In doing so HeaderSecurity::assertValidName is throwing an exception because ! is_string(-1) === true.

I have been unable to identify any documentation which would suggest that these values could be valid. At this point I believe the best behavior is to ignore these keys.

[Xerkus]

Note: technically, -1 is a valid header name.

RFC7230 defines it as following:

header-field = field-name ":" OWS field-value OWS
field-name = token
token = 1*tchar
tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / 
    "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

[jeshuaborges]

@Xerkus in that case, do you think that I should update HeaderSecurity::assertValidName to allow for numeric keys?

[Xerkus]

@jeshuaborges looks like there is a bug, this pattern should allow it

zend-diactoros/src/HeaderSecurity.php

Line 159 in 817def7

if (! preg_match('/^[a-zA-Z0-9\'`#$%&*+.^_|~!-]+$/', $name)) {

[Xerkus]

Could you open a new PR with a test for all those allowed characters?

[jeshuaborges]

@Xerkus Yes.

[Xerkus]

Actually, what was exact error? Was it Invalid header name type; expected string;?

[Xerkus]

I bet header names are used as array keys and php converts them to integers if they are numeric, so it is not a pattern issue. It is actually more serious.

@weierophinney you need to take a look at this.

I do not think there is a practical use for numeric headers, so omitting them might be acceptable.

[weierophinney]

The test and code change are incorrect. We need to instead modify HeaderSecurity::assertValidName() to allow numeric values, and cast them to a string to ensure they fit the pattern.

Essentially, HeaderSecurity::assertValidName would have the following contents:

if (! is_string($name) && ! is_numeric($name)) {
    throw new InvalidArgumentException(sprintf(
        'Invalid header name type; expected string or number; received %s',
        is_object($name) ? get_class($name) : gettype($name)
    ));
}
if (! preg_match('/^[a-zA-Z0-9\'`#$%&*+.^_|~!-]+$/', (string) $name)) {
    throw new InvalidArgumentException(sprintf(
        '"%s" is not a valid header name',
        $name
    ));
}

Tests would then need to include cases for integers both negative and positive, and floats, both negative and positive.

[weierophinney]

Based on RFC 7230, this should be interpreted as the following header:

-1: -1

since - and numeric characters are all valid for header names. As such, this test is not correct.

I just added the above to the current test suite, and then updated the $expected value to include a '-1' => '-1' case, and it passes as expected.

[weierophinney]

I do not think there is a practical use for numeric headers, so omitting them might be acceptable.

It's not up to us to decide what's acceptable; RFC 7230 decides that, and numeric header names are clearly indicated as acceptable by that specification. We just need to fix the assertValidName() code to comply.

[Xerkus]

@weierophinney integer keys with strict types enabled would be a bad breaking surprise for consumers.
Numeric keys being converted to integers and then handled as non-assoc are also a bad surprise with unexpected behavior. We can't really do anything about it the way HTTP Messages PSR defines interfaces, PHP itself doing bad thing here

For example: https://3v4l.org/XUj6h

It's not up to us to decide what's acceptable; RFC 7230 decides that, and numeric header names are clearly indicated as acceptable by that specification.

In that case issue about possibility of integer keys have to be elevated to FIG, for the warning to be included in PSR-7 and interface typehints to be amended

[weierophinney]

integer keys with strict types enabled would be a bad breaking surprise for consumers.
Numeric keys being converted to integers and then handled as non-assoc are also a bad surprise with unexpected behavior.

I was having trouble understanding your argument until I followed the 3v4l link; that was enlightening.

So, what do you propose we do, @Xerkus ?

[Xerkus]

I never found a workaround to force string numerics as keys, and it bit me a number of times in the past.

My cases I solved by hacks: changed code to have all keys prefixed with a static string, it is not possible in his case

[weierophinney]

@jeshuaborges For the interim, what I would suggest is doing some pre-processing of $_SERVER before the request is created, scrubbing out any keys matching /^HTTP__\d/ or /^HTTP_\d/. We'll continue brainstorming ideas in the meantime so we can come up with a more robust solution.

[jeshuaborges]

@weierophinney @Xerkus Thank you both for looking into this and circling back.

[weierophinney]

This repository has been closed and moved to laminas/laminas-diactoros; a new issue has been opened at laminas/laminas-diactoros#11.

[weierophinney]

This repository has been moved to laminas/laminas-diactoros. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

• Squash all commits in your branch (git rebase -i origin/{branch})
• Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
• Run the laminas/laminas-migration tool on the code.
• Clone laminas/laminas-diactoros to another directory.
• Copy the files from the second bullet point to the clone of laminas/laminas-diactoros.
• In your clone of laminas/laminas-diactoros, commit the files, push to your fork, and open the new PR.
  We will be providing tooling via laminas/laminas-migration soon to help automate the process.