feat: redact credentials in remote urls (#2220)

* feat: redact credentials in remote urls

* chore: improve function naming

* fix: TaskfileNotSecureError should use redacted URI

* feat: unexport all node implementation fields

* fix: unexport HTTPNode.url

task_test.go

@@ -727,6 +727,7 @@ func TestIncludesRemote(t *testing.T) {
 	enableExperimentForTest(t, &experiments.RemoteTaskfiles, 1)
 
 	dir := "testdata/includes_remote"
+	os.RemoveAll(filepath.Join(dir, ".task", "remote"))
 
 	srv := httptest.NewServer(http.FileServer(http.Dir(dir)))
 	defer srv.Close()
@@ -802,8 +803,8 @@ func TestIncludesRemote(t *testing.T) {
 				},
 			}
 
-			for j, e := range executors {
+			for _, e := range executors {
-				t.Run(fmt.Sprint(j), func(t *testing.T) {
+				t.Run(e.name, func(t *testing.T) {
 					require.NoError(t, e.executor.Setup())
 
 					for k, taskCall := range taskCalls {

taskfile/node_base.go

@@ -1,19 +1,19 @@
 package taskfile
 
 type (
-	NodeOption func(*BaseNode)
+	NodeOption func(*baseNode)
-	// BaseNode is a generic node that implements the Parent() methods of the
+	// baseNode is a generic node that implements the Parent() methods of the
 	// NodeReader interface. It does not implement the Read() method and it
 	// designed to be embedded in other node types so that this boilerplate code
 	// does not need to be repeated.
-	BaseNode struct {
+	baseNode struct {
 		parent Node
 		dir    string
 	}
 )
 
-func NewBaseNode(dir string, opts ...NodeOption) *BaseNode {
+func NewBaseNode(dir string, opts ...NodeOption) *baseNode {
-	node := &BaseNode{
+	node := &baseNode{
 		parent: nil,
 		dir:    dir,
 	}
@@ -27,15 +27,15 @@ func NewBaseNode(dir string, opts ...NodeOption) *BaseNode {
 }
 
 func WithParent(parent Node) NodeOption {
-	return func(node *BaseNode) {
+	return func(node *baseNode) {
 		node.parent = parent
 	}
 }
 
-func (node *BaseNode) Parent() Node {
+func (node *baseNode) Parent() Node {
 	return node.parent
 }
 
-func (node *BaseNode) Dir() string {
+func (node *baseNode) Dir() string {
 	return node.dir
 }

taskfile/node_cache.go

@@ -11,13 +11,13 @@ import (
 const remoteCacheDir = "remote"
 
 type CacheNode struct {
-	*BaseNode
+	*baseNode
 	source RemoteNode
 }
 
 func NewCacheNode(source RemoteNode, dir string) *CacheNode {
 	return &CacheNode{
-		BaseNode: &BaseNode{
+		baseNode: &baseNode{
 			dir: filepath.Join(dir, remoteCacheDir),
 		},
 		source: source,

taskfile/node_file.go

@@ -13,8 +13,8 @@ import (
 
 // A FileNode is a node that reads a taskfile from the local filesystem.
 type FileNode struct {
-	*BaseNode
+	*baseNode
-	Entrypoint string
+	entrypoint string
 }
 
 func NewFileNode(entrypoint, dir string, opts ...NodeOption) (*FileNode, error) {
@@ -25,13 +25,13 @@ func NewFileNode(entrypoint, dir string, opts ...NodeOption) (*FileNode, error)
 		return nil, err
 	}
 	return &FileNode{
-		BaseNode:   base,
+		baseNode:   base,
-		Entrypoint: entrypoint,
+		entrypoint: entrypoint,
 	}, nil
 }
 
 func (node *FileNode) Location() string {
-	return node.Entrypoint
+	return node.entrypoint
 }
 
 func (node *FileNode) Read() ([]byte, error) {
@@ -63,7 +63,7 @@ func (node *FileNode) ResolveEntrypoint(entrypoint string) (string, error) {
 
 	// NOTE: Uses the directory of the entrypoint (Taskfile), not the current working directory
 	// This means that files are included relative to one another
-	entrypointDir := filepath.Dir(node.Entrypoint)
+	entrypointDir := filepath.Dir(node.entrypoint)
 	return filepathext.SmartJoin(entrypointDir, path), nil
 }
 
@@ -79,6 +79,6 @@ func (node *FileNode) ResolveDir(dir string) (string, error) {
 
 	// NOTE: Uses the directory of the entrypoint (Taskfile), not the current working directory
 	// This means that files are included relative to one another
-	entrypointDir := filepath.Dir(node.Entrypoint)
+	entrypointDir := filepath.Dir(node.entrypoint)
 	return filepathext.SmartJoin(entrypointDir, path), nil
 }

taskfile/node_git.go

@@ -21,8 +21,8 @@ import (
 
 // An GitNode is a node that reads a Taskfile from a remote location via Git.
 type GitNode struct {
-	*BaseNode
+	*baseNode
-	URL    *url.URL
+	url    *url.URL
 	rawUrl string
 	ref    string
 	path   string
@@ -40,23 +40,20 @@ func NewGitNode(
 		return nil, err
 	}
 
-	basePath, path := func() (string, string) {
+	basePath, path := splitURLOnDoubleSlash(u)
-		x := strings.Split(u.Path, "//")
-		return x[0], x[1]
-	}()
 	ref := u.Query().Get("ref")
 
-	rawUrl := u.String()
+	rawUrl := u.Redacted()
 
 	u.RawQuery = ""
 	u.Path = basePath
 
 	if u.Scheme == "http" && !insecure {
-		return nil, &errors.TaskfileNotSecureError{URI: entrypoint}
+		return nil, &errors.TaskfileNotSecureError{URI: u.Redacted()}
 	}
 	return &GitNode{
-		BaseNode: base,
+		baseNode: base,
-		URL:      u,
+		url:      u,
 		rawUrl:   rawUrl,
 		ref:      ref,
 		path:     path,
@@ -79,7 +76,7 @@ func (node *GitNode) ReadContext(_ context.Context) ([]byte, error) {
 	fs := memfs.New()
 	storer := memory.NewStorage()
 	_, err := git.Clone(storer, fs, &git.CloneOptions{
-		URL:           node.URL.String(),
+		URL:           node.url.String(),
 		ReferenceName: plumbing.ReferenceName(node.ref),
 		SingleBranch:  true,
 		Depth:         1,
@@ -102,7 +99,7 @@ func (node *GitNode) ReadContext(_ context.Context) ([]byte, error) {
 
 func (node *GitNode) ResolveEntrypoint(entrypoint string) (string, error) {
 	dir, _ := filepath.Split(node.path)
-	resolvedEntrypoint := fmt.Sprintf("%s//%s", node.URL, filepath.Join(dir, entrypoint))
+	resolvedEntrypoint := fmt.Sprintf("%s//%s", node.url, filepath.Join(dir, entrypoint))
 	if node.ref != "" {
 		return fmt.Sprintf("%s?ref=%s", resolvedEntrypoint, node.ref), nil
 	}
@@ -127,11 +124,23 @@ func (node *GitNode) ResolveDir(dir string) (string, error) {
 
 func (node *GitNode) CacheKey() string {
 	checksum := strings.TrimRight(checksum([]byte(node.Location())), "=")
-	prefix := filepath.Base(filepath.Dir(node.path))
+	lastDir := filepath.Base(filepath.Dir(node.path))
-	lastDir := filepath.Base(node.path)
+	prefix := filepath.Base(node.path)
 	// Means it's not "", nor "." nor "/", so it's a valid directory
 	if len(lastDir) > 1 {
-		prefix = fmt.Sprintf("%s-%s", lastDir, prefix)
+		prefix = fmt.Sprintf("%s.%s", lastDir, prefix)
+	}
+	return fmt.Sprintf("git.%s.%s.%s", node.url.Host, prefix, checksum)
+}
+
+func splitURLOnDoubleSlash(u *url.URL) (string, string) {
+	x := strings.Split(u.Path, "//")
+	switch len(x) {
+	case 0:
+		return "", ""
+	case 1:
+		return x[0], ""
+	default:
+		return x[0], x[1]
 	}
-	return fmt.Sprintf("%s.%s", prefix, checksum)
 }

taskfile/node_git_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestGitNode_ssh(t *testing.T) {
@@ -13,8 +14,8 @@ func TestGitNode_ssh(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "main", node.ref)
 	assert.Equal(t, "Taskfile.yml", node.path)
-	assert.Equal(t, "ssh://git@github.com/foo/bar.git//Taskfile.yml?ref=main", node.rawUrl)
+	assert.Equal(t, "ssh://git@github.com/foo/bar.git//Taskfile.yml?ref=main", node.Location())
-	assert.Equal(t, "ssh://git@github.com/foo/bar.git", node.URL.String())
+	assert.Equal(t, "ssh://git@github.com/foo/bar.git", node.url.String())
 	entrypoint, err := node.ResolveEntrypoint("common.yml")
 	assert.NoError(t, err)
 	assert.Equal(t, "ssh://git@github.com/foo/bar.git//common.yml?ref=main", entrypoint)
@@ -27,8 +28,8 @@ func TestGitNode_sshWithDir(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "main", node.ref)
 	assert.Equal(t, "directory/Taskfile.yml", node.path)
-	assert.Equal(t, "ssh://git@github.com/foo/bar.git//directory/Taskfile.yml?ref=main", node.rawUrl)
+	assert.Equal(t, "ssh://git@github.com/foo/bar.git//directory/Taskfile.yml?ref=main", node.Location())
-	assert.Equal(t, "ssh://git@github.com/foo/bar.git", node.URL.String())
+	assert.Equal(t, "ssh://git@github.com/foo/bar.git", node.url.String())
 	entrypoint, err := node.ResolveEntrypoint("common.yml")
 	assert.NoError(t, err)
 	assert.Equal(t, "ssh://git@github.com/foo/bar.git//directory/common.yml?ref=main", entrypoint)
@@ -41,8 +42,8 @@ func TestGitNode_https(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "main", node.ref)
 	assert.Equal(t, "Taskfile.yml", node.path)
-	assert.Equal(t, "https://github.com/foo/bar.git//Taskfile.yml?ref=main", node.rawUrl)
+	assert.Equal(t, "https://github.com/foo/bar.git//Taskfile.yml?ref=main", node.Location())
-	assert.Equal(t, "https://github.com/foo/bar.git", node.URL.String())
+	assert.Equal(t, "https://github.com/foo/bar.git", node.url.String())
 	entrypoint, err := node.ResolveEntrypoint("common.yml")
 	assert.NoError(t, err)
 	assert.Equal(t, "https://github.com/foo/bar.git//common.yml?ref=main", entrypoint)
@@ -55,8 +56,8 @@ func TestGitNode_httpsWithDir(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "main", node.ref)
 	assert.Equal(t, "directory/Taskfile.yml", node.path)
-	assert.Equal(t, "https://github.com/foo/bar.git//directory/Taskfile.yml?ref=main", node.rawUrl)
+	assert.Equal(t, "https://github.com/foo/bar.git//directory/Taskfile.yml?ref=main", node.Location())
-	assert.Equal(t, "https://github.com/foo/bar.git", node.URL.String())
+	assert.Equal(t, "https://github.com/foo/bar.git", node.url.String())
 	entrypoint, err := node.ResolveEntrypoint("common.yml")
 	assert.NoError(t, err)
 	assert.Equal(t, "https://github.com/foo/bar.git//directory/common.yml?ref=main", entrypoint)
@@ -65,18 +66,28 @@ func TestGitNode_httpsWithDir(t *testing.T) {
 func TestGitNode_CacheKey(t *testing.T) {
 	t.Parallel()
 
-	node, err := NewGitNode("https://github.com/foo/bar.git//directory/Taskfile.yml?ref=main", "", false)
+	tests := []struct {
-	assert.NoError(t, err)
+		entrypoint  string
-	key := node.CacheKey()
+		expectedKey string
-	assert.Equal(t, "Taskfile.yml-directory.f1ddddac425a538870230a3e38fc0cded4ec5da250797b6cab62c82477718fbb", key)
+	}{
-
+		{
-	node, err = NewGitNode("https://github.com/foo/bar.git//Taskfile.yml?ref=main", "", false)
+			entrypoint:  "https://github.com/foo/bar.git//directory/Taskfile.yml?ref=main",
-	assert.NoError(t, err)
+			expectedKey: "git.github.com.directory.Taskfile.yml.f1ddddac425a538870230a3e38fc0cded4ec5da250797b6cab62c82477718fbb",
-	key = node.CacheKey()
+		},
-	assert.Equal(t, "Taskfile.yml-..39d28c1ff36f973705ae188b991258bbabaffd6d60bcdde9693d157d00d5e3a4", key)
+		{
-
+			entrypoint:  "https://github.com/foo/bar.git//Taskfile.yml?ref=main",
-	node, err = NewGitNode("https://github.com/foo/bar.git//multiple/directory/Taskfile.yml?ref=main", "", false)
+			expectedKey: "git.github.com.Taskfile.yml.39d28c1ff36f973705ae188b991258bbabaffd6d60bcdde9693d157d00d5e3a4",
-	assert.NoError(t, err)
+		},
-	key = node.CacheKey()
+		{
-	assert.Equal(t, "Taskfile.yml-directory.1b6d145e01406dcc6c0aa572e5a5d1333be1ccf2cae96d18296d725d86197d31", key)
+			entrypoint:  "https://github.com/foo/bar.git//multiple/directory/Taskfile.yml?ref=main",
+			expectedKey: "git.github.com.directory.Taskfile.yml.1b6d145e01406dcc6c0aa572e5a5d1333be1ccf2cae96d18296d725d86197d31",
+		},
+	}
+
+	for _, tt := range tests {
+		node, err := NewGitNode(tt.entrypoint, "", false)
+		require.NoError(t, err)
+		key := node.CacheKey()
+		assert.Equal(t, tt.expectedKey, key)
+	}
 }

taskfile/node_http.go

@@ -16,9 +16,8 @@ import (
 
 // An HTTPNode is a node that reads a Taskfile from a remote location via HTTP.
 type HTTPNode struct {
-	*BaseNode
+	*baseNode
-	URL        *url.URL // stores url pointing actual remote file. (e.g. with Taskfile.yml)
+	url *url.URL // stores url pointing actual remote file. (e.g. with Taskfile.yml)
-	entrypoint string   // stores entrypoint url. used for building graph vertices.
 }
 
 func NewHTTPNode(
@@ -33,18 +32,16 @@ func NewHTTPNode(
 		return nil, err
 	}
 	if url.Scheme == "http" && !insecure {
-		return nil, &errors.TaskfileNotSecureError{URI: entrypoint}
+		return nil, &errors.TaskfileNotSecureError{URI: url.Redacted()}
 	}
-
 	return &HTTPNode{
-		BaseNode:   base,
+		baseNode: base,
-		URL:        url,
+		url:      url,
-		entrypoint: entrypoint,
 	}, nil
 }
 
 func (node *HTTPNode) Location() string {
-	return node.entrypoint
+	return node.url.Redacted()
 }
 
 func (node *HTTPNode) Read() ([]byte, error) {
@@ -52,14 +49,13 @@ func (node *HTTPNode) Read() ([]byte, error) {
 }
 
 func (node *HTTPNode) ReadContext(ctx context.Context) ([]byte, error) {
-	url, err := RemoteExists(ctx, node.URL)
+	url, err := RemoteExists(ctx, *node.url)
 	if err != nil {
 		return nil, err
 	}
-	node.URL = url
+	req, err := http.NewRequest("GET", url.String(), nil)
-	req, err := http.NewRequest("GET", node.URL.String(), nil)
 	if err != nil {
-		return nil, errors.TaskfileFetchFailedError{URI: node.URL.String()}
+		return nil, errors.TaskfileFetchFailedError{URI: node.Location()}
 	}
 
 	resp, err := http.DefaultClient.Do(req.WithContext(ctx))
@@ -67,12 +63,12 @@ func (node *HTTPNode) ReadContext(ctx context.Context) ([]byte, error) {
 		if ctx.Err() != nil {
 			return nil, err
 		}
-		return nil, errors.TaskfileFetchFailedError{URI: node.URL.String()}
+		return nil, errors.TaskfileFetchFailedError{URI: node.Location()}
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode != http.StatusOK {
 		return nil, errors.TaskfileFetchFailedError{
-			URI:            node.URL.String(),
+			URI:            node.Location(),
 			HTTPStatusCode: resp.StatusCode,
 		}
 	}
@@ -91,7 +87,7 @@ func (node *HTTPNode) ResolveEntrypoint(entrypoint string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	return node.URL.ResolveReference(ref).String(), nil
+	return node.url.ResolveReference(ref).String(), nil
 }
 
 func (node *HTTPNode) ResolveDir(dir string) (string, error) {
@@ -116,12 +112,12 @@ func (node *HTTPNode) ResolveDir(dir string) (string, error) {
 
 func (node *HTTPNode) CacheKey() string {
 	checksum := strings.TrimRight(checksum([]byte(node.Location())), "=")
-	dir, filename := filepath.Split(node.entrypoint)
+	dir, filename := filepath.Split(node.url.Path)
 	lastDir := filepath.Base(dir)
 	prefix := filename
 	// Means it's not "", nor "." nor "/", so it's a valid directory
 	if len(lastDir) > 1 {
-		prefix = fmt.Sprintf("%s-%s", lastDir, filename)
+		prefix = fmt.Sprintf("%s.%s", lastDir, filename)
 	}
-	return fmt.Sprintf("%s.%s", prefix, checksum)
+	return fmt.Sprintf("http.%s.%s.%s", node.url.Host, prefix, checksum)
 }

taskfile/node_http_test.go

@@ -0,0 +1,49 @@
+package taskfile
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestHTTPNode_CacheKey(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		entrypoint  string
+		expectedKey string
+	}{
+		{
+			entrypoint:  "https://github.com",
+			expectedKey: "http.github.com..996e1f714b08e971ec79e3bea686287e66441f043177999a13dbc546d8fe402a",
+		},
+		{
+			entrypoint:  "https://github.com/Taskfile.yml",
+			expectedKey: "http.github.com.Taskfile.yml.85b3c3ad71b78dc74e404c7b4390fc13672925cb644a4d26c21b9f97c17b5fc0",
+		},
+		{
+			entrypoint:  "https://github.com/foo",
+			expectedKey: "http.github.com.foo.df3158dafc823e6847d9bcaf79328446c4877405e79b100723fa6fd545ed3e2b",
+		},
+		{
+			entrypoint:  "https://github.com/foo/Taskfile.yml",
+			expectedKey: "http.github.com.foo.Taskfile.yml.aea946ea7eb6f6bb4e159e8b840b6b50975927778b2e666df988c03bbf10c4c4",
+		},
+		{
+			entrypoint:  "https://github.com/foo/bar",
+			expectedKey: "http.github.com.foo.bar.d3514ad1d4daedf9cc2825225070b49ebc8db47fa5177951b2a5b9994597570c",
+		},
+		{
+			entrypoint:  "https://github.com/foo/bar/Taskfile.yml",
+			expectedKey: "http.github.com.bar.Taskfile.yml.b9cf01e01e47c0e96ea536e1a8bd7b3a6f6c1f1881bad438990d2bfd4ccd0ac0",
+		},
+	}
+
+	for _, tt := range tests {
+		node, err := NewHTTPNode(tt.entrypoint, "", false)
+		require.NoError(t, err)
+		key := node.CacheKey()
+		assert.Equal(t, tt.expectedKey, key)
+	}
+}

taskfile/node_stdin.go

@@ -12,12 +12,12 @@ import (
 
 // A StdinNode is a node that reads a taskfile from the standard input stream.
 type StdinNode struct {
-	*BaseNode
+	*baseNode
 }
 
 func NewStdinNode(dir string) (*StdinNode, error) {
 	return &StdinNode{
-		BaseNode: NewBaseNode(dir),
+		baseNode: NewBaseNode(dir),
 	}, nil
 }
 

taskfile/taskfile.go

@@ -36,11 +36,11 @@ var (
 // at the given URL with any of the default Taskfile files names. If any of
 // these match a file, the first matching path will be returned. If no files are
 // found, an error will be returned.
-func RemoteExists(ctx context.Context, u *url.URL) (*url.URL, error) {
+func RemoteExists(ctx context.Context, u url.URL) (*url.URL, error) {
 	// Create a new HEAD request for the given URL to check if the resource exists
 	req, err := http.NewRequestWithContext(ctx, "HEAD", u.String(), nil)
 	if err != nil {
-		return nil, errors.TaskfileFetchFailedError{URI: u.String()}
+		return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()}
 	}
 
 	// Request the given URL
@@ -49,7 +49,7 @@ func RemoteExists(ctx context.Context, u *url.URL) (*url.URL, error) {
 		if ctx.Err() != nil {
 			return nil, fmt.Errorf("checking remote file: %w", ctx.Err())
 		}
-		return nil, errors.TaskfileFetchFailedError{URI: u.String()}
+		return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()}
 	}
 	defer resp.Body.Close()
 
@@ -61,7 +61,7 @@ func RemoteExists(ctx context.Context, u *url.URL) (*url.URL, error) {
 	if resp.StatusCode == http.StatusOK && slices.ContainsFunc(allowedContentTypes, func(s string) bool {
 		return strings.Contains(contentType, s)
 	}) {
-		return u, nil
+		return &u, nil
 	}
 
 	// If the request was not successful, append the default Taskfile names to
@@ -78,7 +78,7 @@ func RemoteExists(ctx context.Context, u *url.URL) (*url.URL, error) {
 		// Try the alternative URL
 		resp, err = http.DefaultClient.Do(req)
 		if err != nil {
-			return nil, errors.TaskfileFetchFailedError{URI: u.String()}
+			return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()}
 		}
 		defer resp.Body.Close()
 
@@ -88,5 +88,5 @@ func RemoteExists(ctx context.Context, u *url.URL) (*url.URL, error) {
 		}
 	}
 
-	return nil, errors.TaskfileNotFoundError{URI: u.String(), Walk: false}
+	return nil, errors.TaskfileNotFoundError{URI: u.Redacted(), Walk: false}
 }