Skip to content
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

pac4cli does not respect http header case #56

Open
tkluck opened this issue Nov 13, 2018 · 2 comments
Open

pac4cli does not respect http header case #56

tkluck opened this issue Nov 13, 2018 · 2 comments

Comments

@tkluck
Copy link
Owner

tkluck commented Nov 13, 2018

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
Copy link
Owner Author

tkluck commented Nov 13, 2018

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
Copy link
Collaborator

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

denilsonsa@8249fee
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants