From ac2f69c0000bb320aae45a8b233ef2e2c3ce8340 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 13:45:56 +0000 Subject: [PATCH 01/28] fix(tunnel): thread resolved workspace user into browser-IDE SSH tunnel --- pkg/tunnel/browser.go | 19 ++++++++++++++++--- pkg/tunnel/browser_test.go | 33 +++++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 13 deletions(-) diff --git a/pkg/tunnel/browser.go b/pkg/tunnel/browser.go index 7594f158a..35d774d26 100644 --- a/pkg/tunnel/browser.go +++ b/pkg/tunnel/browser.go @@ -71,7 +71,7 @@ func startBrowserTunnelSSH(ctx context.Context, p BrowserTunnelParams) error { writer := log.Writer(log.LevelDebug) defer func() { _ = writer.Close() }() - sshCmd, err := CreateSSHCommand(ctx, p.Client, []string{ + sshCmd, err := CreateSSHCommand(ctx, p.Client, p.User, []string{ names.FlagValue(names.LogOutput, "raw"), names.FlagValue(names.ReuseSSHAuthSock, p.AuthSockID), names.Flag(names.Stdio), @@ -243,9 +243,14 @@ func isTransientBackhaulErr(err error) bool { } // CreateSSHCommand builds an exec.Cmd that runs `devsy ssh` with the given arguments. +// user is the workspace's remote user (e.g. from GetRemoteUser); it must match +// the user the container's ssh-server/gpg-setup sessions run as, or the two +// sessions collide on the shared /tmp coordination files (devsy-gpg-setup.lock, +// devsy.activity), which are owned by whichever session created them first. func CreateSSHCommand( ctx context.Context, client client2.BaseWorkspaceClient, + user string, extraArgs []string, ) (*exec.Cmd, error) { execPath, err := os.Executable() @@ -256,6 +261,7 @@ func CreateSSHCommand( args := buildSSHCommandArgs( client.Context(), client.Workspace(), + user, log.DebugEnabled(), extraArgs, ) @@ -265,11 +271,18 @@ func CreateSSHCommand( } // buildSSHCommandArgs constructs the argument list for `devsy ssh`. -func buildSSHCommandArgs(clientContext, workspace string, debug bool, extraArgs []string) []string { +func buildSSHCommandArgs( + clientContext, workspace, user string, + debug bool, + extraArgs []string, +) []string { + if user == "" { + user = "root" + } args := []string{ "workspace", "ssh", - names.FlagValue(names.User, "root"), + names.FlagValue(names.User, user), names.FlagFalse(names.AgentForwarding), names.FlagFalse(names.StartServices), names.Flag(names.Context), diff --git a/pkg/tunnel/browser_test.go b/pkg/tunnel/browser_test.go index abd8d2ab0..c2803a740 100644 --- a/pkg/tunnel/browser_test.go +++ b/pkg/tunnel/browser_test.go @@ -21,9 +21,9 @@ func exitError(t *testing.T, code int) error { return err } -func baseSSHArgs(ctx, ws string) []string { +func baseSSHArgs(ctx, user, ws string) []string { return []string{ - "workspace", "ssh", "--user=root", "--agent-forwarding=false", + "workspace", "ssh", "--user=" + user, "--agent-forwarding=false", "--start-services=false", "--context", ctx, ws, } } @@ -33,34 +33,47 @@ func TestBuildSSHCommandArgs(t *testing.T) { name string context string workspace string + user string debug bool extraArgs []string expected []string }{ { - name: "basic", context: "default", workspace: "my-workspace", - expected: baseSSHArgs("default", "my-workspace"), + name: "basic root user", context: "default", workspace: "my-workspace", + user: "root", + expected: baseSSHArgs("default", "root", "my-workspace"), + }, + { + name: "non-root workspace user", context: "default", workspace: "my-workspace", + user: "vscode", + expected: baseSSHArgs("default", "vscode", "my-workspace"), + }, + { + name: "empty user falls back to root", context: "default", workspace: "my-workspace", + user: "", + expected: baseSSHArgs("default", "root", "my-workspace"), }, { name: "with debug", context: "default", workspace: "my-workspace", - debug: true, - expected: append(baseSSHArgs("default", "my-workspace"), "--debug"), + user: "vscode", debug: true, + expected: append(baseSSHArgs("default", "vscode", "my-workspace"), "--debug"), }, { name: "with extra args", context: "prod", workspace: "ws", + user: "vscode", extraArgs: []string{"--stdio", "--log-output=raw"}, - expected: append(baseSSHArgs("prod", "ws"), "--stdio", "--log-output=raw"), + expected: append(baseSSHArgs("prod", "vscode", "ws"), "--stdio", "--log-output=raw"), }, { name: "with debug and extra args", context: "default", workspace: "my-workspace", - debug: true, extraArgs: []string{"--stdio"}, - expected: append(baseSSHArgs("default", "my-workspace"), "--debug", "--stdio"), + user: "vscode", debug: true, extraArgs: []string{"--stdio"}, + expected: append(baseSSHArgs("default", "vscode", "my-workspace"), "--debug", "--stdio"), }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := buildSSHCommandArgs(tt.context, tt.workspace, tt.debug, tt.extraArgs) + got := buildSSHCommandArgs(tt.context, tt.workspace, tt.user, tt.debug, tt.extraArgs) assert.Equal(t, tt.expected, got) }) } From a2d134c0ed1bc96475d314cd5c2b93f7590105af Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 13:46:00 +0000 Subject: [PATCH 02/28] fix(ide): pass resolved workspace user to fleet SSH command Required to keep the tree building after Task 1's CreateSSHCommand signature change; full Task 2 scope (tests, browser_tunnel.go audit) is out of scope for this task. --- pkg/ide/opener/opener.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/ide/opener/opener.go b/pkg/ide/opener/opener.go index ff3a26333..bd17384ce 100644 --- a/pkg/ide/opener/opener.go +++ b/pkg/ide/opener/opener.go @@ -470,6 +470,7 @@ func startFleet(ctx context.Context, params IDEParams) (string, error) { sshCmd, err := tunnel.CreateSSHCommand( ctx, params.Client, + params.User, []string{names.Flag(names.Command), "cat " + fleet.FleetURLFileName}, ) if err != nil { From 6fa365f36d1daee161460c68e56344f8e5c7a4c9 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 13:53:38 +0000 Subject: [PATCH 03/28] test(ide): add regression coverage for fleet SSH command user propagation --- pkg/ide/opener/browser_tunnel_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index 46c86c6b6..ce1b4b363 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -11,6 +11,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/tunnel" ) @@ -105,6 +107,21 @@ func TestBuildHelperArgs_OpenBrowser(t *testing.T) { } } +// TestBuildHelperArgs_IncludesResolvedUser guards against the fleet/openBrowserIDE +// user-propagation regression: the detached helper must run as the resolved +// workspace user, never empty/root. +func TestBuildHelperArgs_IncludesResolvedUser(t *testing.T) { + args := buildHelperArgs("default", "my-workspace", tunnel.BrowserTunnelParams{ + User: "vscode", + TargetURL: "http://localhost:1234", + }, false) + + joined := strings.Join(args, " ") + assert.Contains(t, joined, "--user vscode", + "the detached browser-tunnel helper must run its RunServices/backhaul "+ + "connections as the resolved workspace user, not be left empty/root") +} + // setupTempHome redirects the path manager to a temp HOME so the workspace // dir is writable and isolated from the real user's devsy data. func setupTempHome(t *testing.T) { From 4bb0dfc494c23f3d12c60b4bba1f5b35efa7c6f8 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 13:59:22 +0000 Subject: [PATCH 04/28] fix(gpg): make setup-gpg lock file world-lockable for multi-user containers --- cmd/internal/agentworkspace/setup_gpg.go | 15 +++++++++++++- cmd/internal/agentworkspace/setup_gpg_test.go | 20 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index c0c9d10ba..fbaa216fb 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "fmt" + "os" "time" "github.com/devsy-org/devsy/cmd/flags" @@ -103,7 +104,11 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { lockCtx, cancel := context.WithTimeout(ctx, gpgSetupLockTimeout) defer cancel() - lock := flock.New(gpgSetupLockPath) + // 0666: setup-gpg can run as root (SSH tunnel) or as the workspace's + // remoteUser (GPG-forwarding tunnel), sometimes concurrently. flock's + // default 0600 mode lets whichever process creates the file first lock + // every other user out with EACCES on the second process's TryLockContext. + lock := flock.New(gpgSetupLockPath, flock.SetPermissions(0o666)) locked, err := lock.TryLockContext(lockCtx, 200*time.Millisecond) if err != nil { if ctx.Err() != nil { @@ -121,6 +126,14 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { return nil, fmt.Errorf("timed out waiting for another gpg setup to finish") } + // os.Chmod bypasses the umask to ensure the file has world-readable/writable + // permissions, which is essential when setup-gpg runs under different users + // in the same container. + if err := os.Chmod(gpgSetupLockPath, 0o666); err != nil { + _ = lock.Unlock() + return nil, fmt.Errorf("set lock file permissions: %w", err) + } + return func() { _ = lock.Unlock() }, nil } diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index 93f3f30cd..65c09e011 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -3,6 +3,7 @@ package agentworkspace import ( "context" "errors" + "os" "path/filepath" "testing" "time" @@ -101,3 +102,22 @@ func TestAcquireGPGSetupLock_ReturnsCancellationErrorWhenCallerCancels(t *testin err, ) } + +func TestAcquireGPGSetupLock_FileIsWorldLockable(t *testing.T) { + origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout + gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") + gpgSetupLockTimeout = time.Second + defer func() { gpgSetupLockPath, gpgSetupLockTimeout = origPath, origTimeout }() + + unlock, err := acquireGPGSetupLock(context.Background()) + require.NoError(t, err) + unlock() + + info, err := os.Stat(gpgSetupLockPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm(), + "lock file must be 0666 so any container user (root or the workspace's "+ + "remoteUser) can create/open it; the default flock mode of 0600 lets "+ + "whichever user runs setup-gpg first lock out every other user "+ + "with EACCES") +} From 6c8a391ecfde5e977c9fefe87809eb8975b61e57 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 14:11:13 +0000 Subject: [PATCH 05/28] test(ssh-server): add coverage for ensureActivityFile permission/idempotency --- cmd/internal/ssh_server_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/cmd/internal/ssh_server_test.go b/cmd/internal/ssh_server_test.go index 27aaacc36..7cb63bf43 100644 --- a/cmd/internal/ssh_server_test.go +++ b/cmd/internal/ssh_server_test.go @@ -15,6 +15,8 @@ import ( "github.com/devsy-org/devsy/pkg/token" "github.com/devsy-org/ssh" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func encodeTestToken(t *testing.T, tok token.Token) string { @@ -184,3 +186,27 @@ func TestRunActivityHeartbeatExitsOnContextCancel(t *testing.T) { t.Fatal("heartbeat did not exit within 2s of context cancel") } } + +func TestEnsureActivityFile_CreatesWorldWritableFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "devsy.activity") + + require.NoError(t, ensureActivityFile(path)) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(activityFileMode), info.Mode().Perm(), + "the activity file must be 0666 so both the root browser-IDE tunnel and "+ + "a non-root GPG-forwarding tunnel can touch it") +} + +func TestEnsureActivityFile_NoOpsWhenFileAlreadyExists(t *testing.T) { + path := filepath.Join(t.TempDir(), "devsy.activity") + require.NoError(t, os.WriteFile(path, []byte("existing"), 0o600)) + + require.NoError(t, ensureActivityFile(path)) + + data, err := os.ReadFile(path) //nolint:gosec // test-owned temp path + require.NoError(t, err) + assert.Equal(t, "existing", string(data), + "ensureActivityFile must not truncate a file that already exists") +} From fe21e0be6e6698518516a34520c32c71fc631445 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 14:19:50 +0000 Subject: [PATCH 06/28] test(gpg,tunnel): lock in non-root user propagation for backhaul/forward paths --- pkg/gpg/forward_test.go | 17 ++++++++++++ pkg/tunnel/browser_test.go | 53 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/pkg/gpg/forward_test.go b/pkg/gpg/forward_test.go index 7786c9ba9..6e98fb3cb 100644 --- a/pkg/gpg/forward_test.go +++ b/pkg/gpg/forward_test.go @@ -29,6 +29,23 @@ func TestBuildForwardArgs(t *testing.T) { assert.Equal(t, expected, got) } +func TestBuildForwardArgs_NonRootUser(t *testing.T) { + got := buildForwardArgs("vscode", "test-context", "test-workspace") + expected := []string{ + "workspace", + "ssh", + "--ssh-gpg-forwarding=true", + "--agent-forwarding=true", + "--start-services=true", + "--user", "vscode", + "--context", "test-context", + "test-workspace", + "--log-output=raw", + "--command", "sleep infinity", + } + assert.Equal(t, expected, got) +} + func TestSuperviseForward_RestartsUntilCancelled(t *testing.T) { runs := filepath.Join(t.TempDir(), "runs") ctx, cancel := context.WithCancel(context.Background()) diff --git a/pkg/tunnel/browser_test.go b/pkg/tunnel/browser_test.go index c2803a740..50d9a34bf 100644 --- a/pkg/tunnel/browser_test.go +++ b/pkg/tunnel/browser_test.go @@ -1,16 +1,52 @@ package tunnel import ( + "bytes" + "context" "errors" "fmt" "os/exec" + "strings" "testing" + client2 "github.com/devsy-org/devsy/pkg/client" "github.com/devsy-org/devsy/pkg/exitcode" + "github.com/devsy-org/devsy/pkg/provider" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +type fakeWorkspaceClient struct{} + +func (fakeWorkspaceClient) Provider() string { return "" } +func (fakeWorkspaceClient) Context() string { return "" } + +func (fakeWorkspaceClient) RefreshOptions( + ctx context.Context, userOptions []string, reconfigure bool, +) error { + return nil +} + +func (fakeWorkspaceClient) Status( + ctx context.Context, options client2.StatusOptions, +) (client2.Status, error) { + return "", nil +} + +func (fakeWorkspaceClient) Stop(ctx context.Context, options client2.StopOptions) error { + return nil +} + +func (fakeWorkspaceClient) Delete(ctx context.Context, options client2.DeleteOptions) error { + return nil +} +func (fakeWorkspaceClient) Workspace() string { return "" } +func (fakeWorkspaceClient) WorkspaceConfig() *provider.Workspace { return nil } +func (fakeWorkspaceClient) Lock(ctx context.Context) error { return nil } +func (fakeWorkspaceClient) Unlock() {} + +var _ client2.BaseWorkspaceClient = fakeWorkspaceClient{} + // exitError runs a shell command that exits with the given code and returns // the resulting error (which wraps *exec.ExitError). func exitError(t *testing.T, code int) error { @@ -79,6 +115,23 @@ func TestBuildSSHCommandArgs(t *testing.T) { } } +func TestBuildBackhaulCmd_UsesResolvedRemoteUser(t *testing.T) { + writer := &bytes.Buffer{} + cmd := buildBackhaulCmd(context.Background(), backhaulCmdParams{ + execPath: "/usr/bin/true", + remoteUser: "vscode", + client: fakeWorkspaceClient{}, + authSockID: "sock123", + writer: writer, + }) + + joined := strings.Join(cmd.Args, " ") + assert.Contains(t, joined, "--user vscode", + "backhaul connection must use the resolved workspace user, not root, "+ + "so it doesn't fight the primary tunnel's ssh-server/setup-gpg sessions "+ + "over the shared /tmp coordination files") +} + func TestIsTransientBackhaulErr(t *testing.T) { transient := exitError(t, exitcode.Retryable) otherExit := exitError(t, 1) From 30e1cae9a7401cc0cd4cce826a042faf3e8fa618 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 14:56:44 +0000 Subject: [PATCH 07/28] test(e2e): add regression coverage for GPG forwarding + browser IDE with non-root remoteUser --- e2e/tests/ide/browser_returns.go | 58 +++++++++++++++++++ .../testdata-gpg-nonroot/.devcontainer.json | 5 ++ 2 files changed, 63 insertions(+) create mode 100644 e2e/tests/ide/testdata-gpg-nonroot/.devcontainer.json diff --git a/e2e/tests/ide/browser_returns.go b/e2e/tests/ide/browser_returns.go index c704c5afa..93bb2b5a5 100644 --- a/e2e/tests/ide/browser_returns.go +++ b/e2e/tests/ide/browser_returns.go @@ -9,6 +9,7 @@ import ( "net" "net/url" "os" + "path/filepath" "runtime" "strings" "syscall" @@ -21,6 +22,8 @@ import ( "github.com/onsi/gomega" ) +const gpgTestKeyFingerprint = "07F681B9FD6C3411F679BFD1F51769DB572DDD3F" + // setupBrowserIDE prepares a docker provider + workspace tempdir and registers // the standard cleanup deferred to DeferCleanup. It returns the framework and // the workspace tempDir path. @@ -309,6 +312,61 @@ var _ = ginkgo.Describe( }, ) + ginkgo.It( + "does not collide on shared /tmp coordination files when GPG forwarding "+ + "races the browser-IDE tunnel under a non-root remoteUser", + ginkgo.Label("gpg"), + ginkgo.SpecTimeout(framework.TimeoutLong()), + func(ctx context.Context) { + if runtime.GOOS == "windows" { + ginkgo.Skip("skipping on windows") + } + + f := framework.NewDefaultFramework(initialDir + "/bin") + tempDir, err := framework.CopyToTempDir("tests/ide/testdata-gpg-nonroot") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(framework.CleanupTempDir, initialDir, tempDir) + + err = f.DevsyProviderAdd(ctx, "docker") + framework.ExpectNoError(err) + err = f.DevsyProviderUse(ctx, "docker") + framework.ExpectNoError(err) + ginkgo.DeferCleanup(func(cleanupCtx context.Context) { + _ = f.DevsyWorkspaceDelete(cleanupCtx, tempDir) + }) + + ginkgo.GinkgoT().Setenv("GNUPGHOME", ginkgo.GinkgoT().TempDir()) + framework.ExpectNoError( + framework.ImportGpgKey( + filepath.Join(initialDir, "tests/ssh/testdata/gpg-forwarding/gpg-private.key"), + ), + ) + + stdout, stderr, err := f.DevsyUpStreamsRaw(ctx, tempDir, + "--ide=openvscode", "--ide-launch=headless", + names.Flag(names.SSHGPGForwarding), "--debug") + framework.ExpectNoError(err) + combined := stdout + stderr + + gomega.Expect(combined).NotTo(gomega.ContainSubstring("permission denied"), + "root and vscode sessions must not collide on /tmp/devsy-gpg-setup.lock; "+ + "got:\n%s", combined) + gomega.Expect(combined).NotTo(gomega.ContainSubstring("operation not permitted"), + "root and vscode sessions must not collide on /tmp/devsy.activity; "+ + "got:\n%s", combined) + gomega.Expect(combined).NotTo(gomega.ContainSubstring("continuing without it"), + "GPG agent forwarding must succeed end-to-end for a non-root remoteUser "+ + "browser IDE; got:\n%s", combined) + + sshCtx, cancelSSH := context.WithDeadline(ctx, time.Now().Add(30*time.Second)) + defer cancelSSH() + err = f.DevsySSHGpgSecretKeyForwarded(sshCtx, tempDir, gpgTestKeyFingerprint) + framework.ExpectNoError(err) + + framework.ExpectNoError(f.DevsyStop(ctx, tempDir)) + }, + ) + ginkgo.It( "does not log 'setup KubeConfig' on a workspace without kubeconfig forwarding", ginkgo.SpecTimeout(framework.TimeoutLong()), diff --git a/e2e/tests/ide/testdata-gpg-nonroot/.devcontainer.json b/e2e/tests/ide/testdata-gpg-nonroot/.devcontainer.json new file mode 100644 index 000000000..b639e5156 --- /dev/null +++ b/e2e/tests/ide/testdata-gpg-nonroot/.devcontainer.json @@ -0,0 +1,5 @@ +{ + "name": "GPG nonroot browser IDE", + "image": "ghcr.io/devsy-org/test-images/go:1", + "remoteUser": "vscode" +} From e2a45e0342591851b4bdcbc98902f4423547360c Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 15:18:05 +0000 Subject: [PATCH 08/28] fix(e2e): remove dead windows check and fix line-length lint findings --- e2e/tests/ide/browser_returns.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/e2e/tests/ide/browser_returns.go b/e2e/tests/ide/browser_returns.go index 93bb2b5a5..a860b3059 100644 --- a/e2e/tests/ide/browser_returns.go +++ b/e2e/tests/ide/browser_returns.go @@ -318,10 +318,6 @@ var _ = ginkgo.Describe( ginkgo.Label("gpg"), ginkgo.SpecTimeout(framework.TimeoutLong()), func(ctx context.Context) { - if runtime.GOOS == "windows" { - ginkgo.Skip("skipping on windows") - } - f := framework.NewDefaultFramework(initialDir + "/bin") tempDir, err := framework.CopyToTempDir("tests/ide/testdata-gpg-nonroot") framework.ExpectNoError(err) @@ -338,7 +334,10 @@ var _ = ginkgo.Describe( ginkgo.GinkgoT().Setenv("GNUPGHOME", ginkgo.GinkgoT().TempDir()) framework.ExpectNoError( framework.ImportGpgKey( - filepath.Join(initialDir, "tests/ssh/testdata/gpg-forwarding/gpg-private.key"), + filepath.Join( + initialDir, + "tests/ssh/testdata/gpg-forwarding/gpg-private.key", + ), ), ) From e444db92fe28c903e2bdc406961dd24c9312b45b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 16:53:58 +0000 Subject: [PATCH 09/28] fix(gpg): avoid EPERM on redundant lock-file chmod, add #nosec G302 justification acquireGPGSetupLock's os.Chmod(0o666) ran unconditionally, so a non-owning second acquirer (e.g. a different container user than the one that created the lock file) would get EPERM even when the mode was already correct. Skip the chmod when the file's mode already matches, and only attempt (and propagate errors from) it when a real change is needed. --- cmd/internal/agentworkspace/setup_gpg.go | 31 ++++++- cmd/internal/agentworkspace/setup_gpg_test.go | 92 +++++++++++++++++++ pkg/ide/opener/browser_tunnel_test.go | 3 +- pkg/tunnel/browser.go | 7 +- pkg/tunnel/browser_test.go | 53 +++++++---- 5 files changed, 162 insertions(+), 24 deletions(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index fbaa216fb..8cdd80a4c 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -128,15 +128,40 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { // os.Chmod bypasses the umask to ensure the file has world-readable/writable // permissions, which is essential when setup-gpg runs under different users - // in the same container. - if err := os.Chmod(gpgSetupLockPath, 0o666); err != nil { + // in the same container. Only attempted when the mode isn't already 0666: + // chmod requires ownership (or root) regardless of whether it would + // actually change anything, so a non-owning second acquirer (e.g. a + // non-root user acquiring a lock file root already fixed to 0666) would + // get EPERM on a redundant chmod call. + if needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666); err != nil { _ = lock.Unlock() - return nil, fmt.Errorf("set lock file permissions: %w", err) + return nil, fmt.Errorf("stat lock file: %w", err) + } else if needsChmod { + // #nosec G302 -- 0666 is intentional: setup-gpg can run as root (SSH + // tunnel) and as the workspace's remoteUser (GPG-forwarding tunnel), + // sometimes concurrently, and both need to acquire this lock. + if err := os.Chmod(gpgSetupLockPath, 0o666); err != nil { + _ = lock.Unlock() + return nil, fmt.Errorf("set lock file permissions: %w", err) + } } return func() { _ = lock.Unlock() }, nil } +// lockFileNeedsChmod reports whether path's current permission bits differ +// from want. Callers use this to skip a redundant os.Chmod: chmod requires +// file ownership (or root) even when the requested mode already matches, +// so skipping it when unnecessary avoids EPERM for a non-owning acquirer of +// an already-correctly-mode'd lock file. +func lockFileNeedsChmod(path string, want os.FileMode) (bool, error) { + info, err := os.Stat(path) + if err != nil { + return false, err + } + return info.Mode().Perm() != want.Perm(), nil +} + func fetchAndDecodeKeys(ownerTrustB64 string) ([]byte, []byte, error) { log.Debugf("Fetching public key") rawPublicKeys, err := getPublicKeys() diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index 65c09e011..a30505850 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -121,3 +121,95 @@ func TestAcquireGPGSetupLock_FileIsWorldLockable(t *testing.T) { "whichever user runs setup-gpg first lock out every other user "+ "with EACCES") } + +// TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod +// reproduces the multi-user scenario the lock file's world-writable mode +// exists for: one session creates the lock (mode becomes 0666), a later +// session on a different, non-owning user re-acquires it. That second +// acquire must not attempt a redundant chmod, since a non-owning chmod +// would fail with EPERM even though the mode is already correct. +func TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod(t *testing.T) { + origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout + gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") + gpgSetupLockTimeout = time.Second + defer func() { gpgSetupLockPath, gpgSetupLockTimeout = origPath, origTimeout }() + + unlock, err := acquireGPGSetupLock(context.Background()) + require.NoError(t, err) + unlock() + + info, err := os.Stat(gpgSetupLockPath) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o666), info.Mode().Perm()) + + needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) + require.NoError(t, err) + assert.False(t, needsChmod, + "a lock file already at 0666 must not need a chmod, since a "+ + "non-owning second acquirer's chmod would fail with EPERM") + + reacquire, err := acquireGPGSetupLock(context.Background()) + require.NoError(t, err, "second acquisition of an already-0666 lock file must succeed") + reacquire() +} + +// TestAcquireGPGSetupLock_FixesWrongMode ensures the "skip chmod when +// already 0666" optimization doesn't accidentally skip fixing a genuinely +// wrong mode: if the lock file was somehow created with a different mode, +// acquireGPGSetupLock must still attempt (and here, succeed at, since the +// test process owns the file) the chmod back to 0666. +func TestAcquireGPGSetupLock_FixesWrongMode(t *testing.T) { + origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout + gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") + gpgSetupLockTimeout = time.Second + defer func() { gpgSetupLockPath, gpgSetupLockTimeout = origPath, origTimeout }() + + unlock, err := acquireGPGSetupLock(context.Background()) + require.NoError(t, err) + unlock() + + // #nosec G302 -- intentional: simulating a wrong pre-existing mode + require.NoError(t, os.Chmod(gpgSetupLockPath, 0o644)) + + needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) + require.NoError(t, err) + assert.True(t, needsChmod, "a lock file at 0644 must be reported as needing a chmod to 0666") + + reacquire, err := acquireGPGSetupLock(context.Background()) + require.NoError(t, err) + defer reacquire() + + info, err := os.Stat(gpgSetupLockPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm(), + "acquireGPGSetupLock must fix a wrong mode back to 0666") +} + +func TestLockFileNeedsChmod(t *testing.T) { + tests := []struct { + name string + mode os.FileMode + want os.FileMode + expect bool + }{ + {name: "already matches", mode: 0o666, want: 0o666, expect: false}, + {name: "narrower mode needs widening", mode: 0o644, want: 0o666, expect: true}, + {name: "wider mode needs narrowing", mode: 0o777, want: 0o666, expect: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "lock") + require.NoError(t, os.WriteFile(path, nil, tc.mode)) + require.NoError(t, os.Chmod(path, tc.mode)) + + got, err := lockFileNeedsChmod(path, tc.want) + require.NoError(t, err) + assert.Equal(t, tc.expect, got) + }) + } +} + +func TestLockFileNeedsChmod_StatErrorPropagates(t *testing.T) { + _, err := lockFileNeedsChmod(filepath.Join(t.TempDir(), "does-not-exist"), 0o666) + require.Error(t, err) +} diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index ce1b4b363..ab7779160 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -11,10 +11,9 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" - "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/tunnel" + "github.com/stretchr/testify/assert" ) // containsAdjacent returns true if args contains needle followed immediately diff --git a/pkg/tunnel/browser.go b/pkg/tunnel/browser.go index 35d774d26..e3f1a935e 100644 --- a/pkg/tunnel/browser.go +++ b/pkg/tunnel/browser.go @@ -270,7 +270,12 @@ func CreateSSHCommand( return exec.CommandContext(ctx, execPath, args...), nil } -// buildSSHCommandArgs constructs the argument list for `devsy ssh`. +// buildSSHCommandArgs constructs the argument list for `devsy ssh`. Kept as +// plain args rather than a params struct: it's an internal, unexported +// helper with a small stable signature, and a struct would only add +// indirection for CreateSSHCommand's one caller and its existing tests. +// +//nolint:revive // argument-limit func buildSSHCommandArgs( clientContext, workspace, user string, debug bool, diff --git a/pkg/tunnel/browser_test.go b/pkg/tunnel/browser_test.go index 50d9a34bf..0a27e1769 100644 --- a/pkg/tunnel/browser_test.go +++ b/pkg/tunnel/browser_test.go @@ -16,6 +16,12 @@ import ( "github.com/stretchr/testify/require" ) +const ( + testUserRoot = "root" + testUserVSCode = "vscode" + testWorkspaceName = "my-workspace" +) + type fakeWorkspaceClient struct{} func (fakeWorkspaceClient) Provider() string { return "" } @@ -75,35 +81,46 @@ func TestBuildSSHCommandArgs(t *testing.T) { expected []string }{ { - name: "basic root user", context: "default", workspace: "my-workspace", - user: "root", - expected: baseSSHArgs("default", "root", "my-workspace"), + name: "basic root user", context: testCtxName, workspace: testWorkspaceName, + user: testUserRoot, + expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), }, { - name: "non-root workspace user", context: "default", workspace: "my-workspace", - user: "vscode", - expected: baseSSHArgs("default", "vscode", "my-workspace"), + name: "non-root workspace user", context: testCtxName, workspace: testWorkspaceName, + user: testUserVSCode, + expected: baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), }, { - name: "empty user falls back to root", context: "default", workspace: "my-workspace", - user: "", - expected: baseSSHArgs("default", "root", "my-workspace"), + name: "empty user falls back to root", + context: testCtxName, + workspace: testWorkspaceName, + user: "", + expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), }, { - name: "with debug", context: "default", workspace: "my-workspace", - user: "vscode", debug: true, - expected: append(baseSSHArgs("default", "vscode", "my-workspace"), "--debug"), + name: "with debug", + context: testCtxName, + workspace: testWorkspaceName, + user: testUserVSCode, + debug: true, + expected: append( + baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), "--debug", + ), }, { name: "with extra args", context: "prod", workspace: "ws", - user: "vscode", + user: testUserVSCode, extraArgs: []string{"--stdio", "--log-output=raw"}, - expected: append(baseSSHArgs("prod", "vscode", "ws"), "--stdio", "--log-output=raw"), + expected: append( + baseSSHArgs("prod", testUserVSCode, "ws"), "--stdio", "--log-output=raw", + ), }, { - name: "with debug and extra args", context: "default", workspace: "my-workspace", - user: "vscode", debug: true, extraArgs: []string{"--stdio"}, - expected: append(baseSSHArgs("default", "vscode", "my-workspace"), "--debug", "--stdio"), + name: "with debug and extra args", context: testCtxName, workspace: testWorkspaceName, + user: testUserVSCode, debug: true, extraArgs: []string{"--stdio"}, + expected: append( + baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), "--debug", "--stdio", + ), }, } @@ -119,7 +136,7 @@ func TestBuildBackhaulCmd_UsesResolvedRemoteUser(t *testing.T) { writer := &bytes.Buffer{} cmd := buildBackhaulCmd(context.Background(), backhaulCmdParams{ execPath: "/usr/bin/true", - remoteUser: "vscode", + remoteUser: testUserVSCode, client: fakeWorkspaceClient{}, authSockID: "sock123", writer: writer, From 935252d025c31518d2bde937d70372a6683c34e1 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 17:36:22 +0000 Subject: [PATCH 10/28] refactor(tunnel): convert buildSSHCommandArgs to a params struct Resolves revive's argument-limit without a nolint suppression: the function's 5 positional args (3 of them same-typed strings) become one sshCommandArgsParams struct, removing the transposition hazard along with the lint finding. --- pkg/tunnel/browser.go | 46 +++++++++++++++-------------- pkg/tunnel/browser_test.go | 59 +++++++++++++++++++++----------------- 2 files changed, 56 insertions(+), 49 deletions(-) diff --git a/pkg/tunnel/browser.go b/pkg/tunnel/browser.go index e3f1a935e..704791076 100644 --- a/pkg/tunnel/browser.go +++ b/pkg/tunnel/browser.go @@ -258,29 +258,31 @@ func CreateSSHCommand( return nil, err } - args := buildSSHCommandArgs( - client.Context(), - client.Workspace(), - user, - log.DebugEnabled(), - extraArgs, - ) + args := buildSSHCommandArgs(sshCommandArgsParams{ + clientContext: client.Context(), + workspace: client.Workspace(), + user: user, + debug: log.DebugEnabled(), + extraArgs: extraArgs, + }) //nolint:gosec // execPath is the current binary, arguments are controlled return exec.CommandContext(ctx, execPath, args...), nil } -// buildSSHCommandArgs constructs the argument list for `devsy ssh`. Kept as -// plain args rather than a params struct: it's an internal, unexported -// helper with a small stable signature, and a struct would only add -// indirection for CreateSSHCommand's one caller and its existing tests. -// -//nolint:revive // argument-limit -func buildSSHCommandArgs( - clientContext, workspace, user string, - debug bool, - extraArgs []string, -) []string { +// sshCommandArgsParams bundles buildSSHCommandArgs' inputs so the function +// takes one argument instead of five. +type sshCommandArgsParams struct { + clientContext string + workspace string + user string + debug bool + extraArgs []string +} + +// buildSSHCommandArgs constructs the argument list for `devsy ssh`. +func buildSSHCommandArgs(p sshCommandArgsParams) []string { + user := p.user if user == "" { user = "root" } @@ -291,12 +293,12 @@ func buildSSHCommandArgs( names.FlagFalse(names.AgentForwarding), names.FlagFalse(names.StartServices), names.Flag(names.Context), - clientContext, - workspace, + p.clientContext, + p.workspace, } - if debug { + if p.debug { args = append(args, names.Flag(names.Debug)) } - args = append(args, extraArgs...) + args = append(args, p.extraArgs...) return args } diff --git a/pkg/tunnel/browser_test.go b/pkg/tunnel/browser_test.go index 0a27e1769..f49e061f2 100644 --- a/pkg/tunnel/browser_test.go +++ b/pkg/tunnel/browser_test.go @@ -70,54 +70,59 @@ func baseSSHArgs(ctx, user, ws string) []string { } } +func baseParams(user string) sshCommandArgsParams { + return sshCommandArgsParams{ + clientContext: testCtxName, workspace: testWorkspaceName, user: user, + } +} + func TestBuildSSHCommandArgs(t *testing.T) { tests := []struct { - name string - context string - workspace string - user string - debug bool - extraArgs []string - expected []string + name string + params sshCommandArgsParams + expected []string }{ { - name: "basic root user", context: testCtxName, workspace: testWorkspaceName, - user: testUserRoot, + name: "basic root user", + params: baseParams(testUserRoot), expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), }, { - name: "non-root workspace user", context: testCtxName, workspace: testWorkspaceName, - user: testUserVSCode, + name: "non-root workspace user", + params: baseParams(testUserVSCode), expected: baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), }, { - name: "empty user falls back to root", - context: testCtxName, - workspace: testWorkspaceName, - user: "", - expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), + name: "empty user falls back to root", + params: baseParams(""), + expected: baseSSHArgs(testCtxName, testUserRoot, testWorkspaceName), }, { - name: "with debug", - context: testCtxName, - workspace: testWorkspaceName, - user: testUserVSCode, - debug: true, + name: "with debug", + params: sshCommandArgsParams{ + clientContext: testCtxName, workspace: testWorkspaceName, + user: testUserVSCode, debug: true, + }, expected: append( baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), "--debug", ), }, { - name: "with extra args", context: "prod", workspace: "ws", - user: testUserVSCode, - extraArgs: []string{"--stdio", "--log-output=raw"}, + name: "with extra args", + params: sshCommandArgsParams{ + clientContext: "prod", workspace: "ws", user: testUserVSCode, + extraArgs: []string{"--stdio", "--log-output=raw"}, + }, expected: append( baseSSHArgs("prod", testUserVSCode, "ws"), "--stdio", "--log-output=raw", ), }, { - name: "with debug and extra args", context: testCtxName, workspace: testWorkspaceName, - user: testUserVSCode, debug: true, extraArgs: []string{"--stdio"}, + name: "with debug and extra args", + params: sshCommandArgsParams{ + clientContext: testCtxName, workspace: testWorkspaceName, user: testUserVSCode, + debug: true, extraArgs: []string{"--stdio"}, + }, expected: append( baseSSHArgs(testCtxName, testUserVSCode, testWorkspaceName), "--debug", "--stdio", ), @@ -126,7 +131,7 @@ func TestBuildSSHCommandArgs(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := buildSSHCommandArgs(tt.context, tt.workspace, tt.user, tt.debug, tt.extraArgs) + got := buildSSHCommandArgs(tt.params) assert.Equal(t, tt.expected, got) }) } From b209637dfcf0d407d9ea4f486c16f1492c04396f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 17:39:17 +0000 Subject: [PATCH 11/28] docs: trim WHY comments to 2 lines or fewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reasoning is preserved (multi-user /tmp collision, umask vs chmod ownership semantics, EPERM-on-redundant-chmod) — condensed to the essential point rather than removed. --- cmd/internal/agentworkspace/setup_gpg.go | 27 ++++++------------- cmd/internal/agentworkspace/setup_gpg_test.go | 15 +++-------- pkg/tunnel/browser.go | 6 ++--- 3 files changed, 14 insertions(+), 34 deletions(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index 8cdd80a4c..c5ec34e47 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -104,10 +104,8 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { lockCtx, cancel := context.WithTimeout(ctx, gpgSetupLockTimeout) defer cancel() - // 0666: setup-gpg can run as root (SSH tunnel) or as the workspace's - // remoteUser (GPG-forwarding tunnel), sometimes concurrently. flock's - // default 0600 mode lets whichever process creates the file first lock - // every other user out with EACCES on the second process's TryLockContext. + // 0666: setup-gpg can run as root or the workspace's remoteUser, and + // flock's default 0600 mode would lock the second one out with EACCES. lock := flock.New(gpgSetupLockPath, flock.SetPermissions(0o666)) locked, err := lock.TryLockContext(lockCtx, 200*time.Millisecond) if err != nil { @@ -126,20 +124,14 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { return nil, fmt.Errorf("timed out waiting for another gpg setup to finish") } - // os.Chmod bypasses the umask to ensure the file has world-readable/writable - // permissions, which is essential when setup-gpg runs under different users - // in the same container. Only attempted when the mode isn't already 0666: - // chmod requires ownership (or root) regardless of whether it would - // actually change anything, so a non-owning second acquirer (e.g. a - // non-root user acquiring a lock file root already fixed to 0666) would - // get EPERM on a redundant chmod call. + // os.Chmod bypasses the umask flock.SetPermissions is subject to. Skipped + // when already 0666, since chmod needs ownership even when mode wouldn't change. if needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666); err != nil { _ = lock.Unlock() return nil, fmt.Errorf("stat lock file: %w", err) } else if needsChmod { - // #nosec G302 -- 0666 is intentional: setup-gpg can run as root (SSH - // tunnel) and as the workspace's remoteUser (GPG-forwarding tunnel), - // sometimes concurrently, and both need to acquire this lock. + // #nosec G302 -- 0666 is intentional: both root and the workspace's + // remoteUser must be able to acquire this lock. if err := os.Chmod(gpgSetupLockPath, 0o666); err != nil { _ = lock.Unlock() return nil, fmt.Errorf("set lock file permissions: %w", err) @@ -149,11 +141,8 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { return func() { _ = lock.Unlock() }, nil } -// lockFileNeedsChmod reports whether path's current permission bits differ -// from want. Callers use this to skip a redundant os.Chmod: chmod requires -// file ownership (or root) even when the requested mode already matches, -// so skipping it when unnecessary avoids EPERM for a non-owning acquirer of -// an already-correctly-mode'd lock file. +// lockFileNeedsChmod reports whether path's mode differs from want, so +// callers can skip a chmod that would EPERM a non-owning acquirer. func lockFileNeedsChmod(path string, want os.FileMode) (bool, error) { info, err := os.Stat(path) if err != nil { diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index a30505850..33466b61e 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -122,12 +122,8 @@ func TestAcquireGPGSetupLock_FileIsWorldLockable(t *testing.T) { "with EACCES") } -// TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod -// reproduces the multi-user scenario the lock file's world-writable mode -// exists for: one session creates the lock (mode becomes 0666), a later -// session on a different, non-owning user re-acquires it. That second -// acquire must not attempt a redundant chmod, since a non-owning chmod -// would fail with EPERM even though the mode is already correct. +// TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod covers +// a non-owning second acquirer of an already-0666 lock, whose chmod would EPERM. func TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod(t *testing.T) { origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") @@ -153,11 +149,8 @@ func TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod(t *testi reacquire() } -// TestAcquireGPGSetupLock_FixesWrongMode ensures the "skip chmod when -// already 0666" optimization doesn't accidentally skip fixing a genuinely -// wrong mode: if the lock file was somehow created with a different mode, -// acquireGPGSetupLock must still attempt (and here, succeed at, since the -// test process owns the file) the chmod back to 0666. +// TestAcquireGPGSetupLock_FixesWrongMode ensures the skip-chmod-if-0666 +// optimization doesn't skip fixing a genuinely wrong mode. func TestAcquireGPGSetupLock_FixesWrongMode(t *testing.T) { origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") diff --git a/pkg/tunnel/browser.go b/pkg/tunnel/browser.go index 704791076..c45844999 100644 --- a/pkg/tunnel/browser.go +++ b/pkg/tunnel/browser.go @@ -243,10 +243,8 @@ func isTransientBackhaulErr(err error) bool { } // CreateSSHCommand builds an exec.Cmd that runs `devsy ssh` with the given arguments. -// user is the workspace's remote user (e.g. from GetRemoteUser); it must match -// the user the container's ssh-server/gpg-setup sessions run as, or the two -// sessions collide on the shared /tmp coordination files (devsy-gpg-setup.lock, -// devsy.activity), which are owned by whichever session created them first. +// user must match the ssh-server/gpg-setup sessions' user, or they collide on +// shared /tmp coordination files (devsy-gpg-setup.lock, devsy.activity). func CreateSSHCommand( ctx context.Context, client client2.BaseWorkspaceClient, From 80fc319e0fa4c8d90c0f4b28aec3d2c7e454326a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 18:09:19 +0000 Subject: [PATCH 12/28] fix(gpg): reject symlinked lock path before chmod, tighten activity-mode assertion os.Chmod follows symlinks; the GPG setup lock path is a fixed, predictable, world-writable /tmp path, so a symlink planted there could redirect the chmod onto an arbitrary root-owned file. lockFileNeedsChmod now Lstats and refuses to proceed if the path is a symlink. Also compares the activity-file test's mode assertion against the literal 0o666 instead of the production constant it's meant to be checking. Addresses CodeRabbit findings on PR #869. --- cmd/internal/agentworkspace/setup_gpg.go | 9 +++++-- cmd/internal/agentworkspace/setup_gpg_test.go | 25 +++++++++++++++++++ cmd/internal/ssh_server_test.go | 2 +- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index c5ec34e47..39497aab3 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -142,12 +142,17 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { } // lockFileNeedsChmod reports whether path's mode differs from want, so -// callers can skip a chmod that would EPERM a non-owning acquirer. +// callers can skip a chmod that would EPERM a non-owning acquirer. Uses +// Lstat and rejects symlinks: os.Chmod follows them, so a lock path +// replaced with a symlink could redirect the chmod onto an arbitrary file. func lockFileNeedsChmod(path string, want os.FileMode) (bool, error) { - info, err := os.Stat(path) + info, err := os.Lstat(path) if err != nil { return false, err } + if info.Mode()&os.ModeSymlink != 0 { + return false, fmt.Errorf("refusing to chmod %s: path is a symlink", path) + } return info.Mode().Perm() != want.Perm(), nil } diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index 33466b61e..20d594f64 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -206,3 +206,28 @@ func TestLockFileNeedsChmod_StatErrorPropagates(t *testing.T) { _, err := lockFileNeedsChmod(filepath.Join(t.TempDir(), "does-not-exist"), 0o666) require.Error(t, err) } + +// TestAcquireGPGSetupLock_RejectsSymlink guards against a symlink planted at +// the lock path redirecting os.Chmod onto an arbitrary target file, since +// os.Chmod follows symlinks and this lock path is a fixed, predictable +// world-writable /tmp path any container user can pre-create. +func TestAcquireGPGSetupLock_RejectsSymlink(t *testing.T) { + origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout + dir := t.TempDir() + gpgSetupLockPath = filepath.Join(dir, "setup-gpg.lock") + gpgSetupLockTimeout = time.Second + defer func() { gpgSetupLockPath, gpgSetupLockTimeout = origPath, origTimeout }() + + target := filepath.Join(dir, "target") + require.NoError(t, os.WriteFile(target, nil, 0o600)) + require.NoError(t, os.Symlink(target, gpgSetupLockPath)) + + _, err := acquireGPGSetupLock(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") + + info, err := os.Stat(target) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "the symlink target's mode must be untouched, not widened to 0666") +} diff --git a/cmd/internal/ssh_server_test.go b/cmd/internal/ssh_server_test.go index 7cb63bf43..3b11eb66e 100644 --- a/cmd/internal/ssh_server_test.go +++ b/cmd/internal/ssh_server_test.go @@ -194,7 +194,7 @@ func TestEnsureActivityFile_CreatesWorldWritableFile(t *testing.T) { info, err := os.Stat(path) require.NoError(t, err) - assert.Equal(t, os.FileMode(activityFileMode), info.Mode().Perm(), + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm(), "the activity file must be 0666 so both the root browser-IDE tunnel and "+ "a non-root GPG-forwarding tunnel can touch it") } From 6e679ac3c95f184312c018b6e5dfd9cecd0f2134 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 18:39:43 +0000 Subject: [PATCH 13/28] fix(gpg): widen a stale, restrictively-moded lock file before flock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A lock file left behind by a pre-fix binary (or created by a different user) could sit at 0600. flock's own open() would EACCES on it before acquireGPGSetupLock's existing post-lock chmod logic ever ran, wedging a non-owning acquirer exactly like the original bug this branch fixes. widenStaleLockFile runs before the flock attempt: it chmods a restrictive existing lock file to 0666, escalating to sudo when the current process doesn't own it. A sudo failure is non-fatal — the subsequent flock attempt surfaces a clearer error if widening didn't help. Addresses the Major follow-up noted on PR #869's CodeRabbit review. --- cmd/internal/agentworkspace/setup_gpg.go | 40 ++++++++++++++ cmd/internal/agentworkspace/setup_gpg_test.go | 53 +++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index 39497aab3..a98c3ba52 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -3,8 +3,11 @@ package agentworkspace import ( "context" "encoding/base64" + "errors" "fmt" + "io/fs" "os" + "os/exec" "time" "github.com/devsy-org/devsy/cmd/flags" @@ -104,6 +107,12 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { lockCtx, cancel := context.WithTimeout(ctx, gpgSetupLockTimeout) defer cancel() + // A stale lock file (pre-fix binary, or created by another user) may + // still be restrictively-moded; flock's own open() would EACCES on it. + if err := widenStaleLockFile(); err != nil { + return nil, err + } + // 0666: setup-gpg can run as root or the workspace's remoteUser, and // flock's default 0600 mode would lock the second one out with EACCES. lock := flock.New(gpgSetupLockPath, flock.SetPermissions(0o666)) @@ -156,6 +165,37 @@ func lockFileNeedsChmod(path string, want os.FileMode) (bool, error) { return info.Mode().Perm() != want.Perm(), nil } +// widenStaleLockFile chmods a stale, restrictively-moded lock file to 0666 +// (escalating to sudo if we don't own it) before flock's open() can EACCES on it. +func widenStaleLockFile() error { + needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("stat lock file: %w", err) + } + if !needsChmod { + return nil + } + + // #nosec G302 -- 0666 is intentional; see acquireGPGSetupLock. + if err := os.Chmod(gpgSetupLockPath, 0o666); err == nil { + return nil + } else if !errors.Is(err, fs.ErrPermission) { + return fmt.Errorf("widen stale lock file: %w", err) + } + + //nolint:gosec // gpgSetupLockPath is a fixed path, not user input + if err := exec.Command("sudo", "chmod", "0666", gpgSetupLockPath).Run(); err != nil { + log.Debugf( + "sudo chmod stale gpg setup lock (non-fatal, flock will surface the real error): %v", + err, + ) + } + return nil +} + func fetchAndDecodeKeys(ownerTrustB64 string) ([]byte, []byte, error) { log.Debugf("Fetching public key") rawPublicKeys, err := getPublicKeys() diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index 20d594f64..c2f9bcbcd 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -207,6 +207,30 @@ func TestLockFileNeedsChmod_StatErrorPropagates(t *testing.T) { require.Error(t, err) } +// TestAcquireGPGSetupLock_WidensStaleRestrictiveLockFile covers a lock file +// left behind at a restrictive mode (e.g. by a pre-fix binary): flock's own +// open() would EACCES on a 0600 file owned by someone else before ever +// reaching acquireGPGSetupLock's post-lock chmod, so the fix must widen it +// beforehand. This test only exercises the plain-chmod branch (the file is +// owned by the test process); the sudo-escalation branch requires a real +// cross-user setup and isn't reproducible in a unit test. +func TestAcquireGPGSetupLock_WidensStaleRestrictiveLockFile(t *testing.T) { + origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout + gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") + gpgSetupLockTimeout = time.Second + defer func() { gpgSetupLockPath, gpgSetupLockTimeout = origPath, origTimeout }() + + require.NoError(t, os.WriteFile(gpgSetupLockPath, nil, 0o600)) + + unlock, err := acquireGPGSetupLock(context.Background()) + require.NoError(t, err, "a stale restrictively-moded lock file must not block acquisition") + defer unlock() + + info, err := os.Stat(gpgSetupLockPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) +} + // TestAcquireGPGSetupLock_RejectsSymlink guards against a symlink planted at // the lock path redirecting os.Chmod onto an arbitrary target file, since // os.Chmod follows symlinks and this lock path is a fixed, predictable @@ -231,3 +255,32 @@ func TestAcquireGPGSetupLock_RejectsSymlink(t *testing.T) { assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "the symlink target's mode must be untouched, not widened to 0666") } + +func TestWidenStaleLockFile_NoOpsWhenFileMissingOrAlreadyCorrect(t *testing.T) { + origPath := gpgSetupLockPath + defer func() { gpgSetupLockPath = origPath }() + + gpgSetupLockPath = filepath.Join(t.TempDir(), "does-not-exist") + err := widenStaleLockFile() + require.NoError(t, err, "a missing lock file is flock's job to create, not this") + + gpgSetupLockPath = filepath.Join(t.TempDir(), "already-0666") + require.NoError(t, os.WriteFile(gpgSetupLockPath, nil, 0o600)) + //nolint:gosec // test fixture, intentional + require.NoError(t, os.Chmod(gpgSetupLockPath, 0o666)) + require.NoError(t, widenStaleLockFile()) +} + +func TestWidenStaleLockFile_WidensRestrictiveMode(t *testing.T) { + origPath := gpgSetupLockPath + defer func() { gpgSetupLockPath = origPath }() + + gpgSetupLockPath = filepath.Join(t.TempDir(), "restrictive") + require.NoError(t, os.WriteFile(gpgSetupLockPath, nil, 0o600)) + + require.NoError(t, widenStaleLockFile()) + + info, err := os.Stat(gpgSetupLockPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) +} From 0d4dd765a196085fd775194d752efb17fc949d4b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 13:58:55 -0500 Subject: [PATCH 14/28] style: cleanup comments --- cmd/internal/agentworkspace/setup_gpg.go | 14 +++----------- cmd/internal/agentworkspace/setup_gpg_test.go | 15 --------------- e2e/tests/ide/browser_returns.go | 5 ++--- pkg/ide/opener/browser_tunnel_test.go | 5 ----- pkg/tunnel/browser_test.go | 2 +- 5 files changed, 6 insertions(+), 35 deletions(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index a98c3ba52..c3572b8fd 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -107,8 +107,6 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { lockCtx, cancel := context.WithTimeout(ctx, gpgSetupLockTimeout) defer cancel() - // A stale lock file (pre-fix binary, or created by another user) may - // still be restrictively-moded; flock's own open() would EACCES on it. if err := widenStaleLockFile(); err != nil { return nil, err } @@ -133,14 +131,12 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { return nil, fmt.Errorf("timed out waiting for another gpg setup to finish") } - // os.Chmod bypasses the umask flock.SetPermissions is subject to. Skipped - // when already 0666, since chmod needs ownership even when mode wouldn't change. if needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666); err != nil { _ = lock.Unlock() return nil, fmt.Errorf("stat lock file: %w", err) } else if needsChmod { - // #nosec G302 -- 0666 is intentional: both root and the workspace's - // remoteUser must be able to acquire this lock. + // #nosec G302 -- both root and the workspace's remoteUser must + // be able to acquire this lock. if err := os.Chmod(gpgSetupLockPath, 0o666); err != nil { _ = lock.Unlock() return nil, fmt.Errorf("set lock file permissions: %w", err) @@ -151,9 +147,7 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { } // lockFileNeedsChmod reports whether path's mode differs from want, so -// callers can skip a chmod that would EPERM a non-owning acquirer. Uses -// Lstat and rejects symlinks: os.Chmod follows them, so a lock path -// replaced with a symlink could redirect the chmod onto an arbitrary file. +// callers can skip a chmod that would EPERM a non-owning acquirer. func lockFileNeedsChmod(path string, want os.FileMode) (bool, error) { info, err := os.Lstat(path) if err != nil { @@ -165,8 +159,6 @@ func lockFileNeedsChmod(path string, want os.FileMode) (bool, error) { return info.Mode().Perm() != want.Perm(), nil } -// widenStaleLockFile chmods a stale, restrictively-moded lock file to 0666 -// (escalating to sudo if we don't own it) before flock's open() can EACCES on it. func widenStaleLockFile() error { needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) if err != nil { diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index c2f9bcbcd..6f77d3be0 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -122,8 +122,6 @@ func TestAcquireGPGSetupLock_FileIsWorldLockable(t *testing.T) { "with EACCES") } -// TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod covers -// a non-owning second acquirer of an already-0666 lock, whose chmod would EPERM. func TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod(t *testing.T) { origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") @@ -149,8 +147,6 @@ func TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod(t *testi reacquire() } -// TestAcquireGPGSetupLock_FixesWrongMode ensures the skip-chmod-if-0666 -// optimization doesn't skip fixing a genuinely wrong mode. func TestAcquireGPGSetupLock_FixesWrongMode(t *testing.T) { origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") @@ -207,13 +203,6 @@ func TestLockFileNeedsChmod_StatErrorPropagates(t *testing.T) { require.Error(t, err) } -// TestAcquireGPGSetupLock_WidensStaleRestrictiveLockFile covers a lock file -// left behind at a restrictive mode (e.g. by a pre-fix binary): flock's own -// open() would EACCES on a 0600 file owned by someone else before ever -// reaching acquireGPGSetupLock's post-lock chmod, so the fix must widen it -// beforehand. This test only exercises the plain-chmod branch (the file is -// owned by the test process); the sudo-escalation branch requires a real -// cross-user setup and isn't reproducible in a unit test. func TestAcquireGPGSetupLock_WidensStaleRestrictiveLockFile(t *testing.T) { origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") @@ -231,10 +220,6 @@ func TestAcquireGPGSetupLock_WidensStaleRestrictiveLockFile(t *testing.T) { assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) } -// TestAcquireGPGSetupLock_RejectsSymlink guards against a symlink planted at -// the lock path redirecting os.Chmod onto an arbitrary target file, since -// os.Chmod follows symlinks and this lock path is a fixed, predictable -// world-writable /tmp path any container user can pre-create. func TestAcquireGPGSetupLock_RejectsSymlink(t *testing.T) { origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout dir := t.TempDir() diff --git a/e2e/tests/ide/browser_returns.go b/e2e/tests/ide/browser_returns.go index a860b3059..8b1f094d1 100644 --- a/e2e/tests/ide/browser_returns.go +++ b/e2e/tests/ide/browser_returns.go @@ -24,9 +24,8 @@ import ( const gpgTestKeyFingerprint = "07F681B9FD6C3411F679BFD1F51769DB572DDD3F" -// setupBrowserIDE prepares a docker provider + workspace tempdir and registers -// the standard cleanup deferred to DeferCleanup. It returns the framework and -// the workspace tempDir path. +// setupBrowserIDE prepares a docker provider and workspace tempdir and registers +// the standard cleanup deferred to DeferCleanup. func setupBrowserIDE(ctx context.Context, initialDir string) (*framework.Framework, string) { f := framework.NewDefaultFramework(initialDir + "/bin") tempDir, err := framework.CopyToTempDir("tests/ide/testdata") diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index ab7779160..3a4782d97 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -16,8 +16,6 @@ import ( "github.com/stretchr/testify/assert" ) -// containsAdjacent returns true if args contains needle followed immediately -// by value. func containsAdjacent(args []string, needle, value string) bool { for i := 0; i < len(args)-1; i++ { if args[i] == needle && args[i+1] == value { @@ -106,9 +104,6 @@ func TestBuildHelperArgs_OpenBrowser(t *testing.T) { } } -// TestBuildHelperArgs_IncludesResolvedUser guards against the fleet/openBrowserIDE -// user-propagation regression: the detached helper must run as the resolved -// workspace user, never empty/root. func TestBuildHelperArgs_IncludesResolvedUser(t *testing.T) { args := buildHelperArgs("default", "my-workspace", tunnel.BrowserTunnelParams{ User: "vscode", diff --git a/pkg/tunnel/browser_test.go b/pkg/tunnel/browser_test.go index f49e061f2..596e9d2fe 100644 --- a/pkg/tunnel/browser_test.go +++ b/pkg/tunnel/browser_test.go @@ -54,7 +54,7 @@ func (fakeWorkspaceClient) Unlock() {} var _ client2.BaseWorkspaceClient = fakeWorkspaceClient{} // exitError runs a shell command that exits with the given code and returns -// the resulting error (which wraps *exec.ExitError). +// the resulting error. func exitError(t *testing.T, code int) error { t.Helper() // #nosec G204 -- test helper with controlled exit code argument From 4afd110c3f31c3a34be185d2a4e5b7ebe316a0b1 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 19:31:21 +0000 Subject: [PATCH 15/28] fix(gpg): bound widenStaleLockFile's sudo call with context and -n Without a context or sudo -n, a sudo prompt for credentials could block acquireGPGSetupLock indefinitely, defeating gpgSetupLockTimeout entirely. exec.CommandContext ties the subprocess to the caller's existing lock timeout; -n makes sudo fail immediately instead of prompting. Addresses a CodeRabbit finding on PR #869. The paired finding (missing testCtxName in pkg/tunnel/browser_test.go) is a false positive: the constant is declared in the same package's services_test.go:13 and the package already builds and vets clean. --- cmd/internal/agentworkspace/setup_gpg.go | 9 ++++++--- cmd/internal/agentworkspace/setup_gpg_test.go | 6 +++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index c3572b8fd..25f2a7c8c 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -107,7 +107,7 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { lockCtx, cancel := context.WithTimeout(ctx, gpgSetupLockTimeout) defer cancel() - if err := widenStaleLockFile(); err != nil { + if err := widenStaleLockFile(lockCtx); err != nil { return nil, err } @@ -159,7 +159,7 @@ func lockFileNeedsChmod(path string, want os.FileMode) (bool, error) { return info.Mode().Perm() != want.Perm(), nil } -func widenStaleLockFile() error { +func widenStaleLockFile(ctx context.Context) error { needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) if err != nil { if os.IsNotExist(err) { @@ -178,8 +178,11 @@ func widenStaleLockFile() error { return fmt.Errorf("widen stale lock file: %w", err) } + // -n: fail immediately instead of prompting if sudo needs a password, + // so a misconfigured container can't hang acquireGPGSetupLock forever. //nolint:gosec // gpgSetupLockPath is a fixed path, not user input - if err := exec.Command("sudo", "chmod", "0666", gpgSetupLockPath).Run(); err != nil { + cmd := exec.CommandContext(ctx, "sudo", "-n", "chmod", "0666", gpgSetupLockPath) + if err := cmd.Run(); err != nil { log.Debugf( "sudo chmod stale gpg setup lock (non-fatal, flock will surface the real error): %v", err, diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index 6f77d3be0..b2259fb71 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -246,14 +246,14 @@ func TestWidenStaleLockFile_NoOpsWhenFileMissingOrAlreadyCorrect(t *testing.T) { defer func() { gpgSetupLockPath = origPath }() gpgSetupLockPath = filepath.Join(t.TempDir(), "does-not-exist") - err := widenStaleLockFile() + err := widenStaleLockFile(context.Background()) require.NoError(t, err, "a missing lock file is flock's job to create, not this") gpgSetupLockPath = filepath.Join(t.TempDir(), "already-0666") require.NoError(t, os.WriteFile(gpgSetupLockPath, nil, 0o600)) //nolint:gosec // test fixture, intentional require.NoError(t, os.Chmod(gpgSetupLockPath, 0o666)) - require.NoError(t, widenStaleLockFile()) + require.NoError(t, widenStaleLockFile(context.Background())) } func TestWidenStaleLockFile_WidensRestrictiveMode(t *testing.T) { @@ -263,7 +263,7 @@ func TestWidenStaleLockFile_WidensRestrictiveMode(t *testing.T) { gpgSetupLockPath = filepath.Join(t.TempDir(), "restrictive") require.NoError(t, os.WriteFile(gpgSetupLockPath, nil, 0o600)) - require.NoError(t, widenStaleLockFile()) + require.NoError(t, widenStaleLockFile(context.Background())) info, err := os.Stat(gpgSetupLockPath) require.NoError(t, err) From cc5f5d07e5c38079ee0bca577337315e84eddded Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 21:43:13 +0000 Subject: [PATCH 16/28] fix(tunnel): keep the primary browser-tunnel SSH connection running as root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 1 of this fix threaded the resolved remoteUser into startBrowserTunnelSSH's CreateSSHCommand call to align it with the GPG-forwarding tunnel's user, avoiding a collision on shared /tmp coordination files. That connection is also the one runBrowserTunnelServices/RunServices uses to read root-owned files (e.g. DevContainerResultPath, 0600 root:root) and to bootstrap the container-side ssh-server that serves the actual port-forward listener. Su-wrapping that bootstrap into remoteUser broke both: RunServices's forwardDevContainerPorts got permission denied on result.json, and the browser-tunnel's port-forward stopped surviving past the idle-timeout window — reproduced deterministically against real Docker containers, twice. The GPG lock/activity file collision this was meant to prevent is already resolved independently by making /tmp/devsy-gpg-setup.lock and /tmp/devsy.activity safe for a mixed root+remoteUser pair (see acquireGPGSetupLock's world-lockable mode and ensureActivityFile's 0666 mode) — this connection never needed to match remoteUser in the first place. Verified against real Docker containers: the previously-flaky "browser- tunnel port-forward survives past the old 5s idle timeout" e2e spec now passes consistently (5/5 local runs) with this revert; it failed consistently before it, on both the pre- and post-Task-1 code. --- pkg/tunnel/browser.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/tunnel/browser.go b/pkg/tunnel/browser.go index c45844999..b61290131 100644 --- a/pkg/tunnel/browser.go +++ b/pkg/tunnel/browser.go @@ -71,7 +71,13 @@ func startBrowserTunnelSSH(ctx context.Context, p BrowserTunnelParams) error { writer := log.Writer(log.LevelDebug) defer func() { _ = writer.Close() }() - sshCmd, err := CreateSSHCommand(ctx, p.Client, p.User, []string{ + // Stays root: runBrowserTunnelServices/RunServices reads + // root-owned files (e.g. DevContainerResultPath) over this same + // connection. GPG's shared /tmp lock/activity files are already + // safe for a mixed root+remoteUser pair (see acquireGPGSetupLock, + // ensureActivityFile) — this session doesn't need to match + // remoteUser to avoid that collision. + sshCmd, err := CreateSSHCommand(ctx, p.Client, "", []string{ names.FlagValue(names.LogOutput, "raw"), names.FlagValue(names.ReuseSSHAuthSock, p.AuthSockID), names.Flag(names.Stdio), From 17249ce6e22c2a3e167424ebc63183e027ddf817 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 22:31:40 +0000 Subject: [PATCH 17/28] fix(tunnel): decouple browser-tunnel SSH user from DevContainerResultPath ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widens /var/run/devsy/result.json to 0644 (readable by any container user; it holds no secrets, just devcontainer.json's merged config/mounts/port attributes) instead of the previous 0600 root:root, and adds a chmod to correct a stale restrictive mode left by a pre-fix binary. This removes the actual coupling behind the prior commit's "keep the primary browser-tunnel connection root" workaround: getContainerResult and credentials_server.go's portOptionsFromResult both read this file over an SSH session that can be authenticated as either root or the workspace's remoteUser (pkg/ssh/server/ssh_container.go's containerServer.handler sets the process credential directly from the authenticated session user — one identity per session, no separate privilege-drop step). With the file world-readable, startBrowserTunnelSSH's connection can use p.User again, matching the GPG-forwarding tunnel's user as originally intended, without breaking RunServices' privileged-file read. Verified against real Docker containers: the full "browser IDE returns instead of blocking" Ordered container (7 specs, including both the idle- timeout regression from the prior commit and the GPG-collision e2e test) now passes cleanly with p.User restored. --- pkg/devcontainer/setup/setup.go | 34 +++++++++---- pkg/devcontainer/setup/setup_test.go | 76 ++++++++++++++++++++++++++++ pkg/tunnel/browser.go | 18 +++---- 3 files changed, 109 insertions(+), 19 deletions(-) diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index 3e2899e6c..6080ee08f 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -210,21 +210,37 @@ func writeResultFile(cfg *ContainerSetupConfig) { return } - existing, _ := os.ReadFile(pkgconfig.DevContainerResultPath) + if err := writeResultFileTo(pkgconfig.DevContainerResultPath, rawBytes); err != nil { + log.Warnf("error write result to %s: %v", pkgconfig.DevContainerResultPath, err) + } +} + +// writeResultFileTo writes rawBytes to path at mode 0644: readable by any +// container user, not just root. Callers that read this file +// (getContainerResult, portOptionsFromResult) run over SSH sessions +// authenticated as either root or the workspace's remoteUser, and the file +// holds no secrets — just devcontainer.json's merged config, mounts, and +// port attributes. Skips the write entirely if content is unchanged. +func writeResultFileTo(path string, rawBytes []byte) error { + // #nosec G304 -- callers pass a fixed const path; parameterized only for tests + existing, _ := os.ReadFile(path) if string(rawBytes) == string(existing) { - return + return nil } - if err := os.MkdirAll( // #nosec G301 - filepath.Dir(pkgconfig.DevContainerResultPath), - 0o755, - ); err != nil { - log.Warnf("error create %s: %v", filepath.Dir(pkgconfig.DevContainerResultPath), err) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { // #nosec G301 + return fmt.Errorf("create %s: %w", filepath.Dir(path), err) } - if err := os.WriteFile(pkgconfig.DevContainerResultPath, rawBytes, 0o600); err != nil { - log.Warnf("error write result to %s: %v", pkgconfig.DevContainerResultPath, err) + if err := os.WriteFile(path, rawBytes, 0o644); err != nil { //nolint:gosec // see doc comment + return fmt.Errorf("write: %w", err) } + // os.WriteFile's mode is subject to umask; chmod to guarantee 0644 + // regardless of what wrote/created the file first. + if err := os.Chmod(path, 0o644); err != nil { //nolint:gosec // see doc comment + return fmt.Errorf("chmod: %w", err) + } + return nil } func setupWorkspaceOwnership(cfg *ContainerSetupConfig) error { diff --git a/pkg/devcontainer/setup/setup_test.go b/pkg/devcontainer/setup/setup_test.go index 8b283a9d2..83055171d 100644 --- a/pkg/devcontainer/setup/setup_test.go +++ b/pkg/devcontainer/setup/setup_test.go @@ -2,6 +2,7 @@ package setup import ( "context" + "os" "path/filepath" "testing" @@ -104,3 +105,78 @@ func TestSetupKubeConfig_NonEmptyPayloadEmitsInfoLog(t *testing.T) { ) } } + +// TestWriteResultFileTo_ProducesWorldReadableFile guards the fix for a +// SSH-session-user footgun: getContainerResult and portOptionsFromResult +// read this file over sessions that may authenticate as root or the +// workspace's remoteUser, so it must not be locked to whichever user's +// process happened to create it first. +func TestWriteResultFileTo_ProducesWorldReadableFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "nested", "result.json") + + if err := writeResultFileTo(path, []byte(`{"ok":true}`)); err != nil { + t.Fatalf("writeResultFileTo: %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := info.Mode().Perm(); got != 0o644 { + t.Errorf("mode = %o, want 0644 (must be readable by any container user)", got) + } +} + +// TestWriteResultFileTo_SkipsWriteWhenContentUnchanged locks in the +// no-op-on-unchanged-content behavior so frequent writeResultFile calls +// during setup don't repeatedly touch the file's mtime/mode for no reason. +func TestWriteResultFileTo_SkipsWriteWhenContentUnchanged(t *testing.T) { + path := filepath.Join(t.TempDir(), "result.json") + content := []byte(`{"ok":true}`) + + if err := writeResultFileTo(path, content); err != nil { + t.Fatalf("first write: %v", err) + } + first, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + + if err := writeResultFileTo(path, content); err != nil { + t.Fatalf("second write: %v", err) + } + second, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + + if !second.ModTime().Equal(first.ModTime()) { + t.Errorf( + "mtime changed on unchanged content: first=%v second=%v", + first.ModTime(), second.ModTime(), + ) + } +} + +// TestWriteResultFileTo_WidensExistingRestrictiveMode ensures a file left +// behind by a pre-fix binary (0600) gets corrected on the next write, not +// just newly-created files. +func TestWriteResultFileTo_WidensExistingRestrictiveMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "result.json") + // #nosec G306 -- intentional: simulating a pre-fix file left at 0600 + if err := os.WriteFile(path, []byte(`{"old":true}`), 0o600); err != nil { + t.Fatalf("seed file: %v", err) + } + + if err := writeResultFileTo(path, []byte(`{"new":true}`)); err != nil { + t.Fatalf("writeResultFileTo: %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := info.Mode().Perm(); got != 0o644 { + t.Errorf("mode = %o, want 0644 (a stale 0600 file must be widened)", got) + } +} diff --git a/pkg/tunnel/browser.go b/pkg/tunnel/browser.go index b61290131..0233b2495 100644 --- a/pkg/tunnel/browser.go +++ b/pkg/tunnel/browser.go @@ -71,13 +71,7 @@ func startBrowserTunnelSSH(ctx context.Context, p BrowserTunnelParams) error { writer := log.Writer(log.LevelDebug) defer func() { _ = writer.Close() }() - // Stays root: runBrowserTunnelServices/RunServices reads - // root-owned files (e.g. DevContainerResultPath) over this same - // connection. GPG's shared /tmp lock/activity files are already - // safe for a mixed root+remoteUser pair (see acquireGPGSetupLock, - // ensureActivityFile) — this session doesn't need to match - // remoteUser to avoid that collision. - sshCmd, err := CreateSSHCommand(ctx, p.Client, "", []string{ + sshCmd, err := CreateSSHCommand(ctx, p.Client, p.User, []string{ names.FlagValue(names.LogOutput, "raw"), names.FlagValue(names.ReuseSSHAuthSock, p.AuthSockID), names.Flag(names.Stdio), @@ -248,9 +242,13 @@ func isTransientBackhaulErr(err error) bool { return exitErr.ExitCode() == exitcode.Retryable } -// CreateSSHCommand builds an exec.Cmd that runs `devsy ssh` with the given arguments. -// user must match the ssh-server/gpg-setup sessions' user, or they collide on -// shared /tmp coordination files (devsy-gpg-setup.lock, devsy.activity). +// CreateSSHCommand builds an exec.Cmd that runs `devsy ssh` with the given +// arguments. The container's SSH server has one identity per session: user +// both authenticates the connection and is who every command on it runs as +// (pkg/ssh/server/ssh_container.go's getCommand calls PrepareCmdUser with the +// authenticated user, no separate privilege-drop step). Callers whose later +// traffic on this same session needs specific file access must pick user +// accordingly — empty defaults to root. func CreateSSHCommand( ctx context.Context, client client2.BaseWorkspaceClient, From 7eba9f322f234699488796a85c76023405b2113f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 23:10:11 +0000 Subject: [PATCH 18/28] refactor: consolidate world-writable cross-UID coordination files into pkg/sharedfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A devsy container runs commands over SSH sessions authenticated as either root or the workspace's remoteUser, so any file two sessions both touch needs its permissions to survive being created by either one — the standard Unix pattern for this (like /tmp/.X11-unix or /var/run/utmp), not a workaround. Three call sites implemented that pattern's safety requirements inconsistently, having drifted apart over time: - cmd/internal/agentworkspace/setup_gpg.go (acquireGPGSetupLock): correct, stat-first chmod plus a sudo fallback for stale root-owned files — the reference implementation this refactor generalizes. - cmd/internal/agentcontainer/daemon.go (setupTimeout): unconditional os.Chmod on every call, which EPERMs when a non-owning process's activity file is already correctly moded. - cmd/internal/fleet_server.go: os.Create with no explicit mode or chmod at all, so under a typical 022 umask it silently creates the activity file at 0644 and locks out whichever user isn't its own UID. pkg/sharedfile centralizes the safety logic (create-if-missing without truncating a concurrent creator's write, stat-first chmod to avoid EPERM for a non-owning widener, Lstat + symlink rejection so a planted symlink at the fixed coordination-file path can't redirect a chmod onto an arbitrary target, and an optional non-interactive sudo fallback for the one caller that needs it). All three call sites, plus cmd/internal's own ensureActivityFile, now delegate to it instead of re-implementing it. fleet_server.go's activity-touch is extracted into ssh_server.go's new touchActivityFile, parameterized on path (was previously hardcoded to config.ContainerActivityFile, making it untestable). Verified against real Docker containers: the full "browser IDE returns instead of blocking" Ordered container (7 specs, including the idle- timeout and GPG-collision regressions from prior commits on this branch) passes cleanly with this refactor applied. --- cmd/internal/agentcontainer/daemon.go | 12 +- cmd/internal/agentworkspace/setup_gpg.go | 82 +++--------- cmd/internal/agentworkspace/setup_gpg_test.go | 68 ---------- cmd/internal/fleet_server.go | 3 +- cmd/internal/ssh_server.go | 32 +++-- cmd/internal/ssh_server_test.go | 26 ++++ pkg/sharedfile/sharedfile.go | 85 ++++++++++++ pkg/sharedfile/sharedfile_test.go | 124 ++++++++++++++++++ pkg/sharedfile/sudo.go | 46 +++++++ pkg/sharedfile/sudo_test.go | 40 ++++++ 10 files changed, 359 insertions(+), 159 deletions(-) create mode 100644 pkg/sharedfile/sharedfile.go create mode 100644 pkg/sharedfile/sharedfile_test.go create mode 100644 pkg/sharedfile/sudo.go create mode 100644 pkg/sharedfile/sudo_test.go diff --git a/cmd/internal/agentcontainer/daemon.go b/cmd/internal/agentcontainer/daemon.go index cdf0d91ed..bfd74e1e9 100644 --- a/cmd/internal/agentcontainer/daemon.go +++ b/cmd/internal/agentcontainer/daemon.go @@ -20,6 +20,7 @@ import ( "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/platform/client" + "github.com/devsy-org/devsy/pkg/sharedfile" "github.com/devsy-org/devsy/pkg/ts" "github.com/spf13/cobra" "golang.org/x/sync/errgroup" @@ -98,15 +99,8 @@ func (cmd *DaemonCmd) setupTimeout() (time.Duration, error) { return 0, fmt.Errorf("failed to parse timeout duration: %w", err) } if timeoutDuration > 0 { - if err := os.WriteFile( // #nosec G306 - config2.ContainerActivityFile, - nil, - 0o666, - ); err != nil { - return 0, fmt.Errorf("failed to create activity file: %w", err) - } - if err := os.Chmod(config2.ContainerActivityFile, 0o666); err != nil { // #nosec G302 - return 0, fmt.Errorf("failed to set activity file permissions: %w", err) + if err := sharedfile.EnsureMode(config2.ContainerActivityFile, 0o666); err != nil { + return 0, fmt.Errorf("failed to ensure activity file: %w", err) } } diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index 25f2a7c8c..1aafd943a 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -3,11 +3,7 @@ package agentworkspace import ( "context" "encoding/base64" - "errors" "fmt" - "io/fs" - "os" - "os/exec" "time" "github.com/devsy-org/devsy/cmd/flags" @@ -17,6 +13,7 @@ import ( "github.com/devsy-org/devsy/pkg/gitcredentials" "github.com/devsy-org/devsy/pkg/gpg" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/sharedfile" "github.com/gofrs/flock" "github.com/spf13/cobra" ) @@ -101,19 +98,28 @@ func (cmd *SetupGPGCmd) Run(ctx context.Context) error { return nil } +// gpgSetupLockMode is 0666: setup-gpg can run as root or the workspace's +// remoteUser, and flock's default 0600 mode would lock the second one out +// with EACCES. See pkg/sharedfile for why a world-writable coordination +// file needs this and how it's kept safe. +const gpgSetupLockMode = 0o666 + // acquireGPGSetupLock takes the cross-process lock guarding setup-gpg. On // success it returns a func that releases the lock. func acquireGPGSetupLock(ctx context.Context) (func(), error) { lockCtx, cancel := context.WithTimeout(ctx, gpgSetupLockTimeout) defer cancel() - if err := widenStaleLockFile(lockCtx); err != nil { - return nil, err + // Repairs a lock file a pre-existing binary left at a restrictive mode, + // which this process (running as whichever user didn't create it) can't + // fix itself once flock.TryLockContext below fails with EACCES. + if err := sharedfile.WidenWithSudoFallback( + lockCtx, gpgSetupLockPath, gpgSetupLockMode, log.Debugf, + ); err != nil { + return nil, fmt.Errorf("widen stale lock file: %w", err) } - // 0666: setup-gpg can run as root or the workspace's remoteUser, and - // flock's default 0600 mode would lock the second one out with EACCES. - lock := flock.New(gpgSetupLockPath, flock.SetPermissions(0o666)) + lock := flock.New(gpgSetupLockPath, flock.SetPermissions(gpgSetupLockMode)) locked, err := lock.TryLockContext(lockCtx, 200*time.Millisecond) if err != nil { if ctx.Err() != nil { @@ -131,66 +137,16 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { return nil, fmt.Errorf("timed out waiting for another gpg setup to finish") } - if needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666); err != nil { + // flock.SetPermissions is subject to the process umask on create; + // widen again to guarantee the mode regardless of who created it. + if err := sharedfile.WidenIfNeeded(gpgSetupLockPath, gpgSetupLockMode); err != nil { _ = lock.Unlock() - return nil, fmt.Errorf("stat lock file: %w", err) - } else if needsChmod { - // #nosec G302 -- both root and the workspace's remoteUser must - // be able to acquire this lock. - if err := os.Chmod(gpgSetupLockPath, 0o666); err != nil { - _ = lock.Unlock() - return nil, fmt.Errorf("set lock file permissions: %w", err) - } + return nil, fmt.Errorf("set lock file permissions: %w", err) } return func() { _ = lock.Unlock() }, nil } -// lockFileNeedsChmod reports whether path's mode differs from want, so -// callers can skip a chmod that would EPERM a non-owning acquirer. -func lockFileNeedsChmod(path string, want os.FileMode) (bool, error) { - info, err := os.Lstat(path) - if err != nil { - return false, err - } - if info.Mode()&os.ModeSymlink != 0 { - return false, fmt.Errorf("refusing to chmod %s: path is a symlink", path) - } - return info.Mode().Perm() != want.Perm(), nil -} - -func widenStaleLockFile(ctx context.Context) error { - needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) - if err != nil { - if os.IsNotExist(err) { - return nil - } - return fmt.Errorf("stat lock file: %w", err) - } - if !needsChmod { - return nil - } - - // #nosec G302 -- 0666 is intentional; see acquireGPGSetupLock. - if err := os.Chmod(gpgSetupLockPath, 0o666); err == nil { - return nil - } else if !errors.Is(err, fs.ErrPermission) { - return fmt.Errorf("widen stale lock file: %w", err) - } - - // -n: fail immediately instead of prompting if sudo needs a password, - // so a misconfigured container can't hang acquireGPGSetupLock forever. - //nolint:gosec // gpgSetupLockPath is a fixed path, not user input - cmd := exec.CommandContext(ctx, "sudo", "-n", "chmod", "0666", gpgSetupLockPath) - if err := cmd.Run(); err != nil { - log.Debugf( - "sudo chmod stale gpg setup lock (non-fatal, flock will surface the real error): %v", - err, - ) - } - return nil -} - func fetchAndDecodeKeys(ownerTrustB64 string) ([]byte, []byte, error) { log.Debugf("Fetching public key") rawPublicKeys, err := getPublicKeys() diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index b2259fb71..51341bf3d 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -136,12 +136,6 @@ func TestAcquireGPGSetupLock_SecondAcquireOfAlready0666FileDoesNotChmod(t *testi require.NoError(t, err) require.Equal(t, os.FileMode(0o666), info.Mode().Perm()) - needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) - require.NoError(t, err) - assert.False(t, needsChmod, - "a lock file already at 0666 must not need a chmod, since a "+ - "non-owning second acquirer's chmod would fail with EPERM") - reacquire, err := acquireGPGSetupLock(context.Background()) require.NoError(t, err, "second acquisition of an already-0666 lock file must succeed") reacquire() @@ -160,10 +154,6 @@ func TestAcquireGPGSetupLock_FixesWrongMode(t *testing.T) { // #nosec G302 -- intentional: simulating a wrong pre-existing mode require.NoError(t, os.Chmod(gpgSetupLockPath, 0o644)) - needsChmod, err := lockFileNeedsChmod(gpgSetupLockPath, 0o666) - require.NoError(t, err) - assert.True(t, needsChmod, "a lock file at 0644 must be reported as needing a chmod to 0666") - reacquire, err := acquireGPGSetupLock(context.Background()) require.NoError(t, err) defer reacquire() @@ -174,35 +164,6 @@ func TestAcquireGPGSetupLock_FixesWrongMode(t *testing.T) { "acquireGPGSetupLock must fix a wrong mode back to 0666") } -func TestLockFileNeedsChmod(t *testing.T) { - tests := []struct { - name string - mode os.FileMode - want os.FileMode - expect bool - }{ - {name: "already matches", mode: 0o666, want: 0o666, expect: false}, - {name: "narrower mode needs widening", mode: 0o644, want: 0o666, expect: true}, - {name: "wider mode needs narrowing", mode: 0o777, want: 0o666, expect: true}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - path := filepath.Join(t.TempDir(), "lock") - require.NoError(t, os.WriteFile(path, nil, tc.mode)) - require.NoError(t, os.Chmod(path, tc.mode)) - - got, err := lockFileNeedsChmod(path, tc.want) - require.NoError(t, err) - assert.Equal(t, tc.expect, got) - }) - } -} - -func TestLockFileNeedsChmod_StatErrorPropagates(t *testing.T) { - _, err := lockFileNeedsChmod(filepath.Join(t.TempDir(), "does-not-exist"), 0o666) - require.Error(t, err) -} - func TestAcquireGPGSetupLock_WidensStaleRestrictiveLockFile(t *testing.T) { origPath, origTimeout := gpgSetupLockPath, gpgSetupLockTimeout gpgSetupLockPath = filepath.Join(t.TempDir(), "setup-gpg.lock") @@ -240,32 +201,3 @@ func TestAcquireGPGSetupLock_RejectsSymlink(t *testing.T) { assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "the symlink target's mode must be untouched, not widened to 0666") } - -func TestWidenStaleLockFile_NoOpsWhenFileMissingOrAlreadyCorrect(t *testing.T) { - origPath := gpgSetupLockPath - defer func() { gpgSetupLockPath = origPath }() - - gpgSetupLockPath = filepath.Join(t.TempDir(), "does-not-exist") - err := widenStaleLockFile(context.Background()) - require.NoError(t, err, "a missing lock file is flock's job to create, not this") - - gpgSetupLockPath = filepath.Join(t.TempDir(), "already-0666") - require.NoError(t, os.WriteFile(gpgSetupLockPath, nil, 0o600)) - //nolint:gosec // test fixture, intentional - require.NoError(t, os.Chmod(gpgSetupLockPath, 0o666)) - require.NoError(t, widenStaleLockFile(context.Background())) -} - -func TestWidenStaleLockFile_WidensRestrictiveMode(t *testing.T) { - origPath := gpgSetupLockPath - defer func() { gpgSetupLockPath = origPath }() - - gpgSetupLockPath = filepath.Join(t.TempDir(), "restrictive") - require.NoError(t, os.WriteFile(gpgSetupLockPath, nil, 0o600)) - - require.NoError(t, widenStaleLockFile(context.Background())) - - info, err := os.Stat(gpgSetupLockPath) - require.NoError(t, err) - assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) -} diff --git a/cmd/internal/fleet_server.go b/cmd/internal/fleet_server.go index 734b766cc..f579aa2b4 100644 --- a/cmd/internal/fleet_server.go +++ b/cmd/internal/fleet_server.go @@ -72,8 +72,7 @@ func (c *FleetServerCmd) Run(cmd *cobra.Command, _ []string) error { // if ouf last occurrence of notify if "Notify ID connected" // we have an active session, so let's keep alive if strings.Contains(connString[len(connString)-1][0], "is connected") { - file, _ := os.Create(config.ContainerActivityFile) - _ = file.Close() + touchActivityFile(config.ContainerActivityFile) } case <-cmd.Context().Done(): // context is done - either canceled or time is up for timeout diff --git a/cmd/internal/ssh_server.go b/cmd/internal/ssh_server.go index 187c69d60..156e10fce 100644 --- a/cmd/internal/ssh_server.go +++ b/cmd/internal/ssh_server.go @@ -5,7 +5,6 @@ import ( "encoding/base64" "errors" "fmt" - "io/fs" "os" "time" @@ -13,6 +12,7 @@ import ( "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/sharedfile" sshserver "github.com/devsy-org/devsy/pkg/ssh/server" "github.com/devsy-org/devsy/pkg/ssh/server/port" "github.com/devsy-org/devsy/pkg/stdio" @@ -233,22 +233,20 @@ func runActivityHeartbeat(ctx context.Context, path string) { } func ensureActivityFile(path string) error { - _, err := os.Stat(path) - if err == nil { - return nil - } - if !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("stat: %w", err) - } - if err := os.WriteFile( - path, - nil, - activityFileMode, - ); err != nil { // #nosec G306 -- intentionally world-writable; multiple users update activity - return fmt.Errorf("create: %w", err) + return sharedfile.EnsureMode(path, activityFileMode) +} + +// touchActivityFile records liveness by creating path at activityFileMode +// if absent, then updating its mtime. Used by callers (e.g. the +// fleet-server monitor) that report activity on discrete events rather +// than a fixed heartbeat interval. +func touchActivityFile(path string) { + if err := ensureActivityFile(path); err != nil { + log.Errorf("touch activity file: ensure file: %v", err) + return } - if err := os.Chmod(path, activityFileMode); err != nil { // #nosec G302 -- ditto - return fmt.Errorf("chmod: %w", err) + now := time.Now() + if err := os.Chtimes(path, now, now); err != nil { + log.Errorf("touch activity file: %v", err) } - return nil } diff --git a/cmd/internal/ssh_server_test.go b/cmd/internal/ssh_server_test.go index 3b11eb66e..573bf6201 100644 --- a/cmd/internal/ssh_server_test.go +++ b/cmd/internal/ssh_server_test.go @@ -210,3 +210,29 @@ func TestEnsureActivityFile_NoOpsWhenFileAlreadyExists(t *testing.T) { assert.Equal(t, "existing", string(data), "ensureActivityFile must not truncate a file that already exists") } + +func TestTouchActivityFile_CreatesFileAndUpdatesMtime(t *testing.T) { + path := filepath.Join(t.TempDir(), "devsy.activity") + + touchActivityFile(path) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) + assert.WithinDuration(t, time.Now(), info.ModTime(), 2*time.Second) +} + +func TestTouchActivityFile_UpdatesMtimeOfExistingFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "devsy.activity") + old := time.Now().Add(-time.Hour) + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, nil, 0o666)) + require.NoError(t, os.Chtimes(path, old, old)) + + touchActivityFile(path) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.WithinDuration(t, time.Now(), info.ModTime(), 2*time.Second, + "touchActivityFile must advance mtime on an already-existing file") +} diff --git a/pkg/sharedfile/sharedfile.go b/pkg/sharedfile/sharedfile.go new file mode 100644 index 000000000..1b7e1fcc2 --- /dev/null +++ b/pkg/sharedfile/sharedfile.go @@ -0,0 +1,85 @@ +// Package sharedfile hardens the standard Unix pattern of coordinating +// between processes running as different users through a world-writable +// file in a shared, trusted location (e.g. /tmp) — the same idea behind +// /tmp/.X11-unix or /var/run/utmp. A devsy container runs commands over SSH +// sessions authenticated as either root or the workspace's remoteUser +// (pkg/ssh/server/ssh_container.go sets the process credential directly +// from the authenticated session user), so any file two of those sessions +// both touch needs its permissions to survive being created by either one. +// +// The pattern has two failure modes this package exists to close: +// - Whichever process creates the file first can lock every other user +// out, because file creation is subject to the process umask. +// - A symlink planted at the file's path redirects Chmod onto an +// arbitrary target, since Chmod follows symlinks — dangerous for a +// fixed, predictable, world-writable path any container user can +// pre-create. +package sharedfile + +import ( + "errors" + "fmt" + "os" +) + +// EnsureMode ensures path exists with exactly mode permissions, creating it +// if absent. Skips the chmod when the file's mode already matches: chmod +// requires ownership (or root) even when the requested mode wouldn't +// change, so skipping it when unnecessary avoids EPERM for a non-owning +// acquirer of an already-correctly-moded file. +// +// Rejects a path that resolves to a symlink rather than following it. +func EnsureMode(path string, mode os.FileMode) error { + if err := createIfMissing(path, mode); err != nil { + return err + } + return WidenIfNeeded(path, mode) +} + +// WidenIfNeeded chmods path to mode if its current mode differs, skipping +// the chmod entirely when it's already correct. Rejects a path that +// resolves to a symlink rather than following it. +func WidenIfNeeded(path string, mode os.FileMode) error { + needsChmod, err := needsChmod(path, mode) + if err != nil { + return err + } + if !needsChmod { + return nil + } + //nolint:gosec // callers intentionally widen a fixed coordination-file path + if err := os.Chmod(path, mode); err != nil { + return fmt.Errorf("chmod %s: %w", path, err) + } + return nil +} + +// createIfMissing creates path at mode if it doesn't already exist. Leaves +// an existing file untouched — its mode is WidenIfNeeded's job — so this +// never races a concurrent creator into truncating their write. +func createIfMissing(path string, mode os.FileMode) error { + //nolint:gosec // callers intentionally create a fixed, world-accessible coordination file + f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, mode) + if err != nil { + if errors.Is(err, os.ErrExist) { + return nil + } + return fmt.Errorf("create %s: %w", path, err) + } + return f.Close() +} + +// needsChmod reports whether path's current permission bits differ from +// want. Uses Lstat and rejects symlinks: os.Chmod follows them, so a +// coordination-file path replaced with a symlink could redirect a chmod +// onto an arbitrary file. +func needsChmod(path string, want os.FileMode) (bool, error) { + info, err := os.Lstat(path) + if err != nil { + return false, err + } + if info.Mode()&os.ModeSymlink != 0 { + return false, fmt.Errorf("refusing to chmod %s: path is a symlink", path) + } + return info.Mode().Perm() != want.Perm(), nil +} diff --git a/pkg/sharedfile/sharedfile_test.go b/pkg/sharedfile/sharedfile_test.go new file mode 100644 index 000000000..459ead391 --- /dev/null +++ b/pkg/sharedfile/sharedfile_test.go @@ -0,0 +1,124 @@ +package sharedfile + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEnsureMode_CreatesMissingFileAtMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + + require.NoError(t, EnsureMode(path, 0o666)) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) +} + +func TestEnsureMode_WidensExistingRestrictiveFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + require.NoError(t, os.WriteFile(path, []byte("hello"), 0o600)) + + require.NoError(t, EnsureMode(path, 0o666)) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) + //nolint:gosec // test-owned temp path + content, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "hello", string(content), "widening mode must not touch existing content") +} + +func TestCreateIfMissing_LeavesExistingFileUntouched(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, []byte("existing"), 0o644)) + + require.NoError(t, createIfMissing(path, 0o666)) + + //nolint:gosec // test-owned temp path + content, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "existing", string(content), + "createIfMissing must not truncate a file a concurrent creator just wrote") +} + +func TestWidenIfNeeded_SkipsChmodWhenModeAlreadyCorrect(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, nil, 0o666)) + //nolint:gosec // test fixture, intentional + require.NoError(t, os.Chmod(path, 0o666)) + + needsChmod, err := needsChmod(path, 0o666) + require.NoError(t, err) + assert.False(t, needsChmod, + "a file already at the target mode must not need a chmod, since a "+ + "non-owning acquirer's chmod would fail with EPERM") + + require.NoError(t, WidenIfNeeded(path, 0o666)) +} + +func TestNeedsChmod(t *testing.T) { + tests := []struct { + name string + mode os.FileMode + want os.FileMode + expect bool + }{ + {name: "already matches", mode: 0o666, want: 0o666, expect: false}, + {name: "narrower mode needs widening", mode: 0o644, want: 0o666, expect: true}, + {name: "wider mode needs narrowing", mode: 0o777, want: 0o666, expect: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + require.NoError(t, os.WriteFile(path, nil, tc.mode)) + //nolint:gosec // test fixture, intentional + require.NoError(t, os.Chmod(path, tc.mode)) + + got, err := needsChmod(path, tc.want) + require.NoError(t, err) + assert.Equal(t, tc.expect, got) + }) + } +} + +func TestNeedsChmod_StatErrorPropagates(t *testing.T) { + _, err := needsChmod(filepath.Join(t.TempDir(), "does-not-exist"), 0o666) + require.Error(t, err) +} + +func TestNeedsChmod_RejectsSymlink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + link := filepath.Join(dir, "link") + require.NoError(t, os.WriteFile(target, nil, 0o600)) + require.NoError(t, os.Symlink(target, link)) + + _, err := needsChmod(link, 0o666) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") +} + +func TestWidenIfNeeded_RejectsSymlinkWithoutTouchingTarget(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + link := filepath.Join(dir, "link") + require.NoError(t, os.WriteFile(target, nil, 0o600)) + require.NoError(t, os.Symlink(target, link)) + + err := WidenIfNeeded(link, 0o666) + require.Error(t, err) + assert.Contains(t, err.Error(), "symlink") + + info, err := os.Stat(target) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), + "the symlink target's mode must be untouched, not widened") +} diff --git a/pkg/sharedfile/sudo.go b/pkg/sharedfile/sudo.go new file mode 100644 index 000000000..3996ce55b --- /dev/null +++ b/pkg/sharedfile/sudo.go @@ -0,0 +1,46 @@ +package sharedfile + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "os/exec" +) + +// WidenWithSudoFallback behaves like WidenIfNeeded, but on EPERM (the file +// exists at the wrong mode and this process doesn't own it — e.g. a stale +// file a prior root process created before this package's fix, or a +// same-container root/remoteUser pair racing setup) falls back to a +// non-interactive `sudo chmod`. The fallback's failure is logged via logFn +// rather than returned: callers hold a lock/flock that will itself +// surface the real permission error to whichever caller actually needs to +// read or write the file next, so this is a best-effort repair, not a hard +// requirement for the caller's own success. +func WidenWithSudoFallback( + ctx context.Context, + path string, + mode os.FileMode, + logFn func(format string, args ...any), +) error { + err := WidenIfNeeded(path, mode) + if err == nil { + return nil + } + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if !errors.Is(err, fs.ErrPermission) { + return err + } + + // -n: fail immediately instead of prompting if sudo needs a password, + // so a caller holding a timeout-bounded lock can't hang forever. + //nolint:gosec // path is a fixed coordination-file path, not user input + cmd := exec.CommandContext(ctx, "sudo", "-n", "chmod", fmt.Sprintf("%04o", mode.Perm()), path) + if sudoErr := cmd.Run(); sudoErr != nil { + logFn("sudo chmod %s (non-fatal): %v", path, sudoErr) + } + return nil +} diff --git a/pkg/sharedfile/sudo_test.go b/pkg/sharedfile/sudo_test.go new file mode 100644 index 000000000..16075c217 --- /dev/null +++ b/pkg/sharedfile/sudo_test.go @@ -0,0 +1,40 @@ +package sharedfile + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWidenWithSudoFallback_NoOpsWhenFileMissing(t *testing.T) { + path := filepath.Join(t.TempDir(), "does-not-exist") + + err := WidenWithSudoFallback(context.Background(), path, 0o666, t.Logf) + require.NoError(t, err, "a missing file is the caller's job to create, not this") +} + +func TestWidenWithSudoFallback_NoOpsWhenModeAlreadyCorrect(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + //nolint:gosec // test fixture, intentional + require.NoError(t, os.Chmod(path, 0o666)) + + err := WidenWithSudoFallback(context.Background(), path, 0o666, t.Logf) + require.NoError(t, err) +} + +func TestWidenWithSudoFallback_WidensOwnedFileWithoutSudo(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + + require.NoError(t, WidenWithSudoFallback(context.Background(), path, 0o666, t.Logf)) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm(), + "a file this process owns must be widened directly, without needing sudo") +} From 15dfe1f59e03d2ca404c5bb5e3f8c8dfb577ab7b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 23:17:42 +0000 Subject: [PATCH 19/28] style: cleanup comments, route writeResultFileTo through sharedfile - CreateSSHCommand, WidenWithSudoFallback, touchActivityFile, and gpgSetupLockMode: trim WHY comments that had grown into WHAT restatements or cross-package implementation references. - writeResultFileTo: use sharedfile.WidenIfNeeded's umask-safe chmod instead of a fourth hand-rolled copy of the same write-then-chmod logic this session's refactor consolidated everywhere else. - containsAdjacent (browser_tunnel_test.go): restore a doc comment stating argument order, and rename needle/value to flag/value to match. --- cmd/internal/agentworkspace/setup_gpg.go | 6 ++---- cmd/internal/ssh_server.go | 6 ++---- pkg/devcontainer/setup/setup.go | 18 +++++------------- pkg/ide/opener/browser_tunnel_test.go | 6 ++++-- pkg/sharedfile/sudo.go | 14 +++++--------- pkg/tunnel/browser.go | 10 ++++------ 6 files changed, 22 insertions(+), 38 deletions(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index 1aafd943a..d7760b7d2 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -98,10 +98,8 @@ func (cmd *SetupGPGCmd) Run(ctx context.Context) error { return nil } -// gpgSetupLockMode is 0666: setup-gpg can run as root or the workspace's -// remoteUser, and flock's default 0600 mode would lock the second one out -// with EACCES. See pkg/sharedfile for why a world-writable coordination -// file needs this and how it's kept safe. +// gpgSetupLockMode is 0666 — flock's default 0600 would lock out whichever +// of root/remoteUser didn't create the file. See pkg/sharedfile. const gpgSetupLockMode = 0o666 // acquireGPGSetupLock takes the cross-process lock guarding setup-gpg. On diff --git a/cmd/internal/ssh_server.go b/cmd/internal/ssh_server.go index 156e10fce..e58278e3b 100644 --- a/cmd/internal/ssh_server.go +++ b/cmd/internal/ssh_server.go @@ -236,10 +236,8 @@ func ensureActivityFile(path string) error { return sharedfile.EnsureMode(path, activityFileMode) } -// touchActivityFile records liveness by creating path at activityFileMode -// if absent, then updating its mtime. Used by callers (e.g. the -// fleet-server monitor) that report activity on discrete events rather -// than a fixed heartbeat interval. +// touchActivityFile is for callers reporting activity on discrete events +// rather than runActivityHeartbeat's fixed interval (e.g. fleet-server). func touchActivityFile(path string) { if err := ensureActivityFile(path); err != nil { log.Errorf("touch activity file: ensure file: %v", err) diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index 6080ee08f..ed4215694 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -23,6 +23,7 @@ import ( "github.com/devsy-org/devsy/pkg/envfile" "github.com/devsy-org/devsy/pkg/gitcredentials" "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/sharedfile" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" ) @@ -215,12 +216,9 @@ func writeResultFile(cfg *ContainerSetupConfig) { } } -// writeResultFileTo writes rawBytes to path at mode 0644: readable by any -// container user, not just root. Callers that read this file -// (getContainerResult, portOptionsFromResult) run over SSH sessions -// authenticated as either root or the workspace's remoteUser, and the file -// holds no secrets — just devcontainer.json's merged config, mounts, and -// port attributes. Skips the write entirely if content is unchanged. +// writeResultFileTo writes rawBytes to path at 0644: readable by any +// container user, not just root, since getContainerResult and +// portOptionsFromResult read it over sessions authenticated as either. func writeResultFileTo(path string, rawBytes []byte) error { // #nosec G304 -- callers pass a fixed const path; parameterized only for tests existing, _ := os.ReadFile(path) @@ -231,16 +229,10 @@ func writeResultFileTo(path string, rawBytes []byte) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { // #nosec G301 return fmt.Errorf("create %s: %w", filepath.Dir(path), err) } - if err := os.WriteFile(path, rawBytes, 0o644); err != nil { //nolint:gosec // see doc comment return fmt.Errorf("write: %w", err) } - // os.WriteFile's mode is subject to umask; chmod to guarantee 0644 - // regardless of what wrote/created the file first. - if err := os.Chmod(path, 0o644); err != nil { //nolint:gosec // see doc comment - return fmt.Errorf("chmod: %w", err) - } - return nil + return sharedfile.WidenIfNeeded(path, 0o644) } func setupWorkspaceOwnership(cfg *ContainerSetupConfig) error { diff --git a/pkg/ide/opener/browser_tunnel_test.go b/pkg/ide/opener/browser_tunnel_test.go index 3a4782d97..08faf7b90 100644 --- a/pkg/ide/opener/browser_tunnel_test.go +++ b/pkg/ide/opener/browser_tunnel_test.go @@ -16,9 +16,11 @@ import ( "github.com/stretchr/testify/assert" ) -func containsAdjacent(args []string, needle, value string) bool { +// containsAdjacent reports whether flag appears immediately followed by +// value in args, e.g. ["--user", "vscode"]. +func containsAdjacent(args []string, flag, value string) bool { for i := 0; i < len(args)-1; i++ { - if args[i] == needle && args[i+1] == value { + if args[i] == flag && args[i+1] == value { return true } } diff --git a/pkg/sharedfile/sudo.go b/pkg/sharedfile/sudo.go index 3996ce55b..a48ed826b 100644 --- a/pkg/sharedfile/sudo.go +++ b/pkg/sharedfile/sudo.go @@ -9,15 +9,11 @@ import ( "os/exec" ) -// WidenWithSudoFallback behaves like WidenIfNeeded, but on EPERM (the file -// exists at the wrong mode and this process doesn't own it — e.g. a stale -// file a prior root process created before this package's fix, or a -// same-container root/remoteUser pair racing setup) falls back to a -// non-interactive `sudo chmod`. The fallback's failure is logged via logFn -// rather than returned: callers hold a lock/flock that will itself -// surface the real permission error to whichever caller actually needs to -// read or write the file next, so this is a best-effort repair, not a hard -// requirement for the caller's own success. +// WidenWithSudoFallback behaves like WidenIfNeeded, but on EPERM (path +// exists at the wrong mode and this process doesn't own it) falls back to a +// non-interactive `sudo chmod`. The fallback's failure is logged via logFn, +// not returned: this is a best-effort repair, and the caller's own lock +// acquisition will surface the real permission error if the repair fails. func WidenWithSudoFallback( ctx context.Context, path string, diff --git a/pkg/tunnel/browser.go b/pkg/tunnel/browser.go index 0233b2495..05ad5a96a 100644 --- a/pkg/tunnel/browser.go +++ b/pkg/tunnel/browser.go @@ -243,12 +243,10 @@ func isTransientBackhaulErr(err error) bool { } // CreateSSHCommand builds an exec.Cmd that runs `devsy ssh` with the given -// arguments. The container's SSH server has one identity per session: user -// both authenticates the connection and is who every command on it runs as -// (pkg/ssh/server/ssh_container.go's getCommand calls PrepareCmdUser with the -// authenticated user, no separate privilege-drop step). Callers whose later -// traffic on this same session needs specific file access must pick user -// accordingly — empty defaults to root. +// arguments. user both authenticates the session and is who every command +// on it runs as — there's no separate privilege-drop step — so callers +// whose later traffic needs specific file access must pick user +// accordingly. Empty defaults to root. func CreateSSHCommand( ctx context.Context, client client2.BaseWorkspaceClient, From 3231cfe9467446a94d6c219766e7fc7d01594c1e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 23:24:37 +0000 Subject: [PATCH 20/28] refactor(sharedfile): drop WidenWithSudoFallback's injected logger log has no dependency on pkg/sharedfile, so there's no cycle risk in calling pkg/log directly instead of threading a logFn parameter through for the one real caller. --- cmd/internal/agentworkspace/setup_gpg.go | 2 +- pkg/sharedfile/sudo.go | 15 ++++++--------- pkg/sharedfile/sudo_test.go | 6 +++--- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index d7760b7d2..676183047 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -112,7 +112,7 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) { // which this process (running as whichever user didn't create it) can't // fix itself once flock.TryLockContext below fails with EACCES. if err := sharedfile.WidenWithSudoFallback( - lockCtx, gpgSetupLockPath, gpgSetupLockMode, log.Debugf, + lockCtx, gpgSetupLockPath, gpgSetupLockMode, ); err != nil { return nil, fmt.Errorf("widen stale lock file: %w", err) } diff --git a/pkg/sharedfile/sudo.go b/pkg/sharedfile/sudo.go index a48ed826b..bfacd10b3 100644 --- a/pkg/sharedfile/sudo.go +++ b/pkg/sharedfile/sudo.go @@ -7,19 +7,16 @@ import ( "io/fs" "os" "os/exec" + + "github.com/devsy-org/devsy/pkg/log" ) // WidenWithSudoFallback behaves like WidenIfNeeded, but on EPERM (path // exists at the wrong mode and this process doesn't own it) falls back to a -// non-interactive `sudo chmod`. The fallback's failure is logged via logFn, -// not returned: this is a best-effort repair, and the caller's own lock +// non-interactive `sudo chmod`. The fallback's failure is logged, not +// returned: this is a best-effort repair, and the caller's own lock // acquisition will surface the real permission error if the repair fails. -func WidenWithSudoFallback( - ctx context.Context, - path string, - mode os.FileMode, - logFn func(format string, args ...any), -) error { +func WidenWithSudoFallback(ctx context.Context, path string, mode os.FileMode) error { err := WidenIfNeeded(path, mode) if err == nil { return nil @@ -36,7 +33,7 @@ func WidenWithSudoFallback( //nolint:gosec // path is a fixed coordination-file path, not user input cmd := exec.CommandContext(ctx, "sudo", "-n", "chmod", fmt.Sprintf("%04o", mode.Perm()), path) if sudoErr := cmd.Run(); sudoErr != nil { - logFn("sudo chmod %s (non-fatal): %v", path, sudoErr) + log.Debugf("sudo chmod %s (non-fatal): %v", path, sudoErr) } return nil } diff --git a/pkg/sharedfile/sudo_test.go b/pkg/sharedfile/sudo_test.go index 16075c217..2760a8385 100644 --- a/pkg/sharedfile/sudo_test.go +++ b/pkg/sharedfile/sudo_test.go @@ -13,7 +13,7 @@ import ( func TestWidenWithSudoFallback_NoOpsWhenFileMissing(t *testing.T) { path := filepath.Join(t.TempDir(), "does-not-exist") - err := WidenWithSudoFallback(context.Background(), path, 0o666, t.Logf) + err := WidenWithSudoFallback(context.Background(), path, 0o666) require.NoError(t, err, "a missing file is the caller's job to create, not this") } @@ -23,7 +23,7 @@ func TestWidenWithSudoFallback_NoOpsWhenModeAlreadyCorrect(t *testing.T) { //nolint:gosec // test fixture, intentional require.NoError(t, os.Chmod(path, 0o666)) - err := WidenWithSudoFallback(context.Background(), path, 0o666, t.Logf) + err := WidenWithSudoFallback(context.Background(), path, 0o666) require.NoError(t, err) } @@ -31,7 +31,7 @@ func TestWidenWithSudoFallback_WidensOwnedFileWithoutSudo(t *testing.T) { path := filepath.Join(t.TempDir(), "coord") require.NoError(t, os.WriteFile(path, nil, 0o600)) - require.NoError(t, WidenWithSudoFallback(context.Background(), path, 0o666, t.Logf)) + require.NoError(t, WidenWithSudoFallback(context.Background(), path, 0o666)) info, err := os.Stat(path) require.NoError(t, err) From 5b8ec27e4ab0b08347a096edc2133d03a8399de9 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 23:59:10 +0000 Subject: [PATCH 21/28] fix(sharedfile): close TOCTOU race between mode check and chmod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit correctly flagged that WidenIfNeeded's separate Lstat-then- Chmod-by-path had a race window: a symlink planted at path between the check and the chmod would redirect the chmod onto an arbitrary target, since chmod(2) follows symlinks. The prior symlink-rejection fix only caught a symlink already in place at check time, not one swapped in after. WidenIfNeeded now opens path with O_NOFOLLOW and chmods the resulting file descriptor instead of the path — once open, the descriptor is pinned to that inode regardless of what the path later resolves to, so there's no window left to race. WidenWithSudoFallback had the same race one layer up: `sudo chmod ` has no way to refuse following a symlink at its target, unlike chmod's lesser-known variants for other attributes. It now re-execs ` internal widen-shared-file ` under sudo instead, so the escalated mode change still goes through WidenIfNeeded's O_NOFOLLOW open. Also fixes a related bug CodeRabbit flagged in writeResultFileTo: the unchanged-content early return skipped mode-widening entirely, so a stale restrictively-moded file with matching content never got repaired until its content next changed. Verified against real Docker containers: the full "browser IDE returns instead of blocking" Ordered container passes with these changes, including the GPG-collision spec. --- cmd/internal/agentworkspace/setup_gpg_test.go | 4 +- cmd/internal/internal.go | 1 + cmd/internal/widen_shared_file.go | 33 +++++++ cmd/internal/widen_shared_file_test.go | 35 ++++++++ pkg/devcontainer/setup/setup.go | 5 +- pkg/devcontainer/setup/setup_test.go | 25 ++++++ pkg/sharedfile/sharedfile.go | 39 ++++----- pkg/sharedfile/sharedfile_test.go | 86 +++++++++---------- pkg/sharedfile/sudo.go | 27 ++++-- 9 files changed, 179 insertions(+), 76 deletions(-) create mode 100644 cmd/internal/widen_shared_file.go create mode 100644 cmd/internal/widen_shared_file_test.go diff --git a/cmd/internal/agentworkspace/setup_gpg_test.go b/cmd/internal/agentworkspace/setup_gpg_test.go index 51341bf3d..28f2c00cc 100644 --- a/cmd/internal/agentworkspace/setup_gpg_test.go +++ b/cmd/internal/agentworkspace/setup_gpg_test.go @@ -193,8 +193,8 @@ func TestAcquireGPGSetupLock_RejectsSymlink(t *testing.T) { require.NoError(t, os.Symlink(target, gpgSetupLockPath)) _, err := acquireGPGSetupLock(context.Background()) - require.Error(t, err) - assert.Contains(t, err.Error(), "symlink") + require.Error(t, err, + "opening the symlinked lock path with O_NOFOLLOW must fail, not follow it") info, err := os.Stat(target) require.NoError(t, err) diff --git a/cmd/internal/internal.go b/cmd/internal/internal.go index 08c95187a..d7366c03b 100644 --- a/cmd/internal/internal.go +++ b/cmd/internal/internal.go @@ -46,6 +46,7 @@ func NewInternalCmd(globalFlags *flags.GlobalFlags) *cobra.Command { cmd.AddCommand(withPreRun(NewGetImageCmd(globalFlags))) cmd.AddCommand(withPreRun(NewGetImagePlatformsCmd(globalFlags))) cmd.AddCommand(withPreRun(NewBrowserTunnelCmd(globalFlags))) + cmd.AddCommand(withPreRun(NewWidenSharedFileCmd(globalFlags))) return cmd } diff --git a/cmd/internal/widen_shared_file.go b/cmd/internal/widen_shared_file.go new file mode 100644 index 000000000..b1220f62e --- /dev/null +++ b/cmd/internal/widen_shared_file.go @@ -0,0 +1,33 @@ +package cmdinternal + +import ( + "fmt" + "os" + "strconv" + + "github.com/devsy-org/devsy/cmd/flags" + "github.com/devsy-org/devsy/pkg/sharedfile" + "github.com/spf13/cobra" +) + +// NewWidenSharedFileCmd returns a hidden command that runs +// sharedfile.WidenIfNeeded. sharedfile.WidenWithSudoFallback re-execs this +// (via sudo) so the actual mode change still goes through WidenIfNeeded's +// open-with-O_NOFOLLOW-then-fchmod, even when it needs root — `sudo chmod +// ` has no way to refuse following a symlink at path, so re-execing +// into this process is what keeps the escalated path symlink-safe. +func NewWidenSharedFileCmd(globalFlags *flags.GlobalFlags) *cobra.Command { + return &cobra.Command{ + Use: "widen-shared-file ", + Short: "Widen a coordination file's permissions if needed", + Args: cobra.ExactArgs(2), + Hidden: true, + RunE: func(_ *cobra.Command, args []string) error { + mode, err := strconv.ParseUint(args[1], 8, 32) + if err != nil { + return fmt.Errorf("parse mode %q: %w", args[1], err) + } + return sharedfile.WidenIfNeeded(args[0], os.FileMode(mode)) + }, + } +} diff --git a/cmd/internal/widen_shared_file_test.go b/cmd/internal/widen_shared_file_test.go new file mode 100644 index 000000000..719174acc --- /dev/null +++ b/cmd/internal/widen_shared_file_test.go @@ -0,0 +1,35 @@ +package cmdinternal + +import ( + "os" + "path/filepath" + "testing" + + "github.com/devsy-org/devsy/cmd/flags" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestWidenSharedFileCmd_WidensModeArgument(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, nil, 0o644)) + + cmd := NewWidenSharedFileCmd(&flags.GlobalFlags{}) + cmd.SetArgs([]string{path, "0666"}) + require.NoError(t, cmd.Execute()) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) +} + +func TestWidenSharedFileCmd_RejectsInvalidMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, nil, 0o644)) + + cmd := NewWidenSharedFileCmd(&flags.GlobalFlags{}) + cmd.SetArgs([]string{path, "not-a-mode"}) + require.Error(t, cmd.Execute()) +} diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index ed4215694..c0023a460 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -223,7 +223,10 @@ func writeResultFileTo(path string, rawBytes []byte) error { // #nosec G304 -- callers pass a fixed const path; parameterized only for tests existing, _ := os.ReadFile(path) if string(rawBytes) == string(existing) { - return nil + // Widen even when skipping the write: a stale file left at a + // restrictive mode by a pre-fix binary must still get readable by + // the other session's user, not just on the next content change. + return sharedfile.WidenWithSudoFallback(context.Background(), path, 0o644) } if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { // #nosec G301 diff --git a/pkg/devcontainer/setup/setup_test.go b/pkg/devcontainer/setup/setup_test.go index 83055171d..889318ac7 100644 --- a/pkg/devcontainer/setup/setup_test.go +++ b/pkg/devcontainer/setup/setup_test.go @@ -180,3 +180,28 @@ func TestWriteResultFileTo_WidensExistingRestrictiveMode(t *testing.T) { t.Errorf("mode = %o, want 0644 (a stale 0600 file must be widened)", got) } } + +// TestWriteResultFileTo_WidensStaleModeEvenWhenContentUnchanged guards +// against the unchanged-content early return skipping the widen step: a +// file at a stale restrictive mode must get corrected on the next call +// even if the content it's writing happens to already match. +func TestWriteResultFileTo_WidensStaleModeEvenWhenContentUnchanged(t *testing.T) { + path := filepath.Join(t.TempDir(), "result.json") + content := []byte(`{"ok":true}`) + // #nosec G306 -- intentional: simulating a pre-fix file left at 0600 + if err := os.WriteFile(path, content, 0o600); err != nil { + t.Fatalf("seed file: %v", err) + } + + if err := writeResultFileTo(path, content); err != nil { + t.Fatalf("writeResultFileTo: %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := info.Mode().Perm(); got != 0o644 { + t.Errorf("mode = %o, want 0644 even though content was already up to date", got) + } +} diff --git a/pkg/sharedfile/sharedfile.go b/pkg/sharedfile/sharedfile.go index 1b7e1fcc2..8923d8003 100644 --- a/pkg/sharedfile/sharedfile.go +++ b/pkg/sharedfile/sharedfile.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "os" + "syscall" ) // EnsureMode ensures path exists with exactly mode permissions, creating it @@ -37,18 +38,27 @@ func EnsureMode(path string, mode os.FileMode) error { } // WidenIfNeeded chmods path to mode if its current mode differs, skipping -// the chmod entirely when it's already correct. Rejects a path that -// resolves to a symlink rather than following it. +// the chmod entirely when it's already correct. Opens path with O_NOFOLLOW +// and chmods the resulting descriptor rather than the path, so a symlink +// swapped in after a check-then-chmod by path couldn't redirect the chmod +// onto an arbitrary target — path is a fixed, world-writable coordination +// file any container user could otherwise race. func WidenIfNeeded(path string, mode os.FileMode) error { - needsChmod, err := needsChmod(path, mode) + //nolint:gosec // callers intentionally widen a fixed coordination-file path + f, err := os.OpenFile(path, os.O_WRONLY|syscall.O_NOFOLLOW, 0) if err != nil { - return err + return fmt.Errorf("open %s: %w", path, err) + } + defer func() { _ = f.Close() }() + + info, err := f.Stat() + if err != nil { + return fmt.Errorf("stat %s: %w", path, err) } - if !needsChmod { + if info.Mode().Perm() == mode.Perm() { return nil } - //nolint:gosec // callers intentionally widen a fixed coordination-file path - if err := os.Chmod(path, mode); err != nil { + if err := f.Chmod(mode); err != nil { return fmt.Errorf("chmod %s: %w", path, err) } return nil @@ -68,18 +78,3 @@ func createIfMissing(path string, mode os.FileMode) error { } return f.Close() } - -// needsChmod reports whether path's current permission bits differ from -// want. Uses Lstat and rejects symlinks: os.Chmod follows them, so a -// coordination-file path replaced with a symlink could redirect a chmod -// onto an arbitrary file. -func needsChmod(path string, want os.FileMode) (bool, error) { - info, err := os.Lstat(path) - if err != nil { - return false, err - } - if info.Mode()&os.ModeSymlink != 0 { - return false, fmt.Errorf("refusing to chmod %s: path is a symlink", path) - } - return info.Mode().Perm() != want.Perm(), nil -} diff --git a/pkg/sharedfile/sharedfile_test.go b/pkg/sharedfile/sharedfile_test.go index 459ead391..ff047ffbe 100644 --- a/pkg/sharedfile/sharedfile_test.go +++ b/pkg/sharedfile/sharedfile_test.go @@ -55,55 +55,27 @@ func TestWidenIfNeeded_SkipsChmodWhenModeAlreadyCorrect(t *testing.T) { //nolint:gosec // test fixture, intentional require.NoError(t, os.Chmod(path, 0o666)) - needsChmod, err := needsChmod(path, 0o666) - require.NoError(t, err) - assert.False(t, needsChmod, - "a file already at the target mode must not need a chmod, since a "+ - "non-owning acquirer's chmod would fail with EPERM") - + // Not directly observable from outside (the whole point is it's an + // internal fast path), so this only pins the externally visible + // contract: widening an already-correct mode still succeeds. require.NoError(t, WidenIfNeeded(path, 0o666)) } -func TestNeedsChmod(t *testing.T) { - tests := []struct { - name string - mode os.FileMode - want os.FileMode - expect bool - }{ - {name: "already matches", mode: 0o666, want: 0o666, expect: false}, - {name: "narrower mode needs widening", mode: 0o644, want: 0o666, expect: true}, - {name: "wider mode needs narrowing", mode: 0o777, want: 0o666, expect: true}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - path := filepath.Join(t.TempDir(), "coord") - require.NoError(t, os.WriteFile(path, nil, tc.mode)) - //nolint:gosec // test fixture, intentional - require.NoError(t, os.Chmod(path, tc.mode)) - - got, err := needsChmod(path, tc.want) - require.NoError(t, err) - assert.Equal(t, tc.expect, got) - }) - } -} +func TestWidenIfNeeded_WidensNarrowerMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, nil, 0o644)) -func TestNeedsChmod_StatErrorPropagates(t *testing.T) { - _, err := needsChmod(filepath.Join(t.TempDir(), "does-not-exist"), 0o666) - require.Error(t, err) -} + require.NoError(t, WidenIfNeeded(path, 0o666)) -func TestNeedsChmod_RejectsSymlink(t *testing.T) { - dir := t.TempDir() - target := filepath.Join(dir, "target") - link := filepath.Join(dir, "link") - require.NoError(t, os.WriteFile(target, nil, 0o600)) - require.NoError(t, os.Symlink(target, link)) + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) +} - _, err := needsChmod(link, 0o666) +func TestWidenIfNeeded_StatErrorPropagates(t *testing.T) { + err := WidenIfNeeded(filepath.Join(t.TempDir(), "does-not-exist"), 0o666) require.Error(t, err) - assert.Contains(t, err.Error(), "symlink") } func TestWidenIfNeeded_RejectsSymlinkWithoutTouchingTarget(t *testing.T) { @@ -114,11 +86,37 @@ func TestWidenIfNeeded_RejectsSymlinkWithoutTouchingTarget(t *testing.T) { require.NoError(t, os.Symlink(target, link)) err := WidenIfNeeded(link, 0o666) - require.Error(t, err) - assert.Contains(t, err.Error(), "symlink") + require.Error(t, err, "opening link with O_NOFOLLOW must fail (ELOOP), not follow to target") info, err := os.Stat(target) require.NoError(t, err) assert.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "the symlink target's mode must be untouched, not widened") } + +// TestWidenIfNeeded_SymlinkSwappedAfterOpenCannotRedirectChmod is the +// regression test for the TOCTOU this function closes: even if path is +// replaced with a symlink after WidenIfNeeded has already opened it, the +// chmod lands on the descriptor's original inode, not wherever the symlink +// now points. +func TestWidenIfNeeded_SymlinkSwappedAfterOpenCannotRedirectChmod(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "coord") + decoyTarget := filepath.Join(dir, "decoy") + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, nil, 0o644)) + require.NoError(t, os.WriteFile(decoyTarget, nil, 0o600)) + + f, err := os.OpenFile(path, os.O_WRONLY, 0) //nolint:gosec // test-owned temp path + require.NoError(t, err) + require.NoError(t, os.Remove(path)) + require.NoError(t, os.Symlink(decoyTarget, path)) + + require.NoError(t, f.Chmod(0o666), "chmod on an already-open fd must not be redirected") + require.NoError(t, f.Close()) + + decoyInfo, err := os.Stat(decoyTarget) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), decoyInfo.Mode().Perm(), + "the symlink planted after open must not have been affected") +} diff --git a/pkg/sharedfile/sudo.go b/pkg/sharedfile/sudo.go index bfacd10b3..ac648e07b 100644 --- a/pkg/sharedfile/sudo.go +++ b/pkg/sharedfile/sudo.go @@ -12,10 +12,14 @@ import ( ) // WidenWithSudoFallback behaves like WidenIfNeeded, but on EPERM (path -// exists at the wrong mode and this process doesn't own it) falls back to a -// non-interactive `sudo chmod`. The fallback's failure is logged, not -// returned: this is a best-effort repair, and the caller's own lock -// acquisition will surface the real permission error if the repair fails. +// exists at the wrong mode and this process doesn't own it) falls back to +// re-execing ` internal widen-shared-file` under a non-interactive +// sudo, so the escalated mode change still goes through WidenIfNeeded's +// O_NOFOLLOW open rather than a plain `sudo chmod ` — chmod(1) has no +// way to refuse following a symlink at its target path. The fallback's +// failure is logged, not returned: this is a best-effort repair, and the +// caller's own lock acquisition will surface the real permission error if +// the repair fails. func WidenWithSudoFallback(ctx context.Context, path string, mode os.FileMode) error { err := WidenIfNeeded(path, mode) if err == nil { @@ -28,12 +32,21 @@ func WidenWithSudoFallback(ctx context.Context, path string, mode os.FileMode) e return err } + execPath, err := os.Executable() + if err != nil { + log.Debugf("resolve self for sudo widen fallback (non-fatal): %v", err) + return nil + } + // -n: fail immediately instead of prompting if sudo needs a password, // so a caller holding a timeout-bounded lock can't hang forever. - //nolint:gosec // path is a fixed coordination-file path, not user input - cmd := exec.CommandContext(ctx, "sudo", "-n", "chmod", fmt.Sprintf("%04o", mode.Perm()), path) + //nolint:gosec // execPath is the current binary; path is a fixed coordination-file path + cmd := exec.CommandContext( + ctx, "sudo", "-n", execPath, "internal", "widen-shared-file", + path, fmt.Sprintf("%04o", mode.Perm()), + ) if sudoErr := cmd.Run(); sudoErr != nil { - log.Debugf("sudo chmod %s (non-fatal): %v", path, sudoErr) + log.Debugf("sudo widen-shared-file %s (non-fatal): %v", path, sudoErr) } return nil } From 44826c4ae1ef10811e1009731da17234bbbe5062 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 4 Aug 2026 00:23:08 +0000 Subject: [PATCH 22/28] fix(sharedfile): split O_NOFOLLOW open behind a build tag for windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit syscall.O_NOFOLLOW is undefined on windows, breaking the devsy CLI's windows-latest cross-compile — every sharedfile caller (setup-gpg, the SSH server's activity file, the devcontainer result file) only ever runs inside the Linux container, never on a Windows host, but the CLI binary itself still needs to build there. Extracts the platform-specific open into openNoFollow, split supported/unsupported like the existing pkg/file convention. The windows stub errors if ever called, since nothing should reach it in practice. --- pkg/sharedfile/sharedfile.go | 14 ++++++-------- pkg/sharedfile/sharedfile_supported.go | 16 ++++++++++++++++ pkg/sharedfile/sharedfile_unsupported.go | 17 +++++++++++++++++ 3 files changed, 39 insertions(+), 8 deletions(-) create mode 100644 pkg/sharedfile/sharedfile_supported.go create mode 100644 pkg/sharedfile/sharedfile_unsupported.go diff --git a/pkg/sharedfile/sharedfile.go b/pkg/sharedfile/sharedfile.go index 8923d8003..215adbbb6 100644 --- a/pkg/sharedfile/sharedfile.go +++ b/pkg/sharedfile/sharedfile.go @@ -20,7 +20,6 @@ import ( "errors" "fmt" "os" - "syscall" ) // EnsureMode ensures path exists with exactly mode permissions, creating it @@ -38,14 +37,13 @@ func EnsureMode(path string, mode os.FileMode) error { } // WidenIfNeeded chmods path to mode if its current mode differs, skipping -// the chmod entirely when it's already correct. Opens path with O_NOFOLLOW -// and chmods the resulting descriptor rather than the path, so a symlink -// swapped in after a check-then-chmod by path couldn't redirect the chmod -// onto an arbitrary target — path is a fixed, world-writable coordination -// file any container user could otherwise race. +// the chmod entirely when it's already correct. Opens path without +// following a symlink and chmods the resulting descriptor rather than the +// path, so a symlink swapped in after a check-then-chmod by path couldn't +// redirect the chmod onto an arbitrary target — path is a fixed, +// world-writable coordination file any container user could otherwise race. func WidenIfNeeded(path string, mode os.FileMode) error { - //nolint:gosec // callers intentionally widen a fixed coordination-file path - f, err := os.OpenFile(path, os.O_WRONLY|syscall.O_NOFOLLOW, 0) + f, err := openNoFollow(path) if err != nil { return fmt.Errorf("open %s: %w", path, err) } diff --git a/pkg/sharedfile/sharedfile_supported.go b/pkg/sharedfile/sharedfile_supported.go new file mode 100644 index 000000000..f4c3529ac --- /dev/null +++ b/pkg/sharedfile/sharedfile_supported.go @@ -0,0 +1,16 @@ +//go:build linux || darwin || unix + +package sharedfile + +import ( + "os" + "syscall" +) + +// openNoFollow opens path for writing without following a trailing +// symlink, so the caller can chmod the resulting descriptor's inode +// regardless of what path later resolves to. +func openNoFollow(path string) (*os.File, error) { + //nolint:gosec // callers intentionally widen a fixed coordination-file path + return os.OpenFile(path, os.O_WRONLY|syscall.O_NOFOLLOW, 0) +} diff --git a/pkg/sharedfile/sharedfile_unsupported.go b/pkg/sharedfile/sharedfile_unsupported.go new file mode 100644 index 000000000..dc31c5a8c --- /dev/null +++ b/pkg/sharedfile/sharedfile_unsupported.go @@ -0,0 +1,17 @@ +//go:build windows + +package sharedfile + +import ( + "fmt" + "os" + "runtime" +) + +// openNoFollow has no symlink-safe open on Windows. Every sharedfile caller +// only ever runs inside the Linux container (setup-gpg, the SSH server's +// activity file, the devcontainer result file), never on a Windows host, so +// this exists solely to keep the devsy CLI binary itself cross-compiling. +func openNoFollow(string) (*os.File, error) { + return nil, fmt.Errorf("sharedfile: not supported on %s", runtime.GOOS) +} From b08bf041c99fa786e79e49c50f23f6b52fff3bf4 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 4 Aug 2026 00:33:33 +0000 Subject: [PATCH 23/28] fix(sharedfile): open O_RDONLY, not O_WRONLY, to check whether a chmod is needed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WidenIfNeeded's own open required write access to the file, even for the "mode already matches, skip the chmod" fast path — the exact case meant for a non-owning caller. A coordination-file mode like 0644 grants read to everyone but write only to the owner, so a non-owning process calling WidenIfNeeded on an already-correct 0644 file got a false permission error instead of the intended no-op. fchmod cares about ownership, not how the fd was opened, so O_RDONLY is sufficient — it only needs write access in the branch that's about to Chmod anyway, at which point ownership is what governs that regardless. This also meant WidenWithSudoFallback would escalate to sudo needlessly on every call against a 0644 file it doesn't own, even when the mode was already correct, since WidenIfNeeded's false EACCES looked identical to a real EPERM-from-wrong-mode to that caller. Currently masked in production because every real call site but writeResultFileTo's uses 0666 (write for everyone), so this only bit the one 0644 caller — and even there, by luck of that caller always running as the same user (root) on every invocation today. Found by manually re-auditing this package's file-open modes against the actual permission bits each caller uses, not by a failing test. --- pkg/sharedfile/sharedfile_supported.go | 11 +++++++---- pkg/sharedfile/sharedfile_test.go | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/pkg/sharedfile/sharedfile_supported.go b/pkg/sharedfile/sharedfile_supported.go index f4c3529ac..f7c0203da 100644 --- a/pkg/sharedfile/sharedfile_supported.go +++ b/pkg/sharedfile/sharedfile_supported.go @@ -7,10 +7,13 @@ import ( "syscall" ) -// openNoFollow opens path for writing without following a trailing -// symlink, so the caller can chmod the resulting descriptor's inode -// regardless of what path later resolves to. +// openNoFollow opens path without following a trailing symlink, so the +// caller can chmod the resulting descriptor's inode regardless of what +// path later resolves to. Opens O_RDONLY, not O_WRONLY: fchmod only cares +// about ownership, not how the fd was opened, and a coordination-file mode +// (e.g. 0644, 0666) always grants read to the "already correct, skip the +// chmod" caller even when it doesn't grant that caller write. func openNoFollow(path string) (*os.File, error) { //nolint:gosec // callers intentionally widen a fixed coordination-file path - return os.OpenFile(path, os.O_WRONLY|syscall.O_NOFOLLOW, 0) + return os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW, 0) } diff --git a/pkg/sharedfile/sharedfile_test.go b/pkg/sharedfile/sharedfile_test.go index ff047ffbe..ff6ebeb0d 100644 --- a/pkg/sharedfile/sharedfile_test.go +++ b/pkg/sharedfile/sharedfile_test.go @@ -61,6 +61,25 @@ func TestWidenIfNeeded_SkipsChmodWhenModeAlreadyCorrect(t *testing.T) { require.NoError(t, WidenIfNeeded(path, 0o666)) } +// TestWidenIfNeeded_SucceedsOnAlreadyCorrectModeWithoutWriteAccess is the +// regression test for a bug where openNoFollow used O_WRONLY: confirming an +// already-correct target mode must not itself require write access, since +// the whole point of "already correct" is a non-owning caller (e.g. +// coordination-file mode 0644, checked by a session that isn't the file's +// owner) skipping a chmod it couldn't perform anyway. 0444 stands in for +// "correct mode that doesn't grant this process write" without needing a +// real cross-UID setup. +func TestWidenIfNeeded_SucceedsOnAlreadyCorrectModeWithoutWriteAccess(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, nil, 0o444)) + //nolint:gosec // test fixture, intentional + require.NoError(t, os.Chmod(path, 0o444)) + + require.NoError(t, WidenIfNeeded(path, 0o444), + "confirming an already-correct mode must not require write access to the file") +} + func TestWidenIfNeeded_WidensNarrowerMode(t *testing.T) { path := filepath.Join(t.TempDir(), "coord") //nolint:gosec // test fixture, intentional From ec2eb1b301000e86db19770964303572a1260773 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 4 Aug 2026 00:41:48 +0000 Subject: [PATCH 24/28] fix(sharedfile): reject a FIFO at the coordination path instead of hanging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit correctly flagged that openNoFollow's open could block forever: a FIFO planted at the coordination-file path with no writer hangs an O_RDONLY open indefinitely, letting any container user with write access to the parent directory (/tmp) wedge every future caller of acquireGPGSetupLock/ensureActivityFile/etc. openNoFollow now also passes O_NONBLOCK, which makes opening a FIFO return immediately instead of waiting for a writer. WidenIfNeeded then rejects any non-regular file via IsRegular() before comparing modes or chmodding — O_NONBLOCK only keeps the open itself from hanging, it doesn't stop a FIFO fd from being returned. Verified: a regression test creates a real FIFO at the coordination path and confirms WidenIfNeeded returns an error within 2s instead of hanging; fails against the pre-fix code (confirmed by temporarily reverting O_NONBLOCK), passes with it. --- pkg/sharedfile/sharedfile.go | 5 +++ pkg/sharedfile/sharedfile_supported.go | 7 +++-- pkg/sharedfile/sharedfile_supported_test.go | 35 +++++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 pkg/sharedfile/sharedfile_supported_test.go diff --git a/pkg/sharedfile/sharedfile.go b/pkg/sharedfile/sharedfile.go index 215adbbb6..72899f0d3 100644 --- a/pkg/sharedfile/sharedfile.go +++ b/pkg/sharedfile/sharedfile.go @@ -42,6 +42,8 @@ func EnsureMode(path string, mode os.FileMode) error { // path, so a symlink swapped in after a check-then-chmod by path couldn't // redirect the chmod onto an arbitrary target — path is a fixed, // world-writable coordination file any container user could otherwise race. +// Also rejects a FIFO or other non-regular file at path: opening a FIFO +// with no writer would otherwise block this call forever. func WidenIfNeeded(path string, mode os.FileMode) error { f, err := openNoFollow(path) if err != nil { @@ -53,6 +55,9 @@ func WidenIfNeeded(path string, mode os.FileMode) error { if err != nil { return fmt.Errorf("stat %s: %w", path, err) } + if !info.Mode().IsRegular() { + return fmt.Errorf("refusing to chmod %s: not a regular file (mode %s)", path, info.Mode()) + } if info.Mode().Perm() == mode.Perm() { return nil } diff --git a/pkg/sharedfile/sharedfile_supported.go b/pkg/sharedfile/sharedfile_supported.go index f7c0203da..2939b2e13 100644 --- a/pkg/sharedfile/sharedfile_supported.go +++ b/pkg/sharedfile/sharedfile_supported.go @@ -12,8 +12,11 @@ import ( // path later resolves to. Opens O_RDONLY, not O_WRONLY: fchmod only cares // about ownership, not how the fd was opened, and a coordination-file mode // (e.g. 0644, 0666) always grants read to the "already correct, skip the -// chmod" caller even when it doesn't grant that caller write. +// chmod" caller even when it doesn't grant that caller write. Also passes +// O_NONBLOCK: opening a FIFO planted at path would otherwise block forever +// waiting for a writer — the caller must still reject non-regular files +// after Stat, since O_NONBLOCK only prevents the open itself from hanging. func openNoFollow(path string) (*os.File, error) { //nolint:gosec // callers intentionally widen a fixed coordination-file path - return os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW, 0) + return os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) } diff --git a/pkg/sharedfile/sharedfile_supported_test.go b/pkg/sharedfile/sharedfile_supported_test.go new file mode 100644 index 000000000..7c5df08c5 --- /dev/null +++ b/pkg/sharedfile/sharedfile_supported_test.go @@ -0,0 +1,35 @@ +//go:build linux || darwin || unix + +package sharedfile + +import ( + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestWidenIfNeeded_RejectsFIFOWithoutBlocking is the regression test for +// CodeRabbit's finding: opening a FIFO planted at path with no writer +// blocks forever without O_NONBLOCK, letting any container user with +// write access to the coordination file's directory hang every future +// caller. WidenIfNeeded must return promptly with an error instead. +func TestWidenIfNeeded_RejectsFIFOWithoutBlocking(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + require.NoError(t, syscall.Mkfifo(path, 0o666)) + + done := make(chan error, 1) + go func() { done <- WidenIfNeeded(path, 0o666) }() + + select { + case err := <-done: + require.Error(t, err, + "a FIFO at the coordination path must be rejected, not silently accepted") + assert.Contains(t, err.Error(), "not a regular file") + case <-time.After(2 * time.Second): + t.Fatal("WidenIfNeeded blocked for 2s+ opening a FIFO with no writer") + } +} From 5eb4db170eb6da4637d00eae936d10a21ebec70d Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 19:52:12 -0500 Subject: [PATCH 25/28] style: update comments --- cmd/internal/agentworkspace/setup_gpg.go | 2 +- cmd/internal/fleet_server.go | 2 +- cmd/internal/widen_shared_file.go | 2 +- pkg/devcontainer/setup/setup_test.go | 15 ---------- pkg/sharedfile/sharedfile.go | 16 ++++------- pkg/sharedfile/sharedfile_test.go | 35 ++++-------------------- pkg/sharedfile/sharedfile_unsupported.go | 5 +--- pkg/sharedfile/sudo.go | 4 +-- pkg/tunnel/browser.go | 4 +-- 9 files changed, 19 insertions(+), 66 deletions(-) diff --git a/cmd/internal/agentworkspace/setup_gpg.go b/cmd/internal/agentworkspace/setup_gpg.go index 676183047..4083dd05d 100644 --- a/cmd/internal/agentworkspace/setup_gpg.go +++ b/cmd/internal/agentworkspace/setup_gpg.go @@ -99,7 +99,7 @@ func (cmd *SetupGPGCmd) Run(ctx context.Context) error { } // gpgSetupLockMode is 0666 — flock's default 0600 would lock out whichever -// of root/remoteUser didn't create the file. See pkg/sharedfile. +// of root/remoteUser did not create the file. const gpgSetupLockMode = 0o666 // acquireGPGSetupLock takes the cross-process lock guarding setup-gpg. On diff --git a/cmd/internal/fleet_server.go b/cmd/internal/fleet_server.go index f579aa2b4..3370f74f9 100644 --- a/cmd/internal/fleet_server.go +++ b/cmd/internal/fleet_server.go @@ -61,7 +61,7 @@ func (c *FleetServerCmd) Run(cmd *cobra.Command, _ []string) error { } // check if we had at least one fleet client connection, before - // this point, we don't check for connected/disconnected strings + // this point, we do not check for connected/disconnected strings initialized := firstConnection.FindStringSubmatch(string(log)) if len(initialized) == 0 { continue diff --git a/cmd/internal/widen_shared_file.go b/cmd/internal/widen_shared_file.go index b1220f62e..09dfa7bbb 100644 --- a/cmd/internal/widen_shared_file.go +++ b/cmd/internal/widen_shared_file.go @@ -10,7 +10,7 @@ import ( "github.com/spf13/cobra" ) -// NewWidenSharedFileCmd returns a hidden command that runs +// NewWidenSharedFileCmd returns a internal command that runs // sharedfile.WidenIfNeeded. sharedfile.WidenWithSudoFallback re-execs this // (via sudo) so the actual mode change still goes through WidenIfNeeded's // open-with-O_NOFOLLOW-then-fchmod, even when it needs root — `sudo chmod diff --git a/pkg/devcontainer/setup/setup_test.go b/pkg/devcontainer/setup/setup_test.go index 889318ac7..2fd3de80f 100644 --- a/pkg/devcontainer/setup/setup_test.go +++ b/pkg/devcontainer/setup/setup_test.go @@ -106,11 +106,6 @@ func TestSetupKubeConfig_NonEmptyPayloadEmitsInfoLog(t *testing.T) { } } -// TestWriteResultFileTo_ProducesWorldReadableFile guards the fix for a -// SSH-session-user footgun: getContainerResult and portOptionsFromResult -// read this file over sessions that may authenticate as root or the -// workspace's remoteUser, so it must not be locked to whichever user's -// process happened to create it first. func TestWriteResultFileTo_ProducesWorldReadableFile(t *testing.T) { path := filepath.Join(t.TempDir(), "nested", "result.json") @@ -127,9 +122,6 @@ func TestWriteResultFileTo_ProducesWorldReadableFile(t *testing.T) { } } -// TestWriteResultFileTo_SkipsWriteWhenContentUnchanged locks in the -// no-op-on-unchanged-content behavior so frequent writeResultFile calls -// during setup don't repeatedly touch the file's mtime/mode for no reason. func TestWriteResultFileTo_SkipsWriteWhenContentUnchanged(t *testing.T) { path := filepath.Join(t.TempDir(), "result.json") content := []byte(`{"ok":true}`) @@ -158,9 +150,6 @@ func TestWriteResultFileTo_SkipsWriteWhenContentUnchanged(t *testing.T) { } } -// TestWriteResultFileTo_WidensExistingRestrictiveMode ensures a file left -// behind by a pre-fix binary (0600) gets corrected on the next write, not -// just newly-created files. func TestWriteResultFileTo_WidensExistingRestrictiveMode(t *testing.T) { path := filepath.Join(t.TempDir(), "result.json") // #nosec G306 -- intentional: simulating a pre-fix file left at 0600 @@ -181,10 +170,6 @@ func TestWriteResultFileTo_WidensExistingRestrictiveMode(t *testing.T) { } } -// TestWriteResultFileTo_WidensStaleModeEvenWhenContentUnchanged guards -// against the unchanged-content early return skipping the widen step: a -// file at a stale restrictive mode must get corrected on the next call -// even if the content it's writing happens to already match. func TestWriteResultFileTo_WidensStaleModeEvenWhenContentUnchanged(t *testing.T) { path := filepath.Join(t.TempDir(), "result.json") content := []byte(`{"ok":true}`) diff --git a/pkg/sharedfile/sharedfile.go b/pkg/sharedfile/sharedfile.go index 72899f0d3..def92ae37 100644 --- a/pkg/sharedfile/sharedfile.go +++ b/pkg/sharedfile/sharedfile.go @@ -24,7 +24,7 @@ import ( // EnsureMode ensures path exists with exactly mode permissions, creating it // if absent. Skips the chmod when the file's mode already matches: chmod -// requires ownership (or root) even when the requested mode wouldn't +// requires ownership (or root) even when the requested mode would not // change, so skipping it when unnecessary avoids EPERM for a non-owning // acquirer of an already-correctly-moded file. // @@ -37,13 +37,10 @@ func EnsureMode(path string, mode os.FileMode) error { } // WidenIfNeeded chmods path to mode if its current mode differs, skipping -// the chmod entirely when it's already correct. Opens path without +// the chmod entirely when it is already correct. Opens path without // following a symlink and chmods the resulting descriptor rather than the -// path, so a symlink swapped in after a check-then-chmod by path couldn't -// redirect the chmod onto an arbitrary target — path is a fixed, -// world-writable coordination file any container user could otherwise race. -// Also rejects a FIFO or other non-regular file at path: opening a FIFO -// with no writer would otherwise block this call forever. +// path, so a symlink swapped in after a check-then-chmod by path could not +// redirect the chmod onto an arbitrary target. func WidenIfNeeded(path string, mode os.FileMode) error { f, err := openNoFollow(path) if err != nil { @@ -67,9 +64,8 @@ func WidenIfNeeded(path string, mode os.FileMode) error { return nil } -// createIfMissing creates path at mode if it doesn't already exist. Leaves -// an existing file untouched — its mode is WidenIfNeeded's job — so this -// never races a concurrent creator into truncating their write. +// createIfMissing creates path at mode if it does not already exist. Leaves +// an existing file untouched. func createIfMissing(path string, mode os.FileMode) error { //nolint:gosec // callers intentionally create a fixed, world-accessible coordination file f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, mode) diff --git a/pkg/sharedfile/sharedfile_test.go b/pkg/sharedfile/sharedfile_test.go index ff6ebeb0d..f0ca2f44f 100644 --- a/pkg/sharedfile/sharedfile_test.go +++ b/pkg/sharedfile/sharedfile_test.go @@ -28,8 +28,7 @@ func TestEnsureMode_WidensExistingRestrictiveFile(t *testing.T) { info, err := os.Stat(path) require.NoError(t, err) assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) - //nolint:gosec // test-owned temp path - content, err := os.ReadFile(path) + content, err := os.ReadFile(path) //nolint:gosec // test-owned temp path require.NoError(t, err) assert.Equal(t, "hello", string(content), "widening mode must not touch existing content") } @@ -41,8 +40,7 @@ func TestCreateIfMissing_LeavesExistingFileUntouched(t *testing.T) { require.NoError(t, createIfMissing(path, 0o666)) - //nolint:gosec // test-owned temp path - content, err := os.ReadFile(path) + content, err := os.ReadFile(path) //nolint:gosec // test-owned temp path require.NoError(t, err) assert.Equal(t, "existing", string(content), "createIfMissing must not truncate a file a concurrent creator just wrote") @@ -50,25 +48,12 @@ func TestCreateIfMissing_LeavesExistingFileUntouched(t *testing.T) { func TestWidenIfNeeded_SkipsChmodWhenModeAlreadyCorrect(t *testing.T) { path := filepath.Join(t.TempDir(), "coord") - //nolint:gosec // test fixture, intentional - require.NoError(t, os.WriteFile(path, nil, 0o666)) - //nolint:gosec // test fixture, intentional - require.NoError(t, os.Chmod(path, 0o666)) + require.NoError(t, os.WriteFile(path, nil, 0o666)) //nolint:gosec + require.NoError(t, os.Chmod(path, 0o666)) //nolint:gosec - // Not directly observable from outside (the whole point is it's an - // internal fast path), so this only pins the externally visible - // contract: widening an already-correct mode still succeeds. require.NoError(t, WidenIfNeeded(path, 0o666)) } -// TestWidenIfNeeded_SucceedsOnAlreadyCorrectModeWithoutWriteAccess is the -// regression test for a bug where openNoFollow used O_WRONLY: confirming an -// already-correct target mode must not itself require write access, since -// the whole point of "already correct" is a non-owning caller (e.g. -// coordination-file mode 0644, checked by a session that isn't the file's -// owner) skipping a chmod it couldn't perform anyway. 0444 stands in for -// "correct mode that doesn't grant this process write" without needing a -// real cross-UID setup. func TestWidenIfNeeded_SucceedsOnAlreadyCorrectModeWithoutWriteAccess(t *testing.T) { path := filepath.Join(t.TempDir(), "coord") //nolint:gosec // test fixture, intentional @@ -82,9 +67,7 @@ func TestWidenIfNeeded_SucceedsOnAlreadyCorrectModeWithoutWriteAccess(t *testing func TestWidenIfNeeded_WidensNarrowerMode(t *testing.T) { path := filepath.Join(t.TempDir(), "coord") - //nolint:gosec // test fixture, intentional - require.NoError(t, os.WriteFile(path, nil, 0o644)) - + require.NoError(t, os.WriteFile(path, nil, 0o644)) //nolint:gosec require.NoError(t, WidenIfNeeded(path, 0o666)) info, err := os.Stat(path) @@ -113,17 +96,11 @@ func TestWidenIfNeeded_RejectsSymlinkWithoutTouchingTarget(t *testing.T) { "the symlink target's mode must be untouched, not widened") } -// TestWidenIfNeeded_SymlinkSwappedAfterOpenCannotRedirectChmod is the -// regression test for the TOCTOU this function closes: even if path is -// replaced with a symlink after WidenIfNeeded has already opened it, the -// chmod lands on the descriptor's original inode, not wherever the symlink -// now points. func TestWidenIfNeeded_SymlinkSwappedAfterOpenCannotRedirectChmod(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "coord") decoyTarget := filepath.Join(dir, "decoy") - //nolint:gosec // test fixture, intentional - require.NoError(t, os.WriteFile(path, nil, 0o644)) + require.NoError(t, os.WriteFile(path, nil, 0o644)) //nolint:gosec require.NoError(t, os.WriteFile(decoyTarget, nil, 0o600)) f, err := os.OpenFile(path, os.O_WRONLY, 0) //nolint:gosec // test-owned temp path diff --git a/pkg/sharedfile/sharedfile_unsupported.go b/pkg/sharedfile/sharedfile_unsupported.go index dc31c5a8c..4101ab5fc 100644 --- a/pkg/sharedfile/sharedfile_unsupported.go +++ b/pkg/sharedfile/sharedfile_unsupported.go @@ -8,10 +8,7 @@ import ( "runtime" ) -// openNoFollow has no symlink-safe open on Windows. Every sharedfile caller -// only ever runs inside the Linux container (setup-gpg, the SSH server's -// activity file, the devcontainer result file), never on a Windows host, so -// this exists solely to keep the devsy CLI binary itself cross-compiling. +// openNoFollow has no symlink-safe open on Windows. func openNoFollow(string) (*os.File, error) { return nil, fmt.Errorf("sharedfile: not supported on %s", runtime.GOOS) } diff --git a/pkg/sharedfile/sudo.go b/pkg/sharedfile/sudo.go index ac648e07b..d2e3716d2 100644 --- a/pkg/sharedfile/sudo.go +++ b/pkg/sharedfile/sudo.go @@ -12,7 +12,7 @@ import ( ) // WidenWithSudoFallback behaves like WidenIfNeeded, but on EPERM (path -// exists at the wrong mode and this process doesn't own it) falls back to +// exists at the wrong mode and this process does not own it) falls back to // re-execing ` internal widen-shared-file` under a non-interactive // sudo, so the escalated mode change still goes through WidenIfNeeded's // O_NOFOLLOW open rather than a plain `sudo chmod ` — chmod(1) has no @@ -39,7 +39,7 @@ func WidenWithSudoFallback(ctx context.Context, path string, mode os.FileMode) e } // -n: fail immediately instead of prompting if sudo needs a password, - // so a caller holding a timeout-bounded lock can't hang forever. + // so a caller holding a timeout-bounded lock cannot hang forever. //nolint:gosec // execPath is the current binary; path is a fixed coordination-file path cmd := exec.CommandContext( ctx, "sudo", "-n", execPath, "internal", "widen-shared-file", diff --git a/pkg/tunnel/browser.go b/pkg/tunnel/browser.go index 05ad5a96a..6d7743487 100644 --- a/pkg/tunnel/browser.go +++ b/pkg/tunnel/browser.go @@ -244,9 +244,7 @@ func isTransientBackhaulErr(err error) bool { // CreateSSHCommand builds an exec.Cmd that runs `devsy ssh` with the given // arguments. user both authenticates the session and is who every command -// on it runs as — there's no separate privilege-drop step — so callers -// whose later traffic needs specific file access must pick user -// accordingly. Empty defaults to root. +// on it runs as. Empty defaults to root. func CreateSSHCommand( ctx context.Context, client client2.BaseWorkspaceClient, From 26b53f56f6c115e89d66d1a4a28db34a8d2a5fda Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 3 Aug 2026 20:01:44 -0500 Subject: [PATCH 26/28] style: update comments --- pkg/sharedfile/sharedfile_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/sharedfile/sharedfile_test.go b/pkg/sharedfile/sharedfile_test.go index f0ca2f44f..e8c6d0dfa 100644 --- a/pkg/sharedfile/sharedfile_test.go +++ b/pkg/sharedfile/sharedfile_test.go @@ -48,8 +48,10 @@ func TestCreateIfMissing_LeavesExistingFileUntouched(t *testing.T) { func TestWidenIfNeeded_SkipsChmodWhenModeAlreadyCorrect(t *testing.T) { path := filepath.Join(t.TempDir(), "coord") - require.NoError(t, os.WriteFile(path, nil, 0o666)) //nolint:gosec - require.NoError(t, os.Chmod(path, 0o666)) //nolint:gosec + //nolint:gosec + require.NoError(t, os.WriteFile(path, nil, 0o666)) + //nolint:gosec + require.NoError(t, os.Chmod(path, 0o666)) require.NoError(t, WidenIfNeeded(path, 0o666)) } From 7da5ffaa2a3f32998b54d08e3bfd618d3b35e38f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 4 Aug 2026 02:04:57 +0000 Subject: [PATCH 27/28] test/fix: cover stale-mode widening, route writeResultFileTo through sharedfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two legitimate CodeRabbit findings from this session's third review pass: 1. TestTouchActivityFile_UpdatesMtimeOfExistingFile seeded its fixture already at 0666, so it could never have caught touchActivityFile regressing the "widen a stale restrictive mode" behavior. Added the missing mode assertion to ensureActivityFile's own existing-file test instead, since that function is what actually owns the widening. 2. writeResultFileTo did its own raw os.ReadFile/os.WriteFile rather than routing through sharedfile's no-follow/non-blocking primitives, unlike every other coordination-file writer in this codebase. Not currently exploitable (DevContainerResultPath's parent dir is 0755, owner-only — a non-root container user can't plant a symlink or FIFO there today), but inconsistent defense-in-depth for a fixed, predictable path. Adds sharedfile.ReadFile/WriteFile as symlink/FIFO-safe counterparts to os.ReadFile/os.WriteFile, sharing the same openNoFollow primitive WidenIfNeeded already used (now parameterized on flag and create mode so all four functions — EnsureMode, WidenIfNeeded, ReadFile, WriteFile — go through one hardened open). writeResultFileTo now calls these instead of the stdlib versions. Verified against real Docker containers: the full "browser IDE returns instead of blocking" Ordered container (7 specs) passes with this rewiring in place, confirming devsy up's result-file read/write path still works end-to-end. --- cmd/internal/ssh_server_test.go | 5 ++ pkg/devcontainer/setup/setup.go | 12 ++-- pkg/devcontainer/setup/setup_unix_test.go | 64 ++++++++++++++++++++ pkg/sharedfile/sharedfile.go | 65 ++++++++++++++++++-- pkg/sharedfile/sharedfile_supported.go | 22 +++---- pkg/sharedfile/sharedfile_supported_test.go | 33 ++++++++++ pkg/sharedfile/sharedfile_test.go | 67 +++++++++++++++++++++ pkg/sharedfile/sharedfile_unsupported.go | 2 +- 8 files changed, 247 insertions(+), 23 deletions(-) create mode 100644 pkg/devcontainer/setup/setup_unix_test.go diff --git a/cmd/internal/ssh_server_test.go b/cmd/internal/ssh_server_test.go index 573bf6201..0dd4b5a94 100644 --- a/cmd/internal/ssh_server_test.go +++ b/cmd/internal/ssh_server_test.go @@ -209,6 +209,11 @@ func TestEnsureActivityFile_NoOpsWhenFileAlreadyExists(t *testing.T) { require.NoError(t, err) assert.Equal(t, "existing", string(data), "ensureActivityFile must not truncate a file that already exists") + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm(), + "a stale restrictive mode left by an existing file must still get widened") } func TestTouchActivityFile_CreatesFileAndUpdatesMtime(t *testing.T) { diff --git a/pkg/devcontainer/setup/setup.go b/pkg/devcontainer/setup/setup.go index c0023a460..3878e7044 100644 --- a/pkg/devcontainer/setup/setup.go +++ b/pkg/devcontainer/setup/setup.go @@ -219,9 +219,12 @@ func writeResultFile(cfg *ContainerSetupConfig) { // writeResultFileTo writes rawBytes to path at 0644: readable by any // container user, not just root, since getContainerResult and // portOptionsFromResult read it over sessions authenticated as either. +// Goes through sharedfile rather than raw os.ReadFile/os.WriteFile so a +// symlink or FIFO planted at this fixed, predictable path is rejected +// rather than followed or hung on, matching the coordination files this +// package's other callers protect the same way. func writeResultFileTo(path string, rawBytes []byte) error { - // #nosec G304 -- callers pass a fixed const path; parameterized only for tests - existing, _ := os.ReadFile(path) + existing, _ := sharedfile.ReadFile(path) if string(rawBytes) == string(existing) { // Widen even when skipping the write: a stale file left at a // restrictive mode by a pre-fix binary must still get readable by @@ -232,10 +235,7 @@ func writeResultFileTo(path string, rawBytes []byte) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { // #nosec G301 return fmt.Errorf("create %s: %w", filepath.Dir(path), err) } - if err := os.WriteFile(path, rawBytes, 0o644); err != nil { //nolint:gosec // see doc comment - return fmt.Errorf("write: %w", err) - } - return sharedfile.WidenIfNeeded(path, 0o644) + return sharedfile.WriteFile(path, rawBytes, 0o644) } func setupWorkspaceOwnership(cfg *ContainerSetupConfig) error { diff --git a/pkg/devcontainer/setup/setup_unix_test.go b/pkg/devcontainer/setup/setup_unix_test.go new file mode 100644 index 000000000..9bf13b158 --- /dev/null +++ b/pkg/devcontainer/setup/setup_unix_test.go @@ -0,0 +1,64 @@ +//go:build linux || darwin || unix + +package setup + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +// TestWriteResultFileTo_RejectsSymlinkWithoutFollowing guards against a +// symlink planted at DevContainerResultPath's fixed, predictable path +// redirecting the write onto an arbitrary target — the same class of +// attack pkg/sharedfile's other callers (the GPG lock, the activity file) +// already defend against. +func TestWriteResultFileTo_RejectsSymlinkWithoutFollowing(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "result.json") + decoyTarget := filepath.Join(dir, "decoy") + if err := os.WriteFile(decoyTarget, []byte("untouched"), 0o600); err != nil { + t.Fatalf("seed decoy: %v", err) + } + if err := os.Symlink(decoyTarget, path); err != nil { + t.Fatalf("symlink: %v", err) + } + + if err := writeResultFileTo(path, []byte(`{"ok":true}`)); err == nil { + t.Fatal("expected writeResultFileTo to reject a symlinked path, got nil error") + } + + //nolint:gosec // test-owned temp path + content, err := os.ReadFile(decoyTarget) + if err != nil { + t.Fatalf("stat decoy: %v", err) + } + if string(content) != "untouched" { + t.Errorf("decoy content = %q, want unchanged %q", content, "untouched") + } +} + +// TestWriteResultFileTo_RejectsFIFOWithoutBlocking guards against a FIFO +// planted at the result-file path hanging the container's own setup phase +// forever: opening a FIFO for read/write with no matching peer blocks +// indefinitely without O_NONBLOCK. +func TestWriteResultFileTo_RejectsFIFOWithoutBlocking(t *testing.T) { + path := filepath.Join(t.TempDir(), "result.json") + if err := syscall.Mkfifo(path, 0o666); err != nil { + t.Fatalf("mkfifo: %v", err) + } + + done := make(chan error, 1) + go func() { done <- writeResultFileTo(path, []byte(`{"ok":true}`)) }() + + select { + case err := <-done: + if err == nil { + t.Fatal("expected writeResultFileTo to reject a FIFO, got nil error") + } + case <-time.After(2 * time.Second): + t.Fatal("writeResultFileTo blocked for 2s+ opening a FIFO with no peer") + } +} diff --git a/pkg/sharedfile/sharedfile.go b/pkg/sharedfile/sharedfile.go index def92ae37..f284d91c7 100644 --- a/pkg/sharedfile/sharedfile.go +++ b/pkg/sharedfile/sharedfile.go @@ -19,6 +19,7 @@ package sharedfile import ( "errors" "fmt" + "io" "os" ) @@ -42,9 +43,9 @@ func EnsureMode(path string, mode os.FileMode) error { // path, so a symlink swapped in after a check-then-chmod by path could not // redirect the chmod onto an arbitrary target. func WidenIfNeeded(path string, mode os.FileMode) error { - f, err := openNoFollow(path) + f, err := openNoFollowRegular(path, os.O_RDONLY, 0) if err != nil { - return fmt.Errorf("open %s: %w", path, err) + return err } defer func() { _ = f.Close() }() @@ -52,9 +53,6 @@ func WidenIfNeeded(path string, mode os.FileMode) error { if err != nil { return fmt.Errorf("stat %s: %w", path, err) } - if !info.Mode().IsRegular() { - return fmt.Errorf("refusing to chmod %s: not a regular file (mode %s)", path, info.Mode()) - } if info.Mode().Perm() == mode.Perm() { return nil } @@ -64,6 +62,63 @@ func WidenIfNeeded(path string, mode os.FileMode) error { return nil } +// ReadFile reads path the same way os.ReadFile does, but refuses to follow +// a symlink or block on a FIFO planted at path. +func ReadFile(path string) ([]byte, error) { + f, err := openNoFollowRegular(path, os.O_RDONLY, 0) + if err != nil { + return nil, err + } + defer func() { _ = f.Close() }() + return io.ReadAll(f) +} + +// WriteFile writes data to path at mode the same way os.WriteFile does, +// creating path if absent, but refuses to follow a symlink or block on a +// FIFO already at path. O_NOFOLLOW still applies when O_CREATE is also +// set: an existing symlink is rejected rather than followed, while a +// genuinely missing path is created as a fresh regular file. +func WriteFile(path string, data []byte, mode os.FileMode) error { + f, err := openNoFollowRegular(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) + if err != nil { + return err + } + defer func() { _ = f.Close() }() + if _, err := f.Write(data); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + if info, statErr := f.Stat(); statErr == nil && info.Mode().Perm() != mode.Perm() { + if err := f.Chmod(mode); err != nil { + return fmt.Errorf("chmod %s: %w", path, err) + } + } + return nil +} + +// openNoFollowRegular opens path with flag (plus the no-follow/non-blocking +// guards openNoFollow always adds) and rejects the result if it is not a +// regular file — a FIFO would otherwise pass the open (O_NONBLOCK just +// keeps that from hanging) and reach a caller expecting file content. +// createMode is only used when flag includes os.O_CREATE. +func openNoFollowRegular(path string, flag int, createMode os.FileMode) (*os.File, error) { + f, err := openNoFollow(path, flag, createMode) + if err != nil { + return nil, fmt.Errorf("open %s: %w", path, err) + } + info, err := f.Stat() + if err != nil { + _ = f.Close() + return nil, fmt.Errorf("stat %s: %w", path, err) + } + if !info.Mode().IsRegular() { + _ = f.Close() + return nil, fmt.Errorf( + "refusing to use %s: not a regular file (mode %s)", path, info.Mode(), + ) + } + return f, nil +} + // createIfMissing creates path at mode if it does not already exist. Leaves // an existing file untouched. func createIfMissing(path string, mode os.FileMode) error { diff --git a/pkg/sharedfile/sharedfile_supported.go b/pkg/sharedfile/sharedfile_supported.go index 2939b2e13..0cef4a44d 100644 --- a/pkg/sharedfile/sharedfile_supported.go +++ b/pkg/sharedfile/sharedfile_supported.go @@ -8,15 +8,15 @@ import ( ) // openNoFollow opens path without following a trailing symlink, so the -// caller can chmod the resulting descriptor's inode regardless of what -// path later resolves to. Opens O_RDONLY, not O_WRONLY: fchmod only cares -// about ownership, not how the fd was opened, and a coordination-file mode -// (e.g. 0644, 0666) always grants read to the "already correct, skip the -// chmod" caller even when it doesn't grant that caller write. Also passes -// O_NONBLOCK: opening a FIFO planted at path would otherwise block forever -// waiting for a writer — the caller must still reject non-regular files -// after Stat, since O_NONBLOCK only prevents the open itself from hanging. -func openNoFollow(path string) (*os.File, error) { - //nolint:gosec // callers intentionally widen a fixed coordination-file path - return os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) +// caller can chmod/read/write the resulting descriptor's inode regardless +// of what path later resolves to. createMode is used only when flag +// includes os.O_CREATE. This always adds O_NOFOLLOW and O_NONBLOCK — the +// latter so a FIFO planted at path fails the open immediately +// (ENXIO/EAGAIN) instead of blocking forever waiting for a peer. The +// caller must still reject non-regular files after Stat: O_NONBLOCK only +// keeps the open from hanging, it does not stop a FIFO fd from being +// returned. +func openNoFollow(path string, flag int, createMode os.FileMode) (*os.File, error) { + //nolint:gosec // callers intentionally open a fixed coordination-file path + return os.OpenFile(path, flag|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, createMode) } diff --git a/pkg/sharedfile/sharedfile_supported_test.go b/pkg/sharedfile/sharedfile_supported_test.go index 7c5df08c5..48e7a5086 100644 --- a/pkg/sharedfile/sharedfile_supported_test.go +++ b/pkg/sharedfile/sharedfile_supported_test.go @@ -33,3 +33,36 @@ func TestWidenIfNeeded_RejectsFIFOWithoutBlocking(t *testing.T) { t.Fatal("WidenIfNeeded blocked for 2s+ opening a FIFO with no writer") } } + +func TestReadFile_RejectsFIFOWithoutBlocking(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + require.NoError(t, syscall.Mkfifo(path, 0o666)) + + done := make(chan error, 1) + go func() { + _, err := ReadFile(path) + done <- err + }() + + select { + case err := <-done: + require.Error(t, err) + case <-time.After(2 * time.Second): + t.Fatal("ReadFile blocked for 2s+ opening a FIFO with no writer") + } +} + +func TestWriteFile_RejectsFIFOWithoutBlocking(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + require.NoError(t, syscall.Mkfifo(path, 0o666)) + + done := make(chan error, 1) + go func() { done <- WriteFile(path, []byte("x"), 0o644) }() + + select { + case err := <-done: + require.Error(t, err) + case <-time.After(2 * time.Second): + t.Fatal("WriteFile blocked for 2s+ opening a FIFO with no reader") + } +} diff --git a/pkg/sharedfile/sharedfile_test.go b/pkg/sharedfile/sharedfile_test.go index e8c6d0dfa..15b7ed911 100644 --- a/pkg/sharedfile/sharedfile_test.go +++ b/pkg/sharedfile/sharedfile_test.go @@ -118,3 +118,70 @@ func TestWidenIfNeeded_SymlinkSwappedAfterOpenCannotRedirectChmod(t *testing.T) assert.Equal(t, os.FileMode(0o600), decoyInfo.Mode().Perm(), "the symlink planted after open must not have been affected") } + +func TestReadFile_ReadsExistingContent(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + //nolint:gosec // test fixture, intentional + require.NoError(t, os.WriteFile(path, []byte("hello"), 0o644)) + + got, err := ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "hello", string(got)) +} + +func TestReadFile_RejectsSymlink(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + link := filepath.Join(dir, "link") + require.NoError(t, os.WriteFile(target, []byte("secret"), 0o600)) + require.NoError(t, os.Symlink(target, link)) + + _, err := ReadFile(link) + require.Error(t, err) +} + +func TestWriteFile_CreatesNewFileAtMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + + require.NoError(t, WriteFile(path, []byte("hello"), 0o644)) + + //nolint:gosec // test-owned temp path + content, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "hello", string(content)) + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o644), info.Mode().Perm()) +} + +func TestWriteFile_OverwritesExistingContentAndWidensMode(t *testing.T) { + path := filepath.Join(t.TempDir(), "coord") + // #nosec G306 -- test fixture, intentional + require.NoError(t, os.WriteFile(path, []byte("old"), 0o600)) + + require.NoError(t, WriteFile(path, []byte("new"), 0o644)) + + //nolint:gosec // test-owned temp path + content, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "new", string(content)) + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o644), info.Mode().Perm()) +} + +func TestWriteFile_RejectsSymlinkWithoutTouchingTarget(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + link := filepath.Join(dir, "link") + require.NoError(t, os.WriteFile(target, []byte("untouched"), 0o600)) + require.NoError(t, os.Symlink(target, link)) + + err := WriteFile(link, []byte("attacker-controlled"), 0o644) + require.Error(t, err) + + //nolint:gosec // test-owned temp path + content, err := os.ReadFile(target) + require.NoError(t, err) + assert.Equal(t, "untouched", string(content)) +} diff --git a/pkg/sharedfile/sharedfile_unsupported.go b/pkg/sharedfile/sharedfile_unsupported.go index 4101ab5fc..c6e718e54 100644 --- a/pkg/sharedfile/sharedfile_unsupported.go +++ b/pkg/sharedfile/sharedfile_unsupported.go @@ -9,6 +9,6 @@ import ( ) // openNoFollow has no symlink-safe open on Windows. -func openNoFollow(string) (*os.File, error) { +func openNoFollow(string, int, os.FileMode) (*os.File, error) { return nil, fmt.Errorf("sharedfile: not supported on %s", runtime.GOOS) } From f1109b43e301ad918fbd2c015f774c7db2d9a154 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 4 Aug 2026 03:08:25 +0000 Subject: [PATCH 28/28] fix(sharedfile): propagate Stat and Close errors from WriteFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit correctly flagged that WriteFile's Stat error was swallowed (statErr == nil && ... short-circuited the mode-check branch entirely on any Stat failure, falling through to a silent nil return) and its Close error was discarded via the deferred _ = f.Close(). A caller could not distinguish "wrote and confirmed the mode" from "wrote, but I have no idea what mode it ended up at" — and since os.File is unbuffered, a rare Close-time failure (some filesystems defer write errors to close) would report success on data that was not actually durable. Stat's error now returns immediately instead of being ignored. Close's error is captured into the named return via the same error-if-not-already-set pattern os.WriteFile itself uses, so an earlier write/stat/chmod error still takes priority over a Close failure. --- pkg/sharedfile/sharedfile.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/pkg/sharedfile/sharedfile.go b/pkg/sharedfile/sharedfile.go index f284d91c7..867ac7019 100644 --- a/pkg/sharedfile/sharedfile.go +++ b/pkg/sharedfile/sharedfile.go @@ -78,16 +78,25 @@ func ReadFile(path string) ([]byte, error) { // FIFO already at path. O_NOFOLLOW still applies when O_CREATE is also // set: an existing symlink is rejected rather than followed, while a // genuinely missing path is created as a fresh regular file. -func WriteFile(path string, data []byte, mode os.FileMode) error { +func WriteFile(path string, data []byte, mode os.FileMode) (err error) { f, err := openNoFollowRegular(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) if err != nil { return err } - defer func() { _ = f.Close() }() + defer func() { + if closeErr := f.Close(); err == nil && closeErr != nil { + err = fmt.Errorf("close %s: %w", path, closeErr) + } + }() + if _, err := f.Write(data); err != nil { return fmt.Errorf("write %s: %w", path, err) } - if info, statErr := f.Stat(); statErr == nil && info.Mode().Perm() != mode.Perm() { + info, err := f.Stat() + if err != nil { + return fmt.Errorf("stat %s: %w", path, err) + } + if info.Mode().Perm() != mode.Perm() { if err := f.Chmod(mode); err != nil { return fmt.Errorf("chmod %s: %w", path, err) }