Merge pull request #2925 from buildkite/fix-hook-tempdir-perms

Write hooks into new tempdir

Author: DrJosh9000

internal/job/hook/wrapper.go

@@ -121,6 +121,7 @@ type WrapperOpt func(*Wrapper)
 type Wrapper struct {
 	hookPath      string
 	os            string
+	tempDir       string
 	wrapperPath   string
 	beforeEnvPath string
 	afterEnvPath  string
@@ -202,19 +203,33 @@ func NewWrapper(opts ...WrapperOpt) (*Wrapper, error) {
 		templateType = PosixShellTemplateType
 	}
 
-	if wrap.beforeEnvPath, err = tempfile.NewClosed(
-		tempfile.WithDir(hookWrapperDir),
+	// On systems where multiple buildkite-agents are running under different
+	// users, a shared path could be owned by a different user.
+	// Creating a new temporary directory to hold the temporary files avoids
+	// this issue and makes cleanup easier.
+	tempDir, err := os.MkdirTemp(os.TempDir(), hookWrapperDir)
+	if err != nil {
+		return nil, fmt.Errorf("creating temporary directory for hook wrapper: %w", err)
+	}
+	wrap.tempDir = tempDir
+
+	beforeEnvPath, err := tempfile.NewClosed(
+		tempfile.WithDir(tempDir),
 		tempfile.WithName("hook-before-env"),
-	); err != nil {
+	)
+	if err != nil {
 		return nil, err
 	}
+	wrap.beforeEnvPath = beforeEnvPath
 
-	if wrap.afterEnvPath, err = tempfile.NewClosed(
-		tempfile.WithDir(hookWrapperDir),
+	afterEnvPath, err := tempfile.NewClosed(
+		tempfile.WithDir(tempDir),
 		tempfile.WithName("hook-after-env"),
-	); err != nil {
+	)
+	if err != nil {
 		return nil, err
 	}
+	wrap.afterEnvPath = afterEnvPath
 
 	absolutePathToHook, err := filepath.Abs(wrap.hookPath)
 	if err != nil {
@@ -234,13 +249,16 @@ func NewWrapper(opts ...WrapperOpt) (*Wrapper, error) {
 		PathToHook:        absolutePathToHook,
 	}
 
-	if wrap.wrapperPath, err = WriteHookWrapper(
+	wrapperPath, err := WriteHookWrapper(
 		templateType,
 		templateInput,
+		tempDir,
 		scriptFileName,
-	); err != nil {
+	)
+	if err != nil {
 		return nil, err
 	}
+	wrap.wrapperPath = wrapperPath
 
 	return wrap, nil
 }
@@ -251,10 +269,11 @@ func NewWrapper(opts ...WrapperOpt) (*Wrapper, error) {
 func WriteHookWrapper(
 	templateType TemplateType,
 	input WrapperTemplateInput,
+	dir string,
 	hookWrapperName string,
 ) (string, error) {
 	f, err := tempfile.New(
-		tempfile.WithDir(hookWrapperDir),
+		tempfile.WithDir(dir),
 		tempfile.WithName(hookWrapperName),
 		tempfile.KeepingExtension(),
 		tempfile.WithPerms(0o700),
@@ -291,9 +310,7 @@ func (w *Wrapper) Path() string {
 // Close cleans up the wrapper script and the environment files. Ignores errors, in
 // particular the error from os.Remove if the file doesn't exist.
 func (w *Wrapper) Close() {
-	_ = os.Remove(w.wrapperPath)
-	_ = os.Remove(w.beforeEnvPath)
-	_ = os.Remove(w.afterEnvPath)
+	_ = os.RemoveAll(w.tempDir)
 }
 
 // Changes returns the changes in the environment and working dir after the hook script runs

internal/tempfile/tempfile.go

@@ -26,7 +26,9 @@ func WithName(filename string) Opts {
 	}
 }
 
-// WithDir sets the subdirectory of the temporary file within the system temp directory.
+// WithDir sets the subdirectory to contain the temporary file. If dir is
+// absolute, it will be used as-is, otherwise it will be taken relative to the
+// system temporary directory ([os.TempDir]).
 func WithDir(dir string) Opts {
 	return func(tf *request) {
 		tf.dir = dir
@@ -56,14 +58,18 @@ func New(opts ...Opts) (*os.File, error) {
 		opt(req)
 	}
 
-	tempDir := os.TempDir()
+	dir := os.TempDir()
 	if req.dir != "" {
-		tempDir = filepath.Join(tempDir, req.dir)
+		if filepath.IsAbs(req.dir) {
+			dir = filepath.Clean(req.dir)
+		} else {
+			dir = filepath.Join(dir, req.dir)
+		}
 	}
 
 	// umask will make perms more reasonable
-	if err := os.MkdirAll(tempDir, allRWX); err != nil {
-		return nil, fmt.Errorf("failed to create temporary directory %q: %w", tempDir, err)
+	if err := os.MkdirAll(dir, allRWX); err != nil {
+		return nil, fmt.Errorf("failed to create temporary directory %q: %w", dir, err)
 	}
 
 	tempFileName := req.filename
@@ -73,7 +79,7 @@ func New(opts ...Opts) (*os.File, error) {
 		tempFileName = basename + "-*" + extension
 	}
 
-	tempFile, err := os.CreateTemp(tempDir, tempFileName)
+	tempFile, err := os.CreateTemp(dir, tempFileName)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create temporary file %q: %w", req.filename, err)
 	}