From d9a867a4b9d2711336260aab4ab946cc13f5b078 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Fri, 17 Oct 2025 14:49:28 -0700 Subject: [PATCH] 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 | 1 + src/bun.js/bindings/BunString.cpp | 15 +++ src/bun.js/bindings/URL.zig | 18 ++++ src/valkey/js_valkey.zig | 136 ++++++++++++++++------- test/regression/issue/23621.test.ts | 160 ++++++++++++++++++++++++++++ 5 files changed, 290 insertions(+), 40 deletions(-) create mode 100644 test/regression/issue/23621.test.ts diff --git a/docs/api/redis.md b/docs/api/redis.md index d8438bc3cf..6e330adcb9 100644 --- a/docs/api/redis.md +++ b/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 diff --git a/src/bun.js/bindings/BunString.cpp b/src/bun.js/bindings/BunString.cpp index 3fd23c573b..ce4bbd8f51 100644 --- a/src/bun.js/bindings/BunString.cpp +++ b/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()); diff --git a/src/bun.js/bindings/URL.zig b/src/bun.js/bindings/URL.zig index 8bd1c7b96a..2d75fe6301 100644 --- a/src/bun.js/bindings/URL.zig +++ b/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); diff --git a/src/valkey/js_valkey.zig b/src/valkey/js_valkey.zig index 24f77d2648..dd58159e9f 100644 --- a/src/valkey/js_valkey.zig +++ b/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; diff --git a/test/regression/issue/23621.test.ts b/test/regression/issue/23621.test.ts new file mode 100644 index 0000000000..6682d8d8c2 --- /dev/null +++ b/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); + }); +});