From db312e3aa627fd12b7d16c3fbd1dc73b1a451be6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 30 Jul 2026 21:43:09 -0500 Subject: [PATCH 1/3] fix(git): check real write/root capability before installing git-lfs isLocalAgent gates auto-install on "is this a local-docker provider" vs "is this a remote/SSH provider" (#828), but that's not the same as "does devsy have install rights here." A remote SSH machine provider running as an unprivileged user is non-local yet still gets a permission-denied apt/github-release install attempt on every clone, identical to the noise the original fix was meant to remove. pkgManagerStrategy.usable() now also requires root (apt/dpkg need it regardless of host type), and the release strategy checks the install directory is actually writable before downloading anything, so a guaranteed-to-fail install is never attempted rather than attempted and then failing loudly. --- pkg/git/installer.go | 12 +++++++++++- pkg/git/installer_release.go | 26 +++++++++++++++++++++++--- pkg/git/installer_test.go | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/pkg/git/installer.go b/pkg/git/installer.go index 015ca4dec..e966d35bc 100644 --- a/pkg/git/installer.go +++ b/pkg/git/installer.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "os" "github.com/devsy-org/devsy/pkg/command" "github.com/devsy-org/devsy/pkg/log" @@ -106,7 +107,16 @@ type pkgManagerStrategy struct { func (s *pkgManagerStrategy) name() string { return s.manager } -func (s *pkgManagerStrategy) usable() bool { return command.Exists(s.manager) } +// isRoot is overridable in tests. +var isRoot = func() bool { return os.Geteuid() == 0 } + +// usable requires root: package installs write to system-owned locations +// (e.g. apt's lock files, dpkg's database) regardless of whether the host +// is "local" or devsy-provisioned, so checking privilege directly avoids an +// install attempt that's guaranteed to fail with permission denied. +func (s *pkgManagerStrategy) usable() bool { + return command.Exists(s.manager) && isRoot() +} func (s *pkgManagerStrategy) install(ctx context.Context, t tool) error { log.Infof("installing %s with %s", t.pkg, s.manager) diff --git a/pkg/git/installer_release.go b/pkg/git/installer_release.go index f0e331b3e..f3cb3c0fc 100644 --- a/pkg/git/installer_release.go +++ b/pkg/git/installer_release.go @@ -84,6 +84,13 @@ func (s *releaseSource) install(ctx context.Context, binary string) error { } } + // Check writability before downloading: a permission error here is + // guaranteed regardless of host/provider type, so there's no point + // spending a network round trip to discover it after the fact. + if err := ensureDirWritable(installDir); err != nil { + return fmt.Errorf("install dir %q is not writable: %w", installDir, err) + } + req := fetchRequest{ binary: binary, url: s.downloadURL(s.version, asset), @@ -96,9 +103,6 @@ func (s *releaseSource) install(ctx context.Context, binary string) error { } defer cleanup() - if err := os.MkdirAll(installDir, 0o750); err != nil { - return fmt.Errorf("create install dir %q: %w", installDir, err) - } dst := filepath.Join(installDir, req.execName) if err := moveExecutable(src, dst); err != nil { return fmt.Errorf("install %s to %q: %w", binary, dst, err) @@ -206,3 +210,19 @@ func moveExecutable(src, dst string) error { // #nosec G306,G703 -- dst is internally constructed; an executable must be world-executable return os.WriteFile(dst, data, 0o755) } + +// ensureDirWritable creates dir if needed and confirms it's actually +// writable by the current process, without relying on any host/provider +// heuristic. +func ensureDirWritable(dir string) error { + if err := os.MkdirAll(dir, 0o750); err != nil { + return err + } + probe, err := os.CreateTemp(dir, ".devsy-write-test-*") + if err != nil { + return err + } + name := probe.Name() + _ = probe.Close() + return os.Remove(name) +} diff --git a/pkg/git/installer_test.go b/pkg/git/installer_test.go index 6ea52a548..390afe8ba 100644 --- a/pkg/git/installer_test.go +++ b/pkg/git/installer_test.go @@ -3,6 +3,8 @@ package git import ( "context" "fmt" + "os" + "path/filepath" "strings" "testing" @@ -48,6 +50,40 @@ func TestReleaseStrategyRequiresReleaseSource(t *testing.T) { assert.Assert(t, err != nil) } +func TestPkgManagerStrategyUsableRequiresRoot(t *testing.T) { + s := &pkgManagerStrategy{manager: "sh"} // "sh" always exists in test environments + + original := isRoot + t.Cleanup(func() { isRoot = original }) + + isRoot = func() bool { return true } + assert.Assert(t, s.usable()) + + isRoot = func() bool { return false } + assert.Assert(t, !s.usable()) +} + +func TestEnsureDirWritable(t *testing.T) { + assert.NilError(t, ensureDirWritable(t.TempDir())) +} + +func TestEnsureDirWritableCreatesMissingDir(t *testing.T) { + dir := filepath.Join(t.TempDir(), "does", "not", "exist", "yet") + assert.NilError(t, ensureDirWritable(dir)) +} + +func TestEnsureDirWritableRejectsReadOnlyDir(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("permission test not meaningful when running as root") + } + + dir := t.TempDir() + // #nosec G302 -- intentional: testing restrictive perms + assert.NilError(t, os.Chmod(dir, 0o500)) + + assert.Assert(t, ensureDirWritable(dir) != nil) +} + // fakeStrategy is a test installStrategy with configurable behavior. type fakeStrategy struct { label string From c970e9e5a55e5f86c62ef0609dff4f2695924291 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 30 Jul 2026 22:28:42 -0500 Subject: [PATCH 2/3] style: cleanup comments --- pkg/git/installer.go | 5 +---- pkg/git/installer_release.go | 6 ------ 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/pkg/git/installer.go b/pkg/git/installer.go index e966d35bc..aded2ac65 100644 --- a/pkg/git/installer.go +++ b/pkg/git/installer.go @@ -107,13 +107,10 @@ type pkgManagerStrategy struct { func (s *pkgManagerStrategy) name() string { return s.manager } -// isRoot is overridable in tests. var isRoot = func() bool { return os.Geteuid() == 0 } // usable requires root: package installs write to system-owned locations -// (e.g. apt's lock files, dpkg's database) regardless of whether the host -// is "local" or devsy-provisioned, so checking privilege directly avoids an -// install attempt that's guaranteed to fail with permission denied. +// (e.g. apt's lock files, dpkg's database). func (s *pkgManagerStrategy) usable() bool { return command.Exists(s.manager) && isRoot() } diff --git a/pkg/git/installer_release.go b/pkg/git/installer_release.go index f3cb3c0fc..cf5787759 100644 --- a/pkg/git/installer_release.go +++ b/pkg/git/installer_release.go @@ -84,9 +84,6 @@ func (s *releaseSource) install(ctx context.Context, binary string) error { } } - // Check writability before downloading: a permission error here is - // guaranteed regardless of host/provider type, so there's no point - // spending a network round trip to discover it after the fact. if err := ensureDirWritable(installDir); err != nil { return fmt.Errorf("install dir %q is not writable: %w", installDir, err) } @@ -211,9 +208,6 @@ func moveExecutable(src, dst string) error { return os.WriteFile(dst, data, 0o755) } -// ensureDirWritable creates dir if needed and confirms it's actually -// writable by the current process, without relying on any host/provider -// heuristic. func ensureDirWritable(dir string) error { if err := os.MkdirAll(dir, 0o750); err != nil { return err From 9992ae99ac0b6fe92e6ebf46cfba5eecf93e9290 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 30 Jul 2026 23:14:19 -0500 Subject: [PATCH 3/3] fix(git): return probe.Close() error in ensureDirWritable A failed close (e.g. deferred write errors on some filesystems) was silently discarded, letting a doomed writability check report success. --- pkg/git/installer_release.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/git/installer_release.go b/pkg/git/installer_release.go index cf5787759..f1bbab1a2 100644 --- a/pkg/git/installer_release.go +++ b/pkg/git/installer_release.go @@ -217,6 +217,9 @@ func ensureDirWritable(dir string) error { return err } name := probe.Name() - _ = probe.Close() + if err := probe.Close(); err != nil { + _ = os.Remove(name) + return err + } return os.Remove(name) }