New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 5, 2019

Conversation

Projects
None yet
7 participants
@bluebaroncanada
Copy link

bluebaroncanada commented Dec 21, 2018

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 #347 #342 #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 comment has been minimized.

Copy link

bluebaroncanada commented Dec 21, 2018

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.

@webimpress

This comment has been minimized.

Copy link
Contributor

webimpress commented Dec 22, 2018

@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 bluebaroncanada referenced this pull request Dec 22, 2018

Closed

ServerRequest: Header with value "0" discarded #342

2 of 2 tasks complete
@bluebaroncanada

This comment has been minimized.

Copy link

bluebaroncanada commented Dec 22, 2018

@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

This comment has been minimized.

Copy link

bluebaroncanada commented Dec 22, 2018

@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

This comment has been minimized.

Copy link

bluebaroncanada commented Dec 22, 2018

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.

@webimpress

This comment has been minimized.

Copy link
Contributor

webimpress commented Dec 23, 2018

@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

This comment has been minimized.

Copy link

bluebaroncanada commented Dec 31, 2018

  • 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

This comment has been minimized.

Copy link

bluebaroncanada commented Dec 31, 2018

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

webimpress left a comment

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

@bluebaroncanada

This comment has been minimized.

Copy link

bluebaroncanada commented Dec 31, 2018

Thanks for the help!

@xtreamwayz

This comment has been minimized.

Copy link
Member

xtreamwayz commented Jan 2, 2019

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

@xtreamwayz

This comment has been minimized.

Copy link
Member

xtreamwayz commented Jan 2, 2019

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 comment has been minimized.

Copy link
Member

weierophinney commented Jan 5, 2019

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

This comment has been minimized.

Copy link

bluebaroncanada commented Jan 5, 2019

Bug fix: filter out header sources only if the value is an empty string
A previous change checked for a truthy `$value` in SAPI header sources
within the `marshal_headers_from_sapi()` function, which could lead to
problems when the value was `0` or `'0'`.

This patch adds tests to prevent future changes from causing the issue
to re-surface, as well as a fix for the problem.

@weierophinney weierophinney force-pushed the bluebaroncanada:master branch from a16061b to c6ea797 Jan 5, 2019

@weierophinney weierophinney merged commit c6ea797 into zendframework:master Jan 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

weierophinney added a commit that referenced this pull request Jan 5, 2019

weierophinney added a commit that referenced this pull request Jan 5, 2019

Merge branch 'hotfix/349-zero-header-fix' into develop
Forward port #349

Conflicts:
	CHANGELOG.md
@weierophinney

This comment has been minimized.

Copy link
Member

weierophinney commented Jan 5, 2019

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

This comment has been minimized.

Copy link

bluebaroncanada commented Jan 5, 2019

image

asabirov pushed a commit to apliteni/zend-diactoros that referenced this pull request Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment