tkluck / pac4cli

Proxy-auto-discovery for command-line applications
25 stars 9 forks source link

pac4cli does not respect http header case #56

Open tkluck opened 5 years ago

tkluck commented 5 years ago

As reported by @denilsonsa over email, discussing his setup in our shared work environment (some details redacted):

So, I use "adminer" as a web interface to query the database. The way I've set it up is by running PHP cli server in the KVM, and then do SSH port redirection.

After I installed pac4cli, I noticed adminer stopped working. After an hour of debugging, I've manged to track down the exact reason: pac4cli is lower-casing the HTTP headers, and the old PHP 5.4 on my KVM seems to dislike that.

How to reproduce (minimal version):

Create a [centos VM], then run the following commands:
ssh -L 127.0.0.1:1495:127.0.0.1:1495 your-centos-vm
sudo yum install php-cli
mkdir ~/foobar
cd ~/foobar
echo '<form action="" method="POST"> <input type="text" name="foo"> <input type="submit"> </form> <?php var_dump($_POST); ?>' > index.php
php -S 127.0.0.1:1495
Finally, open http://127.0.0.7:1495/ in your browser…
    When you submit the form through pac4cli, nothing gets printed.
    When you submit the form directly (bypassing pac4cli), the submitted value gets printed.

So, yeah, this is a corner case. I tried reproducing it by running PHP cli server on my laptop, but it seems to work fine in PHP 7.

Now… Why does it happen? I've also investigated that by running "nc -l -p 1495". I believe it is caused by pac4cli sending all HTTP headers in lower-case.

But when are they converted to lowercase? Well, inside twisted: https://github.com/twisted/twisted/blob/twisted-18.9.0/src/twisted/web/http.py#L1358

Also, that documentation suggests using another method instead of getAllHeaders.

So, you might want to create some unit tests on pac4cli for checking if the same header passed multiple times on the input will be passed correctly to the remote server; and if the case is preserved from the input to the output (or, at least the case is in canonical capitalization).

Thanks! I hope this is enough to understand and reproduce this bug. :) Feel free to ask any questions!

edit: redacted some corporate details for non-disclosure and more general applicability.

tkluck commented 5 years ago

Thanks Denilson! This is a very clear bug report. We'll need to study twisted's code a bit to find out what's the easiest way to solve it. As it is, the ProxyClientFactory seems to expect a dictionary interface, but ideally, we fix this in a way that respects header duplication and the header order.

denilsonsa commented 5 years ago

I have a commit that adds a unit test for this case. The test is currently failing (of course, because it shows the bug).

https://github.com/denilsonsa/pac4cli/commit/8249feedea3bf9b773379eb8d13e515eed01c378 https://github.com/tkluck/pac4cli/compare/master...denilsonsa:headers?expand=1

The test expected the exact same headers in the exact same order as the output. But I know multiple duplicate headers may be combined into a single one with their values joined by commas. This test does not support this behavior.