[client] Fix SSH proxy mangling shell quoting in forwarded commands

[lixmal]

Describe your changes

• Fix SSH proxy using session.Command() (shlex split + join) instead of session.RawCommand() to forward commands to the backend, which loses shell quoting and breaks tools like Ansible that send commands with subshells: /bin/sh -c '( umask 77 && ... ) && sleep 0'
• Apply same fix to server session handler's hasCommand check

Issue ticket number and link

Checklist

• Is it a bug fix
• Is a typo/documentation fix
• Is a feature enhancement
• It is a refactor
• Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

• I added/updated documentation for this change
• Documentation is not needed for this change (bug fix, no user-facing behavior change)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

Summary by CodeRabbit

• Bug Fixes

  • Improved SSH command handling to preserve shell quoting and special characters when forwarding commands through the proxy.

• Tests

  • Added comprehensive integration tests verifying proper handling of SSH commands with quotes, subshells, and special characters.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes modify SSH command detection and execution logic to use raw command strings instead of parsed command arrays, preserving shell quoting and special characters throughout the proxy and server. A comprehensive integration test validates the correct handling of quoted and special-character command scenarios.

Changes

Cohort / File(s)	Summary
SSH Command Detection Logic client/ssh/proxy/proxy.go, client/ssh/server/session_handlers.go	Changed command presence detection from len(session.Command()) > 0 to session.RawCommand() != "", and updated command execution to use session.RawCommand() instead of joined parsed command array to preserve shell quoting and special characters.
SSH Proxy Test Coverage client/ssh/proxy/proxy_test.go	Added integration test TestSSHProxy_CommandQuoting to verify command forwarding preserves quoting and special characters; introduced helper setupProxySSHClient for test infrastructure and stderr capture during command execution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• [client] Support non-PTY no-command interactive SSH sessions #5093 — Implements related SSH session command detection changes using session.RawCommand() instead of session.Command() for PTY vs command execution branching logic.

Suggested reviewers

• pappz
• mlsmaycon

Poem

🐰 Whiskers twitch at quoted commands so fine,
Raw strings preserve each shell design,
No more splitting what shouldn't break,
Special chars stay whole for the proxy's sake! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main bug fix: replacing session.Command() with session.RawCommand() to preserve shell quoting in SSH proxy command forwarding.
Description check	✅ Passed	The description covers the core issue, fix details, and checklist completion. While issue ticket and stack sections are empty, the essential context about the bug and solution is clearly provided.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/ssh-proxy-command-quoting

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

client/ssh/proxy/proxy_test.go (1)

420-429: Tighten helper cleanup to close all opened resources.

setupProxySSHClient allocates proxy connection and pipe FDs but cleanup currently doesn’t close all of them (or the proxy instance). Closing them reduces cross-test leakage and flakiness.

Suggested patch

 	cleanupFn := func() {
 		_ = client.Close()
 		_ = clientConn.Close()
+		_ = proxyConn.Close()
 		cancel()
 		os.Stdin = origStdin
 		os.Stdout = origStdout
+		_ = stdinReader.Close()
+		_ = stdinWriter.Close()
+		_ = stdoutReader.Close()
+		_ = stdoutWriter.Close()
+		_ = proxyInstance.Close()
 		_ = sshServer.Stop()
 		mockDaemon.stop()
 		jwksServer.Close()
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@client/ssh/proxy/proxy_test.go` around lines 420 - 429, The cleanupFn in
setupProxySSHClient doesn't close all opened descriptors—ensure you close the
proxy instance and any pipe FDs created by the helper in addition to existing
closes; specifically, add calls to close the proxy (e.g., proxy.Close() or
CloseProxy()), close the proxy connection (proxyConn.Close() if different from
clientConn), and close any stdin/stdout pipe endpoints created (e.g.,
stdinPipeReader/Writer, stdoutPipeReader/Writer) using the same `_ = ...Close()`
pattern so all resources allocated by setupProxySSHClient (client, clientConn,
proxy, proxyConn, pipe FDs, sshServer, mockDaemon, jwksServer) are closed and
cancel is called and stdio restored.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@client/ssh/proxy/proxy_test.go`:
- Around line 257-260: The test TestSSHProxy_CommandQuoting runs POSIX-only
/bin/sh payloads and should also skip on Windows CI; update the test (function
TestSSHProxy_CommandQuoting) to detect the OS using runtime.GOOS and call t.Skip
when runtime.GOOS == "windows", and add the runtime import to the test file's
imports so the build passes.

---

Nitpick comments:
In `@client/ssh/proxy/proxy_test.go`:
- Around line 420-429: The cleanupFn in setupProxySSHClient doesn't close all
opened descriptors—ensure you close the proxy instance and any pipe FDs created
by the helper in addition to existing closes; specifically, add calls to close
the proxy (e.g., proxy.Close() or CloseProxy()), close the proxy connection
(proxyConn.Close() if different from clientConn), and close any stdin/stdout
pipe endpoints created (e.g., stdinPipeReader/Writer, stdoutPipeReader/Writer)
using the same `_ = ...Close()` pattern so all resources allocated by
setupProxySSHClient (client, clientConn, proxy, proxyConn, pipe FDs, sshServer,
mockDaemon, jwksServer) are closed and cancel is called and stdio restored.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0bb0aaa7-57b3-462d-9a53-bdd51bf7f34c

📥 Commits

Reviewing files that changed from the base of the PR and between 5b85edb and 3692a2a.

📒 Files selected for processing (3)

• client/ssh/proxy/proxy.go
• client/ssh/proxy/proxy_test.go
• client/ssh/server/session_handlers.go

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud