Handle request headers with numeric keys about laminas-diactoros HOT 18 CLOSED

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)

from laminas-diactoros.

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

from laminas-diactoros.

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

from laminas-diactoros.

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

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

from laminas-diactoros.

@Xerkus Yes.

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

from laminas-diactoros.

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

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

from laminas-diactoros.

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)

from laminas-diactoros.

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)

from laminas-diactoros.

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

from laminas-diactoros.

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)

from laminas-diactoros.

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)

from laminas-diactoros.

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

from laminas-diactoros.

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

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

from laminas-diactoros.

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

from laminas-diactoros.

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.

from laminas-diactoros.

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.

from laminas-diactoros.

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.

from laminas-diactoros.

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:

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

from laminas-diactoros.