Repository navigation
feat(connections): connect through a Unix socket, locally or over the SSH tunnel (#1002) - #1250
Conversation
… SSH tunnel Port of TabularisDB#492 onto current main. A single optional `unix_socket_path` on the connection: dialed locally by the MySQL and PostgreSQL drivers instead of host:port, or by the SSH server (direct-streamlocal / ssh -L port:/path) when the tunnel is enabled. Gated by the new `unix_socket` driver capability; the Kubernetes tunnel ignores it. Adapted while porting (markankamaransheogarat-sys): - SshTunnel::new_with_tcp_override (SSH-scope proxy) takes the forward destination instead of remote host/port, so the proxy keeps working - the AWS SSM tunnel, added since, clears the socket path like K8s - a direct socket connection skips the database-scope proxy - build_postgres_configurations returns Result on main; the socket branch is unix-only (tokio_postgres has no host_path on Windows) and Windows gets a clear error, as for MySQL - build_mysql_options keeps main's IAM/cleartext rules and adds the socket as an endpoint with TLS forced off - the UI is placed in main's connection form next to the plugin host/port field overrides Co-Authored-By: markankamaransheogarat-sys <279850453+markankamaransheogarat-sys@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QF7qKtRn5r6CrdDoU7ZmvY
The two locales were added after TabularisDB#492 was written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QF7qKtRn5r6CrdDoU7ZmvY
… more socket tests (TabularisDB#1002) On top of the TabularisDB#492 port, the parts of TabularisDB#1002 it did not cover: - mysql://user@host/db?socket=/path connection strings import as a socket connection (unix_socket_path) - the local socket endpoint is hidden on Windows (hasUnixSockets); a socket reached through the SSH tunnel stays available there, since the SSH server dials it, and the backend rejects a local socket on Windows with a clear error for MySQL and PostgreSQL - AWS IAM authentication is rejected over a local socket - tests: parser (?socket=, percent-encoding, blank, other drivers), hasUnixSockets, IAM rejection, Windows rejection Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QF7qKtRn5r6CrdDoU7ZmvY
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * @param params - Connection parameters (any object carrying the three fields) | ||
| * @returns The trimmed socket path, or null when none applies | ||
| */ | ||
| /** |
There was a problem hiding this comment.
WARNING: hasUnixSockets was inserted between the effectiveLocalSocketPath JSDoc and its function
The doc block above (lines 181-188, "The local Unix socket path in effect for a connection...") now sits directly before the hasUnixSockets JSDoc, so effectiveLocalSocketPath (below hasUnixSockets) has no documentation and an orphaned doc block precedes an unrelated function. Move hasUnixSockets (with its JSDoc) above the effectiveLocalSocketPath doc block, or below effectiveLocalSocketPath.
|
|
||
| const socketPath = effectiveLocalSocketPath(params); | ||
| if (socketPath) { | ||
| return `${socketPath}/${params.database}`; |
There was a problem hiding this comment.
SUGGESTION: Socket display string reads as a filesystem path
${socketPath}/${params.database} produces e.g. /tmp/mysql.sock/mydb, which looks like a path inside the socket file. connectionSubtitle uses socketPath · db for the same case. Consider a distinct, unambiguous format (e.g. socket:/tmp/mysql.sock/mydb) so the label is not mistaken for a path and stays consistent with the pool key's socket:<path> convention.
| // `mysql://root@localhost/db?socket=/tmp/mysql.sock`: the socket replaces | ||
| // host:port (the host is kept so the form can switch back). | ||
| const unix_socket_path = | ||
| resolved.id === 'mysql' ? url.searchParams.get('socket')?.trim() || undefined : undefined; |
There was a problem hiding this comment.
SUGGESTION: ?socket= import is hardcoded to the driver id mysql
resolved.id === 'mysql' ignores ?socket= for any other MySQL-dialect or unix_socket-capable driver that owns the mysql/mariadb scheme (e.g. a MariaDB plugin), even though the rest of the PR gates the feature on the unix_socket capability. .rules/plugins.md asks for capabilities over driver ids; consider checking the driver's unix_socket capability / sql_dialect === 'mysql' here (the modal already does this at importingMysql).
| connection_uri_in_keychain: false, | ||
| // `?socket=` switches the form to the socket endpoint; a string | ||
| // without it goes back to host and port. | ||
| unix_socket_path: parsed.unix_socket_path, |
There was a problem hiding this comment.
WARNING: Imported ?socket= can be saved while the socket UI is hidden (Windows)
On Windows hasUnixSockets() is false, so canUseSocket/socketMode are false (unless SSH is on) and the host/port inputs are shown, but this line still writes the imported unix_socket_path into formData. It is then persisted and sent to the backend, where local_unix_socket_path() is set and build_mysql_options returns "Unix socket connections are not supported on Windows" - with no visible field the user can edit to fix it, and host/port are silently ignored. Drop the parsed socket when !hasUnixSockets() (and no SSH), or clear unix_socket_path when the socket endpoint is not selectable.
| "socketPathForwardActive": "The SSH server connects to this socket instead of Host and Port. A database on a socket cannot negotiate TLS, so it is turned off; the SSH tunnel still encrypts the whole path.", | ||
| "socketPathNotAbsolute": "Enter an absolute path, as it appears on the machine that connects to the socket.", | ||
| "socketPathDirectory": "Point at the socket file itself, not the directory holding it.", | ||
| "socketPathTunnel": "Not used while a Kubernetes tunnel is enabled.", |
There was a problem hiding this comment.
SUGGESTION: newConnection.socketPathTunnel is never used
The key is added to all 11 locales but nothing in src/ references it (the endpoint selector is hidden while K8s is enabled, so the message is never rendered). Either render it where the socket is ignored or drop the key from every locale.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (16 files in this increment)
Notes: The latest commit resolves all five earlier findings, and the changed code raises no new issues. No code was run (read-only mode). Previous Review Summaries (2 snapshots, latest commit 96af7ee)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 96af7ee)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file in this increment)
Notes: The latest commit only swaps Fix these issues in Kilo Cloud Previous review (commit 5f47447)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (33 files)
Notes: Rust changes were reviewed by reading only; no tests were run (read-only mode). SSH, K8s and SSM tunnels clear the socket path before the drivers see it, and the pool key and driver options use Reviewed by claude-sonnet-5.5 · Input: 248 · Output: 1.4K · Cached: 197.3K Review guidance: REVIEW.md from base branch |
markankamaransheogarat-sys
changed the title
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QF7qKtRn5r6CrdDoU7ZmvY
…abularisDB#1002) - keep effectiveLocalSocketPath's JSDoc on the function it documents - do not import ?socket= where the socket field is hidden (Windows without SSH) - show a socket connection as `path (db)` instead of `path/db` - gate ?socket= on the unix_socket capability instead of the driver id - drop the unused socketPathTunnel string from every locale Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QF7qKtRn5r6CrdDoU7ZmvY
This branch has not been deployed
2 participants
Closes #1002.
This is a fresh PR from current
main, as agreed in #1246. It ports the design of #492 (@debba) and adds the #1002 extras on top. It supersedes both:unix_socket_path,unix_socketcapability, SSH streamlocal forwarding, PostgreSQLhost_path)Commits
feat(connections): Unix socket support…(author: @debba). The feat(connections): Unix socket support — dialed locally or through the SSH tunnel #492 changes, ported. While porting I adapted them to whatmainhas gained since:SshTunnel::new_with_tcp_override(SSH-scope proxy) now takes the forward destination instead of a remote host/port, so a socket can be reached through a proxied bastion too.build_postgres_configurationsreturnsResultonmain. The socket branch is#[cfg(unix)], since tokio_postgres has nohost_path/Host::Unixon Windows, and Windows gets a clear error.build_mysql_optionskeepsmain's IAM/cleartext rules and adds the socket endpoint with TLS forced off.main's form next to the pluginhost/portfield overrides.feat(i18n): … pt-BR and zh: the two locales added after feat(connections): Unix socket support — dialed locally or through the SSH tunnel #492.feat(connections): import ?socket=, hide the local socket on Windows…:mysql://user@host/db?socket=/pathimports as a socket connection.hasUnixSockets). A socket reached through SSH stays available there, because the SSH server dials it.Behaviour
MySqlConnectOptions::socket, TLS off, SSL files ignored, cleartext plugin allowed, IAM rejectedhost_path(dir)+ port parsed from.s.PGSQL.<port>(a directory also works), TLS offdirect-streamlocal@openssh.com(russh) /ssh -L port:/path(system ssh); the driver dials the forwarded local portThe socket path shows up in connection labels instead of
host:port. Ad-hoc pool keys usesocket:<path>as the endpoint.Tests
#[cfg(unix)], the Windows one is#[cfg(windows)], and both kinds run in CI.socket12,ssh_tunnel25,pool_manager94 passed. The full lib has 1938 passed; the 5 Windows-only path tests also fail onmain.unixSocketPathIssue,effectiveLocalSocketPath, labels, validation, the endpoint switch in the modal), plus?socket=parsing andhasUnixSockets.pnpm lint(eslint,prettier --check, theme tokens) andtsc -bare clean. Rustfmt is clean on the changed code; the remainingcargo fmt --checkdiffs are already onmain.🤖 Generated with Claude Code
https://claude.ai/code/session_01QF7qKtRn5r6CrdDoU7ZmvY