more refactoring of the TaskEnv.ClientPath abstraction

client/allocrunner/taskrunner/getter/getter.go

@@ -1,6 +1,7 @@
 package getter
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -31,7 +32,6 @@ const (
 // is usually satisfied by taskenv.TaskEnv.
 type EnvReplacer interface {
 	ReplaceEnv(string) string
-	ReplaceEnvClient(string) string
 	ClientPath(string) (string, bool)
 }
 
@@ -140,8 +140,7 @@ func GetArtifact(taskEnv EnvReplacer, artifact *structs.TaskArtifact) error {
 	// Verify the destination is still in the task sandbox after interpolation
 	if escapes {
 		return newGetError(artifact.RelativeDest,
-			fmt.Errorf("artifact escapes the alloc directory rawDest=%s dest=%s", artifact.RelativeDest, dest),
-			// errors.New("artifact destination path escapes the alloc directory"),
+			errors.New("artifact destination path escapes the alloc directory"),
 			false)
 	}
 

client/allocrunner/taskrunner/getter/getter_test.go

@@ -26,28 +26,23 @@ type noopReplacer struct {
 	taskDir string
 }
 
-func (noopReplacer) ReplaceEnv(s string) string {
-	return s
-}
-
-func (r noopReplacer) ReplaceEnvClient(s string) string {
-	return r.ReplaceEnv(s)
+func clientPath(taskDir, path string) (string, bool) {
+	if !filepath.IsAbs(path) {
+		path = filepath.Join(taskDir, path)
+	}
+	path = filepath.Clean(path)
+	if taskDir != "" && !helper.PathEscapesSandbox(taskDir, path) {
+		return path, false
+	}
+	return path, true
 }
 
-func checkEscape(taskDir, testPath string) bool {
-	if taskDir != "" && !helper.PathEscapesSandbox(taskDir, testPath) {
-		return false
-	}
-	return true
+func (noopReplacer) ReplaceEnv(s string) string {
+	return s
 }
 
 func (r noopReplacer) ClientPath(p string) (string, bool) {
-	path := r.ReplaceEnv(p)
-	if !filepath.IsAbs(path) {
-		path = filepath.Join(r.taskDir, path)
-	}
-	path = filepath.Clean(path)
-	escapes := checkEscape(r.taskDir, path)
+	path, escapes := clientPath(r.taskDir, r.ReplaceEnv(p))
 	return path, escapes
 }
 
@@ -67,16 +62,8 @@ func (upperReplacer) ReplaceEnv(s string) string {
 	return strings.ToUpper(s)
 }
 
-func (u upperReplacer) ReplaceEnvClient(s string) string {
-	return u.ReplaceEnv(s)
-}
-
 func (u upperReplacer) ClientPath(p string) (string, bool) {
-	path := u.ReplaceEnv(p)
-	if !filepath.IsAbs(path) {
-		path = filepath.Join(u.taskDir, path)
-	}
-	escapes := checkEscape(u.taskDir, path)
+	path, escapes := clientPath(u.taskDir, u.ReplaceEnv(p))
 	return path, escapes
 }
 

client/allocrunner/taskrunner/template/template.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 	"math/rand"
 	"os"
-	"path/filepath"
 	"sort"
 	"strconv"
 	"strings"
@@ -725,10 +724,8 @@ func loadTemplateEnv(tmpls []*structs.Template, taskDir string, taskEnv *taskenv
 			continue
 		}
 
-		dest := taskEnv.ReplaceEnvClient(t.DestPath)
-		if !filepath.IsAbs(dest) {
-			dest = filepath.Join(taskDir, dest)
-		}
+		// we checked escape before we rendered the file
+		dest, _ := taskEnv.ClientPath(t.DestPath)
 		f, err := os.Open(dest)
 		if err != nil {
 			return nil, fmt.Errorf("error opening env template: %v", err)

client/taskenv/env.go

@@ -306,16 +306,18 @@ func (t *TaskEnv) ReplaceEnv(arg string) string {
 	return hargs.ReplaceEnv(arg, t.EnvMap, t.NodeAttrs)
 }
 
-// ReplaceEnvClient takes an arg and replaces all occurrences of client-specific
+// replaceEnvClient takes an arg and replaces all occurrences of client-specific
 // environment variables and Nomad variables.  If the variable is found in the
 // passed map it is replaced, otherwise the original string is returned.