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

Add IDN support for redirects also #2424

Merged
merged 4 commits into from Dec 20, 2019
Merged

Add IDN support for redirects also #2424

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1996437
Fix issue 2423 and issue 2448
gmponos Dec 11, 2019
f280749
Fix idn according to suggestions from review
gmponos Dec 11, 2019
d56612f
Revert a check that is not needed
gmponos Dec 15, 2019
a5a84c3
Resolved conflicts with 6.5
gmponos Dec 18, 2019
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
31 changes: 2 additions & 29 deletions src/Client.php
View file
Open in desktop
Expand Up @@ -215,36 +215,9 @@ private function buildUri($uri, array $config)
$uri = Psr7\UriResolver::resolve(Psr7\uri_for($config['base_uri']), $uri);
}

if ($uri->getHost() && isset($config['idn_conversion']) && ($config['idn_conversion'] !== false)) {
if (isset($config['idn_conversion']) && ($config['idn_conversion'] !== false)) {
$idnOptions = ($config['idn_conversion'] === true) ? IDNA_DEFAULT : $config['idn_conversion'];

$asciiHost = idn_to_ascii($uri->getHost(), $idnOptions, INTL_IDNA_VARIANT_UTS46, $info);
if ($asciiHost === false) {
$errorBitSet = isset($info['errors']) ? $info['errors'] : 0;

$errorConstants = array_filter(array_keys(get_defined_constants()), function ($name) {
return substr($name, 0, 11) === 'IDNA_ERROR_';
});

$errors = [];
foreach ($errorConstants as $errorConstant) {
if ($errorBitSet & constant($errorConstant)) {
$errors[] = $errorConstant;
}
}

$errorMessage = 'IDN conversion failed';
if ($errors) {
$errorMessage .= ' (errors: ' . implode(', ', $errors) . ')';
}

throw new InvalidArgumentException($errorMessage);
} else {
if ($uri->getHost() !== $asciiHost) {
// Replace URI only if the ASCII version is different
$uri = $uri->withHost($asciiHost);
}
}
$uri = _idn_uri_convert($uri, $idnOptions);
}

return $uri->getScheme() === '' && $uri->getHost() !== '' ? $uri->withScheme('http') : $uri;
Expand Down
10 changes: 8 additions & 2 deletions src/RedirectMiddleware.php
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@
* Request redirect middleware.
*
* Apply this middleware like other middleware using
* {@see GuzzleHttp\Middleware::redirect()}.
* {@see \GuzzleHttp\Middleware::redirect()}.
*/
class RedirectMiddleware
{
Expand Down Expand Up @@ -190,7 +190,13 @@ public function modifyRequest(
$modify['body'] = '';
}

$modify['uri'] = $this->redirectUri($request, $response, $protocols);
$uri = $this->redirectUri($request, $response, $protocols);
if (isset($options['idn_conversion']) && ($options['idn_conversion'] !== false)) {
$idnOptions = ($options['idn_conversion'] === true) ? IDNA_DEFAULT : $options['idn_conversion'];
$uri = _idn_uri_convert($uri, $idnOptions);
}

$modify['uri'] = $uri;
Psr7\rewind_body($request);

// Add the Referer header if it is told to do so and only
Expand Down
46 changes: 46 additions & 0 deletions src/functions.php
View file
Open in desktop
@@ -1,10 +1,12 @@
<?php
namespace GuzzleHttp;

use GuzzleHttp\Exception\InvalidArgumentException;
use GuzzleHttp\Handler\CurlHandler;
use GuzzleHttp\Handler\CurlMultiHandler;
use GuzzleHttp\Handler\Proxy;
use GuzzleHttp\Handler\StreamHandler;
use Psr\Http\Message\UriInterface;

/**
* Expands a URI template
Expand Down Expand Up @@ -344,3 +346,47 @@ function _current_time()
{
return function_exists('hrtime') ? hrtime(true) / 1e9 : microtime(true);
}


/**
* @param int $options
*
* @return UriInterface
*
* @internal
*/
function _idn_uri_convert(UriInterface $uri, $options = 0)
{
if ($uri->getHost()) {
$variant = defined('INTL_IDNA_VARIANT_UTS46') ? INTL_IDNA_VARIANT_UTS46 : 0;
$asciiHost = idn_to_ascii($uri->getHost(), $options, $variant, $info);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just checked locally, it raises a deprecation error (idn_to_ascii(): INTL_IDNA_VARIANT_2003 is deprecated).

The only way is to call the function without the third argument at all, like here:

$asciiHost = defined('INTL_IDNA_VARIANT_UTS46')
                ? idn_to_ascii($uri->getHost(), $idnOptions, INTL_IDNA_VARIANT_UTS46, $info)
                : idn_to_ascii($uri->getHost(), $idnOptions);

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks again.. done..

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, so it's night and too many versions to keep in the head.

It works on my machine, because I have PHP 7.4 with already right defaults.

On 7.3 it raises a warning by default, if you don't specify INTL_IDNA_VARIANT_UTS46 manually. So there is basically no way to get rid of this deprecation error in PHP 7.2+ if you have an old version of ICU library. Looks like we have to disable idn_conversion by default in this case (PHP 7.2+ and INTL_IDNA_VARIANT_UTS46 not defined).

https://3v4l.org/9MPJh

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gmponos, now I think it's better to open a separate PR for this change. Please take a look at #2453.

Copy link
Member Author

Choose a reason for hiding this comment

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

better we avoid the back and forth :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, but now it seems really out of scope here

if ($asciiHost === false) {
$errorBitSet = isset($info['errors']) ? $info['errors'] : 0;

$errorConstants = array_filter(array_keys(get_defined_constants()), function ($name) {
return substr($name, 0, 11) === 'IDNA_ERROR_';
});

$errors = [];
foreach ($errorConstants as $errorConstant) {
if ($errorBitSet & constant($errorConstant)) {
$errors[] = $errorConstant;
}
}

$errorMessage = 'IDN conversion failed';
if ($errors) {
$errorMessage .= ' (errors: ' . implode(', ', $errors) . ')';
}

throw new InvalidArgumentException($errorMessage);
} else {
if ($uri->getHost() !== $asciiHost) {
// Replace URI only if the ASCII version is different
$uri = $uri->withHost($asciiHost);
}
}
}

return $uri;
}
34 changes: 34 additions & 0 deletions tests/ClientTest.php
View file
Open in desktop
Expand Up @@ -5,11 +5,13 @@
use GuzzleHttp\Cookie\CookieJar;
use GuzzleHttp\Handler\MockHandler;
use GuzzleHttp\HandlerStack;
use GuzzleHttp\Middleware;
use GuzzleHttp\Promise\PromiseInterface;
use GuzzleHttp\Psr7;
use GuzzleHttp\Psr7\Request;
use GuzzleHttp\Psr7\Response;
use GuzzleHttp\Psr7\Uri;
use GuzzleHttp\RequestOptions;
use PHPUnit\Framework\TestCase;
use Psr\Http\Message\ResponseInterface;

Expand Down Expand Up @@ -791,4 +793,36 @@ public function testIdnBaseUri()
self::assertSame('http://xn--d1acpjx3f.xn--p1ai/baz', (string) $mock->getLastRequest()->getUri());
self::assertSame('xn--d1acpjx3f.xn--p1ai', (string) $mock->getLastRequest()->getHeaderLine('Host'));
}

public function testIdnWithRedirect()
{
if (!extension_loaded('intl')) {
self::markTestSkipped('intl PHP extension is not loaded');
}
$mockHandler = new MockHandler([
new Response(302, ['Location' => 'http://www.tést.com/whatever']),
new Response()
]);
$handler = HandlerStack::create($mockHandler);
$requests = [];
$handler->push(Middleware::history($requests));
$client = new Client(['handler' => $handler]);

$client->request('GET', 'https://яндекс.рф/images', [
RequestOptions::ALLOW_REDIRECTS => [
'referer' => true,
'track_redirects' => true
],
'idn_conversion' => true
]);

$request = $mockHandler->getLastRequest();

self::assertSame('http://www.xn--tst-bma.com/whatever', (string) $request->getUri());
self::assertSame('www.xn--tst-bma.com', (string) $request->getHeaderLine('Host'));

$request = $requests[0]['request'];
self::assertSame('https://xn--d1acpjx3f.xn--p1ai/images', (string) $request->getUri());
self::assertSame('xn--d1acpjx3f.xn--p1ai', (string) $request->getHeaderLine('Host'));
}
}
11 changes: 11 additions & 0 deletions tests/functionsTest.php
View file
Open in desktop
Expand Up @@ -133,6 +133,17 @@ public function testCurrentTime()
{
self::assertGreaterThan(0, GuzzleHttp\_current_time());
}

public function testIdnConvert()
{
if (!extension_loaded('intl')) {
self::markTestSkipped('intl PHP extension is not loaded');
}

$uri = GuzzleHttp\Psr7\uri_for('https://яндекс.рф/images');
$uri = GuzzleHttp\_idn_uri_convert($uri);
self::assertSame('xn--d1acpjx3f.xn--p1ai', $uri->getHost());
}
}

final class StrClass
Expand Down