Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion pkg/git/installer.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"os"

"github.com/devsy-org/devsy/pkg/command"
"github.com/devsy-org/devsy/pkg/log"
Expand Down Expand Up @@ -106,7 +107,13 @@ type pkgManagerStrategy struct {

func (s *pkgManagerStrategy) name() string { return s.manager }

func (s *pkgManagerStrategy) usable() bool { return command.Exists(s.manager) }
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).
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)
Expand Down
20 changes: 17 additions & 3 deletions pkg/git/installer_release.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ func (s *releaseSource) install(ctx context.Context, binary string) error {
}
}

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),
Expand All @@ -96,9 +100,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)
Expand Down Expand Up @@ -206,3 +207,16 @@ 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)
}

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)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
36 changes: 36 additions & 0 deletions pkg/git/installer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package git
import (
"context"
"fmt"
"os"
"path/filepath"
"strings"
"testing"

Expand Down Expand Up @@ -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
Expand Down
Loading