test: Ignore the host header before forwarding #162
Conversation
|
I have also experienced problems with the host header when smee-client is forwarding a request with the host header value without a port number to a Jenkins master running in OpenShift, resulting in HTTP 503 response status returned to smee-client. For that reason, removing the host header would help for this issue as well. |
This comment has been minimized.
This comment has been minimized.
Add workaround for [1] by building smee-client from a fork with the fix [2]. [1] probot/smee-client#156 [2] probot/smee-client#162
Add workaround for [1] by building smee-client from a fork with the fix [2]. [1] probot/smee-client#156 [2] probot/smee-client#162
|
Hi @kahowell @JasonEtco @tcbyrd any news here? Thanks! |
|
I definitely think the change makes sense. Given this comment in #156 (comment)
Would anyone be willing to add a test here? Since the regression was most likely introduced in a dependabot upgrade and people are blocked by this, it would probably be best to have even a basic test to ensure upgrading dependencies doesn't re-introduce the problem. |
|
I would like to ask that the bug fix be released before someone waits too much longer trying to design a universally valid test for TLS validation problems applicable to private CI/CD servers receiving github hooks, with internally signed certificates, behind firewalls. That's just the type of oddball situation that this project appeals to. Maybe there's some other project that would make a good dev dependency to create such a test scenario, but the perfect test is looking like an enemy to this simple bug fix. I get the impression TLS is only going to be more and more common on internal networks and will undermine an increasing number of use cases for smee and smee-client. The test is a good idea, don't get me wrong, but it could come following a 2nd issue in the tracker. I was afraid to try & count how many of the 100 forks have fixes for this faulty assumption about the "Host" property in data. |
|
@eidsnessje-nih I noticed that I did not run into this issue when testing today, and I saw that superagent has been updated to v7.1.3 in smee-client v1.2.3, does it resolve the issue for you? Ah, ok, my test is probably not one of
I don't even know what that is. |
|
I tried to solve this issue #156 (comment) by testing this one and it works great. Would be nice to merge this one. Are you planning to do so? |
With some hosts behind shared TLS (using SNI), keeping the Host header was causing the target host to be misidentified. I also observed issues related to TLS verification (as seen in probot#156). Fixes probot#156
kahowell
force-pushed
the
drop-host-header
branch
from
6584331
to
5093aec
Compare
|
@Ginxo I've rebased against latest as requested. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
This seems good to me. Is it possible to add a test to make sure it doesn't happen again? |
|
Its basically impossible to write a useful unit test for this. |
|
I want to make sure #181 is considered as an alternative to this. In my case, the host header is required to have the correct host since the web server or reverse proxy makes decisions based on the host header. Removing the host header entirely may fix cases where the default server is the intended target, but it will make it impossible to correctly target a specific host. Also, #181 has a test that I believe shows that it works as expected. I think? :) Is there any reason to believe #181 won't work for addressing #156? |
|
I guess, that the issue is not existent anymore. We use now fetch, which does not take host header as it is a header which should not be manipulated. |
Uzlopak
changed the title
Uzlopak
changed the title
I still meet this issue on 2.0.1.
It's safe to delete the host header and fetch can add it back from the url. |
|
Do you want to provide a PR? |
With some hosts behind shared TLS (using SNI), keeping the Host header was causing the target host to be misidentified.
I also observed issues related to TLS verification (as seen in #156).
Fixes #156