fix(23621): RedisClient Invalid URL (#23714)

### What does this PR do?

Fixes #23621.

Note that the quality of this code is quite low, but since Redis is
getting a rewrite, this is a stop-gap. The tests are what really matters
here.

This whole PR is claude.

### How did you verify your code works?

CI.

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

docs/api/redis.md

@@ -42,6 +42,7 @@ await client.incr("counter");
 By default, the client reads connection information from the following environment variables (in order of precedence):
 
 - `REDIS_URL`
+- `VALKEY_URL`
 - If not set, defaults to `"redis://localhost:6379"`
 
 ### Connection Lifecycle

src/bun.js/bindings/BunString.cpp

@@ -664,10 +664,25 @@ extern "C" BunString URL__search(WTF::URL* url)
     return Bun::toStringRef(url->query().toStringWithoutCopying());
 }
 
+/// Returns the host WITHOUT the port.
+///
+/// Note that this does NOT match JS behavior, which returns the host with the port.
+///
+/// ```
+/// URL("http://example.com:8080").host() => "example.com"
+/// ```
 extern "C" BunString URL__host(WTF::URL* url)
 {
     return Bun::toStringRef(url->host().toStringWithoutCopying());
 }
+
+/// Returns the host WITH the port.
+///
+/// Note that this does NOT match JS behavior which returns the host without the port.
+///
+/// ```
+/// URL("http://example.com:8080").hostname() => "example.com:8080"
+/// ```
 extern "C" BunString URL__hostname(WTF::URL* url)
 {
     return Bun::toStringRef(url->hostAndPort());

src/bun.js/bindings/URL.zig

@@ -86,10 +86,28 @@ pub const URL = opaque {
         jsc.markBinding(@src());
         return URL__search(url);
     }
+
+    /// Returns the host WITHOUT the port.
+    ///
+    /// Note that this does NOT match JS behavior, which returns the host with the port. See
+    /// `hostname` for the JS equivalent of `host`.
+    ///
+    /// ```
+    /// URL("http://example.com:8080").host() => "example.com"
+    /// ```
     pub fn host(url: *URL) String {
         jsc.markBinding(@src());
         return URL__host(url);
     }
+
+    /// Returns the host WITH the port.
+    ///
+    /// Note that this does NOT match JS behavior which returns the host without the port. See
+    /// `host` for the JS equivalent of `hostname`.
+    ///
+    /// ```
+    /// URL("http://example.com:8080").hostname() => "example.com:8080"
+    /// ```
     pub fn hostname(url: *URL) String {
         jsc.markBinding(@src());
         return URL__hostname(url);

src/valkey/js_valkey.zig

@@ -266,48 +266,96 @@ pub const JSValkeyClient = struct {
         const this_allocator = bun.default_allocator;
 
         const vm = globalObject.bunVM();
-        const url_str = if (arguments.len < 1 or arguments[0].isUndefined())
-            if (vm.transpiler.env.get("REDIS_URL") orelse vm.transpiler.env.get("VALKEY_URL")) |url|
-                bun.String.init(url)
-            else
-                bun.String.init("valkey://localhost:6379")
+
+        const url_str = if (arguments.len >= 1 and !arguments[0].isUndefinedOrNull())
+            try arguments[0].toBunString(globalObject)
+        else if (vm.transpiler.env.get("REDIS_URL") orelse vm.transpiler.env.get("VALKEY_URL")) |url|
+            bun.String.init(url)
         else
-            try arguments[0].toBunString(globalObject);
+            bun.String.static("valkey://localhost:6379");
         defer url_str.deref();
 
-        const url_utf8 = url_str.toUTF8WithoutRef(this_allocator);
-        defer url_utf8.deinit();
-        const url = bun.URL.parse(url_utf8.slice());
+        // Parse and validate the URL using URL.zig's fromString which returns null for invalid URLs
+        const parsed_url = URL.fromString(url_str) orelse {
+            if (url_str.tag != .StaticZigString) {
+                const url_utf8 = url_str.toUTF8WithoutRef(this_allocator);
+                defer url_utf8.deinit();
+                return globalObject.throwInvalidArguments("Invalid URL format: \"{s}\"", .{url_utf8.slice()});
+            }
+            // This should never happen since our default URL is valid
+            return globalObject.throwInvalidArguments("Invalid URL format", .{});
+        };
+        defer parsed_url.deinit();
 
-        const uri: valkey.Protocol = if (url.protocol.len > 0)
-            valkey.Protocol.Map.get(url.protocol) orelse return globalObject.throw("Expected url protocol to be one of redis, valkey, rediss, valkeys, redis+tls, redis+unix, redis+tls+unix", .{})
+        // Extract protocol string
+        const protocol_str = parsed_url.protocol();
+        defer protocol_str.deref();
+        const protocol_utf8 = protocol_str.toUTF8WithoutRef(this_allocator);
+        defer protocol_utf8.deinit();
+        // Remove the trailing ':' from protocol (e.g., "redis:" -> "redis")
+        const protocol_slice = if (protocol_utf8.slice().len > 0 and protocol_utf8.slice()[protocol_utf8.slice().len - 1] == ':')
+            protocol_utf8.slice()[0 .. protocol_utf8.slice().len - 1]
+        else
+            protocol_utf8.slice();
+
+        const uri: valkey.Protocol = if (protocol_slice.len > 0)
+            valkey.Protocol.Map.get(protocol_slice) orelse return globalObject.throw("Expected url protocol to be one of redis, valkey, rediss, valkeys, redis+tls, redis+unix, redis+tls+unix", .{})
         else
             .standalone;
 
-        var username: []const u8 = "";
-        var password: []const u8 = "";
-        var hostname: []const u8 = switch (uri) {
-            .standalone_tls, .standalone => url.displayHostname(),
-            .standalone_unix, .standalone_tls_unix => brk: {
-                const unix_socket_path = bun.strings.indexOf(url_utf8.slice(), "://") orelse {
-                    return globalObject.throwInvalidArguments("Expected unix socket path after valkey+unix:// or valkey+tls+unix://", .{});
-                };
-                const path = url_utf8.slice()[unix_socket_path + 3 ..];
-                if (bun.strings.indexOfChar(path, '?')) |query_index| {
-                    break :brk path[0..query_index];
-                }
-                if (path.len == 0) {
-                    // "valkey+unix://?abc=123"
-                    return globalObject.throwInvalidArguments("Expected unix socket path after valkey+unix:// or valkey+tls+unix://", .{});
-                }
+        // Extract all URL components
+        const username_str = parsed_url.username();
+        defer username_str.deref();
+        const username_utf8 = username_str.toUTF8WithoutRef(this_allocator);
+        defer username_utf8.deinit();
 
-                break :brk path;
+        const password_str = parsed_url.password();
+        defer password_str.deref();
+        const password_utf8 = password_str.toUTF8WithoutRef(this_allocator);
+        defer password_utf8.deinit();
+
+        const hostname_str = parsed_url.host();
+        defer hostname_str.deref();
+        const hostname_utf8 = hostname_str.toUTF8WithoutRef(this_allocator);
+        defer hostname_utf8.deinit();
+
+        const pathname_str = parsed_url.pathname();
+        defer pathname_str.deref();
+        const pathname_utf8 = pathname_str.toUTF8WithoutRef(this_allocator);
+        defer pathname_utf8.deinit();
+
+        // Determine hostname based on protocol type
+        const hostname_slice = switch (uri) {
+            .standalone_tls, .standalone => hostname_utf8.slice(),
+            .standalone_unix, .standalone_tls_unix => brk: {
+                // For unix sockets, the path is in the pathname
+                if (pathname_utf8.slice().len == 0) {
+                    return globalObject.throwInvalidArguments("Expected unix socket path after valkey+unix:// or valkey+tls+unix://", .{});
+                }
+                break :brk pathname_utf8.slice();
             },
         };
 
         const port = switch (uri) {
             .standalone_unix, .standalone_tls_unix => 0,
-            else => url.getPort() orelse 6379,
+            else => brk: {
+                const port_value = parsed_url.port();
+                // URL.port() returns std.math.maxInt(u32) if port is not set
+                if (port_value == std.math.maxInt(u32)) {
+                    // No port specified, use default
+                    break :brk 6379;
+                } else {
+                    // Port was explicitly specified
+                    if (port_value == 0) {
+                        // Port 0 is invalid for TCP connections (though it's allowed for unix sockets)
+                        return globalObject.throwInvalidArguments("Port 0 is not valid for TCP connections", .{});
+                    }
+                    if (port_value > 65535) {
+                        return globalObject.throwInvalidArguments("Invalid port number in URL. Port must be a number between 0 and 65535", .{});
+                    }
+                    break :brk @as(u16, @intCast(port_value));
+                }
+            },
         };
 
         const options = if (arguments.len >= 2 and !arguments[1].isUndefinedOrNull() and arguments[1].isObject())
@@ -315,25 +363,32 @@ pub const JSValkeyClient = struct {
         else
             valkey.Options{};
 
+        // Copy strings into a persistent buffer since the URL object will be deinitialized
         var connection_strings: []u8 = &.{};
-        errdefer {
-            this_allocator.free(connection_strings);
-        }
+        var username: []const u8 = "";
+        var password: []const u8 = "";
+        var hostname: []const u8 = "";
 
-        if (url.username.len > 0 or url.password.len > 0 or hostname.len > 0) {
+        errdefer if (connection_strings.len != 0) this_allocator.free(connection_strings);
+
+        if (username_utf8.slice().len > 0 or password_utf8.slice().len > 0 or hostname_slice.len > 0) {
             var b = bun.StringBuilder{};
-            b.count(url.username);
-            b.count(url.password);
-            b.count(hostname);
+            b.count(username_utf8.slice());
+            b.count(password_utf8.slice());
+            b.count(hostname_slice);
             try b.allocate(this_allocator);
             defer b.deinit(this_allocator);
-            username = b.append(url.username);
-            password = b.append(url.password);
-            hostname = b.append(hostname);
+            username = b.append(username_utf8.slice());
+            password = b.append(password_utf8.slice());
+            hostname = b.append(hostname_slice);
             b.moveToSlice(&connection_strings);
         }
 
-        const database = if (url.pathname.len > 0) std.fmt.parseInt(u32, url.pathname[1..], 10) catch 0 else 0;
+        // Parse database number from pathname (e.g., "/1" -> database 1)
+        const database = if (pathname_utf8.slice().len > 1)
+            std.fmt.parseInt(u32, pathname_utf8.slice()[1..], 10) catch 0
+        else
+            0;
 
         bun.analytics.Features.valkey += 1;
 
@@ -1582,6 +1637,7 @@ const debug = bun.Output.scoped(.RedisJS, .visible);
 const Command = @import("./ValkeyCommand.zig");
 const std = @import("std");
 const valkey = @import("./valkey.zig");
+const URL = @import("../bun.js/bindings/URL.zig").URL;
 
 const protocol = @import("./valkey_protocol.zig");
 const RedisError = protocol.RedisError;

test/regression/issue/23621.test.ts

@@ -0,0 +1,160 @@
+import { RedisClient } from "bun";
+import { describe, expect, test } from "bun:test";
+
+/**
+ * Regression test for issue #23621: Invalid URL and port handling in Redis/Valkey client
+ *
+ * Issue: Redis client silently falls back to localhost:6379 when given invalid URLs or ports
+ * Expected: Client should throw an error for invalid URLs/ports while still allowing no URL
+ *          (which correctly defaults to localhost:6379)
+ *
+ * This test ensures that:
+ * 1. Invalid URLs throw errors immediately during construction
+ * 2. Invalid port numbers are rejected (negative, >65535, port 0 for TCP)
+ * 3. No URL (undefined) correctly defaults to localhost:6379
+ * 4. Valid URLs and port numbers are accepted
+ */
+describe("RedisClient: Invalid URL Handling (#23621)", () => {
+  test("should throw error for completely malformed URLs", () => {
+    expect(() => {
+      new RedisClient("not a valid url at all");
+    }).toThrow(/invalid url/i);
+
+    expect(() => {
+      new RedisClient("://no-protocol");
+    }).toThrow(/invalid url/i);
+
+    expect(() => {
+      new RedisClient("redis://[invalid-ipv6");
+    }).toThrow(/invalid url/i);
+  });
+
+  test("should throw error for empty string URL", () => {
+    expect(() => {
+      new RedisClient("");
+    }).toThrow(/invalid url/i);
+  });
+
+  test("should throw error for URLs with invalid port formats in the URL itself", () => {
+    // These should be caught by URL validation before port parsing
+    expect(() => {
+      new RedisClient("redis://localhost:not-a-number");
+    }).toThrow();
+  });
+
+  test("should accept valid URLs with proper format", () => {
+    // These should not throw during construction
+    expect(() => {
+      const client = new RedisClient("redis://localhost:6379");
+      client.close();
+    }).not.toThrow();
+
+    expect(() => {
+      const client = new RedisClient("valkey://127.0.0.1:6379");
+      client.close();
+    }).not.toThrow();
+
+    expect(() => {
+      const client = new RedisClient("redis://user:pass@localhost:6379");
+      client.close();
+    }).not.toThrow();
+  });
+
+  test("should allow undefined/no URL (defaults to localhost:6379)", () => {
+    // When no URL is provided, it should use the default
+    expect(() => {
+      const client = new RedisClient();
+      client.close();
+    }).not.toThrow();
+
+    expect(() => {
+      const client = new RedisClient(undefined);
+      client.close();
+    }).not.toThrow();
+  });
+
+  test("should distinguish between no URL (valid) and invalid URL (error)", () => {
+    // No URL should work (uses default)
+    const clientNoUrl = new RedisClient();
+    clientNoUrl.close();
+
+    // But an explicitly invalid URL should fail
+    expect(() => {
+      new RedisClient("this is not a url");
+    }).toThrow(/invalid url/i);
+  });
+
+  test("should handle URLs with valid special cases", () => {
+    expect(() => {
+      const client = new RedisClient("redis://localhost");
+      client.close();
+    }).not.toThrow();
+
+    expect(() => {
+      const client = new RedisClient("rediss://localhost:6380");
+      client.close();
+    }).not.toThrow();
+
+    expect(() => {
+      const client = new RedisClient("valkey+unix:///tmp/redis.sock");
+      client.close();
+    }).not.toThrow();
+  });
+
+  test("should throw error for port number exceeding 65535", () => {
+    // Port 130000 exceeds the maximum valid port number (65535)
+    expect(() => {
+      new RedisClient("redis://localhost:130000");
+    }).toThrow(/(invalid port number|invalid url format)/i);
+  });
+
+  test("should throw error for negative port number", () => {
+    expect(() => {
+      new RedisClient("redis://localhost:-1");
+    }).toThrow(/(invalid port number|invalid url format)/i);
+  });
+
+  test("should throw error for explicit port 0 when using TCP (not unix socket)", () => {
+    // Port 0 is invalid for TCP connections when explicitly specified
+    expect(() => {
+      new RedisClient("redis://localhost:0");
+    }).toThrow(/port 0 is not valid/i);
+  });
+
+  test("should accept valid port numbers", () => {
+    // These should not throw during construction (though connection will fail without a server)
+    expect(() => {
+      const client = new RedisClient("redis://localhost:6379");
+      client.close();
+    }).not.toThrow();
+
+    expect(() => {
+      const client = new RedisClient("redis://localhost:1234");
+      client.close();
+    }).not.toThrow();
+
+    expect(() => {
+      const client = new RedisClient("redis://localhost:65535");
+      client.close();
+    }).not.toThrow();
+  });
+
+  test("should use default port 6379 when port is omitted", () => {
+    // When no port is specified, default to 6379
+    // This should not throw
+    expect(() => {
+      const client = new RedisClient("redis://localhost");
+      client.close();
+    }).not.toThrow();
+  });
+
+  test("should throw error for malformed port in URL", () => {
+    expect(() => {
+      new RedisClient("redis://localhost:abc");
+    }).toThrow(/(invalid port number|invalid url format)/i);
+
+    expect(() => {
+      new RedisClient("redis://localhost:12.34");
+    }).toThrow(/(invalid port number|invalid url format)/i);
+  });
+});