Handle request headers with numeric keys

[weierophinney]

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.

---

Originally posted by @jeshuaborges at zendframework/zend-diactoros#286

[weierophinney]

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

---

Originally posted by @Xerkus at zendframework/zend-diactoros#286 (comment)

[weierophinney]

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

---

Originally posted by @jeshuaborges at zendframework/zend-diactoros#286 (comment)

[weierophinney]

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

https://github.com/zendframework/zend-diactoros/blob/817def741b4e7f41e5eae9b9adc117e33323d6bf/src/HeaderSecurity.php#L159

---

Originally posted by @Xerkus at zendframework/zend-diactoros#286 (comment)

[weierophinney]

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

---

Originally posted by @Xerkus at zendframework/zend-diactoros#286 (comment)

[weierophinney]

@Xerkus Yes.

---

Originally posted by @jeshuaborges at zendframework/zend-diactoros#286 (comment)

[weierophinney]

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

---

Originally posted by @Xerkus at zendframework/zend-diactoros#286 (comment)

[weierophinney]

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.

---

Originally posted by @Xerkus at zendframework/zend-diactoros#286 (comment)

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

---

Originally posted by @weierophinney at zendframework/zend-diactoros#286 (comment)

[weierophinney]

@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

---

Originally posted by @Xerkus at zendframework/zend-diactoros#286 (comment)

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

---

Originally posted by @weierophinney at zendframework/zend-diactoros#286 (comment)

[weierophinney]

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

---

Originally posted by @Xerkus at zendframework/zend-diactoros#286 (comment)

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

---

Originally posted by @weierophinney at zendframework/zend-diactoros#286 (comment)

[weierophinney]

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

---

Originally posted by @jeshuaborges at zendframework/zend-diactoros#286 (comment)

[Xerkus]

Tentatively putting this on 3.0.0 milestone.

Proposed partial mitigation for the behavior is to filter out integer numeric headers names in Laminas\Diactoros\marshalHeadersFromSapi() function but otherwise leave it unchanged.

@weierophinney I would like to ask you to propose errata for PSR-7 outlining this quirk of PHP and its effect on headers. PHP bug tracker link for the behavior is https://bugs.php.net/bug.php?id=80309

[weierophinney]

If we want to do this based on PSR-7 errata, we should release 3 first, and not wait; errata takes months, minimum, to work through the process.

More interesting... I'm not sure that we can do anything in marshalHeadersFromSapi(). We check for a string key, because in the SAPI, all headers are prefixed with one of HTTP_, REDIRECT_ or CONTENT_. We then capture the remaining part of the string, and translate all _ characters to -, before using the value as a header name. In other words, the issue is not during marshaling, it's when we validate.

What we could potentially do is modify HeaderSecurity::assertValidName() to not immediately invalidate if the value is numeric. In those cases, we would cast to string, and run our normal regex on it, which would find it to be valid.

I'm going to go with this approach, as it would solve the OP's issue, conforms to RFC-7230, and would not require any errata.

[Xerkus]

In marshalHeadersFromSapi() once headers are processed they can be filtered to remove non-string keys

There is already partial fix that skips numeric keys but it does not account for keys converting to int after prefixes were stripped.

laminas-diactoros/src/functions/marshal_headers_from_sapi.php

Lines 33 to 35 in 6fce157

	if (! is_string($key)) {
	continue;
	}

[weierophinney]

There is already partial fix that skips numeric keys but it does not account for keys converting to int after prefixes were stripped.

You're missing the order of operations. That function is looking at the $_SERVER array, not a list of headers, and we loop through it to find keys that begin with the designated prefixes as I noted in my previous comment. All header names it produces are strings. PHP then casts any strings that also evaluate to integers... to integers, and it's then HeaderSecurity::assertValidName() that flags them as invalid. That's why the proper fix is in HeaderSecurity.

[Xerkus]

That function is looking at the $_SERVER array, not a list of headers,

You are correct. Sorry, I am too tired and distracted.

What I had in mind is to filter out the array of headers produced from SAPI to prevent type errors that would prevent request instance creation.

Once HTTP_1 is stripped it becomes numeric and converts to int as soon as it is assigned to array.

So in array of headers here we can filter out anything that turned into int:

laminas-diactoros/src/functions/marshal_headers_from_sapi.php

Line 66 in 6fce157

return $headers;

If HeaderSecurity were to assert name is not integer-like that would protect from spurious type errors in other cases.