fix(sshproxy): run docker exec as the devcontainer's remoteUser

CLAUDE.md

@@ -227,7 +227,7 @@ DEUCE_WS_ALLOWED_ORIGINS=vmname.exe.xyz
 The browser **Terminal panel** uses `devpod ssh` (VM sshd → DevPod's in-container shim → user shell). The **Open-in-VS-Code** path uses `docker exec`. They diverge on:
 
 - **Environment:** `docker exec` does not set `SSH_*` vars; the SSH proxy explicitly forwards only `VSCODE_*`, `LANG`, `LC_*`, `TERM`, `HOME`, `USER`, `SHELL` (allowlist defeats `LD_PRELOAD` injection). The browser terminal inherits whatever DevPod forwards.
-- **UID:** `devpod ssh` runs as the devcontainer's `remoteUser` (typically `vscode`/`node`). `docker exec` defaults to the image's `USER` directive (which is often the same user, but not guaranteed — verify per image).
+- **UID:** both paths run as the devcontainer's `remoteUser` (typically `vscode`/`node`). `devpod ssh` honors it natively; the SSH proxy resolves it from the container's `devcontainer.metadata` label and passes `--user` (`workspace.ContainerUser`). Without that, `docker exec` would fall back to the image's `USER` — commonly `root` — and git would reject the `remoteUser`-owned workspace with `fatal: detected dubious ownership`. A container declaring neither `remoteUser` nor `containerUser` still falls back to the image's `USER`. Note the `devpod.user` label is *not* this value; it reports `root` on images that declare a `remoteUser`.
 - **Shell startup:** `docker exec -it /bin/bash -l` is a login shell; the terminal panel's interactive bash is non-login.
 
 Users may see "my git config works in the terminal but not VS Code" surprises — document the divergence in user-facing docs. Convergence (route the terminal panel through the SSH proxy too) is reserved as a v2 cleanup.

server/internal/sshproxy/docker.go

@@ -2,6 +2,7 @@ package sshproxy
 
 import (
 	"context"
+	"log/slog"
 	"os/exec"
 	"strconv"
 	"strings"
@@ -76,6 +77,27 @@ type envEntry struct {
 	Value string
 }
 
+// resolveExecUser returns the user docker exec should run as for this
+// container, or "" to leave the image's USER in place.
+//
+// Deliberately fail-open: a devcontainer that declares no remoteUser, a
+// docker inspect that errors, or a proxy built without a workspace manager
+// (tests) all degrade to the previous behaviour rather than killing the
+// channel. The cost of guessing wrong is a git ownership warning; the cost
+// of erroring out is an unusable editor session.
+func (s *Server) resolveExecUser(ctx context.Context, container string) string {
+	if s.workspaces == nil {
+		return ""
+	}
+	user, err := s.workspaces.ContainerUser(ctx, container)
+	if err != nil {
+		slog.Warn("ssh: could not resolve container exec user; using image default",
+			"container", container, "error", err)
+		return ""
+	}
+	return user
+}
+
 // execMode discriminates the docker-exec invocation shapes we use.
 type execMode int
 
@@ -97,11 +119,13 @@ const (
 // matters because docker exec spawns child processes inside the container
 // namespace but the host-side `docker` binary itself may leave grandchild
 // monitors around.
-func buildExecCmd(ctx context.Context, bin, container, command string, mode execMode, env []string) *exec.Cmd {
+// `user` is the devcontainer's remoteUser (see workspace.ContainerUser);
+// empty leaves the image's USER in place.
+func buildExecCmd(ctx context.Context, bin, container, command string, mode execMode, env []string, user string) *exec.Cmd {
 	if bin == "" {
 		bin = defaultDockerBin
 	}
-	args := dockerArgs(container, command, mode)
+	args := dockerArgs(container, command, mode, user)
 	cmd := exec.CommandContext(ctx, bin, args...)
 	cmd.Env = env
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
@@ -131,32 +155,49 @@ func buildExecCmd(ctx context.Context, bin, container, command string, mode exec
 // the inputs go through ssh.Unmarshal of a typed payload struct (host
 // is a length-prefixed string, port is a uint32), so the only attack
 // surface is the validation rules — not shell-parsing.
-func buildTCPForwardCmd(ctx context.Context, bin, container, host string, port uint32) *exec.Cmd {
+func buildTCPForwardCmd(ctx context.Context, bin, container, host string, port uint32, user string) *exec.Cmd {
 	if bin == "" {
 		bin = defaultDockerBin
 	}
 	script := "exec 3<>/dev/tcp/" + host + "/" + strconv.FormatUint(uint64(port), 10) + " || exit 1\n" +
 		"( cat <&3; kill -TERM $$ 2>/dev/null ) &\n" +
 		"cat >&3\n"
-	cmd := exec.CommandContext(ctx, bin, "exec", "-i", container, "bash", "-c", script)
+	args := []string{"exec"}
+	if user != "" {
+		args = append(args, "--user", user)
+	}
+	args = append(args, "-i", container, "bash", "-c", script)
+	cmd := exec.CommandContext(ctx, bin, args...)
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
 	return cmd
 }
 
 // dockerArgs builds the argv slice for a docker exec invocation in the
 // given mode. Exposed for unit tests so they can assert the exact argv
 // without running docker.
-func dockerArgs(container, command string, mode execMode) []string {
+// `user`, when non-empty, is passed as `--user` so the exec runs as the
+// devcontainer's remoteUser rather than the image's USER. Those differ on
+// most devcontainer images (USER root + remoteUser vscode), and the mismatch
+// makes git reject the workspace as dubiously-owned. Empty means "leave the
+// image's USER alone" — an empty `--user` would make docker reject the exec.
+func dockerArgs(container, command string, mode execMode, user string) []string {
+	// The flag belongs to `docker exec`, so it must precede the container
+	// name; anything after that is the in-container command.
+	pre := []string{"exec"}
+	if user != "" {
+		pre = append(pre, "--user", user)
+	}
+
 	switch mode {
 	case execModePTYShell:
-		return []string{"exec", "-it", container, "/bin/bash", "-l"}
+		return append(pre, "-it", container, "/bin/bash", "-l")
 	case execModeNonPTYShell:
-		return []string{"exec", "-i", container, "/bin/bash", "-l"}
+		return append(pre, "-i", container, "/bin/bash", "-l")
 	case execModePTYExec:
-		return []string{"exec", "-it", container, "/bin/sh", "-c", command}
+		return append(pre, "-it", container, "/bin/sh", "-c", command)
 	case execModeSFTP:
-		return []string{"exec", "-i", container, "/usr/lib/openssh/sftp-server", "-e"}
+		return append(pre, "-i", container, "/usr/lib/openssh/sftp-server", "-e")
 	default: // execModeNonPTY
-		return []string{"exec", "-i", container, "/bin/sh", "-c", command}
+		return append(pre, "-i", container, "/bin/sh", "-c", command)
 	}
 }

server/internal/sshproxy/docker_test.go

@@ -0,0 +1,94 @@
+package sshproxy
+
+import (
+	"context"
+	"slices"
+	"strings"
+	"testing"
+)
+
+// allModes covers every docker-exec shape the proxy builds. A missing
+// --user in any one of them puts that channel back on the image's USER,
+// which is what produced `fatal: detected dubious ownership` in VS Code
+// terminals: the image declares USER root while the workspace tree is
+// owned by the devcontainer's remoteUser.
+var allModes = []struct {
+	name string
+	mode execMode
+}{
+	{"non-pty", execModeNonPTY},
+	{"pty-exec", execModePTYExec},
+	{"pty-shell", execModePTYShell},
+	{"non-pty-shell", execModeNonPTYShell},
+	{"sftp", execModeSFTP},
+}
+
+func TestDockerArgsRunsAsResolvedUser(t *testing.T) {
+	for _, m := range allModes {
+		t.Run(m.name, func(t *testing.T) {
+			args := dockerArgs("alice", "echo hi", m.mode, "vscode")
+
+			i := slices.Index(args, "--user")
+			if i < 0 {
+				t.Fatalf("dockerArgs(%s) has no --user: %#v", m.name, args)
+			}
+			if i+1 >= len(args) || args[i+1] != "vscode" {
+				t.Fatalf("dockerArgs(%s) --user value wrong: %#v", m.name, args)
+			}
+			// The flag belongs to `docker exec`, so it has to precede the
+			// container name — after it, docker treats it as part of the
+			// in-container command.
+			c := slices.Index(args, "alice")
+			if c < 0 || i > c {
+				t.Errorf("dockerArgs(%s) --user must precede the container: %#v", m.name, args)
+			}
+		})
+	}
+}
+
+func TestDockerArgsOmitsUserWhenUnknown(t *testing.T) {
+	// No declared user means today's behaviour: let the image's USER stand.
+	// Passing an empty --user would make docker reject the exec outright.
+	for _, m := range allModes {
+		t.Run(m.name, func(t *testing.T) {
+			args := dockerArgs("alice", "echo hi", m.mode, "")
+
+			if slices.Contains(args, "--user") {
+				t.Errorf("dockerArgs(%s) sent --user with no resolved user: %#v", m.name, args)
+			}
+			if slices.Contains(args, "") {
+				t.Errorf("dockerArgs(%s) has an empty argv slot: %#v", m.name, args)
+			}
+		})
+	}
+}
+
+func TestBuildExecCmdPassesUser(t *testing.T) {
+	cmd := buildExecCmd(context.Background(), "docker", "alice", "echo hi", execModePTYShell, nil, "vscode")
+
+	if !slices.Contains(cmd.Args, "--user") || !slices.Contains(cmd.Args, "vscode") {
+		t.Errorf("buildExecCmd argv missing user: %#v", cmd.Args)
+	}
+}
+
+func TestBuildSFTPCmdPassesUser(t *testing.T) {
+	// SFTP writes files into the workspace, so running it as the wrong user
+	// leaves root-owned files in a remoteUser-owned tree.
+	cmd := buildSFTPCmd(context.Background(), "docker", "alice", "vscode")
+
+	if !slices.Contains(cmd.Args, "--user") || !slices.Contains(cmd.Args, "vscode") {
+		t.Errorf("buildSFTPCmd argv missing user: %#v", cmd.Args)
+	}
+}
+
+func TestBuildTCPForwardCmdPassesUser(t *testing.T) {
+	cmd := buildTCPForwardCmd(context.Background(), "docker", "alice", "127.0.0.1", 8080, "vscode")
+
+	if !slices.Contains(cmd.Args, "--user") || !slices.Contains(cmd.Args, "vscode") {
+		t.Errorf("buildTCPForwardCmd argv missing user: %#v", cmd.Args)
+	}
+	// The forwarding script must survive the added flag intact.
+	if !strings.Contains(strings.Join(cmd.Args, " "), "/dev/tcp/127.0.0.1/8080") {
+		t.Errorf("buildTCPForwardCmd lost its script: %#v", cmd.Args)
+	}
+}

server/internal/sshproxy/session.go

@@ -330,14 +330,16 @@ func (s *Server) runSessionChannel(
 				return
 			}
 
+			user := s.resolveExecUser(chanCtx, container)
+
 			if mode == execModeSFTP {
 				// SFTP doesn't honor client-supplied env vars and forwarding
 				// LANG/TERM to the docker CLI would strip its PATH. Use a
 				// dedicated builder that leaves cmd.Env nil (inherit parent).
-				cmd = buildSFTPCmd(chanCtx, s.dockerBin, container)
+				cmd = buildSFTPCmd(chanCtx, s.dockerBin, container, user)
 			} else {
 				env := filterEnv(envBuf)
-				cmd = buildExecCmd(chanCtx, s.dockerBin, container, command, mode, env)
+				cmd = buildExecCmd(chanCtx, s.dockerBin, container, command, mode, env, user)
 			}
 
 			if mode == execModePTYShell || mode == execModePTYExec {

server/internal/sshproxy/session_test.go

@@ -25,15 +25,15 @@ import (
 // ----------------------------------------------------------------------
 
 func TestDockerArgs_NonPTY(t *testing.T) {
-	got := dockerArgs("alice", "echo hi", execModeNonPTY)
+	got := dockerArgs("alice", "echo hi", execModeNonPTY, "")
 	want := []string{"exec", "-i", "alice", "/bin/sh", "-c", "echo hi"}
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("dockerArgs(non-pty):\n got: %#v\nwant: %#v", got, want)
 	}
 }
 
 func TestDockerArgs_PTYShell(t *testing.T) {
-	got := dockerArgs("alice", "", execModePTYShell)
+	got := dockerArgs("alice", "", execModePTYShell, "")
 	want := []string{"exec", "-it", "alice", "/bin/bash", "-l"}
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("dockerArgs(pty-shell):\n got: %#v\nwant: %#v", got, want)
@@ -45,23 +45,23 @@ func TestDockerArgs_PTYShell(t *testing.T) {
 // pty driver can't echo VS Code's piped install-script bytes back and
 // corrupt the byte stream it parses.
 func TestDockerArgs_NonPTYShell(t *testing.T) {
-	got := dockerArgs("alice", "", execModeNonPTYShell)
+	got := dockerArgs("alice", "", execModeNonPTYShell, "")
 	want := []string{"exec", "-i", "alice", "/bin/bash", "-l"}
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("dockerArgs(non-pty-shell):\n got: %#v\nwant: %#v", got, want)
 	}
 }
 
 func TestDockerArgs_PTYExec(t *testing.T) {
-	got := dockerArgs("alice", "ls /", execModePTYExec)
+	got := dockerArgs("alice", "ls /", execModePTYExec, "")
 	want := []string{"exec", "-it", "alice", "/bin/sh", "-c", "ls /"}
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("dockerArgs(pty-exec):\n got: %#v\nwant: %#v", got, want)
 	}
 }
 
 func TestDockerArgs_SFTP(t *testing.T) {
-	got := dockerArgs("alice", "", execModeSFTP)
+	got := dockerArgs("alice", "", execModeSFTP, "")
 	want := []string{"exec", "-i", "alice", "/usr/lib/openssh/sftp-server", "-e"}
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("dockerArgs(sftp):\n got: %#v\nwant: %#v", got, want)
@@ -71,7 +71,7 @@ func TestDockerArgs_SFTP(t *testing.T) {
 func TestBuildExecCmd_SetsPgidAndEnv(t *testing.T) {
 	ctx := context.Background()
 	env := []string{"LANG=C", "VSCODE_X=1"}
-	cmd := buildExecCmd(ctx, "/usr/bin/docker", "alice", "echo hi", execModeNonPTY, env)
+	cmd := buildExecCmd(ctx, "/usr/bin/docker", "alice", "echo hi", execModeNonPTY, env, "")
 	if cmd.SysProcAttr == nil || !cmd.SysProcAttr.Setpgid {
 		t.Errorf("expected Setpgid: true, got %#v", cmd.SysProcAttr)
 	}
@@ -84,7 +84,7 @@ func TestBuildExecCmd_SetsPgidAndEnv(t *testing.T) {
 }
 
 func TestBuildExecCmd_EmptyBinUsesDefault(t *testing.T) {
-	cmd := buildExecCmd(context.Background(), "", "alice", "echo hi", execModeNonPTY, nil)
+	cmd := buildExecCmd(context.Background(), "", "alice", "echo hi", execModeNonPTY, nil, "")
 	if got := cmd.Args[0]; got != defaultDockerBin && !strings.HasSuffix(got, defaultDockerBin) {
 		t.Errorf("empty bin should fall back to %q, got %q", defaultDockerBin, got)
 	}

server/internal/sshproxy/sftp.go

@@ -51,11 +51,11 @@ import (
 //   - No env is forwarded. SFTP doesn't honor client-supplied env vars,
 //     and the U8 allowlist would just be noise here.
 //   - argv MUST be `-i`, never `-it`. See file-level comment above.
-func buildSFTPCmd(ctx context.Context, dockerBin, container string) *exec.Cmd {
+func buildSFTPCmd(ctx context.Context, dockerBin, container, user string) *exec.Cmd {
 	if dockerBin == "" {
 		dockerBin = defaultDockerBin
 	}
-	cmd := exec.CommandContext(ctx, dockerBin, dockerArgs(container, "", execModeSFTP)...)
+	cmd := exec.CommandContext(ctx, dockerBin, dockerArgs(container, "", execModeSFTP, user)...)
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
 	return cmd
 }

server/internal/sshproxy/sftp_test.go

@@ -21,7 +21,7 @@ import (
 // ----------------------------------------------------------------------
 
 func TestBuildSFTPCmd_ArgvIsDockerExecIWithSFTPServerE(t *testing.T) {
-	cmd := buildSFTPCmd(context.Background(), "/usr/bin/docker", "session-container")
+	cmd := buildSFTPCmd(context.Background(), "/usr/bin/docker", "session-container", "")
 
 	// argv[0] is the binary path; argv[1:] is what we passed.
 	wantArgs := []string{"/usr/bin/docker", "exec", "-i", "session-container", "/usr/lib/openssh/sftp-server", "-e"}
@@ -34,7 +34,7 @@ func TestBuildSFTPCmd_NeverUsesPTYFlag(t *testing.T) {
 	// SFTP is binary framing; -it would CRLF-translate and corrupt
 	// packets. The regression risk is that someone "fixes" interactive
 	// SFTP by adding -t, so we lock the constraint down explicitly.
-	cmd := buildSFTPCmd(context.Background(), "docker", "alice")
+	cmd := buildSFTPCmd(context.Background(), "docker", "alice", "")
 	for _, a := range cmd.Args {
 		if a == "-it" || a == "-t" {
 			t.Errorf("SFTP argv must not include %q (PTY would corrupt binary framing): %#v", a, cmd.Args)
@@ -45,7 +45,7 @@ func TestBuildSFTPCmd_NeverUsesPTYFlag(t *testing.T) {
 func TestBuildSFTPCmd_NoShellWrapper(t *testing.T) {
 	// Argv must hit sftp-server directly, not via /bin/sh -c. A shell
 	// wrapper would buffer stdout and add a syscall layer for no gain.
-	cmd := buildSFTPCmd(context.Background(), "docker", "alice")
+	cmd := buildSFTPCmd(context.Background(), "docker", "alice", "")
 	for _, a := range cmd.Args {
 		if a == "/bin/sh" || a == "/bin/bash" || a == "-c" {
 			t.Errorf("SFTP argv must not invoke a shell, got: %#v", cmd.Args)
@@ -54,7 +54,7 @@ func TestBuildSFTPCmd_NoShellWrapper(t *testing.T) {
 }
 
 func TestBuildSFTPCmd_EmptyBinUsesDefault(t *testing.T) {
-	cmd := buildSFTPCmd(context.Background(), "", "alice")
+	cmd := buildSFTPCmd(context.Background(), "", "alice", "")
 	if got := cmd.Args[0]; got != defaultDockerBin && !strings.HasSuffix(got, defaultDockerBin) {
 		t.Errorf("empty bin should fall back to %q, got %q", defaultDockerBin, got)
 	}
@@ -63,7 +63,7 @@ func TestBuildSFTPCmd_EmptyBinUsesDefault(t *testing.T) {
 func TestBuildSFTPCmd_SetsPgid(t *testing.T) {
 	// Setpgid: true is required for the channel-close cleanup path —
 	// kill(-pid) reaches the docker CLI and its grandchildren.
-	cmd := buildSFTPCmd(context.Background(), "docker", "alice")
+	cmd := buildSFTPCmd(context.Background(), "docker", "alice", "")
 	if cmd.SysProcAttr == nil || !cmd.SysProcAttr.Setpgid {
 		t.Errorf("expected Setpgid: true, got %#v", cmd.SysProcAttr)
 	}

server/internal/sshproxy/tcpip.go

@@ -84,7 +84,8 @@ func (s *Server) runDirectTCPIPChannel(
 		return
 	}
 
-	cmd := buildTCPForwardCmd(chanCtx, s.dockerBin, container, payload.DestHost, payload.DestPort)
+	user := s.resolveExecUser(chanCtx, container)
+	cmd := buildTCPForwardCmd(chanCtx, s.dockerBin, container, payload.DestHost, payload.DestPort, user)
 
 	stdinPipe, err := cmd.StdinPipe()
 	if err != nil {

server/internal/sshproxy/tcpip_test.go

@@ -75,7 +75,7 @@ func TestValidateLoopbackDest_RejectsBadPorts(t *testing.T) {
 // directions — are all locked.
 func TestBuildTCPForwardCmd_Argv(t *testing.T) {
 	ctx := context.Background()
-	cmd := buildTCPForwardCmd(ctx, "/usr/bin/docker", "alice", "127.0.0.1", 40301)
+	cmd := buildTCPForwardCmd(ctx, "/usr/bin/docker", "alice", "127.0.0.1", 40301, "")
 
 	if cmd.SysProcAttr == nil || !cmd.SysProcAttr.Setpgid {
 		t.Errorf("expected Setpgid: true, got %#v", cmd.SysProcAttr)
@@ -109,7 +109,7 @@ func TestBuildTCPForwardCmd_Argv(t *testing.T) {
 // TestBuildExecCmd_EmptyBinUsesDefault — empty `bin` should resolve to
 // the package default ("docker" from $PATH).
 func TestBuildTCPForwardCmd_EmptyBinUsesDefault(t *testing.T) {
-	cmd := buildTCPForwardCmd(context.Background(), "", "alice", "127.0.0.1", 22)
+	cmd := buildTCPForwardCmd(context.Background(), "", "alice", "127.0.0.1", 22, "")
 	if len(cmd.Args) < 1 {
 		t.Fatalf("empty argv: %v", cmd.Args)
 	}