Headers with value 0 being stripped out. Previous bugfix caused all headers to be accepted.

[bluebaroncanada]

Provide a narrative description of what you are trying to accomplish:

• Are you fixing a bug?
  • Detail how the bug is invoked currently.

$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, 'http://xxx.xxx.xxx');
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "PATCH");
curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
curl_setopt($ch, CURLOPT_HEADER, true);
curl_setopt($ch, CURLOPT_HTTPHEADER, ["Upload-Offset: 0"]);
curl_exec($ch);

• Detail the original, incorrect behavior.
  The header Upload-Offset is being stripped. (Any header with value "0")
  The value was being used in a conditional. To test !!("0") === false.
  Test was incorrect because the header was being stripped because it was not prefixed with HTTP_.
  Relates to 500 error with Apache #347 ServerRequest: Header with value "0" discarded #342 Bugfix: Header with value "0" is being discarded #344
• Detail the new, expected behavior.
  Values with a string of "0" should be accepted
  Test passes with proper header formation.
• Base your feature on the master branch, and submit against that branch.
• Add a regression test that demonstrates the bug, and proves the fix.
• Add a CHANGELOG.md entry for the fix.

[bluebaroncanada]

This is the if statement that filters out headers that don't begin with HTTP_ in marshal_headers_from_sapi.php on line 38:

        if ($value && strpos($key, 'HTTP_') === 0) {

The problem with that is that if $value === "0" the statement will return false, which is the original reported issue. You can test the truth of this by observing !!("0")===false.
My fix is to change that line to

        if (($value || $value === "0") && strpos($key, 'HTTP_') === 0) {

which accounts for the possibility that the value could be "0", which is still a valid value. It actually might be better to say !empty($value) but I don't know what other side effects that might have.

I then changed your test header in from Accept to HTTP_ACCEPT and the assertion header to ACCEPT. (The HTTP_ gets stripped away.)

Maybe it is better to say !empty($value) because maaaaaaaaaaaybe you want to keep "false". Then again maybe not?

It could break anything that expected "false" to not return a header. I doubt that's the behaviour we would want, though. It could be possible other people are negatively affected by this. For right now there's no issue posted with "false", so I think this pull should stand.

[michalbundyra]

@bluebaroncanada I think it would be good to add tests for all cases you described in your comment. It could prevent the issue in the future.

[bluebaroncanada]

@webimpress Then I have to decide and I hesitate to be the one to decide because I'm not the original author and I don't know his or her intention.
Calling @weierophinney

[bluebaroncanada]

@weierophinney Should headers with values of "false" or "0" be kept. The original behaviour is to discard because we're checking the truth of $value which is a string.
Observe that !!("0") === !!("false") === false.

More information: http://php.net/manual/en/function.boolval.php

[bluebaroncanada]

Well colour me stupid. !!("false") is true. I thought I had read it was false. Reading less stupidly, I realize it's not listed in that document and I ran it through a tester and it came out true.
Still, your attention to this bug might be helpful.

[michalbundyra]

@bluebaroncanada As you are submitting the PR you have to decide how it should form in your opinion. And if you demonstrate it by tests others can see if it makes sense or not. I would change the condition to be $value !== '' and then add tests to cover it. So 0 and false will be kept and we should have tested these cases. Also we would need test with empty value.

[bluebaroncanada]

• Changed checking to $value !== ''
• Any talk about 'false' in this thread can be ignored
• Added new tests to explicitly test that server variables not prefixed with HEADER_ or CONTENT_ are stripped
• Also applied the new checking method to CONTENT_ prefixed variables
• Added a second assertion to check that CONTENT_ variables with value of '0' are kept

These measures should prevent the critical bug from occurring again.

[bluebaroncanada]

• Added test to ensure that server keys with empty values are being omitted from ServerRequestFactory::fromGlobals()

[michalbundyra]

@bluebaroncanada This is exactly what I've expected. Well done! LGTM 👍

[bluebaroncanada]

Thanks for the help!

[geerteltink]

I have just tested this locally and it fixes the issues mentioned in #350.

[geerteltink]

Until this is merged you can use this to skip affected versions: "zendframework/zend-diactoros": "^2.0 !=2.0.2 !=2.1.0",

[weierophinney]

This looks good. I'm going to squash the commits so that I can more easily cherry-pick the changes and create a 2.0.3 release; I'll push those back to your branch before merging.

[bluebaroncanada]

How do you do that exactly? Would like to know. On Sat, 5 Jan 2019 at 14:50, weierophinney <notifications@github.com<mailto:notifications@github.com>> wrote: This looks good. I'm going to squash the commits so that I can more easily cherry-pick the changes and create a 2.0.3 release; I'll push those back to your branch before merging. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#349 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFleUS-O6ZMh9lcmcI6qtcOFWwAWZSKBks5vAQHvgaJpZM4ZeQP9>.

[weierophinney]

How do you do that exactly? Would like to know.

$ git rebase -i HEAD~(number of commits to squash or edit)

The -i switch triggers an interactive rebase, which allows you to remove commits, drop them, edit the mesages, re-order them, etc.; when you start it, the tool opens an editor that has instructions on what is possible and how to do it.

[bluebaroncanada]