Merge pull request #1254 from tgross/f-file-sandboxing-redux

file sandbox: check but don't alter paths passed to 'file'

README.md

@@ -500,8 +500,8 @@ template {
   function_blacklist = []
 
   # If a sandbox path is provided, any path provided to the `file` function is
-  # prefixed by the sandbox path. Relative paths that try to traverse outside
-  # that prefix will exit with an error.
+  # checked that it falls within the sandbox path. Relative paths that try to
+  # traverse outside the sandbox path will exit with an error.
   sandbox_path = ""
 
   # This is the `minimum(:maximum)` to wait before rendering a new template to

template/funcs.go

@@ -104,7 +104,7 @@ func fileFunc(b *Brain, used, missing *dep.Set, sandboxPath string) func(string)
 		if len(s) == 0 {
 			return "", nil
 		}
-		s, err := sandboxedPath(sandboxPath, s)
+		err := pathInSandbox(sandboxPath, s)
 		if err != nil {
 			return "", err
 		}
@@ -1166,26 +1166,21 @@ func blacklisted(...string) (string, error) {
 	return "", errors.New("function is disabled")
 }
 
-// sandboxedPath returns a path that's been prefixed by the sandbox path,
-// if any. If a sandbox path was provided, it will return an error if the
-// path falls outside the sandbox.
-func sandboxedPath(sandbox, s string) (string, error) {
-	path, err := filepath.Abs(filepath.Join(sandbox, s))
-	if err != nil {
-		return "", err
-	}
+// pathInSandbox returns an error if the provided path doesn't fall within the
+// sandbox or if the file can't be evaluated (missing, invalid symlink, etc.)
+func pathInSandbox(sandbox, path string) error {
 	if sandbox != "" {
-		path, err := filepath.EvalSymlinks(path)
+		s, err := filepath.EvalSymlinks(path)
 		if err != nil {
-			return "", err
+			return err
 		}
-		path, err = filepath.Rel(sandbox, path)
+		s, err = filepath.Rel(sandbox, s)
 		if err != nil {
-			return "", err
+			return err
 		}
-		if strings.HasPrefix(path, "..") {
-			return "", fmt.Errorf("'%s' is outside of sandbox", s)
+		if strings.HasPrefix(s, "..") {
+			return fmt.Errorf("'%s' is outside of sandbox", path)
 		}
 	}
-	return path, nil
+	return nil
 }

template/funcs_test.go

@@ -2,91 +2,77 @@ package template
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
 	"runtime"
 	"testing"
-
-	"github.com/pkg/errors"
 )
 
 func TestFileSandbox(t *testing.T) {
 	// while most of the function can be tested lexigraphically,
 	// we need to be able to walk actual symlinks.
 	_, filename, _, _ := runtime.Caller(0)
 	sandboxDir := filepath.Join(filepath.Dir(filename), "testdata", "sandbox")
-	cwd, _ := os.Getwd()
 	cases := []struct {
-		name         string
-		sandbox      string
-		path         string
-		expectedPath string
-		expectedErr  error
+		name     string
+		sandbox  string
+		path     string
+		expected error
 	}{
 		{
 			"absolute_path_no_sandbox",
 			"",
 			"/path/to/file",
-			"/path/to/file",
 			nil,
 		},
 		{
 			"relative_path_no_sandbox",
 			"",
 			"./path/to/file",
-			filepath.Join(cwd, "path/to/file"),
 			nil,
 		},
 		{
 			"absolute_path_with_sandbox",
 			sandboxDir,
-			"/path/to/file",
 			filepath.Join(sandboxDir, "path/to/file"),
 			nil,
 		},
 		{
 			"relative_path_in_sandbox",
 			sandboxDir,
-			"./path/to/file",
-			filepath.Join(sandboxDir, "path/to/file"),
+			filepath.Join(sandboxDir, "path/to/../to/file"),
 			nil,
 		},
 		{
 			"symlink_path_in_sandbox",
 			sandboxDir,
-			"./path/to/ok-symlink",
 			filepath.Join(sandboxDir, "path/to/ok-symlink"),
 			nil,
 		},
 		{
 			"relative_path_escaping_sandbox",
 			sandboxDir,
-			"/path/../../../funcs_test.go",
-			"",
-			errors.New("'/path/../../../funcs_test.go' is outside of sandbox"),
+			filepath.Join(sandboxDir, "path/../../../funcs_test.go"),
+			fmt.Errorf("'%s' is outside of sandbox",
+				filepath.Join(sandboxDir, "path/../../../funcs_test.go")),
 		},
 		{
 			"symlink_escaping_sandbox",
 			sandboxDir,
-			"/path/to/bad-symlink",
-			"",
-			errors.New("'/path/to/bad-symlink' is outside of sandbox"),
+			filepath.Join(sandboxDir, "path/to/bad-symlink"),
+			fmt.Errorf("'%s' is outside of sandbox",
+				filepath.Join(sandboxDir, "path/to/bad-symlink")),
 		},
 	}
 
 	for i, tc := range cases {
 		t.Run(fmt.Sprintf("%d_%s", i, tc.name), func(t *testing.T) {
-			result, err := sandboxedPath(tc.sandbox, tc.path)
-			if tc.expectedErr != nil {
-				if err == nil {
-					t.Fatalf("expected error %s got nil", tc.expectedErr)
+			err := pathInSandbox(tc.sandbox, tc.path)
+			if err != nil && tc.expected != nil {
+				if err.Error() != tc.expected.Error() {
+					t.Fatalf("expected %v got %v", tc.expected, err)
 				}
-				if err.Error() != tc.expectedErr.Error() {
-					t.Fatalf("expected %s got %s", tc.expectedErr, err)
-				}
-			}
-			if result != tc.expectedPath {
-				t.Fatalf("expected %s got %s", tc.expectedPath, result)
+			} else if err != tc.expected {
+				t.Fatalf("expected %v got %v", tc.expected, err)
 			}
 		})
 	}