Make RedisClient constructor throw if given $uri is invalid

clue · Dec 27, 2024 · d0f317c · d0f317c
1 parent cc0c50a
commit d0f317c
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 44 deletions.
diff --git a/README.md b/README.md
@@ -319,10 +319,10 @@ will not have to wait for an actual underlying connection.
 
 #### __construct()
 
-The `new RedisClient(string $url, ConnectorInterface $connector = null)` constructor can be used to
+The `new RedisClient(string $uri, ConnectorInterface $connector = null)` constructor can be used to
 create a new `RedisClient` instance.
 
-The `$url` can be given in the
+The `$uri` can be given in the
 [standard](https://www.iana.org/assignments/uri-schemes/prov/redis) form
 `[redis[s]://][:auth@]host[:port][/db]`.
 You can omit the URI scheme and port if you're connecting to the default port 6379:

diff --git a/src/Io/Factory.php b/src/Io/Factory.php
@@ -43,23 +43,15 @@ public function __construct(?ConnectorInterface $connector = null, ?ProtocolFact
     public function createClient(string $uri): PromiseInterface
     {
         // support `redis+unix://` scheme for Unix domain socket (UDS) paths
-        if (preg_match('/^(redis\+unix:\/\/(?:[^:]*:[^@]*@)?)(.+?)?$/', $uri, $match)) {
+        if (preg_match('/^(redis\+unix:\/\/(?:[^@]*@)?)(.+)$/', $uri, $match)) {
             $parts = parse_url($match[1] . 'localhost/' . $match[2]);
         } else {
-            if (strpos($uri, '://') === false) {
-                $uri = 'redis://' . $uri;
-            }
-
             $parts = parse_url($uri);
         }
 
         $uri = preg_replace(['/(:)[^:\/]*(@)/', '/([?&]password=).*?($|&)/'], '$1***$2', $uri);
-        if ($parts === false || !isset($parts['scheme'], $parts['host']) || !in_array($parts['scheme'], ['redis', 'rediss', 'redis+unix'])) {
-            return reject(new \InvalidArgumentException(
-                'Invalid Redis URI given (EINVAL)',
-                defined('SOCKET_EINVAL') ? SOCKET_EINVAL : 22
-            ));
-        }
+        assert(isset($parts['scheme'], $parts['host']));
+        assert(in_array($parts['scheme'], ['redis', 'rediss', 'redis+unix']));
 
         $args = [];
         parse_str($parts['query'] ?? '', $args);

diff --git a/src/RedisClient.php b/src/RedisClient.php
@@ -28,7 +28,7 @@
 class RedisClient extends EventEmitter
 {
     /** @var string */
-    private $target;
+    private $uri;
 
     /** @var Factory */
     private $factory;
@@ -54,15 +54,39 @@ class RedisClient extends EventEmitter
     /** @var array<string,bool> */
     private $psubscribed = [];
 
-    public function __construct(string $url, ?ConnectorInterface $connector = null)
+    /**
+     * @param string $uri
+     * @param ?ConnectorInterface $connector
+     * @throws \InvalidArgumentException if $uri is not a valid Redis URI
+     */
+    public function __construct(string $uri, ?ConnectorInterface $connector = null)
     {
+        // support `redis+unix://` scheme for Unix domain socket (UDS) paths
+        if (preg_match('/^(redis\+unix:\/\/(?:[^@]*@)?)(.+)$/', $uri, $match)) {
+            $parts = parse_url($match[1] . 'localhost/' . $match[2]);
+        } else {
+            if (strpos($uri, '://') === false) {
+                $uri = 'redis://' . $uri;
+            }
+
+            $parts = parse_url($uri);
+        }
+
+        $uri = (string) preg_replace(['/(:)[^:\/]*(@)/', '/([?&]password=).*?($|&)/'], '$1***$2', $uri);
+        if ($parts === false || !isset($parts['scheme'], $parts['host']) || !in_array($parts['scheme'], ['redis', 'rediss', 'redis+unix'])) {
+            throw new \InvalidArgumentException(
+                'Invalid Redis URI "' . $uri . '" (EINVAL)',
+                defined('SOCKET_EINVAL') ? SOCKET_EINVAL : 22
+            );
+        }
+
         $args = [];
-        \parse_str((string) \parse_url($url, \PHP_URL_QUERY), $args);
+        \parse_str($parts['query'] ?? '', $args);
         if (isset($args['idle'])) {
             $this->idlePeriod = (float)$args['idle'];
         }
 
-        $this->target = $url;
+        $this->uri = $uri;
         $this->factory = new Factory($connector);
     }
 
@@ -75,7 +99,7 @@ private function client(): PromiseInterface
             return $this->promise;
         }
 
-        return $this->promise = $this->factory->createClient($this->target)->then(function (StreamingClient $redis) {
+        return $this->promise = $this->factory->createClient($this->uri)->then(function (StreamingClient $redis) {
             // connection completed => remember only until closed
             $redis->on('close', function () {
                 $this->promise = null;

diff --git a/tests/Io/FactoryStreamingClientTest.php b/tests/Io/FactoryStreamingClientTest.php
@@ -55,15 +55,15 @@ public function testCtor(): void
     public function testWillConnectWithDefaultPort(): void
     {
         $this->connector->expects($this->once())->method('connect')->with('redis.example.com:6379')->willReturn(reject(new \RuntimeException()));
-        $promise = $this->factory->createClient('redis.example.com');
+        $promise = $this->factory->createClient('redis://redis.example.com');
 
         $promise->then(null, $this->expectCallableOnce()); // avoid reporting unhandled rejection
     }
 
     public function testWillConnectToLocalhost(): void
     {
         $this->connector->expects($this->once())->method('connect')->with('localhost:1337')->willReturn(reject(new \RuntimeException()));
-        $promise = $this->factory->createClient('localhost:1337');
+        $promise = $this->factory->createClient('redis://localhost:1337');
 
         $promise->then(null, $this->expectCallableOnce()); // avoid reporting unhandled rejection
     }
@@ -74,7 +74,7 @@ public function testWillResolveIfConnectorResolves(): void
         $stream->expects($this->never())->method('write');
 
         $this->connector->expects($this->once())->method('connect')->willReturn(resolve($stream));
-        $promise = $this->factory->createClient('localhost');
+        $promise = $this->factory->createClient('redis://localhost');
 
         $this->expectPromiseResolve($promise);
     }
@@ -483,23 +483,6 @@ public function testWillRejectIfConnectorRejects(): void
         ));
     }
 
-    public function testWillRejectIfTargetIsInvalid(): void
-    {
-        $promise = $this->factory->createClient('http://invalid target');
-
-        $promise->then(null, $this->expectCallableOnceWith(
-            $this->logicalAnd(
-                $this->isInstanceOf(\InvalidArgumentException::class),
-                $this->callback(function (\InvalidArgumentException $e) {
-                    return $e->getMessage() === 'Invalid Redis URI given (EINVAL)';
-                }),
-                $this->callback(function (\InvalidArgumentException $e) {
-                    return $e->getCode() === (defined('SOCKET_EINVAL') ? SOCKET_EINVAL : 22);
-                })
-            )
-        ));
-    }
-
     public function testCancelWillRejectPromise(): void
     {
         $promise = new \React\Promise\Promise(function () { });
@@ -516,10 +499,6 @@ public function testCancelWillRejectPromise(): void
     public function provideUris(): array
     {
         return [
-            [
-                'localhost',
-                'redis://localhost'
-            ],
             [
                 'redis://localhost',
                 'redis://localhost'
@@ -705,7 +684,7 @@ public function testCreateClientWillCancelTimerWhenConnectionResolves(): void
         $deferred = new Deferred();
         $this->connector->expects($this->once())->method('connect')->with('127.0.0.1:6379')->willReturn($deferred->promise());
 
-        $promise = $this->factory->createClient('127.0.0.1');
+        $promise = $this->factory->createClient('redis://127.0.0.1');
         $promise->then($this->expectCallableOnce());
 
         $deferred->resolve($this->createMock(ConnectionInterface::class));
@@ -723,7 +702,7 @@ public function testCreateClientWillCancelTimerWhenConnectionRejects(): void
         $deferred = new Deferred();
         $this->connector->expects($this->once())->method('connect')->with('127.0.0.1:6379')->willReturn($deferred->promise());
 
-        $promise = $this->factory->createClient('127.0.0.1');
+        $promise = $this->factory->createClient('redis://127.0.0.1');
 
         $promise->then(null, $this->expectCallableOnceWith($this->isInstanceOf('RuntimeException')));
 

diff --git a/tests/RedisClientTest.php b/tests/RedisClientTest.php
@@ -44,6 +44,67 @@ public function setUp(): void
         $ref->setValue($this->redis, $this->factory);
     }
 
+    public static function provideInvalidUris(): \Generator
+    {
+        yield [
+            '',
+            'redis://'
+        ];
+        yield [
+            'localhost:100000',
+            'redis://localhost:100000'
+        ];
+        yield [
+            'tcp://localhost',
+            'tcp://localhost'
+        ];
+        yield [
+            'redis://',
+            'redis://'
+        ];
+        yield [
+            'redis+unix://',
+            'redis+unix://'
+        ];
+        yield [
+            'user@localhost:100000',
+            'redis://user@localhost:100000'
+        ];
+        yield [
+            ':pass@localhost:100000',
+            'redis://:***@localhost:100000'
+        ];
+        yield [
+            'user:@localhost:100000',
+            'redis://user:***@localhost:100000'
+        ];
+        yield [
+            'user:pass@localhost:100000',
+            'redis://user:***@localhost:100000'
+        ];
+        yield [
+            'localhost:100000?password=secret',
+            'redis://localhost:100000?password=***'
+        ];
+        yield [
+            'user@',
+            'redis://user@'
+        ];
+        yield [
+            'user:pass@',
+            'redis://user:***@'
+        ];
+    }
+
+    /** @dataProvider provideInvalidUris() */
+    public function testCtorWithInvalidUriThrows(string $uri, string $message): void
+    {
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionMessage('Invalid Redis URI "' . $message . '" (EINVAL)');
+        $this->expectExceptionCode(defined('SOCKET_EINVAL') ? SOCKET_EINVAL : 22);
+        new RedisClient($uri);
+    }
+
     public function testPingWillCreateUnderlyingClientAndReturnPendingPromise(): void
     {
         $promise = new Promise(function () { });