fix(gpg): resolve tmp file collision breaking GPG forwarding for browser IDEs

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)
 		}
 	}
 

cmd/internal/agentworkspace/setup_gpg.go

@@ -13,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"
 )
@@ -97,13 +98,26 @@ func (cmd *SetupGPGCmd) Run(ctx context.Context) error {
 	return nil
 }
 
+// gpgSetupLockMode is 0666 — flock's default 0600 would lock out whichever
+// of root/remoteUser did not create the file.
+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()
 
-	lock := flock.New(gpgSetupLockPath)
+	// 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,
+	); err != nil {
+		return nil, fmt.Errorf("widen stale lock file: %w", err)
+	}
+
+	lock := flock.New(gpgSetupLockPath, flock.SetPermissions(gpgSetupLockMode))
 	locked, err := lock.TryLockContext(lockCtx, 200*time.Millisecond)
 	if err != nil {
 		if ctx.Err() != nil {
@@ -121,6 +135,13 @@ func acquireGPGSetupLock(ctx context.Context) (func(), error) {
 		return nil, fmt.Errorf("timed out waiting for another gpg setup to finish")
 	}
 
+	// 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("set lock file permissions: %w", err)
+	}
+
 	return func() { _ = lock.Unlock() }, nil
 }
 

cmd/internal/agentworkspace/setup_gpg_test.go

@@ -3,6 +3,7 @@ package agentworkspace
 import (
 	"context"
 	"errors"
+	"os"
 	"path/filepath"
 	"testing"
 	"time"
@@ -101,3 +102,102 @@ 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")
+}
+
+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())
+
+	reacquire, err := acquireGPGSetupLock(context.Background())
+	require.NoError(t, err, "second acquisition of an already-0666 lock file must succeed")
+	reacquire()
+}
+
+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))
+
+	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 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())
+}
+
+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,
+		"opening the symlinked lock path with O_NOFOLLOW must fail, not follow it")
+
+	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")
+}

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
@@ -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

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
 }

cmd/internal/ssh_server.go

@@ -5,14 +5,14 @@ import (
 	"encoding/base64"
 	"errors"
 	"fmt"
-	"io/fs"
 	"os"
 	"time"
 
 	"github.com/devsy-org/devsy/cmd/flags"
 	"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,18 @@ 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 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)
+		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
 }

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,58 @@ 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(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")
+}
+
+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")
+
+	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) {
+	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")
+}

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 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
+// <path>` 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 <path> <mode>",
+		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))
+		},
+	}
+}