From 7382aceb7e09a89b0c91765dc6d011719fbb04b0 Mon Sep 17 00:00:00 2001
From: viettranx <viettranx@gmail.com>
Date: Wed, 25 Mar 2026 08:32:00 +0700
Subject: [PATCH] refactor(pkg-helper): extract hardcoded GID to constant and
 log chown errors

Replace magic number 1000 with goclawGID constant across all chown
calls. Add slog.Warn on chown failures instead of silently ignoring
them, improving debuggability for permission issues in production.
---
 cmd/pkg-helper/main.go | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/cmd/pkg-helper/main.go b/cmd/pkg-helper/main.go
index d367fb9b..198baf01 100644
--- a/cmd/pkg-helper/main.go
+++ b/cmd/pkg-helper/main.go
@@ -21,6 +21,11 @@ import (
 
 const socketPath = "/tmp/pkg.sock"
 
+// goclawGID is the group ID of the goclaw process.
+// Persist files and sockets are chowned to root:goclaw so the
+// unprivileged app process (uid 1000, gid 1000) can read them.
+const goclawGID = 1000
+
 // validPkgName allows alphanumeric, hyphens, underscores, dots, @, / (scoped npm).
 // Rejects names starting with - to prevent argument injection.
 var validPkgName = regexp.MustCompile(`^[a-zA-Z0-9@][a-zA-Z0-9._+\-/@]*$`)
@@ -55,7 +60,7 @@ func main() {
 	// Chown requires CAP_CHOWN; if missing (misconfigured container), warn but continue
 	// since umask already set restrictive permissions.
 	if os.Getuid() == 0 {
-		if err := os.Chown(socketPath, 0, 1000); err != nil {
+		if err := os.Chown(socketPath, 0, goclawGID); err != nil {
 			slog.Warn("pkg-helper: chown socket failed (missing CAP_CHOWN?)", "error", err)
 		}
 	}
@@ -196,7 +201,9 @@ func persistAdd(pkg string) {
 	fmt.Fprintln(f, pkg)
 	// Ensure group ownership allows the goclaw process to read the file.
 	if created {
-		os.Chown(listFile, 0, 1000) //nolint:errcheck
+		if err := os.Chown(listFile, 0, goclawGID); err != nil {
+			slog.Warn("pkg-helper: chown persist file failed", "file", listFile, "error", err)
+		}
 	}
 }
 
@@ -227,10 +234,12 @@ func persistRemove(pkg string) {
 		os.Remove(tmpFile) //nolint:errcheck
 		return
 	}
-	// Restore group ownership so the goclaw process (uid 1000, gid 1000) can read the file.
+	// Restore group ownership so the goclaw process (gid 1000) can read the file.
 	// Without this, the renamed file inherits root:root from the temp file,
 	// causing ListInstalledPackages to return nil for system packages.
-	os.Chown(listFile, 0, 1000) //nolint:errcheck
+	if err := os.Chown(listFile, 0, goclawGID); err != nil {
+		slog.Warn("pkg-helper: chown persist file failed", "file", listFile, "error", err)
+	}
 }
 
 func apkListFile() string {
@@ -258,7 +267,7 @@ func ensurePersistDir() {
 	// Try to fix ownership to root:goclaw (gid 1000) if not already root-owned.
 	// CAP_CHOWN is available even when CAP_DAC_OVERRIDE is dropped.
 	if stat, ok := fi.Sys().(*syscall.Stat_t); ok && stat.Uid != 0 {
-		if err := os.Chown(dir, 0, 1000); err != nil {
+		if err := os.Chown(dir, 0, goclawGID); err != nil {
 			slog.Warn("pkg-helper: cannot fix persist dir ownership", "dir", dir, "error", err)
 		} else {
 			os.Chmod(dir, 0750) //nolint:errcheck