fix(cmd/ssh): default ssh workdir to merged workspaceFolder#517
Merged
skevetter merged 4 commits intoskevetter:mainfrom Feb 25, 2026
Merged
fix(cmd/ssh): default ssh workdir to merged workspaceFolder#517skevetter merged 4 commits intoskevetter:mainfrom
skevetter merged 4 commits intoskevetter:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts workdir resolution into helpers and changes SSH invocation to build a shell-escaped argument slice (using shellescape), appends flags programmatically, and wraps with Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Devpod CLI
participant WS as WorkspaceClient
participant Builder as Command Builder
participant Shell as Remote Shell / SSH process
participant GPG as GPG Agent Setup
CLI->>WS: query merged workspace config
WS-->>CLI: mergedConfig.WorkspaceFolder (or empty)
CLI->>Builder: resolveWorkdir(flag, mergedConfig, workspaceID)
Builder-->>Builder: construct args slice, append flags, quote parts
Builder->>GPG: prepare GPG/SSH-forwarding commands (if needed)
GPG-->>Builder: quoted GPG setup snippets
Builder->>Shell: start SSH with composed, quoted command (wrap with su -c if non-root)
Shell-->>CLI: SSH session established / output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ssh.go`:
- Around line 523-539: The code reads an attacker-controlled WorkspaceFolder
into SSHCmd.resolveWorkdir (result.MergedConfig.WorkspaceFolder) and later
interpolates that into a single-quoted shell command via fmt.Sprintf("'%s'
helper ssh-server ... --workdir '%s'", agent.ContainerDevPodHelperLocation,
workdir), allowing single-quote injection; fix by either rejecting/sanitizing
any workdir containing single quotes (and other shell metacharacters) before
returning from resolveWorkdir, or — preferably — stop building a shell string
and use an exec-style argument list / API that passes
agent.ContainerDevPodHelperLocation and the --workdir value as separate args
(avoiding a shell interpreter), updating the code paths that construct the
command to use the safe argument form instead of string interpolation.
- Around line 523-539: The resolveWorkdir function may call
provider.LoadWorkspaceResult with an empty workspaceConfig.Context causing
spurious debug logs; update SSHCmd.resolveWorkdir to first check
workspaceConfig.Context is non-empty before calling provider.LoadWorkspaceResult
(i.e., only call LoadWorkspaceResult(workspaceConfig.Context,
workspaceConfig.ID) when workspaceConfig.Context != ""), and otherwise skip that
lookup and proceed to the existing fallback logic that returns
filepath.Join("/workspaces", workspaceClient.Workspace()).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/ssh.go (1)
610-633:setupGPGAgentstill uses the old unescaped string-join pattern — consider aligning withshellescape.After this PR's fix,
startTunnelconsistently usesshellescape.QuoteCommand, butsetupGPGAgentstill:
- Joins
forwardAgentwith a barestrings.Join(forwardAgent, " ")(Line 631), leaving any metacharacters in the helper-path or flag values unescaped.- Manually single-quotes
gitKeyviafmt.Sprintf("'%s'", gitKey)(Line 628).gitKeyoriginates fromgit config user.signingKey, which is read from the cloned repo's git config and is therefore attacker-controlled. A value containing'would break out of the single-quote context.- Wraps the
su -ccommand with double-quote interpolation (Line 633) instead of the shellescape-based wrapping.♻️ Proposed refactor to align with the new pattern
- command := strings.Join(forwardAgent, " ") + command := shellescape.QuoteCommand(forwardAgent) if cmd.User != "" && cmd.User != "root" { - command = fmt.Sprintf("su -c \"%s\" '%s'", command, cmd.User) + command = shellescape.QuoteCommand([]string{"su", "-c", command, cmd.User}) }Also replace the manual quoting of
gitKey:if len(gitGpgKey) > 0 { gitKey := strings.TrimSpace(string(gitGpgKey)) forwardAgent = append(forwardAgent, "--gitkey") - forwardAgent = append(forwardAgent, fmt.Sprintf("'%s'", gitKey)) + forwardAgent = append(forwardAgent, gitKey) }(With the first change,
shellescape.QuoteCommandwill handle quotinggitKeycorrectly as part offorwardAgent.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ssh.go` around lines 610 - 633, setupGPGAgent builds the shell command insecurely by joining forwardAgent with strings.Join and manually quoting gitKey and su -c; replace that with shellescape.QuoteCommand to safely quote each argument. Specifically, stop doing fmt.Sprintf("'%s'", gitKey) and instead append the raw gitKey (from gitGpgKey trimmed) to forwardAgent, then replace command := strings.Join(forwardAgent, " ") and the su -c wrapping with command := shellescape.QuoteCommand(forwardAgent) (and if you must run as non-root wrap with su -c using shellescape.QuoteArgument or equivalent to quote the entire command safely); follow the same pattern used in startTunnel and ensure the debug flag logic and ownerTrust/socket args remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/ssh.go`:
- Around line 610-633: setupGPGAgent builds the shell command insecurely by
joining forwardAgent with strings.Join and manually quoting gitKey and su -c;
replace that with shellescape.QuoteCommand to safely quote each argument.
Specifically, stop doing fmt.Sprintf("'%s'", gitKey) and instead append the raw
gitKey (from gitGpgKey trimmed) to forwardAgent, then replace command :=
strings.Join(forwardAgent, " ") and the su -c wrapping with command :=
shellescape.QuoteCommand(forwardAgent) (and if you must run as non-root wrap
with su -c using shellescape.QuoteArgument or equivalent to quote the entire
command safely); follow the same pattern used in startTunnel and ensure the
debug flag logic and ownerTrust/socket args remain unchanged.
skevetter
reviewed
Feb 25, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ssh.go`:
- Line 542: Swap the OS-specific filepath.Join call to POSIX path.Join for the
container workdir fallback: replace the return expression that currently uses
filepath.Join("/workspaces", workspaceClient.Workspace()) with
path.Join("/workspaces", workspaceClient.Workspace()) and update imports to
include the standard "path" package (and remove or keep filepath only if still
needed elsewhere) so the container --workdir always uses forward slashes.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/ssh.go (1)
533-543: LGTM — workdir resolution priority chain is correct.
--workdirflag →mergedWorkspaceFolder→path.Join("/workspaces", ...)fallback matches the stated priority order from the issue, and usingpath.Join(POSIX) instead offilepath.Joinensures forward slashes on Windows hosts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ssh.go` around lines 533 - 543, Summary: The workdir resolution in function resolveWorkdir is correct and follows the intended priority chain (--workdir, resolveMergedWorkspaceFolder, fallback to path.Join("/workspaces", workspaceClient.Workspace())). No code changes required; keep resolveWorkdir and its use of resolveMergedWorkspaceFolder and path.Join as-is to preserve POSIX-style slashes on Windows and the documented priority order.
🧹 Nitpick comments (1)
cmd/ssh.go (1)
174-250:resolveWorkdiris not applied in the tailscale/DaemonClientpath.
jumpContainerTailscalecallsmachine.RunSSHSessiondirectly and never invokesstartTunnel, so the new workdir resolution (--workdirflag,mergedWorkspaceFolder, fallback) has no effect for Tailscale-backed workspaces. If users of that path rely on the devcontainerworkspaceFolder, they won't benefit from this fix.This is pre-existing behaviour and out of scope for this PR, but worth a follow-up issue to bring the tailscale path to parity.
Would you like me to open a follow-up issue to track
workspaceFolderresolution for the tailscale/DaemonClientpath?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ssh.go` around lines 174 - 250, jumpContainerTailscale bypasses resolveWorkdir because it calls machine.RunSSHSession/DirectTunnel directly instead of using the existing startTunnel flow that applies workdir resolution; update jumpContainerTailscale to resolve the workspace before starting the session by reusing the same resolveWorkdir/startTunnel logic (or call startTunnel) used for non-Tailscale paths so the --workdir flag and mergedWorkspaceFolder fallback are honored; locate references to jumpContainerTailscale, startTunnel, resolveWorkdir, client.DirectTunnel, and machine.RunSSHSession to implement the change and ensure the resolved workdir is injected into the SSH session options or tunnel startup flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/ssh.go`:
- Around line 533-543: Summary: The workdir resolution in function
resolveWorkdir is correct and follows the intended priority chain (--workdir,
resolveMergedWorkspaceFolder, fallback to path.Join("/workspaces",
workspaceClient.Workspace())). No code changes required; keep resolveWorkdir and
its use of resolveMergedWorkspaceFolder and path.Join as-is to preserve
POSIX-style slashes on Windows and the documented priority order.
---
Nitpick comments:
In `@cmd/ssh.go`:
- Around line 174-250: jumpContainerTailscale bypasses resolveWorkdir because it
calls machine.RunSSHSession/DirectTunnel directly instead of using the existing
startTunnel flow that applies workdir resolution; update jumpContainerTailscale
to resolve the workspace before starting the session by reusing the same
resolveWorkdir/startTunnel logic (or call startTunnel) used for non-Tailscale
paths so the --workdir flag and mergedWorkspaceFolder fallback are honored;
locate references to jumpContainerTailscale, startTunnel, resolveWorkdir,
client.DirectTunnel, and machine.RunSSHSession to implement the change and
ensure the resolved workdir is injected into the SSH session options or tunnel
startup flow.
skevetter
approved these changes
Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
devpod sshprefermergedConfig.WorkspaceFolderas the default workdir when--workdiris not provided--workdiras the highest-priority override and preserve fallback to/workspaces/<workspace-id>when no merged workspace folder is availableCloses #516
Summary by CodeRabbit