Security & resilience audit fixes (38 items)

.gitignore

@@ -38,6 +38,6 @@ Thumbs.db
 *.log
 
 # Runner workdirs (local dev)
-runners/
-cache/
-state/
+/runners/
+/cache/
+/state/

README.md

@@ -101,15 +101,22 @@ golangci-lint run          # lint (if installed)
 
 ```
 ghr/
-├── cmd/ghr/main.go            # Entrypoint
+├── cmd/ghr/main.go             # Entrypoint
 ├── internal/
-│   ├── cli/                    # Cobra commands
-│   ├── auth/                   # Credentials management
-│   ├── config/                 # YAML + env config
-│   ├── runner/                 # Binary download & process lifecycle
-│   ├── github/                 # Scale set SDK adapter
-│   ├── model/                  # Shared data structs
-│   └── logging/                # Structured logging
+│   ├── cli/                    # Cobra commands (start/stop/run/status/purge/login/...)
+│   ├── auth/                   # Credentials, JWT signing, installation tokens, breaker
+│   ├── config/                 # YAML + env loading, validation, defaults
+│   ├── controller/             # Scale-set orchestration and per-group scaler
+│   ├── runner/                 # Binary download (with SHA-256 verify) and process lifecycle
+│   ├── github/                 # scaleset SDK adapter
+│   ├── health/                 # Health monitor and check functions
+│   ├── notification/           # Discord + webhook providers with filtering
+│   ├── monitoring/             # Uptime Kuma push reporter
+│   ├── api/                    # Unix-socket JSON API exposing status/health
+│   ├── launchd/                # macOS service install/uninstall via bootstrap/bootout
+│   ├── state/                  # Centralized daemon-state file paths (pid/sock/state)
+│   ├── model/                  # Shared data structs (no logic)
+│   └── logging/                # Structured logging, rotation, tagged runner output
 ├── go.mod
 └── go.sum
 ```

config.example.yaml

@@ -1,6 +1,7 @@
 github:
   url: "https://github.com/my-org"
   runner_group: "default"
+  runner_group_id: 1
 
 runner:
   version: "latest"

internal/api/server.go

@@ -8,10 +8,11 @@ import (
 	"net"
 	"net/http"
 	"os"
-	"path/filepath"
+	"time"
 
 	"github.com/RedBoardDev/gh-runners-tool/v2/internal/health"
 	"github.com/RedBoardDev/gh-runners-tool/v2/internal/model"
+	"github.com/RedBoardDev/gh-runners-tool/v2/internal/state"
 )
 
 type controllerState interface {
@@ -32,7 +33,7 @@ type Server struct {
 
 func NewServer(stateDir string, controller controllerState, healthProvider healthState, logger *slog.Logger) *Server {
 	return &Server{
-		socketPath: filepath.Join(stateDir, "ghr.sock"),
+		socketPath: state.New(stateDir).Socket(),
 		controller: controller,
 		health:     healthProvider,
 		logger:     logger,
@@ -50,6 +51,12 @@ func (s *Server) Run(ctx context.Context) error {
 	}
 	s.listener = ln
 
+	if chmodErr := os.Chmod(s.socketPath, 0o600); chmodErr != nil {
+		ln.Close()
+		_ = os.Remove(s.socketPath)
+		return fmt.Errorf("chmod socket %s: %w", s.socketPath, chmodErr)
+	}
+
 	srv := &http.Server{
 		Handler: s.routes(),
 	}
@@ -59,22 +66,23 @@ func (s *Server) Run(ctx context.Context) error {
 		errCh <- srv.Serve(ln)
 	}()
 
+	defer func() {
+		if cleanupErr := os.Remove(s.socketPath); cleanupErr != nil && !os.IsNotExist(cleanupErr) {
+			s.logger.Warn("failed to remove socket file", "path", s.socketPath, "error", cleanupErr)
+		}
+	}()
+
 	select {
 	case <-ctx.Done():
-		shutdownErr := srv.Close()
-		cleanupErr := os.Remove(s.socketPath)
+		shutdownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second)
+		defer cancel()
+		shutdownErr := srv.Shutdown(shutdownCtx)
+		<-errCh
 		if shutdownErr != nil {
 			return fmt.Errorf("shutdown api server: %w", shutdownErr)
 		}
-		if cleanupErr != nil && !os.IsNotExist(cleanupErr) {
-			s.logger.Warn("failed to remove socket file", "path", s.socketPath, "error", cleanupErr)
-		}
 		return nil
 	case err := <-errCh:
-		cleanupErr := os.Remove(s.socketPath)
-		if cleanupErr != nil && !os.IsNotExist(cleanupErr) {
-			s.logger.Warn("failed to remove socket file", "path", s.socketPath, "error", cleanupErr)
-		}
 		if errors.Is(err, http.ErrServerClosed) {
 			return nil
 		}

internal/api/server_test.go

@@ -1,11 +1,13 @@
 package api
 
 import (
+	"context"
 	"encoding/json"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -191,3 +193,46 @@ func TestHandleStatus_EmptyGroups(t *testing.T) {
 		t.Fatalf("expected 0 groups, got %d", len(body.Groups))
 	}
 }
+
+func TestServer_Run_SocketPermissions(t *testing.T) {
+	// Unix domain sockets on macOS cap at ~104 chars, so avoid t.TempDir() which
+	// returns long /var/folders/... paths. Use a short directory under /tmp.
+	stateDir, err := os.MkdirTemp("/tmp", "ghr-sock-")
+	if err != nil {
+		t.Fatalf("mkdir temp: %v", err)
+	}
+	t.Cleanup(func() { os.RemoveAll(stateDir) })
+
+	srv := NewServer(stateDir, &mockController{}, &mockHealth{},
+		slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError + 1})))
+
+	ctx, cancel := context.WithCancel(context.Background())
+	done := make(chan error, 1)
+	go func() { done <- srv.Run(ctx) }()
+
+	socket := filepath.Join(stateDir, "ghr.sock")
+	deadline := time.Now().Add(2 * time.Second)
+	var info os.FileInfo
+	for time.Now().Before(deadline) {
+		if info, err = os.Stat(socket); err == nil {
+			break
+		}
+		time.Sleep(20 * time.Millisecond)
+	}
+
+	if err != nil {
+		cancel()
+		<-done
+		t.Fatalf("stat socket: %v", err)
+	}
+	mode := info.Mode().Perm()
+
+	cancel()
+	if err := <-done; err != nil {
+		t.Fatalf("server run: %v", err)
+	}
+
+	if mode != 0o600 {
+		t.Fatalf("socket perm = %#o, want 0600", mode)
+	}
+}

internal/auth/auth_test.go

@@ -3,6 +3,7 @@ package auth
 import (
 	"context"
 	"encoding/json"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -230,6 +231,39 @@ func TestSave_And_Load(t *testing.T) {
 	}
 }
 
+func TestLoad_WarnsOnLoosePermissions(t *testing.T) {
+	dir := t.TempDir()
+	credFile := filepath.Join(dir, "credentials.json")
+	t.Setenv("GHR_CREDENTIALS_FILE", credFile)
+	t.Setenv("GITHUB_TOKEN", "")
+
+	creds := &Credentials{Method: "pat", PAT: "ghp_loose", GitHubURL: "https://github.com/x"}
+	if err := Save(creds); err != nil {
+		t.Fatalf("Save: %v", err)
+	}
+	if err := os.Chmod(credFile, 0o644); err != nil {
+		t.Fatalf("chmod: %v", err)
+	}
+
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("pipe: %v", err)
+	}
+	origStderr := os.Stderr
+	os.Stderr = w
+	defer func() { os.Stderr = origStderr }()
+
+	if _, _, loadErr := Load(LoadOpts{}); loadErr != nil {
+		t.Fatalf("Load: %v", loadErr)
+	}
+	w.Close()
+
+	out, _ := io.ReadAll(r)
+	if !strings.Contains(string(out), "warning") || !strings.Contains(string(out), "chmod 600") {
+		t.Errorf("expected loose-perm warning, got: %q", out)
+	}
+}
+
 func TestSave_CreatesDirectory(t *testing.T) {
 	dir := t.TempDir()
 	nestedPath := filepath.Join(dir, "nested", "deep", "credentials.json")

internal/auth/breaker.go

@@ -0,0 +1,81 @@
+package auth
+
+import (
+	"errors"
+	"net/http"
+	"sync"
+	"time"
+)
+
+// ErrCircuitOpen is returned by the circuit breaker while it is tripped.
+var ErrCircuitOpen = errors.New("github API circuit open: too many consecutive failures")
+
+const (
+	breakerFailureThreshold = 5
+	breakerOpenDuration     = 60 * time.Second
+)
+
+type circuitBreaker struct {
+	mu               sync.Mutex
+	consecutiveFails int
+	openedAt         time.Time
+	clock            func() time.Time
+}
+
+func newCircuitBreaker() *circuitBreaker {
+	return &circuitBreaker{clock: time.Now}
+}
+
+func (b *circuitBreaker) allow() bool {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	if b.openedAt.IsZero() {
+		return true
+	}
+	if b.clock().Sub(b.openedAt) >= breakerOpenDuration {
+		// Half-open: allow a single probe.
+		b.openedAt = time.Time{}
+		b.consecutiveFails = breakerFailureThreshold - 1
+		return true
+	}
+	return false
+}
+
+func (b *circuitBreaker) recordSuccess() {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	b.consecutiveFails = 0
+	b.openedAt = time.Time{}
+}
+
+func (b *circuitBreaker) recordFailure() {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	b.consecutiveFails++
+	if b.consecutiveFails >= breakerFailureThreshold && b.openedAt.IsZero() {
+		b.openedAt = b.clock()
+	}
+}
+
+func isBreakable(resp *http.Response, err error) bool {
+	if err != nil {
+		return true
+	}
+	return resp.StatusCode >= 500
+}
+
+var apiBreaker = newCircuitBreaker()
+
+// doGuarded routes the request through the package-level circuit breaker.
+func doGuarded(req *http.Request) (*http.Response, error) {
+	if !apiBreaker.allow() {
+		return nil, ErrCircuitOpen
+	}
+	resp, err := httpClient.Do(req)
+	if isBreakable(resp, err) {
+		apiBreaker.recordFailure()
+	} else {
+		apiBreaker.recordSuccess()
+	}
+	return resp, err
+}

internal/auth/breaker_test.go

@@ -0,0 +1,71 @@
+package auth
+
+import (
+	"errors"
+	"net/http"
+	"testing"
+	"time"
+)
+
+func TestCircuitBreaker_OpensAfterThreshold(t *testing.T) {
+	b := newCircuitBreaker()
+	for i := 0; i < breakerFailureThreshold; i++ {
+		if !b.allow() {
+			t.Fatalf("allow() = false before threshold, iteration %d", i)
+		}
+		b.recordFailure()
+	}
+	if b.allow() {
+		t.Error("allow() = true after threshold, want false")
+	}
+}
+
+func TestCircuitBreaker_SuccessResets(t *testing.T) {
+	b := newCircuitBreaker()
+	for i := 0; i < breakerFailureThreshold-1; i++ {
+		b.recordFailure()
+	}
+	b.recordSuccess()
+	if !b.allow() {
+		t.Error("allow() = false after success reset")
+	}
+	for i := 0; i < breakerFailureThreshold-1; i++ {
+		b.recordFailure()
+	}
+	if !b.allow() {
+		t.Error("allow() = false below threshold")
+	}
+}
+
+func TestCircuitBreaker_HalfOpenAfterTimeout(t *testing.T) {
+	b := newCircuitBreaker()
+	now := time.Now()
+	b.clock = func() time.Time { return now }
+
+	for i := 0; i < breakerFailureThreshold; i++ {
+		b.recordFailure()
+	}
+	if b.allow() {
+		t.Fatal("breaker should be open")
+	}
+
+	now = now.Add(breakerOpenDuration + time.Second)
+	if !b.allow() {
+		t.Error("breaker should allow a probe after open duration")
+	}
+}
+
+func TestIsBreakable(t *testing.T) {
+	if !isBreakable(nil, errors.New("net err")) {
+		t.Error("network error should be breakable")
+	}
+	if !isBreakable(&http.Response{StatusCode: 503}, nil) {
+		t.Error("5xx should be breakable")
+	}
+	if isBreakable(&http.Response{StatusCode: 401}, nil) {
+		t.Error("4xx should not trip the breaker")
+	}
+	if isBreakable(&http.Response{StatusCode: 200}, nil) {
+		t.Error("2xx should not trip the breaker")
+	}
+}