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.

Sign up for GitHub

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

Jump to bottom

[8.0] Final for no proxy settings for all php versions #3164

Open
wants to merge 2 commits into
base: 8.0
Choose a base branch
from
Open

[8.0] Final for no proxy settings for all php versions #3164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26a08f4
Add proper no_proxy settings
gergokee Aug 18, 2023
bedd929
Change test to support new changes
gergokee Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Handler/CurlFactory.php
View file
Open in desktop
Expand Up @@ -446,7 +446,9 @@ private function applyHandlerOptions(EasyHandle $easy, array &$conf): void
if (isset($options['proxy'][$scheme])) {
$host = $easy->request->getUri()->getHost();
if (isset($options['proxy']['no']) && Utils::isHostInNoProxy($host, $options['proxy']['no'])) {
unset($conf[\CURLOPT_PROXY]);
// To support older versions of php
$conf[\CURLOPT_PROXY] = '';
$conf[\CURLOPT_NOPROXY] = $host;
} else {
$conf[\CURLOPT_PROXY] = $options['proxy'][$scheme];
}
Comment on lines 452 to 454
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this covers the case where our request matches the ['proxy']['no'] configuration (and we want it not to use the proxy), it misses an inverse case. :)

Image that in the environment variables there's NO_PROXY="foo.bar" / no_proxy="foo.bar", and we want to configure guzzle to override/ignore this (to proxy everything, or at least proxy requests to foo.bar).

If we set:

'proxy' => [
    'http' => 'http://example:8080',
    'https' => 'http://example:8080',
    # Send everything through the proxy
    'no' => []
]

Then the environment's behavior will still get used and requests to foo.bar will (unexpectedly) not go through the proxy.

Perhaps something like this in the else branch?

Suggested change
} else {
$conf[\CURLOPT_PROXY] = $options['proxy'][$scheme];
}
} else {
$conf[\CURLOPT_PROXY] = $options['proxy'][$scheme];
$conf[\CURLOPT_NOPROXY] = "";
}

(Related comment on the issue: #3163 (comment))

Expand Down
7 changes: 3 additions & 4 deletions tests/Handler/CurlFactoryTest.php
View file
Open in desktop
Expand Up @@ -191,10 +191,9 @@ private function checkNoProxyForHost($url, $noProxy, $assertUseProxy)
'no' => $noProxy,
],
]);
if ($assertUseProxy) {
self::assertArrayHasKey(\CURLOPT_PROXY, $_SERVER['_curl']);
} else {
self::assertArrayNotHasKey(\CURLOPT_PROXY, $_SERVER['_curl']);
self::assertArrayHasKey(\CURLOPT_PROXY, $_SERVER['_curl']);
if (!$assertUseProxy) {
self::assertEquals('', $_SERVER['_curl'][\CURLOPT_PROXY]);
}
}

Expand Down