-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpClient] Fix using https with proxies #38368
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
Conversation
Hello, I'm not sure I get what the issue is here.
Where is this line coming from? Note that HTTP/1.1 section 5.3.2 tells that the full uri must be used when going through a proxy:
can you give more hints about this? could your proxy be non-compliant? How does it behave when using CurlHttpClient? |
I'm not sure what changed the
I'm testing it with privoxy, I don't know which proxy is compliant if it isn't. Tested on Symfony 5.1.6, Debian 10, PHP (deb.sury.org) 7.4.10, curl 7.72.0 (from unstable repo to enable the debug log) <?php
declare(strict_types=1);
use Symfony\Component\HttpClient\CurlHttpClient;
use Symfony\Component\HttpClient\NativeHttpClient;
use Symfony\Component\HttpClient\HttpClient;
require __DIR__ . '/vendor/autoload.php';
$client = new NativeHttpClient();
$resp = $client->request('GET', 'https://httpbin.org/get');
echo $resp->getContent();
echo $resp->getInfo('debug'); Without
|
According to https://www.php.net/manual/fr/context.http.php#context.http.request-fulluri, the |
Thanks for the insights. Looking at the details, this means that the fulluri should be used only when https is not in use. The patch should be
|
why would it depend on https ? |
|
Patch applied. Worked with privoxy. |
Thank you @bohanyang. |
Can you open an issue on the v2ray repo to tell them about this? It should be fixed on their side IMHO. |
@nicolas-grekas my understanding of the HTTP/1.1 spec is that the request to the proxy should be |
It should, but here this conflicts with the local DNS resolver/cache. But does it matter?
Why would it? |
because of https://tools.ietf.org/html/rfc7230#section-5.4:
|
@stof I missed that part, grrr. This means we have a conflict between the local DNS resolver/cache and proxies when using NativeHttpClient. I'm going to submit a PR to account for this. @bohanyang maybe this will fix your issue with |
@bohanyang can you please test with #38375? |
…las-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] fix using proxies with NativeHttpClient | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - As spotted by @stof in #38368 (comment), we cannot use local DNS resolution with HTTP proxies. Commits ------- 28f301b [HttpClient] fix using proxies with NativeHttpClient
According to my test, when
request_fulluri
is set to true,the host appears in the URL will be the Host header,
even if the Host header is set in the context http header.
Since HttpClient has its own DNS cache, the host inside the URL is usually an IP address.
So this can break many things.
I've also found this guzzle/guzzle#791
We can also create an option to make it customizable.